diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 58b63abd23..f27d2fe559 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -1,5 +1,6 @@ from django.contrib.auth.models import User, Group from django.core.exceptions import PermissionDenied +from django.conf import settings from xmodule.modulestore import Location @@ -12,6 +13,9 @@ but this implementation should be data compatible with the LMS implementation INSTRUCTOR_ROLE_NAME = 'instructor' STAFF_ROLE_NAME = 'staff' +# This is the group of people who have permission to create new courses on edge or edx. +COURSE_CREATOR_GROUP_NAME = "course_creator_group" + # we're just making a Django group for each location/role combo # to do this we're just creating a Group name which is a formatted string # of those two variables @@ -36,10 +40,10 @@ def get_users_in_course_group_by_role(location, role): return group.user_set.all() -''' -Create all permission groups for a new course and subscribe the caller into those roles -''' def create_all_course_groups(creator, location): + """ + Create all permission groups for a new course and subscribe the caller into those roles + """ create_new_course_group(creator, location, INSTRUCTOR_ROLE_NAME) create_new_course_group(creator, location, STAFF_ROLE_NAME) @@ -56,10 +60,10 @@ def create_new_course_group(creator, location, role): return def _delete_course_group(location): - ''' + """ This is to be called only by either a command line code path or through a app which has already asserted permissions - ''' + """ # remove all memberships instructors = Group.objects.get(name=get_course_groupname_for_role(location, INSTRUCTOR_ROLE_NAME)) for user in instructors.user_set.all(): @@ -72,10 +76,10 @@ def _delete_course_group(location): user.save() def _copy_course_group(source, dest): - ''' + """ This is to be called only by either a command line code path or through an app which has already asserted permissions to do this action - ''' + """ instructors = Group.objects.get(name=get_course_groupname_for_role(source, INSTRUCTOR_ROLE_NAME)) new_instructors_group = Group.objects.get(name=get_course_groupname_for_role(dest, INSTRUCTOR_ROLE_NAME)) for user in instructors.user_set.all(): @@ -94,10 +98,29 @@ def add_user_to_course_group(caller, user, location, role): if not is_user_in_course_group_role(caller, location, INSTRUCTOR_ROLE_NAME): raise PermissionDenied - if user.is_active and user.is_authenticated: - groupname = get_course_groupname_for_role(location, role) + group = Group.objects.get(name=get_course_groupname_for_role(location, role)) + return _add_user_to_group(user, group) - group = Group.objects.get(name=groupname) + +def add_user_to_creator_group(user): + """ + Adds the user to the group of course creators. + + Note that on the edX site, we currently limit course creators to edX staff, and this + method is a no-op in that environment. + """ + (group, created) = Group.objects.get_or_create(name=COURSE_CREATOR_GROUP_NAME) + if created: + group.save() + return _add_user_to_group(user, group) + + +def _add_user_to_group(user, group): + """ + This is to be called only by either a command line code path or through an app which has already + asserted permissions to do this action + """ + if user.is_active and user.is_authenticated: user.groups.add(group) user.save() return True @@ -123,11 +146,24 @@ def remove_user_from_course_group(caller, user, location, role): # see if the user is actually in that role, if not then we don't have to do anything if is_user_in_course_group_role(user, location, role): - groupname = get_course_groupname_for_role(location, role) + _remove_user_from_group(user, get_course_groupname_for_role(location, role)) - group = Group.objects.get(name=groupname) - user.groups.remove(group) - user.save() + +def remove_user_from_creator_group(user): + """ + Removes user from the course creator group. + """ + _remove_user_from_group(user, COURSE_CREATOR_GROUP_NAME) + + +def _remove_user_from_group(user, group_name): + """ + This is to be called only by either a command line code path or through an app which has already + asserted permissions to do this action + """ + group = Group.objects.get(name=group_name) + user.groups.remove(group) + user.save() def is_user_in_course_group_role(user, location, role): @@ -136,3 +172,26 @@ def is_user_in_course_group_role(user, location, role): return user.is_staff or user.groups.filter(name=get_course_groupname_for_role(location, role)).count() > 0 return False + + +def is_user_in_creator_group(user): + """ + Returns true if the user has permissions to create a course. + + Will always return True if user.is_staff is True. + + Note that on the edX site, we currently limit course creators to edX staff. On + other sites, this method checks that the user is in the course creator group. + """ + if user.is_staff: + return True + + # On edx, we only allow edX staff to create courses. This may be relaxed in the future. + if settings.MITX_FEATURES.get('DISABLE_COURSE_CREATION', False): + return False + + # Feature flag for using the creator group setting. Will be removed once the feature is complete. + if settings.MITX_FEATURES.get('ENABLE_CREATOR_GROUP', False): + return user.groups.filter(name=COURSE_CREATOR_GROUP_NAME).count() > 0 + + return True diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py new file mode 100644 index 0000000000..4e44471ebf --- /dev/null +++ b/cms/djangoapps/auth/tests/test_authz.py @@ -0,0 +1,78 @@ +""" +Tests authz.py +""" +import mock + +from django.test import TestCase +from django.contrib.auth.models import User + +from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group + +class CreatorGroupTest(TestCase): + """ + Tests for the course creator group. + """ + def setUp(self): + """ Test case setup """ + self.user = User.objects.create_user('testuser', 'test+courses@edx.org', 'foo') + + def test_creator_group_not_enabled(self): + """ + Tests that is_user_in_creator_group always returns True if ENABLE_CREATOR_GROUP + and DISABLE_COURSE_CREATION are both not turned on. + """ + self.assertTrue(is_user_in_creator_group(self.user)) + + def test_creator_group_enabled_but_empty(self): + """ Tests creator group feature on, but group empty. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + self.assertFalse(is_user_in_creator_group(self.user)) + + # Make user staff. This will cause is_user_in_creator_group to return True. + self.user.is_staff = True + self.assertTrue(is_user_in_creator_group(self.user)) + + def test_creator_group_enabled_nonempty(self): + """ Tests creator group feature on, user added. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + self.assertTrue(add_user_to_creator_group(self.user)) + self.assertTrue(is_user_in_creator_group(self.user)) + + # check that a user who has not been added to the group still returns false + user_not_added = User.objects.create_user('testuser2', 'test+courses2@edx.org', 'foo2') + self.assertFalse(is_user_in_creator_group(user_not_added)) + + # remove first user from the group and verify that is_user_in_creator_group now returns false + remove_user_from_creator_group(self.user) + self.assertFalse(is_user_in_creator_group(self.user)) + + def test_add_user_not_authenticated(self): + """ + Tests that adding to creator group fails if user is not authenticated + """ + self.user.is_authenticated = False + self.assertFalse(add_user_to_creator_group(self.user)) + + def test_add_user_not_active(self): + """ + Tests that adding to creator group fails if user is not active + """ + self.user.is_active = False + self.assertFalse(add_user_to_creator_group(self.user)) + + def test_course_creation_disabled(self): + """ Tests that the COURSE_CREATION_DISABLED flag overrides course creator group settings. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True, "ENABLE_CREATOR_GROUP" : True}): + # Add user to creator group. + self.assertTrue(add_user_to_creator_group(self.user)) + + # DISABLE_COURSE_CREATION overrides (user is not marked as staff). + self.assertFalse(is_user_in_creator_group(self.user)) + + # Mark as staff. Now is_user_in_creator_group returns true. + self.user.is_staff = True + self.assertTrue(is_user_in_creator_group(self.user)) + + # Remove user from creator group. is_user_in_creator_group still returns true because is_staff=True + remove_user_from_creator_group(self.user) + self.assertTrue(is_user_in_creator_group(self.user)) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 66fead562e..093532c71d 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1,5 +1,6 @@ import json import shutil +import mock from django.test.client import Client from django.test.utils import override_settings from django.conf import settings @@ -16,6 +17,8 @@ from django.dispatch import Signal from contentstore.utils import get_modulestore from contentstore.tests.utils import parse_json +from auth.authz import add_user_to_creator_group + from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -860,6 +863,12 @@ class ContentStoreTest(ModuleStoreTestCase): def test_create_course(self): """Test new course creation - happy path""" + self.assert_created_course() + + def assert_created_course(self): + """ + Checks that the course was created properly. + """ resp = self.client.post(reverse('create_new_course'), self.course_data) self.assertEqual(resp.status_code, 200) data = parse_json(resp) @@ -867,41 +876,71 @@ class ContentStoreTest(ModuleStoreTestCase): def test_create_course_check_forum_seeding(self): """Test new course creation and verify forum seeding """ - resp = self.client.post(reverse('create_new_course'), self.course_data) - self.assertEqual(resp.status_code, 200) - data = parse_json(resp) - self.assertEqual(data['id'], 'i4x://MITx/999/course/Robot_Super_Course') + self.assert_created_course() self.assertTrue(are_permissions_roles_seeded('MITx/999/Robot_Super_Course')) def test_create_course_duplicate_course(self): """Test new course creation - error path""" self.client.post(reverse('create_new_course'), self.course_data) + self.assert_course_creation_failed('There is already a course defined with this name.') + + def assert_course_creation_failed(self, error_message): + """ + Checks that the course did not get created + """ resp = self.client.post(reverse('create_new_course'), self.course_data) - data = parse_json(resp) self.assertEqual(resp.status_code, 200) - self.assertEqual(data['ErrMsg'], 'There is already a course defined with this name.') + data = parse_json(resp) + self.assertEqual(data['ErrMsg'], error_message) def test_create_course_duplicate_number(self): """Test new course creation - error path""" self.client.post(reverse('create_new_course'), self.course_data) self.course_data['display_name'] = 'Robot Super Course Two' - resp = self.client.post(reverse('create_new_course'), self.course_data) - data = parse_json(resp) - - self.assertEqual(resp.status_code, 200) - self.assertEqual(data['ErrMsg'], - 'There is already a course defined with the same organization and course number.') + self.assert_course_creation_failed('There is already a course defined with the same organization and course number.') def test_create_course_with_bad_organization(self): """Test new course creation - error path for bad organization name""" self.course_data['org'] = 'University of California, Berkeley' - resp = self.client.post(reverse('create_new_course'), self.course_data) - data = parse_json(resp) + self.assert_course_creation_failed("Unable to create course 'Robot Super Course'.\n\nInvalid characters in 'University of California, Berkeley'.") - self.assertEqual(resp.status_code, 200) - self.assertEqual(data['ErrMsg'], - "Unable to create course 'Robot Super Course'.\n\nInvalid characters in 'University of California, Berkeley'.") + def test_create_course_with_course_creation_disabled_staff(self): + """Test new course creation -- course creation disabled, but staff access.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True}): + self.assert_created_course() + + def test_create_course_with_course_creation_disabled_not_staff(self): + """Test new course creation -- error path for course creation disabled, not staff access.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True}): + self.user.is_staff = False + self.user.save() + self.assert_course_permission_denied() + + def test_create_course_no_course_creators_staff(self): + """Test new course creation -- course creation group enabled, staff, group is empty.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_CREATOR_GROUP': True}): + self.assert_created_course() + + def test_create_course_no_course_creators_not_staff(self): + """Test new course creation -- error path for course creator group enabled, not staff, group is empty.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + self.user.is_staff = False + self.user.save() + self.assert_course_permission_denied() + + def test_create_course_with_course_creator(self): + """Test new course creation -- use course creator group""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + add_user_to_creator_group(self.user) + self.assert_created_course() + + def assert_course_permission_denied(self): + """ + Checks that the course did not get created due to a PermissionError. + """ + resp = self.client.post(reverse('create_new_course'), self.course_data) + self.assertEqual(resp.status_code, 403) def test_course_index_view_with_no_courses(self): """Test viewing the index page with no courses""" diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index dd7573bad5..8862115c45 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -21,7 +21,7 @@ from contentstore.utils import get_lms_link_for_item, add_extra_panel_tab, remov from models.settings.course_details import CourseDetails, CourseSettingsEncoder from models.settings.course_grading import CourseGradingModel from models.settings.course_metadata import CourseMetadata -from auth.authz import create_all_course_groups +from auth.authz import create_all_course_groups, is_user_in_creator_group from util.json_request import expect_json from .access import has_access, get_location_and_verify_access @@ -81,7 +81,7 @@ def course_index(request, org, course, name): @expect_json def create_new_course(request): - if settings.MITX_FEATURES.get('DISABLE_COURSE_CREATION', False) and not request.user.is_staff: + if not is_user_in_creator_group(request.user): raise PermissionDenied() # This logic is repeated in xmodule/modulestore/tests/factories.py