From 6250d9f7e75731b1df04fba6f4ee5e825dd31874 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Fri, 22 Apr 2016 12:09:19 -0400 Subject: [PATCH 1/9] Optimized RBAC visible_roles query --- awx/main/models/rbac.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 64e748a20a..3ee63c8497 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -348,7 +348,22 @@ class Role(models.Model): @staticmethod def visible_roles(user): - return Role.objects.filter(Q(descendents__in=user.roles.filter()) | Q(ancestors__in=user.roles.filter())) + sql_params = { + 'ancestors_table': Role.ancestors.through._meta.db_table, + 'parents_table': Role.parents.through._meta.db_table, + 'roles_table': Role._meta.db_table, + 'ids': ','.join(str(x) for x in user.roles.values_list('id', flat=True)) + } + qs = Role.objects.extra( + where = [''' + %(roles_table)s.id IN ( + SELECT descendent_id FROM %(ancestors_table)s WHERE ancestor_id IN (%(ids)s) + UNION + SELECT ancestor_id FROM %(ancestors_table)s WHERE descendent_id IN (%(ids)s) + ) + ''' % sql_params] + ) + return qs @staticmethod def singleton(name): From 754f8546a6a7a6f419eec2613e9e6063828b0b2a Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Fri, 22 Apr 2016 15:17:20 -0400 Subject: [PATCH 2/9] Switched /api/v1/roles/ to a cursor paginator so we don't have to do a count() on that potentially very large result set --- awx/api/serializers.py | 9 +++++++-- awx/api/views.py | 4 ++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 5dae65e338..ec497d97f6 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -1443,8 +1443,13 @@ class RoleSerializer(BaseSerializer): class Meta: model = Role - fields = ('*', 'description', 'name') - read_only_fields = ('description', 'name') + read_only_fields = ('id', 'role_field', 'description', 'name') + + def to_representation(self, obj): + ret = super(RoleSerializer, self).to_representation(obj) + ret.pop('created') + ret.pop('modified') + return ret def get_related(self, obj): ret = super(RoleSerializer, self).get_related(obj) diff --git a/awx/api/views.py b/awx/api/views.py index 3e70cb8099..be7dcbb215 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -38,6 +38,7 @@ from rest_framework.permissions import AllowAny, IsAuthenticated from rest_framework.response import Response from rest_framework.settings import api_settings from rest_framework.views import exception_handler +from rest_framework.pagination import CursorPagination from rest_framework import status # Django REST Framework YAML @@ -3475,6 +3476,9 @@ class RoleList(ListAPIView): model = Role serializer_class = RoleSerializer permission_classes = (IsAuthenticated,) + class CursorPaginationById(CursorPagination): + ordering = 'id' + pagination_class = CursorPaginationById new_in_300 = True def get_queryset(self): From 8e4d013342fdd784686053e3af9ee0413da04c33 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 25 Apr 2016 09:06:40 -0400 Subject: [PATCH 3/9] Optimized /api/v1/hosts/ --- awx/main/access.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index ae99592afe..ec1ad16317 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -379,12 +379,16 @@ class HostAccess(BaseAccess): def get_queryset(self): inv_qs = Inventory.accessible_objects(self.user, 'read_role') - group_qs = Group.accessible_objects(self.user, 'read_role') - qs = (self.model.objects.filter(inventory=inv_qs) | self.model.objects.filter(groups=group_qs)).distinct() - #qs = qs.select_related('created_by', 'modified_by', 'inventory', - # 'last_job__job_template', - # 'last_job_host_summary__job') - #return qs.prefetch_related('groups').all() + group_qs = Group.accessible_objects(self.user, 'read_role').exclude(inventory__in=inv_qs) + if group_qs.count(): + qs = self.model.objects.filter(Q(inventory__in=inv_qs) | Q(groups__in=group_qs)) + else: + qs = self.model.objects.filter(inventory__in=inv_qs) + + qs = qs.select_related('created_by', 'modified_by', 'inventory', + 'last_job__job_template', + 'last_job_host_summary__job') + qs =qs.prefetch_related('groups').all() return qs def can_read(self, obj): @@ -491,7 +495,7 @@ class InventorySourceAccess(BaseAccess): def get_queryset(self): qs = self.model.objects.all() qs = qs.select_related('created_by', 'modified_by', 'group', 'inventory') - inventory_ids = set(self.user.get_queryset(Inventory).values_list('id', flat=True)) + inventory_ids = self.user.get_queryset(Inventory) return qs.filter(Q(inventory_id__in=inventory_ids) | Q(group__inventory_id__in=inventory_ids)) From 9df157c9711c5f6e5ff97298fa915c444a02b071 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 25 Apr 2016 11:17:15 -0400 Subject: [PATCH 4/9] Added gfk index pair for Role for our access_list queries --- awx/main/migrations/0008_v300_rbac_changes.py | 4 ++++ awx/main/models/rbac.py | 3 +++ 2 files changed, 7 insertions(+) diff --git a/awx/main/migrations/0008_v300_rbac_changes.py b/awx/main/migrations/0008_v300_rbac_changes.py index 896e81558a..dd112700cf 100644 --- a/awx/main/migrations/0008_v300_rbac_changes.py +++ b/awx/main/migrations/0008_v300_rbac_changes.py @@ -129,6 +129,10 @@ class Migration(migrations.Migration): name='ancestors', field=models.ManyToManyField(related_name='descendents', through='main.RoleAncestorEntry', to='main.Role'), ), + migrations.AlterIndexTogether( + name='role', + index_together=set([('content_type', 'object_id')]), + ), migrations.AlterIndexTogether( name='roleancestorentry', index_together=set([('ancestor', 'content_type_id', 'object_id'), ('ancestor', 'content_type_id', 'role_field'), ('ancestor', 'descendent')]), diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 3ee63c8497..ed3e9272d5 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -106,6 +106,9 @@ class Role(models.Model): app_label = 'main' verbose_name_plural = _('roles') db_table = 'main_rbac_roles' + index_together = [ + ("content_type", "object_id") + ] role_field = models.TextField(null=False) singleton_name = models.TextField(null=True, default=None, db_index=True, unique=True) From 4c15374b05bb4f708644bd4be2f4058420959206 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 25 Apr 2016 14:07:32 -0400 Subject: [PATCH 5/9] Optimized (user|team)/:n/roles/ --- awx/api/views.py | 9 ++++++--- awx/main/models/rbac.py | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index be7dcbb215..3b0942dc65 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -816,7 +816,8 @@ class TeamRolesList(SubListCreateAttachDetachAPIView): def get_queryset(self): team = Team.objects.get(pk=self.kwargs['pk']) - return team.member_role.children.filter(id__in=Role.visible_roles(self.request.user)) + #return team.member_role.children.filter(id__in=Role.visible_roles(self.request.user)) + return Role.filter_visible_roles(self.request.user, team.member_role.children.all()) # XXX: Need to enforce permissions def post(self, request, *args, **kwargs): @@ -1082,8 +1083,10 @@ class UserRolesList(SubListCreateAttachDetachAPIView): permission_classes = (IsAuthenticated,) def get_queryset(self): - #u = User.objects.get(pk=self.kwargs['pk']) - return Role.visible_roles(self.request.user).filter(members__in=[int(self.kwargs['pk']), ]) + u = get_object_or_404(User, pk=self.kwargs['pk']) + if not self.request.user.can_access(User, 'read', u): + raise PermissionDenied() + return Role.filter_visible_roles(self.request.user, u.roles.all()) def post(self, request, *args, **kwargs): # Forbid implicit role creation here diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index ed3e9272d5..54d10e39f1 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -357,6 +357,7 @@ class Role(models.Model): 'roles_table': Role._meta.db_table, 'ids': ','.join(str(x) for x in user.roles.values_list('id', flat=True)) } + qs = Role.objects.extra( where = [''' %(roles_table)s.id IN ( @@ -368,6 +369,26 @@ class Role(models.Model): ) return qs + @staticmethod + def filter_visible_roles(user, roles_qs): + sql_params = { + 'ancestors_table': Role.ancestors.through._meta.db_table, + 'parents_table': Role.parents.through._meta.db_table, + 'roles_table': Role._meta.db_table, + 'ids': ','.join(str(x) for x in user.roles.all().values_list('id', flat=True)) + } + + qs = roles_qs.extra( + where = [''' + EXISTS ( + SELECT 1 FROM + %(ancestors_table)s + WHERE (descendent_id = %(roles_table)s.id AND ancestor_id IN (%(ids)s)) + OR (ancestor_id = %(roles_table)s.id AND descendent_id IN (%(ids)s)) + ) ''' % sql_params] + ) + return qs + @staticmethod def singleton(name): role, _ = Role.objects.get_or_create(singleton_name=name, role_field=name) From d0e9044dad5140c82c5c69c9c19eee3542e65b77 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 25 Apr 2016 14:12:07 -0400 Subject: [PATCH 6/9] Enforce team access permissions on team/:n/roles --- awx/api/views.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index 3b0942dc65..be4cf4fd8b 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -815,8 +815,9 @@ class TeamRolesList(SubListCreateAttachDetachAPIView): relationship='member_role.children' def get_queryset(self): - team = Team.objects.get(pk=self.kwargs['pk']) - #return team.member_role.children.filter(id__in=Role.visible_roles(self.request.user)) + team = get_object_or_404(Team, pk=self.kwargs['pk']) + if not self.request.user.can_access(Team, 'read', team): + raise PermissionDenied() return Role.filter_visible_roles(self.request.user, team.member_role.children.all()) # XXX: Need to enforce permissions From 30ed9ab740f5ff8d3c4d4c52a1017b8ae4d2c7cd Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 25 Apr 2016 14:13:05 -0400 Subject: [PATCH 7/9] Reverted cursor pagination for /roles/ Turns out it doesn't play well with our custom filters, and performance is still tolerable with the normal pagination.. --- awx/api/views.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index be4cf4fd8b..1e13771ef8 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -3480,9 +3480,6 @@ class RoleList(ListAPIView): model = Role serializer_class = RoleSerializer permission_classes = (IsAuthenticated,) - class CursorPaginationById(CursorPagination): - ordering = 'id' - pagination_class = CursorPaginationById new_in_300 = True def get_queryset(self): From 40147f28f8673495fd53fab8ac817736aba2a8ce Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 25 Apr 2016 14:19:02 -0400 Subject: [PATCH 8/9] Updated test --- awx/main/tests/functional/test_rbac_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/awx/main/tests/functional/test_rbac_api.py b/awx/main/tests/functional/test_rbac_api.py index 9c1fd34356..2297868aa6 100644 --- a/awx/main/tests/functional/test_rbac_api.py +++ b/awx/main/tests/functional/test_rbac_api.py @@ -96,6 +96,7 @@ def test_user_view_other_user_roles(organization, inventory, team, get, alice, b 'Users can see roles for other users, but only the roles that that user has access to see as well' organization.member_role.members.add(alice) organization.admin_role.members.add(bob) + organization.member_role.members.add(bob) custom_role = Role.objects.create(name='custom_role-test_user_view_admin_roles_list') organization.member_role.children.add(custom_role) team.member_role.members.add(bob) From 3ffefd30a378302aa199278e5af151dea4974fef Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Mon, 25 Apr 2016 14:43:03 -0400 Subject: [PATCH 9/9] flake8 --- awx/api/views.py | 1 - awx/main/models/rbac.py | 1 - 2 files changed, 2 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index 1e13771ef8..81351a9066 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -38,7 +38,6 @@ from rest_framework.permissions import AllowAny, IsAuthenticated from rest_framework.response import Response from rest_framework.settings import api_settings from rest_framework.views import exception_handler -from rest_framework.pagination import CursorPagination from rest_framework import status # Django REST Framework YAML diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 54d10e39f1..d2869fca38 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -8,7 +8,6 @@ import contextlib # Django from django.db import models, transaction, connection -from django.db.models import Q from django.core.urlresolvers import reverse from django.utils.translation import ugettext_lazy as _ from django.contrib.contenttypes.models import ContentType