dont show remote and email options if we have an exact match for local user email

[icewind1991]

Signed-off-by: Robin Appelman robin@icewind.nl

[wiswedel]

ok: guest users aren't shown for sharebymail anymore
not ok: also valid federated share targets aren't assumed anymore

before:

after:

[icewind1991]

imo, if a local user with a given email exists than it's highly unlikely that that email is also a fed share target that the user wants to share with. It already hides the fed. share option if the email is found in the user's address book

[wiswedel]

It already hides the fed. share option if the email is found in the user's address book

Fair enough. Didn't know that

if a local user with a given email exists than it's highly unlikely that that email is also a fed share target that the user wants to share with.

Tricky to assume things like that. But I'm not going to open a general discussion about this. So let's go for your solution.

[wiswedel]

@rullzer@blizzz Can you please add your review? We need to get this on the road. Thx.

[MorrisJobke]

I just tested again and for email addresses with exact match this is already the case since 15 at least. See also #15665 (comment)

[MorrisJobke]

Tested and works 👍

[MorrisJobke]

Those are the tests that fail:

server/tests/lib/Collaboration/Collaborators/UserPluginTest.php

Lines 244 to 358 in 03f1fef

	[
	'test',
	true,
	true,
	['abc', 'xyz'],
	[
	['abc', 'test', 2, 0, ['test1' => 'Test One']],
	['xyz', 'test', 2, 0, []],
	],
	[],
	[
	['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']],
	],
	true,
	false,
	],
	[
	'test',
	true,
	false,
	['abc', 'xyz'],
	[
	['abc', 'test', 2, 0, ['test1' => 'Test One']],
	['xyz', 'test', 2, 0, []],
	],
	[],
	[],
	true,
	false,
	],
	[
	'test',
	true,
	true,
	['abc', 'xyz'],
	[
	['abc', 'test', 2, 0, [
	'test1' => 'Test One',
	'test2' => 'Test Two',
	]],
	['xyz', 'test', 2, 0, [
	'test1' => 'Test One',
	'test2' => 'Test Two',
	]],
	],
	[],
	[
	['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']],
	['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']],
	],
	false,
	false,
	],
	[
	'test',
	true,
	false,
	['abc', 'xyz'],
	[
	['abc', 'test', 2, 0, [
	'test1' => 'Test One',
	'test2' => 'Test Two',
	]],
	['xyz', 'test', 2, 0, [
	'test1' => 'Test One',
	'test2' => 'Test Two',
	]],
	],
	[],
	[],
	true,
	false,
	],
	[
	'test',
	true,
	true,
	['abc', 'xyz'],
	[
	['abc', 'test', 2, 0, [
	'test' => 'Test One',
	]],
	['xyz', 'test', 2, 0, [
	'test2' => 'Test Two',
	]],
	],
	[
	['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']],
	],
	[
	['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']],
	],
	false,
	false,
	],
	[
	'test',
	true,
	false,
	['abc', 'xyz'],
	[
	['abc', 'test', 2, 0, [
	'test' => 'Test One',
	]],
	['xyz', 'test', 2, 0, [
	'test2' => 'Test Two',
	]],
	],
	[
	['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']],
	],
	[],
	true,
	false,
	],

All of them do things like this:

 [
['abc', 'test', 2, 0, ['test1' => 'Test One']],
['xyz', 'test', 2, 0, []],
],

as the user results instead of

[
$this->getUserMock('test0', 'Test'),
$this->getUserMock('test1', 'Test One'),
$this->getUserMock('test2', 'Test Two'),
],

Thus I have no idea what the initial thoughts where behind this.

They are coming from somewhere here: https://github.com/nextcloud/server/blame/dd9e191d373217b2a07e4ac5b2cc294c0a6227a1/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php#L1272

@nickvergessen@icewind1991 Any idea how to "fix" them?

[nickvergessen]

Seems like the tests where just not updated. It should return getUserMocks there as well.

[nickvergessen]

This needs adjustments in the tests.
It returns tons of null which of course later on dont have a getDisplayName() function, etc.

[skjnldsv]

Bump?

[nickvergessen]

Rebased and fixed the tests.
Let's get this in

[blizzz]

another rebase, as some failing tests are fixed on master meanwhile

[icewind1991]

rebased and fixed tests

[skjnldsv]

Integration tests are broken

[skjnldsv]

@danxuliu can you have a look, I'm really not familiar with the integration tests :(

[danxuliu]

@danxuliu can you have a look, I'm really not familiar with the integration tests :(

The sharees integration test failures are legit. The problem is that now $userGroup->searchDisplayName is used, and it calls $backend->usersInGroup, which performs a case sensitive search. As the search string in the tests is shareethe user Sharee1 now is not found.

[skjnldsv]

Yep, needed for 18!
/backport to stable18

[backportbot-nextcloud]

backport to stable18 in #20574

[nickvergessen]

This breaks searching for users by displaynames when shareapi_only_share_with_group_members is on see #21451

[DanScharon]

unfortunately this doesn't work if the mail address is pasted with preceding or following whitespace(s).
We have users who do that and hit the first result (federated share) which fails, but they very often don't see the error message.