Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: promote re-shares when deleting the parent share #47425

Merged
merged 10 commits into from
Nov 4, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Aug 22, 2024

  • Resolves: #

Summary

Same as #43025 but without the new command.

  • Catch share exception in transfer ownership and delete invalid shares
  • Promote reshares into direct shares when a user loses access to a file after an unshare

Checklist

@come-nc come-nc added the 2. developing Work in progress label Aug 22, 2024
@come-nc come-nc self-assigned this Aug 22, 2024
@come-nc come-nc force-pushed the fix/avoid-invalid-share-on-transfer-ownership branch from 88e1618 to 941934a Compare August 22, 2024 13:59
@come-nc come-nc requested review from icewind1991, a team, Altahrim and yemkareems and removed request for a team August 22, 2024 15:58
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 22, 2024
@come-nc come-nc marked this pull request as ready for review August 22, 2024 15:58
luka-nextcloud
luka-nextcloud previously approved these changes Aug 23, 2024
sorbaugh
sorbaugh previously approved these changes Aug 26, 2024
@come-nc
Copy link
Contributor Author

come-nc commented Aug 26, 2024

The failing tests are related.

The test failing is testing this scenario:

Scenario: transferring ownership of folder reshared with another user
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And user "user3" exists
And User "user3" created a folder "/test"
And User "user3" uploads file "data/textfile.txt" to "/test/somefile.txt"
And folder "/test" of user "user3" is shared with user "user0" with permissions 31
And user "user0" accepts last share
And folder "/test" of user "user0" is shared with user "user2" with permissions 31
And user "user2" accepts last share
When transferring ownership from "user0" to "user1"
And the command was successful
And As an "user2"
Then Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"
And using old dav path
And as "user0" the folder "/test" exists
And using received transfer folder of "user1" as dav path
And as "user1" the folder "/test" does not exist
And As an "user0"
And Getting info of last share
And the OCS status code should be "100"
And Share fields of last share match with
| uid_owner | user0 |
| uid_file_owner | user3 |
| share_with | user2 |

It fails at the step in bold.
The problem here is that we ask the command to transfer a reshare from user0 to user1 without transferring the incoming share. That would result in a situation where user1 shares the folder without having access to it. The test actually expects it to fail, it checks that user1 does not see the folder.
But with the new code, the share is seen as invalid and is deleted. While the old code would let it in place.

If I reorder the transferring code and use --transfer-incoming-shares=1 then I can successfully transfer the share.

The more I think about it the more it feels like it’s not the job of transfer-ownership to delete broken shares.
The fix in the share manager is enough to avoid share breakage on share deletion.
What would remain to fix is to behave better when encountering an already broken share.
From original bug report I see 2 occurences listed:
#43025 (comment)
#43025 (comment)

@come-nc
Copy link
Contributor Author

come-nc commented Sep 5, 2024

After internal discussion, reshares should not be deleted when share is deleted but turned into direct shares instead.
So something more along the line of what the repair share proposition from Luka did.

We should still be careful about what that means for transfer-ownership when incomming shares are not transfered.

@come-nc come-nc force-pushed the fix/avoid-invalid-share-on-transfer-ownership branch from 56e6eff to 1527836 Compare September 13, 2024 07:33
@come-nc come-nc changed the title fix: delete re-shares when deleting the parent share fix: promote re-shares when deleting the parent share Sep 13, 2024
@susnux
Copy link
Contributor

susnux commented Sep 14, 2024

To make sure I understand this correct:

  1. A shares F with B
  2. B re-shares F with C
  3. A deletes share of F with B
  4. B -> C share of F is converted to a share A -> C of F?

I think there was a discussion recently with @jancborchardt if this is expected behavior.
If I remember correctly if the original sharer now has control over the shares this should be fine, but C should not have access to F after B is removed without that A can control this.

@come-nc
Copy link
Contributor Author

come-nc commented Sep 15, 2024

To make sure I understand this correct:

  1. A shares F with B
  2. B re-shares F with C
  3. A deletes share of F with B
  4. B -> C share of F is converted to a share A -> C of F?

Yes that is correct

I think there was a discussion recently with @jancborchardt if this is expected behavior. If I remember correctly if the original sharer now has control over the shares this should be fine, but C should not have access to F after B is removed without that A can control this.

Before deleting the share to B, A sees shares both to B and C in the UI. There is no visible distinction between the share and the reshare.
After deleting the share to B, the share to C is untouched in the UI, so A knows that he need to delete it if needed. As it’s now a direct share he can also edit the permissions on it.

@come-nc come-nc dismissed stale reviews from sorbaugh and luka-nextcloud September 15, 2024 14:13

Obsolete

@come-nc come-nc force-pushed the fix/avoid-invalid-share-on-transfer-ownership branch from 7c3f054 to f389c6f Compare September 16, 2024 16:15
@come-nc come-nc force-pushed the fix/avoid-invalid-share-on-transfer-ownership branch from 9968018 to 8aeedbc Compare September 23, 2024 08:55
@come-nc come-nc force-pushed the fix/avoid-invalid-share-on-transfer-ownership branch from 8aeedbc to 8974542 Compare October 3, 2024 07:57
@come-nc
Copy link
Contributor Author

come-nc commented Oct 3, 2024

Rebased and fixed code style.

luka-nextcloud and others added 9 commits October 14, 2024 11:58
Note: Removed part about fix command from original PR

Signed-off-by: Luka Trovic <[email protected]>
Signed-off-by: Côme Chilliet <[email protected]>
Canceling the previous add of deletion of invalid shares in
 transferownership because in some cases it deletes valid reshares, if
 incoming shares are not transfered on purpose.
Inverting the order of transfer between incoming and outgoing so that
 reshare can be migrated when incoming shares are transfered.

Signed-off-by: Côme Chilliet <[email protected]>
Co-authored-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the fix/avoid-invalid-share-on-transfer-ownership branch from ab967de to e584e9b Compare October 14, 2024 09:58
@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 17, 2024
@come-nc come-nc added this to the Nextcloud 31 milestone Oct 17, 2024
@come-nc
Copy link
Contributor Author

come-nc commented Oct 17, 2024

I’m not sure if cypress failures can be related or not. Should be double check as sharing sidebar is on the failure screenshot.

@susnux
Copy link
Contributor

susnux commented Oct 17, 2024

I’m not sure if cypress failures can be related or not. Should be double check as sharing sidebar is on the failure screenshot.

Never saw that failing but is is 99,99% unrelated because what is tested is purely frontend behavior and unrelated to your changes

@come-nc come-nc merged commit 9014962 into master Nov 4, 2024
177 checks passed
@come-nc come-nc deleted the fix/avoid-invalid-share-on-transfer-ownership branch November 4, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants