From a344ceda0ee6a475c13924e2a72143e60d54e328 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 5 Apr 2018 14:46:52 -0400 Subject: [PATCH 1/2] 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 | 93 +++++++++++++++++++-- 2 files changed, 117 insertions(+), 9 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index 172d34df08..d543c00509 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -538,12 +538,37 @@ class UserAccess(BaseAccess): return False 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): + def can_admin(self, obj, data, allow_orphans=False): if not settings.MANAGE_ORGANIZATION_AUTH: return False - return Organization.objects.filter(Q(member_role__members=obj) | Q(admin_role__members=obj), - Q(admin_role__members=self.user)).exists() + 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: @@ -2535,7 +2560,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 438a72182b..abaa8a4410 100644 --- a/awx/main/tests/functional/test_rbac_role.py +++ b/awx/main/tests/functional/test_rbac_role.py @@ -4,7 +4,7 @@ from awx.main.access import ( RoleAccess, UserAccess, TeamAccess) -from awx.main.models import Role +from awx.main.models import Role, Organization @pytest.mark.django_db @@ -52,13 +52,96 @@ def test_visible_roles(admin_user, system_auditor, rando, organization, project) 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): +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'}) + + +# Sanity check user editing permissions combined with new org roles +@pytest.mark.django_db +def test_org_object_role_not_sufficient(user, organization): + member = user('amember') + obj_admin = user('icontrolallworkflows') + + organization.member_role.members.add(member) + organization.workflow_admin_role.members.add(obj_admin) + + user_access = UserAccess(obj_admin) + assert not user_access.can_change(member, {'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'}) From 12979260bb1cd78da68694930b5259af399e4b7b Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Fri, 6 Apr 2018 12:03:43 -0400 Subject: [PATCH 2/2] include new org roles in permissions fix --- awx/main/access.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/awx/main/access.py b/awx/main/access.py index d543c00509..aeb82ce36e 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -541,7 +541,12 @@ class UserAccess(BaseAccess): 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'], + role_field__in=[ + 'admin_role', 'member_role', + 'execute_role', 'project_admin_role', 'inventory_admin_role', + 'credential_admin_role', 'workflow_admin_role', + 'notification_admin_role' + ], members=u )