Enhance privacy of contactsmenu fixes #5107 #5585

LEDfan · 2017-07-02T12:22:24Z

This filter outs all contacts which shouldn't be visible according to the rules in #5107.

codecov · 2017-07-02T14:07:10Z

Codecov Report

Merging #5585 into master will decrease coverage by 16.42%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #5585       +/-   ##
=============================================
- Coverage     53.44%   37.01%   -16.43%     
- Complexity    22542    22552       +10     
=============================================
  Files          1408     1408               
  Lines         87206    87233       +27     
  Branches       1328     1328               
=============================================
- Hits          46604    32291    -14313     
- Misses        40602    54942    +14340

Impacted Files	Coverage Δ	Complexity Δ
...ib/private/Contacts/ContactsMenu/ContactsStore.php	`0% <0%> (-92.31%)`	`34 <10> (+10)`
apps/files/lib/Activity/Settings/FileFavorite.php	`0% <0%> (-100%)`	`8% <0%> (ø)`
apps/sharebymail/lib/Settings.php	`0% <0%> (-100%)`	`3% <0%> (ø)`
lib/private/Files/SimpleFS/SimpleFolder.php	`0% <0%> (-100%)`	`11% <0%> (ø)`
core/Command/TwoFactorAuth/Disable.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
core/Command/TwoFactorAuth/Enable.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
apps/federation/lib/Hooks.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
...ddleware/Security/Exceptions/NotAdminException.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
...pps/files/lib/Activity/Settings/FavoriteAction.php	`0% <0%> (-100%)`	`8% <0%> (ø)`
settings/Activity/SecurityFilter.php	`0% <0%> (-100%)`	`7% <0%> (ø)`
... and 482 more

ChristophWurst

Some early feedback. I haven't had a closer look yet, nor did I test this. Will do ASAP.
Thanks!

ChristophWurst · 2017-07-07T11:30:07Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+	 *  1. filter the current user
+	 *  2. if the `shareapi_exclude_groups` config option is enabled and the
+	 * current user is in an excluded group it will filter all local users.
+	 *  3. if the ``shareapi_only_share_with_group_members config option is


Should be

`shareapi_only_share_with_group_members`

I guess

