Allow disabling object-level roles #475
Conversation
AlanCoding
added
the
blocked
AlanCoding
added
Ready for review
AlanCoding
force-pushed
the
no_object_roles
branch
from
94ba079 to
f5aa66c
Compare
AlanCoding
force-pushed
the
no_object_roles
branch
from
f5aa66c to
4d1a4c8
Compare
|
| @@ -60,7 +60,11 @@ def get(self, request, format=None): | |||
| if cls is None: | |||
| cls_repr = 'system' | |||
| else: | |||
| info = permission_registry.get_info(cls) | |||
| if not info.allow_object_roles: | |||
| continue | |||
There was a problem hiding this comment.
Should we warn or info if we hit this continue?
| if model._meta.model_name not in self._registry: | ||
| info = ModelPermissionInfo(model, **kwargs) | ||
| self._registry[info.model_name] = info | ||
| elif self._registry[model._meta.model_name] is model: |
There was a problem hiding this comment.
This will account for the app_name because we are comparing the whole model right?
There was a problem hiding this comment.
It will yell at you if you register 2 models with the same name, even if they're in different apps. Proxy models are fine, but don't register the original... and also register the proxy.
| return None | ||
| return model._meta.get_field(parent_field_name).related_model | ||
| return model._meta.get_field(info.parent_field_name).related_model |
There was a problem hiding this comment.
Is it possible to register a model with a parent but not the parent?
There was a problem hiding this comment.
Yes, very much so, InstanceGroup does not have a related organization. So you can have an object-level role, but no organization association. This call was made in fairly recent development, but before any work on the new RBAC system had started.
Even more interestingly, the Instance model is the one case where not being organization-scoped is truly defensible (an instance is a property of the cluster...), but object-level roles might also not make sense. It's more likely that you would only want to delegate permissions with system-level roles. Because you're kind of either managing an AWX cluster or you're not... it's 1 thing, and sub-dividing it doesn't make a ton of sense.
| return None | ||
| return model._meta.get_field(parent_field_name).related_model | ||
| return model._meta.get_field(info.parent_field_name).related_model | ||
|
|
||
| def get_parent_fd_name(self, model) -> Optional[str]: |
There was a problem hiding this comment.
It would be really nice to rename this to get_parent_field_name for clarity.
There was a problem hiding this comment.
Not needed to merge this PR if its too much.
There was a problem hiding this comment.
I used that method name once, and only in eda-server tests, so it shouldn't be difficult to rename. But it is a cross-repo change.
| if not managed: | ||
| if not permission_registry.object_roles_enabled(role_model): | ||
| print_model = role_model._meta.verbose_name if role_model else 'global roles' | ||
| raise ValidationError({'content_type': f'Creating roles for the {print_model} model is disabled'}) |
There was a problem hiding this comment.
Is this the right termonology in the exception? the global roles model is disabled.
Also, we should likely translate these strings if they can get raised to the browser.
There was a problem hiding this comment.
Yeah, the flow of this isn't good. The method object_roles_enabled unconditionally returns True if passed None, and the None value signifies a global role. So, it would be more clear to have this block be if (not managed) and role_model:, so we're only considering this condition when it's an object-based (as opposed to system) role definition.
john-westcott-iv
added
Ready To Merge
comments left
|
Retrospective: The problem here is the creator roles. Permissions are an inter-connected system. If someone has "add" permission for a model, then after creating an object, they need the object-level admin role (or whatever role is configured for creators) after creation. This means that we could only disable object-level roles created in the API, but they would still have to exist generally. This is an additional piece of information which would have to be communicated to clients, and increases confusion. For overall system stability and sanity I am backing out of this approach. I may also close the issue as a won't-fix. |
Fixes #424
Enabling work for AAP-25268