nextcloud
fix: promote re-shares when deleting the parent share
- 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
- Code is properly formatted
- Sign-off message is added to all commits
- [x] Tests (unit, integration, api and/or acceptance) are included
- [x] Screenshots before/after for front-end changes
- [x] Documentation (manuals or wiki) has been updated or is not required
- [ ] Backports requested where applicable (ex: critical bugfixes)
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: https://github.com/nextcloud/server/pull/43025#issuecomment-2189118746 https://github.com/nextcloud/server/pull/43025#issuecomment-1914759785
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.
To make sure I understand this correct:
- A shares F with B
- B re-shares F with C
- A deletes share of F with B
- 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.
To make sure I understand this correct:
- A shares F with B
- B re-shares F with C
- A deletes share of F with B
- 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.
I’m not sure if cypress failures can be related or not. Should be double check as sharing sidebar is on the failure screenshot.
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
