Deny admin access when system config is set #273
Conversation
DescriptionThe idea here is to deny admin access the custom groups. Inorder to acheive this in this PR I have added a system config Issue
Testing done
|
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
============================================
+ Coverage 84.67% 84.75% +0.08%
- Complexity 315 318 +3
============================================
Files 22 22
Lines 985 997 +12
============================================
+ Hits 834 845 +11
- Misses 151 152 +1
Continue to review full report at Codecov.
|
lib/Service/MembershipHelper.php
Outdated
| // ownCloud admin is always admin of any custom group | ||
| if ($this->isUserSuperAdmin()) { | ||
| return true; | ||
| $disallowedGroupsForAdmin = $this->config->getSystemValue('customgroups.disallow-admin-edit', null); |
There was a problem hiding this comment.
double-check we want to store the information there. It might be fine if we want the "ops guy" or whoever is managing the host to manage that.
There was a problem hiding this comment.
my point here is, who is going to maintain this option? because new groups can be created and for some of them we might not want the admin to edit them.
There was a problem hiding this comment.
Its a valid point. But lets say if there is an option to automate this, then also the admin can see this in the config.php file, right?
lib/Service/MembershipHelper.php
Outdated
| $isUserAdmin = $this->isUserSuperAdmin(); | ||
| $groupInfo = $this->groupsHandler->getGroup((string)$groupId); | ||
| foreach ($disallowedGroupsForAdmin as $disallowedGroupForAdmin) { | ||
| if ($isUserAdmin && $groupInfo !== null && $disallowedGroupForAdmin === $groupInfo['display_name']) { |
There was a problem hiding this comment.
Maybe we can change this a bit
if ($isUserAdmin) {
foreach ($disallowedGroupsForAdmin......) {
if ($groupInfo !== null && $disallowedGroupForAdmin === $groupInfo['display_name']) {
return false;
}
}
}
I think this is easier to read.
Note that the $this->config->getSystemValue('customgroups.disallow-admin-edit', null); can also be moved inside the if ($isUserAdmin) (ifi the user isn't admin, no need to look for the groups), same for the $groupInfo
lib/Service/MembershipHelper.php
Outdated
| /** | ||
| * If ownCloud admin is not denied from the groups listed in system config | ||
| * customgroups.disallow-admin-edit, then: | ||
| * ownCloud admin is always admin of any custom group |
sharidas
changed the title
sharidas
force-pushed
the
deny-admin
branch
2 times, most recently
from
130b32e to
3e9b22c
Compare
| * @param int $groupId group id | ||
| * @return boolean true if the user can administrate, false otherwise | ||
| */ | ||
| public function isUserAdmin($groupId) { |
There was a problem hiding this comment.
can we change the method name? the PHPDoc clarifies what the method does, but if I see isUserAdmin('admin') it isn't intuitive for me.
There was a problem hiding this comment.
I don't mind changing the method name, but there are instances where it would affect. For example the grep in the customgroups app shows:
lib/Dav/GroupMembershipCollection.php: if (!$this->helper->isUserAdmin($groupId)) {
lib/Dav/GroupMembershipCollection.php: if (!$this->helper->isUserAdmin($groupId)) {
lib/Dav/GroupMembershipCollection.php: if (!$this->helper->isUserMember($groupId) && !$this->helper->isUserAdmin($groupId)) {
lib/Dav/GroupMembershipCollection.php: && !$this->helper->isUserAdmin($groupId)) {
lib/Dav/GroupMembershipCollection.php: if (!$this->helper->isUserMember($groupId) && !$this->helper->isUserAdmin($groupId)) {
lib/Dav/GroupMembershipCollection.php: if (!$this->helper->isUserAdmin($this->groupInfo['group_id'])) {
lib/Dav/MembershipNode.php: if (!$this->helper->isUserAdmin($groupId)
lib/Dav/MembershipNode.php: if (!$this->helper->isUserAdmin($groupId)) {
Would have to change in these locations too.
There was a problem hiding this comment.
pinging @micbar to decide. We can leave the method rename for a follow up PR, but we should make the PR soon.
|
While reviewing the PR I've noticed a thing that should be refactored: Changes in the Users and Root collections (in this PR) shows that the config object is just being passing around. Neither the users nor the root collections are doing anything with the config object, and it just being used because they create a group collection at some point (which is where the config object is required). This should be refactored to use a factory and move the object creation there. I don't think we have time to fix it here (it doesn't seem an easy change), but we should include it as a technical debt to be checked the sooner the better. |
| * @param int $groupId group id | ||
| * @return boolean true if the user can administrate, false otherwise | ||
| */ | ||
| public function isUserAdmin($groupId) { |
There was a problem hiding this comment.
pinging @micbar to decide. We can leave the method rename for a follow up PR, but we should make the PR soon.
Deny admin access when system config is set by the administrator. This will make admin user behave as regular user for the custom groups. Signed-off-by: Sujith H <sharidasan@owncloud.com>
|
Nothing to add from me. If this is tested and works properly, I think we can merge it. |
|
Another round of testing done with this PR: Without the config setting:
With the config setting:
With the config option set, the admin does behave like normal or regular user. It can only access the groups which it has access to (Not ALL). |
|
@micbar I have performed tests ( #273 (comment) and #273 (comment) )
|
Deny admin access when system config is set
by the administrator. This will make admin
user behave as regular user for the custom groups.
Signed-off-by: Sujith H sharidasan@owncloud.com