Use select2 for the groups excluded from sharing in admin page #10968

PVince81 · 2014-09-09T15:11:15Z

First commit moves select2 to core
Second commit uses select2 in the admin page for the groups selection:

Please review @schiesbn @owncloud/designers @butonic @MTRichards @LukasReschke

Fixes #10936
Fixes second part of #9014

If you guys like it I'll do the same for the apps page, next.

PVince81 · 2014-09-09T15:12:45Z

CC @ser72 this should fix the admin page slowness, check it out!

ser72 · 2014-09-09T15:40:48Z

@butonic @PVince81 @MTRichards

Tested with an LDAP consisting of 10000 users.
Admin page came up very quickly with External Storage app enabled 👍

Admin page hangs with Sharepoint integration, and I assume it will with Windows Network Drive as well, however I am having issues enabling that app. 👎

ghost · 2014-09-09T15:47:54Z

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

ghost · 2014-09-09T15:47:57Z

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

PVince81 · 2014-09-09T16:01:53Z

@ser72 you have issue enabling the app because the app page is slow ?
I'll soon push the second part of the fix: putting the same select box on the apps page.

PVince81 · 2014-09-09T16:03:17Z

Added select2 on the apps page as well.

Fixes #9012

Will rebase now.

The ajax call is now using ajax/appconfig.php instead

Moved setupGroupsSelect() from admin.js to a common settings.js as OC.Settings.setupGoupsSelect(). Now using select2 as well on the apps page.

PVince81 · 2014-09-09T16:07:30Z

Rebased.

@ser72 please try again: the apps page should be more responsive now.

Please review @icewind1991 @schiesbn @butonic @owncloud/designers

Two things to look at now:

"Exclude group groups" in admin page, groups select box
Enable an app for specific groups: groups select box

Have fun 😄

ghost · 2014-09-09T16:20:59Z

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

LukasReschke · 2014-09-09T21:00:27Z

settings/js/settings.js

