From e94d441fb0f7beb497981414f1949ed46d35ea07 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 29 Feb 2016 16:59:20 -0500 Subject: [PATCH 1/4] Add support for following parental changes on save and delete in the RBAC system --- awx/main/fields.py | 162 ++++++++++++++++---- awx/main/models/rbac.py | 51 +++++- awx/main/tests/functional/test_rbac_core.py | 41 ++++- 3 files changed, 215 insertions(+), 39 deletions(-) diff --git a/awx/main/fields.py b/awx/main/fields.py index 15224d43fd..1db59e296f 100644 --- a/awx/main/fields.py +++ b/awx/main/fields.py @@ -3,7 +3,11 @@ # Django from django.db import connection -from django.db.models.signals import post_save +from django.db.models.signals import ( + post_init, + post_save, + post_delete, +) from django.db.models.signals import m2m_changed from django.db import models from django.db.models.fields.related import ( @@ -69,7 +73,8 @@ class ResourceFieldDescriptor(ReverseSingleRelatedObjectDescriptor): raise TransactionManagementError('Current transaction has failed, cannot create implicit resource') resource = Resource.objects.create(content_object=instance) setattr(instance, self.field.name, resource) - instance.save(update_fields=[self.field.name,]) + if instance.pk: + instance.save(update_fields=[self.field.name,]) return resource @@ -85,12 +90,45 @@ class ImplicitResourceField(models.ForeignKey): def contribute_to_class(self, cls, name): super(ImplicitResourceField, self).contribute_to_class(cls, name) setattr(cls, self.name, ResourceFieldDescriptor(self)) - post_save.connect(self._save, cls, True) + post_save.connect(self._post_save, cls, True) + post_delete.connect(self._post_delete, cls, True) + + def _post_save(self, instance, *args, **kwargs): + # Ensures our resource object exists and that it's content_object + # points back to our hosting instance. + this_resource = getattr(instance, self.name) + if not this_resource.object_id: + this_resource.content_object = instance + this_resource.save() + + def _post_delete(self, instance, *args, **kwargs): + getattr(instance, self.name).delete() + + + + +def resolve_role_field(obj, field): + ret = [] + + field_components = field.split('.', 1) + if hasattr(obj, field_components[0]): + obj = getattr(obj, field_components[0]) + else: + return [] + + if len(field_components) == 1: + if type(obj) is not ImplicitRoleDescriptor and type(obj) is not Role: + raise Exception('%s refers to a %s, not an ImplicitRoleField or Role' % (field, str(type(obj)))) + ret.append(obj) + else: + if type(obj) is ManyRelatedObjectsDescriptor: + for o in obj.all(): + ret += resolve_role_field(o, field_components[1]) + else: + ret += resolve_role_field(obj, field_components[1]) + + return ret - def _save(self, instance, *args, **kwargs): - # Ensure that our field gets initialized after our first save - if not hasattr(instance, self.name): - getattr(instance, self.name) class ImplicitRoleDescriptor(ReverseSingleRelatedObjectDescriptor): @@ -116,27 +154,6 @@ class ImplicitRoleDescriptor(ReverseSingleRelatedObjectDescriptor): role = Role.objects.create(name=self.role_name, content_object=instance) if self.parent_role: - def resolve_field(obj, field): - ret = [] - - field_components = field.split('.', 1) - if hasattr(obj, field_components[0]): - obj = getattr(obj, field_components[0]) - else: - return [] - - if len(field_components) == 1: - if type(obj) is not ImplicitRoleDescriptor and type(obj) is not Role: - raise Exception('%s refers to a %s, not an ImplicitRoleField or Role' % (field, str(type(obj)))) - ret.append(obj) - else: - if type(obj) is ManyRelatedObjectsDescriptor: - for o in obj.all(): - ret += resolve_field(o, field_components[1]) - else: - ret += resolve_field(obj, field_components[1]) - - return ret # Add all non-null parent roles as parents paths = self.parent_role if type(self.parent_role) is list else [self.parent_role] @@ -144,11 +161,12 @@ class ImplicitRoleDescriptor(ReverseSingleRelatedObjectDescriptor): if path.startswith("singleton:"): parents = [Role.singleton(path[10:])] else: - parents = resolve_field(instance, path) + parents = resolve_role_field(instance, path) for parent in parents: role.parents.add(parent) setattr(instance, self.field.name, role) - instance.save(update_fields=[self.field.name,]) + if instance.pk: + instance.save(update_fields=[self.field.name,]) if self.permissions is not None: permissions = RolePermission( @@ -198,7 +216,9 @@ class ImplicitRoleField(models.ForeignKey): self ) ) - post_save.connect(self._save, cls, True) + post_init.connect(self._post_init, cls, True) + post_save.connect(self._post_save, cls, True) + post_delete.connect(self._post_delete, cls, True) add_lazy_relation(cls, self, "self", self.bind_m2m_changed) def bind_m2m_changed(self, _self, _role_class, cls): @@ -263,7 +283,81 @@ class ImplicitRoleField(models.ForeignKey): getattr(instance, self.name).parents.remove(getattr(obj, self.m2m_field_attr)) - def _save(self, instance, *args, **kwargs): + def _post_init(self, instance, *args, **kwargs): + if not self.parent_role: + return + #if not hasattr(instance, self.name): + # getattr(instance, self.name) + + if not hasattr(self, '__original_parent_roles'): + paths = self.parent_role if type(self.parent_role) is list else [self.parent_role] + all_parents = set() + for path in paths: + if path.startswith("singleton:"): + parents = [Role.singleton(path[10:])] + else: + parents = resolve_role_field(instance, path) + for parent in parents: + all_parents.add(parent) + #role.parents.add(parent) + self.__original_parent_roles = all_parents + + ''' + field_names = self.parent_role + if type(field_names) is not list: + field_names = [field_names] + self.__original_values = {} + for field_name in field_names: + if field_name.startswith('singleton:'): + continue + first_field_name = field_name.split('.')[0] + self.__original_values[first_field_name] = getattr(instance, first_field_name) + ''' + else: + print('WE DO NEED THIS') + pass + + def _post_save(self, instance, *args, **kwargs): # Ensure that our field gets initialized after our first save - if not hasattr(instance, self.name): - getattr(instance, self.name) + this_role = getattr(instance, self.name) + if not this_role.object_id: + # Ensure our ref back to our instance is set. This will not be set the + # first time the object is saved because we create the role in our _post_init + # but that happens before an id for the instance has been set (because it + # hasn't been saved yet!). Now that everything has an id, we patch things + # so the role references the instance. + this_role.content_object = instance + this_role.save() + + # As object relations change, the role hierarchy might also change if the relations + # that changed were referenced in our magic parent_role field. This code synchronizes + # these changes. + if not self.parent_role: + return + + paths = self.parent_role if type(self.parent_role) is list else [self.parent_role] + original_parents = self.__original_parent_roles + new_parents = set() + for path in paths: + if path.startswith("singleton:"): + parents = [Role.singleton(path[10:])] + else: + parents = resolve_role_field(instance, path) + for parent in parents: + new_parents.add(parent) + + Role.pause_role_ancestor_rebuilding() + for role in original_parents - new_parents: + this_role.parents.remove(role) + for role in new_parents - original_parents: + this_role.parents.add(role) + Role.unpause_role_ancestor_rebuilding() + + self.__original_parent_roles = new_parents + + def _post_delete(self, instance, *args, **kwargs): + this_role = getattr(instance, self.name) + children = [c for c in this_role.children.all()] + this_role.delete() + for child in children: + children.rebuild_role_ancestor_list() diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index bdf33e0a84..396dcd71c3 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -15,13 +15,23 @@ from django.contrib.contenttypes.fields import GenericForeignKey # AWX from awx.main.models.base import * # noqa -__all__ = ['Role', 'RolePermission', 'Resource', 'ROLE_SINGLETON_SYSTEM_ADMINISTRATOR', 'ROLE_SINGLETON_SYSTEM_AUDITOR'] +__all__ = [ + 'Role', + 'RolePermission', + 'Resource', + 'ROLE_SINGLETON_SYSTEM_ADMINISTRATOR', + 'ROLE_SINGLETON_SYSTEM_AUDITOR', +] logger = logging.getLogger('awx.main.models.rbac') ROLE_SINGLETON_SYSTEM_ADMINISTRATOR='System Administrator' ROLE_SINGLETON_SYSTEM_AUDITOR='System Auditor' +role_rebuilding_paused = False +roles_needing_rebuilding = set() + + class Role(CommonModelNameNotUnique): ''' @@ -48,6 +58,36 @@ class Role(CommonModelNameNotUnique): def get_absolute_url(self): return reverse('api:role_detail', args=(self.pk,)) + @staticmethod + def pause_role_ancestor_rebuilding(): + ''' + Pauses role ancestor list updating. This is useful when you're making + many changes to the same roles, for example doing bulk inserts or + making many changes to the same object in succession. + + Note that the unpause_role_ancestor_rebuilding MUST be called within + the same execution context (preferably within the same transaction), + otherwise the RBAC role ancestor hierarchy will not be properly + updated. + ''' + + global role_rebuilding_paused + role_rebuilding_paused = True + + @staticmethod + def unpause_role_ancestor_rebuilding(): + ''' + Unpauses the role ancestor list updating. This will will rebuild all + roles that need updating since the last call to + pause_role_ancestor_rebuilding and bring everything back into sync. + ''' + global role_rebuilding_paused + global roles_needing_rebuilding + role_rebuilding_paused = False + for role in Role.objects.filter(id__in=list(roles_needing_rebuilding)).all(): + role.rebuild_role_ancestor_list() + roles_needing_rebuilding = set() + def rebuild_role_ancestor_list(self): ''' Updates our `ancestors` map to accurately reflect all of the ancestors for a role @@ -57,6 +97,11 @@ class Role(CommonModelNameNotUnique): Note that this method relies on any parents' ancestor list being correct. ''' + global role_rebuilding_paused, roles_needing_rebuilding + + if role_rebuilding_paused: + roles_needing_rebuilding.add(self.id) + return actual_ancestors = set(Role.objects.filter(id=self.id).values_list('parents__ancestors__id', flat=True)) actual_ancestors.add(self.id) @@ -67,9 +112,9 @@ class Role(CommonModelNameNotUnique): # If it differs, update, and then update all of our children if actual_ancestors != stored_ancestors: for id in actual_ancestors - stored_ancestors: - self.ancestors.add(Role.objects.get(id=id)) + self.ancestors.add(id) for id in stored_ancestors - actual_ancestors: - self.ancestors.remove(Role.objects.get(id=id)) + self.ancestors.remove(id) for child in self.children.all(): child.rebuild_role_ancestor_list() diff --git a/awx/main/tests/functional/test_rbac_core.py b/awx/main/tests/functional/test_rbac_core.py index b31ef310b0..020023f9bd 100644 --- a/awx/main/tests/functional/test_rbac_core.py +++ b/awx/main/tests/functional/test_rbac_core.py @@ -2,6 +2,7 @@ import pytest from awx.main.models import ( Role, + Resource, Organization, ) @@ -90,14 +91,50 @@ def test_auto_m2m_adjuments(organization, project, alice): assert project.accessible_by(alice, {'read': True}) is True @pytest.mark.django_db -@pytest.mark.skipif(True, reason='Unimplemented') def test_auto_field_adjuments(organization, inventory, team, alice): - 'Ensures the auto role reparenting is working correctly through m2m maps' + 'Ensures the auto role reparenting is working correctly through non m2m fields' org2 = Organization.objects.create(name='Org 2', description='org 2') org2.admin_role.members.add(alice) assert inventory.accessible_by(alice, {'read': True}) is False inventory.organization = org2 + inventory.save() assert inventory.accessible_by(alice, {'read': True}) is True inventory.organization = organization + inventory.save() assert inventory.accessible_by(alice, {'read': True}) is False + #assert False + +@pytest.mark.django_db +def test_implicit_deletes(alice): + 'Ensures implicit resources and roles delete themselves' + delorg = Organization.objects.create(name='test-org') + delorg.admin_role.members.add(alice) + + resource_id = delorg.resource.id + admin_role_id = delorg.admin_role.id + auditor_role_id = delorg.auditor_role.id + + assert Role.objects.filter(id=admin_role_id).count() == 1 + assert Role.objects.filter(id=auditor_role_id).count() == 1 + assert Resource.objects.filter(id=resource_id).count() == 1 + n_alice_roles = alice.roles.count() + n_system_admin_children = Role.singleton('System Administrator').children.count() + + delorg.delete() + + assert Role.objects.filter(id=admin_role_id).count() == 0 + assert Role.objects.filter(id=auditor_role_id).count() == 0 + assert Resource.objects.filter(id=resource_id).count() == 0 + assert alice.roles.count() == (n_alice_roles - 1) + assert Role.singleton('System Administrator').children.count() == (n_system_admin_children - 1) + +@pytest.mark.django_db +def test_content_object(user): + 'Ensure our conent_object stuf seems to be working' + + print('Creating organization') + org = Organization.objects.create(name='test-org') + print('Organizaiton id: %d resource: %d admin_role: %d' % (org.id, org.resource.id, org.admin_role.id)) + assert org.resource.content_object.id == org.id + assert org.admin_role.content_object.id == org.id From 73dc0617166fc3e98f382decda04e4c03998cf11 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Tue, 1 Mar 2016 09:47:26 -0500 Subject: [PATCH 2/4] Patch up our credential migration tests to undo some automatic work that needs to be done in the migration --- awx/main/tests/functional/test_rbac_credential.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/awx/main/tests/functional/test_rbac_credential.py b/awx/main/tests/functional/test_rbac_credential.py index 2e44442528..e2febb456d 100644 --- a/awx/main/tests/functional/test_rbac_credential.py +++ b/awx/main/tests/functional/test_rbac_credential.py @@ -30,7 +30,10 @@ def test_credential_migration_team_member(credential, team, user, permissions): credential.team = team credential.save() - # No permissions pre-migration + + # No permissions pre-migration (this happens automatically so we patch this) + team.admin_role.children.remove(credential.owner_role) + team.member_role.children.remove(credential.usage_role) assert not credential.accessible_by(u, permissions['admin']) migrated = rbac.migrate_credential(apps, None) @@ -47,6 +50,8 @@ def test_credential_migration_team_admin(credential, team, user, permissions): credential.save() # No permissions pre-migration + team.admin_role.children.remove(credential.owner_role) + team.member_role.children.remove(credential.usage_role) assert not credential.accessible_by(u, permissions['usage']) # Usage permissions post migration From 41c06dc2d0542da49f0ad6667d9740f676d70cc9 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Tue, 1 Mar 2016 09:54:35 -0500 Subject: [PATCH 3/4] Update user migration to not bomb out when a UserResource already exists for a user --- awx/main/migrations/_rbac.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/awx/main/migrations/_rbac.py b/awx/main/migrations/_rbac.py index 414a5009de..546721c4f6 100644 --- a/awx/main/migrations/_rbac.py +++ b/awx/main/migrations/_rbac.py @@ -5,10 +5,9 @@ def migrate_users(apps, schema_editor): migrations = list() User = apps.get_model('auth', "User") Role = apps.get_model('main', "Role") - UserResource = apps.get_model('main', "UserResource") for user in User.objects.all(): - ur = UserResource.objects.create(user=user) + ur = user.resource # implicitly creates the UserResource field if it didn't already exist ur.admin_role.members.add(user) if user.is_superuser: From f5e311f5ac3f7829134f774760fcded5f01f79e7 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Tue, 1 Mar 2016 09:56:43 -0500 Subject: [PATCH 4/4] Undo some more automatic work that we're suppsoed to test with our migrations --- awx/main/tests/functional/test_rbac_organization.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/awx/main/tests/functional/test_rbac_organization.py b/awx/main/tests/functional/test_rbac_organization.py index 6f4a2e0623..c8f1d709a2 100644 --- a/awx/main/tests/functional/test_rbac_organization.py +++ b/awx/main/tests/functional/test_rbac_organization.py @@ -14,6 +14,8 @@ def test_organization_migration_admin(organization, permissions, user): u = user('admin', False) organization.admins.add(u) + # Undo some automatic work that we're supposed to be testing with our migration + organization.admin_role.members.remove(u) assert not organization.accessible_by(u, permissions['admin']) migrations = rbac.migrate_organization(apps, None) @@ -26,6 +28,8 @@ def test_organization_migration_user(organization, permissions, user): u = user('user', False) organization.users.add(u) + # Undo some automatic work that we're supposed to be testing with our migration + organization.member_role.members.remove(u) assert not organization.accessible_by(u, permissions['auditor']) migrations = rbac.migrate_organization(apps, None)