diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py
deleted file mode 100644
index 9ad595c66808895abe1a8f73470bc1d8cd3f8225..0000000000000000000000000000000000000000
--- a/cms/djangoapps/auth/authz.py
+++ /dev/null
@@ -1,291 +0,0 @@
-"""
-Studio authorization functions primarily for course creators, instructors, and staff
-"""
-#=======================================================================================================================
-#
-# This code is somewhat duplicative of access.py in the LMS. We will unify the code as a separate story
-# but this implementation should be data compatible with the LMS implementation
-#
-#=======================================================================================================================
-from django.contrib.auth.models import User, Group
-from django.core.exceptions import PermissionDenied
-from django.conf import settings
-
-from xmodule.modulestore import Location
-from xmodule.modulestore.locator import CourseLocator, Locator
-from xmodule.modulestore.django import loc_mapper
-from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError
-import itertools
-
-
-# define a couple of simple roles, we just need ADMIN and EDITOR now for our purposes
-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
-
-
-def get_all_course_role_groupnames(location, role, use_filter=True):
- '''
- Get all of the possible groupnames for this role location pair. If use_filter==True,
- only return the ones defined in the groups collection.
- '''
- location = Locator.to_locator_or_location(location)
-
- groupnames = []
- if isinstance(location, Location):
- try:
- groupnames.append('{0}_{1}'.format(role, location.course_id))
- except InvalidLocationError: # will occur on old locations where location is not of category course
- pass
- try:
- locator = loc_mapper().translate_location(location.course_id, location, False, False)
- groupnames.append('{0}_{1}'.format(role, locator.package_id))
- except (InvalidLocationError, ItemNotFoundError):
- pass
- # least preferred role_course format for legacy reasons
- groupnames.append('{0}_{1}'.format(role, location.course))
- elif isinstance(location, CourseLocator):
- groupnames.append('{0}_{1}'.format(role, location.package_id))
- old_location = loc_mapper().translate_locator_to_location(location, get_course=True)
- if old_location:
- # the slashified version of the course_id (myu/mycourse/myrun)
- groupnames.append('{0}_{1}'.format(role, old_location.course_id))
- # add the least desirable but sometimes occurring format.
- groupnames.append('{0}_{1}'.format(role, old_location.course))
- # filter to the ones which exist
- default = groupnames[0]
- if use_filter:
- groupnames = [group.name for group in Group.objects.filter(name__in=groupnames)]
- return groupnames, default
-
-
-def get_course_groupname_for_role(location, role):
- '''
- Get the preferred used groupname for this role, location combo.
- Preference order:
- * role_course_id (e.g., staff_myu.mycourse.myrun)
- * role_old_course_id (e.g., staff_myu/mycourse/myrun)
- * role_old_course (e.g., staff_mycourse)
- '''
- groupnames, default = get_all_course_role_groupnames(location, role)
- return groupnames[0] if groupnames else default
-
-
-def get_course_role_users(course_locator, role):
- '''
- Get all of the users with the given role in the given course.
- '''
- groupnames, _ = get_all_course_role_groupnames(course_locator, role)
- groups = [Group.objects.get(name=groupname) for groupname in groupnames]
- return list(itertools.chain.from_iterable(group.user_set.all() for group in groups))
-
-
-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)
-
-
-def create_new_course_group(creator, location, role):
- '''
- Create the new course group always using the preferred name even if another form already exists.
- '''
- groupnames, __ = get_all_course_role_groupnames(location, role, use_filter=False)
- group, __ = Group.objects.get_or_create(name=groupnames[0])
- creator.groups.add(group)
- creator.save()
-
- 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
- for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]:
- groupnames, _ = get_all_course_role_groupnames(location, role)
- for groupname in groupnames:
- group = Group.objects.get(name=groupname)
- for user in group.user_set.all():
- user.groups.remove(group)
- 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
- """
- for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]:
- groupnames, _ = get_all_course_role_groupnames(source, role)
- for groupname in groupnames:
- group = Group.objects.get(name=groupname)
- new_group, _ = Group.objects.get_or_create(name=get_course_groupname_for_role(dest, INSTRUCTOR_ROLE_NAME))
- for user in group.user_set.all():
- user.groups.add(new_group)
- user.save()
-
-
-def add_user_to_course_group(caller, user, location, role):
- """
- If caller is authorized, add the given user to the given course's role
- """
- # only admins can add/remove other users
- if not is_user_in_course_group_role(caller, location, INSTRUCTOR_ROLE_NAME):
- raise PermissionDenied
-
- group, _ = Group.objects.get_or_create(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_or_create(name=COURSE_CREATOR_GROUP_NAME)
- 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
-
- return False
-
-
-def get_user_by_email(email):
- """
- Get the user whose email is the arg. Return None if no such user exists.
- """
- user = None
- # try to look up user, return None if not found
- try:
- user = User.objects.get(email=email)
- except:
- pass
-
- return user
-
-
-def remove_user_from_course_group(caller, user, location, role):
- """
- If caller is authorized, remove the given course x role authorization for user
- """
- # only admins can add/remove other users
- if not is_user_in_course_group_role(caller, location, INSTRUCTOR_ROLE_NAME):
- raise PermissionDenied
-
- # see if the user is actually in that role, if not then we don't have to do anything
- groupnames, _ = get_all_course_role_groupnames(location, role)
- user.groups.remove(*user.groups.filter(name__in=groupnames))
- 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, check_staff=True):
- """
- Check whether the given user has the given role in this course. If check_staff
- then give permission if the user is staff without doing a course-role query.
- """
- if user.is_active and user.is_authenticated:
- # all "is_staff" flagged accounts belong to all groups
- if check_staff and user.is_staff:
- return True
- groupnames, _ = get_all_course_role_groupnames(location, role)
- return user.groups.filter(name__in=groupnames).exists()
-
- 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.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.FEATURES.get('ENABLE_CREATOR_GROUP', False):
- return user.groups.filter(name=COURSE_CREATOR_GROUP_NAME).exists()
-
- return True
-
-
-def get_users_with_instructor_role():
- """
- Returns all users with the role 'instructor'
- """
- return _get_users_with_role(INSTRUCTOR_ROLE_NAME)
-
-
-def get_users_with_staff_role():
- """
- Returns all users with the role 'staff'
- """
- return _get_users_with_role(STAFF_ROLE_NAME)
-
-
-def _get_users_with_role(role):
- """
- Returns all users with the specified role.
- """
- users = set()
- for group in Group.objects.all():
- if group.name.startswith(role + "_"):
- for user in group.user_set.all():
- users.add(user)
- return users
diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py
index cf0337628e0e63b5c1d1a05ca317c4e0322fdc08..289c911d4aae6a619b6dd3605c6631f6025a3612 100644
--- a/cms/djangoapps/auth/tests/test_authz.py
+++ b/cms/djangoapps/auth/tests/test_authz.py
@@ -8,10 +8,9 @@ from django.contrib.auth.models import User
from xmodule.modulestore import Location
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, get_users_with_staff_role,\
- get_users_with_instructor_role
+from student.roles import CourseInstructorRole, CourseStaffRole, CourseCreatorRole
+from student.tests.factories import AdminFactory
+from student.auth import has_access, add_users, remove_users
class CreatorGroupTest(TestCase):
@@ -27,98 +26,104 @@ class CreatorGroupTest(TestCase):
def test_creator_group_not_enabled(self):
"""
- Tests that is_user_in_creator_group always returns True if ENABLE_CREATOR_GROUP
+ Tests that CourseCreatorRole().has_user 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))
+ self.assertTrue(has_access(self.user, CourseCreatorRole()))
def test_creator_group_enabled_but_empty(self):
""" Tests creator group feature on, but group empty. """
with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}):
- self.assertFalse(is_user_in_creator_group(self.user))
+ self.assertFalse(has_access(self.user, CourseCreatorRole()))
- # Make user staff. This will cause is_user_in_creator_group to return True.
+ # Make user staff. This will cause CourseCreatorRole().has_user to return True.
self.user.is_staff = True
- self.assertTrue(is_user_in_creator_group(self.user))
+ self.assertTrue(has_access(self.user, CourseCreatorRole()))
def test_creator_group_enabled_nonempty(self):
""" Tests creator group feature on, user added. """
with mock.patch.dict('django.conf.settings.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))
+ add_users(self.admin, CourseCreatorRole(), self.user)
+ self.assertTrue(has_access(self.user, CourseCreatorRole()))
# 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))
+ self.assertFalse(has_access(user_not_added, CourseCreatorRole()))
- # 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))
+ # remove first user from the group and verify that CourseCreatorRole().has_user now returns false
+ remove_users(self.admin, CourseCreatorRole(), self.user)
+ self.assertFalse(has_access(self.user, CourseCreatorRole()))
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.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))
+ add_users(self.admin, CourseCreatorRole(), self.user)
# DISABLE_COURSE_CREATION overrides (user is not marked as staff).
- self.assertFalse(is_user_in_creator_group(self.user))
+ self.assertFalse(has_access(self.user, CourseCreatorRole()))
- # Mark as staff. Now is_user_in_creator_group returns true.
+ # Mark as staff. Now CourseCreatorRole().has_user returns true.
self.user.is_staff = True
- self.assertTrue(is_user_in_creator_group(self.user))
+ self.assertTrue(has_access(self.user, CourseCreatorRole()))
+
+ # Remove user from creator group. CourseCreatorRole().has_user still returns true because is_staff=True
+ remove_users(self.admin, CourseCreatorRole(), self.user)
+ self.assertTrue(has_access(self.user, CourseCreatorRole()))
+
+ def test_add_user_not_authenticated(self):
+ """
+ Tests that adding to creator group fails if user is not authenticated
+ """
+ with mock.patch.dict('django.conf.settings.FEATURES',
+ {'DISABLE_COURSE_CREATION': False, "ENABLE_CREATOR_GROUP": True}):
+ self.user.is_authenticated = False
+ add_users(self.admin, CourseCreatorRole(), self.user)
+ self.assertFalse(has_access(self.user, CourseCreatorRole()))
- # 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_not_active(self):
+ """
+ Tests that adding to creator group fails if user is not active
+ """
+ with mock.patch.dict('django.conf.settings.FEATURES',
+ {'DISABLE_COURSE_CREATION': False, "ENABLE_CREATOR_GROUP": True}):
+ self.user.is_active = False
+ add_users(self.admin, CourseCreatorRole(), self.user)
+ self.assertFalse(has_access(self.user, CourseCreatorRole()))
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)
+ add_users(self.admin, CourseCreatorRole(), self.user)
with self.assertRaises(PermissionDenied):
- add_user_to_creator_group(self.user, self.user)
+ add_users(self.user, CourseCreatorRole(), 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)
+ add_users(self.admin, CourseCreatorRole(), 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)
+ add_users(self.admin, CourseCreatorRole(), 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)
+ remove_users(self.admin, CourseCreatorRole(), 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)
+ remove_users(self.admin, CourseCreatorRole(), 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)
+ remove_users(self.admin, CourseCreatorRole(), self.user)
class CourseGroupTest(TestCase):
@@ -128,6 +133,7 @@ class CourseGroupTest(TestCase):
def setUp(self):
""" Test case setup """
+ self.global_admin = AdminFactory()
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 = Location('i4x', 'mitX', '101', 'course', 'test')
@@ -137,67 +143,47 @@ class CourseGroupTest(TestCase):
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))
+ self.assertFalse(has_access(self.creator, CourseInstructorRole(self.location)))
+ add_users(self.global_admin, CourseInstructorRole(self.location), self.creator)
+ add_users(self.global_admin, CourseStaffRole(self.location), self.creator)
+ self.assertTrue(has_access(self.creator, CourseInstructorRole(self.location)))
# 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))
+ self.assertFalse(has_access(self.staff, CourseStaffRole(self.location)))
+ add_users(self.creator, CourseStaffRole(self.location), self.staff)
+ self.assertTrue(has_access(self.staff, CourseStaffRole(self.location)))
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)
+ add_users(self.global_admin, CourseInstructorRole(self.location), self.creator)
+ add_users(self.global_admin, CourseStaffRole(self.location), self.creator)
with self.assertRaises(PermissionDenied):
- add_user_to_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME)
+ add_users(self.staff, CourseStaffRole(self.location), self.staff)
def test_remove_user_from_course_group(self):
"""
Tests removing user from course group (happy path).
"""
- create_all_course_groups(self.creator, self.location)
+ add_users(self.global_admin, CourseInstructorRole(self.location), self.creator)
+ add_users(self.global_admin, CourseStaffRole(self.location), self.creator)
- 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))
+ add_users(self.creator, CourseStaffRole(self.location), self.staff)
+ self.assertTrue(has_access(self.staff, CourseStaffRole(self.location)))
- 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_users(self.creator, CourseStaffRole(self.location), self.staff)
+ self.assertFalse(has_access(self.staff, CourseStaffRole(self.location)))
- 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))
+ remove_users(self.creator, CourseInstructorRole(self.location), self.creator)
+ self.assertFalse(has_access(self.creator, CourseInstructorRole(self.location)))
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)
+ add_users(self.global_admin, CourseInstructorRole(self.location), self.creator)
+ another_staff = User.objects.create_user('another', 'teststaff+anothercourses@edx.org', 'foo')
+ add_users(self.global_admin, CourseStaffRole(self.location), self.creator, self.staff, another_staff)
with self.assertRaises(PermissionDenied):
- remove_user_from_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME)
-
- def test_get_staff(self):
- # Do this test with staff in 2 different classes.
- create_all_course_groups(self.creator, self.location)
- add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
-
- location2 = Location('i4x', 'mitX', '103', 'course', 'test2')
- staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
- create_all_course_groups(self.creator, location2)
- add_user_to_course_group(self.creator, staff2, location2, STAFF_ROLE_NAME)
-
- self.assertSetEqual({self.staff, staff2, self.creator}, get_users_with_staff_role())
-
- def test_get_instructor(self):
- # Do this test with creators in 2 different classes.
- create_all_course_groups(self.creator, self.location)
- add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
-
- location2 = Location('i4x', 'mitX', '103', 'course', 'test2')
- creator2 = User.objects.create_user('testcreator2', 'testcreator2+courses@edx.org', 'foo')
- staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
- create_all_course_groups(creator2, location2)
- add_user_to_course_group(creator2, staff2, location2, STAFF_ROLE_NAME)
-
- self.assertSetEqual({self.creator, creator2}, get_users_with_instructor_role())
+ remove_users(self.staff, CourseStaffRole(self.location), another_staff)
diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py
index cd9c00c09ca5660ae8b7cd5f934a858b9a5772dc..c97c5d170424f3f09b8bcebc57b22264991b035d 100644
--- a/cms/djangoapps/contentstore/features/common.py
+++ b/cms/djangoapps/contentstore/features/common.py
@@ -1,18 +1,20 @@
# pylint: disable=C0111
# pylint: disable=W0621
+import time
+import os
from lettuce import world, step
-from nose.tools import assert_true, assert_in, assert_false # pylint: disable=E0611
-
-from auth.authz import get_user_by_email, get_course_groupname_for_role
+from nose.tools import assert_true, assert_in # pylint: disable=no-name-in-module
from django.conf import settings
+from student.roles import CourseRole, CourseStaffRole, CourseInstructorRole
+from student.models import get_user
+
from selenium.webdriver.common.keys import Keys
-import time
-import os
-from django.contrib.auth.models import Group
from logging import getLogger
+from student.tests.factories import AdminFactory
+from student import auth
logger = getLogger(__name__)
from terrain.browser import reset_data
@@ -158,11 +160,9 @@ def add_course_author(user, course):
Add the user to the instructor group of the course
so they will have the permissions to see it in studio
"""
- for role in ("staff", "instructor"):
- groupname = get_course_groupname_for_role(course.location, role)
- group, __ = Group.objects.get_or_create(name=groupname)
- user.groups.add(group)
- user.save()
+ global_admin = AdminFactory()
+ for role in (CourseStaffRole, CourseInstructorRole):
+ auth.add_users(global_admin, role(course.location), user)
def create_a_course():
@@ -171,7 +171,7 @@ def create_a_course():
user = world.scenario_dict.get("USER")
if not user:
- user = get_user_by_email('robot+studio@edx.org')
+ user = get_user('robot+studio@edx.org')
add_course_author(user, course)
@@ -358,7 +358,7 @@ def other_user_login(step, name):
login_form.find_by_name('submit').click()
world.retry_on_exception(fill_login_form)
assert_true(world.is_css_present('.new-course-button'))
- world.scenario_dict['USER'] = get_user_by_email(name + '@edx.org')
+ world.scenario_dict['USER'] = get_user(name + '@edx.org')
@step(u'the user "([^"]*)" exists( as a course (admin|staff member|is_staff))?$')
@@ -368,18 +368,17 @@ def create_other_user(_step, name, has_extra_perms, role_name):
if has_extra_perms:
if role_name == "is_staff":
user.is_staff = True
+ user.save()
else:
if role_name == "admin":
# admins get staff privileges, as well
- roles = ("staff", "instructor")
+ roles = (CourseStaffRole, CourseInstructorRole)
else:
- roles = ("staff",)
+ roles = (CourseStaffRole,)
location = world.scenario_dict["COURSE"].location
+ global_admin = AdminFactory()
for role in roles:
- groupname = get_course_groupname_for_role(location, role)
- group, __ = Group.objects.get_or_create(name=groupname)
- user.groups.add(group)
- user.save()
+ auth.add_users(global_admin, role(location), user)
@step('I log out')
@@ -393,7 +392,7 @@ def i_edit_a_draft(_step):
@step(u'I click on "replace with draft"$')
-def i_edit_a_draft(_step):
+def i_replace_w_draft(_step):
world.css_click("a.publish-draft")
diff --git a/cms/djangoapps/contentstore/management/commands/clone_course.py b/cms/djangoapps/contentstore/management/commands/clone_course.py
index 04f0c890879db3954077ca42c5febe0d878d29e0..67f23d20eb6db644254472dd6047fa0753f5ddff 100644
--- a/cms/djangoapps/contentstore/management/commands/clone_course.py
+++ b/cms/djangoapps/contentstore/management/commands/clone_course.py
@@ -6,8 +6,7 @@ from xmodule.modulestore.store_utilities import clone_course
from xmodule.modulestore.django import modulestore
from xmodule.contentstore.django import contentstore
from xmodule.course_module import CourseDescriptor
-
-from auth.authz import _copy_course_group
+from student.roles import CourseInstructorRole, CourseStaffRole
#
@@ -20,7 +19,7 @@ class Command(BaseCommand):
def handle(self, *args, **options):
"Execute the command"
if len(args) != 2:
- raise CommandError("clone requires two arguments: <source-course_id> <dest-course_id>")
+ raise CommandError("clone requires 2 arguments: <source-course_id> <dest-course_id>")
source_course_id = args[0]
dest_course_id = args[1]
@@ -28,7 +27,7 @@ class Command(BaseCommand):
mstore = modulestore('direct')
cstore = contentstore()
- org, course_num, run = dest_course_id.split("/")
+ org, course_num, _ = dest_course_id.split("/")
mstore.ignore_write_events_on_courses.append('{0}/{1}'.format(org, course_num))
print("Cloning course {0} to {1}".format(source_course_id, dest_course_id))
@@ -41,4 +40,10 @@ class Command(BaseCommand):
mstore.refresh_cached_metadata_inheritance_tree(dest_location)
print("copying User permissions...")
- _copy_course_group(source_location, dest_location)
+ # purposely avoids auth.add_user b/c it doesn't have a caller to authorize
+ CourseInstructorRole(dest_location).add_users(
+ *CourseInstructorRole(source_location).users_with_role()
+ )
+ CourseStaffRole(dest_location).add_users(
+ *CourseStaffRole(source_location).users_with_role()
+ )
diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py
index c23d9bb44bc4d2d1a48633b01bda3a39255dd055..2f57158f0cbf48911ace79782edd941f27541469 100644
--- a/cms/djangoapps/contentstore/tests/test_contentstore.py
+++ b/cms/djangoapps/contentstore/tests/test_contentstore.py
@@ -20,8 +20,6 @@ from django.dispatch import Signal
from contentstore.utils import get_modulestore
from contentstore.tests.utils import parse_json, AjaxEnabledTestClient
-from auth.authz import add_user_to_creator_group
-
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from contentstore.tests.modulestore_config import TEST_MODULESTORE
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
@@ -57,6 +55,8 @@ import re
from contentstore.utils import delete_course_and_groups
from xmodule.modulestore.django import loc_mapper
+from student.roles import CourseCreatorRole
+from student import auth
TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex
@@ -1530,7 +1530,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.FEATURES', {"ENABLE_CREATOR_GROUP": True}):
- add_user_to_creator_group(self.user, self.user)
+ auth.add_users(self.user, CourseCreatorRole(), self.user)
self.assert_created_course()
def assert_course_permission_denied(self):
diff --git a/cms/djangoapps/contentstore/tests/test_permissions.py b/cms/djangoapps/contentstore/tests/test_permissions.py
index 8362fc1369b26a803f8f60f60d4f2a70befcfc6c..1bd63045a71e8b828b7022d8f1a07d601f73a0c4 100644
--- a/cms/djangoapps/contentstore/tests/test_permissions.py
+++ b/cms/djangoapps/contentstore/tests/test_permissions.py
@@ -1,6 +1,8 @@
"""
Test CRUD for authorization.
"""
+import copy
+
from django.test.utils import override_settings
from django.contrib.auth.models import User, Group
@@ -9,10 +11,9 @@ from contentstore.tests.modulestore_config import TEST_MODULESTORE
from contentstore.tests.utils import AjaxEnabledTestClient
from xmodule.modulestore.django import loc_mapper
from xmodule.modulestore import Location
-from auth.authz import INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME
-from auth import authz
-import copy
-from contentstore.views.access import has_access
+from student.roles import CourseInstructorRole, CourseStaffRole
+from contentstore.views.access import has_course_access
+from student import auth
@override_settings(MODULESTORE=TEST_MODULESTORE)
@@ -87,30 +88,35 @@ class TestCourseAccess(ModuleStoreTestCase):
Test getting all authors for a course where their permissions run the gamut of allowed group
types.
"""
- # first check the groupname for the course creator.
+ # first check the course creator.has explicit access (don't use has_access as is_staff
+ # will trump the actual test)
self.assertTrue(
- self.user.groups.filter(
- name="{}_{}".format(INSTRUCTOR_ROLE_NAME, self.course_locator.package_id)
- ).exists(),
+ CourseInstructorRole(self.course_locator).has_user(self.user),
"Didn't add creator as instructor."
)
users = copy.copy(self.users)
+ # doesn't use role.users_with_role b/c it's verifying the roles.py behavior
user_by_role = {}
# add the misc users to the course in different groups
- for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]:
+ for role in [CourseInstructorRole, CourseStaffRole]:
user_by_role[role] = []
- groupnames, _ = authz.get_all_course_role_groupnames(self.course_locator, role)
+ # pylint: disable=protected-access
+ groupnames = role(self.course_locator)._group_names
+ self.assertGreater(len(groupnames), 1, "Only 0 or 1 groupname for {}".format(role.ROLE))
+ # NOTE: this loop breaks the roles.py abstraction by purposely assigning
+ # users to one of each possible groupname in order to test that has_course_access
+ # and remove_user work
for groupname in groupnames:
group, _ = Group.objects.get_or_create(name=groupname)
user = users.pop()
user_by_role[role].append(user)
user.groups.add(group)
user.save()
- self.assertTrue(has_access(user, self.course_locator), "{} does not have access".format(user))
- self.assertTrue(has_access(user, self.course_location), "{} does not have access".format(user))
+ self.assertTrue(has_course_access(user, self.course_locator), "{} does not have access".format(user))
+ self.assertTrue(has_course_access(user, self.course_location), "{} does not have access".format(user))
response = self.client.get_html(self.course_locator.url_reverse('course_team'))
- for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]:
+ for role in [CourseInstructorRole, CourseStaffRole]:
for user in user_by_role[role]:
self.assertContains(response, user.email)
@@ -119,9 +125,23 @@ class TestCourseAccess(ModuleStoreTestCase):
copy_course_locator = loc_mapper().translate_location(
copy_course_location.course_id, copy_course_location, False, True
)
- # pylint: disable=protected-access
- authz._copy_course_group(self.course_locator, copy_course_locator)
- for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]:
+ for role in [CourseInstructorRole, CourseStaffRole]:
+ auth.add_users(
+ self.user,
+ role(copy_course_locator),
+ *role(self.course_locator).users_with_role()
+ )
+ # verify access in copy course and verify that removal from source course w/ the various
+ # groupnames works
+ for role in [CourseInstructorRole, CourseStaffRole]:
for user in user_by_role[role]:
- self.assertTrue(has_access(user, copy_course_locator), "{} no copy access".format(user))
- self.assertTrue(has_access(user, copy_course_location), "{} no copy access".format(user))
+ # forcefully decache the groups: premise is that any real request will not have
+ # multiple objects repr the same user but this test somehow uses different instance
+ # in above add_users call
+ if hasattr(user, '_groups'):
+ del user._groups
+
+ self.assertTrue(has_course_access(user, copy_course_locator), "{} no copy access".format(user))
+ self.assertTrue(has_course_access(user, copy_course_location), "{} no copy access".format(user))
+ auth.remove_users(self.user, role(self.course_locator), user)
+ self.assertFalse(has_course_access(user, self.course_locator), "{} remove didn't work".format(user))
diff --git a/cms/djangoapps/contentstore/tests/test_users.py b/cms/djangoapps/contentstore/tests/test_users.py
index ec8b5570a5342568862af6603751f809ec2399c5..27b87fe191930b67ce8d637f93f577a83b235563 100644
--- a/cms/djangoapps/contentstore/tests/test_users.py
+++ b/cms/djangoapps/contentstore/tests/test_users.py
@@ -4,9 +4,10 @@ Tests for contentstore/views/user.py.
import json
from .utils import CourseTestCase
from django.contrib.auth.models import User, Group
-from auth.authz import get_course_groupname_for_role
from student.models import CourseEnrollment
from xmodule.modulestore.django import loc_mapper
+from student.roles import CourseStaffRole, CourseInstructorRole
+from student import auth
class UsersTestCase(CourseTestCase):
@@ -29,8 +30,6 @@ class UsersTestCase(CourseTestCase):
self.detail_url = self.location.url_reverse('course_team', self.ext_user.email)
self.inactive_detail_url = self.location.url_reverse('course_team', self.inactive_user.email)
self.invalid_detail_url = self.location.url_reverse('course_team', "nonexistent@user.com")
- self.staff_groupname = get_course_groupname_for_role(self.course_locator, "staff")
- self.inst_groupname = get_course_groupname_for_role(self.course_locator, "instructor")
def test_index(self):
resp = self.client.get(self.index_url, HTTP_ACCEPT='text/html')
@@ -39,9 +38,7 @@ class UsersTestCase(CourseTestCase):
self.assertNotContains(resp, self.ext_user.email)
def test_index_member(self):
- group, _ = Group.objects.get_or_create(name=self.staff_groupname)
- self.ext_user.groups.add(group)
- self.ext_user.save()
+ auth.add_users(self.user, CourseStaffRole(self.course_locator), self.ext_user)
resp = self.client.get(self.index_url, HTTP_ACCEPT='text/html')
self.assertContains(resp, self.ext_user.email)
@@ -73,10 +70,9 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
# no content: should not be in any roles
- self.assertNotIn(self.staff_groupname, groups)
- self.assertNotIn(self.inst_groupname, groups)
+ self.assertFalse(auth.has_access(ext_user, CourseStaffRole(self.course_locator)))
+ self.assertFalse(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
self.assert_not_enrolled()
def test_detail_post_staff(self):
@@ -89,15 +85,12 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertIn(self.staff_groupname, groups)
- self.assertNotIn(self.inst_groupname, groups)
+ self.assertTrue(auth.has_access(ext_user, CourseStaffRole(self.course_locator)))
+ self.assertFalse(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
self.assert_enrolled()
def test_detail_post_staff_other_inst(self):
- inst_group, _ = Group.objects.get_or_create(name=self.inst_groupname)
- self.user.groups.add(inst_group)
- self.user.save()
+ auth.add_users(self.user, CourseInstructorRole(self.course_locator), self.user)
resp = self.client.post(
self.detail_url,
@@ -108,15 +101,13 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertIn(self.staff_groupname, groups)
- self.assertNotIn(self.inst_groupname, groups)
+ self.assertTrue(auth.has_access(ext_user, CourseStaffRole(self.course_locator)))
+ self.assertFalse(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
self.assert_enrolled()
# check that other user is unchanged
user = User.objects.get(email=self.user.email)
- groups = [g.name for g in user.groups.all()]
- self.assertNotIn(self.staff_groupname, groups)
- self.assertIn(self.inst_groupname, groups)
+ self.assertTrue(auth.has_access(user, CourseInstructorRole(self.course_locator)))
+ self.assertFalse(CourseStaffRole(self.course_locator).has_user(user))
def test_detail_post_instructor(self):
resp = self.client.post(
@@ -128,9 +119,8 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertNotIn(self.staff_groupname, groups)
- self.assertIn(self.inst_groupname, groups)
+ self.assertTrue(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
+ self.assertFalse(CourseStaffRole(self.course_locator).has_user(ext_user))
self.assert_enrolled()
def test_detail_post_missing_role(self):
@@ -154,15 +144,12 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertIn(self.staff_groupname, groups)
- self.assertNotIn(self.inst_groupname, groups)
+ self.assertTrue(auth.has_access(ext_user, CourseStaffRole(self.course_locator)))
+ self.assertFalse(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
self.assert_enrolled()
def test_detail_delete_staff(self):
- group, _ = Group.objects.get_or_create(name=self.staff_groupname)
- self.ext_user.groups.add(group)
- self.ext_user.save()
+ auth.add_users(self.user, CourseStaffRole(self.course_locator), self.ext_user)
resp = self.client.delete(
self.detail_url,
@@ -171,15 +158,10 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertNotIn(self.staff_groupname, groups)
+ self.assertFalse(auth.has_access(ext_user, CourseStaffRole(self.course_locator)))
def test_detail_delete_instructor(self):
- group, _ = Group.objects.get_or_create(name=self.inst_groupname)
- self.user.groups.add(group)
- self.ext_user.groups.add(group)
- self.user.save()
- self.ext_user.save()
+ auth.add_users(self.user, CourseInstructorRole(self.course_locator), self.ext_user, self.user)
resp = self.client.delete(
self.detail_url,
@@ -188,13 +170,10 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertNotIn(self.inst_groupname, groups)
+ self.assertFalse(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
def test_delete_last_instructor(self):
- group, _ = Group.objects.get_or_create(name=self.inst_groupname)
- self.ext_user.groups.add(group)
- self.ext_user.save()
+ auth.add_users(self.user, CourseInstructorRole(self.course_locator), self.ext_user)
resp = self.client.delete(
self.detail_url,
@@ -205,13 +184,10 @@ class UsersTestCase(CourseTestCase):
self.assertIn("error", result)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertIn(self.inst_groupname, groups)
+ self.assertTrue(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
def test_post_last_instructor(self):
- group, _ = Group.objects.get_or_create(name=self.inst_groupname)
- self.ext_user.groups.add(group)
- self.ext_user.save()
+ auth.add_users(self.user, CourseInstructorRole(self.course_locator), self.ext_user)
resp = self.client.post(
self.detail_url,
@@ -223,12 +199,10 @@ class UsersTestCase(CourseTestCase):
self.assertIn("error", result)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertIn(self.inst_groupname, groups)
+ self.assertTrue(auth.has_access(ext_user, CourseInstructorRole(self.course_locator)))
def test_permission_denied_self(self):
- group, _ = Group.objects.get_or_create(name=self.staff_groupname)
- self.user.groups.add(group)
+ auth.add_users(self.user, CourseStaffRole(self.course_locator), self.user)
self.user.is_staff = False
self.user.save()
@@ -244,8 +218,7 @@ class UsersTestCase(CourseTestCase):
self.assertIn("error", result)
def test_permission_denied_other(self):
- group, _ = Group.objects.get_or_create(name=self.staff_groupname)
- self.user.groups.add(group)
+ auth.add_users(self.user, CourseStaffRole(self.course_locator), self.user)
self.user.is_staff = False
self.user.save()
@@ -259,8 +232,7 @@ class UsersTestCase(CourseTestCase):
self.assertIn("error", result)
def test_staff_can_delete_self(self):
- group, _ = Group.objects.get_or_create(name=self.staff_groupname)
- self.user.groups.add(group)
+ auth.add_users(self.user, CourseStaffRole(self.course_locator), self.user)
self.user.is_staff = False
self.user.save()
@@ -270,16 +242,12 @@ class UsersTestCase(CourseTestCase):
self.assertEqual(resp.status_code, 204)
# reload user from DB
user = User.objects.get(email=self.user.email)
- groups = [g.name for g in user.groups.all()]
- self.assertNotIn(self.staff_groupname, groups)
+ self.assertFalse(auth.has_access(user, CourseStaffRole(self.course_locator)))
def test_staff_cannot_delete_other(self):
- group, _ = Group.objects.get_or_create(name=self.staff_groupname)
- self.user.groups.add(group)
+ auth.add_users(self.user, CourseStaffRole(self.course_locator), self.user, self.ext_user)
self.user.is_staff = False
self.user.save()
- self.ext_user.groups.add(group)
- self.ext_user.save()
resp = self.client.delete(self.detail_url)
self.assertEqual(resp.status_code, 400)
@@ -287,8 +255,7 @@ class UsersTestCase(CourseTestCase):
self.assertIn("error", result)
# reload user from DB
ext_user = User.objects.get(email=self.ext_user.email)
- groups = [g.name for g in ext_user.groups.all()]
- self.assertIn(self.staff_groupname, groups)
+ self.assertTrue(auth.has_access(ext_user, CourseStaffRole(self.course_locator)))
def test_user_not_initially_enrolled(self):
# Verify that ext_user is not enrolled in the new course before being added as a staff member.
diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py
index b7d9cf9e77594e8dc367de9a4587935f5f08fd0f..1aeb447331f2433533069932069f705fabb2244a 100644
--- a/cms/djangoapps/contentstore/utils.py
+++ b/cms/djangoapps/contentstore/utils.py
@@ -13,10 +13,10 @@ from xmodule.modulestore import Location
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.exceptions import ItemNotFoundError
from django_comment_common.utils import unseed_permissions_roles
-from auth.authz import _delete_course_group
from xmodule.modulestore.store_utilities import delete_course
from xmodule.course_module import CourseDescriptor
from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES
+from student.roles import CourseInstructorRole, CourseStaffRole
log = logging.getLogger(__name__)
@@ -35,7 +35,7 @@ def delete_course_and_groups(course_id, commit=False):
module_store = modulestore('direct')
content_store = contentstore()
- org, course_num, run = course_id.split("/")
+ org, course_num, _ = course_id.split("/")
module_store.ignore_write_events_on_courses.append('{0}/{1}'.format(org, course_num))
loc = CourseDescriptor.id_to_location(course_id)
@@ -47,7 +47,10 @@ def delete_course_and_groups(course_id, commit=False):
# in the django layer, we need to remove all the user permissions groups associated with this course
if commit:
try:
- _delete_course_group(loc)
+ staff_role = CourseStaffRole(loc)
+ staff_role.remove_users(*staff_role.users_with_role())
+ instructor_role = CourseInstructorRole(loc)
+ instructor_role.remove_users(*instructor_role.users_with_role())
except Exception as err:
log.error("Error in deleting course groups for {0}: {1}".format(loc, err))
diff --git a/cms/djangoapps/contentstore/views/access.py b/cms/djangoapps/contentstore/views/access.py
index df4fbed623657c28fcc7b30c7c3d1e8065bf9e73..215c8becf2ecbfa3c459ae53899347120f990d3d 100644
--- a/cms/djangoapps/contentstore/views/access.py
+++ b/cms/djangoapps/contentstore/views/access.py
@@ -1,10 +1,10 @@
-from auth.authz import STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME
-from auth.authz import is_user_in_course_group_role
from ..utils import get_course_location_for_item
from xmodule.modulestore.locator import CourseLocator
+from student.roles import CourseStaffRole, GlobalStaff
+from student import auth
-def has_access(user, location, role=STAFF_ROLE_NAME):
+def has_course_access(user, location, role=CourseStaffRole):
"""
Return True if user allowed to access this piece of data
Note that the CMS permissions model is with respect to courses
@@ -14,14 +14,9 @@ def has_access(user, location, role=STAFF_ROLE_NAME):
will not be in both INSTRUCTOR and STAFF groups, so we have to cascade our
queries here as INSTRUCTOR has all the rights that STAFF do
"""
+ if GlobalStaff().has_user(user):
+ return True
if not isinstance(location, CourseLocator):
+ # this can be expensive if location is not category=='course'
location = get_course_location_for_item(location)
- _has_access = is_user_in_course_group_role(user, location, role)
- # if we're not in STAFF, perhaps we're in INSTRUCTOR groups
- if not _has_access and role == STAFF_ROLE_NAME:
- _has_access = is_user_in_course_group_role(
- user,
- location,
- INSTRUCTOR_ROLE_NAME
- )
- return _has_access
+ return auth.has_access(user, role(location))
diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py
index 255717c73e45a59ab8bba3f4b7a7b3ea9523651e..cd59b811d7641629ff9625a8429fe6dc00f26fcc 100644
--- a/cms/djangoapps/contentstore/views/assets.py
+++ b/cms/djangoapps/contentstore/views/assets.py
@@ -21,13 +21,13 @@ from xmodule.modulestore import InvalidLocationError
from xmodule.exceptions import NotFoundError
from django.core.exceptions import PermissionDenied
from xmodule.modulestore.django import loc_mapper
-from .access import has_access
from xmodule.modulestore.locator import BlockUsageLocator
from util.json_request import JsonResponse
from django.http import HttpResponseNotFound
from django.utils.translation import ugettext as _
from pymongo import ASCENDING, DESCENDING
+from .access import has_course_access
__all__ = ['assets_handler']
@@ -56,7 +56,7 @@ def assets_handler(request, tag=None, package_id=None, branch=None, version_guid
json: delete an asset
"""
location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, location):
+ if not has_course_access(request.user, location):
raise PermissionDenied()
response_format = request.REQUEST.get('format', 'html')
diff --git a/cms/djangoapps/contentstore/views/checklist.py b/cms/djangoapps/contentstore/views/checklist.py
index 9fddf774c658cb79443cd9198779f4b890c4418e..123ff08f9a0a0230e4659ec6763569f0fc026786 100644
--- a/cms/djangoapps/contentstore/views/checklist.py
+++ b/cms/djangoapps/contentstore/views/checklist.py
@@ -15,7 +15,7 @@ from xmodule.modulestore.inheritance import own_metadata
from ..utils import get_modulestore
-from .access import has_access
+from .access import has_course_access
from xmodule.course_module import CourseDescriptor
from xmodule.modulestore.locator import BlockUsageLocator
@@ -37,7 +37,7 @@ def checklists_handler(request, tag=None, package_id=None, branch=None, version_
json: updates the checked state for items within a particular checklist. checklist_index is required.
"""
location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, location):
+ if not has_course_access(request.user, location):
raise PermissionDenied()
old_location = loc_mapper().translate_locator_to_location(location)
diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py
index 6dd5b6c2933684bd54de866fb85a5c3e446ba7e9..38b05c094c0bd6ee7372b7b4da8d0a4b5bb0cd62 100644
--- a/cms/djangoapps/contentstore/views/component.py
+++ b/cms/djangoapps/contentstore/views/component.py
@@ -29,7 +29,7 @@ from contentstore.utils import get_lms_link_for_item, compute_unit_state, UnitSt
from models.settings.course_grading import CourseGradingModel
-from .access import has_access
+from .access import has_course_access
__all__ = ['OPEN_ENDED_COMPONENT_TYPES',
'ADVANCED_COMPONENT_POLICY_KEY',
@@ -311,7 +311,7 @@ def _get_item_in_course(request, locator):
Verifies that the caller has permission to access this item.
"""
- if not has_access(request.user, locator):
+ if not has_course_access(request.user, locator):
raise PermissionDenied()
old_location = loc_mapper().translate_locator_to_location(locator)
diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py
index d0848595367d9ed422717186d47421015f92f787..5c0eea910c6259eb86b2a158e29ab679c882fd95 100644
--- a/cms/djangoapps/contentstore/views/course.py
+++ b/cms/djangoapps/contentstore/views/course.py
@@ -35,10 +35,9 @@ 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, is_user_in_creator_group
from util.json_request import expect_json
-from .access import has_access
+from .access import has_course_access
from .tabs import initialize_course_tabs
from .component import (
OPEN_ENDED_COMPONENT_TYPES, NOTE_COMPONENT_TYPES,
@@ -52,6 +51,8 @@ from xmodule.html_module import AboutDescriptor
from xmodule.modulestore.locator import BlockUsageLocator
from course_creators.views import get_course_creator_status, add_user_with_status_unrequested
from contentstore import utils
+from student.roles import CourseInstructorRole, CourseStaffRole, CourseCreatorRole
+from student import auth
__all__ = ['course_info_handler', 'course_handler', 'course_info_update_handler',
'settings_handler',
@@ -66,7 +67,7 @@ def _get_locator_and_course(package_id, branch, version_guid, block_id, user, de
for the view functions in this file.
"""
locator = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block_id)
- if not has_access(user, locator):
+ if not has_course_access(user, locator):
raise PermissionDenied()
course_location = loc_mapper().translate_locator_to_location(locator)
course_module = modulestore().get_item(course_location, depth=depth)
@@ -102,7 +103,7 @@ def course_handler(request, tag=None, package_id=None, branch=None, version_guid
raise NotImplementedError('coming soon')
elif request.method == 'POST': # not sure if this is only post. If one will have ids, it goes after access
return create_new_course(request)
- elif not has_access(
+ elif not has_course_access(
request.user,
BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
):
@@ -136,7 +137,7 @@ def course_listing(request):
"""
Get courses to which this user has access
"""
- return (has_access(request.user, course.location)
+ return (has_course_access(request.user, course.location)
# pylint: disable=fixme
# TODO remove this condition when templates purged from db
and course.location.course != 'templates'
@@ -207,7 +208,7 @@ def create_new_course(request):
Returns the URL for the course overview page.
"""
- if not is_user_in_creator_group(request.user):
+ if not auth.has_access(request.user, CourseCreatorRole()):
raise PermissionDenied()
org = request.json.get('org')
@@ -297,7 +298,10 @@ def create_new_course(request):
initialize_course_tabs(new_course)
new_location = loc_mapper().translate_location(new_course.location.course_id, new_course.location, False, True)
- create_all_course_groups(request.user, new_location)
+ # can't use auth.add_users here b/c it requires request.user to already have Instructor perms in this course
+ # however, we can assume that b/c this user had authority to create the course, the user can add themselves
+ CourseInstructorRole(new_location).add_users(request.user)
+ auth.add_users(request.user, CourseStaffRole(new_location), request.user)
# seed the forums
seed_permissions_roles(new_course.location.course_id)
@@ -370,7 +374,7 @@ def course_info_update_handler(request, tag=None, package_id=None, branch=None,
provided_id = None
# check that logged in user has permissions to this item (GET shouldn't require this level?)
- if not has_access(request.user, updates_location):
+ if not has_course_access(request.user, updates_location):
raise PermissionDenied()
if request.method == 'GET':
diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py
index f532734dafed9425e00edda24d9e4eed57dd77dd..1bb8fea4a6d50664683c4c9dbc534cba12b580e3 100644
--- a/cms/djangoapps/contentstore/views/import_export.py
+++ b/cms/djangoapps/contentstore/views/import_export.py
@@ -22,7 +22,6 @@ from django.views.decorators.http import require_http_methods, require_GET
from django.utils.translation import ugettext as _
from edxmako.shortcuts import render_to_response
-from auth.authz import create_all_course_groups
from xmodule.modulestore.xml_importer import import_from_xml
from xmodule.contentstore.django import contentstore
@@ -31,10 +30,12 @@ from xmodule.modulestore.django import modulestore, loc_mapper
from xmodule.exceptions import SerializationError
from xmodule.modulestore.locator import BlockUsageLocator
-from .access import has_access
+from .access import has_course_access
from util.json_request import JsonResponse
from extract_tar import safetar_extractall
+from student.roles import CourseInstructorRole, CourseStaffRole
+from student import auth
__all__ = ['import_handler', 'import_status_handler', 'export_handler']
@@ -61,7 +62,7 @@ def import_handler(request, tag=None, package_id=None, branch=None, version_guid
json: import a course via the .tar.gz file specified in request.FILES
"""
location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, location):
+ if not has_course_access(request.user, location):
raise PermissionDenied()
old_location = loc_mapper().translate_locator_to_location(location)
@@ -225,13 +226,15 @@ def import_handler(request, tag=None, package_id=None, branch=None, version_guid
draft_store=modulestore()
)
- logging.debug('new course at {0}'.format(course_items[0].location))
+ new_location = course_items[0].location
+ logging.debug('new course at {0}'.format(new_location))
session_status[key] = 3
request.session.modified = True
- create_all_course_groups(request.user, course_items[0].location)
- logging.debug('created all course groups at {0}'.format(course_items[0].location))
+ auth.add_users(request.user, CourseInstructorRole(new_location), request.user)
+ auth.add_users(request.user, CourseStaffRole(new_location), request.user)
+ logging.debug('created all course groups at {0}'.format(new_location))
# Send errors to client with stage at which error occured.
except Exception as exception: # pylint: disable=W0703
@@ -272,7 +275,7 @@ def import_status_handler(request, tag=None, package_id=None, branch=None, versi
"""
location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, location):
+ if not has_course_access(request.user, location):
raise PermissionDenied()
try:
@@ -303,7 +306,7 @@ def export_handler(request, tag=None, package_id=None, branch=None, version_guid
which describes the error.
"""
location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, location):
+ if not has_course_access(request.user, location):
raise PermissionDenied()
old_location = loc_mapper().translate_locator_to_location(location)
diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py
index d6a8ff3abb4ba67efefa7d85607c70572548acca..9c1fcbad6002ccf10767478fad0ee630ba4339be 100644
--- a/cms/djangoapps/contentstore/views/item.py
+++ b/cms/djangoapps/contentstore/views/item.py
@@ -28,7 +28,7 @@ from ..transcripts_utils import manage_video_subtitles_save
from ..utils import get_modulestore
-from .access import has_access
+from .access import has_course_access
from .helpers import _xmodule_recurse
from preview import handler_prefix, get_preview_html
from edxmako.shortcuts import render_to_response, render_to_string
@@ -81,7 +81,7 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid
"""
if package_id is not None:
locator = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, locator):
+ if not has_course_access(request.user, locator):
raise PermissionDenied()
old_location = loc_mapper().translate_locator_to_location(locator)
@@ -250,7 +250,7 @@ def _create_item(request):
display_name = request.json.get('display_name')
- if not has_access(request.user, parent_location):
+ if not has_course_access(request.user, parent_location):
raise PermissionDenied()
parent = get_modulestore(category).get_item(parent_location)
@@ -335,7 +335,7 @@ def orphan_handler(request, tag=None, package_id=None, branch=None, version_guid
# DHM: when split becomes back-end, move or conditionalize this conversion
old_location = loc_mapper().translate_locator_to_location(location)
if request.method == 'GET':
- if has_access(request.user, old_location):
+ if has_course_access(request.user, old_location):
return JsonResponse(modulestore().get_orphans(old_location, DETACHED_CATEGORIES, 'draft'))
else:
raise PermissionDenied()
diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py
index e4f9985b3a8b90937d5e4dd1f73e631273695b6b..d917fbbc99435784f9d7516b749c187ab707583d 100644
--- a/cms/djangoapps/contentstore/views/tabs.py
+++ b/cms/djangoapps/contentstore/views/tabs.py
@@ -1,7 +1,7 @@
"""
Views related to course tabs
"""
-from access import has_access
+from access import has_course_access
from util.json_request import expect_json, JsonResponse
from django.http import HttpResponseNotFound
@@ -63,7 +63,7 @@ def tabs_handler(request, tag=None, package_id=None, branch=None, version_guid=N
Instead use the general xblock URL (see item.xblock_handler).
"""
locator = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, locator):
+ if not has_course_access(request.user, locator):
raise PermissionDenied()
old_location = loc_mapper().translate_locator_to_location(locator)
diff --git a/cms/djangoapps/contentstore/views/transcripts_ajax.py b/cms/djangoapps/contentstore/views/transcripts_ajax.py
index 9d5cdf8e80f7b90ab40b769b0676401aa715735d..28a674cb89b7317b29d3ac5073e6fe957ebb2e6d 100644
--- a/cms/djangoapps/contentstore/views/transcripts_ajax.py
+++ b/cms/djangoapps/contentstore/views/transcripts_ajax.py
@@ -37,7 +37,7 @@ from ..transcripts_utils import (
TranscriptsRequestValidationException
)
-from .access import has_access
+from .access import has_course_access
__all__ = [
'upload_transcripts',
@@ -546,11 +546,11 @@ def _get_item(request, data):
locator = BlockUsageLocator(data.get('locator'))
old_location = loc_mapper().translate_locator_to_location(locator)
- # This is placed before has_access() to validate the location,
- # because has_access() raises InvalidLocationError if location is invalid.
+ # This is placed before has_course_access() to validate the location,
+ # because has_course_access() raises InvalidLocationError if location is invalid.
item = modulestore().get_item(old_location)
- if not has_access(request.user, locator):
+ if not has_course_access(request.user, locator):
raise PermissionDenied()
return item
diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py
index 02327bb82277dd807f45eb319d00a6e426818fef..468263d8d67219458a211f974c968d9f655be756 100644
--- a/cms/djangoapps/contentstore/views/user.py
+++ b/cms/djangoapps/contentstore/views/user.py
@@ -1,6 +1,5 @@
-import json
from django.core.exceptions import PermissionDenied
-from django.contrib.auth.models import User, Group
+from django.contrib.auth.models import User
from django.contrib.auth.decorators import login_required
from django.views.decorators.http import require_http_methods
from django.utils.translation import ugettext as _
@@ -10,17 +9,15 @@ from edxmako.shortcuts import render_to_response
from xmodule.modulestore.django import modulestore, loc_mapper
from util.json_request import JsonResponse, expect_json
-from auth.authz import (
- STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME, get_course_groupname_for_role,
- get_course_role_users
-)
+from student.roles import CourseRole, CourseInstructorRole, CourseStaffRole, GlobalStaff
from course_creators.views import user_requested_access
-from .access import has_access
+from .access import has_course_access
from student.models import CourseEnrollment
from xmodule.modulestore.locator import BlockUsageLocator
from django.http import HttpResponseNotFound
+from student import auth
__all__ = ['request_course_creator', 'course_team_handler']
@@ -53,7 +50,7 @@ def course_team_handler(request, tag=None, package_id=None, branch=None, version
json: remove a particular course team member from the course team (email is required).
"""
location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
- if not has_access(request.user, location):
+ if not has_course_access(request.user, location):
raise PermissionDenied()
if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
@@ -71,19 +68,19 @@ def _manage_users(request, locator):
old_location = loc_mapper().translate_locator_to_location(locator)
# check that logged in user has permissions to this item
- if not has_access(request.user, locator):
+ if not has_course_access(request.user, locator):
raise PermissionDenied()
course_module = modulestore().get_item(old_location)
- instructors = get_course_role_users(locator, INSTRUCTOR_ROLE_NAME)
+ instructors = CourseInstructorRole(locator).users_with_role()
# the page only lists staff and assumes they're a superset of instructors. Do a union to ensure.
- staff = set(get_course_role_users(locator, STAFF_ROLE_NAME)).union(instructors)
+ staff = set(CourseStaffRole(locator).users_with_role()).union(instructors)
return render_to_response('manage_users.html', {
'context_course': course_module,
'staff': staff,
'instructors': instructors,
- 'allow_actions': has_access(request.user, locator, role=INSTRUCTOR_ROLE_NAME),
+ 'allow_actions': has_course_access(request.user, locator, role=CourseInstructorRole),
})
@@ -93,10 +90,10 @@ def _course_team_user(request, locator, email):
Handle the add, remove, promote, demote requests ensuring the requester has authority
"""
# check that logged in user has permissions to this item
- if has_access(request.user, locator, role=INSTRUCTOR_ROLE_NAME):
+ if has_course_access(request.user, locator, role=CourseInstructorRole):
# instructors have full permissions
pass
- elif has_access(request.user, locator, role=STAFF_ROLE_NAME) and email == request.user.email:
+ elif has_course_access(request.user, locator, role=CourseStaffRole) and email == request.user.email:
# staff can only affect themselves
pass
else:
@@ -107,15 +104,13 @@ def _course_team_user(request, locator, email):
try:
user = User.objects.get(email=email)
- except:
+ except Exception:
msg = {
"error": _("Could not find user by email address '{email}'.").format(email=email),
}
return JsonResponse(msg, 404)
- # role hierarchy: "instructor" has more permissions than "staff" (in a course)
- roles = ["instructor", "staff"]
-
+ # role hierarchy: globalstaff > "instructor" > "staff" (in a course)
if request.method == "GET":
# just return info about the user
msg = {
@@ -123,12 +118,10 @@ def _course_team_user(request, locator, email):
"active": user.is_active,
"role": None,
}
- # what's the highest role that this user has?
- groupnames = set(g.name for g in user.groups.all())
- for role in roles:
- role_groupname = get_course_groupname_for_role(locator, role)
- if role_groupname in groupnames:
- msg["role"] = role
+ # what's the highest role that this user has? (How should this report global staff?)
+ for role in [CourseInstructorRole(locator), CourseStaffRole(locator)]:
+ if role.has_user(user):
+ msg["role"] = role.ROLE
break
return JsonResponse(msg)
@@ -139,29 +132,20 @@ def _course_team_user(request, locator, email):
}
return JsonResponse(msg, 400)
- # make sure that the role groups exist
- groups = {}
- for role in roles:
- groupname = get_course_groupname_for_role(locator, role)
- group, __ = Group.objects.get_or_create(name=groupname)
- groups[role] = group
-
if request.method == "DELETE":
# remove all roles in this course from this user: but fail if the user
# is the last instructor in the course team
- instructors = set(groups["instructor"].user_set.all())
- staff = set(groups["staff"].user_set.all())
- if user in instructors and len(instructors) == 1:
- msg = {
- "error": _("You may not remove the last instructor from a course")
- }
- return JsonResponse(msg, 400)
+ instructors = CourseInstructorRole(locator)
+ if instructors.has_user(user):
+ if instructors.users_with_role().count() == 1:
+ msg = {
+ "error": _("You may not remove the last instructor from a course")
+ }
+ return JsonResponse(msg, 400)
+ else:
+ instructors.remove_users(request.user, user)
- if user in instructors:
- user.groups.remove(groups["instructor"])
- if user in staff:
- user.groups.remove(groups["staff"])
- user.save()
+ auth.remove_users(request.user, CourseStaffRole(locator), user)
return JsonResponse()
# all other operations require the requesting user to specify a role
@@ -171,28 +155,30 @@ def _course_team_user(request, locator, email):
old_location = loc_mapper().translate_locator_to_location(locator)
if role == "instructor":
- if not has_access(request.user, locator, role=INSTRUCTOR_ROLE_NAME):
+ if not has_course_access(request.user, locator, role=CourseInstructorRole):
msg = {
"error": _("Only instructors may create other instructors")
}
return JsonResponse(msg, 400)
- user.groups.add(groups["instructor"])
- user.save()
+ auth.add_users(request.user, CourseInstructorRole(locator), user)
# auto-enroll the course creator in the course so that "View Live" will work.
CourseEnrollment.enroll(user, old_location.course_id)
elif role == "staff":
+ # add to staff regardless (can't do after removing from instructors as will no longer
+ # be allowed)
+ auth.add_users(request.user, CourseStaffRole(locator), user)
# if we're trying to downgrade a user from "instructor" to "staff",
# make sure we have at least one other instructor in the course team.
- instructors = set(groups["instructor"].user_set.all())
- if user in instructors:
- if len(instructors) == 1:
+ instructors = CourseInstructorRole(locator)
+ if instructors.has_user(user):
+ if instructors.users_with_role().count() == 1:
msg = {
"error": _("You may not remove the last instructor from a course")
}
return JsonResponse(msg, 400)
- user.groups.remove(groups["instructor"])
- user.groups.add(groups["staff"])
- user.save()
+ else:
+ instructors.remove_users(request.user, user)
+
# auto-enroll the course creator in the course so that "View Live" will work.
CourseEnrollment.enroll(user, old_location.course_id)
diff --git a/cms/djangoapps/course_creators/tests/test_admin.py b/cms/djangoapps/course_creators/tests/test_admin.py
index 4d28f263997effafce6e9cc58b75eef555585352..ec27053444c159b153c78e5b3c78a8177e1fbbbe 100644
--- a/cms/djangoapps/course_creators/tests/test_admin.py
+++ b/cms/djangoapps/course_creators/tests/test_admin.py
@@ -10,8 +10,9 @@ import mock
from course_creators.admin import CourseCreatorAdmin
from course_creators.models import CourseCreator
-from auth.authz import is_user_in_creator_group
from django.core import mail
+from student.roles import CourseCreatorRole
+from student import auth
def mock_render_to_string(template_name, context):
@@ -54,7 +55,7 @@ class CourseCreatorAdminTest(TestCase):
def change_state_and_verify_email(state, is_creator):
""" Changes user state, verifies creator status, and verifies e-mail is sent based on transition """
self._change_state(state)
- self.assertEqual(is_creator, is_user_in_creator_group(self.user))
+ self.assertEqual(is_creator, auth.has_access(self.user, CourseCreatorRole()))
context = {'studio_request_email': self.studio_request_email}
if state == CourseCreator.GRANTED:
@@ -72,7 +73,7 @@ class CourseCreatorAdminTest(TestCase):
with mock.patch.dict('django.conf.settings.FEATURES', self.enable_creator_group_patch):
# User is initially unrequested.
- self.assertFalse(is_user_in_creator_group(self.user))
+ self.assertFalse(auth.has_access(self.user, CourseCreatorRole()))
change_state_and_verify_email(CourseCreator.GRANTED, True)
diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py
index dbd92365b7deb8e88ab39245b98b72ea5c6cbac3..c88d0df72ac1cebd0327afce2d8e8dce66152fee 100644
--- a/cms/djangoapps/course_creators/tests/test_views.py
+++ b/cms/djangoapps/course_creators/tests/test_views.py
@@ -8,9 +8,9 @@ from django.core.exceptions import PermissionDenied
from course_creators.views import add_user_with_status_unrequested, add_user_with_status_granted
from course_creators.views import get_course_creator_status, update_course_creator_group, user_requested_access
-from course_creators.models import CourseCreator
-from auth.authz import is_user_in_creator_group
import mock
+from student.roles import CourseCreatorRole
+from student import auth
class CourseCreatorView(TestCase):
@@ -48,7 +48,7 @@ class CourseCreatorView(TestCase):
def test_add_granted(self):
with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}):
# Calling add_user_with_status_granted impacts is_user_in_course_group_role.
- self.assertFalse(is_user_in_creator_group(self.user))
+ self.assertFalse(auth.has_access(self.user, CourseCreatorRole()))
add_user_with_status_granted(self.admin, self.user)
self.assertEqual('granted', get_course_creator_status(self.user))
@@ -57,15 +57,15 @@ class CourseCreatorView(TestCase):
add_user_with_status_unrequested(self.user)
self.assertEqual('granted', get_course_creator_status(self.user))
- self.assertTrue(is_user_in_creator_group(self.user))
+ self.assertTrue(auth.has_access(self.user, CourseCreatorRole()))
def test_update_creator_group(self):
with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}):
- self.assertFalse(is_user_in_creator_group(self.user))
+ self.assertFalse(auth.has_access(self.user, CourseCreatorRole()))
update_course_creator_group(self.admin, self.user, True)
- self.assertTrue(is_user_in_creator_group(self.user))
+ self.assertTrue(auth.has_access(self.user, CourseCreatorRole()))
update_course_creator_group(self.admin, self.user, False)
- self.assertFalse(is_user_in_creator_group(self.user))
+ self.assertFalse(auth.has_access(self.user, CourseCreatorRole()))
def test_user_requested_access(self):
add_user_with_status_unrequested(self.user)
diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py
index e9b38ed16951d9a3d7249ef19b42d1e6b7b4ab07..c0c1b4a4528cd47dd888033dc71060976d8e131f 100644
--- a/cms/djangoapps/course_creators/views.py
+++ b/cms/djangoapps/course_creators/views.py
@@ -2,8 +2,8 @@
Methods for interacting programmatically with the user creator table.
"""
from course_creators.models import CourseCreator
-
-from auth.authz import add_user_to_creator_group, remove_user_from_creator_group
+from student.roles import CourseCreatorRole
+from student import auth
def add_user_with_status_unrequested(user):
@@ -43,9 +43,9 @@ def update_course_creator_group(caller, user, add):
Caller must have staff permissions.
"""
if add:
- add_user_to_creator_group(caller, user)
+ auth.add_users(caller, CourseCreatorRole(), user)
else:
- remove_user_from_creator_group(caller, user)
+ auth.remove_users(caller, CourseCreatorRole(), user)
def get_course_creator_status(user):
diff --git a/cms/envs/common.py b/cms/envs/common.py
index eb7d600c209ddc7d0743eb69622d5b90ebf76638..999b344332648fd595645876ffc8fda0c5a21c7e 100644
--- a/cms/envs/common.py
+++ b/cms/envs/common.py
@@ -203,7 +203,7 @@ TEMPLATE_DEBUG = False
# Site info
SITE_ID = 1
-SITE_NAME = "localhost:8000"
+SITE_NAME = "localhost:8001"
HTTPS = 'on'
ROOT_URLCONF = 'cms.urls'
IGNORABLE_404_ENDS = ('favicon.ico')
diff --git a/cms/templates/manage_users.html b/cms/templates/manage_users.html
index 6e7e8c80125ce6822f8aba0e68b4300f7cb2d3df..5ad8aaea26c8a1b1c42d1537524f7e5e0326c6f7 100644
--- a/cms/templates/manage_users.html
+++ b/cms/templates/manage_users.html
@@ -1,7 +1,8 @@
+<%! import json %>
<%! from django.utils.translation import ugettext as _ %>
<%! from django.core.urlresolvers import reverse %>
-<%! from auth.authz import is_user_in_course_group_role %>
-<%! import json %>
+<%! from student.roles import CourseInstructorRole %>
+<%! from student.auth import has_access %>
<%! from xmodule.modulestore.django import loc_mapper %>
<%inherit file="base.html" />
<%block name="title">${_("Course Team Settings")}</%block>
@@ -65,7 +66,7 @@
<li class="user-item" data-email="${user.email}" data-url="${new_location.url_reverse('course_team/', user.email) }">
- <% is_instuctor = is_user_in_course_group_role(user, context_course.location, 'instructor', check_staff=False) %>
+ <% is_instuctor = has_access(user, CourseInstructorRole(context_course.location)) %>
% if is_instuctor:
<span class="wrapper-ui-badge">
<span class="flag flag-role flag-role-admin is-hanging">
@@ -120,7 +121,7 @@
% endfor
</ol>
- <% user_is_instuctor = is_user_in_course_group_role(request.user, context_course.location, 'instructor', check_staff=False) %>
+ <% user_is_instuctor = has_access(request.user, CourseInstructorRole(context_course.location)) %>
% if user_is_instuctor and len(staff) == 1:
<div class="notice notice-incontext notice-create has-actions">
<div class="msg">
diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py
new file mode 100644
index 0000000000000000000000000000000000000000..97583f69dc300af65906cfd4565549cecf78dccf
--- /dev/null
+++ b/common/djangoapps/student/auth.py
@@ -0,0 +1,86 @@
+"""
+The application interface to roles which checks whether any user trying to change
+authorization has authorization to do so, which infers authorization via role hierarchy
+(GlobalStaff is superset of auths of course instructor, ...), which consults the config
+to decide whether to check course creator role, and other such functions.
+"""
+from django.core.exceptions import PermissionDenied
+from django.conf import settings
+
+from student.roles import GlobalStaff, CourseCreatorRole, CourseStaffRole, CourseInstructorRole, CourseRole, \
+ CourseBetaTesterRole
+
+
+def has_access(user, role):
+ """
+ Check whether this user has access to this role (either direct or implied)
+ :param user:
+ :param role: an AccessRole
+ """
+ if not user.is_active:
+ return False
+ # do cheapest check first even tho it's not the direct one
+ if GlobalStaff().has_user(user):
+ return True
+ # CourseCreator is odd b/c it can be disabled via config
+ if isinstance(role, CourseCreatorRole):
+ # completely shut down course creation setting
+ if settings.FEATURES.get('DISABLE_COURSE_CREATION', False):
+ return False
+ # wide open course creation setting
+ if not settings.FEATURES.get('ENABLE_CREATOR_GROUP', False):
+ return True
+
+ if role.has_user(user):
+ return True
+ # if not, then check inferred permissions
+ if (isinstance(role, (CourseStaffRole, CourseBetaTesterRole)) and
+ CourseInstructorRole(role.location).has_user(user)):
+ return True
+ return False
+
+
+def add_users(caller, role, *users):
+ """
+ The caller requests adding the given users to the role. Checks that the caller
+ has sufficient authority.
+
+ :param caller: a user
+ :param role: an AccessRole
+ """
+ _check_caller_authority(caller, role)
+ role.add_users(*users)
+
+
+def remove_users(caller, role, *users):
+ """
+ The caller requests removing the given users from the role. Checks that the caller
+ has sufficient authority.
+
+ :param caller: a user
+ :param role: an AccessRole
+ """
+ # can always remove self (at this layer)
+ if not(len(users) == 1 and caller == users[0]):
+ _check_caller_authority(caller, role)
+ role.remove_users(*users)
+
+
+def _check_caller_authority(caller, role):
+ """
+ Internal function to check whether the caller has authority to manipulate this role
+ :param caller: a user
+ :param role: an AccessRole
+ """
+ if not (caller.is_authenticated and caller.is_active):
+ raise PermissionDenied
+ # superuser
+ if GlobalStaff().has_user(caller):
+ return
+
+ if isinstance(role, (GlobalStaff, CourseCreatorRole)):
+ raise PermissionDenied
+ elif isinstance(role, CourseRole): # instructors can change the roles w/in their course
+ if not has_access(caller, CourseInstructorRole(role.location)):
+ raise PermissionDenied
+
diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py
index d99a5ecb9136f3a5fd685bf6ea13e5cf00a94211..9da258e19beacc7d801afb1f61020061d8585ef8 100644
--- a/common/djangoapps/student/roles.py
+++ b/common/djangoapps/student/roles.py
@@ -64,11 +64,13 @@ class GlobalStaff(AccessRole):
def add_users(self, *users):
for user in users:
- user.is_staff = True
- user.save()
+ if (user.is_authenticated and user.is_active):
+ user.is_staff = True
+ user.save()
def remove_users(self, *users):
for user in users:
+ # don't check is_authenticated nor is_active on purpose
user.is_staff = False
user.save()
@@ -96,10 +98,10 @@ class GroupBasedRole(AccessRole):
"""
Return whether the supplied django user has access to this role.
"""
- # pylint: disable=protected-access
- if not user.is_authenticated():
+ if not (user.is_authenticated and user.is_active):
return False
+ # pylint: disable=protected-access
if not hasattr(user, '_groups'):
user._groups = set(name.lower() for name in user.groups.values_list('name', flat=True))
@@ -109,8 +111,12 @@ class GroupBasedRole(AccessRole):
"""
Add the supplied django users to this role.
"""
+ # silently ignores anonymous and inactive users so that any that are
+ # legit get updated.
+ users = [user for user in users if user.is_authenticated and user.is_active]
group, _ = Group.objects.get_or_create(name=self._group_names[0])
group.user_set.add(*users)
+ # remove cache
for user in users:
if hasattr(user, '_groups'):
del user._groups
@@ -119,8 +125,10 @@ class GroupBasedRole(AccessRole):
"""
Remove the supplied django users from this role.
"""
- group, _ = Group.objects.get_or_create(name=self._group_names[0])
- group.user_set.remove(*users)
+ groups = Group.objects.filter(name__in=self._group_names)
+ for group in groups:
+ group.user_set.remove(*users)
+ # remove cache
for user in users:
if hasattr(user, '_groups'):
del user._groups
@@ -144,31 +152,33 @@ class CourseRole(GroupBasedRole):
"""
# TODO: figure out how to make the group name generation lazy so it doesn't force the
# loc mapping?
- location = Locator.to_locator_or_location(location)
+ self.location = Locator.to_locator_or_location(location)
+ self.role = role
# direct copy from auth.authz.get_all_course_role_groupnames will refactor to one impl asap
groupnames = []
# pylint: disable=no-member
- if isinstance(location, Location):
+ if isinstance(self.location, Location):
try:
- groupnames.append('{0}_{1}'.format(role, location.course_id))
+ groupnames.append('{0}_{1}'.format(role, self.location.course_id))
+ course_context = self.location.course_id # course_id is valid for translation
except InvalidLocationError: # will occur on old locations where location is not of category course
if course_context is None:
raise CourseContextRequired()
else:
groupnames.append('{0}_{1}'.format(role, course_context))
try:
- locator = loc_mapper().translate_location(location.course_id, location, False, False)
+ locator = loc_mapper().translate_location(course_context, self.location, False, False)
groupnames.append('{0}_{1}'.format(role, locator.package_id))
except (InvalidLocationError, ItemNotFoundError):
# if it's never been mapped, the auth won't be via the Locator syntax
pass
# least preferred legacy role_course format
- groupnames.append('{0}_{1}'.format(role, location.course))
- elif isinstance(location, CourseLocator):
- groupnames.append('{0}_{1}'.format(role, location.package_id))
+ groupnames.append('{0}_{1}'.format(role, self.location.course))
+ elif isinstance(self.location, CourseLocator):
+ groupnames.append('{0}_{1}'.format(role, self.location.package_id))
# handle old Location syntax
- old_location = loc_mapper().translate_locator_to_location(location, get_course=True)
+ old_location = loc_mapper().translate_locator_to_location(self.location, get_course=True)
if old_location:
# the slashified version of the course_id (myu/mycourse/myrun)
groupnames.append('{0}_{1}'.format(role, old_location.course_id))
@@ -191,20 +201,23 @@ class OrgRole(GroupBasedRole):
class CourseStaffRole(CourseRole):
"""A Staff member of a course"""
+ ROLE = 'staff'
def __init__(self, *args, **kwargs):
- super(CourseStaffRole, self).__init__('staff', *args, **kwargs)
+ super(CourseStaffRole, self).__init__(self.ROLE, *args, **kwargs)
class CourseInstructorRole(CourseRole):
"""A course Instructor"""
+ ROLE = 'instructor'
def __init__(self, *args, **kwargs):
- super(CourseInstructorRole, self).__init__('instructor', *args, **kwargs)
+ super(CourseInstructorRole, self).__init__(self.ROLE, *args, **kwargs)
class CourseBetaTesterRole(CourseRole):
"""A course Beta Tester"""
+ ROLE = 'beta_testers'
def __init__(self, *args, **kwargs):
- super(CourseBetaTesterRole, self).__init__('beta_testers', *args, **kwargs)
+ super(CourseBetaTesterRole, self).__init__(self.ROLE, *args, **kwargs)
class OrgStaffRole(OrgRole):
@@ -217,3 +230,13 @@ class OrgInstructorRole(OrgRole):
"""An organization instructor"""
def __init__(self, *args, **kwargs):
super(OrgInstructorRole, self).__init__('instructor', *args, **kwargs)
+
+
+class CourseCreatorRole(GroupBasedRole):
+ """
+ This is the group of people who have permission to create new courses (we may want to eventually
+ make this an org based role).
+ """
+ ROLE = "course_creator_group"
+ def __init__(self, *args, **kwargs):
+ super(CourseCreatorRole, self).__init__(self.ROLE, *args, **kwargs)
diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py
index 952d017283e1a03c1362f294efe023eadd964671..d20d532c97002632357a17c1108101680fae9c26 100644
--- a/lms/djangoapps/bulk_email/tests/test_email.py
+++ b/lms/djangoapps/bulk_email/tests/test_email.py
@@ -53,7 +53,8 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase):
self.instructor = InstructorFactory(course=self.course.location)
# Create staff
- self.staff = [StaffFactory(course=self.course.location) for _ in xrange(STAFF_COUNT)]
+ self.staff = [StaffFactory(course=self.course.location)
+ for _ in xrange(STAFF_COUNT)]
# Create students
self.students = [UserFactory() for _ in xrange(STUDENT_COUNT)]
diff --git a/lms/djangoapps/course_wiki/tests/test_access.py b/lms/djangoapps/course_wiki/tests/test_access.py
index 72a0e7dcbd891b51e2d4b590f90fa7dd3d0fb305..aca2223d13274f0ecd358fdb5cf722525df3ac9d 100644
--- a/lms/djangoapps/course_wiki/tests/test_access.py
+++ b/lms/djangoapps/course_wiki/tests/test_access.py
@@ -25,7 +25,10 @@ class TestWikiAccessBase(ModuleStoreTestCase):
self.wiki = get_or_create_root()
self.course_math101 = CourseFactory.create(org='org', number='math101', display_name='Course')
- self.course_math101_staff = [InstructorFactory(course=self.course_math101.location), StaffFactory(course=self.course_math101.location)]
+ self.course_math101_staff = [
+ InstructorFactory(course=self.course_math101.location),
+ StaffFactory(course=self.course_math101.location)
+ ]
wiki_math101 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101))
wiki_math101_page = self.create_urlpath(wiki_math101, 'Child')
@@ -44,9 +47,15 @@ class TestWikiAccess(TestWikiAccessBase):
super(TestWikiAccess, self).setUp()
self.course_310b = CourseFactory.create(org='org', number='310b', display_name='Course')
- self.course_310b_staff = [InstructorFactory(course=self.course_310b.location), StaffFactory(course=self.course_310b.location)]
+ self.course_310b_staff = [
+ InstructorFactory(course=self.course_310b.location),
+ StaffFactory(course=self.course_310b.location)
+ ]
self.course_310b_ = CourseFactory.create(org='org', number='310b_', display_name='Course')
- self.course_310b__staff = [InstructorFactory(course=self.course_310b_.location), StaffFactory(course=self.course_310b_.location)]
+ self.course_310b__staff = [
+ InstructorFactory(course=self.course_310b_.location),
+ StaffFactory(course=self.course_310b_.location)
+ ]
self.wiki_310b = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b))
self.wiki_310b_ = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b_))
@@ -105,7 +114,10 @@ class TestWikiAccessForNumericalCourseNumber(TestWikiAccessBase):
super(TestWikiAccessForNumericalCourseNumber, self).setUp()
self.course_200 = CourseFactory.create(org='org', number='200', display_name='Course')
- self.course_200_staff = [InstructorFactory(course=self.course_200.location), StaffFactory(course=self.course_200.location)]
+ self.course_200_staff = [
+ InstructorFactory(course=self.course_200.location),
+ StaffFactory(course=self.course_200.location)
+ ]
wiki_200 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_200))
wiki_200_page = self.create_urlpath(wiki_200, 'Child')
@@ -126,7 +138,10 @@ class TestWikiAccessForOldFormatCourseStaffGroups(TestWikiAccessBase):
self.course_math101c = CourseFactory.create(org='org', number='math101c', display_name='Course')
Group.objects.get_or_create(name='instructor_math101c')
- self.course_math101c_staff = [InstructorFactory(course=self.course_math101c.location), StaffFactory(course=self.course_math101c.location)]
+ self.course_math101c_staff = [
+ InstructorFactory(course=self.course_math101c.location),
+ StaffFactory(course=self.course_math101c.location)
+ ]
wiki_math101c = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101c))
wiki_math101c_page = self.create_urlpath(wiki_math101c, 'Child')
diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py
index acb307e4650e6940a450966c4ca5185ef6c513b5..acc39c500107e3462ef3b4f3f4281643436f951d 100644
--- a/lms/djangoapps/courseware/tests/test_view_authentication.py
+++ b/lms/djangoapps/courseware/tests/test_view_authentication.py
@@ -3,7 +3,6 @@ import pytz
from mock import patch
-from django.contrib.auth.models import User
from django.core.urlresolvers import reverse
from django.test.utils import override_settings
@@ -128,6 +127,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
display_name='Welcome'
)
+ self.global_staff_user = GlobalStaffFactory()
self.unenrolled_user = UserFactory(last_name="Unenrolled")
self.enrolled_user = UserFactory(last_name="Enrolled")
@@ -135,10 +135,11 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
CourseEnrollmentFactory(user=self.enrolled_user, course_id=self.test_course.id)
self.staff_user = StaffFactory(course=self.course.location)
- self.instructor_user = InstructorFactory(course=self.course.location)
+ self.instructor_user = InstructorFactory(
+ course=self.course.location)
self.org_staff_user = OrgStaffFactory(course=self.course.location)
- self.org_instructor_user = OrgInstructorFactory(course=self.course.location)
- self.global_staff_user = GlobalStaffFactory()
+ self.org_instructor_user = OrgInstructorFactory(
+ course=self.course.location)
def test_redirection_unenrolled(self):
"""
diff --git a/lms/djangoapps/instructor/tests/test_legacy_anon_csv.py b/lms/djangoapps/instructor/tests/test_legacy_anon_csv.py
index 7f05023b30163ad9ae9ca21828068adf661f3b72..b3a329ee05095e3db34d8845fd3ee798b421d755 100644
--- a/lms/djangoapps/instructor/tests/test_legacy_anon_csv.py
+++ b/lms/djangoapps/instructor/tests/test_legacy_anon_csv.py
@@ -44,11 +44,7 @@ class TestInstructorDashboardAnonCSV(ModuleStoreTestCase, LoginEnrollmentTestCas
self.activate_user(self.student)
self.activate_user(self.instructor)
- def make_instructor(course):
- """ Create an instructor for the course."""
- CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
-
- make_instructor(self.toy)
+ CourseStaffRole(self.toy.location).add_users(User.objects.get(email=self.instructor))
self.logout()
self.login(self.instructor, self.password)
diff --git a/lms/djangoapps/instructor/tests/test_legacy_download_csv.py b/lms/djangoapps/instructor/tests/test_legacy_download_csv.py
index d6bcb7a065022d6d61deaad37fc173999694bf38..a03f029ff3e49575cfd37bf5a7f58307a8df47be 100644
--- a/lms/djangoapps/instructor/tests/test_legacy_download_csv.py
+++ b/lms/djangoapps/instructor/tests/test_legacy_download_csv.py
@@ -41,11 +41,7 @@ class TestInstructorDashboardGradeDownloadCSV(ModuleStoreTestCase, LoginEnrollme
self.activate_user(self.student)
self.activate_user(self.instructor)
- def make_instructor(course):
- """ Create an instructor for the course. """
- CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
-
- make_instructor(self.toy)
+ CourseStaffRole(self.toy.location).add_users(User.objects.get(email=self.instructor))
self.logout()
self.login(self.instructor, self.password)