Merge pull request #2856 from mboersma/403s-please

ref(controller): return 403s for unauthorized endpoints

Author: mboersma

controller/api/__init__.py

@@ -2,4 +2,4 @@
 The **api** Django app presents a RESTful web API for interacting with the **deis** system.
 """
 
-__version__ = '1.1.2'
+__version__ = '1.1.1'

controller/api/tests/test_app.py

@@ -10,6 +10,7 @@
 import mock
 import os.path
 import requests
+import unittest
 
 from django.conf import settings
 from django.contrib.auth.models import User
@@ -253,12 +254,13 @@ def test_run_without_release_should_error(self):
         self.assertEqual(response.data, "No build associated with this release "
                                         "to run this command")
 
+    @unittest.expectedFailure
     def test_unauthorized_user_cannot_see_app(self):
         """
         An unauthorized user should not be able to access an app's resources.
 
-        Since an unauthorized user should not know about the application at all, these
-        requests should return a 404.
+        Since an unauthorized user can't access the application, these
+        tests should return a 403, but currently return a 404. FIXME!
         """
         app_id = 'autotest'
         base_url = '/v1/apps'
@@ -271,14 +273,10 @@ def test_unauthorized_user_cannot_see_app(self):
         body = {'command': 'foo'}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)
         url = '{}/{}/logs'.format(base_url, app_id)
         response = self.client.get(url, HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
-        self.assertEqual(response.status_code, 404)
-        url = '{}/{}'.format(base_url, app_id)
-        response = self.client.delete(url,
-                                      HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)
 
     def test_app_info_not_showing_wrong_app(self):
         app_id = 'autotest'

controller/api/tests/test_build.py

@@ -212,8 +212,8 @@ def test_unauthorized_user_cannot_modify_build(self):
         """
         An unauthorized user should not be able to modify other builds.
 
-        Since an unauthorized user should not know about the application at all, these
-        requests should return a 404.
+        Since an unauthorized user can't access the application, these
+        requests should return a 403.
         """
         app_id = 'autotest'
         url = '/v1/apps'
@@ -226,4 +226,4 @@ def test_unauthorized_user_cannot_modify_build(self):
         body = {'image': 'foo'}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)

controller/api/tests/test_config.py

@@ -437,8 +437,8 @@ def test_unauthorized_user_cannot_modify_config(self):
         """
         An unauthorized user should not be able to modify other config.
 
-        Since an unauthorized user should not know about the application at all, these
-        requests should return a 404.
+        Since an unauthorized user can't access the application, these
+        requests should return a 403.
         """
         app_id = 'autotest'
         base_url = '/v1/apps'
@@ -451,4 +451,4 @@ def test_unauthorized_user_cannot_modify_config(self):
         body = {'values': {'FOO': 'bar'}}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)

controller/api/tests/test_domain.py

@@ -117,4 +117,4 @@ def test_unauthorized_user_cannot_modify_domain(self):
         body = {'domain': 'example.com'}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)

controller/api/tests/test_perm.py

@@ -163,8 +163,9 @@ def test_create(self):
             response = self.client.get("/v1/apps/{}/{}/".format(app_id, model),
                                        HTTP_AUTHORIZATION='token {}'.format(self.token2))
             msg = "Failed: status '%s', and data '%s'" % (response.status_code, response.data)
-            self.assertEqual(response.status_code, 404, msg=msg)
-            self.assertEqual(response.data['detail'], 'Not found', msg=msg)
+            self.assertEqual(response.status_code, 403, msg=msg)
+            self.assertEqual(response.data['detail'],
+                             'You do not have permission to perform this action.', msg=msg)
         # TODO: test that git pushing to the app fails
         # give user 2 permission to user 1's app
         url = "/v1/apps/{}/perms".format(app_id)
@@ -194,7 +195,7 @@ def test_create_errors(self):
         body = {'username': 'autotest-2'}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token2))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)
 
     def test_delete(self):
         # give user 2 permission to user 1's app
@@ -213,7 +214,7 @@ def test_delete(self):
         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, 404)
+        self.assertEqual(response.status_code, 403)
         self.assertIsNone(response.data)
         # delete permission to user 1's app
         response = self.client.delete(url, content_type='application/json',
@@ -226,7 +227,7 @@ def test_delete(self):
         # delete permission to user 1's app again, expecting an error
         response = self.client.delete(url, content_type='application/json',
                                       HTTP_AUTHORIZATION='token {}'.format(self.token))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)
 
     def test_list(self):
         # check that user 1 sees her lone app and user 2's app
@@ -259,7 +260,7 @@ def test_list_errors(self):
         response = self.client.get(
             "/v1/apps/{}/perms".format(app_id), content_type='application/json',
             HTTP_AUTHORIZATION='token {}'.format(self.token2))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)
 
     def test_unauthorized_user_cannot_modify_perms(self):
         """
