Commit dfadb283 by Clinton Blackburn

Fixed Permissions Bug

The permissions class now supports non-GET requests.

ECOM-2893
parent f042784b
...@@ -3,8 +3,8 @@ API library for Django REST Framework permissions-oriented workflows ...@@ -3,8 +3,8 @@ API library for Django REST Framework permissions-oriented workflows
""" """
from django.conf import settings from django.conf import settings
from rest_framework import permissions
from django.http import Http404 from django.http import Http404
from rest_framework import permissions
from student.roles import CourseStaffRole from student.roles import CourseStaffRole
...@@ -13,6 +13,7 @@ class ApiKeyHeaderPermission(permissions.BasePermission): ...@@ -13,6 +13,7 @@ class ApiKeyHeaderPermission(permissions.BasePermission):
""" """
Django REST Framework permissions class used to manage API Key integrations Django REST Framework permissions class used to manage API Key integrations
""" """
def has_permission(self, request, view): def has_permission(self, request, view):
""" """
Check for permissions by matching the configured API key and header Check for permissions by matching the configured API key and header
...@@ -35,8 +36,9 @@ class ApiKeyHeaderPermissionIsAuthenticated(ApiKeyHeaderPermission, permissions. ...@@ -35,8 +36,9 @@ class ApiKeyHeaderPermissionIsAuthenticated(ApiKeyHeaderPermission, permissions.
See ApiKeyHeaderPermission for more information how the API key portion is implemented. See ApiKeyHeaderPermission for more information how the API key portion is implemented.
""" """
def has_permission(self, request, view): 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) api_permissions = ApiKeyHeaderPermission.has_permission(self, request, view)
is_authenticated_permissions = permissions.IsAuthenticated.has_permission(self, request, view) is_authenticated_permissions = permissions.IsAuthenticated.has_permission(self, request, view)
return api_permissions or is_authenticated_permissions return api_permissions or is_authenticated_permissions
...@@ -46,6 +48,7 @@ class IsUserInUrl(permissions.BasePermission): ...@@ -46,6 +48,7 @@ class IsUserInUrl(permissions.BasePermission):
""" """
Permission that checks to see if the request user matches the user in the URL. Permission that checks to see if the request user matches the user in the URL.
""" """
def has_permission(self, request, view): def has_permission(self, request, view):
""" """
Returns true if the current request is by the user themselves. Returns true if the current request is by the user themselves.
...@@ -65,6 +68,7 @@ class IsUserInUrlOrStaff(IsUserInUrl): ...@@ -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. 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): def has_permission(self, request, view):
if request.user.is_staff: if request.user.is_staff:
return True return True
...@@ -76,6 +80,7 @@ class IsStaffOrReadOnly(permissions.BasePermission): ...@@ -76,6 +80,7 @@ class IsStaffOrReadOnly(permissions.BasePermission):
"""Permission that checks to see if the user is global or course """Permission that checks to see if the user is global or course
staff, permitting only read-only access if they are not. staff, permitting only read-only access if they are not.
""" """
def has_object_permission(self, request, view, obj): def has_object_permission(self, request, view, obj):
return (request.user.is_staff or return (request.user.is_staff or
CourseStaffRole(obj.course_id).has_user(request.user) or CourseStaffRole(obj.course_id).has_user(request.user) or
...@@ -87,9 +92,12 @@ class IsStaffOrOwner(permissions.BasePermission): ...@@ -87,9 +92,12 @@ class IsStaffOrOwner(permissions.BasePermission):
Permission that allows access to admin users or the owner of an object. Permission that allows access to admin users or the owner of an object.
The owner is considered the User object represented by obj.user. The owner is considered the User object represented by obj.user.
""" """
def has_object_permission(self, request, view, obj): def has_object_permission(self, request, view, obj):
return request.user.is_staff or obj.user == request.user return request.user.is_staff or obj.user == request.user
def has_permission(self, request, view): def has_permission(self, request, view):
user = request.user 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'))
""" Tests for API permissions classes. """ """ Tests for API permissions classes. """
import ddt
from django.test import TestCase, RequestFactory from django.test import TestCase, RequestFactory
from openedx.core.lib.api.permissions import IsStaffOrOwner from openedx.core.lib.api.permissions import IsStaffOrOwner
...@@ -10,8 +12,10 @@ class TestObject(object): ...@@ -10,8 +12,10 @@ class TestObject(object):
user = None user = None
@ddt.ddt
class IsStaffOrOwnerTests(TestCase): class IsStaffOrOwnerTests(TestCase):
""" Tests for IsStaffOrOwner permission class. """ """ Tests for IsStaffOrOwner permission class. """
def setUp(self): def setUp(self):
super(IsStaffOrOwnerTests, self).setUp() super(IsStaffOrOwnerTests, self).setUp()
self.permission = IsStaffOrOwner() self.permission = IsStaffOrOwner()
...@@ -50,13 +54,24 @@ class IsStaffOrOwnerTests(TestCase): ...@@ -50,13 +54,24 @@ class IsStaffOrOwnerTests(TestCase):
self.request.user = UserFactory.create(is_staff=True) self.request.user = UserFactory.create(is_staff=True)
self.assertTrue(self.permission.has_permission(self.request, None)) self.assertTrue(self.permission.has_permission(self.request, None))
def test_has_permission_as_owner(self): def test_has_permission_as_owner_with_get(self):
""" Owners always have permission. """ """ Owners always have permission to make GET actions. """
user = UserFactory.create() user = UserFactory.create()
request = RequestFactory().get('/?username={}'.format(user.username)) request = RequestFactory().get('/?username={}'.format(user.username))
request.user = user request.user = user
self.assertTrue(self.permission.has_permission(request, None)) 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): def test_has_permission_as_non_owner(self):
""" Non-owners should not have permission. """ """ Non-owners should not have permission. """
user = UserFactory.create() user = UserFactory.create()
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment