From ce17c1cdcf37f6a69c93fc5aae6756d2ed9b2997 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Mon, 28 Nov 2016 11:19:35 +0200 Subject: [PATCH] make check of creation of library a true/false for forntend, add security in api call, clean tests --- cms/djangoapps/contentstore/tests/utils.py | 12 ------- cms/djangoapps/contentstore/views/course.py | 24 ++++++-------- cms/djangoapps/contentstore/views/library.py | 9 +++-- .../views/tests/test_course_index.py | 33 +------------------ .../contentstore/views/tests/test_library.py | 32 ++++++++++++++++-- cms/envs/common.py | 1 - cms/templates/index.html | 7 ++-- .../xmodule/modulestore/tests/django_utils.py | 16 --------- 8 files changed, 50 insertions(+), 84 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 55812ab8bb..af5e7b8e52 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -100,18 +100,6 @@ class CourseTestCase(ProceduralCourseTestMixin, ModuleStoreTestCase): nonstaff.is_authenticated = lambda: authenticate return client, nonstaff - def create_staff_authed_user_client(self, authenticate=True): - """ - Create a staff user, log them in (if authenticate=True), and return the client, user to use for testing. - """ - staff, password = self.create_staff_user() - - client = AjaxEnabledTestClient() - if authenticate: - client.login(username=staff.username, password=password) - staff.is_authenticated = lambda: authenticate - return client, staff - def reload_course(self): """ Reloads the course object from the database diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 93b108f4e1..b5895a3bb1 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -519,18 +519,18 @@ def course_listing(request): return render_to_response('index.html', { 'courses': courses, 'in_process_course_actions': in_process_course_actions, + 'libraries_enabled': LIBRARIES_ENABLED, + 'libraries': [format_library_for_view(lib) for lib in libraries], + 'show_new_library_button': _get_library_creator_status(user), 'user': user, 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), 'course_creator_status': _get_course_creator_status(user), 'rerun_creator_status': GlobalStaff().has_user(user), 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), - 'libraries_enabled': LIBRARIES_ENABLED, - 'libraries': [format_library_for_view(lib) for lib in libraries], - 'library_creator_status': _get_library_creator_status(user), 'is_programs_enabled': programs_config.is_studio_tab_enabled and user.is_staff, 'programs': programs, - 'program_authoring_url': reverse('programs') + 'program_authoring_url': reverse('programs'), }) @@ -1655,19 +1655,15 @@ def _get_course_creator_status(user): def _get_library_creator_status(user): """ Helper method for returning the library creation status for a particular user, - taking into account the values of DISABLE_LIBRARY_CREATION and LIBRARIES_ENABLED. + taking into account the value LIBRARIES_ENABLED. """ if not LIBRARIES_ENABLED: - library_creator_status = 'disallowed_for_this_site' + return False elif user.is_staff: - library_creator_status = 'granted' - elif CourseCreatorRole().has_user(user): - library_creator_status = 'granted' - elif settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): - library_creator_status = 'disallowed_for_this_site' + return True + elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + return CourseCreatorRole().has_user(user) else: - library_creator_status = 'disallowed_for_this_site' - - return library_creator_status + return False diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index cb3cd4e18b..8bae2df764 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, add_instructor -from django.http import HttpResponseNotAllowed, Http404 +from django.http import HttpResponseNotAllowed, Http404, HttpResponseForbidden from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django.conf import settings @@ -29,7 +29,7 @@ from .component import get_component_templates, CONTAINER_TEMPLATES 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 CourseInstructorRole, CourseStaffRole, LibraryUserRole +from student.roles import CourseInstructorRole, CourseStaffRole, LibraryUserRole, CourseCreatorRole from util.json_request import expect_json, JsonResponse, JsonResponseBadRequest __all__ = ['library_handler', 'manage_library_users'] @@ -50,6 +50,11 @@ def library_handler(request, library_key_string=None): log.exception("Attempted to use the content library API when the libraries feature is disabled.") raise Http404 # Should never happen because we test the feature in urls.py also + if not CourseCreatorRole().has_user(request.user): + if not request.user.is_staff: + return HttpResponseForbidden() + + if library_key_string is not None and request.method == 'POST': return HttpResponseNotAllowed(("POST",)) diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index e5d5417825..e8756eb896 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -16,7 +16,7 @@ from contentstore.courseware_index import CoursewareSearchIndexer, SearchIndexin from contentstore.tests.utils import CourseTestCase from contentstore.utils import reverse_course_url, reverse_library_url, add_instructor, reverse_usage_url from contentstore.views.course import ( - course_outline_initial_state, reindex_course_and_check_access, _deprecated_blocks_info, _get_library_creator_status + course_outline_initial_state, reindex_course_and_check_access, _deprecated_blocks_info ) from contentstore.views.item import create_xblock_info, VisibilityState from course_action_state.managers import CourseRerunUIStateManager @@ -30,7 +30,6 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory -from student import auth from student.roles import CourseCreatorRole @@ -50,36 +49,6 @@ class TestCourseIndex(CourseTestCase): display_name='dotted.course.name-2', ) - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", False) - def test_library_creator_status_libraries_not_enabled(self): - _, nostaff_user = self.create_non_staff_authed_user_client() - self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") - - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_is_staff_user(self): - _, staff_user = self.create_staff_authed_user_client() - self.assertEqual(_get_library_creator_status(staff_user), "granted") - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_disable_library_creation(self): - _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"DISABLE_LIBRARY_CREATION": True}): - self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_course_creator_role(self): - _, staff_user = self.create_staff_authed_user_client() - _, nostaff_user = self.create_non_staff_authed_user_client() - auth.add_users(staff_user, CourseCreatorRole(), nostaff_user) - self.assertEqual(_get_library_creator_status(nostaff_user), "granted") - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_no_course_creator_role_no_is_staff_no_disable_library_creation(self): - _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"DISABLE_LIBRARY_CREATION": False}): - self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") - def check_index_and_outline(self, authed_client): """ Test getting the list of courses and then pulling up their outlines diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index 2a704c002d..f8c96df9a9 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -5,13 +5,17 @@ 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.tests.utils import CourseTestCase from contentstore.views.component import get_component_templates +from contentstore.views.course import _get_library_creator_status 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 +import mock +from student.auth import add_users +from student.roles import CourseCreatorRole, LibraryUserRole LIBRARY_REST_URL = '/library/' # URL for GET/POST requests involving libraries @@ -24,7 +28,7 @@ def make_url_for_lib(key): @ddt.ddt -class UnitTestLibraries(ModuleStoreTestCase): +class UnitTestLibraries(CourseTestCase): """ Unit tests for library views """ @@ -38,6 +42,28 @@ class UnitTestLibraries(ModuleStoreTestCase): ###################################################### # Tests for /library/ - list and create libraries: + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", False) + def test_library_creator_status_libraries_not_enabled(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + self.assertEqual(_get_library_creator_status(nostaff_user), False) + + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_is_staff_user(self): + self.assertEqual(_get_library_creator_status(self.user), True) + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_course_creator_role(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + add_users(self.user, CourseCreatorRole(), nostaff_user) + self.assertEqual(_get_library_creator_status(nostaff_user), True) + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_no_course_creator_role(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + self.assertEqual(_get_library_creator_status(nostaff_user), False) + @patch("contentstore.views.library.LIBRARIES_ENABLED", False) def test_with_libraries_disabled(self): """ @@ -93,7 +119,7 @@ class UnitTestLibraries(ModuleStoreTestCase): self.client.logout() ns_user, password = self.create_non_staff_user() self.client.login(username=ns_user.username, password=password) - + add_users(self.user, CourseCreatorRole(), ns_user) response = self.client.ajax_post(LIBRARY_REST_URL, { 'org': 'org', 'library': 'lib', 'display_name': "New Library", }) diff --git a/cms/envs/common.py b/cms/envs/common.py index 3c94ee9ed1..11c0842b8d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -168,7 +168,6 @@ FEATURES = { # Enable support for content libraries. Note that content libraries are # only supported in courses using split mongo. 'ENABLE_CONTENT_LIBRARIES': True, - 'DISABLE_LIBRARY_CREATION': False, # Milestones application flag 'MILESTONES_APP': False, diff --git a/cms/templates/index.html b/cms/templates/index.html index ad652dd5e3..8760d5951f 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -33,8 +33,7 @@ from openedx.core.djangolib.markup import HTML, Text % elif course_creator_status=='disallowed_for_this_site' and settings.FEATURES.get('STUDIO_REQUEST_EMAIL',''): ${_("Email staff to create course")} % endif - - % if library_creator_status=='granted': + % if show_new_library_button: ${_("New Library")} % endif @@ -130,7 +129,7 @@ from openedx.core.djangolib.markup import HTML, Text % endif - %if libraries_enabled and library_creator_status=='granted': + %if libraries_enabled and show_new_library_button:
@@ -497,7 +496,7 @@ from openedx.core.djangolib.markup import HTML, Text
- % if library_creator_status=='granted': + % if show_new_library_button:

${_('Create Your First Library')}

diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 4d3a4a02f7..c80317d4d7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -436,22 +436,6 @@ class ModuleStoreTestCase(ModuleStoreIsolationMixin, TestCase): nonstaff_user.save() return nonstaff_user, password - def create_staff_user(self): - """ - Creates a staff test user. - Returns the staff test user and its password. - """ - uname = 'teststaff' - password = 'bar' - staff_user = User.objects.create_user(uname, 'test+staff@edx.org', password) - - # Note that we do not actually need to do anything - # for registration if we directly mark them active. - staff_user.is_active = True - staff_user.is_staff = True - staff_user.save() - return staff_user, password - def update_course(self, course, user_id): """ Updates the version of course in the modulestore