Allow admin to blacklist groups for the files_sharing app #31672

jvillafanez · 2018-06-07T08:23:59Z

Description

Allow admins to blacklist groups so user can't share with those groups. Blacklist will be done per backend + group displayname for now.

Related Issue

#31578

Motivation and Context

How Has This Been Tested?

Manually tested for now

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2018-06-07T12:20:14Z

Codecov Report

Merging #31672 into master will increase coverage by 0.01%.
The diff coverage is 91.3%.

@@             Coverage Diff              @@
##             master   #31672      +/-   ##
============================================
+ Coverage     63.16%   63.17%   +0.01%     
- Complexity    18444    18461      +17     
============================================
  Files          1158     1161       +3     
  Lines         69284    69327      +43     
  Branches       1260     1260              
============================================
+ Hits          43760    43800      +40     
- Misses        25155    25158       +3     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.39% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.41% <91.3%> (+0.01%)`	`18461 <14> (+17)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
apps/files_sharing/lib/API/OCSShareWrapper.php	`5.71% <0%> (-0.17%)`	`9 <0> (ø)`
...files_sharing/lib/Controller/ShareesController.php	`89.87% <100%> (+0.08%)`	`102 <0> (+2)`	⬆️
apps/files_sharing/lib/API/Share20OCS.php	`93.41% <100%> (+0.04%)`	`183 <0> (+1)`	⬆️
apps/files_sharing/templates/settings.php	`100% <100%> (ø)`	`0 <0> (?)`
...s/files_sharing/lib/Panels/Admin/SettingsPanel.php	`100% <100%> (ø)`	`4 <4> (?)`
apps/files_sharing/lib/SharingBlacklist.php	`92.3% <92.3%> (ø)`	`10 <10> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9edd97a...bf8f99d. Read the comment docs.

PVince81

See review comments.

I'm not convinced about the need to have a cache here.

PVince81 · 2018-06-07T20:04:52Z

apps/files_sharing/js/settings.js

+ */
+
+$(document).ready(function(){
+	$('#files_sharing .files_sharing_settings').change(function(){


so the generic functions didn't work ? might be possible to adjust the generic existing JS function for saving settings to include textarea additionally to input

I'm not aware of any generic function to save values in the appconfig table. I've just found the OC.AppConfig.setValue function to perform the operation (code already adjusted to use it, soon), but there isn't auto binding, so I'll need to bind the event manually.

PVince81 · 2018-06-07T20:06:19Z

apps/files_sharing/lib/API/Share20OCS.php

@@ -396,6 +401,9 @@ public function createShare() {
 				$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
 				return new \OC\OCS\Result(null, 404, $this->l->t('Please specify a valid group'));
 			}
+			if ($this->sharingBlacklist->isGroupBlacklisted($this->groupManager->get($shareWith))) {
+				return new \OC\OCS\Result(null, 403, $this->l->t('The group is blacklisted for sharing'));


not sure if this qualifies as information disclosure, maybe just return 404 "Please specify a valid group" as above

cc @settermjd

Honestly? I'm don't believe that this is information disclosure. However, something more generic, yet still informative, such as "This group is not available for sharing, would work better.

PVince81 · 2018-06-07T20:07:13Z

apps/files_sharing/lib/Controller/SettingsController.php

+	 * Set with the known configuration keys. Only these keys should be able to be set and retrieved
+	 */
+	private $knownKeys = [
+		'blacklisted_group_displaynames' => true,


looks like early optimization/refactoring before it is even needed, but ok

I'll remove the class because get and set for the appconfig table will go through core (using OC.Appconfig in js). No need to handle this on our own.

PVince81 · 2018-06-07T20:08:17Z

apps/files_sharing/lib/Panels/Admin/SettingsPanel.php

+		}, $groupBackendList);
+
+		$tmpl = new Template('files_sharing', 'settings');
+		$tmpl->assign('backendNames', $backendNames);


great that you put this in 👍

PVince81 · 2018-06-07T20:08:54Z

apps/files_sharing/lib/SharingBlacklist.php

+use OCP\IConfig;
+use OCP\IGroup;
+
+class SharingBlacklist {


Add PHPDoc. Is this the actual black list array or just a utility class ? This is what a developer wonders about when seeing only the class name used in various places

PVince81 · 2018-06-07T20:09:12Z

apps/files_sharing/lib/SharingBlacklist.php

+
+	/**
+	 * Check if the target group is blacklisted
+	 * @param IGroup $group the group to check


missing @return

PVince81 · 2018-06-07T20:10:51Z

apps/files_sharing/lib/SharingBlacklist.php

+		$blacklistedComponents = explode("\n", $configuredBlacklist);
+
+		$result = [];
+		foreach ($blacklistedComponents as $blacklistedComponent) {


is this operation so expensive that we need a cache with all complexity that comes with it ?

in general I don't think we expect more than 10 or so blacklisted groups

For the sharees we'll need to check if each group is blacklisted. This leads potentially to a lot of calls to this function.
Since the blacklist will be stored as a large string, each call will need to fetch the string and process it in order to make the decission. This might be heavy we need to check a lot of groups.
In addition, I'd rather not rely on the appconfig to be cached to have a decent performance.

I also don't expect a lot of blacklisted groups, but we might need to include more options for the blacklist, such as blacklist by group id, and I'd prefer the check to be as fast as possible.

Ok, fair enough. Make sure the unit tests also take the cache into account and prove that it properly invalidates, if applicable.

PVince81 · 2018-06-07T20:13:15Z

apps/files_sharing/templates/settings.php

+				<?php endforeach; ?>
+			</ul>
+		</div>
+		<textarea placeholder="OC\Group\Database::localGroups&#10OCA\User_LDAP\Group_Proxy::ldapGroup" class="files_sharing_settings" name="blacklisted_group_displaynames"><?php p($_['blacklistedDisplaynames']); ?></textarea>


what's this weird &#10 entity here ? likely a newline. I suggest to put this in a template variable instead and initialize it from the PHP template in which you can write it in a more readable format that includes \n

jvillafanez · 2018-06-08T13:27:00Z

Code should be mostly ready, but there are tests missing for the SettingsPanel.php and SharingBlacklist.php and the getBackends method of the IGroupManager

mmattel · 2018-06-11T17:28:00Z

Documentation relevant

tomneedham · 2018-06-12T14:28:52Z

apps/files_sharing/lib/SharingBlacklist.php

+		$groupBackend = \get_class($group->getBackend());
+		$groupDisplayname = $group->getDisplayName();
+
+		if (isset($this->blacklistCache['displaynames'][$groupBackend][$groupDisplayname])) {


I am still concerned we do the matching on the displaynames. Can't we store ID's but allow admins to specify displaynames?

behaviour

jvillafanez · 2018-06-12T16:28:27Z

Backport to stable10 in #31740

PVince81

apart from the settings cache this looks good

PVince81 · 2018-06-15T13:33:48Z

apps/files_sharing/lib/SharingBlacklist.php

+	}
+
+	private function fetchBlacklistedReceiverGroupIds() {
+		$configuredBlacklist = $this->config->getAppValue('files_sharing', 'blacklisted_receiver_groups', '[]');


since we now removed the class names from the group ids, maybe we don't need the cache any more ?

based on @tomneedham's comment here #31578 (comment) we can use the same approach as "exclude groups from sharing" which also doesn't expect a higher number of groups than the blacklist.

also now the logic for reading the settings has simplified itself since we don't store the prefix any more

@jvillafanez can you remove the cache and read directly from the setting ?

I think we'll still need this. The problem is with sharees: we'll fetch a list of groups and each group needs to be checked whether is blacklisted or not; without proper caching we'll need to fetch the list from the configuration and then parse the list for each group.
Even if we rely on the configuration being cached, we still need to parse the stored json several times.

is there no way to change the caller code then to only fetch the blacklist array once ?

We might use a filterBlacklistedGroups(array $groups) function, but the calls would be a bit weird:

The share API just needs to check one group, so you'll need to wrap it in an array, which feels bad.

For the sharees, the intention would be to return the groups that aren't blacklisted. However, in case we need to do something with the blacklisted groups (maybe log them) things will get complicated both for the caller and the implementation. Note that, in any case, we'll need to traverse the sharees list twice.

I agree the code is a bit complex, but it should be fine if this complexity is isolated in this class. On the other hand, the usage is simple. Maybe we can document in the class what is the expected usage.

I was more thinking that the caller code calls "give me all the blacklisted groups", store it in a local var and then use that when checking against each group. Unless the caller codes is spread across various classes, this could work.

You'll move the complexity to the callers. you still need to compare the target group with the blacklist. This comparison code will be spread into several classes, and the way you'll likely compare will use the in_array function instead of more efficients approaches (you don't want to add more complexity to the caller).

Current code has a faster performance an a cleaner interface at a cost of including a cache.

Current code has a faster performance an a cleaner interface at a cost of including a cache.

Okay, let's leave it then

PVince81

Code looks good.

What happens if shares exist that were shared with a group that has now been blacklisted ? I assume these must still appear in the shares list and be unshareable by users. I don't think we need to add a mechanism to also exclude existing shares/mounts that point to blacklisted groups.

@tomneedham thoughts on the latter ?

PVince81 · 2018-06-21T08:10:32Z

@jvillafanez let's move on with this. The additional concern, if addressed in any way, will not affect the code of this PR.

PVince81 · 2018-06-21T08:11:27Z

I've added a comment here to address as part of the ticket/concept before we close it: #31578 (comment)

PVince81 · 2018-06-21T08:11:57Z

@jvillafanez please adjust the backport PR and ping me there when done

settermjd · 2018-07-26T11:23:10Z

apps/files_sharing/lib/SharingBlacklist.php

+
+/**
+ * Class to handle a blacklist for sharing. The main functionality is to check if a particular group
+ * has been blacklisted for sharing, which means that noone should share with that group.


"should share", or be "should able to share"?

settermjd · 2018-07-26T11:34:22Z

apps/files_sharing/lib/Controller/ShareesController.php

@@ -284,7 +290,7 @@ protected function getGroups($search) {
 		foreach ($groups as $group) {
 			// FIXME: use a more efficient approach


I can't think of a more efficient approach off the top of my head. However, you could use an implementation of a FilterIterator to simplify the logic here, making it that much simpler to maintain over time.

settermjd · 2018-07-26T11:54:46Z

apps/files_sharing/tests/SharingBlacklistTest.php

+		return [
+			[$groupMock1, '["Mygroup"]'],
+			[$groupMock1, '["Mygroup", "my_other_group"]'],
+			[$groupMock1, '["one group", "Mygroup"]'],


Isn't this, effectively, the same as the line above?

There were issues in various places with group names that contain a space - maybe that was a concern here?

lock · 2019-07-30T10:02:05Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jvillafanez added 2 - Developing feature:sharing labels Jun 7, 2018

jvillafanez added this to the development milestone Jun 7, 2018

jvillafanez self-assigned this Jun 7, 2018

jvillafanez mentioned this pull request Jun 7, 2018

Prevent sharing with system groups #31578

Open

3 tasks

jvillafanez force-pushed the sharing_blacklist_groups branch from 1d21dda to 28c782d Compare June 7, 2018 12:36

PVince81 reviewed Jun 7, 2018

View reviewed changes

jvillafanez added 7 commits June 12, 2018 10:42

Allow admin to blacklist groups for the files_sharing app

261f8e6

Show available group backends

962d653

Change settings to use core, adjust code comments and trim white spaces

b6f3156

Adjust code to pass the tests

94b679a

Add new tests for group blacklist

09f2322

Add tests for the new feature

482e659

Swap position between the text are and the backend list

87bfefd

tomneedham reviewed Jun 12, 2018

View reviewed changes

jvillafanez force-pushed the sharing_blacklist_groups branch from 6061a60 to 5364d96 Compare June 12, 2018 14:47

jvillafanez added 2 commits June 12, 2018 17:27

Change the implementation to match the "exclude groups from sharing"

8c4d26d

behaviour

Remove unnneded changes

bf8f99d

jvillafanez force-pushed the sharing_blacklist_groups branch from 5364d96 to bf8f99d Compare June 12, 2018 15:28

jvillafanez changed the title ~~[WIP] Allow admin to blacklist groups for the files_sharing app~~ Allow admin to blacklist groups for the files_sharing app Jun 12, 2018

jvillafanez added 3 - To Review and removed 2 - Developing labels Jun 12, 2018

jvillafanez mentioned this pull request Jun 12, 2018

[stable10] Sharing blacklist groups stable10 #31740

Merged

9 tasks

PVince81 suggested changes Jun 15, 2018

View reviewed changes

PVince81 reviewed Jun 19, 2018

View reviewed changes

PVince81 approved these changes Jun 21, 2018

View reviewed changes

PVince81 merged commit 2df5b94 into master Jun 21, 2018

PVince81 deleted the sharing_blacklist_groups branch June 21, 2018 08:11

phil-davis mentioned this pull request Jun 21, 2018

Fix SharingBlacklistTest setGetBlacklistedReceiverGroupsProvider #31867

Merged

9 tasks

settermjd reviewed Jul 26, 2018

View reviewed changes

mmattel mentioned this pull request Nov 19, 2018

Allow admin to blacklist groups for the files_sharing app owncloud/docs#288

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019

		@@ -284,7 +290,7 @@ protected function getGroups($search) {
		foreach ($groups as $group) {
		// FIXME: use a more efficient approach

Allow admin to blacklist groups for the files_sharing app #31672

Allow admin to blacklist groups for the files_sharing app #31672

Conversation

jvillafanez commented Jun 7, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Jun 7, 2018 • edited Loading

Codecov Report

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Jun 8, 2018

mmattel commented Jun 11, 2018

Choose a reason for hiding this comment

jvillafanez commented Jun 12, 2018

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Jun 21, 2018

PVince81 commented Jun 21, 2018

PVince81 commented Jun 21, 2018

settermjd Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Jul 30, 2019

jvillafanez commented Jun 7, 2018 •

edited

Loading

codecov bot commented Jun 7, 2018 •

edited

Loading

settermjd Jul 26, 2018 •

edited

Loading