Prevent group assignment in Web UI if not supported #38298

JammingBen · 2021-01-14T10:28:50Z

Description

This enhancement checks if users can be assigned to (or removed from) groups via Web UI. All group backends which do not support this functionality will be disabled in corresponding menus.

Related Issue

Fixes Error on adding "guest_app" group to user; No Feedback in UI guests#431

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

settings/js/users/users.js

settings/Controller/UsersController.php

jvillafanez · 2021-01-18T08:27:45Z

https://github.com/owncloud/core/pull/38298/files#diff-b229d762bcebc1e3804ce9733dc70e9dfbf5a1890eca0825dafd5e10b2ecafccR173 introduces a dependency from ownCloud to the app. Such dependency shouldn't happen.

We might need to include a new feature to solve this problem.

The group backend interface could expose some options to be implemented. One of them could be "allow adding only specific users". This option not only could force the guest group to allow only guest users, but also force ldap groups to have only ldap users (this isn't restricted at the moment).
The main goal would be for core to know what options are available and how core can interact with those options.

Although providing a good migration path to implement this feature will likely be a challenge (user_ldap and customgroups apps will be affected, maybe more), I think this will be a more consistent solution.

JammingBen · 2021-01-18T12:29:44Z

@jvillafanez
Thank's for your input regarding this! I looked into it and found the method getSupportedActions() for group backends, which already has this functionality, kind of. This way, the controller would check all supported actions for a specific group backend. Then the UI knows what actions to display and what not.

From my point of view, this issue is less about "Prevent user abc from being assigned to group xyz" and more about "Prevent group yxz from being assigned to any user via Web UI (and vice versa)". Plus we wouldn't need to change the interfaces.

JammingBen · 2021-01-18T13:29:12Z

@jvillafanez See https://github.com/owncloud/core/pull/38298/files#diff-b229d762bcebc1e3804ce9733dc70e9dfbf5a1890eca0825dafd5e10b2ecafccR166 for the above mentioned implementation. What do you think?

jvillafanez · 2021-01-18T13:54:55Z

It might fit if we assume that the operations to add and remove guest users are hidden. Basically, only the guest app is allowed to add or remove users from the guest group. I don't like too much relying on private stuff, but the solution could fit for this case.
My only worry is that I'm not fully sure how the guests are added to the group. For consistency, it should be the guest app the one adding the users via this private / internal call because core isn't allowed according to what the backend is reporting.

For now, I think it's worth to try

settings/Controller/GroupsController.php

…e groups for assignment

JammingBen · 2021-01-19T15:05:11Z

As discussed I implemented a getBackends method for the GroupManager. Should be far more efficient now.

Technically speaking we now have a breaking change as I had to extend the IGroupManager interface. Although I did not find any OC apps implementing it... Let's wait for the pipeline first.

lib/private/Group/Manager.php

settings/Controller/GroupsController.php

jvillafanez · 2021-01-19T15:31:05Z

settings/js/users/users.js

+					return;
+				}
+
+				$groupsSelect.append($('<option value="' + escapeHTML(group) + '">' + escapeHTML(group) + '</option>'));


Maybe I'm missing something, but it seems the subadmin can bypass the restrictions. I don't think it's a good idea that the subadmin can have more power than the admin.

As far as I can see, this is only used for the second group menu "Group Admin for" (=subadmin). Whereas a user cannot be assigned to the guest-group manually, the user can be assigned as its group admin. So we need to bypass the restriction here.

Ok, got it. I'm not sure if we can make it more clear... I'm pretty sure I'll ask myself the same question again the next time I go through this code.

I at least added a comment for now

settings/js/users/users.js

phil-davis · 2021-01-20T03:52:23Z

Technically speaking we now have a breaking change as I had to extend the IGroupManager interface. Although I did not find any OC apps implementing it... Let's wait for the pipeline first.

I can see lots of apps that reference it, for example https://github.com/owncloud/activity/blob/master/lib/FilesHooks.php#L82

But the change is an extension of the interface, and the object seems to be injected via the constructor. So the extended object should get passed in fine, and the apps should not complain that there is a bonus method.

After this is merged, the nightly CI of every app will run all the app unit tests etc, so we will know the next day if there is any app to change.

JammingBen · 2021-01-20T07:22:19Z

I can see lots of apps that reference it, for example https://github.com/owncloud/activity/blob/master/lib/FilesHooks.php#L82

But the change is an extension of the interface, and the object seems to be injected via the constructor. So the extended object should get passed in fine, and the apps should not complain that there is a bonus method.

Right, but that's just an injection of the Group Manager which should be fine. Implementing the interface in another class (like the GroupManager here https://github.com/owncloud/core/blob/master/lib/private/Group/Manager.php#L61) would be a problem as it would also require the newly added method getBackends.

jvillafanez · 2021-01-20T09:07:53Z

settings/js/users/users.js

+				return;
+			}
+
+			var groupIsChecked = new Set(checked).has(group) === true;


Please double-check if we still support IE because it doesn't support this way of initialization.
In addition, the === true isn't needed.

Also, the idea I had is that the checked variable (the assignableGroups and removableGroups too) were a set already. I didn't want to keep traversing the whole list several times, but this solution will still do it in order to create the set.

Ooops yeah, you're totally right. Should create the sets before that loop.

Still, IE is an issue as OC supports IE11. Since I'm not a JS expert, I did some research on it. The proper way to convert an array to a set (supporting IE) seems to be to loop through the array and call set.add(el)... not sure about this. I think it requires some testing which way is more efficient at the end.

