Merge pull request #266 from edx/christina/course-create-permissions

New course creator group.

cms/djangoapps/auth/authz.py

 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,34 @@ 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)
+
+
+def add_user_to_creator_group(caller, user):
+    """
+    Adds the user to the group of course creators.
+
+    The caller must have staff access to perform this operation.
+
+    Note that on the edX site, we currently limit course creators to edX staff, and this
+    method is a no-op in that environment.
+    """
+    if not caller.is_active or not caller.is_authenticated or not caller.is_staff:
+        raise PermissionDenied
 
-        group = Group.objects.get(name=groupname)
+    (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 +151,29 @@ 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(caller, user):
+    """
+    Removes user from the course creator group.
+
+    The caller must have staff access to perform this operation.
+    """
+    if not caller.is_active or not caller.is_authenticated or not caller.is_staff:
+        raise PermissionDenied
+
+    _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 +182,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

cms/djangoapps/auth/tests/test_authz.py

+"""
+Tests authz.py
+"""
+import mock
+
+from django.test import TestCase
+from django.contrib.auth.models import User
+from django.core.exceptions import PermissionDenied
+
+from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group,\
+    create_all_course_groups, add_user_to_course_group, STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME,\
+    is_user_in_course_group_role, remove_user_from_course_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')
+        self.admin = User.objects.create_user('Mark', 'admin+courses@edx.org', 'foo')
+        self.admin.is_staff = True
+
+    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.admin, 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.admin, 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.admin, 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.admin, 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.admin, 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.admin, self.user)
+            self.assertTrue(is_user_in_creator_group(self.user))
+
+    def test_add_user_to_group_requires_staff_access(self):
+        with self.assertRaises(PermissionDenied):
+            self.admin.is_staff = False
+            add_user_to_creator_group(self.admin, self.user)
+
+        with self.assertRaises(PermissionDenied):
+            add_user_to_creator_group(self.user, self.user)
+
+    def test_add_user_to_group_requires_active(self):
+        with self.assertRaises(PermissionDenied):
+            self.admin.is_active = False
+            add_user_to_creator_group(self.admin, self.user)
+
+    def test_add_user_to_group_requires_authenticated(self):
+        with self.assertRaises(PermissionDenied):
+            self.admin.is_authenticated = False
+            add_user_to_creator_group(self.admin, self.user)
+
+    def test_remove_user_from_group_requires_staff_access(self):
+        with self.assertRaises(PermissionDenied):
+            self.admin.is_staff = False
+            remove_user_from_creator_group(self.admin, self.user)
+
+    def test_remove_user_from_group_requires_active(self):
+        with self.assertRaises(PermissionDenied):
+            self.admin.is_active = False
+            remove_user_from_creator_group(self.admin, self.user)
+
+    def test_remove_user_from_group_requires_authenticated(self):
+        with self.assertRaises(PermissionDenied):
+            self.admin.is_authenticated = False
+            remove_user_from_creator_group(self.admin, self.user)
+
+
+class CourseGroupTest(TestCase):
+    """
+    Tests for instructor and staff groups for a particular course.
+    """
+
+    def setUp(self):
+        """ Test case setup """
+        self.creator = User.objects.create_user('testcreator', 'testcreator+courses@edx.org', 'foo')
+        self.staff = User.objects.create_user('teststaff', 'teststaff+courses@edx.org', 'foo')
+        self.location = 'i4x', 'mitX', '101', 'course', 'test'
+
+    def test_add_user_to_course_group(self):
+        """
+        Tests adding user to course group (happy path).
+        """
+        # Create groups for a new course (and assign instructor role to the creator).
+        self.assertFalse(is_user_in_course_group_role(self.creator, self.location, INSTRUCTOR_ROLE_NAME))
+        create_all_course_groups(self.creator, self.location)
+        self.assertTrue(is_user_in_course_group_role(self.creator, self.location, INSTRUCTOR_ROLE_NAME))
+
+        # Add another user to the staff role.
+        self.assertFalse(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME))
+        self.assertTrue(add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME))
+        self.assertTrue(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME))
+
+    def test_add_user_to_course_group_permission_denied(self):
+        """
+        Verifies PermissionDenied if caller of add_user_to_course_group is not instructor role.
+        """
+        create_all_course_groups(self.creator, self.location)
+        with self.assertRaises(PermissionDenied):
+            add_user_to_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME)
+
+    def test_remove_user_from_course_group(self):
+        """
+        Tests removing user from course group (happy path).
+        """
+        create_all_course_groups(self.creator, self.location)
+
+        self.assertTrue(add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME))
+        self.assertTrue(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME))
+
+        remove_user_from_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
+        self.assertFalse(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME))
+
+        remove_user_from_course_group(self.creator, self.creator, self.location, INSTRUCTOR_ROLE_NAME)
+        self.assertFalse(is_user_in_course_group_role(self.creator, self.location, INSTRUCTOR_ROLE_NAME))
+
+    def test_remove_user_from_course_group_permission_denied(self):
+        """
+        Verifies PermissionDenied if caller of remove_user_from_course_group is not instructor role.
+        """
+        create_all_course_groups(self.creator, self.location)
+        with self.assertRaises(PermissionDenied):
+            remove_user_from_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME)

cms/djangoapps/contentstore/tests/test_contentstore.py

 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
 
@@ -528,7 +531,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.assertGreater(len(all_assets), 0)
 
         # make sure we have some thumbnails in our trashcan
-        all_thumbnails = trash_store.get_all_content_thumbnails_for_course(course_location)
+        _all_thumbnails = trash_store.get_all_content_thumbnails_for_course(course_location)
         #
         # cdodge: temporarily comment out assertion on thumbnails because many environments
         # will not have the jpeg converter installed and this test will fail
@@ -589,20 +592,18 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         location = Location('i4x://MITx/999/chapter/neuvo')
         self.assertRaises(InvalidVersionError, draft_store.clone_item, 'i4x://edx/templates/chapter/Empty',
-            location)
+                          location)
         direct_store.clone_item('i4x://edx/templates/chapter/Empty', location)
-        self.assertRaises(InvalidVersionError, draft_store.clone_item, location,
-            location)
+        self.assertRaises(InvalidVersionError, draft_store.clone_item, location, location)
 
-        self.assertRaises(InvalidVersionError, draft_store.update_item, location,
-            'chapter data')
+        self.assertRaises(InvalidVersionError, draft_store.update_item, location, 'chapter data')
 
         # taking advantage of update_children and other functions never checking that the ids are valid
         self.assertRaises(InvalidVersionError, draft_store.update_children, location,
-            ['i4x://MITx/999/problem/doesntexist'])
+                          ['i4x://MITx/999/problem/doesntexist'])
 
         self.assertRaises(InvalidVersionError, draft_store.update_metadata, location,
-            {'due': datetime.datetime.now(UTC)})
+                          {'due': datetime.datetime.now(UTC)})
 
         self.assertRaises(InvalidVersionError, draft_store.unpublish, location)
 
@@ -860,6 +861,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 +874,72 @@ 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'
+        self.assert_course_creation_failed(
+            "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.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)
-        data = parse_json(resp)
-
-        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'.")
+        self.assertEqual(resp.status_code, 403)
 
     def test_course_index_view_with_no_courses(self):
         """Test viewing the index page with no courses"""

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