From ad28783cf0d2dc7287b4566f1469b1655168f078 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 6 Nov 2014 20:01:03 -0800 Subject: [PATCH 1/9] Three levels of user permissions for content libraries: Admin ("Instructor") - Can edit and assign permissions to other users Normal ("Staff") - Can edit User - Can view the library and use content from it but cannot edit it or its blocks. --- cms/djangoapps/contentstore/views/course.py | 19 ++++++------ cms/djangoapps/contentstore/views/item.py | 23 +++++++++------ cms/djangoapps/contentstore/views/library.py | 23 ++++++++++----- cms/static/js/factories/library.js | 3 +- cms/static/js/views/paged_container.js | 3 +- cms/static/js/views/pages/container.js | 31 +++++++++++++------- cms/static/sass/views/_dashboard.scss | 15 ++++------ cms/templates/container.html | 3 +- cms/templates/index.html | 3 ++ cms/templates/library.html | 8 +++-- common/djangoapps/student/auth.py | 31 ++++++++++++++++++-- common/djangoapps/student/roles.py | 22 ++++++++++++++ 12 files changed, 130 insertions(+), 54 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 57ffa6b9e1ce..5bad2efa2207 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -48,7 +48,7 @@ from models.settings.course_metadata import CourseMetadata from util.json_request import expect_json from util.string_utils import _has_non_ascii_characters -from student.auth import has_course_author_access +from student.auth import has_studio_write_access, has_studio_read_access from .component import ( OPEN_ENDED_COMPONENT_TYPES, NOTE_COMPONENT_TYPES, @@ -96,7 +96,7 @@ def get_course_and_check_access(course_key, user, depth=0): Internal method used to calculate and return the locator and course module for the view functions in this file. """ - if not has_course_author_access(user, course_key): + if not has_studio_read_access(user, course_key): raise PermissionDenied() course_module = modulestore().get_course(course_key, depth=depth) return course_module @@ -130,7 +130,7 @@ def course_notifications_handler(request, course_key_string=None, action_state_i course_key = CourseKey.from_string(course_key_string) if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): - if not has_course_author_access(request.user, course_key): + if not has_studio_write_access(request.user, course_key): raise PermissionDenied() if request.method == 'GET': return _course_notifications_json_get(action_state_id) @@ -220,7 +220,7 @@ def course_handler(request, course_key_string=None): return JsonResponse(_course_outline_json(request, course_module)) elif request.method == 'POST': # not sure if this is only post. If one will have ids, it goes after access return _create_or_rerun_course(request) - elif not has_course_author_access(request.user, CourseKey.from_string(course_key_string)): + elif not has_studio_write_access(request.user, CourseKey.from_string(course_key_string)): raise PermissionDenied() elif request.method == 'PUT': raise NotImplementedError() @@ -292,7 +292,7 @@ def course_filter(course): if course.location.course == 'templates': return False - return has_course_author_access(request.user, course.id) + return has_studio_read_access(request.user, course.id) courses = filter(course_filter, modulestore().get_courses()) in_process_course_actions = [ @@ -300,7 +300,7 @@ def course_filter(course): CourseRerunState.objects.find_all( exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED}, should_display=True ) - if has_course_author_access(request.user, course.course_key) + if has_studio_read_access(request.user, course.course_key) ] return courses, in_process_course_actions @@ -348,7 +348,7 @@ def _accessible_libraries_list(user): List all libraries available to the logged in user by iterating through all libraries """ # No need to worry about ErrorDescriptors - split's get_libraries() never returns them. - return [lib for lib in modulestore().get_libraries() if has_course_author_access(user, lib.location)] + return [lib for lib in modulestore().get_libraries() if has_studio_read_access(user, lib.location.library_key)] @login_required @@ -418,6 +418,7 @@ def format_library_for_view(library): 'url': reverse_library_url('library_handler', unicode(library.location.library_key)), 'org': library.display_org_with_default, 'number': library.display_number_with_default, + 'can_edit': has_studio_write_access(request.user, library.location.library_key), } # remove any courses in courses that are also in the in_process_course_actions list @@ -647,7 +648,7 @@ def _rerun_course(request, org, number, run, fields): source_course_key = CourseKey.from_string(request.json.get('source_course_key')) # verify user has access to the original course - if not has_course_author_access(request.user, source_course_key): + if not has_studio_write_access(request.user, source_course_key): raise PermissionDenied() # create destination course key @@ -728,7 +729,7 @@ def course_info_update_handler(request, course_key_string, provided_id=None): provided_id = None # check that logged in user has permissions to this item (GET shouldn't require this level?) - if not has_course_author_access(request.user, usage_key.course_key): + if not has_studio_write_access(request.user, usage_key.course_key): raise PermissionDenied() if request.method == 'GET': diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index fa1135e15f9c..748b811b13b5 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -37,7 +37,7 @@ from util.json_request import expect_json, JsonResponse -from student.auth import has_course_author_access +from student.auth import has_studio_write_access, has_studio_read_access from contentstore.utils import find_release_date_source, find_staff_lock_source, is_currently_visible_to_students, \ ancestor_has_staff_lock from contentstore.views.helpers import is_unit, xblock_studio_url, xblock_primary_child_category, \ @@ -130,7 +130,8 @@ def xblock_handler(request, usage_key_string): if usage_key_string: usage_key = usage_key_with_run(usage_key_string) - if not has_course_author_access(request.user, usage_key.course_key): + access_check = has_studio_read_access if request.method == 'GET' else has_studio_write_access + if not access_check(request.user, usage_key.course_key): raise PermissionDenied() if request.method == 'GET': @@ -166,6 +167,11 @@ def xblock_handler(request, usage_key_string): parent_usage_key = usage_key_with_run(request.json['parent_locator']) duplicate_source_usage_key = usage_key_with_run(request.json['duplicate_source_locator']) + source_course = duplicate_source_usage_key.course_key + dest_course = parent_usage_key.course_key + if not has_studio_write_access(request.user, dest_course) or not has_studio_read_access(request.user, source_course): + raise PermissionDenied() + dest_usage_key = _duplicate_item( parent_usage_key, duplicate_source_usage_key, @@ -197,7 +203,7 @@ def xblock_view_handler(request, usage_key_string, view_name): the second is the resource description """ usage_key = usage_key_with_run(usage_key_string) - if not has_course_author_access(request.user, usage_key.course_key): + if not has_studio_read_access(request.user, usage_key.course_key): raise PermissionDenied() accept_header = request.META.get('HTTP_ACCEPT', 'application/json') @@ -304,7 +310,7 @@ def xblock_outline_handler(request, usage_key_string): a course. """ usage_key = usage_key_with_run(usage_key_string) - if not has_course_author_access(request.user, usage_key.course_key): + if not has_studio_read_access(request.user, usage_key.course_key): raise PermissionDenied() response_format = request.REQUEST.get('format', 'html') @@ -474,13 +480,12 @@ def _save_xblock(user, xblock, data=None, children_strings=None, metadata=None, def _create_item(request): """View for create items.""" usage_key = usage_key_with_run(request.json['parent_locator']) - category = request.json['category'] + if not has_studio_write_access(request.user, usage_key.course_key): + raise PermissionDenied() + category = request.json['category'] display_name = request.json.get('display_name') - if not has_course_author_access(request.user, usage_key.course_key): - raise PermissionDenied() - if isinstance(usage_key, LibraryUsageLocator): # Only these categories are supported at this time. if category not in ['html', 'problem', 'video']: @@ -627,7 +632,7 @@ def orphan_handler(request, course_key_string): """ course_usage_key = CourseKey.from_string(course_key_string) if request.method == 'GET': - if has_course_author_access(request.user, course_usage_key): + if has_studio_read_access(request.user, course_usage_key): return JsonResponse([unicode(item) for item in modulestore().get_orphans(course_usage_key)]) else: raise PermissionDenied() diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 1fdc8381a8f4..afebda11f77b 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -9,7 +9,7 @@ import logging from contentstore.views.item import create_xblock_info -from contentstore.utils import reverse_library_url +from contentstore.utils import reverse_library_url, add_instructor from django.http import HttpResponseNotAllowed, Http404 from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied @@ -26,7 +26,7 @@ from xmodule.modulestore.django import modulestore from .component import get_component_templates, CONTAINER_TEMPATES -from student.auth import has_course_author_access +from student.auth import has_studio_write_access, has_studio_read_access from student.roles import CourseCreatorRole from student import auth from util.json_request import expect_json, JsonResponse, JsonResponseBadRequest @@ -70,7 +70,7 @@ def _display_library(library_key_string, request): if not isinstance(library_key, LibraryLocator): log.exception("Non-library key passed to content libraries API.") # Should never happen due to url regex raise Http404 # This is not a library - if not has_course_author_access(request.user, library_key): + if not has_studio_read_access(request.user, library_key): log.exception(u"User %s tried to access library %s without permission", request.user.username, unicode(library_key)) raise PermissionDenied() @@ -83,7 +83,7 @@ def _display_library(library_key_string, request): if request.REQUEST.get('format', 'html') == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'text/html'): response_format = 'json' - return library_blocks_view(library, response_format) + return library_blocks_view(library, request.user, response_format) def _list_libraries(request): @@ -96,7 +96,7 @@ def _list_libraries(request): "library_key": unicode(lib.location.library_key), } for lib in modulestore().get_libraries() - if has_course_author_access(request.user, lib.location.library_key) + if has_studio_read_access(request.user, lib.location.library_key) ] return JsonResponse(lib_info) @@ -124,6 +124,8 @@ def _create_library(request): user_id=request.user.id, fields={"display_name": display_name}, ) + # Give the user admin ("Instructor") role for this library: + add_instructor(new_lib.location.library_key, request.user, request.user) except KeyError as error: log.exception("Unable to create library - missing required JSON key.") return JsonResponseBadRequest({ @@ -151,13 +153,15 @@ def _create_library(request): }) -def library_blocks_view(library, response_format): +def library_blocks_view(library, user, response_format): """ The main view of a course's content library. Shows all the XBlocks in the library, and allows adding/editing/deleting them. Can be called with response_format="json" to get a JSON-formatted list of the XBlocks in the library along with library metadata. + + Assumes that read permissions have been checked before calling this. """ assert isinstance(library.location.library_key, LibraryLocator) assert isinstance(library.location, LibraryUsageLocator) @@ -168,16 +172,19 @@ def library_blocks_view(library, response_format): prev_version = library.runtime.course_entry.structure['previous_version'] return JsonResponse({ "display_name": library.display_name, - "library_id": unicode(library.course_id), + "library_id": unicode(library.location.library_key), "version": unicode(library.runtime.course_entry.course_key.version), "previous_version": unicode(prev_version) if prev_version else None, "blocks": [unicode(x) for x in children], }) + can_edit = has_studio_write_access(user, library.location.library_key) + xblock_info = create_xblock_info(library, include_ancestor_info=False, graders=[]) - component_templates = get_component_templates(library, library=True) + component_templates = get_component_templates(library, library=True) if can_edit else [] return render_to_response('library.html', { + 'can_edit': can_edit, 'context_library': library, 'component_templates': json.dumps(component_templates), 'xblock_info': xblock_info, diff --git a/cms/static/js/factories/library.js b/cms/static/js/factories/library.js index 76ac47413ddc..59f447dead35 100644 --- a/cms/static/js/factories/library.js +++ b/cms/static/js/factories/library.js @@ -11,7 +11,8 @@ function($, _, XBlockInfo, PagedContainerPage, LibraryContainerView, ComponentTe model: new XBlockInfo(XBlockInfoJson, {parse: true}), templates: new ComponentTemplates(componentTemplates, {parse: true}), action: 'view', - viewClass: LibraryContainerView + viewClass: LibraryContainerView, + canEdit: true }; xmoduleLoader.done(function () { diff --git a/cms/static/js/views/paged_container.js b/cms/static/js/views/paged_container.js index a8cd7aec3242..4b300843b50a 100644 --- a/cms/static/js/views/paged_container.js +++ b/cms/static/js/views/paged_container.js @@ -52,7 +52,8 @@ define(["jquery", "underscore", "js/views/container", "js/utils/module", "gettex success: function(fragment) { self.handleXBlockFragment(fragment, options); self.processPaging({ requested_page: options.page_number }); - self.page.renderAddXBlockComponents() + self.page.renderAddXBlockComponents(); + self.page.updateBlockActions(); } }); }, diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 406e6e9b0354..018b03fe8fcc 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -20,7 +20,8 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views }, options: { - collapsedClass: 'is-collapsed' + collapsedClass: 'is-collapsed', + canEdit: true // If not specified, assume user has permission to make changes }, view: 'container_preview', @@ -113,9 +114,8 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views // Notify the runtime that the page has been successfully shown xblockView.notifyRuntime('page-shown', self); - // Render the add buttons. Paged containers should do this on their own. if (self.components_on_init) { - // Render the add buttons + // Render the add buttons. Paged containers should do this on their own. self.renderAddXBlockComponents(); } @@ -140,20 +140,31 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views onXBlockRefresh: function(xblockView, block_added) { this.xblockView.refresh(block_added); + this.updateBlockActions(); // Update publish and last modified information from the server. this.model.fetch(); }, renderAddXBlockComponents: function() { var self = this; - this.$('.add-xblock-component').each(function(index, element) { - var component = new AddXBlockComponent({ - el: element, - createComponent: _.bind(self.createComponent, self), - collection: self.options.templates + if (self.options.canEdit) { + this.$('.add-xblock-component').each(function(index, element) { + var component = new AddXBlockComponent({ + el: element, + createComponent: _.bind(self.createComponent, self), + collection: self.options.templates + }); + component.render(); }); - component.render(); - }); + } else { + this.$('.add-xblock-component').remove(); + } + }, + + updateBlockActions: function() { + if (!this.options.canEdit) { + this.xblockView.$el.find('.action-duplicate, .action-delete, .action-drag').remove(); + } }, editXBlock: function(event) { diff --git a/cms/static/sass/views/_dashboard.scss b/cms/static/sass/views/_dashboard.scss index 1a1430f0127e..63f55a9c097e 100644 --- a/cms/static/sass/views/_dashboard.scss +++ b/cms/static/sass/views/_dashboard.scss @@ -495,26 +495,21 @@ .metadata-item { display: inline-block; - &:after { + & + .metadata-item:before { content: "/"; margin-left: ($baseline/10); margin-right: ($baseline/10); color: $gray-l4; } - &:last-child { - - &:after { - content: ""; - margin-left: 0; - margin-right: 0; - } - } - .label { @extend %cont-text-sr; } } + + .extra-metadata { + margin-left: ($baseline/10); + } } .course-actions { diff --git a/cms/templates/container.html b/cms/templates/container.html index e36914506b1b..9a6ce236a1c8 100644 --- a/cms/templates/container.html +++ b/cms/templates/container.html @@ -33,7 +33,8 @@ ${component_templates | n}, ${json.dumps(xblock_info) | n}, "${action}", { - isUnitPage: ${json.dumps(is_unit_page)} + isUnitPage: ${json.dumps(is_unit_page)}, + canEdit: true } ); }); diff --git a/cms/templates/index.html b/cms/templates/index.html index 56f2d5b7ee00..cd2f8978aecb 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -456,6 +456,9 @@

${library_info['display_name']}

${_("Course Number:")} ${library_info['number']} + % if not library_info["can_edit"]: + ${_("(Read-only)")} + % endif diff --git a/cms/templates/library.html b/cms/templates/library.html index 5d06317f04cb..119900f87304 100644 --- a/cms/templates/library.html +++ b/cms/templates/library.html @@ -22,10 +22,12 @@ <%block name="requirejs"> require(["js/factories/library"], function(LibraryFactory) { LibraryFactory( - ${component_templates | n}, ${json.dumps(xblock_info) | n}, + ${component_templates | n}, + ${json.dumps(xblock_info) | n}, { isUnitPage: false, - page_size: 10 + page_size: 10, + canEdit: ${"true" if can_edit else "false"} } ); }); @@ -65,10 +67,12 @@
${_("Library ID")}

+ % if can_edit:

${_("Adding content components")}

${_("You can add components to the library. Help text here.")}

+ % endif
${_("Learn more about content libraries")}
diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 1a0caac94868..d272249b3df5 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -6,9 +6,10 @@ """ from django.core.exceptions import PermissionDenied from django.conf import settings +from opaque_keys.edx.locator import LibraryLocator from student.roles import GlobalStaff, CourseCreatorRole, CourseStaffRole, CourseInstructorRole, CourseRole, \ - CourseBetaTesterRole, OrgInstructorRole, OrgStaffRole + CourseBetaTesterRole, OrgInstructorRole, OrgStaffRole, LibraryUserRole, OrgLibraryUserRole def has_access(user, role): @@ -40,9 +41,9 @@ def has_access(user, role): return False -def has_course_author_access(user, course_key, role=CourseStaffRole): +def has_studio_write_access(user, course_key, role=CourseStaffRole): """ - Return True if user has studio (write) access to the given course. + Return True if user has studio write access to the given course. Note that the CMS permissions model is with respect to courses. There is a super-admin permissions if user.is_staff is set. Also, since we're unifying the user database between LMS and CAS, @@ -64,6 +65,30 @@ def has_course_author_access(user, course_key, role=CourseStaffRole): return has_access(user, role(course_key.for_branch(None))) +def has_course_author_access(*args, **kwargs): + """ + Old name for has_studio_author_access + """ + return has_studio_read_access(*args, **kwargs) + + +def has_studio_read_access(user, course_key): + """ + Return True iff user is allowed to view this course/library in studio. + Will also return True if user has write access in studio (has_course_author_access) + + 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 + + def add_users(caller, role, *users): """ The caller requests adding the given users to the role. Checks that the caller diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 5165966acbe1..a0eb5856c4d0 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -219,6 +219,17 @@ def __init__(self, *args, **kwargs): super(CourseBetaTesterRole, self).__init__(self.ROLE, *args, **kwargs) +class LibraryUserRole(CourseRole): + """ + A user who can view a library and import content from it, but not edit it. + Used in Studio only. + """ + ROLE = 'library_user' + + def __init__(self, *args, **kwargs): + super(LibraryUserRole, self).__init__(self.ROLE, *args, **kwargs) + + class OrgStaffRole(OrgRole): """An organization staff member""" def __init__(self, *args, **kwargs): @@ -231,6 +242,17 @@ def __init__(self, *args, **kwargs): super(OrgInstructorRole, self).__init__('instructor', *args, **kwargs) +class OrgLibraryUserRole(OrgRole): + """ + A user who can view any libraries in an org and import content from them, but not edit them. + Used in Studio only. + """ + ROLE = LibraryUserRole.ROLE + + def __init__(self, *args, **kwargs): + super(OrgLibraryUserRole, self).__init__(self.ROLE, *args, **kwargs) + + class CourseCreatorRole(RoleBase): """ This is the group of people who have permission to create new courses (we may want to eventually From 07cbc7b9268c89c78185bbff8a637b499a4a1c83 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 6 Nov 2014 23:34:37 -0800 Subject: [PATCH 2/9] Unit tests for content library permissions --- .../contentstore/tests/test_libraries.py | 260 +++++++++++++++++- 1 file changed, 255 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 8b4f48b1fc5a..5828c223f3b5 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -2,10 +2,16 @@ Content library unit tests that require the CMS runtime. """ from contentstore.tests.utils import AjaxEnabledTestClient, parse_json -from contentstore.utils import reverse_usage_url +from contentstore.utils import reverse_url, reverse_usage_url, reverse_library_url from contentstore.views.preview import _load_preview_module from contentstore.views.tests.test_library import LIBRARY_REST_URL import ddt +from mock import patch +from student.auth import has_studio_read_access, has_studio_write_access +from student.roles import ( + CourseInstructorRole, CourseStaffRole, CourseCreatorRole, LibraryUserRole, + OrgStaffRole, OrgInstructorRole, OrgLibraryUserRole, +) from xmodule.library_content_module import LibraryVersionReference from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore @@ -15,13 +21,12 @@ from opaque_keys.edx.locator import CourseKey, LibraryLocator -@ddt.ddt -class TestLibraries(ModuleStoreTestCase): +class LibraryTestCase(ModuleStoreTestCase): """ - High-level tests for libraries + Common functionality for content libraries tests """ def setUp(self): - user_password = super(TestLibraries, self).setUp() + user_password = super(LibraryTestCase, self).setUp() self.client = AjaxEnabledTestClient() self.client.login(username=self.user.username, password=user_password) @@ -98,6 +103,20 @@ def _update_item(self, usage_key, metadata): } ) + def _list_libraries(self): + """ + Use the REST API to get a list of libraries visible to the current user. + """ + response = self.client.get_json(LIBRARY_REST_URL) + self.assertEqual(response.status_code, 200) + return parse_json(response) + + +@ddt.ddt +class TestLibraries(LibraryTestCase): + """ + High-level tests for libraries + """ @ddt.data( (2, 1, 1), (2, 2, 2), @@ -306,3 +325,234 @@ def test_change_after_first_sync(self): self.assertEqual(len(lc_block.children), 1) # Children should not be deleted due to a bad setting. html_block = modulestore().get_item(lc_block.children[0]) self.assertEqual(html_block.data, data_value) + + +@ddt.ddt +class TestLibraryAccess(LibraryTestCase): + """ + Test Roles and Permissions related to Content Libraries + """ + def setUp(self): + """ Create a library, staff user, and non-staff user """ + super(TestLibraryAccess, self).setUp() + self.non_staff_user, self.non_staff_user_password = self.create_non_staff_user() + + def _login_as_non_staff_user(self, logout_first=True): + """ Login as a user that starts out with no roles/permissions granted. """ + if logout_first: + self.client.logout() # We start logged in as a staff user + self.client.login(username=self.non_staff_user.username, password=self.non_staff_user_password) + + def _assert_cannot_create_library(self, org="org", library="libfail", expected_code=403): + """ Ensure the current user is not able to create a library. """ + self.assertTrue(expected_code >= 300) + response = self.client.ajax_post(LIBRARY_REST_URL, {'org': org, 'library': library, 'display_name': "Irrelevant"}) + self.assertEqual(response.status_code, expected_code) + key = LibraryLocator(org=org, library=library) + self.assertEqual(modulestore().get_library(key), None) + + def _can_access_library(self, library): + """ + Use the normal studio library URL to check if we have access + + `library` can be a LibraryLocator or the library's root XBlock + """ + if isinstance(library, (basestring, LibraryLocator)): + lib_key = library + else: + lib_key = library.location.library_key + response = self.client.get(reverse_library_url('library_handler', unicode(lib_key))) + self.assertIn(response.status_code, (200, 302, 403)) + return response.status_code == 200 + + def tearDown(self): + """ + Log out when done each test + """ + self.client.logout() + super(TestLibraryAccess, self).tearDown() + + def test_creation(self): + """ + The user that creates a library should have instructor (admin) and staff permissions + """ + # self.library has been auto-created by the staff user. + self.assertTrue(has_studio_write_access(self.user, self.lib_key)) + self.assertTrue(has_studio_read_access(self.user, self.lib_key)) + # Make sure the user was actually assigned the instructor role and not just using is_staff superpowers: + self.assertTrue(CourseInstructorRole(self.lib_key).has_user(self.user)) + + # Now log out and ensure we are forbidden from creating a library: + self.client.logout() + self._assert_cannot_create_library(expected_code=302) # 302 redirect to login expected + + # Now create a non-staff user with no permissions: + self._login_as_non_staff_user(logout_first=False) + self.assertFalse(CourseCreatorRole().has_user(self.non_staff_user)) + + # Now check that logged-in users without any permissions cannot create libraries + with patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}): + self._assert_cannot_create_library() + + @ddt.data( + CourseInstructorRole, + CourseStaffRole, + LibraryUserRole, + ) + def test_acccess(self, access_role): + """ + Test the various roles that allow viewing libraries are working correctly. + """ + # At this point, one library exists, created by the currently-logged-in staff user. + # Create another library as staff: + library2_key = self._create_library(library="lib2") + # Login as non_staff_user: + self._login_as_non_staff_user() + + # non_staff_user shouldn't be able to access any libraries: + lib_list = self._list_libraries() + self.assertEqual(len(lib_list), 0) + self.assertFalse(self._can_access_library(self.library)) + self.assertFalse(self._can_access_library(library2_key)) + + # Now manually intervene to give non_staff_user access to library2_key: + access_role(library2_key).add_users(self.non_staff_user) + + # Now non_staff_user should be able to access library2_key only: + lib_list = self._list_libraries() + self.assertEqual(len(lib_list), 1) + self.assertEqual(lib_list[0]["library_key"], unicode(library2_key)) + self.assertTrue(self._can_access_library(library2_key)) + self.assertFalse(self._can_access_library(self.library)) + + @ddt.data( + OrgStaffRole, + OrgInstructorRole, + OrgLibraryUserRole, + ) + def test_org_based_access(self, org_access_role): + """ + Test the various roles that allow viewing all of an organization's + libraries are working correctly. + """ + # Create some libraries as the staff user: + lib_key_pacific = self._create_library(org="PacificX", library="libP") + lib_key_atlantic = self._create_library(org="AtlanticX", library="libA") + + # Login as a non-staff: + self._login_as_non_staff_user() + + # Now manually intervene to give non_staff_user access to all "PacificX" libraries: + org_access_role(lib_key_pacific.org).add_users(self.non_staff_user) + + # Now non_staff_user should be able to access lib_key_pacific only: + lib_list = self._list_libraries() + self.assertEqual(len(lib_list), 1) + self.assertEqual(lib_list[0]["library_key"], unicode(lib_key_pacific)) + self.assertTrue(self._can_access_library(lib_key_pacific)) + self.assertFalse(self._can_access_library(lib_key_atlantic)) + self.assertFalse(self._can_access_library(self.lib_key)) + + @ddt.data(True, False) + def test_read_only_role(self, use_org_level_role): + """ + Test the read-only role (LibraryUserRole and its org-level equivalent) + """ + # As staff user, add a block to self.library: + block = ItemFactory.create(category="html", parent_location=self.library.location, user_id=self.user.id, publish_item=False) + + # Login as a non_staff_user: + self._login_as_non_staff_user() + self.assertFalse(self._can_access_library(self.library)) + + block_url = reverse_usage_url('xblock_handler', block.location) + + def can_read_block(): + """ Check if studio lets us view the XBlock in the library """ + response = self.client.get_json(block_url) + self.assertIn(response.status_code, (200, 403)) # 400 would be ambiguous + return response.status_code == 200 + + def can_edit_block(): + """ Check if studio lets us edit the XBlock in the library """ + response = self.client.ajax_post(block_url) + self.assertIn(response.status_code, (200, 403)) # 400 would be ambiguous + return response.status_code == 200 + + def can_delete_block(): + """ Check if studio lets us delete the XBlock in the library """ + response = self.client.delete(block_url) + self.assertIn(response.status_code, (200, 403)) # 400 would be ambiguous + return response.status_code == 200 + + def can_copy_block(): + """ Check if studio lets us duplicate the XBlock in the library """ + response = self.client.ajax_post(reverse_url('xblock_handler'), { + 'parent_locator': unicode(self.library.location), + 'duplicate_source_locator': unicode(block.location), + }) + self.assertIn(response.status_code, (200, 403)) # 400 would be ambiguous + return response.status_code == 200 + + def can_create_block(): + """ Check if studio lets us make a new XBlock in the library """ + response = self.client.ajax_post(reverse_url('xblock_handler'), { + 'parent_locator': unicode(self.library.location), 'category': 'html', + }) + self.assertIn(response.status_code, (200, 403)) # 400 would be ambiguous + return response.status_code == 200 + + # Check that we do not have read or write access to block: + self.assertFalse(can_read_block()) + self.assertFalse(can_edit_block()) + self.assertFalse(can_delete_block()) + self.assertFalse(can_copy_block()) + self.assertFalse(can_create_block()) + + # Give non_staff_user read-only permission: + if use_org_level_role: + OrgLibraryUserRole(self.lib_key.org).add_users(self.non_staff_user) + else: + LibraryUserRole(self.lib_key).add_users(self.non_staff_user) + + self.assertTrue(self._can_access_library(self.library)) + self.assertTrue(can_read_block()) + self.assertFalse(can_edit_block()) + self.assertFalse(can_delete_block()) + self.assertFalse(can_copy_block()) + self.assertFalse(can_create_block()) + + @ddt.data( + (LibraryUserRole, CourseStaffRole, True), + (CourseStaffRole, CourseStaffRole, True), + (None, CourseStaffRole, False), + (LibraryUserRole, None, False), + ) + @ddt.unpack + def test_duplicate_across_courses(self, library_role, course_role, expected_result): + """ + Test that the REST API will correctly allow/refuse when copying + from a library with (write, read, or no) access to a course with (write or no) access. + """ + # As staff user, add a block to self.library: + block = ItemFactory.create(category="html", parent_location=self.library.location, user_id=self.user.id, publish_item=False) + # And create a course: + with modulestore().default_store(ModuleStoreEnum.Type.split): + course = CourseFactory.create() + + self._login_as_non_staff_user() + + # Assign roles: + if library_role: + library_role(self.lib_key).add_users(self.non_staff_user) + if course_role: + course_role(course.location.course_key).add_users(self.non_staff_user) + + # Copy block to the course: + response = self.client.ajax_post(reverse_url('xblock_handler'), { + 'parent_locator': unicode(course.location), + 'duplicate_source_locator': unicode(block.location), + }) + self.assertIn(response.status_code, (200, 403)) # 400 would be ambiguous + duplicate_action_allowed = (response.status_code == 200) + self.assertEqual(duplicate_action_allowed, expected_result) From e0e6f25379602c0e5322d9cb576e81f24c59672f Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Nov 2014 19:38:39 -0800 Subject: [PATCH 3/9] 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 | 134 ++++++++---------- cms/urls.py | 8 +- common/djangoapps/student/auth.py | 61 +++++--- 5 files changed, 116 insertions(+), 98 deletions(-) diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index afebda11f77b..3957765a38da 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 f4fcc609d346..ce99a266ef8d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_user.py +++ b/cms/djangoapps/contentstore/views/tests/test_user.py @@ -70,7 +70,7 @@ def test_detail_invalid(self): 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 @@ def test_permission_denied_self(self): 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 @@ def test_permission_denied_other(self): 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 @@ def test_staff_cannot_delete_other(self): 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 c73ecc651ff9..812890a451dc 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -9,11 +9,12 @@ 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) - - auth.remove_users(request.user, CourseStaffRole(course_key), user) - return JsonResponse() - - # 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 role == "instructor": - if not has_course_author_access(request.user, course_key, role=CourseInstructorRole): - msg = { - "error": _("Only instructors may create other instructors") - } + 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) + + 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) + + if new_role and not role_added: + return JsonResponse({"error": _("Invalid `role` specified.")}, 400) + + 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) - - # auto-enroll the course creator in the course so that "View Live" will work. + auth.remove_users(request.user, role, user) + + 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 c7394c536f98..b4dd931482ab 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -5,6 +5,11 @@ 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 @@ 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), @@ -112,7 +117,6 @@ ) 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 d272249b3df5..90b311e77e86 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -12,6 +12,14 @@ 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): From adf7bf4a5c204cfb433bd4a4f088489e2c01a8e4 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 12 Nov 2014 14:05:36 -0800 Subject: [PATCH 4/9] Basic UI for managing library users with limited editing functionality (shared with course code) --- cms/djangoapps/contentstore/views/library.py | 40 +++- .../contentstore/views/tests/test_library.py | 26 +++ cms/static/js/factories/manage_users_lib.js | 159 +++++++++++++++ cms/static/sass/views/_users.scss | 62 +++--- cms/templates/manage_users_lib.html | 181 ++++++++++++++++++ cms/templates/widgets/header.html | 34 +++- cms/urls.py | 2 + 7 files changed, 469 insertions(+), 35 deletions(-) create mode 100644 cms/static/js/factories/manage_users_lib.js create mode 100644 cms/templates/manage_users_lib.html diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 3957765a38da..c599cc1559f9 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -26,12 +26,12 @@ from xmodule.modulestore.django import modulestore from .component import get_component_templates, CONTAINER_TEMPATES -from student.auth import has_studio_write_access, has_studio_read_access -from student.roles import CourseCreatorRole +from student.auth import STUDIO_VIEW_USERS, STUDIO_EDIT_ROLES, get_user_permissions, has_studio_read_access, has_studio_write_access +from student.roles import CourseCreatorRole, CourseInstructorRole, CourseStaffRole, LibraryUserRole from student import auth from util.json_request import expect_json, JsonResponse, JsonResponseBadRequest -__all__ = ['library_handler'] +__all__ = ['library_handler', 'manage_library_users'] log = logging.getLogger(__name__) @@ -191,3 +191,37 @@ def library_blocks_view(library, user, response_format): 'templates': CONTAINER_TEMPATES, 'lib_users_url': reverse_library_url('manage_library_users', unicode(library.location.library_key)), }) + + +def manage_library_users(request, library_key_string): + """ + Studio UI for editing the users within a library. + + Uses the /course_team/:library_key/:user_email/ REST API to make changes. + """ + library_key = CourseKey.from_string(library_key_string) + if not isinstance(library_key, LibraryLocator): + raise Http404 # This is not a library + user_perms = get_user_permissions(request.user, library_key) + if not (user_perms & STUDIO_VIEW_USERS): + raise PermissionDenied() + library = modulestore().get_library(library_key) + if library is None: + raise Http404 + + # Segment all the users explicitly associated with this library, ensuring each user only has one role listed: + instructors = set(CourseInstructorRole(library_key).users_with_role()) + staff = set(CourseStaffRole(library_key).users_with_role()) - instructors + users = set(LibraryUserRole(library_key).users_with_role()) - instructors - staff + all_users = instructors | staff | users + + return render_to_response('manage_users_lib.html', { + 'context_library': library, + 'staff': staff, + 'instructors': instructors, + 'users': users, + 'all_users': all_users, + 'allow_actions': bool(user_perms & STUDIO_EDIT_ROLES), + 'library_key': unicode(library_key), + 'lib_users_url': reverse_library_url('manage_library_users', library_key_string), + }) diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index 9ab5bc06ccf0..0b0abe2e3b29 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -4,12 +4,14 @@ More important high-level tests are in contentstore/tests/test_libraries.py """ from contentstore.tests.utils import AjaxEnabledTestClient, parse_json +from contentstore.utils import reverse_course_url, reverse_library_url from contentstore.views.component import get_component_templates from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import LibraryFactory from mock import patch from opaque_keys.edx.locator import CourseKey, LibraryLocator import ddt +from student.roles import LibraryUserRole LIBRARY_REST_URL = '/library/' # URL for GET/POST requests involving libraries @@ -197,3 +199,27 @@ def test_get_component_templates(self): self.assertIn('problem', templates) self.assertNotIn('discussion', templates) self.assertNotIn('advanced', templates) + + def test_manage_library_users(self): + """ + Simple test that the Library "User Access" view works. + Also tests that we can use the REST API to assign a user to a library. + """ + library = LibraryFactory.create() + extra_user, _ = self.create_non_staff_user() + manage_users_url = reverse_library_url('manage_library_users', unicode(library.location.library_key)) + + response = self.client.get(manage_users_url) + self.assertEqual(response.status_code, 200) + # extra_user has not been assigned to the library so should not show up in the list: + self.assertNotIn(extra_user.username, response.content) + + # Now add extra_user to the library: + user_details_url = reverse_course_url('course_team_handler', library.location.library_key, kwargs={'email': extra_user.email}) + edit_response = self.client.ajax_post(user_details_url, {"role": LibraryUserRole.ROLE}) + self.assertIn(edit_response.status_code, (200, 204)) + + # Now extra_user should apear in the list: + response = self.client.get(manage_users_url) + self.assertEqual(response.status_code, 200) + self.assertIn(extra_user.username, response.content) diff --git a/cms/static/js/factories/manage_users_lib.js b/cms/static/js/factories/manage_users_lib.js new file mode 100644 index 000000000000..bf2fdcd6f745 --- /dev/null +++ b/cms/static/js/factories/manage_users_lib.js @@ -0,0 +1,159 @@ +/* + Code for editing users and assigning roles within a library context. +*/ +define(['jquery', 'underscore', 'gettext', 'js/views/feedback_prompt', 'js/views/utils/view_utils'], +function($, _, gettext, PromptView, ViewUtils) { + 'use strict'; + return function (libraryName, allUserEmails, tplUserURL) { + var unknownErrorMessage = gettext('Unknown'), + $createUserForm = $('#create-user-form'), + $createUserFormWrapper = $createUserForm.closest('.wrapper-create-user'), + $cancelButton; + + // Our helper method that calls the RESTful API to add/remove/change user roles: + var changeRole = function(email, newRole, opts) { + var url = tplUserURL.replace('@@EMAIL@@', email); + var errMessage = opts.errMessage || gettext("There was an error changing the user's role"); + var onSuccess = opts.onSuccess || function(data){ ViewUtils.reload(); }; + var onError = opts.onError || function(){}; + $.ajax({ + url: url, + type: newRole ? 'POST' : 'DELETE', + dataType: 'json', + contentType: 'application/json', + notifyOnError: false, + data: JSON.stringify({role: newRole}), + success: onSuccess, + error: function(jqXHR, textStatus, errorThrown) { + var message, prompt; + try { + message = JSON.parse(jqXHR.responseText).error || unknownErrorMessage; + } catch (e) { + message = unknownErrorMessage; + } + prompt = new PromptView.Error({ + title: errMessage, + message: message, + actions: { + primary: { text: gettext('OK'), click: function(view) { view.hide(); onError(); } } + } + }); + prompt.show(); + } + }); + }; + + $createUserForm.bind('submit', function(event) { + event.preventDefault(); + var email = $('#user-email-input').val().trim(); + var msg; + + if(!email) { + msg = new PromptView.Error({ + title: gettext('A valid email address is required'), + message: gettext('You must enter a valid email address in order to add an instructor'), + actions: { + primary: { + text: gettext('Return and add email address'), + click: function(view) { view.hide(); $('#user-email-input').focus(); } + } + } + }); + msg.show(); + return; + } + + if(_.contains(allUserEmails, email)) { + msg = new PromptView.Warning({ + title: gettext('Already a library team member'), + message: _.template( + gettext("{email} is already on the “{course}” team. If you're trying to add a new member, please double-check the email address you provided."), { + email: email, + course: libraryName + }, {interpolate: /\{(.+?)\}/g} + ), + actions: { + primary: { + text: gettext('Return to team listing'), + click: function(view) { view.hide(); $('#user-email-input').focus(); } + } + } + }); + msg.show(); + return; + } + + // Use the REST API to create the user, giving them a role of "library_user" for now: + changeRole( + $('#user-email-input').val().trim(), + 'library_user', + { + errMessage: gettext('Error adding user'), + onError: function() { $('#user-email-input').focus(); } + } + ); + }); + + $cancelButton = $createUserForm.find('.action-cancel'); + $cancelButton.on('click', function(event) { + event.preventDefault(); + $('.create-user-button').toggleClass('is-disabled'); + $createUserFormWrapper.toggleClass('is-shown'); + $('#user-email-input').val(''); + }); + + $('.create-user-button').on('click', function(event) { + event.preventDefault(); + $('.create-user-button').toggleClass('is-disabled'); + $createUserFormWrapper.toggleClass('is-shown'); + $createUserForm.find('#user-email-input').focus(); + }); + + $('body').on('keyup', function(event) { + if(event.which == jQuery.ui.keyCode.ESCAPE && $createUserFormWrapper.is('.is-shown')) { + $cancelButton.click(); + } + }); + + $('.remove-user').click(function() { + var email = $(this).closest('li[data-email]').data('email'), + msg = new PromptView.Warning({ + title: gettext('Are you sure?'), + message: _.template(gettext('Are you sure you want to delete {email} from the library “{library}”?'), {email: email, library: libraryName}, {interpolate: /\{(.+?)\}/g}), + actions: { + primary: { + text: gettext('Delete'), + click: function(view) { + // User the REST API to delete the user: + changeRole(email, null, { errMessage: gettext('Error removing user') }); + } + }, + secondary: { + text: gettext('Cancel'), + click: function(view) { view.hide(); } + } + } + }); + msg.show(); + }); + + $('.user-actions .make-instructor').click(function(event) { + event.preventDefault(); + var email = $(this).closest('li[data-email]').data('email'); + changeRole(email, 'instructor', {}); + }); + + $('.user-actions .make-staff').click(function(event) { + event.preventDefault(); + var email = $(this).closest('li[data-email]').data('email'); + changeRole(email, 'staff', {}); + }); + + $('.user-actions .make-user').click(function(event) { + event.preventDefault(); + var email = $(this).closest('li[data-email]').data('email'); + changeRole(email, 'library_user', {}); + }); + + }; +}); diff --git a/cms/static/sass/views/_users.scss b/cms/static/sass/views/_users.scss index 11c89dd19635..a729d9416720 100644 --- a/cms/static/sass/views/_users.scss +++ b/cms/static/sass/views/_users.scss @@ -116,11 +116,16 @@ &.flag-role-admin { background: $pink; } + + &.flag-role-user { + background: $yellow-d1; + .msg-you { color: $yellow-l1; } + } } // ELEM: item - metadata .item-metadata { - width: flex-grid(5, 9); + width: flex-grid(4, 9); @include margin-right(flex-gutter()); .user-username, .user-email { @@ -143,7 +148,7 @@ // ELEM: item - actions .item-actions { - width: flex-grid(4, 9); + width: flex-grid(5, 9); position: static; // nasty reset needed due to base.scss text-align: right; @@ -153,12 +158,34 @@ } .action-role { - width: flex-grid(3, 4); + width: flex-grid(7, 8); margin-right: flex-gutter(); + + .add-admin-role { + @include blue-button; + @include transition(all .15s); + @extend %t-action2; + @extend %t-strong; + display: inline-block; + padding: ($baseline/5) $baseline; + } + + .remove-admin-role { + @include grey-button; + @include transition(all .15s); + @extend %t-action2; + @extend %t-strong; + display: inline-block; + padding: ($baseline/5) $baseline; + } + .notoggleforyou { + @extend %t-copy-sub1; + color: $gray-l2; + } } .action-delete { - width: flex-grid(1, 4); + width: flex-grid(1, 8); // STATE: disabled &.is-disabled { @@ -178,33 +205,6 @@ float: none; color: inherit; } - - // ELEM: admin role controls - .toggle-admin-role { - - &.add-admin-role { - @include blue-button; - @include transition(all .15s); - @extend %t-action2; - @extend %t-strong; - display: inline-block; - padding: ($baseline/5) $baseline; - } - - &.remove-admin-role { - @include grey-button; - @include transition(all .15s); - @extend %t-action2; - @extend %t-strong; - display: inline-block; - padding: ($baseline/5) $baseline; - } - } - - .notoggleforyou { - @extend %t-copy-sub1; - color: $gray-l2; - } } // STATE: hover diff --git a/cms/templates/manage_users_lib.html b/cms/templates/manage_users_lib.html new file mode 100644 index 000000000000..f361e01d5071 --- /dev/null +++ b/cms/templates/manage_users_lib.html @@ -0,0 +1,181 @@ +<%! import json %> +<%! from django.utils.translation import ugettext as _ %> +<%! from django.core.urlresolvers import reverse %> +<%inherit file="base.html" /> +<%def name="online_help_token()"><% return "team" %> +<%block name="title">${_("Library User Access")} +<%block name="bodyclass">is-signedin course users view-team + +<%block name="content"> + +
+
+

+ ${_("Settings")} + > ${_("User Access")} +

+ + +
+
+ +
+
+
+ %if allow_actions: +
+
+
+

${_("Grant Access to This Library")}

+ +
+ ${_("New Team Member Information")} + +
    +
  1. + + + ${_("Please provide the email address of the user you'd like to add")} +
    2. +
+
+
+ +
+ + +
+
+
+ %endif + +
    + % for user in all_users: + <% + is_instructor = user in instructors + is_staff = user in staff + role_id = 'admin' if is_instructor else ('staff' if is_staff else 'user') + role_desc = _("Admin") if is_instructor else (_("Staff") if is_staff else _("User")) + %> + +
  1. + + + + ${_("Current Role:")} + + ${role_desc} + % if request.user.id == user.id: + ${_("You!")} + % endif + + + + +
    +

    + ${user.username} + + ${user.email} + +

    +
    + + % if allow_actions: + + % elif request.user.id == user.id: + + % endif + +
    2. + % endfor +
+ + % if allow_actions and len(all_users) == 1: +
+
+

${_('Add More Users to This Library')}

+
+

${_('Adding team members makes content authoring collaborative. Users must be signed up for Studio and have an active account. ')}

+
+
+ + +
+ %endif +
+ + +
+
+ + +<%block name="requirejs"> + require(["js/factories/manage_users_lib"], function(ManageUsersFactory) { + ManageUsersFactory( + "${context_library.display_name_with_default | h}", + ${json.dumps([user.email for user in all_users])}, + "${reverse('contentstore.views.course_team_handler', kwargs={'course_key_string': library_key, 'email': '@@EMAIL@@'})}" + ); + }); + diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index cd509abf6a2a..6233c1aaf3db 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -39,7 +39,7 @@