ShareesAPIController should respect groups disallowed from sharing

[Tetrachloromethane250]

ShareesAPIController should return an empty list if the person asking for who they're allowed to share with is only in groups that are excluded from sharing.

fix #20950

[Tetrachloromethane250]

@ChristophWurst It's a rather old issue so I don't want to mention everyone from the issue, but the homepage says to mention someone (apologies if I'm interpreting that wrong). Just wondering if it would be possible to check that this is actually a suitable fix before I go and fix the unit tests (I put my reasoning in the issue conversation)

[kesselb]

Thanks @Tetrachloromethane250 for your time and sending patch 👍

It looks to me that #25391 addresses a similar case?

[Tetrachloromethane250]

I think #25391 is fixing a very different but similarly worded issue - from what I can tell they're changing it so that it listens to the option to allow/disallow the recipient of a share to be a group (so that everyone in the group receives the share). This is making it respect the setting where entire groups can be prevented from initiating a share. Thanks @kesselb for the suggestions - I've committed them. I'll start on rewriting the tests.

[Tetrachloromethane250]

Just poking this again so it (hopefully) doesn't completely fall off the radar

[artonge]

1. If $excludedGroups = ['A'] and $usersGroups = ['A'], then this returns early. OK
2. If $excludedGroups = ['A'] and $usersGroups = ['B'], then this does not return early. OK
3. If $excludedGroups = ['A'] and $usersGroups = ['A', 'B'], then this does not return early. NOT OK ?
4. If $excludedGroups = ['A', 'B'] and $usersGroups = ['A'], then this returns early. OK

Am I right that case #3 should return early?

This might be more correct:

Suggested change

	if (array_intersect($usersGroups, $excludedGroups) === $usersGroups) {
	return new DataResponse($this->result);
	}
	if (count(array_intersect($usersGroups, $excludedGroups) !== 0) {
	return new DataResponse($this->result);
	}

[Tetrachloromethane250]

It's been a while since I've looked at Nextcloud, so please excuse any terminology mixups.

If I remember correctly my theory was that a user should be blocked from viewing potential Sharees only if all the groups they are a member of are blocked from sharing. (#20950 (comment))

I was looking at it from a scenario where some imaginary Admin user is also a member of a group which is blocked from sharing: should membership of a single excludedGroup prevent a user from getting a list of sharees? At the time Share Manager took the approach that a user should be blocked from sharing only if they were only a member of excluded groups so I followed that precedent (appears to still be the decision made in https://github.com/nextcloud/server/blob/master/lib/private/Share20/ShareDisableChecker.php#L52)

Either way, that's probably a higher level design decision that I'll leave to someone else.

[susnux]

Yes sharing is only disabled if the user is only in groups disabled for sharing.

[susnux]

I think it can be simplified to just:

if ($this->shareManager->sharingDisabledForUser($this->userId)) {
	return new DataResponse($this->result);
}

[artonge]

@Tetrachloroethene, sorry for the huge delay here. If you are still motivated to push this forward, then feel free to rebase and address my comment. :)
I think it makes sense anyway.

[Tetrachloromethane250]

@artonge Unfortunately I probably don't have the time to push this forward any more - very happy for anyone else to take over / fix in a different way though.

I've replied to the suggestions with what I can remember of my reasoning, and most of my thought process at the time is in a comment on the linked issue.

Sorry!

[come-nc]

Logic looks sane to me, did not test though.
There is a psalm warning about sending the ?IUser to the groupmanager without checking for null first.

[susnux]

@Tetrachloromethane250 thank you very much for the pull request, we fixed it the same way but now that we have a manager function we could use, we could simplify the needed changes a lot :)