Fix file being shared with approver group even if the group already has access

[julien-nc]

When requesting approval with a rule having a group as "approver", the file is always shared with this group to make sure the group has access.

Two problems:

1. The group might already have access (via a file share or through a group folder) and we create an unnecessary file share
2. The group does not have access but some of its members have access. Those users will receive access with the group share we create but they already had access.

This PR kind of fixes 1.
We only create the file share to the group if at least one member of this group does not have access to the file.

Iterating over group members to check if they have access is not ideal performance-wise.
@skjnldsv Is there a more efficient way to check if a group has access to a file (via a group folder, a file share or anything else)?

cc @blizzz

[skjnldsv]

@skjnldsv Is there a more efficient way to check if a group has access to a file (via a group folder, a file share or anything else)?

Not sure about most efficient, but this is what is being used in the files app

$uid = $this->userSession->getUser()->getUID();
$baseFolder = $this->rootFolder->getUserFolder($uid);
$files = $baseFolder->getById($fileId);
if (empty($files)) {
	// not access to this fileID
}

[julien-nc]

@skjnldsv Thanks for the feedback. That's also how it's done in Approval to check if one user has access to a specific file.

My question is more about how to check if a group has access to a file.

The implementation in this PR iterates on the group members and calls getById on the user folder, just like you mentioned. The concern is that it's pretty costly if the group has a lot of member...

[skjnldsv]

Sorry, then I'm lacking expertise! 🙈

[pierluigizagaria]

This is needed

[blizzz]

Iterating over group members to check if they have access is not ideal performance-wise.

Ack! Maybe @nfebe or @artonge know a better way?

[pierluigizagaria]

Point 2 is also very important. As now this plugin cannot be used with single-user shared folder. In the moment the plugin asks for approve, the folder is then shared with the entire group selected in the "Who can approve" field. It would probably be better if the plugin checks for the permission without applying it automatically to the file. I should probably dig more in how the plugin works, but my suggestion is only based on my user experience.

[nfebe]

The group might already have access (via a file share or through a group folder) and we create an unnecessary file share

Approach A:

• Check the list of shares on the file (we can reasonably assume, this is almost always a smaller list than group members) [No share means no access via file sharing at all, early stop]
• Check for direct group shares (if the group is listed a recipient on one of the shares)
• Check user shares for group members, filter list by user-shares, and check if any of the existing recipients on the share is in target group. If not, no group share, no user on the share list has access, so group does not have access.

Not though about it long, but I think with this approach we might never have to iterate through the group members.

Approach B (Similar to A but uses iteration as last resort):

• First, check for direct group shares on the file (fastest check)
• Then, check group folder access (If the file is in a group folder, check if the group has access to the parent folder)
• Only as a last resort, fall back to checking individual users

[lukasdotcom]

There are kind of two parts to this where the first part should be pretty fast and just checks if a group share with the correct group exists (groupHasAccessTo function) and if yes no share is needed. The second part actually will find all the users that don't have access to the document by going over all the user and group shares and filtering the list of users. Right now I just put a user cap on that to prevent performance problems.

[DaphneMuller]

@nfebe would you be able to provide a review here?

[nfebe]

🐘

[lukasdotcom]

Performant part was moved to #320 which should be merged while this will be kept in case someone else wants to do further work on the slow part (sharing only with part of group when some members have access).

[julien-nc]

Edited previous comment: 52 -> 320