Skip to content
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

Allow disabling object-level roles #475

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ansible_base/rbac/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,8 @@ def create(self, validated_data):
# Object role assignment
if not obj:
raise ValidationError({'object_id': _('Object must be specified for this role assignment')})
elif not permission_registry.object_roles_enabled(obj):
raise ValidationError({'role_definition': _('Roles are not assignable through the API for this model')})

check_content_obj_permission(requesting_user, obj)
check_locally_managed(rd)
Expand Down
4 changes: 4 additions & 0 deletions ansible_base/rbac/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we warn or info if we hit this continue?

cls_repr = f"{permission_registry.get_resource_prefix(cls)}.{cls._meta.model_name}"

allowed_permissions[cls_repr] = []
for codename in list_combine_values(permissions_allowed_for_role(cls)):
perm = permission_registry.permission_qs.get(codename=codename)
Expand Down
2 changes: 1 addition & 1 deletion ansible_base/rbac/management/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def create_dab_permissions(app_config, verbosity=2, interactive=True, using=DEFA

# exit early if nothing is registered for this app
app_label = app_config.label
if not any(cls._meta.app_label == app_label for cls in permission_registry._registry):
if not any(info.app_label == app_label for info in permission_registry._registry.values()):
return

# Ensure that contenttypes are created for this app. Needed if
Expand Down
2 changes: 1 addition & 1 deletion ansible_base/rbac/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def contribute_to_class(self, cls: Type[models.Model], name: str) -> None:
super().contribute_to_class(cls, name)
self.managed = ManagedRoleManager(self.model._meta.apps)

def give_creator_permissions(self, user, obj) -> Optional['RoleUserAssignment']:
def give_creator_permissions(self, user: models.Model, obj: models.Model) -> Optional['RoleUserAssignment']:
# If the user is a superuser, no need to bother giving the creator permissions
for super_flag in settings.ANSIBLE_BASE_BYPASS_SUPERUSER_FLAGS:
if getattr(user, super_flag):
Expand Down
90 changes: 56 additions & 34 deletions ansible_base/rbac/permission_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,43 +22,59 @@
logger = logging.getLogger('ansible_base.rbac.permission_registry')


class ModelPermissionInfo:
"""Container of RBAC registration information for a model in permission_registry"""

def __init__(self, model, parent_field_name='organization', allow_object_roles=True):
self.model_name = model._meta.model_name
self.app_label = model._meta.app_label
if parent_field_name == self.model_name:
# model can not be its own parent
self.parent_field_name = None
else:
self.parent_field_name = parent_field_name
self.allow_object_roles = allow_object_roles
self.model = model


class PermissionRegistry:
def __init__(self):
self._registry = set() # model registry
self._name_to_model = dict()
self._parent_fields = dict()
self._registry = dict() # model registry
self._managed_roles = dict() # code-defined role definitions, managed=True
self.apps_ready = False
self._tracked_relationships = set()
self._trackers = dict()

def register(self, *args, parent_field_name='organization'):
def register(self, *args, **kwargs):
if self.apps_ready:
raise RuntimeError('Cannot register model to permission_registry after apps are ready')
for cls in args:
if cls not in self._registry:
self._registry.add(cls)
model_name = cls._meta.model_name
if model_name in self._name_to_model:
raise RuntimeError(f'Two models registered with same name {model_name}')
self._name_to_model[model_name] = cls
if model_name != 'organization':
self._parent_fields[model_name] = parent_field_name
for model in args:
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will account for the app_name because we are comparing the whole model right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

logger.debug(f'Model {model._meta.model_name} registered to permission registry more than once')
else:
logger.debug(f'Model {cls._meta.model_name} registered to permission registry more than once')
raise RuntimeError(f'Two models registered with same name {model._meta.model_name}')

def get_info(self, obj: Union[ModelBase, Model]) -> ModelPermissionInfo:
return self._registry[obj._meta.model_name]

def track_relationship(self, cls, relationship, role_name):
self._tracked_relationships.add((cls, relationship, role_name))

def get_parent_model(self, model) -> Optional[type]:
model = self._name_to_model[model._meta.model_name]
parent_field_name = self.get_parent_fd_name(model)
if parent_field_name is None:
info = self._registry[model._meta.model_name]
if info.parent_field_name is None:
return None
return model._meta.get_field(parent_field_name).related_model
return model._meta.get_field(info.parent_field_name).related_model
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to register a model with a parent but not the parent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def get_parent_fd_name(self, model) -> Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really nice to rename this to get_parent_field_name for clarity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed to merge this PR if its too much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return self._parent_fields.get(model._meta.model_name)
model_name = model._meta.model_name
if model_name not in self._registry:
return None
info = self._registry[model_name]
return info.parent_field_name

def get_child_models(self, parent_model, seen=None) -> list[tuple[str, Type[Model]]]:
"""Returns child models and the filter relationship to the parent
Expand All @@ -72,19 +88,18 @@ def get_child_models(self, parent_model, seen=None) -> list[tuple[str, Type[Mode
seen = set()
child_filters = []
parent_model_name = parent_model._meta.model_name
for model_name, parent_field_name in self._parent_fields.items():
if parent_field_name is None:
for model_name, info in self._registry.items():
if info.parent_field_name is None:
continue
child_model = self._name_to_model[model_name]
this_parent_name = child_model._meta.get_field(parent_field_name).related_model._meta.model_name
this_parent_name = info.model._meta.get_field(info.parent_field_name).related_model._meta.model_name
if this_parent_name == parent_model_name:
if model_name in seen:
continue
seen.add(model_name)

child_filters.append((parent_field_name, child_model))
for next_parent_filter, grandchild_model in self.get_child_models(child_model, seen=seen):
child_filters.append((f'{next_parent_filter}__{parent_field_name}', grandchild_model))
child_filters.append((info.parent_field_name, info.model))
for next_parent_filter, grandchild_model in self.get_child_models(info.model, seen=seen):
child_filters.append((f'{next_parent_filter}__{info.parent_field_name}', grandchild_model))
return child_filters

def get_resource_prefix(self, cls: Type[Model]) -> str:
Expand Down Expand Up @@ -150,8 +165,8 @@ def call_when_apps_ready(self, apps, app_config):
self.apps = apps
self.apps_ready = True

if self.team_model not in self._registry:
self._registry.add(self.team_model)
if self.team_model._meta.model_name not in self._registry:
self.register(self.team_model)

# Do no specify sender for create_dab_permissions, because that is passed as app_config
# and we want to create permissions for external apps, not the dab_rbac app
Expand All @@ -169,9 +184,9 @@ def call_when_apps_ready(self, apps, app_config):
self.user_model.add_to_class('singleton_permissions', bound_singleton_permissions)
post_delete.connect(triggers.rbac_post_user_delete, sender=self.user_model, dispatch_uid='permission-registry-user-delete')

for cls in self._registry:
triggers.connect_rbac_signals(cls)
connect_rbac_methods(cls)
for cls in self._registry.values():
triggers.connect_rbac_signals(cls.model)
connect_rbac_methods(cls.model)

for cls, relationship, role_name in self._tracked_relationships:
if role_name in self._trackers:
Expand Down Expand Up @@ -221,12 +236,19 @@ def team_permission(self):
return f'member_{self.team_model._meta.model_name}'

@property
def all_registered_models(self):
return list(self._registry)
def all_registered_models(self) -> list[Type[Model]]:
return [info.model for info in self._registry.values()]

def is_registered(self, obj: Union[ModelBase, Model]) -> bool:
"""Tells if the given object or class is a type tracked by DAB RBAC"""
return any(obj._meta.model_name == cls._meta.model_name for cls in self._registry)
return bool(obj._meta.model_name in self._registry)

def object_roles_enabled(self, obj: Optional[Union[ModelBase, Model]]) -> bool:
"""For given model tells if object roles are enabled on that model"""
if not obj:
return True # can create system roles, although weird call pattern
info = self.get_info(obj)
return bool(info.allow_object_roles)


permission_registry = PermissionRegistry()
7 changes: 6 additions & 1 deletion ansible_base/rbac/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def permissions_allowed_for_system_role() -> dict[type, list[str]]:
return permissions_by_model


def permissions_allowed_for_role(cls) -> dict[type, list[str]]:
def permissions_allowed_for_role(cls: Union[Model, Type[Model], None]) -> dict[type, list[str]]:
"Permission codenames valid for a RoleDefinition of given class, organized by permission class"
if cls is None:
return permissions_allowed_for_system_role()
Expand Down Expand Up @@ -100,6 +100,11 @@ def validate_permissions_for_model(permissions, content_type: Optional[Model], m
role_model = content_type.model_class()
permissions_by_model = permissions_allowed_for_role(role_model)

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'})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


invalid_codenames = codename_list - combine_values(permissions_by_model)
if invalid_codenames:
print_codenames = ', '.join(f'"{codename}"' for codename in invalid_codenames)
Expand Down
15 changes: 14 additions & 1 deletion docs/apps/rbac/for_app_developers.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ Removing permission will delete the object role if no other assignments exist.
Any Django Model (except your user model) can
be made into a resource in the RBAC system by registering that resource in the registry.

```
```python
from ansible_base.rbac.permission_registry import permission_registry

permission_registry.register(MyModel, parent_field_name='organization')
Expand Down Expand Up @@ -133,6 +133,19 @@ then no other _registered_ model should list that permission.
This rules out things like registering a model, and also registering a proxy
model _of that model_.

#### Disabling Object-level Permissions

You can specifically disable giving object-level permissions at registration of a model.

```python
permission_registry.register(MyModel, allow_object_roles=False)
```

This will not allow creating any role_definitions with a `content_type` for that model.
Users will still be able to get permission to `MyModel` objects by either
obtaining an organization-level (or other parent object) role, or by a system-level role.
This just prevents using object-level roles through the API.

### Parent Resources

Assuming `obj` has a related `organization` which was declared by `parent_field_name` when registering,
Expand Down
33 changes: 33 additions & 0 deletions test_app/migrations/0001_initial.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,4 +284,37 @@ class Migration(migrations.Migration):
},
bases=('test_app.original2',),
),
migrations.CreateModel(
name='MemberGuide',
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('modified', models.DateTimeField(auto_now=True, help_text='The date/time this resource was created')),
('created', models.DateTimeField(auto_now_add=True, help_text='The date/time this resource was created')),
('name', models.CharField(help_text='The name of this resource', max_length=512)),
('article', models.TextField(default='-- Help article stub --')),
('created_by', models.ForeignKey(default=None, editable=False, help_text='The user who created this resource', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='%(app_label)s_%(class)s_created+', to=settings.AUTH_USER_MODEL)),
('modified_by', models.ForeignKey(default=None, editable=False, help_text='The user who last modified this resource', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='%(app_label)s_%(class)s_modified+', to=settings.AUTH_USER_MODEL)),
('organization', models.ForeignKey(help_text='Docs for all org members', on_delete=django.db.models.deletion.CASCADE, related_name='member_guides', to='test_app.organization')),
],
options={
'abstract': False,
},
),
migrations.CreateModel(
name='PublicData',
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('modified', models.DateTimeField(auto_now=True, help_text='The date/time this resource was created')),
('created', models.DateTimeField(auto_now_add=True, help_text='The date/time this resource was created')),
('name', models.CharField(help_text='The name of this resource', max_length=512)),
('data', models.JSONField(blank=True, default=dict)),
('created_by', models.ForeignKey(default=None, editable=False, help_text='The user who created this resource', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='%(app_label)s_%(class)s_created+', to=settings.AUTH_USER_MODEL)),
('modified_by', models.ForeignKey(default=None, editable=False, help_text='The user who last modified this resource', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='%(app_label)s_%(class)s_modified+', to=settings.AUTH_USER_MODEL)),
('organization', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='public_data', to='test_app.organization')),
],
options={
'ordering': ['id'],
'default_permissions': ('add', 'change', 'delete'),
},
),
]
32 changes: 0 additions & 32 deletions test_app/migrations/0011_publicdata.py

This file was deleted.

15 changes: 15 additions & 0 deletions test_app/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,20 @@ class UUIDModel(models.Model):
organization = models.ForeignKey(Organization, on_delete=models.CASCADE, related_name='uuidmodels')


class MemberGuide(NamedCommonModel):
"""Demonstrates use case for hypothetical model with no object-level roles

The pretend use-case is that this saves articles for documentation, which is intended to
be available for all members of the organization.
Since this tracks so closely with organization member permission, object-level roles
would be overkill and confusing.
Our intent is that permissions are only delegated at the organization level.
"""

organization = models.ForeignKey(Organization, on_delete=models.CASCADE, related_name='member_guides', help_text='Docs for all org members')
article = models.TextField(default='-- Help article stub --')


class ImmutableTask(models.Model):
"Hypothetical immutable task-like thing, can be created and canceled but not edited"

Expand Down Expand Up @@ -273,6 +287,7 @@ class Meta:
permission_registry.register(ParentName, parent_field_name='my_organization')
permission_registry.register(CollectionImport, parent_field_name='namespace')
permission_registry.register(InstanceGroup, ImmutableTask, parent_field_name=None)
permission_registry.register(MemberGuide, allow_object_roles=False)

# NOTE(cutwater): Using hard coded role names instead of ones defined in ReconcileUser class,
# to avoid circular dependency between models and claims modules. This is a temporary workarond,
Expand Down
2 changes: 2 additions & 0 deletions test_app/router.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ def filter_associate_queryset(self, qs):
'positionmodels': (views.PositionModelViewSet, 'positionmodels'),
'weirdperms': (views.WeirdPermViewSet, 'weirdperms'),
'publicdata': (views.PublicDataViewSet, 'public_data'),
'member_guides': (views.MemberGuideViewSet, 'member_guides'),
},
)

Expand Down Expand Up @@ -90,6 +91,7 @@ def filter_associate_queryset(self, qs):
router.register(r'cows', views.CowViewSet)
router.register(r'uuidmodels', views.UUIDModelViewSet)
router.register(r'public_data', views.PublicDataViewSet)
router.register(r'member_guides', views.MemberGuideViewSet)
router.register(r'cities', views.CityViewSet)
router.register(r'animals', views.AnimalViewSet)
router.register(r'namespaces', views.NamespaceViewSet, related_views={'collections': (views.CollectionImportViewSet, 'collections')})
Expand Down
6 changes: 6 additions & 0 deletions test_app/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ class Meta:
fields = '__all__'


class MemberGuideSerializer(RelatedAccessMixin, ModelSerializer):
class Meta:
model = models.MemberGuide
fields = '__all__'


class ImmutableLogEntrySerializer(ImmutableCommonModelSerializer):
class Meta:
model = models.ImmutableLogEntry
Expand Down
Loading
Loading