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

Respect sharing options when searching using MailPlugin #7428 #7490

Merged
merged 2 commits into from
Dec 18, 2017

Conversation

LEDfan
Copy link
Member

@LEDfan LEDfan commented Dec 13, 2017

I think I found the problem with #7428, basically when a user gr2_foo has set an email address and is in group gr2 and a user of gr1 tries to share something gr2_foo will show up. This is of course because there is a match based on the email address. However there shouldn't be searched for systemusers using the mail plugin, because there is the UserPlugin.

This can very easily be backported to 12 (I have already pushed a branch) if wanted.

@codecov
Copy link

codecov bot commented Dec 14, 2017

Codecov Report

Merging #7490 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7490      +/-   ##
============================================
+ Coverage     51.16%   51.18%   +0.01%     
- Complexity    24866    24870       +4     
============================================
  Files          1601     1601              
  Lines         94684    94696      +12     
  Branches       1368     1368              
============================================
+ Hits          48448    48468      +20     
+ Misses        46236    46228       -8
Impacted Files Coverage Δ Complexity Δ
...private/Collaboration/Collaborators/MailPlugin.php 72.82% <100%> (+14.07%) 23 <1> (+4) ⬆️
lib/private/Security/CertificateManager.php 91.08% <0%> (-1%) 39% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.28% <0%> (-0.25%) 136% <0%> (ø)
lib/private/Server.php 81.01% <0%> (+0.11%) 134% <0%> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️

@LEDfan
Copy link
Member Author

LEDfan commented Dec 15, 2017

Mmm, actually, it seems like my fix isn't 100% correct, because now if you type the full e-mail address the user won't show up....

… we may share with

Signed-off-by: Tobia De Koninck <tobia@ledfan.be>
@LEDfan
Copy link
Member Author

LEDfan commented Dec 15, 2017

I managed to create a better fix. The commit 9d60f7f is now fixing:

Steps to reproduce

  1. create groups gr1 and gr2, create users gr1_foo, gr1_bar1, gr2_foo and gr2_bar, add the users to the corresponding groups
  2. add the email email@email.be to gr1_foo and add the email abc@abc.be to gr2_bar
  3. without changing settings you should now be able to lookup all users and gr2_bar and gr1_foo by their email addresses
  4. enable the option shareapi_only_share_with_group_members

Expected behaviour

  1. only users in your own group should show up
  2. searching for abc or email should not popup users from other groups

Actual behaviour

  1. the other users do show up

@LEDfan LEDfan changed the title Don't load system users when searching by email in Collaborators fixes #7428 Don't show other groups when searching for #7428 Dec 15, 2017
@LEDfan LEDfan changed the title Don't show other groups when searching for #7428 Respect sharing options when searching using MailPlugin #7428 Dec 15, 2017
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good and works

@blizzz blizzz requested review from rullzer and schiessle December 15, 2017 21:27
@blizzz
Copy link
Member

blizzz commented Dec 15, 2017

Tests need to be adjusted, though

Signed-off-by: Tobia De Koninck <tobia@ledfan.be>
@LEDfan
Copy link
Member Author

LEDfan commented Dec 16, 2017

@blizzz I fixed the tests and add some test cases for this patch.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

@MorrisJobke MorrisJobke merged commit 3da92a9 into master Dec 18, 2017
@MorrisJobke MorrisJobke deleted the fix_7428 branch December 18, 2017 13:08
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Dec 18, 2017
@MorrisJobke
Copy link
Member

This can very easily be backported to 12 (I have already pushed a branch) if wanted.

Yes - please open a backport PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants