From 325f5250db18e233fa29200a54ef2e32d781311e Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Thu, 14 Dec 2023 21:30:47 -0500 Subject: [PATCH] Narrow the actor types accepted for RBAC evaluations (#14709) * Narrow the scope of RBAC evaluations * Update tests for RBAC method changes * Simplify querset for credentials in org * Fix call pattern to pass in team role obj --- awx/api/views/__init__.py | 6 +++--- awx/main/models/mixins.py | 4 +--- awx/main/models/rbac.py | 9 +-------- awx/main/tests/functional/test_rbac_core.py | 4 ++-- awx/main/tests/functional/test_rbac_team.py | 4 ++-- 5 files changed, 9 insertions(+), 18 deletions(-) diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 221db21b00..80fc152bf4 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -742,8 +742,8 @@ class TeamActivityStreamList(SubListAPIView): qs = self.request.user.get_queryset(self.model) return qs.filter( Q(team=parent) - | Q(project__in=models.Project.accessible_objects(parent, 'read_role')) - | Q(credential__in=models.Credential.accessible_objects(parent, 'read_role')) + | Q(project__in=models.Project.accessible_objects(parent.member_role, 'read_role')) + | Q(credential__in=models.Credential.accessible_objects(parent.member_role, 'read_role')) ) @@ -1397,7 +1397,7 @@ class OrganizationCredentialList(SubListCreateAPIView): self.check_parent_access(organization) user_visible = models.Credential.accessible_objects(self.request.user, 'read_role').all() - org_set = models.Credential.accessible_objects(organization.admin_role, 'read_role').all() + org_set = models.Credential.objects.filter(organization=organization) if self.request.user.is_superuser or self.request.user.is_system_auditor: return org_set diff --git a/awx/main/models/mixins.py b/awx/main/models/mixins.py index 8f7ed9ade3..3c8d8ae1ef 100644 --- a/awx/main/models/mixins.py +++ b/awx/main/models/mixins.py @@ -9,7 +9,6 @@ import requests # Django from django.apps import apps from django.conf import settings -from django.contrib.auth.models import User # noqa from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ValidationError from django.db import models @@ -69,8 +68,7 @@ class ResourceMixin(models.Model): elif type(accessor) == Role: ancestor_roles = [accessor] else: - accessor_type = ContentType.objects.get_for_model(accessor) - ancestor_roles = Role.objects.filter(content_type__pk=accessor_type.id, object_id=accessor.id) + raise RuntimeError(f'Role filters only valid for users and ancestor role, received {accessor}') if content_types is None: ct_kwarg = dict(content_type_id=ContentType.objects.get_for_model(cls).id) diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 6b1ecb6bf1..9078436404 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -15,7 +15,6 @@ from django.utils.translation import gettext_lazy as _ # AWX from awx.api.versioning import reverse -from django.contrib.auth.models import User # noqa __all__ = [ 'Role', @@ -171,14 +170,8 @@ class Role(models.Model): def __contains__(self, accessor): if accessor._meta.model_name == 'user': return self.ancestors.filter(members=accessor).exists() - elif accessor.__class__.__name__ == 'Team': - return self.ancestors.filter(pk=accessor.member_role.id).exists() - elif type(accessor) == Role: - return self.ancestors.filter(pk=accessor.pk).exists() else: - accessor_type = ContentType.objects.get_for_model(accessor) - roles = Role.objects.filter(content_type__pk=accessor_type.id, object_id=accessor.id) - return self.ancestors.filter(pk__in=roles).exists() + raise RuntimeError(f'Role evaluations only valid for users, received {accessor}') @property def name(self): diff --git a/awx/main/tests/functional/test_rbac_core.py b/awx/main/tests/functional/test_rbac_core.py index 7029bbe544..1fa0f11ed5 100644 --- a/awx/main/tests/functional/test_rbac_core.py +++ b/awx/main/tests/functional/test_rbac_core.py @@ -208,6 +208,6 @@ def test_auto_parenting(): @pytest.mark.django_db def test_update_parents_keeps_teams(team, project): project.update_role.parents.add(team.member_role) - assert team.member_role in project.update_role # test prep sanity check + assert list(Project.accessible_objects(team.member_role, 'update_role')) == [project] # test prep sanity check update_role_parentage_for_instance(project) - assert team.member_role in project.update_role # actual assertion + assert list(Project.accessible_objects(team.member_role, 'update_role')) == [project] # actual assertion diff --git a/awx/main/tests/functional/test_rbac_team.py b/awx/main/tests/functional/test_rbac_team.py index a18a69a94b..177923b2bf 100644 --- a/awx/main/tests/functional/test_rbac_team.py +++ b/awx/main/tests/functional/test_rbac_team.py @@ -92,7 +92,7 @@ def test_team_accessible_by(team, user, project): u = user('team_member', False) team.member_role.children.add(project.use_role) - assert team in project.read_role + assert list(Project.accessible_objects(team.member_role, 'read_role')) == [project] assert u not in project.read_role team.member_role.members.add(u) @@ -104,7 +104,7 @@ def test_team_accessible_objects(team, user, project): u = user('team_member', False) team.member_role.children.add(project.use_role) - assert len(Project.accessible_objects(team, 'read_role')) == 1 + assert len(Project.accessible_objects(team.member_role, 'read_role')) == 1 assert not Project.accessible_objects(u, 'read_role') team.member_role.members.add(u)