Merge pull request #25 from matburt/remove_crazy_decorators

Purge old super user check decorator in favor of using RBAC.  Update unit tests

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

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.

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

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')