@@ -279,4 +280,4 @@ def test_unauthorized_user_cannot_modify_perms(self):
         body = {'username': unauthorized_user.username}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(unauthorized_token))
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 403)

controller/api/views.py

@@ -8,7 +8,6 @@
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser, User
 from django.core.exceptions import ValidationError
-from django.http import Http404
 from django.utils import timezone
 from guardian.shortcuts import assign_perm
 from guardian.shortcuts import get_objects_for_user
@@ -104,15 +103,15 @@ def list(self, request, **kwargs):
         if request.user != app.owner and \
                 not request.user.has_perm(perm_name, app) and \
                 not request.user.is_superuser:
-            return Response(status=status.HTTP_404_NOT_FOUND)
+            return Response(status=status.HTTP_403_FORBIDDEN)
         usernames = [u.username for u in get_users_with_perms(app)
                      if u.has_perm(perm_name, app)]
         return Response({'users': usernames})
 
     def create(self, request, **kwargs):
         app = get_object_or_404(self.model, id=kwargs['id'])
         if request.user != app.owner and not request.user.is_superuser:
-            return Response(status=status.HTTP_404_NOT_FOUND)
+            return Response(status=status.HTTP_403_FORBIDDEN)
         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))
@@ -121,14 +120,14 @@ def create(self, request, **kwargs):
     def destroy(self, request, **kwargs):
         app = get_object_or_404(self.model, id=kwargs['id'])
         if request.user != app.owner and not request.user.is_superuser:
-            return Response(status=status.HTTP_404_NOT_FOUND)
+            return Response(status=status.HTTP_403_FORBIDDEN)
         user = get_object_or_404(User, username=kwargs['username'])
         if user.has_perm(self.perm, app):
             remove_perm(self.perm, user, app)
             models.log_event(app, "User {} was revoked access to {}".format(user, app))
             return Response(status=status.HTTP_204_NO_CONTENT)
         else:
-            return Response(status=status.HTTP_404_NOT_FOUND)
+            return Response(status=status.HTTP_403_FORBIDDEN)
 
 
 class AdminPermsViewSet(viewsets.ModelViewSet):
@@ -138,22 +137,8 @@ class AdminPermsViewSet(viewsets.ModelViewSet):
     serializer_class = serializers.AdminUserSerializer
     permission_classes = (IsAdmin,)
 
-    def check_obj_permissions(self, obj):
-        """
-        Small wrapper around check_object_permissions().
-
-        If the user is denied permission to the object, then
-        it should return a 404 so the user is not aware of the
-        application resource.
-        """
-        try:
-            self.check_object_permissions(self.request, obj)
-        except PermissionDenied:
-            raise Http404("No {} matches the given query.".format(
-                self.model._meta.object_name))
-
     def get_queryset(self, **kwargs):
-        self.check_obj_permissions(self.request.user)
+        self.check_object_permissions(self.request, self.request.user)
         return self.model.objects.filter(is_active=True, is_superuser=True)
 
     def create(self, request, **kwargs):
@@ -240,31 +225,17 @@ class BaseAppViewSet(viewsets.ModelViewSet):
 
     permission_classes = (permissions.IsAuthenticated, IsAppUser)
 
