From 70b0679a0cd7468dd4ebb7b403ea33185943a940 Mon Sep 17 00:00:00 2001 From: Jeff Bradberry Date: Wed, 17 Apr 2019 15:37:02 -0400 Subject: [PATCH 1/4] Adjust the access logic for settings.MANAGE_ORGANIZATION_AUTH = False so that changing the membership of Organizations and Teams are disallowed unless you are a superuser, but granting resource privileges is still permitted. --- awx/main/access.py | 30 +++++++++++---------- awx/main/tests/functional/test_rbac_user.py | 16 ++++++++--- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index 250ce99a66..511f4fdca7 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -645,22 +645,24 @@ class UserAccess(BaseAccess): return False def can_attach(self, obj, sub_obj, relationship, *args, **kwargs): - if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: - return False - # Reverse obj and sub_obj, defer to RoleAccess if this is a role assignment. if relationship == 'roles': role_access = RoleAccess(self.user) return role_access.can_attach(sub_obj, obj, 'members', *args, **kwargs) - return super(UserAccess, self).can_attach(obj, sub_obj, relationship, *args, **kwargs) - def can_unattach(self, obj, sub_obj, relationship, *args, **kwargs): if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: return False + return super(UserAccess, self).can_attach(obj, sub_obj, relationship, *args, **kwargs) + + def can_unattach(self, obj, sub_obj, relationship, *args, **kwargs): if relationship == 'roles': role_access = RoleAccess(self.user) return role_access.can_unattach(sub_obj, obj, 'members', *args, **kwargs) + + if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: + return False + return super(UserAccess, self).can_unattach(obj, sub_obj, relationship, *args, **kwargs) @@ -2700,10 +2702,6 @@ class RoleAccess(BaseAccess): @check_superuser def can_unattach(self, obj, sub_obj, relationship, data=None, skip_sub_obj_read_check=False): - if isinstance(obj.content_object, Team): - if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: - return False - if not skip_sub_obj_read_check and relationship in ['members', 'member_role.parents', 'parents']: # If we are unattaching a team Role, check the Team read access if relationship == 'parents': @@ -2715,18 +2713,22 @@ class RoleAccess(BaseAccess): # Being a user in the member_role or admin_role of an organization grants # administrators of that Organization the ability to edit that user. To prevent - # unwanted escalations lets ensure that the Organization administartor has the abilty + # unwanted escalations let's ensure that the Organization administrator has the ability # to admin the user being added to the role. - if (isinstance(obj.content_object, Organization) and - obj.role_field in (Organization.member_role.field.parent_role + ['member_role'])): + if isinstance(obj.content_object, Organization) and obj.role_field in ['admin_role', 'member_role']: if not isinstance(sub_obj, User): logger.error('Unexpected attempt to associate {} with organization role.'.format(sub_obj)) return False + if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: + return False if not UserAccess(self.user).can_admin(sub_obj, None, allow_orphans=True): return False - if isinstance(obj.content_object, ResourceMixin) and \ - self.user in obj.content_object.admin_role: + if isinstance(obj.content_object, Team) and obj.role_field in ['admin_role', 'member_role']: + if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: + return False + + if isinstance(obj.content_object, ResourceMixin) and self.user in obj.content_object.admin_role: return True return False diff --git a/awx/main/tests/functional/test_rbac_user.py b/awx/main/tests/functional/test_rbac_user.py index 96cb5524d0..403767749f 100644 --- a/awx/main/tests/functional/test_rbac_user.py +++ b/awx/main/tests/functional/test_rbac_user.py @@ -77,19 +77,27 @@ def test_manage_org_auth_setting(ext_auth, superuser, expect, organization, rand assert [ # use via /api/v2/users/N/roles/ UserAccess(u).can_attach(rando, organization.admin_role, 'roles'), + UserAccess(u).can_attach(rando, organization.member_role, 'roles'), UserAccess(u).can_attach(rando, team.admin_role, 'roles'), + UserAccess(u).can_attach(rando, team.member_role, 'roles'), # use via /api/v2/roles/N/users/ RoleAccess(u).can_attach(organization.admin_role, rando, 'members'), - RoleAccess(u).can_attach(team.admin_role, rando, 'members') - ] == [expect for i in range(4)] + RoleAccess(u).can_attach(organization.member_role, rando, 'members'), + RoleAccess(u).can_attach(team.admin_role, rando, 'members'), + RoleAccess(u).can_attach(team.member_role, rando, 'members'), + ] == [expect for i in range(8)] assert [ # use via /api/v2/users/N/roles/ UserAccess(u).can_unattach(rando, organization.admin_role, 'roles'), + UserAccess(u).can_unattach(rando, organization.member_role, 'roles'), UserAccess(u).can_unattach(rando, team.admin_role, 'roles'), + UserAccess(u).can_unattach(rando, team.member_role, 'roles'), # use via /api/v2/roles/N/users/ RoleAccess(u).can_unattach(organization.admin_role, rando, 'members'), - RoleAccess(u).can_unattach(team.admin_role, rando, 'members') - ] == [expect for i in range(4)] + RoleAccess(u).can_unattach(organization.member_role, rando, 'members'), + RoleAccess(u).can_unattach(team.admin_role, rando, 'members'), + RoleAccess(u).can_unattach(team.member_role, rando, 'members'), + ] == [expect for i in range(8)] @pytest.mark.django_db From 8ad0b3f78750786f075ee20024e5fa2e6d8b8260 Mon Sep 17 00:00:00 2001 From: Jeff Bradberry Date: Wed, 17 Apr 2019 17:45:20 -0400 Subject: [PATCH 2/4] Check the permissions for adding users to orgs/teams in the other direction --- awx/main/access.py | 37 ++++++++++++++----- .../functional/test_rbac_organization.py | 12 ++++++ awx/main/tests/functional/test_rbac_team.py | 16 +++++++- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index 511f4fdca7..6d615e8677 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -645,25 +645,22 @@ class UserAccess(BaseAccess): return False def can_attach(self, obj, sub_obj, relationship, *args, **kwargs): - # Reverse obj and sub_obj, defer to RoleAccess if this is a role assignment. + # The only thing that a User should ever have attached is a Role if relationship == 'roles': role_access = RoleAccess(self.user) return role_access.can_attach(sub_obj, obj, 'members', *args, **kwargs) - if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: - return False - - return super(UserAccess, self).can_attach(obj, sub_obj, relationship, *args, **kwargs) + logger.error('Unexpected attempt to associate {} with a user.'.format(sub_obj)) + return False def can_unattach(self, obj, sub_obj, relationship, *args, **kwargs): + # The only thing that a User should ever have to be unattached is a Role if relationship == 'roles': role_access = RoleAccess(self.user) return role_access.can_unattach(sub_obj, obj, 'members', *args, **kwargs) - if not settings.MANAGE_ORGANIZATION_AUTH and not self.user.is_superuser: - return False - - return super(UserAccess, self).can_unattach(obj, sub_obj, relationship, *args, **kwargs) + logger.error('Unexpected attempt to de-associate {} from a user.'.format(sub_obj)) + return False class OAuth2ApplicationAccess(BaseAccess): @@ -774,6 +771,11 @@ class OrganizationAccess(NotificationAttachMixin, BaseAccess): return True def can_attach(self, obj, sub_obj, relationship, *args, **kwargs): + # If the request is updating the membership, check the membership role permissions instead + if relationship in ('member_role.members', 'admin_role.members'): + rel_role = getattr(obj, relationship.split('.')[0]) + return RoleAccess(self.user).can_attach(rel_role, sub_obj, 'members', *args, **kwargs) + if relationship == "instance_groups": if self.user.is_superuser: return True @@ -781,6 +783,11 @@ class OrganizationAccess(NotificationAttachMixin, BaseAccess): return super(OrganizationAccess, self).can_attach(obj, sub_obj, relationship, *args, **kwargs) def can_unattach(self, obj, sub_obj, relationship, *args, **kwargs): + # If the request is updating the membership, check the membership role permissions instead + if relationship in ('member_role.members', 'admin_role.members'): + rel_role = getattr(obj, relationship.split('.')[0]) + return RoleAccess(self.user).can_unattach(rel_role, sub_obj, 'members', *args, **kwargs) + if relationship == "instance_groups": return self.can_attach(obj, sub_obj, relationship, *args, **kwargs) return super(OrganizationAccess, self).can_attach(obj, sub_obj, relationship, *args, **kwargs) @@ -1301,6 +1308,12 @@ class TeamAccess(BaseAccess): *args, **kwargs) if self.user.is_superuser: return True + + # If the request is updating the membership, check the membership role permissions instead + if relationship in ('member_role.members', 'admin_role.members'): + rel_role = getattr(obj, relationship.split('.')[0]) + return RoleAccess(self.user).can_attach(rel_role, sub_obj, 'members', *args, **kwargs) + return super(TeamAccess, self).can_attach(obj, sub_obj, relationship, *args, **kwargs) @@ -1311,6 +1324,12 @@ class TeamAccess(BaseAccess): role_access = RoleAccess(self.user) return role_access.can_unattach(sub_obj, obj, 'member_role.parents', *args, **kwargs) + + # If the request is updating the membership, check the membership role permissions instead + if relationship in ('member_role.members', 'admin_role.members'): + rel_role = getattr(obj, relationship.split('.')[0]) + return RoleAccess(self.user).can_unattach(rel_role, sub_obj, 'members', *args, **kwargs) + return super(TeamAccess, self).can_unattach(obj, sub_obj, relationship, *args, **kwargs) diff --git a/awx/main/tests/functional/test_rbac_organization.py b/awx/main/tests/functional/test_rbac_organization.py index c3c78cf09e..ddb0692ea3 100644 --- a/awx/main/tests/functional/test_rbac_organization.py +++ b/awx/main/tests/functional/test_rbac_organization.py @@ -36,3 +36,15 @@ def test_organization_access_user(cl, organization, user): org = access.get_queryset()[0] assert len(org.admin_role.members.all()) == 0 assert len(org.member_role.members.all()) == 1 + + +@pytest.mark.django_db +@pytest.mark.parametrize('ext_auth', [True, False]) +def test_org_resource_role(ext_auth, organization, rando, org_admin): + with mock.patch('awx.main.access.settings') as settings_mock: + settings_mock.MANAGE_ORGANIZATION_AUTH = ext_auth + access = OrganizationAccess(org_admin) + + assert access.can_attach(organization, rando, 'member_role.members') == ext_auth + organization.member_role.members.add(rando) + assert access.can_unattach(organization, rando, 'member_role.members') == ext_auth diff --git a/awx/main/tests/functional/test_rbac_team.py b/awx/main/tests/functional/test_rbac_team.py index 80a825edd5..a18a69a94b 100644 --- a/awx/main/tests/functional/test_rbac_team.py +++ b/awx/main/tests/functional/test_rbac_team.py @@ -21,7 +21,21 @@ def test_team_attach_unattach(team, user): u2 = user('non-member', False) access = TeamAccess(u2) assert not access.can_attach(team, team.member_role, 'member_role.children', None) - assert not access.can_unattach(team, team.member_role, 'member_role.chidlren') + assert not access.can_unattach(team, team.member_role, 'member_role.children') + + +@pytest.mark.django_db +@pytest.mark.parametrize('ext_auth', [True, False]) +def test_team_org_resource_role(ext_auth, team, user, rando): + with mock.patch('awx.main.access.settings') as settings_mock: + settings_mock.MANAGE_ORGANIZATION_AUTH = ext_auth + u = user('member', False) + team.organization.admin_role.members.add(u) + access = TeamAccess(u) + + assert access.can_attach(team, rando, 'member_role.members') == ext_auth + team.member_role.members.add(rando) + assert access.can_unattach(team, rando, 'member_role.members') == ext_auth @pytest.mark.django_db From 41b476544da6a2e54c017963dfbe9f8cde54ca9f Mon Sep 17 00:00:00 2001 From: Jeff Bradberry Date: Thu, 18 Apr 2019 13:35:35 -0400 Subject: [PATCH 3/4] Improve test coverage of attaching a user to an organization --- awx/main/tests/functional/test_rbac_role.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/awx/main/tests/functional/test_rbac_role.py b/awx/main/tests/functional/test_rbac_role.py index 29598b6753..09268b2ded 100644 --- a/awx/main/tests/functional/test_rbac_role.py +++ b/awx/main/tests/functional/test_rbac_role.py @@ -3,7 +3,9 @@ import pytest from awx.main.access import ( RoleAccess, UserAccess, - TeamAccess) + OrganizationAccess, + TeamAccess, +) from awx.main.models import Role, Organization @@ -160,12 +162,17 @@ def test_need_all_orgs_to_admin_user(user): assert not user_access.can_change(org12_member, {'last_name': 'Witzel'}) role_access = RoleAccess(org1_admin) + org_access = OrganizationAccess(org1_admin) assert not role_access.can_attach(org1.admin_role, org12_member, 'members', None) assert not role_access.can_attach(org1.member_role, org12_member, 'members', None) + assert not org_access.can_attach(org1, org12_member, 'admin_role.members') + assert not org_access.can_attach(org1, org12_member, 'member_role.members') org2.admin_role.members.add(org1_admin) assert role_access.can_attach(org1.admin_role, org12_member, 'members', None) assert role_access.can_attach(org1.member_role, org12_member, 'members', None) + assert org_access.can_attach(org1, org12_member, 'admin_role.members') + assert org_access.can_attach(org1, org12_member, 'member_role.members') # Orphaned user can be added to member role, only in special cases From 0ba87c9729345bdada2918a23c42508814c3771c Mon Sep 17 00:00:00 2001 From: Jeff Bradberry Date: Thu, 18 Apr 2019 14:53:19 -0400 Subject: [PATCH 4/4] Add more test checks for the alternate code path to the role checks --- awx/main/tests/functional/test_rbac_role.py | 33 ++++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/awx/main/tests/functional/test_rbac_role.py b/awx/main/tests/functional/test_rbac_role.py index 09268b2ded..838a410d58 100644 --- a/awx/main/tests/functional/test_rbac_role.py +++ b/awx/main/tests/functional/test_rbac_role.py @@ -17,24 +17,21 @@ def test_team_access_attach(rando, team, inventory): # team has read_role for the inventory team.member_role.children.add(inventory.read_role) - access = TeamAccess(rando) + team_access = TeamAccess(rando) + role_access = RoleAccess(rando) data = {'id': inventory.admin_role.pk} - assert not access.can_attach(team, inventory.admin_role, 'member_role.children', data, False) + assert not team_access.can_attach(team, inventory.admin_role, 'member_role.children', data, False) + assert not role_access.can_attach(inventory.admin_role, team, 'member_role.parents', data, False) @pytest.mark.django_db def test_user_access_attach(rando, inventory): inventory.read_role.members.add(rando) - access = UserAccess(rando) + user_access = UserAccess(rando) + role_access = RoleAccess(rando) data = {'id': inventory.admin_role.pk} - assert not access.can_attach(rando, inventory.admin_role, 'roles', data, False) - - -@pytest.mark.django_db -def test_role_access_attach(rando, inventory): - inventory.read_role.members.add(rando) - access = RoleAccess(rando) - assert not access.can_attach(inventory.admin_role, rando, 'members', None) + assert not user_access.can_attach(rando, inventory.admin_role, 'roles', data, False) + assert not role_access.can_attach(inventory.admin_role, rando, 'members', data, False) @pytest.mark.django_db @@ -68,8 +65,11 @@ def test_org_user_role_attach(user, organization, inventory): organization.admin_role.members.add(admin) role_access = RoleAccess(admin) + org_access = OrganizationAccess(admin) assert not role_access.can_attach(organization.member_role, nonmember, 'members', None) assert not role_access.can_attach(organization.admin_role, nonmember, 'members', None) + assert not org_access.can_attach(organization, nonmember, 'member_role.members', None) + assert not org_access.can_attach(organization, nonmember, 'admin_role.members', None) # Permissions when adding users/teams to org special-purpose roles @@ -83,9 +83,15 @@ def test_user_org_object_roles(organization, org_admin, org_member): assert RoleAccess(org_admin).can_attach( organization.notification_admin_role, org_member, 'members', None ) + assert OrganizationAccess(org_admin).can_attach( + organization, org_member, 'notification_admin_role.members', None + ) assert not RoleAccess(org_member).can_attach( organization.notification_admin_role, org_member, 'members', None ) + assert not OrganizationAccess(org_member).can_attach( + organization, org_member, 'notification_admin_role.members', None + ) @pytest.mark.django_db @@ -120,8 +126,11 @@ def test_org_superuser_role_attach(admin_user, org_admin, organization): organization.member_role.members.add(admin_user) role_access = RoleAccess(org_admin) + org_access = OrganizationAccess(org_admin) assert not role_access.can_attach(organization.member_role, admin_user, 'members', None) assert not role_access.can_attach(organization.admin_role, admin_user, 'members', None) + assert not org_access.can_attach(organization, admin_user, 'member_role.members', None) + assert not org_access.can_attach(organization, admin_user, 'admin_role.members', None) user_access = UserAccess(org_admin) assert not user_access.can_change(admin_user, {'last_name': 'Witzel'}) @@ -185,7 +194,9 @@ def test_orphaned_user_allowed(org_admin, rando, organization): *orphaned means user is not a member of any organization ''' role_access = RoleAccess(org_admin) + org_access = OrganizationAccess(org_admin) assert role_access.can_attach(organization.member_role, rando, 'members', None) + assert org_access.can_attach(organization, rando, 'member_role.members', None) # Cannot edit the user directly without adding to org first user_access = UserAccess(org_admin) assert not user_access.can_change(rando, {'last_name': 'Witzel'})