Respect user enumeration settings in user status lists

[mejo-]

The functions to find user statuses listed didn't respect user
enumeration settings (shareapi_allow_share_dialog_user_enumeration
and shareapi_restrict_user_enumeration_to_group core app settings).

Fixes: #27122

[mejo-]

As mentioned in issue #27122, I'd prefer to remove the route Statuses#findAll and statusService->findAll() altogether. But in order to be the least intrusive, for now this PR only implements the second part of my suggested fix:

Make statusService->findAllRecentStatusChanges return an empty array if config option shareapi_allow_share_dialog_user_enumeration is disabled.

Please let me know what you think about removing the findAll routes and functions.

[nickvergessen]

From my POV this is not enumeration. You can not start to type a name and find the users. but of course since the limit and offset are controlled, you can still create a user list, so sounds good.

But if shareapi_restrict_user_enumeration_to_phone or shareapi_restrict_user_enumeration_to_group is enabled we should return those users I think. Maybe

server/lib/private/Collaboration/Collaborators/UserPlugin.php

Lines 99 to 134 in 215aef3

	if ($this->shareWithGroupOnly || $this->shareeEnumerationInGroupOnly) {
	// Search in all the groups this user is part of
	foreach ($currentUserGroups as $userGroupId) {
	$usersInGroup = $this->groupManager->displayNamesInGroup($userGroupId, $search, $limit, $offset);
	foreach ($usersInGroup as $userId => $displayName) {
	$userId = (string) $userId;
	$user = $this->userManager->get($userId);
	if (!$user->isEnabled()) {
	// Ignore disabled users
	continue;
	}
	$users[$userId] = $user;
	}
	if (count($usersInGroup) >= $limit) {
	$hasMoreResults = true;
	}
	}
	if (!$this->shareWithGroupOnly && $this->shareeEnumerationPhone) {
	$usersTmp = $this->userManager->searchKnownUsersByDisplayName($currentUserId, $search, $limit, $offset);
	if (!empty($usersTmp)) {
	foreach ($usersTmp as $user) {
	if ($user->isEnabled()) { // Don't keep deactivated users
	$users[$user->getUID()] = $user;
	}
	}
	uasort($users, function ($a, $b) {
	/**
	* @var \OC\User\User $a
	* @var \OC\User\User $b
	*/
	return strcasecmp($a->getDisplayName(), $b->getDisplayName());
	});
	}
	}

can give you some inspiration

[mejo-]

From my POV this is not enumeration. You can not start to type a name and find the users. but of course since the limit and offset are controlled, you can still create a user list, so sounds good.

You're right, strictly speaking it's not enumeration. But with the Statuses#findAll route, you can simply get a list of all users on the instance, which is even worse than enumeration 😉 And as you said, by controlling limits and offsets, it's possible to get all users even with the Statuses#find route.

[mejo-]

But if shareapi_restrict_user_enumeration_to_phone or shareapi_restrict_user_enumeration_to_group is enabled we should return those users I think.

If I understand it correctly, then this would be further limitations to if shareapi_allow_share_dialog_user_enumeration is enabled, right? If shareapi_allow_share_dialog_user_enumeration is disabled, then it's not possible to select those two options you mentioned. So my understanding is that they're there to limit the scope of user enumeration in case it's enabled in general.

So far, this PR is about fixing the user_status API to respect admins choice to disable user enumeration altogether.

Would you prefer to implement support for shareapi_restrict_user_enumeration_to_phone and shareapi_restrict_user_enumeration_to_group in this PR as well? If you want me to, I could certainly look into it. But it would increase scope and complexibility of this PR a lot 😉

[nickvergessen]

So my understanding is that they're there to limit the scope of user enumeration in case it's enabled in general.

Yeah, you are right. My head tricked me thinking it was the other way around.

Would you prefer to implement support for shareapi_restrict_user_enumeration_to_phone and shareapi_restrict_user_enumeration_to_group in this PR as well? If you want me to, I could certainly look into it. But it would increase scope and complexibility of this PR a lot wink

Yes, because either we say the setting does influence it, then the further settings should too, or not then we don't need this PR or a follow up.

[mejo-]

So I took a deeper look now and changed the logic to respect both shareapi_allow_share_dialog_user_enumeration and shareapi_restrict_user_enumeration_to_group as suggested by @nickvergessen 😊

As far as I understand, shareapi_restrict_user_enumeration_to_phone is not relevant here as it's only about limiting partial matches for particular user names.

[mejo-]

Now all tests are passing again. Sorry, the last days CI had some hickups and I didn't spot that the drone check didn't pass due to errors in the unit tests.

[mejo-]

Mh, any chance to get a review here 😊

[nickvergessen]

As far as I understand, shareapi_restrict_user_enumeration_to_phone is not relevant here as it's only about limiting partial matches for particular user names.

Well so it should also be respected if possible.

Basically if:

• shareapi_allow_share_dialog_user_enumeration: yes
• shareapi_restrict_user_enumeration_to_group: no
• shareapi_restrict_user_enumeration_to_phone: yes

you should only see users which you have the phone number matched.
But I would be okay to ignore it for now and fix this in a follow up as it increases complexity a lot.

[mejo-]

But I would be okay to ignore it for now and fix this in a follow up as it increases complexity a lot.

@nickvergessen thanks for your feedback 😊

So are you ok with the PR as is or do you see the need for further changes/improvements?

[nickvergessen]

I think this does not scale. We need to do it in another way. In worst case someone is in hundreds of (automated) groups, you run hundreds of queries to generate the user list to then later know that they don't even have a status set or something. Can't we do it similar to lib/private/Collaboration/Collaborators/UserPlugin.php and first get the recent user statuses and then filter out the results?
Basically get the status of 100 people, then filter out, if less than the requested users remain retry

[mejo-]

Mhhh, you certainly have an argument here 😬

But I'm unsure whether the other way around scales better: It's rather unlikely that the desired 8 users from shared groups with recent status changes are amongh the first 100. So in many cases we'll end up iterating over all users with recent status changes. And for each user we have to check whether they're in a shared group.

So I see two approaches and I'm not sure which one scales better:

• Iterate over all usergroups and their members, return a list of users. Filter the search for user statuses by this list of users.
• Iterate over most users with recent status changes. For some/many of them, check whether we're in a shared group.

@nickvergessen you're way more experienced, so I'd trust you on what scales better on common setups/instances.

Besides: I'm not sure what you mean with your reference to lib/private/Collaboration/Collaborators/UserPlugin.php. I mostly copied its implementation to search for users from shared groups. Looking at lib/private/Collaboration/Collaborators/UserPlugin.php#L98-L115, it iterates over all members of all groups that the user is member of just like the implementation in this PR, no? Maybe I miss something :grimaced:

[mejo-]

After further discussion with @nickvergessen, we'll go with a more drastic and simpler approach that fixes the privacy issue first. See #29260.