From 1195385492ccbf0a8ad596619852d2b83badff71 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 5 Apr 2018 14:46:52 -0400 Subject: [PATCH] User editing permission changes Only allow administrative action for a user who is a system admin or auditor if the the requesting-user is a system admin. Previously a user could be edited if the requesting-user was an admin of ANY of the orgs the user was member of. This is changed to require admin permission to ALL orgs the user is member of. As a special-case, allow org admins to add a user as a member to their organization if the following conditions are met: - the user is not member of any other orgs - the org admin has permissions to all of the roles the user has --- awx/main/access.py | 33 ++++++- awx/main/tests/functional/test_rbac_role.py | 96 ++++++++++++++++++++- 2 files changed, 121 insertions(+), 8 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index ddecce39e2..73281630bf 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -491,10 +491,35 @@ class UserAccess(BaseAccess): # that a user should be able to edit for themselves. return bool(self.user == obj or self.can_admin(obj, data)) + def user_membership_roles(self, u): + return Role.objects.filter( + content_type=ContentType.objects.get_for_model(Organization), + role_field__in=['admin_role', 'member_role'], + members=u + ) + + def is_all_org_admin(self, u): + return not self.user_membership_roles(u).exclude( + ancestors__in=self.user.roles.filter(role_field='admin_role') + ).exists() + + def user_is_orphaned(self, u): + return not self.user_membership_roles(u).exists() + @check_superuser - def can_admin(self, obj, data): - return Organization.objects.filter(Q(member_role__members=obj) | Q(admin_role__members=obj), - Q(admin_role__members=self.user)).exists() + def can_admin(self, obj, data, allow_orphans=False): + if obj.is_superuser or obj.is_system_auditor: + # must be superuser to admin users with system roles + return False + if self.user_is_orphaned(obj): + if not allow_orphans: + # in these cases only superusers can modify orphan users + return False + return not obj.roles.all().exclude( + content_type=ContentType.objects.get_for_model(User) + ).filter(ancestors__in=self.user.roles.all()).exists() + else: + return self.is_all_org_admin(obj) def can_delete(self, obj): if obj == self.user: @@ -2401,7 +2426,7 @@ class RoleAccess(BaseAccess): # unwanted escalations lets ensure that the Organization administartor has the abilty # to admin the user being added to the role. if isinstance(obj.content_object, Organization) and obj.role_field in ['member_role', 'admin_role']: - if not UserAccess(self.user).can_admin(sub_obj, None): + if not UserAccess(self.user).can_admin(sub_obj, None, allow_orphans=True): return False if isinstance(obj.content_object, ResourceMixin) and \ diff --git a/awx/main/tests/functional/test_rbac_role.py b/awx/main/tests/functional/test_rbac_role.py index 62b8469abf..7ec80e8d94 100644 --- a/awx/main/tests/functional/test_rbac_role.py +++ b/awx/main/tests/functional/test_rbac_role.py @@ -4,6 +4,7 @@ from awx.main.access import ( RoleAccess, UserAccess, TeamAccess) +from awx.main.models import Role, Organization @pytest.mark.django_db @@ -35,12 +36,99 @@ def test_role_access_attach(rando, inventory): @pytest.mark.django_db -def test_org_user_role_attach(user, organization): +def test_visible_roles(admin_user, system_auditor, rando, organization, project): + ''' + system admin & system auditor fixtures needed to create system roles + ''' + organization.auditor_role.members.add(rando) + access = RoleAccess(rando) + + assert rando not in organization.admin_role + assert access.can_read(organization.admin_role) + assert organization.admin_role in Role.visible_roles(rando) + + assert rando not in project.admin_role + assert access.can_read(project.admin_role) + assert project.admin_role in Role.visible_roles(rando) + + +# Permissions when adding users to org member/admin +@pytest.mark.django_db +def test_org_user_role_attach(user, organization, inventory): + ''' + Org admins must not be able to add arbitrary users to their + organization, because that would give them admin permission to that user + ''' admin = user('admin') nonmember = user('nonmember') + inventory.admin_role.members.add(nonmember) organization.admin_role.members.add(admin) - access = RoleAccess(admin) - assert not access.can_attach(organization.member_role, nonmember, 'members', None) - assert not access.can_attach(organization.admin_role, nonmember, 'members', None) + role_access = RoleAccess(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) + + +# Singleton user editing restrictions +@pytest.mark.django_db +def test_org_superuser_role_attach(admin_user, org_admin, organization): + ''' + Ideally, you would not add superusers to roles (particularly member_role) + but it has historically been possible + this checks that the situation does not grant unexpected permissions + ''' + organization.member_role.members.add(admin_user) + + role_access = RoleAccess(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) + user_access = UserAccess(org_admin) + assert not user_access.can_change(admin_user, {'last_name': 'Witzel'}) + + +# Org admin user editing permission ANY to ALL change +@pytest.mark.django_db +def test_need_all_orgs_to_admin_user(user): + ''' + Old behavior - org admin to ANY organization that a user is member of + grants permission to admin that user + New behavior enforced here - org admin to ALL organizations that a + user is member of grants permission to admin that user + ''' + org1 = Organization.objects.create(name='org1') + org2 = Organization.objects.create(name='org2') + + org1_admin = user('org1-admin') + org1.admin_role.members.add(org1_admin) + + org12_member = user('org12-member') + org1.member_role.members.add(org12_member) + org2.member_role.members.add(org12_member) + + user_access = UserAccess(org1_admin) + assert not user_access.can_change(org12_member, {'last_name': 'Witzel'}) + + role_access = RoleAccess(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) + + 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) + + +# Orphaned user can be added to member role, only in special cases +@pytest.mark.django_db +def test_orphaned_user_allowed(org_admin, rando, organization): + ''' + We still allow adoption of orphaned* users by assigning them to + organization member role, but only in the situation where the + org admin already posesses indirect access to all of the user's roles + *orphaned means user is not a member of any organization + ''' + role_access = RoleAccess(org_admin) + assert role_access.can_attach(organization.member_role, rando, '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'})