ChristophWurst · 2017-07-07T11:30:49Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+	 * @return array the filtered contacts
+	 */
+	private function filterContacts(IUser $self, Array $entries) {
+		$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes' ? true : false;


? true : false is not necessary, the === check will already result in a boolean value.

ChristophWurst · 2017-07-07T11:31:18Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+		$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes' ? true : false;
+
+		$skipLocal = false; // whether to filter out local users
+		$ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes' ? true : false; // whether to filter out all users which doesn't have the same group as the current user


same as above

rotanid · 2017-08-03T23:24:13Z

milestone nextcloud 13?? i would definitely call this serious privacy problem a regression that has to be fixed with the next maintenance release.

ChristophWurst

Tested and works as expected. Thanks a lot 👍

I've left some comments regarding coding style.

ChristophWurst · 2017-08-05T07:02:32Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+	 * enabled it will filter all users which doens't have a common group
+	 * with the current user.
+	 * @param IUser $self
+	 * @param $entries Entry[]


should be @param Entry[] $entries

ChristophWurst · 2017-08-05T07:02:56Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+	 * @param $entries Entry[]
+	 * @return array the filtered contacts
+	 */
+	private function filterContacts(IUser $self, Array $entries) {


array instead of Array

ChristophWurst · 2017-08-05T07:03:14Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+	 * with the current user.
+	 * @param IUser $self
+	 * @param $entries Entry[]
+	 * @return array the filtered contacts


@return Entry[]

ChristophWurst · 2017-08-05T07:04:13Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+	private function filterContacts(IUser $self, Array $entries) {
+		$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes';
+
+		$skipLocal = false; // whether to filter out local users


move inline comments to a new line please

ChristophWurst · 2017-08-05T07:08:03Z

tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php

+	}
+
+	public function testGetContactsOnlyIfInTheSameGroup() {
+		$this->config->expects($this->at(0)) ->method('getAppValue')


great to have this tested 👍

LEDfan · 2017-08-05T14:19:51Z

@ChristophWurst I addressed your issues, thanks for testing.

@rotanid I do agree with you.
@karlitschek should this be backported to 12?

karlitschek · 2017-08-05T20:04:15Z

i think this should be backported

LEDfan · 2017-08-06T09:50:51Z

I just realized that the ContactsStore API probably should be public, so apps who interact with the contacts menu knows which contacts are available in the menu. E.g. in the Chat app we want to limit chatting to the users available in the Contacts menu (i.e. filtered by this PR)

An alternative way could be to move the filtering to the ContactsManager API.

What do you think?

LEDfan · 2017-08-14T09:48:25Z

@ChristophWurst I fixed a bug and also updated the findOne method because that wasn't filtered already.

LEDfan · 2017-08-31T17:08:42Z

@MorrisJobke @LukasReschke @blizzz @rullzer @schiessle @nickvergessen @georgehrke Excuse me for pinging you all 😄 , but can someone please review this? There are many people asking for this.

Also please point out your opinion about #5585 (comment) and #5107 (comment)

Thanks!

LukasReschke · 2017-09-15T12:29:59Z

I'll review this and additionally revert 56a9084 in here and add the handling here itself.

- Groups, which are excluded from sharing should not see local users at all - If sharing is restricted to users own groups, he should only see contacts from his groups: Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

…ption + fix findOne Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

LukasReschke · 2017-09-15T12:32:06Z

Rebased upon master.

…letion is disabled" This reverts commit 56a9084.

This adjusts the contacts menu to also support searching by email address which is relevant in scenarios where no UID is known such as LDAP, etc. Furthermore, if `shareapi_allow_share_dialog_user_enumeration` is disabled only results are shown that match the full user ID or email address. Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

LukasReschke · 2017-09-15T14:04:57Z

I added 5896176...705432c, in particular:

Searching for email addresses with shareapi_allow_share_dialog_user_enumeration is now possible again as it was in stable11:

The contacts menu in the top right show contacts if a full UID or the full email has been searched for:

Note that the search is on "UID" and "EMAIL". So "FN" is not searched.

Seems all fixed

LukasReschke

Works 👍

danxuliu

CI complains because reverted commit was not signed, but as it is a revert commit it is not a big deal.

Tested and works; code looks good (but, please, next time do not mix code style changes and behaviour changes in the same commit!) 👍

LEDfan · 2017-09-16T11:06:07Z

@LukasReschke thanks for taking care of this!

Note that the search is on "UID" and "EMAIL". So "FN" is not searched.

Is there a reason for? E.g. on LDAP you as user would know the FN but not the UID?

LukasReschke · 2017-09-16T11:52:11Z

Is there a reason for? E.g. on LDAP you as user would know the FN but not the UID?

Mainly because we have a business requirement requiring this behaviour: Filtering via UID / EMAIL should be possible but for whatever reason filtering to FN shouldn't be possible.

cc @oparoz Or would filtering for complete FN here also be acceptable? Your call 😉

LukasReschke · 2017-09-18T15:35:18Z

Backport at #6554

juliusknorr added 3. to review Waiting for reviews enhancement feature: contacts menu labels Jul 3, 2017

juliusknorr added this to the Nextcloud 13 milestone Jul 3, 2017

ChristophWurst self-requested a review July 5, 2017 09:02

ChristophWurst requested changes Jul 7, 2017

View reviewed changes

LEDfan force-pushed the contacts_menu_privacy branch from f10cd5b to 6b79567 Compare July 8, 2017 06:04

ChristophWurst mentioned this pull request Aug 5, 2017

Error when updating Deck nextcloud/deck#237

Closed

ChristophWurst previously requested changes Aug 5, 2017

View reviewed changes

ebogaard mentioned this pull request Aug 9, 2017

Privacy enhancements for contacts menu #5107

Closed

4 tasks

LEDfan force-pushed the contacts_menu_privacy branch from 60b961f to 3253e11 Compare August 14, 2017 09:47

LEDfan mentioned this pull request Aug 14, 2017

Limit chatting to users in your group and allow to exclude certain groups nextcloud/jsxc.nextcloud#57

Merged

14 tasks

LukasReschke self-assigned this Sep 15, 2017

LEDfan added 6 commits September 15, 2017 14:31

Privacy enhancements for contacts menu

92c238e

- Groups, which are excluded from sharing should not see local users at all - If sharing is restricted to users own groups, he should only see contacts from his groups: Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

Fix tests

473a1ec

Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

Add tests

fa402c7

Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

Some code improvements

f0370c0

Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

Improve code style

7dfa527

Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

Fix issue when disabling the shareapi_only_share_with_group_members o…

5896176

…ption + fix findOne Signed-off-by: Tobia De Koninck <tobia@ledfan.be>

LukasReschke force-pushed the contacts_menu_privacy branch from 3253e11 to 5896176 Compare September 15, 2017 12:31

LukasReschke added 2 commits September 15, 2017 14:32

Revert "Do not list system users in contacts menu if sharing autocomp…

6d02fe0

…letion is disabled" This reverts commit 56a9084.

LukasReschke requested review from ChristophWurst and danxuliu September 15, 2017 14:24

LukasReschke removed their assignment Sep 15, 2017

LukasReschke approved these changes Sep 15, 2017

View reviewed changes

danxuliu approved these changes Sep 15, 2017

View reviewed changes

danxuliu mentioned this pull request Sep 16, 2017

Sharing dropdown leaks email addresses of users in other groups #6530

Closed

MorrisJobke merged commit 8b1eaba into master Sep 18, 2017

MorrisJobke deleted the contacts_menu_privacy branch September 18, 2017 12:25

LukasReschke mentioned this pull request Sep 18, 2017

[Stable12] contacts menu privacy #6554

Merged

nextcloud deleted a comment from LukasReschke Sep 18, 2017

tcitworld mentioned this pull request Sep 20, 2017

Attendee-Autocomplete: Prioritize user's address books over system address book nextcloud/calendar#168

Closed

LEDfan mentioned this pull request Sep 25, 2017

Make ContactsStore a public API #6637

Merged

9662 mentioned this pull request Nov 3, 2017

Sharing dialogue and “search contacts” widget leak contact information from other users #7055

Closed

tflidd mentioned this pull request Jan 27, 2018

Security problem / sharing options #8028

Closed

ChristophWurst mentioned this pull request Mar 19, 2018

Fix NULL users? #8765

Closed

gmright2 mentioned this pull request Jan 10, 2023

[Snyk] Security upgrade puppeteer from 1.20.0 to 3.0.0 gmright2/server#110

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance privacy of contactsmenu fixes #5107 #5585

Enhance privacy of contactsmenu fixes #5107 #5585

LEDfan commented Jul 2, 2017

codecov bot commented Jul 2, 2017 •

edited

Loading

ChristophWurst left a comment

ChristophWurst Jul 7, 2017

ChristophWurst Jul 7, 2017

ChristophWurst Jul 7, 2017

rotanid commented Aug 3, 2017

ChristophWurst left a comment

ChristophWurst Aug 5, 2017

ChristophWurst Aug 5, 2017

ChristophWurst Aug 5, 2017

ChristophWurst Aug 5, 2017

ChristophWurst Aug 5, 2017

LEDfan commented Aug 5, 2017

karlitschek commented Aug 5, 2017

LEDfan commented Aug 6, 2017

LEDfan commented Aug 14, 2017

LEDfan commented Aug 31, 2017

LukasReschke commented Sep 15, 2017

LukasReschke commented Sep 15, 2017

LukasReschke commented Sep 15, 2017

LukasReschke left a comment

danxuliu left a comment •

edited

Loading

LEDfan commented Sep 16, 2017 •

edited

Loading

LukasReschke commented Sep 16, 2017 •

edited

Loading

LukasReschke commented Sep 18, 2017

Enhance privacy of contactsmenu fixes #5107 #5585

Enhance privacy of contactsmenu fixes #5107 #5585

Conversation

LEDfan commented Jul 2, 2017

codecov bot commented Jul 2, 2017 • edited Loading

Codecov Report

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotanid commented Aug 3, 2017

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LEDfan commented Aug 5, 2017

karlitschek commented Aug 5, 2017

LEDfan commented Aug 6, 2017

LEDfan commented Aug 14, 2017

LEDfan commented Aug 31, 2017

LukasReschke commented Sep 15, 2017

LukasReschke commented Sep 15, 2017

LukasReschke commented Sep 15, 2017

LukasReschke left a comment

Choose a reason for hiding this comment

danxuliu left a comment • edited Loading

Choose a reason for hiding this comment

LEDfan commented Sep 16, 2017 • edited Loading

LukasReschke commented Sep 16, 2017 • edited Loading

LukasReschke commented Sep 18, 2017

codecov bot commented Jul 2, 2017 •

edited

Loading

danxuliu left a comment •

edited

Loading

LEDfan commented Sep 16, 2017 •

edited

Loading

LukasReschke commented Sep 16, 2017 •

edited

Loading