From ce4cb536728c0b1dafcdd7578939f5c141ed9ae4 Mon Sep 17 00:00:00 2001
From: Nimisha Asthagiri <nasthagiri@edx.org>
Date: Thu, 28 Jun 2018 18:13:23 -0400
Subject: [PATCH] Update Permissions to use latest edx-drf-extensions and
rest_condition
---
lms/djangoapps/mobile_api/testutils.py | 4 +-
.../djangoapps/bookmarks/tests/test_views.py | 24 +++++++---
.../profile_images/tests/test_views.py | 16 ++++++-
.../djangoapps/user_api/preferences/api.py | 3 +-
.../user_api/preferences/tests/test_api.py | 10 ++--
.../user_api/preferences/tests/test_views.py | 33 +++++++------
openedx/core/lib/api/permissions.py | 48 ++-----------------
7 files changed, 61 insertions(+), 77 deletions(-)
diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py
index b1af12a70e4..1a19894316d 100644
--- a/lms/djangoapps/mobile_api/testutils.py
+++ b/lms/djangoapps/mobile_api/testutils.py
@@ -116,7 +116,7 @@ class MobileAuthUserTestMixin(MobileAuthTestMixin):
"""
def test_invalid_user(self):
self.login_and_enroll()
- self.api_response(expected_response_code=404, username='no_user')
+ self.api_response(expected_response_code=403, username='no_user')
def test_other_user(self):
# login and enroll as the test user
@@ -131,7 +131,7 @@ class MobileAuthUserTestMixin(MobileAuthTestMixin):
# now login and call the API as the test user
self.login()
- self.api_response(expected_response_code=404, username=other.username)
+ self.api_response(expected_response_code=403, username=other.username)
@ddt.ddt
diff --git a/openedx/core/djangoapps/bookmarks/tests/test_views.py b/openedx/core/djangoapps/bookmarks/tests/test_views.py
index 79ad114512f..e6ab3dc3b51 100644
--- a/openedx/core/djangoapps/bookmarks/tests/test_views.py
+++ b/openedx/core/djangoapps/bookmarks/tests/test_views.py
@@ -17,7 +17,6 @@ from .test_api import BookmarkApiEventTestMixin
from .test_models import BookmarksTestsBase
-# pylint: disable=no-member
class BookmarksViewsTestsBase(BookmarksTestsBase, BookmarkApiEventTestMixin):
"""
Base class for bookmarks views tests.
@@ -397,15 +396,28 @@ class BookmarksDetailViewTests(BookmarksViewsTestsBase):
def test_get_bookmark_that_belongs_to_other_user(self):
"""
- Test that requesting bookmark that belongs to other user returns 404 status code.
+ Test that requesting bookmark that belongs to other user returns 403 status code.
"""
self.send_get(
client=self.client,
url=reverse(
'bookmarks_detail',
- kwargs={'username': 'other', 'usage_id': unicode(self.vertical_1.location)}
+ kwargs={'username': self.other_user.username, 'usage_id': unicode(self.vertical_1.location)}
),
- expected_status=404
+ expected_status=403
+ )
+
+ def test_get_bookmark_that_belongs_to_nonexistent_user(self):
+ """
+ Test that requesting bookmark that belongs to a non-existent user also returns 403 status code.
+ """
+ self.send_get(
+ client=self.client,
+ url=reverse(
+ 'bookmarks_detail',
+ kwargs={'username': 'non-existent', 'usage_id': unicode(self.vertical_1.location)}
+ ),
+ expected_status=403
)
def test_get_bookmark_that_does_not_exist(self):
@@ -482,7 +494,7 @@ class BookmarksDetailViewTests(BookmarksViewsTestsBase):
def test_delete_bookmark_that_belongs_to_other_user(self):
"""
- Test that delete bookmark that belongs to other user returns 404.
+ Test that delete bookmark that belongs to other user returns 403.
"""
self.send_delete(
client=self.client,
@@ -490,7 +502,7 @@ class BookmarksDetailViewTests(BookmarksViewsTestsBase):
'bookmarks_detail',
kwargs={'username': 'other', 'usage_id': unicode(self.vertical_1.location)}
),
- expected_status=404
+ expected_status=403
)
def test_delete_bookmark_that_does_not_exist(self):
diff --git a/openedx/core/djangoapps/profile_images/tests/test_views.py b/openedx/core/djangoapps/profile_images/tests/test_views.py
index 3d8e6d59989..09898364f9a 100644
--- a/openedx/core/djangoapps/profile_images/tests/test_views.py
+++ b/openedx/core/djangoapps/profile_images/tests/test_views.py
@@ -243,6 +243,18 @@ class ProfileImageViewPostTestCase(ProfileImageEndpointMixin, APITestCase):
self.assertFalse(mock_log.info.called)
self.assert_no_events_were_emitted()
+ def test_upload_nonexistent_user(self, mock_log):
+ """
+ Test that an authenticated user who POSTs to a non-existent user's upload
+ endpoint gets an indistinguishable 403.
+ """
+ nonexistent_user_url = reverse(self._view_name, kwargs={'username': 'nonexistent'})
+
+ with make_image_file() as image_file:
+ response = self.client.post(nonexistent_user_url, {'file': image_file}, format='multipart')
+ self.check_response(response, 403)
+ self.assertFalse(mock_log.info.called)
+
def test_upload_other(self, mock_log):
"""
Test that an authenticated user cannot POST to another user's upload
@@ -255,7 +267,7 @@ class ProfileImageViewPostTestCase(ProfileImageEndpointMixin, APITestCase):
different_client.login(username=different_user.username, password=TEST_PASSWORD)
with make_image_file() as image_file:
response = different_client.post(self.url, {'file': image_file}, format='multipart')
- self.check_response(response, 404)
+ self.check_response(response, 403)
self.check_images(False)
self.check_has_profile_image(False)
self.assertFalse(mock_log.info.called)
@@ -407,7 +419,7 @@ class ProfileImageViewDeleteTestCase(ProfileImageEndpointMixin, APITestCase):
different_client = APIClient()
different_client.login(username=different_user.username, password=TEST_PASSWORD)
response = different_client.delete(self.url)
- self.check_response(response, 404)
+ self.check_response(response, 403)
self.check_images(True) # thumbnails should remain intact.
self.check_has_profile_image(True)
self.assertFalse(mock_log.info.called)
diff --git a/openedx/core/djangoapps/user_api/preferences/api.py b/openedx/core/djangoapps/user_api/preferences/api.py
index 54080183498..ecf2dfff0d6 100644
--- a/openedx/core/djangoapps/user_api/preferences/api.py
+++ b/openedx/core/djangoapps/user_api/preferences/api.py
@@ -315,12 +315,13 @@ def _get_authorized_user(requesting_user, username=None, allow_staff=False):
# Otherwise, treat this as a request against a separate user
username = requesting_user.username
+ _check_authorized(requesting_user, username, allow_staff)
+
try:
existing_user = User.objects.get(username=username)
except ObjectDoesNotExist:
raise UserNotFound()
- _check_authorized(requesting_user, username, allow_staff)
return existing_user
diff --git a/openedx/core/djangoapps/user_api/preferences/tests/test_api.py b/openedx/core/djangoapps/user_api/preferences/tests/test_api.py
index 4109aa583f0..5acaca5afe3 100644
--- a/openedx/core/djangoapps/user_api/preferences/tests/test_api.py
+++ b/openedx/core/djangoapps/user_api/preferences/tests/test_api.py
@@ -77,7 +77,7 @@ class TestPreferenceAPI(CacheIsolationTestCase):
"""
Verifies that get_user_preference returns appropriate errors.
"""
- with self.assertRaises(UserNotFound):
+ with self.assertRaises(UserNotAuthorized):
get_user_preference(self.user, self.test_preference_key, username="no_such_user")
with self.assertRaises(UserNotFound):
@@ -100,7 +100,7 @@ class TestPreferenceAPI(CacheIsolationTestCase):
"""
Verifies that get_user_preferences returns appropriate errors.
"""
- with self.assertRaises(UserNotFound):
+ with self.assertRaises(UserNotAuthorized):
get_user_preferences(self.user, username="no_such_user")
with self.assertRaises(UserNotFound):
@@ -125,7 +125,7 @@ class TestPreferenceAPI(CacheIsolationTestCase):
"""
Verifies that set_user_preference returns appropriate errors.
"""
- with self.assertRaises(UserNotFound):
+ with self.assertRaises(UserNotAuthorized):
set_user_preference(self.user, self.test_preference_key, "new_value", username="no_such_user")
with self.assertRaises(UserNotFound):
@@ -227,7 +227,7 @@ class TestPreferenceAPI(CacheIsolationTestCase):
update_data = {
self.test_preference_key: "new_value"
}
- with self.assertRaises(UserNotFound):
+ with self.assertRaises(UserNotAuthorized):
update_user_preferences(self.user, update_data, user="no_such_user")
with self.assertRaises(UserNotFound):
@@ -303,7 +303,7 @@ class TestPreferenceAPI(CacheIsolationTestCase):
"""
Verifies that delete_user_preference returns appropriate errors.
"""
- with self.assertRaises(UserNotFound):
+ with self.assertRaises(UserNotAuthorized):
delete_user_preference(self.user, self.test_preference_key, username="no_such_user")
with self.assertRaises(UserNotFound):
diff --git a/openedx/core/djangoapps/user_api/preferences/tests/test_views.py b/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
index eabc42e315a..0d4e17eb2cb 100644
--- a/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
+++ b/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
@@ -52,7 +52,7 @@ class TestPreferencesAPI(UserAPITestCase):
Test that a client (logged in) cannot get the preferences information for a different client.
"""
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
- self.send_get(self.different_client, expected_status=404)
+ self.send_get(self.different_client, expected_status=403)
@ddt.data(
("client", "user"),
@@ -61,11 +61,11 @@ class TestPreferencesAPI(UserAPITestCase):
@ddt.unpack
def test_get_unknown_user(self, api_client, username):
"""
- Test that requesting a user who does not exist returns a 404.
+ Test that requesting a user who does not exist returns a 404 for staff users, but 403 for others.
"""
client = self.login_client(api_client, username)
response = client.get(reverse(self.url_endpoint_name, kwargs={'username': "does_not_exist"}))
- self.assertEqual(404, response.status_code)
+ self.assertEqual(404 if username == "staff_user" else 403, response.status_code)
def test_get_preferences_default(self):
"""
@@ -83,8 +83,7 @@ class TestPreferencesAPI(UserAPITestCase):
@ddt.unpack
def test_get_preferences(self, api_client, user):
"""
- Test that a client (logged in) can get her own preferences information. Also verifies that a "is_staff"
- user can get the preferences information for other users.
+ Test that a client (logged in) can get her own preferences information.
"""
# Create some test preferences values.
set_user_preference(self.user, "dict_pref", {"int_key": 10})
@@ -104,14 +103,14 @@ class TestPreferencesAPI(UserAPITestCase):
@ddt.unpack
def test_patch_unknown_user(self, api_client, user):
"""
- Test that trying to update preferences for a user who does not exist returns a 404.
+ Test that trying to update preferences for a user who does not exist returns a 403.
"""
client = self.login_client(api_client, user)
response = client.patch(
reverse(self.url_endpoint_name, kwargs={'username': "does_not_exist"}),
data=json.dumps({"string_pref": "value"}), content_type="application/merge-patch+json"
)
- self.assertEqual(404, response.status_code)
+ self.assertEqual(403, response.status_code)
def test_patch_bad_content_type(self):
"""
@@ -168,7 +167,7 @@ class TestPreferencesAPI(UserAPITestCase):
"dict_pref": {"int_key": 10},
"string_pref": "value",
},
- expected_status=403 if user == "staff_user" else 404,
+ expected_status=403,
)
def test_update_preferences(self):
@@ -311,7 +310,7 @@ class TestPreferencesAPI(UserAPITestCase):
"new_pref": "new_value",
"extra_pref": None,
},
- expected_status=403 if user == "staff_user" else 404
+ expected_status=403
)
@@ -405,9 +404,9 @@ class TestPreferencesDetailAPI(UserAPITestCase):
Test that a client (logged in) cannot manipulate a preference for a different client.
"""
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
- self.send_get(self.different_client, expected_status=404)
- self.send_put(self.different_client, "new_value", expected_status=404)
- self.send_delete(self.different_client, expected_status=404)
+ self.send_get(self.different_client, expected_status=403)
+ self.send_put(self.different_client, "new_value", expected_status=403)
+ self.send_delete(self.different_client, expected_status=403)
@ddt.data(
("client", "user"),
@@ -416,13 +415,13 @@ class TestPreferencesDetailAPI(UserAPITestCase):
@ddt.unpack
def test_get_unknown_user(self, api_client, username):
"""
- Test that requesting a user who does not exist returns a 404.
+ Test that requesting a user who does not exist returns a 404 for staff users, but 403 for others.
"""
client = self.login_client(api_client, username)
response = client.get(
reverse(self.url_endpoint_name, kwargs={'username': "does_not_exist", 'preference_key': self.test_pref_key})
)
- self.assertEqual(404, response.status_code)
+ self.assertEqual(404 if username == "staff_user" else 403, response.status_code)
def test_get_preference_does_not_exist(self):
"""
@@ -532,7 +531,7 @@ class TestPreferencesDetailAPI(UserAPITestCase):
self._set_url("new_key")
client = self.login_client(api_client, user)
new_value = "new value"
- self.send_put(client, new_value, expected_status=403 if user == "staff_user" else 404)
+ self.send_put(client, new_value, expected_status=403)
@ddt.data(
(u"new value",),
@@ -560,7 +559,7 @@ class TestPreferencesDetailAPI(UserAPITestCase):
"""
client = self.login_client(api_client, user)
new_value = "new value"
- self.send_put(client, new_value, expected_status=403 if user == "staff_user" else 404)
+ self.send_put(client, new_value, expected_status=403)
@ddt.data(
(None,),
@@ -607,4 +606,4 @@ class TestPreferencesDetailAPI(UserAPITestCase):
Test that a client (logged in) cannot delete a preference for another user.
"""
client = self.login_client(api_client, user)
- self.send_delete(client, expected_status=403 if user == "staff_user" else 404)
+ self.send_delete(client, expected_status=403)
diff --git a/openedx/core/lib/api/permissions.py b/openedx/core/lib/api/permissions.py
index 84684fd7e6e..f5520923cfd 100644
--- a/openedx/core/lib/api/permissions.py
+++ b/openedx/core/lib/api/permissions.py
@@ -6,8 +6,10 @@ from django.conf import settings
from django.http import Http404
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
+from rest_condition import C
from rest_framework import permissions
+from edx_rest_framework_extensions.permissions import IsStaff, IsUserInUrl
from openedx.core.lib.log_utils import audit_log
from student.roles import CourseInstructorRole, CourseStaffRole
@@ -20,7 +22,6 @@ class ApiKeyHeaderPermission(permissions.BasePermission):
def has_permission(self, request, view):
"""
Check for permissions by matching the configured API key and header
-
Allow the request if and only if settings.EDX_API_KEY is set and
the X-Edx-Api-Key HTTP header is present in the request and
matches the setting.
@@ -39,7 +40,6 @@ class ApiKeyHeaderPermission(permissions.BasePermission):
class ApiKeyHeaderPermissionIsAuthenticated(ApiKeyHeaderPermission, permissions.IsAuthenticated):
"""
Allow someone to access the view if they have the API key OR they are authenticated.
-
See ApiKeyHeaderPermission for more information how the API key portion is implemented.
"""
@@ -50,29 +50,6 @@ class ApiKeyHeaderPermissionIsAuthenticated(ApiKeyHeaderPermission, permissions.
return api_permissions or is_authenticated_permissions
-class IsUserInUrl(permissions.BasePermission):
- """
- Permission that checks to see if the request user matches the user in the URL.
- """
-
- def has_permission(self, request, view):
- """
- Returns true if the current request is by the user themselves.
-
- Note: a 404 is returned for non-staff instead of a 403. This is to prevent
- users from being able to detect the existence of accounts.
- """
- url_username = (
- request.parser_context.get('kwargs', {}).get('username') or
- request.GET.get('username', '')
- )
- if request.user.username.lower() != url_username.lower():
- if request.user.is_staff:
- return False # staff gets 403
- raise Http404()
- return True
-
-
class IsCourseStaffInstructor(permissions.BasePermission):
"""
Permission to check that user is a course instructor or staff of
@@ -118,26 +95,9 @@ class IsMasterCourseStaffInstructor(permissions.BasePermission):
return False
-class IsStaff(permissions.BasePermission):
- """
- Permission that checks to see if the request user has is_staff access.
- """
-
+class IsUserInUrlOrStaff(permissions.BasePermission):
def has_permission(self, request, view):
- if request.user.is_staff:
- return True
-
-
-class IsUserInUrlOrStaff(IsUserInUrl):
- """
- Permission that checks to see if the request user matches the user in the URL or has is_staff access.
- """
-
- def has_permission(self, request, view):
- if request.user.is_staff:
- return True
-
- return super(IsUserInUrlOrStaff, self).has_permission(request, view)
+ return C(IsStaff) | IsUserInUrl
class IsStaffOrReadOnly(permissions.BasePermission):
--
GitLab