From dfadb28343190ff0dcd504a1e2b93a3ddd9cdd0c Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Fri, 11 Dec 2015 12:45:02 -0500 Subject: [PATCH] Fixed Permissions Bug The permissions class now supports non-GET requests. ECOM-2893 --- openedx/core/lib/api/permissions.py | 14 +++++++++++--- .../core/lib/api/tests/test_permissions.py | 19 +++++++++++++++++-- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/openedx/core/lib/api/permissions.py b/openedx/core/lib/api/permissions.py index 2d27c58bf9..6b7de66e7e 100644 --- a/openedx/core/lib/api/permissions.py +++ b/openedx/core/lib/api/permissions.py @@ -3,8 +3,8 @@ API library for Django REST Framework permissions-oriented workflows """ from django.conf import settings -from rest_framework import permissions from django.http import Http404 +from rest_framework import permissions from student.roles import CourseStaffRole @@ -13,6 +13,7 @@ class ApiKeyHeaderPermission(permissions.BasePermission): """ Django REST Framework permissions class used to manage API Key integrations """ + def has_permission(self, request, view): """ Check for permissions by matching the configured API key and header @@ -35,8 +36,9 @@ class ApiKeyHeaderPermissionIsAuthenticated(ApiKeyHeaderPermission, permissions. See ApiKeyHeaderPermission for more information how the API key portion is implemented. """ + def has_permission(self, request, view): - #TODO We can optimize this later on when we know which of these methods is used more often. + # TODO We can optimize this later on when we know which of these methods is used more often. api_permissions = ApiKeyHeaderPermission.has_permission(self, request, view) is_authenticated_permissions = permissions.IsAuthenticated.has_permission(self, request, view) return api_permissions or is_authenticated_permissions @@ -46,6 +48,7 @@ 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. @@ -65,6 +68,7 @@ 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 @@ -76,6 +80,7 @@ class IsStaffOrReadOnly(permissions.BasePermission): """Permission that checks to see if the user is global or course staff, permitting only read-only access if they are not. """ + def has_object_permission(self, request, view, obj): return (request.user.is_staff or CourseStaffRole(obj.course_id).has_user(request.user) or @@ -87,9 +92,12 @@ class IsStaffOrOwner(permissions.BasePermission): Permission that allows access to admin users or the owner of an object. The owner is considered the User object represented by obj.user. """ + def has_object_permission(self, request, view, obj): return request.user.is_staff or obj.user == request.user def has_permission(self, request, view): user = request.user - return user.is_staff or (user.username == request.GET.get('username')) + return user.is_staff \ + or (user.username == request.GET.get('username')) \ + or (user.username == getattr(request, 'data', {}).get('username')) diff --git a/openedx/core/lib/api/tests/test_permissions.py b/openedx/core/lib/api/tests/test_permissions.py index cc2436085a..1ab26cd4d2 100644 --- a/openedx/core/lib/api/tests/test_permissions.py +++ b/openedx/core/lib/api/tests/test_permissions.py @@ -1,4 +1,6 @@ """ Tests for API permissions classes. """ + +import ddt from django.test import TestCase, RequestFactory from openedx.core.lib.api.permissions import IsStaffOrOwner @@ -10,8 +12,10 @@ class TestObject(object): user = None +@ddt.ddt class IsStaffOrOwnerTests(TestCase): """ Tests for IsStaffOrOwner permission class. """ + def setUp(self): super(IsStaffOrOwnerTests, self).setUp() self.permission = IsStaffOrOwner() @@ -50,13 +54,24 @@ class IsStaffOrOwnerTests(TestCase): self.request.user = UserFactory.create(is_staff=True) self.assertTrue(self.permission.has_permission(self.request, None)) - def test_has_permission_as_owner(self): - """ Owners always have permission. """ + def test_has_permission_as_owner_with_get(self): + """ Owners always have permission to make GET actions. """ user = UserFactory.create() request = RequestFactory().get('/?username={}'.format(user.username)) request.user = user self.assertTrue(self.permission.has_permission(request, None)) + @ddt.data('patch', 'post', 'put') + def test_has_permission_as_owner_with_edit(self, action): + """ Owners always have permission to edit. """ + user = UserFactory.create() + + data = {'username': user.username} + request = getattr(RequestFactory(), action)('/', data, format='json') + request.user = user + request.data = data # Note (CCB): This is a hack that should be fixed. (ECOM-3171) + self.assertTrue(self.permission.has_permission(request, None)) + def test_has_permission_as_non_owner(self): """ Non-owners should not have permission. """ user = UserFactory.create()