server icon indicating copy to clipboard operation
server copied to clipboard

fix: update re-share if shared-by user has been revoked

Open luka-nextcloud opened this issue 1 year ago • 1 comments

  • Resolves: #42407

Summary

TODO

  • [ ] ...

Checklist

luka-nextcloud avatar Jan 22 '24 14:01 luka-nextcloud

Another effected case by this issue:

  1. UserA share Folder1 to UserB
  2. UserB share Folder1 to UserC
  3. UserA revoke UserB
  4. UserA try to update sharing permission of UserC -> Error: Could not get proper share mount for <NODE ID>. Failing since else the next calls are called with null

luka-nextcloud avatar Jan 29 '24 14:01 luka-nextcloud

@artonge I've updated as you requested, please check again.

luka-nextcloud avatar Feb 05 '24 18:02 luka-nextcloud

Can you add comments to clarify what each if group does?

I am also wondering if we could find this shares with the following query:

SELECT
	f.fileid,
	f.path,
	f.storage,
	s.id as share_id,
	s.uid_owner as share_owner,
	s.uid_initiator as share_initiator,
	s.share_with as share_recipient
FROM
	oc_filecache f
	JOIN oc_share s ON f.fileid = s.file_source
	AND s.uid_initiator NOT IN (
		SELECT
			user_id
		FROM
			oc_mounts m
		WHERE
			f.storage = m.storage_id
	)

If so, then we could have a background job to remove them every hour or so like https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/DeleteOrphanedSharesJob.php, which might be easier than the current solution.

@artonge It only works if the shared-by user refreshes his files list after his share has revoked. So, I don't think this query would do the job.

luka-nextcloud avatar Feb 06 '24 19:02 luka-nextcloud

Tests are failing

come-nc avatar Jun 10 '24 13:06 come-nc

@come-nc Could you please review again? All checks have passed now.

luka-nextcloud avatar Jun 17 '24 18:06 luka-nextcloud

@luka-nextcloud After talking with Robin, actually the ownership transfer is supposed to update the owner of the share so there should be no problem in the first place?

come-nc avatar Jun 24 '24 15:06 come-nc

@come-nc Does it mean that these changes are enough for this issue?

image

image

luka-nextcloud avatar Jun 24 '24 17:06 luka-nextcloud

@come-nc Does it mean that these changes are enough for this issue?

image

image

No, the second diff looks like it will cause trouble by arbitrary changing sharedBy of shares.

I lost track of what you are trying to fix, can you explain how to reproduce your problem? Why do you remove the exception handling removing invalid shares in OwnershipTransferService?

come-nc avatar Jun 25 '24 08:06 come-nc

@come-nc

  • Reproduce steps:
  1. UserA share Folder1 to UserB
  2. UserB share Folder1 to UserC
  3. UserA deleted sharing to UserB
  4. UserA transfer Folder1 to UserD -> UserC lost access to Folder1. When UserC open sharing tab of Folder1, the error message Error: Could not get proper share mount for . Failing since else the next calls are called with null returned.
  • Issue occurred because the sharing of UserB to UserC was not deleted or updated after UserA deleted sharing to UserB. Then, when UserA transfering Folder1 to UserD, that re-share record cannot be transferred because it throws GenericShareException error.

luka-nextcloud avatar Jun 25 '24 14:06 luka-nextcloud

@come-nc

  • Reproduce steps:
  1. UserA share Folder1 to UserB
  2. UserB share Folder1 to UserC
  3. UserA deleted sharing to UserB
  4. UserA transfer Folder1 to UserD -> UserC lost access to Folder1. When UserC open sharing tab of Folder1, the error message Error: Could not get proper share mount for . Failing since else the next calls are called with null returned.
  • Issue occurred because the sharing of UserB to UserC was not deleted or updated after UserA deleted sharing to UserB. Then, when UserA transfering Folder1 to UserD, that re-share record cannot be transferred because it throws GenericShareException error.

UserC already loses access to Folder1 after step 3, right? At that point the sharing tab shows no error?

come-nc avatar Jun 27 '24 12:06 come-nc

@come-nc

UserC already loses access to Folder1 after step 3, right?

No, UserC didn't lose access to Folder1 after step3. This is the issue we are trying to fix.

At that point the sharing tab shows no error?

No error.

luka-nextcloud avatar Jun 27 '24 14:06 luka-nextcloud

@come-nc Original problem: Re-share was not deleted when parent share deleted. Then, it caused error while transferring ownership.

Reproduce steps: Mentioned above.

Solution: Delete the re-share and its children when parent share is deleted.

luka-nextcloud avatar Jul 10 '24 08:07 luka-nextcloud