From 318372f2c0a3c838fb4b785637375e1e632bf143 Mon Sep 17 00:00:00 2001
From: cahrens <christina@edx.org>
Date: Tue, 25 Jun 2013 15:11:55 -0400
Subject: [PATCH 1/5] Introduce course creator group.

---
 cms/djangoapps/auth/authz.py                  | 87 ++++++++++++++++---
 cms/djangoapps/auth/tests/test_authz.py       | 78 +++++++++++++++++
 .../contentstore/tests/test_contentstore.py   | 73 ++++++++++++----
 cms/djangoapps/contentstore/views/course.py   |  4 +-
 4 files changed, 209 insertions(+), 33 deletions(-)
 create mode 100644 cms/djangoapps/auth/tests/test_authz.py

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

From 2c60a7dbc142b1fbd973eca5931b8f0b00f6b397 Mon Sep 17 00:00:00 2001
From: cahrens <christina@edx.org>
Date: Tue, 25 Jun 2013 15:46:08 -0400
Subject: [PATCH 2/5] pep8 cleanup

---
 cms/djangoapps/auth/tests/test_authz.py       |  9 +++++---
 .../contentstore/tests/test_contentstore.py   | 21 +++++++++----------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py
index 4e44471ebf..8ecf3689b3 100644
--- a/cms/djangoapps/auth/tests/test_authz.py
+++ b/cms/djangoapps/auth/tests/test_authz.py
@@ -8,10 +8,12 @@ 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')
@@ -25,7 +27,7 @@ class CreatorGroupTest(TestCase):
 
     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}):
+        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.
@@ -34,7 +36,7 @@ class CreatorGroupTest(TestCase):
 
     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}):
+        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))
 
@@ -62,7 +64,8 @@ class CreatorGroupTest(TestCase):
 
     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}):
+        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))
 
diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py
index 093532c71d..ea4b0bbb5b 100644
--- a/cms/djangoapps/contentstore/tests/test_contentstore.py
+++ b/cms/djangoapps/contentstore/tests/test_contentstore.py
@@ -531,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
@@ -592,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)
 
@@ -903,7 +901,8 @@ class ContentStoreTest(ModuleStoreTestCase):
     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'.")
+        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."""
@@ -924,14 +923,14 @@ class ContentStoreTest(ModuleStoreTestCase):
 
     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}):
+        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}):
+        with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}):
             add_user_to_creator_group(self.user)
             self.assert_created_course()
 

From 190c07d9540c44ee671a7c50322412079c3c0eff Mon Sep 17 00:00:00 2001
From: cahrens <christina@edx.org>
Date: Tue, 25 Jun 2013 16:15:47 -0400
Subject: [PATCH 3/5] Add smoke coverage for add and remove of course group
 permissions.

---
 cms/djangoapps/auth/tests/test_authz.py | 62 ++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py
index 8ecf3689b3..61ac682908 100644
--- a/cms/djangoapps/auth/tests/test_authz.py
+++ b/cms/djangoapps/auth/tests/test_authz.py
@@ -5,8 +5,11 @@ 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
+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):
@@ -79,3 +82,60 @@ class CreatorGroupTest(TestCase):
             # 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))
+
+
+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 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.location, self.staff, 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.location, self.creator, 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)

From 4a697a8da171dad2a1056791607e91ad3e1c6dec Mon Sep 17 00:00:00 2001
From: cahrens <christina@edx.org>
Date: Tue, 25 Jun 2013 17:07:51 -0400
Subject: [PATCH 4/5] Verify that caller of add or remove from creator group is
 staff.

---
 cms/djangoapps/auth/authz.py            | 14 ++++++-
 cms/djangoapps/auth/tests/test_authz.py | 53 ++++++++++++++++++++-----
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py
index f27d2fe559..a544906875 100644
--- a/cms/djangoapps/auth/authz.py
+++ b/cms/djangoapps/auth/authz.py
@@ -102,13 +102,18 @@ def add_user_to_course_group(caller, user, location, role):
     return _add_user_to_group(user, group)
 
 
-def add_user_to_creator_group(user):
+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, created) = Group.objects.get_or_create(name=COURSE_CREATOR_GROUP_NAME)
     if created:
         group.save()
@@ -149,10 +154,15 @@ def remove_user_from_course_group(caller, user, location, role):
         _remove_user_from_group(user, get_course_groupname_for_role(location, role))
 
 
-def remove_user_from_creator_group(user):
+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)
 
 
diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py
index 61ac682908..173155df4c 100644
--- a/cms/djangoapps/auth/tests/test_authz.py
+++ b/cms/djangoapps/auth/tests/test_authz.py
@@ -20,6 +20,8 @@ class CreatorGroupTest(TestCase):
     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):
         """
@@ -40,7 +42,7 @@ class CreatorGroupTest(TestCase):
     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(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
@@ -48,7 +50,7 @@ class CreatorGroupTest(TestCase):
             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)
+            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):
@@ -56,21 +58,21 @@ class CreatorGroupTest(TestCase):
         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))
+        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.user))
+        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.user))
+            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))
@@ -80,9 +82,42 @@ class CreatorGroupTest(TestCase):
             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)
+            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):
     """
@@ -117,7 +152,7 @@ class CourseGroupTest(TestCase):
         with self.assertRaises(PermissionDenied):
             add_user_to_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME)
 
-    def remove_user_from_course_group(self):
+    def test_remove_user_from_course_group(self):
         """
         Tests removing user from course group (happy path).
         """
@@ -126,10 +161,10 @@ class CourseGroupTest(TestCase):
         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.location, self.staff, 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.location, self.creator, INSTRUCTOR_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):

From e487521289f4d89dc8164b377b40969cd8912236 Mon Sep 17 00:00:00 2001
From: cahrens <christina@edx.org>
Date: Wed, 26 Jun 2013 09:06:12 -0400
Subject: [PATCH 5/5] Update for change in add_user_to_creator_group API.

---
 cms/djangoapps/contentstore/tests/test_contentstore.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py
index ea4b0bbb5b..b946aac6bb 100644
--- a/cms/djangoapps/contentstore/tests/test_contentstore.py
+++ b/cms/djangoapps/contentstore/tests/test_contentstore.py
@@ -931,7 +931,7 @@ class ContentStoreTest(ModuleStoreTestCase):
     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)
+            add_user_to_creator_group(self.user, self.user)
             self.assert_created_course()
 
     def assert_course_permission_denied(self):