From ac7d50048cd2584244f9146015808703eb5973f1 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Wed, 10 Feb 2016 16:09:57 -0500 Subject: [PATCH 1/4] Removing unused resource_parent Forgot to remove these bits when we removed the concept a few commits ago --- awx/main/fields.py | 18 ++---------------- awx/main/models/rbac.py | 2 -- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/awx/main/fields.py b/awx/main/fields.py index 7d903d1278..e002ab74c9 100644 --- a/awx/main/fields.py +++ b/awx/main/fields.py @@ -55,8 +55,7 @@ def resolve_field(obj, field): class ResourceFieldDescriptor(ReverseSingleRelatedObjectDescriptor): """Descriptor for access to the object from its related class.""" - def __init__(self, parent_resource, *args, **kwargs): - self.parent_resource = parent_resource + def __init__(self, *args, **kwargs): super(ResourceFieldDescriptor, self).__init__(*args, **kwargs) def __get__(self, instance, instance_type=None): @@ -64,18 +63,6 @@ class ResourceFieldDescriptor(ReverseSingleRelatedObjectDescriptor): if resource: return resource resource = Resource._default_manager.create() - if self.parent_resource: - # Take first non null parent resource - parent = None - if type(self.parent_resource) is list: - for path in self.parent_resource: - parent = resolve_field(instance, path) - if parent: - break - else: - parent = resolve_field(instance, self.parent_resource) - resource.parent = parent - resource.save() setattr(instance, self.field.name, resource) instance.save(update_fields=[self.field.name,]) return resource @@ -84,8 +71,7 @@ class ResourceFieldDescriptor(ReverseSingleRelatedObjectDescriptor): class ImplicitResourceField(models.ForeignKey): """Creates an associated resource object if one doesn't already exist""" - def __init__(self, parent_resource=None, *args, **kwargs): - self.parent_resource = parent_resource + def __init__(self, *args, **kwargs): kwargs.setdefault('to', 'Resource') kwargs.setdefault('related_name', '+') kwargs.setdefault('null', 'True') diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 75ff67cb96..1268f68443 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -113,8 +113,6 @@ class Resource(CommonModelNameNotUnique): verbose_name_plural = _('resources') db_table = 'main_rbac_resources' - parent = models.ForeignKey('Resource', related_name='children', null=True, default=None) - class RolePermission(CreatedModifiedModel): ''' From 9a3ef6b99851cb86ffb1ca82d78a5eaa53e33471 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Thu, 11 Feb 2016 16:13:41 -0500 Subject: [PATCH 2/4] ORMified RBAC classes; Added GenericForeignKey backref for convenience The RoleHierarchy table has been eliminated in favor of just using a ManyToMany map, which is what we should have been using all along. ORMifications still need improvement, in particular filtering on ResourceMixin.accessible_by should reduce permission calculation overhead, but with the current implemenation this is not true. ResourceMixin.get_permission performs adequately but not as good as it can yet. --- awx/main/models/mixins.py | 250 +++++++++++++++++++++++++------------- awx/main/models/rbac.py | 64 +++++----- 2 files changed, 197 insertions(+), 117 deletions(-) diff --git a/awx/main/models/mixins.py b/awx/main/models/mixins.py index 8ce444bbb4..3b7b5eb8d4 100644 --- a/awx/main/models/mixins.py +++ b/awx/main/models/mixins.py @@ -1,11 +1,14 @@ # Django from django.db import models from django.db import connection +from django.db.models.aggregates import Max +from django.contrib.contenttypes.models import ContentType # AWX -from awx.main.models.rbac import RolePermission, Role, RoleHierarchy +from awx.main.models.rbac import RolePermission, Role, Resource from awx.main.fields import ImplicitResourceField + __all__ = 'ResourceMixin' class ResourceMixin(models.Model): @@ -29,62 +32,105 @@ class ResourceMixin(models.Model): `myresource.get_permissions(user)`. ''' - aggregate_where_clause = '' - aggregates = '' - group_clause = '' - where_clause = '' + # TODO: Clean this up once we have settled on an optimal implementation - if len(permissions) > 1: - group_clause = 'GROUP BY %s.resource_id' % RolePermission._meta.db_table + if False: + # This query does not work, but it is not apparent to me at this + # time why it does not. The intent is to be able to return a query + # set that does not involve a subselect, with the hope that this + # will perform better than our subselect method. I'm leaving it + # here for now so that I can revisit it with fresh eyes and + # hopefully find the issue. + # + # If someone else comes along and fixes or eliminates this, please + # tag me or let me know! - anoek 2016-02-11 + + qs = cls.objects for perm in permissions: - if not aggregate_where_clause: - aggregate_where_clause = 'WHERE ' - else: - aggregate_where_clause += ' AND ' - aggregate_where_clause += '"%s" = %d' % (perm, int(permissions[perm])) - aggregates += ', MAX("%s") as "%s"' % (perm, perm) - if len(permissions) == 1: - perm = list(permissions.keys())[0] - where_clause = 'AND "%s" = %d' % (perm, int(permissions[perm])) + kw = {'max_' + perm: Max('resource__permissions__' + perm)} + qs = qs.annotate(**kw) + kw = {'max_' + perm: int(permissions[perm])} + qs = qs.filter(**kw) + qs = qs.filter(resource__permissions__role__ancestors__members=user) + return qs - return cls.objects.extra( - where=[ - ''' - %(table_name)s.resource_id in ( - SELECT resource_id FROM ( - SELECT %(rbac_permission)s.resource_id %(aggregates)s - FROM %(rbac_role)s_members - LEFT JOIN %(rbac_role_hierachy)s - ON (%(rbac_role_hierachy)s.ancestor_id = %(rbac_role)s_members.role_id) - LEFT JOIN %(rbac_permission)s - ON (%(rbac_permission)s.role_id = %(rbac_role_hierachy)s.role_id) - WHERE %(rbac_role)s_members.user_id=%(user_id)d - %(where_clause)s - %(group_clause)s - ) summarized_permissions - %(aggregate_where_clause)s - ) - ''' - % - { - 'table_name' : cls._meta.db_table, - 'aggregates' : aggregates, - 'user_id' : user.id, - 'aggregate_where_clause' : aggregate_where_clause, - 'group_clause' : group_clause, - 'where_clause' : where_clause, - 'rbac_role' : Role._meta.db_table, - 'rbac_permission' : RolePermission._meta.db_table, - 'rbac_role_hierachy' : RoleHierarchy._meta.db_table - } - ] - ) + if True: + # Begin working code.. this results in a subselect which I'm not too + # thrilled about, but performs reasonably ok + + qs = Resource.objects.filter( + content_type=ContentType.objects.get_for_model(cls), + permissions__role__ancestors__members=user + ) + for perm in permissions: + kw = {'max_' + perm: Max('permissions__' + perm)} + qs = qs.annotate(**kw) + kw = {'max_' + perm: int(permissions[perm])} + qs = qs.filter(**kw) + + return cls.objects.filter(resource__in=qs) + + if False: + # This works and remains the most performant implementation. Keeping it here + # until we can dethrone it with a proper ORM implementation + + aggregate_where_clause = '' + aggregates = '' + group_clause = '' + where_clause = '' + + if len(permissions) > 1: + group_clause = 'GROUP BY %s.resource_id' % RolePermission._meta.db_table + for perm in permissions: + if not aggregate_where_clause: + aggregate_where_clause = 'WHERE ' + else: + aggregate_where_clause += ' AND ' + aggregate_where_clause += '"%s" = %d' % (perm, int(permissions[perm])) + aggregates += ', MAX("%s") as "%s"' % (perm, perm) + if len(permissions) == 1: + perm = list(permissions.keys())[0] + where_clause = 'AND "%s" = %d' % (perm, int(permissions[perm])) + + return cls.objects.extra( + where=[ + ''' + %(table_name)s.resource_id in ( + SELECT resource_id FROM ( + SELECT %(rbac_permission)s.resource_id %(aggregates)s + FROM %(rbac_role)s_members + LEFT JOIN %(rbac_role_hierachy)s + ON (%(rbac_role_hierachy)s.to_role_id = %(rbac_role)s_members.role_id) + LEFT JOIN %(rbac_permission)s + ON (%(rbac_permission)s.role_id = %(rbac_role_hierachy)s.from_role_id) + WHERE %(rbac_role)s_members.user_id=%(user_id)d + %(where_clause)s + %(group_clause)s + order by resource_id + ) summarized_permissions + %(aggregate_where_clause)s + ) + ''' + % + { + 'table_name' : cls._meta.db_table, + 'aggregates' : aggregates, + 'user_id' : user.id, + 'aggregate_where_clause' : aggregate_where_clause, + 'group_clause' : group_clause, + 'where_clause' : where_clause, + 'rbac_role' : Role._meta.db_table, + 'rbac_permission' : RolePermission._meta.db_table, + 'rbac_role_hierachy' : 'main_rbac_roles_ancestors' + } + ] + ) def get_permissions(self, user): ''' Returns a dict (or None) of the permissions a user has for a given - resource. + resource. Note: Each field in the dict is the `or` of all respective permissions that have been granted to the roles that are applicable for the given @@ -96,45 +142,79 @@ class ResourceMixin(models.Model): access. ''' + # TODO: Clean this up once we have settled on an optimal implementation - with connection.cursor() as cursor: - cursor.execute( - ''' - SELECT - MAX("create") as "create", - MAX("read") as "read", - MAX("write") as "write", - MAX("update") as "update", - MAX("delete") as "delete", - MAX("scm_update") as "scm_update", - MAX("execute") as "execute", - MAX("use") as "use" + if True: + # This works well enough at scale, but is about 5x slower than the + # raw sql variant, further optimization is desired. - FROM %(rbac_permission)s - LEFT JOIN %(rbac_role_hierachy)s - ON (%(rbac_permission)s.role_id = %(rbac_role_hierachy)s.role_id) - INNER JOIN %(rbac_role)s_members - ON ( - %(rbac_role)s_members.role_id = %(rbac_role_hierachy)s.ancestor_id - AND %(rbac_role)s_members.user_id = %(user_id)d - ) + qs = user.__class__.objects.filter(id=user.id, roles__descendents__permissions__resource=self.resource) + + qs = qs.annotate(max_create = Max('roles__descendents__permissions__create')) + qs = qs.annotate(max_read = Max('roles__descendents__permissions__read')) + qs = qs.annotate(max_write = Max('roles__descendents__permissions__write')) + qs = qs.annotate(max_update = Max('roles__descendents__permissions__update')) + qs = qs.annotate(max_delete = Max('roles__descendents__permissions__delete')) + qs = qs.annotate(max_scm_update = Max('roles__descendents__permissions__scm_update')) + qs = qs.annotate(max_execute = Max('roles__descendents__permissions__execute')) + qs = qs.annotate(max_use = Max('roles__descendents__permissions__use')) + + qs = qs.values('max_create', 'max_read', 'max_write', 'max_update', + 'max_delete', 'max_scm_update', 'max_execute', 'max_use') + + #print('###############') + #print(qs.query) + #print('###############') + + res = qs.all() + if len(res): + return {k[4:]:v for k,v in res[0].items()} + return None + + + if False: + # This works and remains the most performant implementation. Keeping it here + # until we can dethrone it with a proper ORM implementation + + with connection.cursor() as cursor: + cursor.execute( + ''' + SELECT + MAX("create") as "create", + MAX("read") as "read", + MAX("write") as "write", + MAX("update") as "update", + MAX("delete") as "delete", + MAX("scm_update") as "scm_update", + MAX("execute") as "execute", + MAX("use") as "use" + + FROM %(rbac_permission)s + LEFT JOIN main_rbac_roles_ancestors + ON (%(rbac_permission)s.role_id = main_rbac_roles_ancestors.from_role_id) + INNER JOIN %(rbac_role)s_members + ON ( + %(rbac_role)s_members.role_id = main_rbac_roles_ancestors.to_role_id + AND %(rbac_role)s_members.user_id = %(user_id)d + ) + + WHERE %(rbac_permission)s.resource_id=%(resource_id)s + GROUP BY %(rbac_role)s_members.user_id + ''' + % + { + 'user_id': user.id, + 'resource_id': self.resource.id, + 'rbac_role': Role._meta.db_table, + 'rbac_permission': RolePermission._meta.db_table, + #'rbac_role_hierachy': RoleHierarchy._meta.db_table + } + ) + row = cursor.fetchone() + if row: + return dict(zip([x[0] for x in cursor.description], row)) + return None - WHERE %(rbac_permission)s.resource_id=%(resource_id)s - GROUP BY %(rbac_role)s_members.user_id - ''' - % - { - 'user_id': user.id, - 'resource_id': self.resource.id, - 'rbac_role': Role._meta.db_table, - 'rbac_permission': RolePermission._meta.db_table, - 'rbac_role_hierachy': RoleHierarchy._meta.db_table - } - ) - row = cursor.fetchone() - if row: - return dict(zip([x[0] for x in cursor.description], row)) - return None def accessible_by(self, user, permissions): ''' diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 1268f68443..1a5c189892 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -7,11 +7,13 @@ import logging # Django from django.db import models from django.utils.translation import ugettext_lazy as _ +from django.contrib.contenttypes.models import ContentType +from django.contrib.contenttypes.fields import GenericForeignKey # AWX from awx.main.models.base import * # noqa -__all__ = ['Role', 'RolePermission', 'Resource', 'RoleHierarchy'] +__all__ = ['Role', 'RolePermission', 'Resource'] logger = logging.getLogger('awx.main.models.rbac') @@ -28,32 +30,41 @@ class Role(CommonModelNameNotUnique): singleton_name = models.TextField(null=True, default=None, db_index=True, unique=True) parents = models.ManyToManyField('Role', related_name='children') + ancestors = models.ManyToManyField('Role', related_name='descendents') # auto-generated by `rebuild_role_ancestor_list` members = models.ManyToManyField('auth.User', related_name='roles') + content_type = models.ForeignKey(ContentType, null=True, default=None) + object_id = models.PositiveIntegerField(null=True, default=None) + content_object = GenericForeignKey('content_type', 'object_id') def save(self, *args, **kwargs): super(Role, self).save(*args, **kwargs) - self.rebuild_role_hierarchy_cache() + self.rebuild_role_ancestor_list() - def rebuild_role_hierarchy_cache(self): - 'Rebuilds the associated entries in the RoleHierarchy model' + def rebuild_role_ancestor_list(self): + ''' + Updates our `ancestors` map to accurately reflect all of the ancestors for a role - # Compute what our hierarchy should be. (Note: this depends on our - # parent's cached hierarchy being correct) - parent_ids = set([parent.id for parent in self.parents.all()]) - actual_ancestors = set([r.ancestor.id for r in RoleHierarchy.objects.filter(role__id__in=parent_ids)]) + You should never need to call this. Signal handlers should be calling + this method when the role hierachy changes automatically. + + Note that this method relies on any parents' ancestor list being correct. + ''' + + actual_ancestors = set(Role.objects.filter(id=self.id).values_list('parents__ancestors__id', flat=True)) actual_ancestors.add(self.id) - - # Compute what we have stored - stored_ancestors = set([r.ancestor.id for r in RoleHierarchy.objects.filter(role__id=self.id)]) + if None in actual_ancestors: + actual_ancestors.remove(None) + stored_ancestors = set(self.ancestors.all().values_list('id', flat=True)) # If it differs, update, and then update all of our children if actual_ancestors != stored_ancestors: - RoleHierarchy.objects.filter(role__id=self.id).delete() - for id in actual_ancestors: - rh = RoleHierarchy(role=self, ancestor=Role.objects.get(id=id)) - rh.save() + for id in actual_ancestors - stored_ancestors: + self.ancestors.add(Role.objects.get(id=id)) + for id in stored_ancestors - actual_ancestors: + self.ancestors.remove(Role.objects.get(id=id)) + for child in self.children.all(): - child.rebuild_role_hierarchy_cache() + child.rebuild_role_ancestor_list() def grant(self, resource, permissions): # take either the raw Resource or something that includes the ResourceMixin @@ -85,22 +96,7 @@ class Role(CommonModelNameNotUnique): return ret def is_ancestor_of(self, role): - return RoleHierarchy.objects.filter(role_id=role.id, ancestor_id=self.id).count() > 0 - - - -class RoleHierarchy(CreatedModifiedModel): - ''' - Stores a flattened relation map of all roles in the system for easy joining - ''' - - class Meta: - app_label = 'main' - verbose_name_plural = _('role_ancestors') - db_table = 'main_rbac_role_hierarchy' - - role = models.ForeignKey('Role', related_name='+', on_delete=models.CASCADE) - ancestor = models.ForeignKey('Role', related_name='+', on_delete=models.CASCADE) + return role.ancestors.filter(id=self.id).exists() class Resource(CommonModelNameNotUnique): @@ -113,6 +109,10 @@ class Resource(CommonModelNameNotUnique): verbose_name_plural = _('resources') db_table = 'main_rbac_resources' + content_type = models.ForeignKey(ContentType, null=True, default=None) + object_id = models.PositiveIntegerField(null=True, default=None) + content_object = GenericForeignKey('content_type', 'object_id') + class RolePermission(CreatedModifiedModel): ''' From 72419f7eb9f1454b677467367a9d0d05623e4fe1 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Thu, 11 Feb 2016 16:59:32 -0500 Subject: [PATCH 3/4] Generically handle automatic role rebinding through m2m relations --- awx/main/fields.py | 129 ++++++++++++++++++++++++++++++++++---------- awx/main/signals.py | 35 ++---------- 2 files changed, 107 insertions(+), 57 deletions(-) diff --git a/awx/main/fields.py b/awx/main/fields.py index e002ab74c9..69a5cfa089 100644 --- a/awx/main/fields.py +++ b/awx/main/fields.py @@ -3,17 +3,26 @@ # Django from django.db.models.signals import post_save +from django.db.models.signals import m2m_changed from django.db import models -from django.db.models.fields.related import SingleRelatedObjectDescriptor -from django.db.models.fields.related import ReverseSingleRelatedObjectDescriptor +from django.db.models.fields.related import ( + add_lazy_relation, + SingleRelatedObjectDescriptor, + ReverseSingleRelatedObjectDescriptor, + ManyRelatedObjectsDescriptor, + ReverseManyRelatedObjectsDescriptor, +) + from django.core.exceptions import FieldError + # AWX from awx.main.models.rbac import Resource, RolePermission, Role __all__ = ['AutoOneToOneField', 'ImplicitResourceField', 'ImplicitRoleField'] + # Based on AutoOneToOneField from django-annoying: # https://bitbucket.org/offline/django-annoying/src/a0de8b294db3/annoying/fields.py @@ -44,14 +53,6 @@ class AutoOneToOneField(models.OneToOneField): -def resolve_field(obj, field): - for f in field.split('.'): - if hasattr(obj, f): - obj = getattr(obj, f) - else: - obj = None - return obj - class ResourceFieldDescriptor(ReverseSingleRelatedObjectDescriptor): """Descriptor for access to the object from its related class.""" @@ -62,7 +63,7 @@ class ResourceFieldDescriptor(ReverseSingleRelatedObjectDescriptor): resource = super(ResourceFieldDescriptor, self).__get__(instance, instance_type) if resource: return resource - resource = Resource._default_manager.create() + resource = Resource._default_manager.create(content_object=instance) setattr(instance, self.field.name, resource) instance.save(update_fields=[self.field.name,]) return resource @@ -79,7 +80,7 @@ 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.parent_resource, self)) + setattr(cls, self.name, ResourceFieldDescriptor(self)) post_save.connect(self._save, cls, True) def _save(self, instance, *args, **kwargs): @@ -106,23 +107,38 @@ class ImplicitRoleDescriptor(ReverseSingleRelatedObjectDescriptor): if not self.role_name: raise FieldError('Implicit role missing `role_name`') - role = Role._default_manager.create(name=self.role_name) + role = Role._default_manager.create(name=self.role_name, content_object=instance) if self.parent_role: - # Add all non-null parent roles as parents - if type(self.parent_role) is list: - for path in self.parent_role: - if path.startswith("singleton:"): - parent = Role.singleton(path[10:]) - else: - parent = resolve_field(instance, path) - if parent: - role.parents.add(parent) - else: - if self.parent_role.startswith("singleton:"): - parent = Role.singleton(self.parent_role[10:]) + def resolve_field(obj, field): + ret = [] + + field_components = field.split('.', 1) + if hasattr(obj, field_components[0]): + obj = getattr(obj, field_components[0]) else: - parent = resolve_field(instance, self.parent_role) - if parent: + 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] + for path in paths: + if path.startswith("singleton:"): + parents = [Role.singleton(path[10:])] + else: + parents = resolve_field(instance, path) + for parent in parents: role.parents.add(parent) setattr(instance, self.field.name, role) instance.save(update_fields=[self.field.name,]) @@ -178,6 +194,65 @@ class ImplicitRoleField(models.ForeignKey): ) ) post_save.connect(self._save, cls, True) + add_lazy_relation(cls, self, "self", self.bind_m2m_changed) + + def bind_m2m_changed(self, _self, _role_class, cls): + if not self.parent_role: + return + field_names = self.parent_role + if type(field_names) is not list: + field_names = [field_names] + + found_m2m_field = False + for field_name in field_names: + if field_name.startswith('singleton:'): + continue + + first_field_name = field_name.split('.')[0] + field = getattr(cls, first_field_name) + + if type(field) is ReverseManyRelatedObjectsDescriptor: + if found_m2m_field: + # This limitation is due to a lack of understanding on my part, the + # trouble being that I can't seem to get m2m_changed to call anything that + # encapsulates what field we're working with. So the workaround that I've + # settled on for the time being is to simply iterate over the fields in the + # m2m_update and look for the m2m map and update accordingly. This solution + # is lame, I'd love for someone to show me the way to get the necessary + # state down to the m2m_update callback. - anoek 2016-02-10 + raise Exception('Multiple ManyToMany fields not allowed in parent_role list') + if len(field_name.split('.')) != 2: + raise Exception('Referencing deep roles through ManyToMany fields is unsupported.') + + found_m2m_field = True + self.m2m_field_name = first_field_name + self.m2m_field_attr = field_name.split('.',1)[1] + m2m_changed.connect(self.m2m_update, field.through) + + if type(field) is ManyRelatedObjectsDescriptor: + raise Exception('ManyRelatedObjectsDescriptor references are currently unsupported ' + + '(but the reverse is, so supporting this is probably easy to add)): %s.%s' % + (cls.__name__, first_field_name)) + + + def m2m_update(self, sender, instance, action, reverse, model, pk_set, **kwargs): + if action == 'post_add' or action == 'pre_remove': + if reverse: + for pk in pk_set: + obj = model.objects.get(pk=pk) + if action == 'post_add': + getattr(instance, self.name).children.add(getattr(obj, self.m2m_field_attr)) + if action == 'pre_remove': + getattr(instance, self.name).children.remove(getattr(obj, self.m2m_field_attr)) + + else: + for pk in pk_set: + obj = model.objects.get(pk=pk) + if action == 'post_add': + getattr(instance, self.name).parents.add(getattr(obj, self.m2m_field_attr)) + if action == 'pre_remove': + getattr(instance, self.name).parents.remove(getattr(obj, self.m2m_field_attr)) + def _save(self, instance, *args, **kwargs): # Ensure that our field gets initialized after our first save diff --git a/awx/main/signals.py b/awx/main/signals.py index 302db7e60b..f5778dbb2e 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -18,7 +18,6 @@ from crum.signals import current_user_getter # AWX from awx.main.models import * # noqa from awx.api.serializers import * # noqa -from awx.main.fields import ImplicitRoleField from awx.main.utils import model_instance_diff, model_to_dict, camelcase_to_underscore, emit_websocket_notification from awx.main.utils import ignore_inventory_computed_fields, ignore_inventory_group_removal, _inventory_updates from awx.main.tasks import update_inventory_computed_fields @@ -116,37 +115,13 @@ def store_initial_active_state(sender, **kwargs): else: instance._saved_active_state = True -def rebuild_role_hierarchy_cache(sender, reverse, model, pk_set, **kwargs): +def rebuild_role_ancestor_list(sender, reverse, model, instance, pk_set, **kwargs): if reverse: for id in pk_set: - model.objects.get(id=id).rebuild_role_hierarchy_cache() + model.objects.get(id=id).rebuild_role_ancestor_list() else: - kwargs['instance'].rebuild_role_hierarchy_cache() + instance.rebuild_role_ancestor_list() -def rebuild_group_parent_roles(instance, action, reverse, **kwargs): - objects = [] - if reverse: - objects = instance.children.all() - else: - objects = instance.parents.all() - - for obj in objects: - fields = [f for f in instance._meta.get_fields() if type(f) is ImplicitRoleField] - for field in fields: - role = None - if reverse: - if hasattr(obj, field.name): - parent_role = getattr(instance, field.name) - role = getattr(obj, field.name) - else: - role = getattr(instance, field.name) - parent_role = getattr(obj, field.name) - - if role: - if action == 'post_add': - role.parents.add(parent_role) - elif action == 'pre_remove': - role.parents.remove(parent_role) pre_save.connect(store_initial_active_state, sender=Host) post_save.connect(emit_update_inventory_on_created_or_deleted, sender=Host) @@ -166,8 +141,8 @@ post_save.connect(emit_update_inventory_on_created_or_deleted, sender=Job) post_delete.connect(emit_update_inventory_on_created_or_deleted, sender=Job) post_save.connect(emit_job_event_detail, sender=JobEvent) post_save.connect(emit_ad_hoc_command_event_detail, sender=AdHocCommandEvent) -m2m_changed.connect(rebuild_role_hierarchy_cache, Role.parents.through) -m2m_changed.connect(rebuild_group_parent_roles, Group.parents.through) +m2m_changed.connect(rebuild_role_ancestor_list, Role.parents.through) +#m2m_changed.connect(rebuild_group_parent_roles, Group.parents.through) # Migrate hosts, groups to parent group(s) whenever a group is deleted or # marked as inactive. From 319252f555ea0514ba024ddc0daec89ccae50390 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Fri, 12 Feb 2016 10:16:29 -0500 Subject: [PATCH 4/4] Finish removing our raw SQL implemenations from our mixins Boiled out our current-best ORM implemenations. These can likely be optimized further, but are adequate for the time being. --- awx/main/models/mixins.py | 188 +++++--------------------------------- 1 file changed, 25 insertions(+), 163 deletions(-) diff --git a/awx/main/models/mixins.py b/awx/main/models/mixins.py index 3b7b5eb8d4..effdc7d436 100644 --- a/awx/main/models/mixins.py +++ b/awx/main/models/mixins.py @@ -1,11 +1,10 @@ # Django from django.db import models -from django.db import connection from django.db.models.aggregates import Max from django.contrib.contenttypes.models import ContentType # AWX -from awx.main.models.rbac import RolePermission, Role, Resource +from awx.main.models.rbac import Resource from awx.main.fields import ImplicitResourceField @@ -32,99 +31,15 @@ class ResourceMixin(models.Model): `myresource.get_permissions(user)`. ''' - # TODO: Clean this up once we have settled on an optimal implementation + qs = Resource.objects.filter( + content_type=ContentType.objects.get_for_model(cls), + permissions__role__ancestors__members=user + ) + for perm in permissions: + qs = qs.annotate(**{'max_' + perm: Max('permissions__' + perm)}) + qs = qs.filter(**{'max_' + perm: int(permissions[perm])}) - if False: - # This query does not work, but it is not apparent to me at this - # time why it does not. The intent is to be able to return a query - # set that does not involve a subselect, with the hope that this - # will perform better than our subselect method. I'm leaving it - # here for now so that I can revisit it with fresh eyes and - # hopefully find the issue. - # - # If someone else comes along and fixes or eliminates this, please - # tag me or let me know! - anoek 2016-02-11 - - qs = cls.objects - for perm in permissions: - kw = {'max_' + perm: Max('resource__permissions__' + perm)} - qs = qs.annotate(**kw) - kw = {'max_' + perm: int(permissions[perm])} - qs = qs.filter(**kw) - qs = qs.filter(resource__permissions__role__ancestors__members=user) - return qs - - if True: - # Begin working code.. this results in a subselect which I'm not too - # thrilled about, but performs reasonably ok - - qs = Resource.objects.filter( - content_type=ContentType.objects.get_for_model(cls), - permissions__role__ancestors__members=user - ) - for perm in permissions: - kw = {'max_' + perm: Max('permissions__' + perm)} - qs = qs.annotate(**kw) - kw = {'max_' + perm: int(permissions[perm])} - qs = qs.filter(**kw) - - return cls.objects.filter(resource__in=qs) - - if False: - # This works and remains the most performant implementation. Keeping it here - # until we can dethrone it with a proper ORM implementation - - aggregate_where_clause = '' - aggregates = '' - group_clause = '' - where_clause = '' - - if len(permissions) > 1: - group_clause = 'GROUP BY %s.resource_id' % RolePermission._meta.db_table - for perm in permissions: - if not aggregate_where_clause: - aggregate_where_clause = 'WHERE ' - else: - aggregate_where_clause += ' AND ' - aggregate_where_clause += '"%s" = %d' % (perm, int(permissions[perm])) - aggregates += ', MAX("%s") as "%s"' % (perm, perm) - if len(permissions) == 1: - perm = list(permissions.keys())[0] - where_clause = 'AND "%s" = %d' % (perm, int(permissions[perm])) - - return cls.objects.extra( - where=[ - ''' - %(table_name)s.resource_id in ( - SELECT resource_id FROM ( - SELECT %(rbac_permission)s.resource_id %(aggregates)s - FROM %(rbac_role)s_members - LEFT JOIN %(rbac_role_hierachy)s - ON (%(rbac_role_hierachy)s.to_role_id = %(rbac_role)s_members.role_id) - LEFT JOIN %(rbac_permission)s - ON (%(rbac_permission)s.role_id = %(rbac_role_hierachy)s.from_role_id) - WHERE %(rbac_role)s_members.user_id=%(user_id)d - %(where_clause)s - %(group_clause)s - order by resource_id - ) summarized_permissions - %(aggregate_where_clause)s - ) - ''' - % - { - 'table_name' : cls._meta.db_table, - 'aggregates' : aggregates, - 'user_id' : user.id, - 'aggregate_where_clause' : aggregate_where_clause, - 'group_clause' : group_clause, - 'where_clause' : where_clause, - 'rbac_role' : Role._meta.db_table, - 'rbac_permission' : RolePermission._meta.db_table, - 'rbac_role_hierachy' : 'main_rbac_roles_ancestors' - } - ] - ) + return cls.objects.filter(resource__in=qs) def get_permissions(self, user): @@ -142,78 +57,25 @@ class ResourceMixin(models.Model): access. ''' - # TODO: Clean this up once we have settled on an optimal implementation + qs = user.__class__.objects.filter(id=user.id, roles__descendents__permissions__resource=self.resource) - if True: - # This works well enough at scale, but is about 5x slower than the - # raw sql variant, further optimization is desired. + qs = qs.annotate(max_create = Max('roles__descendents__permissions__create')) + qs = qs.annotate(max_read = Max('roles__descendents__permissions__read')) + qs = qs.annotate(max_write = Max('roles__descendents__permissions__write')) + qs = qs.annotate(max_update = Max('roles__descendents__permissions__update')) + qs = qs.annotate(max_delete = Max('roles__descendents__permissions__delete')) + qs = qs.annotate(max_scm_update = Max('roles__descendents__permissions__scm_update')) + qs = qs.annotate(max_execute = Max('roles__descendents__permissions__execute')) + qs = qs.annotate(max_use = Max('roles__descendents__permissions__use')) - qs = user.__class__.objects.filter(id=user.id, roles__descendents__permissions__resource=self.resource) + qs = qs.values('max_create', 'max_read', 'max_write', 'max_update', + 'max_delete', 'max_scm_update', 'max_execute', 'max_use') - qs = qs.annotate(max_create = Max('roles__descendents__permissions__create')) - qs = qs.annotate(max_read = Max('roles__descendents__permissions__read')) - qs = qs.annotate(max_write = Max('roles__descendents__permissions__write')) - qs = qs.annotate(max_update = Max('roles__descendents__permissions__update')) - qs = qs.annotate(max_delete = Max('roles__descendents__permissions__delete')) - qs = qs.annotate(max_scm_update = Max('roles__descendents__permissions__scm_update')) - qs = qs.annotate(max_execute = Max('roles__descendents__permissions__execute')) - qs = qs.annotate(max_use = Max('roles__descendents__permissions__use')) - - qs = qs.values('max_create', 'max_read', 'max_write', 'max_update', - 'max_delete', 'max_scm_update', 'max_execute', 'max_use') - - #print('###############') - #print(qs.query) - #print('###############') - - res = qs.all() - if len(res): - return {k[4:]:v for k,v in res[0].items()} - return None - - - if False: - # This works and remains the most performant implementation. Keeping it here - # until we can dethrone it with a proper ORM implementation - - with connection.cursor() as cursor: - cursor.execute( - ''' - SELECT - MAX("create") as "create", - MAX("read") as "read", - MAX("write") as "write", - MAX("update") as "update", - MAX("delete") as "delete", - MAX("scm_update") as "scm_update", - MAX("execute") as "execute", - MAX("use") as "use" - - FROM %(rbac_permission)s - LEFT JOIN main_rbac_roles_ancestors - ON (%(rbac_permission)s.role_id = main_rbac_roles_ancestors.from_role_id) - INNER JOIN %(rbac_role)s_members - ON ( - %(rbac_role)s_members.role_id = main_rbac_roles_ancestors.to_role_id - AND %(rbac_role)s_members.user_id = %(user_id)d - ) - - WHERE %(rbac_permission)s.resource_id=%(resource_id)s - GROUP BY %(rbac_role)s_members.user_id - ''' - % - { - 'user_id': user.id, - 'resource_id': self.resource.id, - 'rbac_role': Role._meta.db_table, - 'rbac_permission': RolePermission._meta.db_table, - #'rbac_role_hierachy': RoleHierarchy._meta.db_table - } - ) - row = cursor.fetchone() - if row: - return dict(zip([x[0] for x in cursor.description], row)) - return None + res = qs.all() + if len(res): + # strip away the 'max_' prefix + return {k[4:]:v for k,v in res[0].items()} + return None def accessible_by(self, user, permissions):