You're getting a list with the assignable and removable groups in https://github.com/owncloud/core/pull/38298/files#diff-89fda7202f65d0e3ad5878d84f67bb8d2f49da91cbe9efb1eebebf0228dc0e4cR672-R673 . There is where you should convert that list into a set. You'll have to traverse the list once to add the items to the set, but that's probably not different of what the "good" constructor would do it for you.

After that, that variable can be used as a set here, which should perform better.

Okay, I did some tests with ~200 groups. It's indeed faster that way. Not by a significant amount, bit still.

jvillafanez · 2021-01-21T08:22:11Z

Ok, nothing more to add from me.
I think there is planned a 10.6.1 release, which means this will have to wait to be merged.

phil-davis · 2021-01-21T11:04:44Z

I think there is planned a 10.6.1 release, which means this will have to wait to be merged.

There is lots of other stuff already merged in master that will already cause a minor version bump to 10.7.0 - so IMO stuff like this can be merged when working. @micbar please confirm.

sonarcloud · 2021-01-21T11:54:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

jnweiger · 2021-03-18T02:16:08Z

Confirmed in 10.7.0RC1

guest user cannot be removed from group guest_app, ✔️
admin user cannot be added to user guest_app. ✔️

Prevent migration from normal users to guest users and vice versa in UI

e8df97f

JammingBen self-assigned this Jan 14, 2021

owncloud deleted a comment from update-docs bot Jan 14, 2021

Add changelog file

2105d05

JammingBen force-pushed the issues/guest-group-fix branch from 5c30fa1 to 2105d05 Compare January 14, 2021 10:39

Fix tests

066542d

JammingBen added the 3 - To Review label Jan 14, 2021

JammingBen requested review from phil-davis, AlexAndBear, jvillafanez and mrow4a January 14, 2021 14:47

jvillafanez reviewed Jan 14, 2021

View reviewed changes

settings/js/users/users.js Outdated Show resolved Hide resolved

settings/Controller/UsersController.php Outdated Show resolved Hide resolved

JammingBen requested a review from jvillafanez January 15, 2021 10:31

JammingBen force-pushed the issues/guest-group-fix branch from e1adb65 to fdcf96b Compare January 15, 2021 10:53

Add ajax call to get the guest group name

4c5d61f

JammingBen force-pushed the issues/guest-group-fix branch from fdcf96b to 4c5d61f Compare January 15, 2021 10:59

Fix acceptance tests

1f978cf

JammingBen added 2 - Developing and removed 3 - To Review labels Jan 18, 2021

Use getSupportedActions() to get all available actions in WebUI

d7c714e

jvillafanez reviewed Jan 18, 2021

View reviewed changes

settings/Controller/GroupsController.php Outdated Show resolved Hide resolved

settings/Controller/GroupsController.php Outdated Show resolved Hide resolved

Implement getBackends method for groupManager, use it to get availabl…

5792f29

…e groups for assignment

JammingBen changed the title ~~Prevent migration from normal users to guest users and vice versa in UI~~ Prevent group assignment in Web UI if not supported Jan 19, 2021

jvillafanez reviewed Jan 19, 2021

View reviewed changes

Minor improvements

306d7e6

JammingBen requested a review from jvillafanez January 20, 2021 08:46

jvillafanez reviewed Jan 20, 2021

View reviewed changes

Minor improvements

6fa0bc7

JammingBen requested a review from jvillafanez January 20, 2021 11:20

JammingBen added 3 - To Review and removed 2 - Developing labels Jan 21, 2021

AlexAndBear approved these changes Jan 21, 2021

View reviewed changes

JammingBen added 4 - To release and removed 3 - To Review labels Jan 25, 2021

AlexAndBear merged commit 839ed86 into master Jan 28, 2021

delete-merged-branch bot deleted the issues/guest-group-fix branch January 28, 2021 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent group assignment in Web UI if not supported #38298

Prevent group assignment in Web UI if not supported #38298

JammingBen commented Jan 14, 2021 •

edited

Loading

jvillafanez commented Jan 18, 2021

JammingBen commented Jan 18, 2021

JammingBen commented Jan 18, 2021

jvillafanez commented Jan 18, 2021 •

edited

Loading

JammingBen commented Jan 19, 2021

jvillafanez Jan 19, 2021

JammingBen Jan 20, 2021

jvillafanez Jan 20, 2021

JammingBen Jan 20, 2021

phil-davis commented Jan 20, 2021

JammingBen commented Jan 20, 2021

jvillafanez Jan 20, 2021

jvillafanez Jan 20, 2021

JammingBen Jan 20, 2021 •

edited

Loading

jvillafanez Jan 20, 2021

JammingBen Jan 20, 2021

jvillafanez commented Jan 21, 2021

phil-davis commented Jan 21, 2021

sonarcloud bot commented Jan 21, 2021

jnweiger commented Mar 18, 2021

Prevent group assignment in Web UI if not supported #38298

Prevent group assignment in Web UI if not supported #38298

Conversation

JammingBen commented Jan 14, 2021 • edited Loading

Description

Related Issue

Screenshots (if appropriate):

Types of changes

Checklist:

jvillafanez commented Jan 18, 2021

JammingBen commented Jan 18, 2021

JammingBen commented Jan 18, 2021

jvillafanez commented Jan 18, 2021 • edited Loading

JammingBen commented Jan 19, 2021

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

phil-davis commented Jan 20, 2021

JammingBen commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JammingBen Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Jan 21, 2021

phil-davis commented Jan 21, 2021

sonarcloud bot commented Jan 21, 2021

jnweiger commented Mar 18, 2021

JammingBen commented Jan 14, 2021 •

edited

Loading

jvillafanez commented Jan 18, 2021 •

edited

Loading

JammingBen Jan 20, 2021 •

edited

Loading