-
-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mk restrict domain admin #22757
Mk restrict domain admin #22757
Conversation
836eb44
to
0fb9573
Compare
@@ -652,6 +652,9 @@ def has_permission(self, domain, permission, data=None, restrict_global_admin=Fa | |||
|
|||
dm = self.get_domain_membership(domain) | |||
if dm: | |||
# an admin has access to all features by default, restrict that if needed | |||
if dm.is_admin and restrict_global_admin: | |||
return False |
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.
I think you need to pass restrict_global_admin
along to DomainMembership.has_permission
. This code will return false even if the user does specifically have the permssions assigned to them.
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.
i actually did that earlier and dm.is_admin
again short circuited permissions here. I believe is_admin is true for the Admin role which has all permissions by default and is not editable.
We want this to return false even if the user is domain admin.
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.
That's what I'm saying: pass restrict_global_admin
into that function and change it from
return self.is_admin or self.permissions.has(permission, data)
to
return (self.is_admin and not restrict_global_admin) or self.permissions.has(permission, data)
Right now if restrict_global_admin
is true, there's no way for this has_permission
to return true, which seems wrong.
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.
I was wrong about this, nevermind.
Follow up from #22559
we need to restrict even domain admins
also modified the implementation by checking only at permissions.