Add callback to validate role assignment #490
Conversation
|
This should formally solve #445. The method signatures might be non-optimal for that particular use case, which is "don't allow _system user to get any permissions". What you have works best for "don't allow _system user to get inventory permissions". If we were to solve the former problem in AWX, then I believe the method would get attached to I'm also interested in (later, if not now) UI involvement in this. By having this take arguments, we are not able to surface it in the API in any format. The _system user case might be better as a property on the User model, so that the API could give a true/false reading on whether that particular user was disabled from receiving roles. Likewise the other way, a property on objects that told true/false permissions could be delegated. When would an object be prohibited from having roles assigned? The AWX control plane EE. Talking this out, cross-referencing against your AWX PR, I'm leaning in the direction of eventually having both solutions. |
No, there's no other kwargs to pass it? The caller here is DAB RBAC. It's giving all the parameters for the |
I take it back. I've been looking further into the EE case and it's potentially more complicated with a lot of wrinkles We allow some edits for the control plane EE, but not everything. The default EE can be edited, but not deleted(?). That might be (and should be) handled in the validators, so maybe irrelevant. And org-less EEs are another special case. So I'm leaning towards a boolean being an anti-feature. Even if we had different parameterizations of the callback, returning a message seems pretty critical in the end-game. These are exceptions, and they need to return a reason. |
There was a problem hiding this comment.
Let's change the expectation for validate_role_assignment to return nothing, and raise either a ValidationError or a PermissionDenied error, and all the issues related to those choices will be pushed down to the app using this.
Given that change, I'd say this is what we need 👍
fosterseth
force-pushed
the
callback_for_role_assignment
branch
from
6f081a0
to
0829e4d
Compare
|
I checked out your branch and used it in ansible/awx#15289 and initially got this, what do you think? Is AWX running some non-standard logic in its generics that is the problem? |
|
Resolved by adding a field key to the error: diff --git a/awx/main/models/execution_environments.py b/awx/main/models/execution_environments.py
index 5d55b55caf..137533d0f4 100644
--- a/awx/main/models/execution_environments.py
+++ b/awx/main/models/execution_environments.py
@@ -60,13 +60,13 @@ class ExecutionEnvironment(CommonModel):
def validate_role_assignment(self, actor, role_definition):
if self.managed:
- raise ValidationError(_('Can not assign object roles to managed Execution Environments'))
+ raise ValidationError({'object_id': _('Can not assign object roles to managed Execution Environments')})
if self.organization_id is None:
- raise ValidationError(_('Can not assign object roles to global Execution Environments'))
+ raise ValidationError({'object_id': _('Can not assign object roles to global Execution Environments')})
if actor._meta.model_name == 'user' and (not actor.has_obj_perm(self.organization, 'view')):
- raise ValidationError(_('User must have view permission to Execution Environment organization'))
+ raise ValidationError({'user': _('User must have view permission to Execution Environment organization')})
if actor._meta.model_name == 'team':
organization_cls = self._meta.get_field('organization').related_model
if self.orgaanization not in organization_cls.access_qs(actor, 'view'):
- raise ValidationError(_('Team must have view permission to Execution Environment organization'))
+ raise ValidationError({'team': _('Team must have view permission to Execution Environment organization')})I'm okay with this, but it should be eventually documented. If not now, then please drop some quick notes in a DAB issue. |
fosterseth
force-pushed
the
callback_for_role_assignment
branch
from
9d4f487
to
5e09f0e
Compare
Some dab-based apps like AWX may wish to add exceptions to the user and team role assignments. This PR adds logic to the serializer .create method to execute a callback method that is optionally defined on the model. This callback signature looks like: validate_role_assignment(self, actor, role_definition) This callback should raise exceptions if necessary. Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
AlanCoding
force-pushed
the
callback_for_role_assignment
branch
from
5e09f0e
to
9ce8b88
Compare
|
Some dab-based apps like AWX may wish to add exceptions to the user and team role assignments.
This PR adds logic to the serializer
.createmethod to execute a callback method that is optionally defined on the model.This callback signature looks like:
validate_role_assignment(self, actor, role_definition)and should return either
If a string is returned, the string will be wrapped up in a
PermissionDeniederror (HTTP 403)Open questions:
**kwargs?