allow admin to disable sharing for specific groups of users #8599

schiessle · 2014-05-15T08:44:15Z

add admin setting to disable sharing for a some groups of users

@jancborchardt probably you also want to have a look at it from the user experience point of view.

karlitschek · 2014-05-15T08:46:21Z

nice 👍

DeepDiver1975 · 2014-05-15T08:47:29Z

apps/files_sharing/js/share.js

+			if (oc_appconfig.core.sharingDisabledForUser) {
+				var allShared = $('#fileList').find('[data-share-owner]');
+				var shareNotification = '<a class="action action-share-notification permanent" data-action="Share-Notification" href="#" original-title="">' +
+						' <img class="svg" src="/oc/core/img/actions/share.svg"></img>';


wrong path? /oc/core/img/actions/share.svg

DeepDiver1975 · 2014-05-15T08:54:10Z

@schiesbn please respect the 80 chars line length - makes reviews much easier - thanks a lot!

jancborchardt · 2014-05-15T12:55:31Z

Why introduce yet another mechanic for »add the option to enable apps only for specific groups«? We should simply use the stuff from @icewind1991 in pull #8264, right?

jancborchardt · 2014-05-15T12:55:43Z

(So, 👎 from me)

schiessle · 2014-05-15T13:16:35Z

This is something slightly different. We want to enable the sharing app but exclude certain groups from actively sharing stuff. We can't disable sharing completely for them because they should still be able to receive shares. This feature request comes from @MTRichards I'm sure he can elaborate on it if needed.

MTRichards · 2014-05-15T16:39:29Z

Goal here is just as said, there are situations where you want complete flexibility over the ability for a user to share, not a binary on or off for everyone. In some cases, you want to distribute content to a group of users without allowing them to also share on the system. You also want to be sure you can track their use of the system - so you know if they did receive the content, for example. So you are not just turning of sharing, rather you are turning off sharing for specific groups.

If a user is a member of a group that can't share and a group that can share, they can of course share.

schiessle · 2014-05-15T17:57:02Z

@DeepDiver1975 I think all your comments are addressed. The open questions can be addressed separately... Maybe you want to have a final look? Thanks!

DeepDiver1975 · 2014-05-16T05:47:48Z

Sure - asap this morning

jancborchardt · 2014-05-16T07:26:16Z

Ok, then we should have a clarification sentence below the setting saying »These groups will still be able to receive shares, but not initiate them.« (or something similar).

schiessle · 2014-05-16T08:37:20Z

@jancborchardt good point, I added it.

schiessle · 2014-05-16T16:24:09Z

rebased, ready for review again 😄

schiessle · 2014-05-19T15:36:52Z

@MorrisJobke maybe you can also give it a try? Thanks!

DeepDiver1975 · 2014-05-19T19:00:59Z

apps/files_sharing/js/share.js

+			// shared with him otherwise we just update the existing share action.
+			if (oc_appconfig.core.sharingDisabledForUser) {
+				var $fileList = $(this);
+				var allShared = $fileList.find('[data-share-owner]');


allShared is declared twice - here and in line 43

it's an if-else-statement. You can't execute both paths so it is always declared exactly once.

Well jshint is still crying ...

Von Samsung Mobile gesendet

-------- Ursprüngliche Nachricht --------
Von: Björn Schießle notifications@github.com
Datum:20.05.2014 00:02 (GMT+01:00)
An: owncloud/core core@noreply.github.com
Cc: Thomas Müller thomas.mueller@tmit.eu
Betreff: Re: [core] allow admin to disable sharing for specific groups of
users (#8599)

In apps/files_sharing/js/share.js:

@@ -25,13 +25,28 @@ $(document).ready(function() {
};

$('#fileList').on('fileActionsReady',function(){

var $fileList = $(this);

var allShared = $fileList.find('[data-share-owner] [data-Action="Share"]');

allShared.addClass('permanent');

allShared.find('span').text(function(){

var $owner = $(this).closest('tr').attr('data-share-owner');

return ' ' + t('files_sharing', 'Shared by {owner}', {owner: $owner});

});

// if no share action exists because the admin disabled sharing for this user

// we create a share notification action to inform the user about files

// shared with him otherwise we just update the existing share action.

if (oc_appconfig.core.sharingDisabledForUser) {

var $fileList = $(this);

var allShared = $fileList.find('[data-share-owner]');
it's an if-else-statement. You can't execute both paths so it is always declared exactly once.

—
Reply to this email directly or view it on GitHub.

Thing is, in Javascript all variables declared inside a sub-block always exist in the full-scope of the function. So declaring it twice as you did will be interpreted as duplicate. (yeah I know, it's weird). See http://code.tutsplus.com/tutorials/javascript-hoisting-explained--net-15092

Best practice is to always declare all variables at the top of functions, because this is what JS does internally.

DeepDiver1975 · 2014-05-19T19:17:24Z

Not working for me - I have one user and one group (both named admin)

I enable "Exclude groups from sharing" and I select no group.

Util::isSharingDisabledForUser() is still returning false because of

$remainingGroups = array_diff($usersGroups, $excludedGroups);

is still returning my group.

Proper unit tests could have easily detect this.

schiessle · 2014-05-19T21:57:24Z

Not working for me - I have one user and one group (both named admin)
I enable "Exclude groups from sharing" and I select no group.
Util::isSharingDisabledForUser() is still returning false because of

And that's correct. No group was excluded from sharing -> sharing isn't disabled for your user "admin" -> Util::isSharingDisabledForUser() returns false

DeepDiver1975 · 2014-05-20T05:22:22Z

In this case please excuse my comment. Maybe we need work on the description because I understood it quite the other way around.

Unit tests seem still valid ...

schiessle · 2014-05-20T08:27:53Z

@DeepDiver1975 feel free to provide a better description but I think "Exclude groups from sharing" and "These groups will still be able to receive shares, but not to initiate them." describes it quite well

schiessle · 2014-05-20T08:58:54Z

@DeepDiver1975 I added a explicit unit test for the method in OC\Util, although it was already tested indirectly by https://github.com/owncloud/core/pull/8599/files#diff-f66a31c05b175f42b73375da18b8eeaaR177

DeepDiver1975 · 2014-05-20T09:07:02Z

Awesome - Thanks!

PVince81 · 2014-05-21T09:33:59Z

Please rebase. (I guess it's probably related to the recent removal of "brief" in all comments...)

schiessle · 2014-05-21T10:23:18Z

rebased, ready for final review

ghost · 2014-05-21T21:21:29Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4928/

schiessle · 2014-05-22T07:48:58Z

@DeepDiver1975 the JSHint JavaScript warning is now gone too... Maybe you want to give a +1? 😉

PVince81 · 2014-05-22T08:03:33Z

apps/files_sharing/js/share.js

+					return shareNotification + '<span> ' + shareBy + '</span></span>';
+				});
+			} else {
+				allShared = $('#fileList').find('[data-share-owner] [data-Action="Share"]');


Next time please use $fileList, no more globals!
Your code should still work but I'll fix this for the sharing overview code.

PVince81 · 2014-05-22T08:17:51Z

Please have a look at my comments above.
I'll test it once they are addressed:

Here is a test plan:

when no excluded groups exist

check that all users can share

with group "egroup1" being the excluded group and "group1" a regular group

Add "user1" only to "group1", check that sharing is possible
Add "user1" only to "egroup1", check that sharing is not possible
Add "user1" to both "group1" and "egroup1", check that sharing is still possible
Add "user1" to "egroup1". Share a file with "user1". Check that resharing is not possible.

UI testing in settings page

Try adding/removing exclusion groups and refresh the page, check that they are there
Try removing all exclusion groups (empty string test), check that it's saved correctly

schiessle · 2014-05-22T08:42:10Z

@PVince81 all your comments are addressed

scrutinizer-notifier · 2014-05-22T09:09:30Z

The inspection completed: 23 new issues, 13 updated code elements

ghost · 2014-05-22T09:50:44Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4933/

PVince81 · 2014-05-22T10:38:58Z

settings/js/admin.js

+			if (element.data('userGroups')) {
+				checked = element.data('userGroups');
+			}
+			var checkHandeler = function(group) {


You forgot Chandeler 😉 (just leave it)

PVince81 · 2014-05-22T10:39:20Z

Great, going to test this.

PVince81 · 2014-05-22T10:56:50Z

I noticed that if I share a file as "user1" with "user2", then remove user1's permission to share, the shared file is still visible.

Is it possible to prevent mounting the shares when they come from an "unauthorized" user ? (with, I suspect, possible performance hits)

This is better than auto-deleting them because of this use case: it could happen that the admin added the user to the wrong group "egroup" by mistake, then puts him back to the correct authorized one "group1", without wanting the user to lose all his shares.

PVince81 · 2014-05-22T11:01:20Z

unauthorized user (user1 in egroup) can still receive shares
adding one user in two excluded groups also excludes properly

PVince81 · 2014-05-22T11:03:14Z

Small UX issue: when receiving a share from user2 as unauthorized user (user1 in egroup), the user sees "Shared by user2" which is correct. Clicking on it doesn't open the dropdown, it is correct, but what happens might confuse the user: it will trigger the default action. In my case it opened the text editor.
We might want to block the action altogether, not sure how.
Can be fixed in a separate PR as it's a minor issue but might need some research.

PVince81 · 2014-05-22T11:04:01Z

@schiesbn I agree to merge this after clearing out the point I raised here: #8599 (comment)

schiessle · 2014-05-22T11:30:18Z

Is it possible to prevent mounting the shares when they come from an "unauthorized" user ?

Probably this would be possible but I wonder if this is really needed? If a user shared a file at a point in time the admin allowed him to share files then it was a legitimate share. So why should we remove/hide it? I see this more as a forward-facing feature.

PVince81 · 2014-05-22T12:01:04Z

Yes, possibly. I just wanted to make sure that we are aware of this.

I think this can be merged already 👍

allow admin to disable sharing for specific groups of users

DeepDiver1975 reviewed May 15, 2014
View reviewed changes

MorrisJobke added the 5 - To review label May 19, 2014

DeepDiver1975 reviewed May 19, 2014
View reviewed changes

PVince81 reviewed May 22, 2014
View reviewed changes

allow admin to disable sharing for specific groups of users

12338e0

PVince81 reviewed May 22, 2014
View reviewed changes

schiessle pushed a commit that referenced this pull request May 22, 2014

Merge pull request #8599 from owncloud/sharing_disable_for_groups

050df76

allow admin to disable sharing for specific groups of users

schiessle merged commit 050df76 into master May 22, 2014

schiessle deleted the sharing_disable_for_groups branch May 22, 2014 12:19

lock bot locked as resolved and limited conversation to collaborators Aug 21, 2019

allow admin to disable sharing for specific groups of users #8599

allow admin to disable sharing for specific groups of users #8599

Conversation

schiessle commented May 15, 2014

karlitschek commented May 15, 2014

DeepDiver1975 May 15, 2014

Choose a reason for hiding this comment

DeepDiver1975 commented May 15, 2014

jancborchardt commented May 15, 2014

jancborchardt commented May 15, 2014

schiessle commented May 15, 2014

MTRichards commented May 15, 2014

schiessle commented May 15, 2014

DeepDiver1975 commented May 16, 2014

jancborchardt commented May 16, 2014

schiessle commented May 16, 2014

schiessle commented May 16, 2014

schiessle commented May 19, 2014

DeepDiver1975 May 19, 2014

Choose a reason for hiding this comment

schiessle May 19, 2014

Choose a reason for hiding this comment

DeepDiver1975 May 20, 2014

Choose a reason for hiding this comment

PVince81 May 21, 2014

Choose a reason for hiding this comment

DeepDiver1975 commented May 19, 2014

schiessle commented May 19, 2014

DeepDiver1975 commented May 20, 2014

schiessle commented May 20, 2014

schiessle commented May 20, 2014

DeepDiver1975 commented May 20, 2014

PVince81 commented May 21, 2014

schiessle commented May 21, 2014

ghost commented May 21, 2014

schiessle commented May 22, 2014

PVince81 May 22, 2014

Choose a reason for hiding this comment

PVince81 commented May 22, 2014

when no excluded groups exist

with group "egroup1" being the excluded group and "group1" a regular group

UI testing in settings page

schiessle commented May 22, 2014

scrutinizer-notifier commented May 22, 2014

ghost commented May 22, 2014

PVince81 May 22, 2014

Choose a reason for hiding this comment

PVince81 commented May 22, 2014

PVince81 commented May 22, 2014

PVince81 commented May 22, 2014

PVince81 commented May 22, 2014

PVince81 commented May 22, 2014

schiessle commented May 22, 2014

PVince81 commented May 22, 2014