From 4a697a8da171dad2a1056791607e91ad3e1c6dec Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 25 Jun 2013 17:07:51 -0400 Subject: [PATCH] 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):