fix(sharing): do not notify remote if owner and sharee are local users#57667
Open
fix(sharing): do not notify remote if owner and sharee are local users#57667
Conversation
1977e86 to
bbf2e7e
Compare
5d52a0c to
823c8d3
Compare
1712e6d to
75a7f12
Compare
If both the owner and the sharee are local users, no remote notification is required. If either the owner or the sharee is not a local user, the change must be propagated. Based on owncloud/core#37534 Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de> Co-authored-by: Victor Dubiniuk <victor.dubiniuk@gmail.com>
75a7f12 to
b735ee5
Compare
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This one is a bit tricky and caused me some headaches 🤯
Steps to reproduce:
The ShareNotFound exception is thrown in
FederatedShareProvider.getRemoteIdwhen there is no record in federated_shares. That seems like a valid state, since Alice and Bob are both on instance A.The check in
FederatedShareProvider.updateonly verifies that alice != bob and therefore callsFederatedShareProvider.sendPermissionUpdate. However, because Alice and Bob are on the same instance, there is no need to send a permission update to a remote server.This change adjusts the condition accordingly, similar to what was done in oC a few years ago.
Given that this code path has barely been touched in recent years, it was surprising to see this error now. My assumption is that updating federated shares used to be a relatively uncommon operation, and since it usually worked despite that error, there were no reports.
Since #51250, we now call create followed by update when creating a share, which makes this case much more likely to occur and exposes the issue more frequently.
How to test:
Case 1:
Case 2:
(Note: Unsharing works, but item is not removed from the share list in the view currently. That's unrelated to this pr.)
Case 3:
(Note: Unsharing works, but item is not removed from the share list in the view currently. That's unrelated to this pr.)
Tests:
I agree the tests are not great due to the heavy mocking around the query builder and related components. Still, I thought it was important to have at least some validation that the correct code path is executed.
The existing FederatedShareProviderTest does not write anything into the federated_shares table, so this code path is never called there.
Once the app is migrated to our usual QBMapper, the tests should become much cleaner and easier to maintain.
TODO
Checklist
3. to review, feature component)stable32)