Restrict autocomplete to group

[hrenard]

Fix #6017.

[welcome[bot]]

Thanks for opening your first pull request in this repository! :v:

[hrenard]

I want to add a test, but I'm no php developer.

$this->groupManager->expects($this->once())
	->method('isInGroup')
	->will($this->returnValue(false));
$this->groupManager->expects($this->once())
	->method('isInGroup')
	->with('auser', 'agroup')
	->will($this->returnValue(true));

Does this work ?

[hrenard]

Hey, sorry to make noise. But it's a privacy issue, I think we should try to merge this soon.

[miaulalala]

I want to add a test, but I'm no php developer.

$this->groupManager->expects($this->once())
	->method('isInGroup')
	->will($this->returnValue(false));
$this->groupManager->expects($this->once())
	->method('isInGroup')
	->with('auser', 'agroup')
	->will($this->returnValue(true));

Does this work ?

You need to specify that the same method is called twice in this way:

		$this->config->expects(self::exactly(2))
			->method('getAppValue')
			->withConsecutive(['core', 'shareapi_allow_share_dialog_user_enumeration', 'no'], ['core', 'shareapi_restrict_user_enumeration_to_group', 'no'])
			->willReturnOnConsecutiveCalls(['yes', 'no']);

[hrenard]

Thanks for the feedback ! I added testGetMatchingRecipientOnlyGroupUsers for this use case, but I've not tried it.

[miaulalala]

Thanks for the feedback ! I added testGetMatchingRecipientOnlyGroupUsers for this use case, but I've not tried it.

You can run a singular unit test with composer run test:unit:dev ./tests/Unit/Service/ContactsIntegrationTest.php

You need composer2 :)

[hrenard]

You can run a singular unit test with composer run test:unit:dev ./tests/Unit/Service/ContactsIntegrationTest.php

You need composer2 :)

And a configured Nextcloud :sweat_smile:. I havn't quite took the time to setup a proper dev env... You need me to ?

[hrenard]

Okay. The tests are working.

[hrenard]

And the coding standards... Sorry for that.

[miaulalala]

Postgres tests are failing because of an incompatibility. Can you rebase your brach onto the latest mail main? The fix for this has been merged :)

[hrenard]

Postgres tests are failing because of an incompatibility. Can you rebase your brach onto the latest mail main? The fix for this has been merged :)

Done.

[hrenard]

Hi @hrenard , thanks for your hard work so far!

After some feedback I found out there are more conditions to be checked for:

https://github.com/nextcloud/server/blob/88b0aea2b0f7cc30ab0de7d13914474d0711e5cd/lib/private/Collaboration/Collaborators/MailPlugin.php#L88-L93

Would you be up for implementing these too?

If that's too overwhelming, I can take over and build upon your excellent work.

Thanks again!

Yep, will do.

Only shareapi_restrict_user_enumeration_full_match & shareapi_restrict_user_enumeration_full_match_email are missing, right ?

[miaulalala]

Hi @hrenard , thanks for your hard work so far! After some feedback I found out there are more conditions to be checked for: https://github.com/nextcloud/server/blob/88b0aea2b0f7cc30ab0de7d13914474d0711e5cd/lib/private/Collaboration/Collaborators/MailPlugin.php#L88-L93 Would you be up for implementing these too? If that's too overwhelming, I can take over and build upon your excellent work. Thanks again!

Yep, will do.

Only shareapi_restrict_user_enumeration_full_match & shareapi_restrict_user_enumeration_full_match_email are missing, right ?

And the shareapi_restrict_user_enumeration_to_phone I think. Thanks so much!

[hrenard]

As we only search by FN or EMAIL, can we omit the phone ?

shareapi_restrict_user_enumeration_full_match is supposed to match the username. Do we consider the search result's UID to be the username or do we just skip system users if shareapi_restrict_user_enumeration_full_match_email is not set ?

Maybe we should also hide group recipients according to shareapi_allow_group_sharing and when other restrictions are set ? If I'm to be in the same group or to know the full email of someone to send an email, it's weird that I can send emails to random groups thus random peoples. And if someone replies, I get the list of this group members.

Or should we introduce config specific to this app ?

[miaulalala]

shareapi_restrict_user_enumeration_full_match is supposed to match the username. Do we consider the search result's UID to be the username or do we just skip system users if shareapi_restrict_user_enumeration_full_match_email is not set ?

You could pass the fullmatch option to the search if the shareapi_restrict_user_enumeration_full_match_email is set and let the IManager handle it if that does what I think you want to get done? Have a look at this method:

https://github.com/nextcloud/server/blob/master/lib/private/ContactsManager.php#L52

So you could do something like:

$shareapi_restrict_user_enumeration_full_match_email = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_email', 'no') === 'yes';

$result = ($shareapi_restrict_user_enumeration_full_match_email 
                             ? $this->contactsManager->search($term, ['FN', 'EMAIL'], ['fullmatch' => true])
                             : $this->contactsManager->search($term, ['FN', 'EMAIL']));

I think the phone match is fine to leave out, who searches for a users phone no when they want to send an email anyway :sweat_smile:

[hrenard]

Interesting, I'll look into it.

What behavior is intended when both shareapi_restrict_user_enumeration_to_group and shareapi_restrict_user_enumeration_full_match_email are true ?

1. Only match system user with full email.
2. Only match system user in the same group.
3. Match system user with full email or in the same group.

I found the 3 more logical, but I'm not sure of the initial design.

[hrenard]

I think shareapi_restrict_user_enumeration_full_match_email is not used anymore.

@miaulalala I pushed a proposal for option 3 (previous comment). Tell me what you think. I'll update tests when we agree on the behavior.

[miaulalala]

I think shareapi_restrict_user_enumeration_full_match_email is not used anymore.

As far as I can see it is still used in the \OC\Collaboration\Collaborators\UserPlugin::search

If I'm not mistaken, it is done in the way you suggested in 3:

$lowerSearch = strtolower($search);
if (
    $this->shareeEnumerationFullMatch && $lowerSearch !== '' && (strtolower($uid) === $lowerSearch ||
    strtolower($userDisplayName) === $lowerSearch ||
    ($this->shareeEnumerationFullMatchIgnoreSecondDisplayName && trim(strtolower(preg_replace('/ \(.*\)$/', '', $userDisplayName))) === $lowerSearch) ||
    ($this->shareeEnumerationFullMatchEmail && strtolower($userEmail ?? '') === $lowerSearch))
) {}

If I have my ANDs and ORs right, we can proceed with your suggestion :)

[hrenard]

I can't find the shareapi_restrict_user_enumeration_full_match_email setting in the UI of the latest NC which led me to assume it's deprecated. But, I haven't searched that much, so I'm probably wrong.

(I'll try to finish this next week.)

[hrenard]

@miaulalala is this good enough ?