-
Notifications
You must be signed in to change notification settings - Fork 3
Display group display name instead of gid #28
base: master
Are you sure you want to change the base?
Conversation
20b5161
to
f397652
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few notes. I will try this later (today?)
if (!empty($groups)) { | ||
foreach ($groups as $key => $group) { | ||
// Is probably subadmin | ||
// FIXME: Can not admin actually create users ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is building the list of groups that a subadmin has "control" over. In the webUI user management page, a subadmin only sees the groups that they are subadmin of. So they can only adjust membership of those.
Also, they cannot remove a user from all groups that the subadmin has "control" of. i.e. a subadmin cannot "lose control" of a user in their group(s). But a subadmin who is subadmin of multiple groups can add/move users between those groups.
The UI enforces that OK (before this PR - I will test this PR soon to see if there is any regression). We should check the private API used underneath here, to make sure a subadmin cannot "hack" add extra groups in their browser console and then get any level of control over those groups.
$items = array_flip($userlistParams['subadmingroups']); | ||
unset($items['admin']); | ||
$userlistParams['subadmingroups'] = array_flip($items); | ||
for($i=0; $i < count($allGroups); $i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: need to check where this subadmin groups array ends up being used. The code here looks like it might show a lot of subadmin groups. Maybe that is OK - it will be all the groups that some user could be made a subadmin of.
f397652
to
5bd4c3c
Compare
@phil-davis @PVince81 Ok, there were not many changes required. This implementation should work, but needs some unit and integration tests to be passed. |
Proof of concept try-out:
How did I test? Created guest user with - owncloud/guests#200 - Should work also with customgroups.