From 4ae11f655735a70395034f6f0f23734c6885fdfb Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Thu, 13 Oct 2016 11:45:34 -0400 Subject: [PATCH 1/5] Patch up missing org access checks in access.py --- awx/main/access.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/awx/main/access.py b/awx/main/access.py index 421e0e73ab..b37cd223a6 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -871,6 +871,11 @@ class ProjectAccess(BaseAccess): @check_superuser def can_change(self, obj, data): + org_pk = get_pk_from_dict(data, 'organization') + if obj and org_pk and obj.organization.pk != org_pk: + org = get_object_or_400(Organization, pk=org_pk) + if self.user not in org.admin_role: + return False return self.user in obj.admin_role def can_delete(self, obj): @@ -2045,11 +2050,16 @@ class CustomInventoryScriptAccess(BaseAccess): @check_superuser def can_admin(self, obj, data=None): + org_pk = get_pk_from_dict(data, 'organization') + if obj and org_pk and obj.organization.pk != org_pk: + org = get_object_or_400(Organization, pk=org_pk) + if self.user not in org.admin_role: + return False return self.user in obj.admin_role @check_superuser def can_change(self, obj, data): - return self.can_admin(obj) + return self.can_admin(obj, data=data) @check_superuser def can_delete(self, obj): From 1d3c890e79fa5f23834e7dc4ae068bcc25a21135 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Thu, 13 Oct 2016 14:59:57 -0400 Subject: [PATCH 2/5] null value check added. --- awx/main/access.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/awx/main/access.py b/awx/main/access.py index b37cd223a6..c09b30e9a7 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -871,6 +871,8 @@ class ProjectAccess(BaseAccess): @check_superuser def can_change(self, obj, data): + if obj.organization is None: + return False org_pk = get_pk_from_dict(data, 'organization') if obj and org_pk and obj.organization.pk != org_pk: org = get_object_or_400(Organization, pk=org_pk) From a0ea2337c6324566d1ef2a6e82cc6adbcce87fc4 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Thu, 13 Oct 2016 15:35:03 -0400 Subject: [PATCH 3/5] Unit tests added. --- awx/main/tests/functional/test_rbac_inventory.py | 9 +++++++-- awx/main/tests/functional/test_rbac_project.py | 7 +++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/awx/main/tests/functional/test_rbac_inventory.py b/awx/main/tests/functional/test_rbac_inventory.py index 287919ad31..5b28645922 100644 --- a/awx/main/tests/functional/test_rbac_inventory.py +++ b/awx/main/tests/functional/test_rbac_inventory.py @@ -9,12 +9,13 @@ from awx.main.models import ( from awx.main.access import ( InventoryAccess, HostAccess, - InventoryUpdateAccess + InventoryUpdateAccess, + CustomInventoryScriptAccess ) from django.apps import apps @pytest.mark.django_db -def test_custom_inv_script_access(organization, user): +def test_custom_inv_script_access(organization, user, organization_factory): u = user('user', False) ou = user('oadm', False) @@ -29,6 +30,10 @@ def test_custom_inv_script_access(organization, user): organization.admin_role.members.add(ou) assert ou in custom_inv.admin_role + other_org = organization_factory('not-my-org').organization + access = CustomInventoryScriptAccess(ou) + assert not access.can_change(custom_inv, {'organization': other_org.pk, 'name': 'new-project'}) + @pytest.mark.django_db def test_inventory_admin_user(inventory, permissions, user): u = user('admin', False) diff --git a/awx/main/tests/functional/test_rbac_project.py b/awx/main/tests/functional/test_rbac_project.py index ba88226b2e..2b342df198 100644 --- a/awx/main/tests/functional/test_rbac_project.py +++ b/awx/main/tests/functional/test_rbac_project.py @@ -217,3 +217,10 @@ def test_create_project_foreign_org_admin(org_admin, organization, organization_ other_org = organization_factory('not-my-org').organization access = ProjectAccess(org_admin) assert not access.can_add({'organization': other_org.pk, 'name': 'new-project'}) + +@pytest.mark.django_db +def test_modify_project_foreign_org_admin(org_admin, organization, organization_factory, project): + """Org admins can only modify projects in their own org.""" + other_org = organization_factory('not-my-org').organization + access = ProjectAccess(org_admin) + assert not access.can_change(project, {'organization': other_org.pk, 'name': 'new-project'}) From f301cb0f9bc4f2b2c9e98234b142d4f083758af1 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Thu, 13 Oct 2016 17:06:37 -0400 Subject: [PATCH 4/5] Split unit tests & add access obj org sanity check. --- awx/main/access.py | 2 +- awx/main/tests/functional/test_rbac_inventory.py | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index c09b30e9a7..cfc881e4af 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -2053,7 +2053,7 @@ class CustomInventoryScriptAccess(BaseAccess): @check_superuser def can_admin(self, obj, data=None): org_pk = get_pk_from_dict(data, 'organization') - if obj and org_pk and obj.organization.pk != org_pk: + if obj and org_pk and obj.organization and obj.organization.pk != org_pk: org = get_object_or_400(Organization, pk=org_pk) if self.user not in org.admin_role: return False diff --git a/awx/main/tests/functional/test_rbac_inventory.py b/awx/main/tests/functional/test_rbac_inventory.py index 5b28645922..4198d71565 100644 --- a/awx/main/tests/functional/test_rbac_inventory.py +++ b/awx/main/tests/functional/test_rbac_inventory.py @@ -15,7 +15,7 @@ from awx.main.access import ( from django.apps import apps @pytest.mark.django_db -def test_custom_inv_script_access(organization, user, organization_factory): +def test_custom_inv_script_access(organization, user): u = user('user', False) ou = user('oadm', False) @@ -30,8 +30,14 @@ def test_custom_inv_script_access(organization, user, organization_factory): organization.admin_role.members.add(ou) assert ou in custom_inv.admin_role +@pytest.mark.django_db +def test_modify_inv_script_foreign_org_admin(org_admin, organization, organization_factory, project): + custom_inv = CustomInventoryScript.objects.create(name='test', script='test', description='test') + custom_inv.organization = organization + custom_inv.save() + other_org = organization_factory('not-my-org').organization - access = CustomInventoryScriptAccess(ou) + access = CustomInventoryScriptAccess(org_admin) assert not access.can_change(custom_inv, {'organization': other_org.pk, 'name': 'new-project'}) @pytest.mark.django_db From 3999ffd52a3111b4da4f9c4908b9e6223be7a3b6 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Fri, 14 Oct 2016 11:36:20 -0400 Subject: [PATCH 5/5] Unit test & project access refactoring to prevent potential issue. --- awx/main/access.py | 4 +--- awx/main/tests/functional/test_rbac_inventory.py | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index cfc881e4af..c70e6331c9 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -871,10 +871,8 @@ class ProjectAccess(BaseAccess): @check_superuser def can_change(self, obj, data): - if obj.organization is None: - return False org_pk = get_pk_from_dict(data, 'organization') - if obj and org_pk and obj.organization.pk != org_pk: + if obj and org_pk and obj.organization and obj.organization.pk != org_pk: org = get_object_or_400(Organization, pk=org_pk) if self.user not in org.admin_role: return False diff --git a/awx/main/tests/functional/test_rbac_inventory.py b/awx/main/tests/functional/test_rbac_inventory.py index 4198d71565..68e183c68b 100644 --- a/awx/main/tests/functional/test_rbac_inventory.py +++ b/awx/main/tests/functional/test_rbac_inventory.py @@ -32,9 +32,8 @@ def test_custom_inv_script_access(organization, user): @pytest.mark.django_db def test_modify_inv_script_foreign_org_admin(org_admin, organization, organization_factory, project): - custom_inv = CustomInventoryScript.objects.create(name='test', script='test', description='test') - custom_inv.organization = organization - custom_inv.save() + custom_inv = CustomInventoryScript.objects.create(name='test', script='test', description='test', + organization=organization) other_org = organization_factory('not-my-org').organization access = CustomInventoryScriptAccess(org_admin)