+					callback(selection);
+				},
+				formatResult: function (element) {
+					return element.displayname;


💣 💣 💣

👎 XSS detected - you need to escape this. (escapeHTML(element.displayname))

💣 💣 💣

LukasReschke · 2014-09-09T21:08:01Z

When adding a new group to the exclude from sharing dropdown two requests are send to appconfig.php - the first one results in an error 500
The exclude from share checkbox does not work with groups that contain a comma in the name
Enable app for specific group dropdown does not work with groups that contain a comma in the name

PVince81 · 2014-09-10T09:01:02Z

@LukasReschke select2 already escapes this internally, that's why I removed the escapeHTML things. From the docs:

String escapeMarkup(String markup)

Function used to post-process markup returned from formatter functions. By default this function escapes html entities to prevent javascript injection.

http://ivaynberg.github.io/select2/

But I'll double check with a debugger.

I didn't know group names could have a comma !? I'm pretty sure that will break other parts of OC as well. Is there a safer separator ? (select2 can work with a configurable separator)

LukasReschke · 2014-09-10T09:03:55Z

@LukasReschke select2 already escapes this internally, that's why I removed the escapeHTML things. From the docs:

Well, it doesn't change anything about the fact that this is exploitable. Do not trust the docs ;-)

LukasReschke · 2014-09-10T09:04:11Z

I didn't know group names could have a comma !? I'm pretty sure that will break other parts of OC as well. Is there a safer separator ? (select2 can work with a configurable separator)

What about JSON serialize the data?

PVince81 · 2014-09-10T09:22:41Z

The dumb thing is that if I manually escape markup in "formatResults()" I'll need to override "escapeMarkup()" from select2 to disable escaping... else it will double escape. Is that what we want ?

select2 still needs a separator to be able to parse the initial value from the hidden "input" field and also to be able to store the values back there.

PVince81 · 2014-09-10T10:51:13Z

The format of "exclude groups" in the config is currently comma separated.
If I change it I'll need to change the sharing code as well.
As this is blowing this PR out of proportions, I'll fix it separately first, then merge, then continue this one here.

PVince81 · 2014-09-10T11:09:49Z

Raised comma=>json task as #10983

Added explicit escaping. Now internally using a pipe symbol as separator for select2, to make it possible to use group names containing commas.

PVince81 · 2014-09-10T12:32:11Z

To avoid having to make complex changes that affect other parts of ownCloud I settled on using a pipe symbol internally to make it work with select2.
So for now:

I fixed the XSS issue with explicit escaping
Groups with commas now work on the apps page because it already uses JSON internally
Groups with comma will not work on the "exclude groups from sharing" because it does not use JSON internally: should be addressed as part of Exclude with groups cannot work with groups containing commas #10983
Groups with pipe will cause trouble, but I expect such to not be a common use case. Select2 doesn't work with a JSON structure and requires a comma (or other) separated list of values. I won't patch select2.
I'm not getting any 500 error for appconfig.php, possibly fixed.

Please review/try again.
Thanks.

ghost · 2014-09-10T12:50:00Z

💣 Test Failed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org/job/pull-request-analyser/7190/

LukasReschke · 2014-09-10T15:30:59Z

👍

@th3fallen Please review. Thanks!

PVince81 · 2014-09-10T15:43:42Z

Also @owncloud/designers please confirm that it looks ok UX-wise

jancborchardt · 2014-09-10T16:10:15Z

I fixed the select box style to fit into the ownCloud style. @owncloud/designers please review.

ghost · 2014-09-10T17:37:17Z

💣 Test Failed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org/job/pull-request-analyser/7204/

To avoid making a server request every time the dropdown opens, the whole list of groups are cached (from the last request): Whenever the user types in a search term it will still send server requests.

scrutinizer-notifier · 2014-09-11T09:13:44Z

A new inspection was created.

PVince81 · 2014-09-11T09:14:45Z

I was worried that select2 was making too many ajax requests so I've added a local JS cache that contains all groups. This way when opening the dropdown again it will not redo the request.

However if you type in search terms it will make ajax requests (uncached).

PVince81 · 2014-09-16T09:51:22Z

@blizzz I saw in some other issue/PR related to the share autocomplete (the one that returns only 15 entries) that it is not good to use the $search argument.
This PR here also uses the search when the user types in a group name.

Do you think that issue would apply here as well ?

blizzz · 2014-09-16T14:26:09Z

I have not seen a corresponding line here, or is it included in /settings/ajax/grouplist? In general yes, the group backend cannot properly search users, only user backends can. We should look into a more general solution for it.

PVince81 · 2014-09-16T14:45:21Z

@blizzz I haven't checked the backend code, am just using this endpoint and but the search seems to work. So it's probably filtered by our own code, not the backend ?

blizzz · 2014-09-17T10:27:00Z

On a second glance i don't see that users are search for at all, also not in settings/ajax/grouplist. Searching groups using the group backend is of course totally correct.

PVince81 · 2014-09-18T09:43:14Z

@blizzz if you don't see anything against this ticket then we can merge it ?

blizzz · 2014-09-19T10:00:15Z

reviewed and tested 👍

blizzz · 2014-09-19T10:01:39Z

@owncloud-bot retest this please

ghost · 2014-09-19T14:45:34Z

🚀 Test Passed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org/job/pull-request-analyser/7475/

Use select2 for the groups excluded from sharing in admin page

LukasReschke · 2014-09-19T14:51:15Z

@karlitschek Another goldy that is crying "Backport me, please!" ;-)

karlitschek · 2014-09-19T20:20:21Z

yes. please backport

PVince81 · 2014-09-22T08:15:06Z

I'm on it.

PVince81 · 2014-09-22T08:25:54Z

Backported to stable7 as 97b83b9..c16c680

I also had a quick test and saw that it still works on that branch 😄

PVince81 added this to the 2014-sprint-03-current milestone Sep 9, 2014

Vincent Petry added 3 commits September 9, 2014 18:03

Move select2 from files_external to core

412da87

Now using select2 for the groups excluded from sharing

d236215

Load select2 on admin page

