From c477bab6f4bbffd294afb68f5caef2a78347161d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Nov 2014 19:38:39 -0800 Subject: [PATCH] Update the REST API course_team_handler to support library roles, fix bugs --- cms/djangoapps/contentstore/views/library.py | 3 +- .../contentstore/views/tests/test_user.py | 8 +- cms/djangoapps/contentstore/views/user.py | 126 ++++++++---------- cms/urls.py | 8 +- common/djangoapps/student/auth.py | 61 ++++++--- 5 files changed, 112 insertions(+), 94 deletions(-) diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index afebda11f7..3957765a38 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -188,5 +188,6 @@ def library_blocks_view(library, user, response_format): 'context_library': library, 'component_templates': json.dumps(component_templates), 'xblock_info': xblock_info, - 'templates': CONTAINER_TEMPATES + 'templates': CONTAINER_TEMPATES, + 'lib_users_url': reverse_library_url('manage_library_users', unicode(library.location.library_key)), }) diff --git a/cms/djangoapps/contentstore/views/tests/test_user.py b/cms/djangoapps/contentstore/views/tests/test_user.py index f4fcc609d3..ce99a266ef 100644 --- a/cms/djangoapps/contentstore/views/tests/test_user.py +++ b/cms/djangoapps/contentstore/views/tests/test_user.py @@ -70,7 +70,7 @@ class UsersTestCase(CourseTestCase): def test_detail_post(self): resp = self.client.post( self.detail_url, - data={"role": None}, + data={"role": ""}, ) self.assertEqual(resp.status_code, 204) # reload user from DB @@ -218,7 +218,7 @@ class UsersTestCase(CourseTestCase): data={"role": "instructor"}, HTTP_ACCEPT="application/json", ) - self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.status_code, 403) result = json.loads(resp.content) self.assertIn("error", result) @@ -232,7 +232,7 @@ class UsersTestCase(CourseTestCase): data={"role": "instructor"}, HTTP_ACCEPT="application/json", ) - self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.status_code, 403) result = json.loads(resp.content) self.assertIn("error", result) @@ -255,7 +255,7 @@ class UsersTestCase(CourseTestCase): self.user.save() resp = self.client.delete(self.detail_url) - self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.status_code, 403) result = json.loads(resp.content) self.assertIn("error", result) # reload user from DB diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index c73ecc651f..812890a451 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -9,11 +9,12 @@ from edxmako.shortcuts import render_to_response from xmodule.modulestore.django import modulestore from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryLocator from util.json_request import JsonResponse, expect_json -from student.roles import CourseInstructorRole, CourseStaffRole +from student.roles import CourseInstructorRole, CourseStaffRole, LibraryUserRole from course_creators.views import user_requested_access -from student.auth import has_course_author_access +from student.auth import STUDIO_EDIT_ROLES, STUDIO_VIEW_USERS, get_user_permissions from student.models import CourseEnrollment from django.http import HttpResponseNotFound @@ -50,8 +51,7 @@ def course_team_handler(request, course_key_string=None, email=None): json: remove a particular course team member from the course team (email is required). """ course_key = CourseKey.from_string(course_key_string) if course_key_string else None - if not has_course_author_access(request.user, course_key): - raise PermissionDenied() + # No permissions check here - each helper method does its own check. if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): return _course_team_user(request, course_key, email) @@ -66,7 +66,8 @@ def _manage_users(request, course_key): This view will return all CMS users who are editors for the specified course """ # check that logged in user has permissions to this item - if not has_course_author_access(request.user, course_key): + user_perms = get_user_permissions(request.user, course_key) + if not (user_perms & STUDIO_VIEW_USERS): raise PermissionDenied() course_module = modulestore().get_course(course_key) @@ -78,7 +79,7 @@ def _manage_users(request, course_key): 'context_course': course_module, 'staff': staff, 'instructors': instructors, - 'allow_actions': has_course_author_access(request.user, course_key, role=CourseInstructorRole), + 'allow_actions': bool(user_perms & STUDIO_EDIT_ROLES), }) @@ -88,17 +89,14 @@ def _course_team_user(request, course_key, email): Handle the add, remove, promote, demote requests ensuring the requester has authority """ # check that logged in user has permissions to this item - if has_course_author_access(request.user, course_key, role=CourseInstructorRole): - # instructors have full permissions - pass - elif has_course_author_access(request.user, course_key, role=CourseStaffRole) and email == request.user.email: - # staff can only affect themselves + requester_perms = get_user_permissions(request.user, course_key) + permissions_error_response = JsonResponse({"error": _("Insufficient permissions")}, 403) + if (requester_perms & STUDIO_VIEW_USERS) or (email == request.user.email): + # This user has permissions to at least view the list of users or is editing themself pass else: - msg = { - "error": _("Insufficient permissions") - } - return JsonResponse(msg, 400) + # This user is not even allowed to know who the authorized users are. + return permissions_error_response try: user = User.objects.get(email=email) @@ -108,7 +106,13 @@ def _course_team_user(request, course_key, email): } return JsonResponse(msg, 404) - # role hierarchy: globalstaff > "instructor" > "staff" (in a course) + is_library = isinstance(course_key, LibraryLocator) + # Ordered list of roles: can always move self to the right, but need STUDIO_EDIT_ROLES to move any user left + if is_library: + role_hierarchy = (CourseInstructorRole, CourseStaffRole, LibraryUserRole) + else: + role_hierarchy = (CourseInstructorRole, CourseStaffRole) + if request.method == "GET": # just return info about the user msg = { @@ -117,12 +121,17 @@ def _course_team_user(request, course_key, email): "role": None, } # what's the highest role that this user has? (How should this report global staff?) - for role in [CourseInstructorRole(course_key), CourseStaffRole(course_key)]: - if role.has_user(user): + for role in role_hierarchy: + if role(course_key).has_user(user): msg["role"] = role.ROLE break return JsonResponse(msg) + # All of the following code is for editing/promoting/deleting users. + # Check that the user has STUDIO_EDIT_ROLES permission or is editing themselves: + if not ((requester_perms & STUDIO_EDIT_ROLES) or (user.id == request.user.id)): + return permissions_error_response + # can't modify an inactive user if not user.is_active: msg = { @@ -131,60 +140,43 @@ def _course_team_user(request, course_key, email): return JsonResponse(msg, 400) if request.method == "DELETE": - try: - try_remove_instructor(request, course_key, user) - except CannotOrphanCourse as oops: - return JsonResponse(oops.msg, 400) + new_role = None + else: + # only other operation supported is to promote/demote a user by changing their role: + # role may be None or "" (equivalent to a DELETE request) but must be set. + # Check that the new role was specified: + if "role" in request.json or "role" in request.POST: + new_role = request.json.get("role", request.POST.get("role")) + else: + return JsonResponse({"error": _("No `role` specified.")}, 400) - auth.remove_users(request.user, CourseStaffRole(course_key), user) - return JsonResponse() + old_roles = set() + role_added = False + for role_type in role_hierarchy: + role = role_type(course_key) + if role_type.ROLE == new_role: + if (requester_perms & STUDIO_EDIT_ROLES) or (user.id == request.user.id and old_roles): + # User has STUDIO_EDIT_ROLES permission or is currently a member of a higher role, and is thus demoting themself + auth.add_users(request.user, role, user) + role_added = True + else: + return permissions_error_response + elif role.has_user(user): + # Remove the user from this old role: + old_roles.add(role) - # all other operations require the requesting user to specify a role - role = request.json.get("role", request.POST.get("role")) - if role is None: - return JsonResponse({"error": _("`role` is required")}, 400) + if new_role and not role_added: + return JsonResponse({"error": _("Invalid `role` specified.")}, 400) - if role == "instructor": - if not has_course_author_access(request.user, course_key, role=CourseInstructorRole): - msg = { - "error": _("Only instructors may create other instructors") - } + for role in old_roles: + if isinstance(role, CourseInstructorRole) and role.users_with_role().count() == 1: + msg = {"error": _("You may not remove the last Admin. Add another Admin first.")} return JsonResponse(msg, 400) - auth.add_users(request.user, CourseInstructorRole(course_key), user) - # auto-enroll the course creator in the course so that "View Live" will work. - CourseEnrollment.enroll(user, course_key) - elif role == "staff": - # add to staff regardless (can't do after removing from instructors as will no longer - # be allowed) - auth.add_users(request.user, CourseStaffRole(course_key), user) - try: - try_remove_instructor(request, course_key, user) - except CannotOrphanCourse as oops: - return JsonResponse(oops.msg, 400) + auth.remove_users(request.user, role, user) - # auto-enroll the course creator in the course so that "View Live" will work. + if new_role and not is_library: + # The user may be newly added to this course. + # auto-enroll the user in the course so that "View Live" will work. CourseEnrollment.enroll(user, course_key) return JsonResponse() - - -class CannotOrphanCourse(Exception): - """ - Exception raised if an attempt is made to remove all responsible instructors from course. - """ - def __init__(self, msg): - self.msg = msg - Exception.__init__(self) - - -def try_remove_instructor(request, course_key, user): - - # remove all roles in this course from this user: but fail if the user - # is the last instructor in the course team - instructors = CourseInstructorRole(course_key) - if instructors.has_user(user): - if instructors.users_with_role().count() == 1: - msg = {"error": _("You may not remove the last instructor from a course")} - raise CannotOrphanCourse(msg) - else: - auth.remove_users(request.user, instructors, user) diff --git a/cms/urls.py b/cms/urls.py index d5a3b9b83a..293672afbd 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -5,6 +5,11 @@ from django.conf.urls import patterns, include, url from ratelimitbackend import admin admin.autodiscover() +# Pattern to match a course key or a library key +COURSELIKE_KEY_PATTERN = r'(?P({}|{}))'.format(r'[^/]+/[^/]+/[^/]+', r'[^/:]+:[^/+]+\+[^/+]+(\+[^/]+)?') +# Pattern to match a library key only +LIBRARY_KEY_PATTERN = r'(?Plibrary-v1:[^/+]+\+[^/+]+)' + urlpatterns = patterns('', # nopep8 url(r'^transcripts/upload$', 'contentstore.views.upload_transcripts', name='upload_transcripts'), @@ -66,7 +71,7 @@ urlpatterns += patterns( url(r'^signin$', 'login_page', name='login'), url(r'^request_course_creator$', 'request_course_creator'), - url(r'^course_team/{}/(?P.+)?$'.format(settings.COURSE_KEY_PATTERN), 'course_team_handler'), + url(r'^course_team/{}/(?P.+)?$'.format(COURSELIKE_KEY_PATTERN), 'course_team_handler'), url(r'^course_info/{}$'.format(settings.COURSE_KEY_PATTERN), 'course_info_handler'), url( r'^course_info_update/{}/(?P\d+)?$'.format(settings.COURSE_KEY_PATTERN), @@ -113,7 +118,6 @@ urlpatterns += patterns( ) if settings.FEATURES.get('ENABLE_CONTENT_LIBRARIES'): - LIBRARY_KEY_PATTERN = r'(?Plibrary-v1:[^/+]+\+[^/+]+)' urlpatterns += ( url(r'^library/{}?$'.format(LIBRARY_KEY_PATTERN), 'contentstore.views.library_handler', name='library_handler'), diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index d272249b3d..90b311e77e 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -12,6 +12,14 @@ from student.roles import GlobalStaff, CourseCreatorRole, CourseStaffRole, Cours CourseBetaTesterRole, OrgInstructorRole, OrgStaffRole, LibraryUserRole, OrgLibraryUserRole +# Studio permissions: +STUDIO_EDIT_ROLES = 8 +STUDIO_VIEW_USERS = 4 +STUDIO_EDIT_CONTENT = 2 +STUDIO_VIEW_CONTENT = 1 +# In addition to the above, one is always allowed to "demote" oneself to a lower role within a course, or remove oneself. + + def has_access(user, role): """ Check whether this user has access to this role (either direct or implied) @@ -41,7 +49,34 @@ def has_access(user, role): return False -def has_studio_write_access(user, course_key, role=CourseStaffRole): +def get_user_permissions(user, course_key, org=None): + """ + Get the bitmask of permissions that this user has in the given course context. + Can also set course_key=None and pass in an org to get the user's + permissions for that organization as a whole. + """ + if org is None: + org = course_key.org + course_key = course_key.for_branch(None) + else: + assert course_key is None + all_perms = STUDIO_EDIT_ROLES | STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT + # global staff, org instructors, and course instructors have all permissions: + if GlobalStaff().has_user(user) or OrgInstructorRole(org=org).has_user(user): + return all_perms + if course_key and has_access(user, CourseInstructorRole(course_key)): + return all_perms + # Staff have all permissions except EDIT_ROLES: + if OrgStaffRole(org=org).has_user(user) or (course_key and has_access(user, CourseStaffRole(course_key))): + return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT + # Otherwise, for libraries, users can view only: + if (course_key and isinstance(course_key, LibraryLocator)): + if OrgLibraryUserRole(org=org).has_user(user) or has_access(user, LibraryUserRole(course_key)): + return STUDIO_VIEW_USERS | STUDIO_VIEW_CONTENT + return 0 + + +def has_studio_write_access(user, course_key): """ Return True if user has studio write access to the given course. Note that the CMS permissions model is with respect to courses. @@ -53,23 +88,15 @@ def has_studio_write_access(user, course_key, role=CourseStaffRole): :param user: :param course_key: a CourseKey - :param role: an AccessRole """ - if GlobalStaff().has_user(user): - return True - if OrgInstructorRole(org=course_key.org).has_user(user): - return True - if OrgStaffRole(org=course_key.org).has_user(user): - return True - # temporary to ensure we give universal access given a course until we impl branch specific perms - return has_access(user, role(course_key.for_branch(None))) + return bool(STUDIO_EDIT_CONTENT & get_user_permissions(user, course_key)) -def has_course_author_access(*args, **kwargs): +def has_course_author_access(user, course_key): """ - Old name for has_studio_author_access + Old name for has_studio_write_access """ - return has_studio_read_access(*args, **kwargs) + return has_studio_write_access(user, course_key) def has_studio_read_access(user, course_key): @@ -80,13 +107,7 @@ def has_studio_read_access(user, course_key): There is currently no such thing as read-only course access in studio, but there is read-only access to content libraries. """ - if has_studio_write_access(user, course_key): - return True # Global, Org, or Course "Instructors" and "Staff" can read and write - if isinstance(course_key, LibraryLocator): - if OrgLibraryUserRole(org=course_key.org).has_user(user): - return True # User has read-only access to all libraries in this organization - return LibraryUserRole(course_key.for_branch(None)).has_user(user) # User has read-only access this library - return False + return bool(STUDIO_VIEW_CONTENT & get_user_permissions(user, course_key)) def add_users(caller, role, *users):