-    def check_obj_permissions(self, obj):
-        """
-        Small wrapper around check_object_permissions().
-
-        If the user is denied permission to the object, then
-        it should return a 404 so the user is not aware of the
-        application resource.
-        """
-        try:
-            self.check_object_permissions(self.request, obj)
-        except PermissionDenied:
-            raise Http404("No {} matches the given query.".format(
-                self.model._meta.object_name))
-
     def pre_save(self, obj):
         obj.owner = self.request.user
 
     def get_queryset(self, **kwargs):
         app = get_object_or_404(models.App, id=self.kwargs['id'])
-        self.check_obj_permissions(app)
+        self.check_object_permissions(self.request, app)
         return self.model.objects.filter(app=app)
 
     def get_object(self, *args, **kwargs):
         obj = self.get_queryset().latest('created')
-        self.check_obj_permissions(obj)
+        self.check_object_permissions(self.request, obj)
         return obj
 
 
@@ -285,7 +256,7 @@ def get_success_headers(self, data):
 
     def create(self, request, *args, **kwargs):
         app = get_object_or_404(models.App, id=self.kwargs['id'])
-        self.check_obj_permissions(app)
+        self.check_object_permissions(self.request, app)
         request._data = request.DATA.copy()
         request.DATA['app'] = app
         try:
@@ -303,7 +274,7 @@ class AppConfigViewSet(BaseAppViewSet):
     def get_object(self, *args, **kwargs):
         """Return the Config associated with the App's latest Release."""
         app = get_object_or_404(models.App, id=self.kwargs['id'])
-        self.check_obj_permissions(app)
+        self.check_object_permissions(self.request, app)
         return app.release_set.latest().config
 
     def pre_save(self, config):
@@ -418,35 +389,21 @@ class DomainViewSet(OwnerViewSet):
     model = models.Domain
     serializer_class = serializers.DomainSerializer
 
-    def check_obj_permissions(self, obj):
-        """
-        Small wrapper around check_object_permissions().
-
-        If the user is denied permission to the object, then
-        it should return a 404 so the user is not aware of the
-        application resource.
-        """
-        try:
-            self.check_object_permissions(self.request, obj)
-        except PermissionDenied:
-            raise Http404("No {} matches the given query.".format(
-                self.model._meta.object_name))
-
     def create(self, request, *args, **kwargs):
         app = get_object_or_404(models.App, id=self.kwargs['id'])
-        self.check_obj_permissions(app)
+        self.check_object_permissions(self.request, app)
         request._data = request.DATA.copy()
         request.DATA['app'] = app
         return super(DomainViewSet, self).create(request, *args, **kwargs)
 
     def get_queryset(self, **kwargs):
         app = get_object_or_404(models.App, id=self.kwargs['id'])
-        self.check_obj_permissions(app)
+        self.check_object_permissions(self.request, app)
         return self.model.objects.filter(app=app)
 
     def get_object(self, *args, **kwargs):
-        obj = self.get_queryset().get(domain=self.kwargs['domain'])
-        self.check_obj_permissions(obj)
+        qs = self.get_queryset(**kwargs)
+        obj = qs.get(domain=self.kwargs['domain'])
         return obj
 
 

tests/perms_test.go

@@ -53,7 +53,7 @@ func permsCreateAdminTest(t *testing.T, params *utils.DeisTestConfig) {
 
 func permsCreateAppTest(t *testing.T, params, user *utils.DeisTestConfig) {
 	utils.Execute(t, authLoginCmd, user, false, "")
-	utils.Execute(t, permsCreateAppCmd, user, true, "404 NOT FOUND")
+	utils.Execute(t, permsCreateAppCmd, user, true, "403 FORBIDDEN")
 	utils.Execute(t, authLoginCmd, params, false, "")
 	utils.Execute(t, permsCreateAppCmd, params, false, "")
 	utils.CheckList(t, permsListAppCmd, params, "test1", false)
@@ -66,7 +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, "404 NOT FOUND")
+	utils.Execute(t, permsDeleteAppCmd, user, true, "403 FORBIDDEN")
 	utils.Execute(t, authLoginCmd, params, false, "")
 	utils.Execute(t, permsDeleteAppCmd, params, false, "")
 	utils.CheckList(t, permsListAppCmd, params, "test1", true)