Merge pull request #4256 from Joshua-Anderson/permissions-sec-fix

fix(controller): don't check permissions with @permissions_classes

Author: mboersma

controller/api/fixtures/test_sharing.json

@@ -53,6 +53,24 @@
     "date_joined": "2013-11-25T21:59:30.760Z"
   }
 },
+{
+  "pk": 4,
+  "model": "auth.user",
+  "fields": {
+    "username": "autotest-3",
+    "first_name": "",
+    "last_name": "",
+    "is_active": true,
+    "is_superuser": false,
+    "is_staff": false,
+    "last_login": "2013-11-25T21:59:31.404Z",
+    "groups": [],
+    "user_permissions": [],
+    "password": "pbkdf2_sha256$10000$FrfwTVAtWPMD$HUfDokMeY37YshdyS3uhDZ+d/r8galU7kNuBfZxJl2s=",
+    "email": "autotest@deis.io",
+    "date_joined": "2013-11-25T21:59:30.760Z"
+  }
+},
 {
   "pk": "5a09a1e0-a27e-4839-928b-449310ed90f0",
   "model": "api.app",

controller/api/tests/test_perm.py

@@ -147,6 +147,8 @@ def setUp(self):
         self.token = Token.objects.get(user=self.user).key
         self.user2 = User.objects.get(username='autotest-2')
         self.token2 = Token.objects.get(user=self.user2).key
+        self.user3 = User.objects.get(username='autotest-3')
+        self.token3 = Token.objects.get(user=self.user3).key
 
     def test_create(self):
         # check that user 1 sees her lone app and user 2's app
@@ -210,12 +212,8 @@ def test_delete(self):
         response = self.client.get('/v1/apps', HTTP_AUTHORIZATION='token {}'.format(self.token2))
         self.assertEqual(response.status_code, 200)
         self.assertEqual(len(response.data['results']), 2)
-        # try to delete the permission as user 2
-        url = "/v1/apps/{}/perms/{}".format(app_id, 'autotest-2')
-        response = self.client.delete(url, content_type='application/json',
-                                      HTTP_AUTHORIZATION='token {}'.format(self.token2))
-        self.assertEqual(response.status_code, 403)
         # delete permission to user 1's app
+        url = "/v1/apps/{}/perms/{}".format(app_id, 'autotest-2')
         response = self.client.delete(url, content_type='application/json',
                                       HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 204)
@@ -280,3 +278,42 @@ def test_unauthorized_user_cannot_modify_perms(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
         self.assertEqual(response.status_code, 403)
+
+    def test_collaborator_cannot_share(self):
+        """
+        An collaborator should not be able to modify the app's permissions.
+        """
+        app_id = "autotest-1-app"
+        owner_token = self.token
+        collab = self.user2
+        collab_token = self.token2
+        url = '/v1/apps/{}/perms'.format(app_id)
+        # Share app with collaborator
+        body = {'username': collab.username}
+        response = self.client.post(url, json.dumps(body), content_type='application/json',
+                                    HTTP_AUTHORIZATION='token {}'.format(owner_token))
+        self.assertEqual(response.status_code, 201)
+        # Collaborator should fail to share app
+        body = {'username': self.user3.username}
+        response = self.client.post(url, json.dumps(body), content_type='application/json',
+                                    HTTP_AUTHORIZATION='token {}'.format(collab_token))
+        self.assertEqual(response.status_code, 403)
+        # Collaborator can list
+        response = self.client.get(url, content_type='application/json',
+                                   HTTP_AUTHORIZATION='token {}'.format(collab_token))
+        self.assertEqual(response.status_code, 200)
+        # Share app with user 3 for rest of tests
+        response = self.client.post(url, json.dumps(body), content_type='application/json',
+                                    HTTP_AUTHORIZATION='token {}'.format(owner_token))
+        self.assertEqual(response.status_code, 201)
+        response = self.client.get(url, content_type='application/json',
+                                   HTTP_AUTHORIZATION='token {}'.format(collab_token))
+        self.assertEqual(response.status_code, 200)
+        # Collaborator cannot delete other collaborator
+        url += "/{}".format(self.user3.username)
+        response = self.client.delete(url, HTTP_AUTHORIZATION='token {}'.format(collab_token))
+        self.assertEqual(response.status_code, 403)
+        # Collaborator can delete themselves
+        url = '/v1/apps/{}/perms/{}'.format(app_id, collab.username)
+        response = self.client.delete(url, HTTP_AUTHORIZATION='token {}'.format(collab_token))
+        self.assertEqual(response.status_code, 204)

controller/api/views.py

@@ -8,7 +8,6 @@
 from guardian.shortcuts import assign_perm, get_objects_for_user, \
     get_users_with_perms, remove_perm
 from rest_framework import mixins, renderers, status
-from rest_framework.decorators import permission_classes
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.permissions import IsAuthenticated
 from rest_framework.response import Response
@@ -404,27 +403,35 @@ class AppPermsViewSet(BaseDeisViewSet):
     def get_queryset(self):
         return self.model.objects.all()
 
-    @permission_classes([permissions.IsAppUser])
     def list(self, request, **kwargs):
         app = self.get_object()
         perm_name = "api.{}".format(self.perm)
         usernames = [u.username for u in get_users_with_perms(app)
                      if u.has_perm(perm_name, app)]
         return Response({'users': usernames})
 
-    @permission_classes([permissions.IsOwnerOrAdmin])
     def create(self, request, **kwargs):
         app = self.get_object()
+        if not permissions.IsOwnerOrAdmin.has_object_permission(permissions.IsOwnerOrAdmin(),
+                                                                request, self, app):
+            raise PermissionDenied()
+
         user = get_object_or_404(User, username=request.data['username'])
         assign_perm(self.perm, user, app)
         models.log_event(app, "User {} was granted access to {}".format(user, app))
         return Response(status=status.HTTP_201_CREATED)
 
-    @permission_classes([permissions.IsOwnerOrAdmin])
     def destroy(self, request, **kwargs):
-        app = self.get_object()
+        app = get_object_or_404(models.App, id=self.kwargs['id'])
         user = get_object_or_404(User, username=kwargs['username'])
-        if not user.has_perm(self.perm, app):
+
+        perm_name = "api.{}".format(self.perm)
+        if not user.has_perm(perm_name, app):
+            raise PermissionDenied()
+
+        if (user != request.user and
+            not permissions.IsOwnerOrAdmin.has_object_permission(permissions.IsOwnerOrAdmin(),
+                                                                 request, self, app)):
             raise PermissionDenied()
         remove_perm(self.perm, user, app)
         models.log_event(app, "User {} was revoked access to {}".format(user, app))

tests/perms_test.go

@@ -66,8 +66,7 @@ func permsDeleteAdminTest(t *testing.T, params *utils.DeisTestConfig) {
 
 func permsDeleteAppTest(t *testing.T, params, user *utils.DeisTestConfig) {
 	utils.Execute(t, authLoginCmd, user, false, "")
-	utils.Execute(t, permsDeleteAppCmd, user, true, "403 FORBIDDEN")
+	utils.Execute(t, permsDeleteAppCmd, user, false, "")
 	utils.Execute(t, authLoginCmd, params, false, "")
-	utils.Execute(t, permsDeleteAppCmd, params, false, "")
 	utils.CheckList(t, permsListAppCmd, params, "test1", true)
 }