4a27f4b

Vincent Petry added 2 commits September 9, 2014 18:04

Remove excludegroup.php

39f5580

The ajax call is now using ajax/appconfig.php instead

Added select2 on the apps page

0d28ba0

Moved setupGroupsSelect() from admin.js to a common settings.js as OC.Settings.setupGoupsSelect(). Now using select2 as well on the apps page.

PVince81 force-pushed the admin-groupsselect2 branch from bde9641 to 0d28ba0 Compare September 9, 2014 16:05

LukasReschke reviewed Sep 9, 2014
View reviewed changes

LukasReschke added the 3 - To Review label Sep 9, 2014

PVince81 mentioned this pull request Sep 10, 2014

Exclude with groups cannot work with groups containing commas #10983

Closed

Fixed select2 for admin and apps page

a064536

Added explicit escaping. Now internally using a pipe symbol as separator for select2, to make it possible to use group names containing commas.

PVince81 mentioned this pull request Sep 10, 2014

Users page loads all groups, make page unaccessible, i.e. not possible to manage users #9201

Closed

PVince81 mentioned this pull request Sep 10, 2014

[WIP] Ajax load groups in users page #10994

Closed

3 tasks

fit select2 to ownCloud input style

8d0d336

Add select2 cache for complete group list

71776e3

To avoid making a server request every time the dropdown opens, the whole list of groups are cached (from the last request): Whenever the user types in a search term it will still send server requests.

craigpg modified the milestones: 2014-sprint-04-current, 2014-sprint-03 Sep 15, 2014

PVince81 mentioned this pull request Sep 17, 2014

[stable7] Exclude group from shares dropdown broken #10936

Closed

LukasReschke added a commit that referenced this pull request Sep 19, 2014

Merge pull request #10968 from owncloud/admin-groupsselect2

fed8100

Use select2 for the groups excluded from sharing in admin page

LukasReschke merged commit fed8100 into master Sep 19, 2014

LukasReschke deleted the admin-groupsselect2 branch September 19, 2014 14:50

PVince81 mentioned this pull request Oct 7, 2014

Apps page pulls in all groups at once, can cause long loading time #9012

Closed

phil-davis mentioned this pull request Jun 20, 2018

Group names containing vertical bar cannot be used in "exclude groups from sharing" #31836

Closed

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

Use select2 for the groups excluded from sharing in admin page #10968

Use select2 for the groups excluded from sharing in admin page #10968

Conversation

PVince81 commented Sep 9, 2014

PVince81 commented Sep 9, 2014

ser72 commented Sep 9, 2014

ghost commented Sep 9, 2014

ghost commented Sep 9, 2014

PVince81 commented Sep 9, 2014

PVince81 commented Sep 9, 2014

PVince81 commented Sep 9, 2014

ghost commented Sep 9, 2014

LukasReschke Sep 9, 2014

Choose a reason for hiding this comment

LukasReschke commented Sep 9, 2014

PVince81 commented Sep 10, 2014

LukasReschke commented Sep 10, 2014

LukasReschke commented Sep 10, 2014

PVince81 commented Sep 10, 2014

PVince81 commented Sep 10, 2014

PVince81 commented Sep 10, 2014

PVince81 commented Sep 10, 2014

ghost commented Sep 10, 2014

LukasReschke commented Sep 10, 2014

PVince81 commented Sep 10, 2014

jancborchardt commented Sep 10, 2014

ghost commented Sep 10, 2014

scrutinizer-notifier commented Sep 11, 2014

PVince81 commented Sep 11, 2014

PVince81 commented Sep 16, 2014

blizzz commented Sep 16, 2014

PVince81 commented Sep 16, 2014

blizzz commented Sep 17, 2014

PVince81 commented Sep 18, 2014

blizzz commented Sep 19, 2014

blizzz commented Sep 19, 2014

ghost commented Sep 19, 2014

LukasReschke commented Sep 19, 2014

karlitschek commented Sep 19, 2014

PVince81 commented Sep 22, 2014

PVince81 commented Sep 22, 2014