Exclude groups for sharing

[T0mWz]

See also issue #49198

Certain application or configuration options require the use of groups. Think of enabling certain apps for a specific group of the option to restrict autocomplete with sharing for only users in a specific group.
When you make use of these options, it can only result in large groups. When someone accidentally shares data with such a group then, it can have data impact in addition to large system impact.

Example array response which we got in on line; https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/Controller/ShareesAPIController.php#L200

Array
(
    [exact] => Array
        (
            [users] => Array
                (
                )

            [groups] => Array
                (
                )

            [remotes] => Array
                (
                )

            [remote_groups] => Array
                (
                )

            [emails] => Array
                (
                )

            [circles] => Array
                (
                )

            [rooms] => Array
                (
                )

        )

    [users] => Array
        (
            [0] => Array
                (
                    [label] => jupyterUser
                    [subline] => 
                    [icon] => icon-user
                    [value] => Array
                        (
                            [shareType] => 0
                            [shareWith] => jupyterUser
                        )

                    [shareWithDisplayNameUnique] => jupyterUser
                    [status] => Array
                        (
                        )

                )

        )

    [groups] => Array
        (
            [0] => Array
                (
                    [label] => jupyter
                    [value] => Array
                        (
                            [shareType] => 1
                            [shareWith] => jupyter
                        )

                )

            [1] => Array
                (
                    [label] => jupyter1
                    [value] => Array
                        (
                            [shareType] => 1
                            [shareWith] => jupyter1
                        )

                )

        )

    [remotes] => Array
        (
        )

    [remote_groups] => Array
        (
        )

    [emails] => Array
        (
        )

    [lookup] => Array
        (
        )

    [circles] => Array
        (
        )

    [rooms] => Array
        (
        )

    [lookupEnabled] => 1
)

From this result we will filter the group jupyter as example.
Array will contain a list of groups which will be excluded for sharing purposes.

vi config.php
..
  'blacklisted_groups' =>
  array(
     0 => "admin",
     1 => "jupyter",
     2 => "all_employees",
  ),
..

[provokateurin]

Please add a test for this

[provokateurin]

Suggested change

	// Get a list of groups to exclude from search results
	$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);
	// Get a list of groups to exclude from search results
	$sharingDisabledGroups = $this->config->getSystemValue('sharing.disabled_groups', true);

This name is too generic for a system option and blacklist should be avoided (it's better and clearer to use blocklist).

[provokateurin]

Suggested change

	// Get a list of groups to exclude from search results
	$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);
	// Get a list of groups to exclude from search results
	$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', []);

The default value needs to be an empty array, as otherwise the later code will fail if the config is not set.

[provokateurin]

Suggested change

	$this->result['groups'] = array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {
	return !in_array($group['label'], $blacklisted_groups);
	});
	// Reindex array to maintain sequential numeric keys
	$this->result['groups'] = array_values($this->result['groups']);
	$this->result['groups'] = array_values(array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {
	return !in_array($group['label'], $blacklisted_groups);
	}));

[provokateurin]

Suggested change

	$this->result['exact']['groups'] = array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {
	return !in_array($group['label'], $blacklisted_groups);
	});
	// Reindex array to maintain sequential numeric keys
	$this->result['exact']['groups'] = array_values($this->result['exact']['groups']);
	$this->result['exact']['groups'] = array_values(array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {
	return !in_array($group['label'], $blacklisted_groups);
	}));

[provokateurin]

Also some documentation in https://github.com/nextcloud/documentation about this would be nice as well.

[sorbaugh]

@tomwezepoel thanks for the contribution and good luck with the reviews!

[provokateurin]

Additionally to excluding groups from the search results we should actually prevent creating shares with one of the groups as the recipient. This needs to be implemented in the ShareAPIController.

[artonge]

• Maybe that logic would fit better in lib/private/Collaboration/Collaborators/GroupPlugin.php?
• The current solution limits which groups you can search for, but don't limit which you can share with. This is probably the place to add that logic: https://github.com/nextcloud-gmbh/server/blob/1d69ffa48948bbcebc892cc89cd8b551940c58eb/lib/private/Share20/Manager.php#L491-L492
• You'll also need to add checkbox in the admin settings. Probably in apps/settings/lib/Settings/Admin/Sharing.php

[nfebe]

DRY adjustments and possible type guard addition

[nfebe]

Suggested change

	$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);
	$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);
	if (!is_array($blacklisted_groups)) {
	$blacklisted_groups = [];
	}

Graceful handling if non array is passed? This can be needless if the source if SURE to provide arrays (when probably not configurable)

[provokateurin]

I think it is fine to assume we get an array, but the default value needs to be an array like I already suggested above.

[nfebe]

Suggested change

	if (isset($this->result['groups'])) {
	$this->result['groups'] = array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {
	return !in_array($group['label'], $blacklisted_groups);
	});
	// Reindex array to maintain sequential numeric keys
	$this->result['groups'] = array_values($this->result['groups']);
	}
	if (isset($this->result['exact']['groups'])) {
	$this->result['exact']['groups'] = array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {
	return !in_array($group['label'], $blacklisted_groups);
	});
	// Reindex array to maintain sequential numeric keys
	$this->result['exact']['groups'] = array_values($this->result['exact']['groups']);
	}
	$filterBlacklistedGroups = function ($groups) use ($blacklisted_groups) {
	return array_values(array_filter($groups, function ($group) use ($blacklisted_groups) {
	return isset($group['label']) && !in_array($group['label'], $blacklisted_groups);
	}));
	};
	if (isset($this->result['groups'])) {
	$this->result['groups'] = $filterBlacklistedGroups($this->result['groups']);
	}
	if (isset($this->result['exact']['groups'])) {
	$this->result['exact']['groups'] = $filterBlacklistedGroups($this->result['exact']['groups']);
	}

Since the filter logic is repeated, you can use an inline closure..... or define a function outside this method.

[T0mWz]

Thanks for all your feedback, really appreciated. 👍 😃
We will dive into it!