Merge pull request #14223 from edx/douglashall/fix_staff_or_owner_api_permissions_check

ECOM-6706 Check API View kwargs for username when checking for staff or owner permission

openedx/core/djangoapps/user_api/verification_api/tests/test_views.py

@@ -26,8 +26,8 @@ class PhotoVerificationStatusViewTests(TestCase):
 
     def setUp(self):
         super(PhotoVerificationStatusViewTests, self).setUp()
-        self.user = UserFactory.create(password=self.PASSWORD)
-        self.staff = UserFactory.create(is_staff=True, password=self.PASSWORD)
+        self.user = UserFactory(password=self.PASSWORD)
+        self.staff = UserFactory(is_staff=True, password=self.PASSWORD)
         self.verification = SoftwareSecurePhotoVerification.objects.create(user=self.user, status='submitted')
         self.path = reverse('verification_status', kwargs={'username': self.user.username})
         self.client.login(username=self.staff.username, password=self.PASSWORD)
@@ -57,7 +57,7 @@ class PhotoVerificationStatusViewTests(TestCase):
 
     def test_no_verifications(self):
         """ The endpoint should return HTTP 404 if the user has no verifications. """
-        user = UserFactory.create()
+        user = UserFactory()
         path = reverse('verification_status', kwargs={'username': user.username})
         self.assert_path_not_found(path)
 
@@ -69,17 +69,19 @@ class PhotoVerificationStatusViewTests(TestCase):
 
     def test_staff_user(self):
         """ The endpoint should be accessible to staff users. """
+        self.client.logout()
         self.client.login(username=self.staff.username, password=self.PASSWORD)
         self.assert_verification_returned()
 
     def test_owner(self):
         """ The endpoint should be accessible to the user who submitted the verification request. """
-        self.client.login(username=self.user.username, password=self.user.password)
+        self.client.logout()
+        self.client.login(username=self.user.username, password=self.PASSWORD)
         self.assert_verification_returned()
 
     def test_non_owner_or_staff_user(self):
         """ The endpoint should NOT be accessible if the request is not made by the submitter or staff user. """
-        user = UserFactory.create()
+        user = UserFactory()
         self.client.login(username=user.username, password=self.PASSWORD)
         response = self.client.get(self.path)
         self.assertEqual(response.status_code, 403)
@@ -88,5 +90,6 @@ class PhotoVerificationStatusViewTests(TestCase):
         """ The endpoint should return that the user is verified if the user's verification is accepted. """
         self.verification.status = 'approved'
         self.verification.save()
-        self.client.login(username=self.user.username, password=self.user.password)
+        self.client.logout()
+        self.client.login(username=self.user.username, password=self.PASSWORD)
         self.assert_verification_returned(verified=True)

openedx/core/lib/api/permissions.py

@@ -156,4 +156,5 @@ class IsStaffOrOwner(permissions.BasePermission):
         user = request.user
         return user.is_staff \
             or (user.username == request.GET.get('username')) \
-            or (user.username == getattr(request, 'data', {}).get('username'))
+            or (user.username == getattr(request, 'data', {}).get('username')) \
+            or (user.username == getattr(view, 'kwargs', {}).get('username'))

openedx/core/lib/api/tests/test_permissions.py

@@ -5,6 +5,7 @@ from django.contrib.auth.models import AnonymousUser
 from django.http import Http404
 from django.test import TestCase, RequestFactory
 from nose.plugins.attrib import attr
+from rest_framework.generics import GenericAPIView
 
 from student.roles import CourseStaffRole, CourseInstructorRole
 from openedx.core.lib.api.permissions import (
@@ -37,8 +38,8 @@ class IsCourseStaffInstructorTests(TestCase):
     def setUp(self):
         super(IsCourseStaffInstructorTests, self).setUp()
         self.permission = IsCourseStaffInstructor()
-        self.coach = UserFactory.create()
-        self.user = UserFactory.create()
+        self.coach = UserFactory()
+        self.user = UserFactory()
         self.request = RequestFactory().get('/')
         self.request.user = self.user
         self.course_key = CourseKey.from_string('edx/test123/run')
@@ -72,7 +73,7 @@ class IsMasterCourseStaffInstructorTests(TestCase):
         super(IsMasterCourseStaffInstructorTests, self).setUp()
         self.permission = IsMasterCourseStaffInstructor()
         master_course_id = 'edx/test123/run'
-        self.user = UserFactory.create()
+        self.user = UserFactory()
         self.get_request = RequestFactory().get('/?master_course_id={}'.format(master_course_id))
         self.get_request.user = self.user
         self.post_request = RequestFactory().post('/', data={'master_course_id': master_course_id})
@@ -133,36 +134,44 @@ class IsStaffOrOwnerTests(TestCase):
 
     def test_staff_user(self):
         """ Staff users should be permitted. """
-        user = UserFactory.create(is_staff=True)
+        user = UserFactory(is_staff=True)
         self.assert_user_has_object_permission(user, True)
 
     def test_owner(self):
         """ Owners should be permitted. """
-        user = UserFactory.create()
+        user = UserFactory()
         self.obj.user = user
         self.assert_user_has_object_permission(user, True)
 
     def test_non_staff_test_non_owner_or_staff_user(self):
         """ Non-staff and non-owner users should not be permitted. """
-        user = UserFactory.create()
+        user = UserFactory()
         self.assert_user_has_object_permission(user, False)
 
     def test_has_permission_as_staff(self):
         """ Staff users always have permission. """
-        self.request.user = UserFactory.create(is_staff=True)
+        self.request.user = UserFactory(is_staff=True)
         self.assertTrue(self.permission.has_permission(self.request, None))
 
     def test_has_permission_as_owner_with_get(self):
         """ Owners always have permission to make GET actions. """
-        user = UserFactory.create()
+        user = UserFactory()
         request = RequestFactory().get('/?username={}'.format(user.username))
         request.user = user
         self.assertTrue(self.permission.has_permission(request, None))
 
+    def test_has_permission_with_view_kwargs_as_owner_with_get(self):
+        """ Owners always have permission to make GET actions. """
+        user = UserFactory()
+        self.request.user = user
+        view = GenericAPIView()
+        view.kwargs = {'username': user.username}
+        self.assertTrue(self.permission.has_permission(self.request, view))
+
     @ddt.data('patch', 'post', 'put')
     def test_has_permission_as_owner_with_edit(self, action):
         """ Owners always have permission to edit. """
-        user = UserFactory.create()
+        user = UserFactory()
 
         data = {'username': user.username}
         request = getattr(RequestFactory(), action)('/', data, format='json')
@@ -172,7 +181,15 @@ class IsStaffOrOwnerTests(TestCase):
 
     def test_has_permission_as_non_owner(self):
         """ Non-owners should not have permission. """
-        user = UserFactory.create()
+        user = UserFactory()
         request = RequestFactory().get('/?username={}'.format(user.username))
-        request.user = UserFactory.create()
+        request.user = UserFactory()
         self.assertFalse(self.permission.has_permission(request, None))
+
+    def test_has_permission_with_view_kwargs_as_non_owner(self):
+        """ Non-owners should not have permission. """
+        user = UserFactory()
+        self.request.user = user
+        view = GenericAPIView()
+        view.kwargs = {'username': UserFactory().username}
+        self.assertFalse(self.permission.has_permission(self.request, view))