Verify that caller of add or remove from creator group is staff.

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)
 
 

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):