From 96020f4879539f8cd2e2d45f5e1eae8a4a1d711a Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Thu, 20 Jun 2024 12:00:40 -0400 Subject: [PATCH 1/5] Add initial test for deletion of stale permission --- awx/main/migrations/0195_EE_permissions.py | 17 +++++++++++++++++ awx/main/models/execution_environments.py | 2 ++ awx/main/tests/functional/test_migrations.py | 7 +++++++ 3 files changed, 26 insertions(+) create mode 100644 awx/main/migrations/0195_EE_permissions.py diff --git a/awx/main/migrations/0195_EE_permissions.py b/awx/main/migrations/0195_EE_permissions.py new file mode 100644 index 000000000000..70a72787cb04 --- /dev/null +++ b/awx/main/migrations/0195_EE_permissions.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.6 on 2024-06-20 15:55 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('main', '0194_alter_inventorysource_source_and_more'), + ] + + operations = [ + migrations.AlterModelOptions( + name='executionenvironment', + options={'default_permissions': ('add', 'change', 'delete'), 'ordering': ('-created',)}, + ), + ] diff --git a/awx/main/models/execution_environments.py b/awx/main/models/execution_environments.py index 55ce69098b9a..3c6cf1d0a99c 100644 --- a/awx/main/models/execution_environments.py +++ b/awx/main/models/execution_environments.py @@ -12,6 +12,8 @@ class ExecutionEnvironment(CommonModel): class Meta: ordering = ('-created',) + # Remove view permission, as a temporary solution, defer to organization read permission + default_permissions = ('add', 'change', 'delete') PULL_CHOICES = [ ('always', _("Always pull container before running.")), diff --git a/awx/main/tests/functional/test_migrations.py b/awx/main/tests/functional/test_migrations.py index 41eab9dd3852..74e446ce3b9c 100644 --- a/awx/main/tests/functional/test_migrations.py +++ b/awx/main/tests/functional/test_migrations.py @@ -92,3 +92,10 @@ def test_migrate_DAB_RBAC(self, migrator): # Test special cases in managed role creation assert not RoleDefinition.objects.filter(name='Organization Team Admin').exists() assert not RoleDefinition.objects.filter(name='Organization InstanceGroup Admin').exists() + + # Test that a removed EE model permission has been deleted + new_state = migrator.apply_tested_migration( + ('main', '0195_EE_permissions'), + ) + DABPermission = new_state.apps.get_model('dab_rbac', 'DABPermission') + assert not DABPermission.objects.filter(codename='view_executionenvironment').exists() From fccc6479f4eaff78eca0f894754ac838100cfba2 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Wed, 26 Jun 2024 08:24:44 -0400 Subject: [PATCH 2/5] Delete existing EE view permission --- awx/main/migrations/0195_EE_permissions.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/awx/main/migrations/0195_EE_permissions.py b/awx/main/migrations/0195_EE_permissions.py index 70a72787cb04..8216d474f30f 100644 --- a/awx/main/migrations/0195_EE_permissions.py +++ b/awx/main/migrations/0195_EE_permissions.py @@ -1,6 +1,14 @@ # Generated by Django 4.2.6 on 2024-06-20 15:55 -from django.db import migrations, models +from django.db import migrations + + +def delete_execution_environment_read_role(apps, schema_editor): + permission_classes = [apps.get_model('auth', 'Permission'), apps.get_model('dab_rbac', 'DABPermission')] + for permission_cls in permission_classes: + ee_read_perm = permission_cls.objects.filter(codename='view_executionenvironment').first() + if ee_read_perm: + ee_read_perm.delete() class Migration(migrations.Migration): @@ -14,4 +22,5 @@ class Migration(migrations.Migration): name='executionenvironment', options={'default_permissions': ('add', 'change', 'delete'), 'ordering': ('-created',)}, ), + migrations.RunPython(delete_execution_environment_read_role, migrations.RunPython.noop), ] From d6461e95051d640f19908b2dbc96d4d63bb1a6c0 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Wed, 26 Jun 2024 11:53:31 -0400 Subject: [PATCH 3/5] Hypothetically complete update of EE model permissions setup --- awx/main/access.py | 11 +- awx/main/migrations/_dab_rbac.py | 3 +- awx/main/models/execution_environments.py | 15 +++ .../test_rbac_execution_environment.py | 100 ++++++++++++++++++ 4 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 awx/main/tests/functional/test_rbac_execution_environment.py diff --git a/awx/main/access.py b/awx/main/access.py index 9819f9d9aa07..ae39bba1b135 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -1387,12 +1387,11 @@ def can_unattach(self, obj, sub_obj, relationship, *args, **kwargs): class ExecutionEnvironmentAccess(BaseAccess): """ I can see an execution environment when: - - I'm a superuser - - I'm a member of the same organization - - it is a global ExecutionEnvironment + - I can see its organization + - It is a global ExecutionEnvironment I can create/change an execution environment when: - I'm a superuser - - I'm an admin for the organization(s) + - I have an organization or object role that gives access """ model = ExecutionEnvironment @@ -1416,7 +1415,7 @@ def can_change(self, obj, data): raise PermissionDenied if settings.ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED: if not self.user.has_obj_perm(obj, 'change'): - raise PermissionDenied + return False else: if self.user not in obj.organization.execution_environment_admin_role: raise PermissionDenied @@ -1424,7 +1423,7 @@ def can_change(self, obj, data): new_org = get_object_from_data('organization', Organization, data, obj=obj) if not new_org or self.user not in new_org.execution_environment_admin_role: return False - return self.check_related('organization', Organization, data, obj=obj, mandatory=True, role_field='execution_environment_admin_role') + return self.check_related('organization', Organization, data, obj=obj, role_field='execution_environment_admin_role') def can_delete(self, obj): if obj.managed: diff --git a/awx/main/migrations/_dab_rbac.py b/awx/main/migrations/_dab_rbac.py index 4ffe46890fb8..1263414f0214 100644 --- a/awx/main/migrations/_dab_rbac.py +++ b/awx/main/migrations/_dab_rbac.py @@ -134,8 +134,7 @@ def get_permissions_for_role(role_field, children_map, apps): # more special cases for those same above special org-level roles if role_field.name == 'auditor_role': - for codename in ('view_notificationtemplate', 'view_executionenvironment'): - perm_list.append(Permission.objects.get(codename=codename)) + perm_list.append(Permission.objects.get(codename='view_notificationtemplate')) return perm_list diff --git a/awx/main/models/execution_environments.py b/awx/main/models/execution_environments.py index 3c6cf1d0a99c..5d55b55caf77 100644 --- a/awx/main/models/execution_environments.py +++ b/awx/main/models/execution_environments.py @@ -1,6 +1,8 @@ from django.db import models from django.utils.translation import gettext_lazy as _ +from rest_framework.exceptions import ValidationError + from awx.api.versioning import reverse from awx.main.models.base import CommonModel from awx.main.validators import validate_container_image_name @@ -55,3 +57,16 @@ class Meta: def get_absolute_url(self, request=None): return reverse('api:execution_environment_detail', kwargs={'pk': self.pk}, request=request) + + def validate_role_assignment(self, actor, role_definition): + if self.managed: + raise ValidationError(_('Can not assign object roles to managed Execution Environments')) + if self.organization_id is None: + raise ValidationError(_('Can not assign object roles to global Execution Environments')) + + if actor._meta.model_name == 'user' and (not actor.has_obj_perm(self.organization, 'view')): + raise ValidationError(_('User must have view permission to Execution Environment organization')) + if actor._meta.model_name == 'team': + organization_cls = self._meta.get_field('organization').related_model + if self.orgaanization not in organization_cls.access_qs(actor, 'view'): + raise ValidationError(_('Team must have view permission to Execution Environment organization')) diff --git a/awx/main/tests/functional/test_rbac_execution_environment.py b/awx/main/tests/functional/test_rbac_execution_environment.py new file mode 100644 index 000000000000..dd2440f66f6a --- /dev/null +++ b/awx/main/tests/functional/test_rbac_execution_environment.py @@ -0,0 +1,100 @@ +import pytest + +from django.contrib.contenttypes.models import ContentType + +from awx.main.access import ExecutionEnvironmentAccess +from awx.main.models import ExecutionEnvironment, Organization +from awx.main.models.rbac import get_role_codenames + +from awx.api.versioning import reverse +from django.urls import reverse as django_reverse + +from ansible_base.rbac.models import RoleDefinition + + +@pytest.fixture +def ee_rd(): + return RoleDefinition.objects.create_from_permissions( + name='EE object admin', + permissions=['change_executionenvironment', 'delete_executionenvironment'], + content_type=ContentType.objects.get_for_model(ExecutionEnvironment), + ) + + +@pytest.fixture +def org_ee_rd(): + return RoleDefinition.objects.create_from_permissions( + name='EE org admin', + permissions=['change_executionenvironment', 'delete_executionenvironment', 'view_organization'], + content_type=ContentType.objects.get_for_model(Organization), + ) + + +@pytest.mark.django_db +def test_old_ee_role_maps_to_correct_permissions(organization): + assert set(get_role_codenames(organization.execution_environment_admin_role)) == { + 'view_organization', + 'add_executionenvironment', + 'change_executionenvironment', + 'delete_executionenvironment', + } + + +@pytest.fixture +def org_ee(organization): + return ExecutionEnvironment.objects.create(name='some user ee', organization=organization) + + +@pytest.mark.django_db +def test_managed_ee_not_assignable(control_plane_execution_environment, ee_rd, rando, admin_user, post): + url = django_reverse('roleuserassignment-list') + r = post(url, {'role_definition': ee_rd.pk, 'user': rando.id, 'object_id': control_plane_execution_environment.pk}, user=admin_user, expect=400) + assert 'foo' in str(r.data) + + +@pytest.mark.django_db +def test_org_member_required_for_assignment(org_ee, ee_rd, rando, admin_user, post): + url = django_reverse('roleuserassignment-list') + r = post(url, {'role_definition': ee_rd.pk, 'user': rando.id, 'object_id': org_ee.pk}, user=admin_user, expect=400) + assert 'foo' in str(r.data) + + +@pytest.fixture +def check_user_capabilities(get): + def _rf(user, obj, expected): + url = reverse('api:execution_environment_list') + r = get(url, user=user, expect=200) + for item in r.data['results']: + if item['id'] == obj.pk: + assert expected == item['summary_fields']['user_capabilities'] + break + else: + raise RuntimeError(f'Could not find expected object ({obj}) in EE list result: {r.data}') + + return _rf + + +@pytest.mark.django_db +def test_give_object_permission_to_ee(org_ee, ee_rd, org_member, check_user_capabilities): + access = ExecutionEnvironmentAccess(org_member) + assert access.can_read(org_ee) # by virtue of being an org member + assert not access.can_change(org_ee, {'name': 'new'}) + check_user_capabilities(org_member, org_ee, {'edit': False, 'delete': False, 'copy': False}) + + ee_rd.give_permission(org_member, org_ee) + assert access.can_change(org_ee, {'name': 'new'}) + + check_user_capabilities(org_member, org_ee, {'edit': True, 'delete': True, 'copy': False}) + + +@pytest.mark.django_db +def test_give_org_permission_to_ee(org_ee, organization, org_member, check_user_capabilities): + access = ExecutionEnvironmentAccess(org_member) + assert not access.can_change(org_ee, {'name': 'new'}) + check_user_capabilities(org_member, org_ee, {'edit': False, 'delete': False, 'copy': False}) + + # NOTE: user_capabilities has problem with org_ee_rd.give_permission(org_member, organization) + organization.execution_environment_admin_role.members.add(org_member) + + assert access.can_change(org_ee, {'name': 'new'}) + check_user_capabilities(org_member, org_ee, {'edit': True, 'delete': True, 'copy': True}) From 249d75d4ba864ffa87d63b2f44d6cda2494430d3 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Wed, 26 Jun 2024 14:36:29 -0400 Subject: [PATCH 4/5] Tests passing locally --- awx/main/models/execution_environments.py | 8 ++--- .../test_rbac_execution_environment.py | 31 ++++++++++--------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/awx/main/models/execution_environments.py b/awx/main/models/execution_environments.py index 5d55b55caf77..137533d0f43d 100644 --- a/awx/main/models/execution_environments.py +++ b/awx/main/models/execution_environments.py @@ -60,13 +60,13 @@ def get_absolute_url(self, request=None): def validate_role_assignment(self, actor, role_definition): if self.managed: - raise ValidationError(_('Can not assign object roles to managed Execution Environments')) + raise ValidationError({'object_id': _('Can not assign object roles to managed Execution Environments')}) if self.organization_id is None: - raise ValidationError(_('Can not assign object roles to global Execution Environments')) + raise ValidationError({'object_id': _('Can not assign object roles to global Execution Environments')}) if actor._meta.model_name == 'user' and (not actor.has_obj_perm(self.organization, 'view')): - raise ValidationError(_('User must have view permission to Execution Environment organization')) + raise ValidationError({'user': _('User must have view permission to Execution Environment organization')}) if actor._meta.model_name == 'team': organization_cls = self._meta.get_field('organization').related_model if self.orgaanization not in organization_cls.access_qs(actor, 'view'): - raise ValidationError(_('Team must have view permission to Execution Environment organization')) + raise ValidationError({'team': _('Team must have view permission to Execution Environment organization')}) diff --git a/awx/main/tests/functional/test_rbac_execution_environment.py b/awx/main/tests/functional/test_rbac_execution_environment.py index dd2440f66f6a..164e61eb8c68 100644 --- a/awx/main/tests/functional/test_rbac_execution_environment.py +++ b/awx/main/tests/functional/test_rbac_execution_environment.py @@ -45,20 +45,6 @@ def org_ee(organization): return ExecutionEnvironment.objects.create(name='some user ee', organization=organization) -@pytest.mark.django_db -def test_managed_ee_not_assignable(control_plane_execution_environment, ee_rd, rando, admin_user, post): - url = django_reverse('roleuserassignment-list') - r = post(url, {'role_definition': ee_rd.pk, 'user': rando.id, 'object_id': control_plane_execution_environment.pk}, user=admin_user, expect=400) - assert 'foo' in str(r.data) - - -@pytest.mark.django_db -def test_org_member_required_for_assignment(org_ee, ee_rd, rando, admin_user, post): - url = django_reverse('roleuserassignment-list') - r = post(url, {'role_definition': ee_rd.pk, 'user': rando.id, 'object_id': org_ee.pk}, user=admin_user, expect=400) - assert 'foo' in str(r.data) - - @pytest.fixture def check_user_capabilities(get): def _rf(user, obj, expected): @@ -74,6 +60,23 @@ def _rf(user, obj, expected): return _rf +# ___ begin tests ___ + + +@pytest.mark.django_db +def test_managed_ee_not_assignable(control_plane_execution_environment, ee_rd, rando, admin_user, post): + url = django_reverse('roleuserassignment-list') + r = post(url, {'role_definition': ee_rd.pk, 'user': rando.id, 'object_id': control_plane_execution_environment.pk}, user=admin_user, expect=400) + assert 'Can not assign object roles to managed Execution Environment' in str(r.data) + + +@pytest.mark.django_db +def test_org_member_required_for_assignment(org_ee, ee_rd, rando, admin_user, post): + url = django_reverse('roleuserassignment-list') + r = post(url, {'role_definition': ee_rd.pk, 'user': rando.id, 'object_id': org_ee.pk}, user=admin_user, expect=400) + assert 'User must have view permission to Execution Environment organization' in str(r.data) + + @pytest.mark.django_db def test_give_object_permission_to_ee(org_ee, ee_rd, org_member, check_user_capabilities): access = ExecutionEnvironmentAccess(org_member) From c1e9349a3532c7f3a073556ff1a85c37fbd49a1e Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Wed, 26 Jun 2024 15:22:06 -0400 Subject: [PATCH 5/5] Issue with user_capabilities was a test bug, fixed --- .../functional/test_rbac_execution_environment.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/awx/main/tests/functional/test_rbac_execution_environment.py b/awx/main/tests/functional/test_rbac_execution_environment.py index 164e61eb8c68..9146bc9b7bfd 100644 --- a/awx/main/tests/functional/test_rbac_execution_environment.py +++ b/awx/main/tests/functional/test_rbac_execution_environment.py @@ -25,7 +25,7 @@ def ee_rd(): def org_ee_rd(): return RoleDefinition.objects.create_from_permissions( name='EE org admin', - permissions=['change_executionenvironment', 'delete_executionenvironment', 'view_organization'], + permissions=['add_executionenvironment', 'change_executionenvironment', 'delete_executionenvironment', 'view_organization'], content_type=ContentType.objects.get_for_model(Organization), ) @@ -46,7 +46,7 @@ def org_ee(organization): @pytest.fixture -def check_user_capabilities(get): +def check_user_capabilities(get, setup_managed_roles): def _rf(user, obj, expected): url = reverse('api:execution_environment_list') r = get(url, user=user, expect=200) @@ -91,13 +91,17 @@ def test_give_object_permission_to_ee(org_ee, ee_rd, org_member, check_user_capa @pytest.mark.django_db -def test_give_org_permission_to_ee(org_ee, organization, org_member, check_user_capabilities): +@pytest.mark.parametrize('style', ['new', 'old']) +def test_give_org_permission_to_ee(org_ee, organization, org_member, check_user_capabilities, style, org_ee_rd): access = ExecutionEnvironmentAccess(org_member) assert not access.can_change(org_ee, {'name': 'new'}) check_user_capabilities(org_member, org_ee, {'edit': False, 'delete': False, 'copy': False}) - # NOTE: user_capabilities has problem with org_ee_rd.give_permission(org_member, organization) - organization.execution_environment_admin_role.members.add(org_member) + if style == 'new': + org_ee_rd.give_permission(org_member, organization) + assert org_member.has_obj_perm(org_ee.organization, 'add_executionenvironment') # sanity + else: + organization.execution_environment_admin_role.members.add(org_member) assert access.can_change(org_ee, {'name': 'new'}) check_user_capabilities(org_member, org_ee, {'edit': True, 'delete': True, 'copy': True})