diff --git a/awx/api/views.py b/awx/api/views.py index f430025886..3845d9c5e5 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -438,35 +438,6 @@ class DashboardInventoryGraphView(APIView): return Response(dashboard_data) -def disallow_superuser_escalation(cls): - """Decorator that ensures that the post, put, and patch methods on the - class, if they exist, perform a sanity check and disallow superuser - escalation by non-superusers. - """ - # Create a method decorator that ensures superuser escalation by - # non-superusers is disallowed. - def superuser_lockdown(method): - @functools.wraps(method) - def fx(self, request, *a, **kw): - if not request.user.is_superuser: - is_su = request.DATA.get('is_superuser', False) - if is_su and is_su not in ('false', 'f', 'False', '0'): - raise PermissionDenied('Only superusers may create ' - 'other superusers.') - return method(self, request, *a, **kw) - return fx - - # Ensure that if post, put, or patch methods exist, that they are decorated - # with the sanity check decorator. - for vuln_method in ('post', 'put', 'patch'): - original_method = getattr(cls, vuln_method, None) - if original_method is not None: - setattr(cls, vuln_method, superuser_lockdown(original_method)) - - # Return the class object. - return cls - - class ScheduleList(ListAPIView): view_name = "Schedules" @@ -528,7 +499,6 @@ class OrganizationInventoriesList(SubListAPIView): parent_model = Organization relationship = 'inventories' -@disallow_superuser_escalation class OrganizationUsersList(SubListCreateAPIView): model = User @@ -536,7 +506,6 @@ class OrganizationUsersList(SubListCreateAPIView): parent_model = Organization relationship = 'users' -@disallow_superuser_escalation class OrganizationAdminsList(SubListCreateAPIView): model = User @@ -577,7 +546,6 @@ class TeamDetail(RetrieveUpdateDestroyAPIView): model = Team serializer_class = TeamSerializer -@disallow_superuser_escalation class TeamUsersList(SubListCreateAPIView): model = User @@ -764,13 +732,11 @@ class ProjectUpdateCancel(RetrieveAPIView): return self.http_method_not_allowed(request, *args, **kwargs) -@disallow_superuser_escalation class UserList(ListCreateAPIView): model = User serializer_class = UserSerializer -@disallow_superuser_escalation class UserMeList(ListAPIView): model = User @@ -845,7 +811,6 @@ class UserActivityStreamList(SubListAPIView): return qs.filter(Q(actor=parent) | Q(user__in=[parent])) -@disallow_superuser_escalation class UserDetail(RetrieveUpdateDestroyAPIView): model = User diff --git a/awx/main/access.py b/awx/main/access.py index a0ef5b0796..f65bf4cdba 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -183,10 +183,16 @@ class UserAccess(BaseAccess): ).distinct() def can_add(self, data): + if data is not None and 'is_superuser' in data: + if bool(data['is_superuser']) and not self.user.is_superuser: + return False return bool(self.user.is_superuser or self.user.admin_of_organizations.filter(active=True).exists()) def can_change(self, obj, data): + if data is not None and 'is_superuser' in data: + if bool(data['is_superuser']) and not self.user.is_superuser: + return False # A user can be changed if they are themselves, or by org admins or # superusers. Change permission implies changing only certain fields # that a user should be able to edit for themselves. diff --git a/awx/main/tests/projects.py b/awx/main/tests/projects.py index f0fc3befa8..9fd3a56792 100644 --- a/awx/main/tests/projects.py +++ b/awx/main/tests/projects.py @@ -421,10 +421,14 @@ class ProjectsTest(BaseTransactionTest): self.post(team_users, data=x, expect=403, auth=self.get_nobody_credentials()) self.post(team_users, data=dict(x, is_superuser=False), expect=204, auth=self.get_normal_credentials()) - self.post(team_users, data=dict(x, is_superuser=True), - expect=403, auth=self.get_normal_credentials()) + # The normal admin user can't create a super user vicariously through the team/project + self.post(team_users, data=dict(username='attempted_superuser_create', is_superuser=True), + expect=403, auth=self.get_normal_credentials()) + # ... but a superuser can + self.post(team_users, data=dict(username='attempted_superuser_create', is_superuser=True), + expect=201, auth=self.get_super_credentials()) - self.assertEqual(Team.objects.get(pk=team.pk).users.count(), 4) + self.assertEqual(Team.objects.get(pk=team.pk).users.count(), 5) # can remove users from teams for x in all_users['results']: @@ -432,7 +436,7 @@ class ProjectsTest(BaseTransactionTest): self.post(team_users, data=y, expect=403, auth=self.get_nobody_credentials()) self.post(team_users, data=y, expect=204, auth=self.get_normal_credentials()) - self.assertEquals(Team.objects.get(pk=team.pk).users.count(), 0) + self.assertEquals(Team.objects.get(pk=team.pk).users.count(), 1) # Leaving just the super user we created # ===================================================================== # USER TEAMS diff --git a/awx/main/tests/users.py b/awx/main/tests/users.py index 03d9761db3..767a7f73f4 100644 --- a/awx/main/tests/users.py +++ b/awx/main/tests/users.py @@ -51,6 +51,15 @@ class UsersTest(BaseTest): new_user3 = dict(username='blippy3') self.post(url, expect=403, data=new_user3, auth=self.get_normal_credentials()) + def test_only_super_user_can_use_superuser_flag(self): + url = reverse('api:user_list') + new_super_user = dict(username='nommy', is_superuser=True) + patch_new_super_user = dict(is_superuser=True) + self.post(url, expect=401, data=new_super_user, auth=self.get_invalid_credentials()) + self.post(url, expect=403, data=new_super_user, auth=self.get_other_credentials()) + self.post(url, expect=403, data=new_super_user, auth=self.get_normal_credentials()) + self.post(url, expect=201, data=new_super_user, auth=self.get_super_credentials()) + def test_auth_token_login(self): auth_token_url = reverse('api:auth_token_view') user_me_url = reverse('api:user_me_list')