Formalize various access roles as objects

This centralizes the logic for group membership, and means that we
don't need to make queries to find out whether the legacy groups names exist.

common/djangoapps/student/tests/factories.py

@@ -4,9 +4,17 @@ from student.models import (User, UserProfile, Registration,
                             PendingEmailChange, UserStanding,
                             )
 from course_modes.models import CourseMode
-from django.contrib.auth.models import Group
+from django.contrib.auth.models import Group, AnonymousUser
 from datetime import datetime
-from factory import DjangoModelFactory, SubFactory, PostGenerationMethodCall, post_generation, Sequence
+from factory import (
+    DjangoModelFactory,
+    Factory,
+    LazyAttribute,
+    post_generation,
+    PostGenerationMethodCall,
+    Sequence,
+    SubFactory,
+)
 from uuid import uuid4
 from pytz import UTC
 
@@ -16,8 +24,10 @@ from pytz import UTC
 
 class GroupFactory(DjangoModelFactory):
     FACTORY_FOR = Group
+    FACTORY_DJANGO_GET_OR_CREATE = ('name', )
+
+    name = Sequence(u'group{0}'.format)
 
-    name = u'staff_MITx/999/Robot_Super_Course'
 
 
 class UserStandingFactory(DjangoModelFactory):
@@ -30,9 +40,10 @@ class UserStandingFactory(DjangoModelFactory):
 
 class UserProfileFactory(DjangoModelFactory):
     FACTORY_FOR = UserProfile
+    FACTORY_DJANGO_GET_OR_CREATE = ('user', )
 
     user = None
-    name = u'Robot Test'
+    name = LazyAttribute(u'{0.user.first_name} {0.user.last_name}'.format)
     level_of_education = None
     gender = u'm'
     mailing_address = None
@@ -59,6 +70,7 @@ class RegistrationFactory(DjangoModelFactory):
 
 class UserFactory(DjangoModelFactory):
     FACTORY_FOR = User
+    FACTORY_DJANGO_GET_OR_CREATE = ('email', 'username')
 
     username = Sequence(u'robot{0}'.format)
     email = Sequence(u'robot+test+{0}@edx.org'.format)
@@ -82,6 +94,21 @@ class UserFactory(DjangoModelFactory):
         else:
             return None
 
+    @post_generation
+    def groups(self, create, extracted, **kwargs):
+        if extracted is None:
+            return
+
+        if isinstance(extracted, basestring):
+            extracted = [extracted]
+
+        for group_name in extracted:
+            self.groups.add(GroupFactory.simple_generate(create, name=group_name))
+
+
+class AnonymousUserFactory(Factory):
+    FACTORY_FOR = AnonymousUser
+
 
 class AdminFactory(UserFactory):
     is_staff = True

lms/djangoapps/bulk_email/tasks.py

@@ -28,7 +28,7 @@ from celery.states import SUCCESS, FAILURE, RETRY
 from celery.exceptions import RetryTaskError
 
 from django.conf import settings
-from django.contrib.auth.models import User, Group
+from django.contrib.auth.models import User
 from django.core.mail import EmailMultiAlternatives, get_connection
 from django.core.urlresolvers import reverse
 
@@ -36,8 +36,8 @@ from bulk_email.models import (
     CourseEmail, Optout, CourseEmailTemplate,
     SEND_TO_MYSELF, SEND_TO_ALL, TO_OPTIONS,
 )
-from courseware.access import _course_staff_group_name, _course_instructor_group_name
 from courseware.courses import get_course, course_image_url
+from courseware.roles import CourseStaffRole, CourseInstructorRole
 from instructor_task.models import InstructorTask
 from instructor_task.subtasks import (
     SubtaskStatus,
@@ -106,12 +106,8 @@ def _get_recipient_queryset(user_id, to_option, course_id, course_location):
     if to_option == SEND_TO_MYSELF:
         recipient_qset = User.objects.filter(id=user_id)
     else:
-        staff_grpname = _course_staff_group_name(course_location)
-        staff_group, _ = Group.objects.get_or_create(name=staff_grpname)
-        staff_qset = staff_group.user_set.all()
-        instructor_grpname = _course_instructor_group_name(course_location)
-        instructor_group, _ = Group.objects.get_or_create(name=instructor_grpname)
-        instructor_qset = instructor_group.user_set.all()
+        staff_qset = CourseStaffRole(course_location).users_with_role()
+        instructor_qset = CourseInstructorRole(course_location).users_with_role()
         recipient_qset = staff_qset | instructor_qset
         if to_option == SEND_TO_ALL:
             # We also require students to have activated their accounts to

lms/djangoapps/bulk_email/tests/test_email.py

@@ -50,10 +50,10 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase):
     def setUp(self):
         self.course = CourseFactory.create()
 
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
 
         # Create staff
-        self.staff = [StaffFactory(self.course) 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)]

lms/djangoapps/course_wiki/tests/test_access.py

@@ -25,7 +25,7 @@ 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(self.course_math101), StaffFactory(self.course_math101)]
+        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 +44,9 @@ class TestWikiAccess(TestWikiAccessBase):
         super(TestWikiAccess, self).setUp()
 
         self.course_310b = CourseFactory.create(org='org', number='310b', display_name='Course')
-        self.course_310b_staff = [InstructorFactory(self.course_310b), StaffFactory(self.course_310b)]
+        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(self.course_310b_), StaffFactory(self.course_310b_)]
+        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 +105,7 @@ class TestWikiAccessForNumericalCourseNumber(TestWikiAccessBase):
         super(TestWikiAccessForNumericalCourseNumber, self).setUp()
 
         self.course_200 = CourseFactory.create(org='org', number='200', display_name='Course')
-        self.course_200_staff = [InstructorFactory(self.course_200), StaffFactory(self.course_200)]
+        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 +126,7 @@ 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(self.course_math101c), StaffFactory(self.course_math101c)]
+        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')

lms/djangoapps/courseware/roles.py

+"""
+Classes used to model the roles used in the courseware. Each role is responsible for checking membership,
+adding users, removing users, and listing members
+"""
+
+from abc import ABCMeta, abstractmethod
+from functools import partial
+
+from django.contrib.auth.models import User, Group
+
+from xmodule.modulestore import Location
+
+
+class CourseContextRequired(Exception):
+    """
+    Raised when a course_context is required to determine permissions
+    """
+    pass
+
+
+class AccessRole(object):
+    """
+    Object representing a role with particular access to a resource
+    """
+    __metaclass__ = ABCMeta
+
+    @abstractmethod
+    def has_user(self, user):  # pylint: disable=unused-argument
+        """
+        Return whether the supplied django user has access to this role.
+        """
+        return False
+
+    @abstractmethod
+    def add_users(self, *users):
+        """
+        Add the role to the supplied django users.
+        """
+        pass
+
+    @abstractmethod
+    def remove_users(self, *users):
+        """
+        Remove the role from the supplied django users.
+        """
+        pass
+
+    @abstractmethod
+    def users_with_role(self):
+        """
+        Return a django QuerySet for all of the users with this role
+        """
+        return User.objects.none()
+
+
+class GlobalStaff(AccessRole):
+    """
+    The global staff role
+    """
+    def has_user(self, user):
+        return user.is_staff
+
+    def add_users(self, *users):
+        for user in users:
+            user.is_staff = True
+            user.save()
+
+    def remove_users(self, *users):
+        for user in users:
+            user.is_staff = False
+            user.save()
+
+    def users_with_role(self):
+        raise Exception("This operation is un-indexed, and shouldn't be used")
+
+
+class GroupBasedRole(AccessRole):
+    """
+    A role based on membership to any of a set of groups.
+    """
+    def __init__(self, group_names):
+        """
+        Create a GroupBasedRole from a list of group names
+
+        The first element of `group_names` will be the preferred group
+        to use when adding a user to this Role.
+
+        If a user is a member of any of the groups in the list, then
+        they will be consider a member of the Role
+        """
+        self._group_names = [name.lower() for name in group_names]
+
+    def has_user(self, user):
+        """
+        Return whether the supplied django user has access to this role.
+        """
+        # pylint: disable=protected-access
+        if not user.is_authenticated():
+            return False
+
+        if not hasattr(user, '_groups'):
+            user._groups = set(name.lower() for name in user.groups.values_list('name', flat=True))
+
+        return len(user._groups.intersection(self._group_names)) > 0
+
+    def add_users(self, *users):
+        """
+        Add the supplied django users to this role.
+        """
+        group, _ = Group.objects.get_or_create(name=self._group_names[0])
+        group.user_set.add(*users)
+        for user in users:
+            if hasattr(user, '_groups'):
+                del user._groups
+
+    def remove_users(self, *users):
+        """
+        Remove the supplied django users from this role.
+        """
+        group, _ = Group.objects.get_or_create(name=self._group_names[0])
+        group.user_set.remove(*users)
+        for user in users:
+            if hasattr(user, '_groups'):
+                del user._groups
+
+    def users_with_role(self):
+        """
+        Return a django QuerySet for all of the users with this role
+        """
+        return User.objects.filter(groups__name__in=self._group_names)
+
+
+class CourseRole(GroupBasedRole):
+    """
+    A named role in a particular course
+    """
+    def __init__(self, role, location, course_context=None):
+        # pylint: disable=no-member
+        loc = Location(location)
+        legacy_group_name = '{0}_{1}'.format(role, loc.course)
+
+        if loc.category.lower() == 'course':
+            course_id = loc.course_id
+        else:
+            if course_context is None:
+                raise CourseContextRequired()
+            course_id = course_context
+
+        group_name = '{0}_{1}'.format(role, course_id)
+
+        super(CourseRole, self).__init__([group_name, legacy_group_name])
+
+
+class OrgRole(GroupBasedRole):
+    """
+    A named role in a particular org
+    """
+    def __init__(self, role, location):
+        # pylint: disable=no-member
+
+        location = Location(location)
+        super(OrgRole, self).__init__(['{}_{}'.format(role, location.org)])
+
+
+class CourseStaffRole(CourseRole):
+    """A Staff member of a course"""
+    def __init__(self, *args, **kwargs):
+        super(CourseStaffRole, self).__init__('staff', *args, **kwargs)
+
+
+class CourseInstructorRole(CourseRole):
+    """A course Instructor"""
+    def __init__(self, *args, **kwargs):
+        super(CourseInstructorRole, self).__init__('instructor', *args, **kwargs)
+
+
+class CourseBetaTesterRole(CourseRole):
+    """A course Beta Tester"""
+    def __init__(self, *args, **kwargs):
+        super(CourseBetaTesterRole, self).__init__('beta_testers', *args, **kwargs)
+
+
+class OrgStaffRole(OrgRole):
+    """An organization staff member"""
+    def __init__(self, *args, **kwargs):
+        super(OrgStaffRole, self).__init__('staff', *args, **kwargs)
+
+
+class OrgInstructorRole(OrgRole):
+    """An organization staff member"""
+    def __init__(self, *args, **kwargs):
+        super(OrgInstructorRole, self).__init__('staff', *args, **kwargs)

lms/djangoapps/courseware/tests/factories.py

-from datetime import datetime
 import json
 from functools import partial
 
-from factory import DjangoModelFactory, SubFactory
-from student.tests.factories import UserFactory as StudentUserFactory
-from student.tests.factories import GroupFactory as StudentGroupFactory
+from factory import DjangoModelFactory, SubFactory, post_generation
+
+# Imported to re-export
+# pylint: disable=unused-import
+from student.tests.factories import UserFactory  # Imported to re-export
+from student.tests.factories import GroupFactory  # Imported to re-export
+from student.tests.factories import CourseEnrollmentAllowedFactory  # Imported to re-export
+from student.tests.factories import RegistrationFactory  # Imported to re-export
+# pylint: enable=unused-import
+
 from student.tests.factories import UserProfileFactory as StudentUserProfileFactory
-from student.tests.factories import CourseEnrollmentAllowedFactory as StudentCourseEnrollmentAllowedFactory
-from student.tests.factories import RegistrationFactory as StudentRegistrationFactory
 from courseware.models import StudentModule, XModuleUserStateSummaryField
 from courseware.models import XModuleStudentInfoField, XModuleStudentPrefsField
-from instructor.access import allow_access
+from courseware.roles import CourseInstructorRole, CourseStaffRole
 
 from xmodule.modulestore import Location
-from pytz import UTC
+
 
 location = partial(Location, 'i4x', 'edX', 'test_course', 'problem')
 
 
 class UserProfileFactory(StudentUserProfileFactory):
-    name = 'Robot Studio'
     courseware = 'course.xml'
 
 
-class RegistrationFactory(StudentRegistrationFactory):
-    pass
-
-
-class UserFactory(StudentUserFactory):
-    email = 'robot@edx.org'
-    last_name = 'Tester'
-    last_login = datetime.now(UTC)
-    date_joined = datetime.now(UTC)
-
-
-def InstructorFactory(course):  # pylint: disable=invalid-name
+class InstructorFactory(UserFactory):
     """
-    Given a course object, returns a User object with instructor
+    Given a course Location, returns a User object with instructor
     permissions for `course`.
     """
-    user = StudentUserFactory.create(last_name="Instructor")
-    allow_access(course, user, "instructor")
-    return user
+    last_name = "Instructor"
+
+    @post_generation
+    def course(self, create, extracted, **kwargs):
+        if extracted is None:
+            raise ValueError("Must specify a course location for a course instructor user")
+        CourseInstructorRole(extracted).add_users(self)
 
 
-def StaffFactory(course):  # pylint: disable=invalid-name
+class StaffFactory(UserFactory):
     """
-    Given a course object, returns a User object with staff
+    Given a course Location, returns a User object with staff
     permissions for `course`.
     """
-    user = StudentUserFactory.create(last_name="Staff")
-    allow_access(course, user, "staff")
-    return user
-
-
-class GroupFactory(StudentGroupFactory):
-    name = 'test_group'
-
+    last_name = "Staff"
 
-class CourseEnrollmentAllowedFactory(StudentCourseEnrollmentAllowedFactory):
-    pass
+    @post_generation
+    def course(self, create, extracted, **kwargs):
+        if extracted is None:
+            raise ValueError("Must specify a course location for a course staff user")
+        CourseStaffRole(extracted).add_users(self)
 
 
 class StudentModuleFactory(DjangoModelFactory):

lms/djangoapps/courseware/tests/test_access.py

+import courseware.access as access
+import datetime
+
 from mock import Mock
 
 from django.test import TestCase
 from django.test.utils import override_settings
 
+from courseware.tests.factories import UserFactory, CourseEnrollmentAllowedFactory, StaffFactory, InstructorFactory
+from student.tests.factories import AnonymousUserFactory
 from xmodule.modulestore import Location
-import courseware.access as access
 from courseware.tests.tests import TEST_DATA_MIXED_MODULESTORE
-from .factories import CourseEnrollmentAllowedFactory
-import datetime
 import pytz
 
 
+# pylint: disable=protected-access
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class AccessTestCase(TestCase):
     """
     Tests for the various access controls on the student dashboard
     """
-    def test__has_global_staff_access(self):
-        u = Mock(is_staff=False)
-        self.assertFalse(access._has_global_staff_access(u))
 
-        u = Mock(is_staff=True)
-        self.assertTrue(access._has_global_staff_access(u))
+    def setUp(self):
+        self.course = Location('i4x://edX/toy/course/2012_Fall')
+        self.anonymous_user = AnonymousUserFactory()
+        self.student = UserFactory()
+        self.global_staff = UserFactory(is_staff=True)
+        self.course_staff = StaffFactory(course=self.course)
+        self.course_instructor = InstructorFactory(course=self.course)
 
     def test__has_access_to_location(self):
-        location = Location('i4x://edX/toy/course/2012_Fall')
+        self.assertFalse(access._has_access_to_location(None, self.course, 'staff', None))
+
+        self.assertFalse(access._has_access_to_location(self.anonymous_user, self.course, 'staff', None))
+        self.assertFalse(access._has_access_to_location(self.anonymous_user, self.course, 'instructor', None))
+
+        self.assertTrue(access._has_access_to_location(self.global_staff, self.course, 'staff', None))
+        self.assertTrue(access._has_access_to_location(self.global_staff, self.course, 'instructor', None))
 
-        self.assertFalse(access._has_access_to_location(None, location,
-                                                        'staff', None))
-        u = Mock()
-        u.is_authenticated.return_value = False
-        self.assertFalse(access._has_access_to_location(u, location,
-                                                        'staff', None))
-        u = Mock(is_staff=True)
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                       'instructor', None))
         # A user has staff access if they are in the staff group
-        u = Mock(is_staff=False)
-        g = Mock()
-        g.name = 'staff_edX/toy/2012_Fall'
-        u.groups.all.return_value = [g]
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                        'staff', None))
-        # A user has staff access if they are in the instructor group
-        g.name = 'instructor_edX/toy/2012_Fall'
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                        'staff', None))
-
-        # A user has instructor access if they are in the instructor group
-        g.name = 'instructor_edX/toy/2012_Fall'
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                        'instructor', None))
-
-        # A user does not have staff access if they are
+        self.assertTrue(access._has_access_to_location(self.course_staff, self.course, 'staff', None))
+        self.assertFalse(access._has_access_to_location(self.course_staff, self.course, 'instructor', None))
+
+        # A user has staff and instructor access if they are in the instructor group
+        self.assertTrue(access._has_access_to_location(self.course_instructor, self.course, 'staff', None))
+        self.assertTrue(access._has_access_to_location(self.course_instructor, self.course, 'instructor', None))
+
+        # A user does not have staff or instructor access if they are
         # not in either the staff or the the instructor group
-        g.name = 'student_only'
-        self.assertFalse(access._has_access_to_location(u, location,
-                                                        'staff', None))
-
-        # A user does not have instructor access if they are
-        # not in the instructor group
-        g.name = 'student_only'
-        self.assertFalse(access._has_access_to_location(u, location,
-                                                        'instructor', None))
+        self.assertFalse(access._has_access_to_location(self.student, self.course, 'staff', None))
+        self.assertFalse(access._has_access_to_location(self.student, self.course, 'instructor', None))
 
     def test__has_access_string(self):
         u = Mock(is_staff=True)

lms/djangoapps/courseware/tests/test_masquerade.py

@@ -12,11 +12,11 @@ import json
 
 from django.test.utils import override_settings
 from django.core.urlresolvers import reverse
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
 from lms.lib.xblock.runtime import quote_slashes
@@ -42,9 +42,7 @@ class TestStaffMasqueradeAsStudent(ModuleStoreTestCase, LoginEnrollmentTestCase)
         self.activate_user(self.instructor)
 
         def make_instructor(course):
-            group_name = _course_staff_group_name(course.location)
-            g = Group.objects.create(name=group_name)
-            g.user_set.add(User.objects.get(email=self.instructor))
+            CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
 
         make_instructor(self.graded_course)
 

lms/djangoapps/courseware/tests/test_roles.py

+"""
+Tests of courseware.roles
+"""
+
+from django.test import TestCase
+
+from xmodule.modulestore import Location
+from courseware.tests.factories import UserFactory, StaffFactory, InstructorFactory
+from student.tests.factories import AnonymousUserFactory
+
+from courseware.roles import GlobalStaff, CourseRole
+
+
+class RolesTestCase(TestCase):
+    """
+    Tests of courseware.roles
+    """
+
+    def setUp(self):
+        self.course = Location('i4x://edX/toy/course/2012_Fall')
+        self.anonymous_user = AnonymousUserFactory()
+        self.student = UserFactory()
+        self.global_staff = UserFactory(is_staff=True)
+        self.course_staff = StaffFactory(course=self.course)
+        self.course_instructor = InstructorFactory(course=self.course)
+
+    def test_global_staff(self):
+        self.assertFalse(GlobalStaff().has_user(self.student))
+        self.assertFalse(GlobalStaff().has_user(self.course_staff))
+        self.assertFalse(GlobalStaff().has_user(self.course_instructor))
+        self.assertTrue(GlobalStaff().has_user(self.global_staff))
+
+    def test_group_name_case_insensitive(self):
+        uppercase_loc = "i4x://ORG/COURSE/course/NAME"
+        lowercase_loc = uppercase_loc.lower()
+
+        lowercase_group = "role_org/course/name"
+        uppercase_group = lowercase_group.upper()
+
+        lowercase_user = UserFactory(groups=lowercase_group)
+        uppercase_user = UserFactory(groups=uppercase_group)
+
+        self.assertTrue(CourseRole("role", lowercase_loc).has_user(lowercase_user))
+        self.assertTrue(CourseRole("role", uppercase_loc).has_user(lowercase_user))
+        self.assertTrue(CourseRole("role", lowercase_loc).has_user(uppercase_user))
+        self.assertTrue(CourseRole("role", uppercase_loc).has_user(uppercase_user))
+

lms/djangoapps/courseware/tests/test_view_authentication.py

@@ -3,19 +3,19 @@ import pytz
 
 from mock import patch
 
-from django.contrib.auth.models import User, Group
+from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from courseware.access import (has_access, _course_staff_group_name,
-                               course_beta_test_group_name, settings as access_settings)
+from courseware.access import has_access
+from courseware.roles import CourseBetaTesterRole, CourseInstructorRole, CourseStaffRole, GlobalStaff
 
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 
-from helpers import LoginEnrollmentTestCase, check_for_get_code
+from courseware.tests.helpers import LoginEnrollmentTestCase, check_for_get_code
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 
 
@@ -173,9 +173,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         email, password = self.ACCOUNT_INFO[1]
 
         # Make the instructor staff in self.course
-        group_name = _course_staff_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(User.objects.get(email=email))
+        CourseInstructorRole(self.course.location).add_users(User.objects.get(email=email))
 
         self.login(email, password)
 
@@ -206,7 +204,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         for url in urls:
             check_for_get_code(self, 200, url)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_dark_launch_enrolled_student(self):
         """
         Make sure that before course start, students can't access course
@@ -237,7 +235,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self._check_non_staff_light(self.test_course)
         self._check_non_staff_dark(self.test_course)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_dark_launch_instructor(self):
         """
         Make sure that before course start instructors can access the
@@ -253,9 +251,8 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.test_course = self.update_course(self.test_course, test_course_data)
 
         # Make the instructor staff in  self.course
-        group_name = _course_staff_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(User.objects.get(email=instructor_email))
+        CourseStaffRole(self.course.location).add_users(User.objects.get(email=instructor_email))
+
         self.logout()
         self.login(instructor_email, instructor_password)
         # Enroll in the classes---can't see courseware otherwise.
@@ -267,7 +264,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self._check_non_staff_dark(self.test_course)
         self._check_staff(self.course)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_dark_launch_staff(self):
         """
         Make sure that before course start staff can access
@@ -295,7 +292,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self._check_staff(self.course)
         self._check_staff(self.test_course)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_enrollment_period(self):
         """
         Check that enrollment periods work.
@@ -323,25 +320,22 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertTrue(self.enroll(self.test_course))
 
         # Make the instructor staff in the self.course
-        group_name = _course_staff_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(User.objects.get(email=instructor_email))
+        instructor_role = CourseInstructorRole(self.course.location)
+        instructor_role.add_users(User.objects.get(email=instructor_email))
 
         self.logout()
         self.login(instructor_email, instructor_password)
         self.assertTrue(self.enroll(self.course))
 
         # now make the instructor global staff, but not in the instructor group
-        group.user_set.remove(User.objects.get(email=instructor_email))
-        instructor = User.objects.get(email=instructor_email)
-        instructor.is_staff = True
-        instructor.save()
+        instructor_role.remove_users(User.objects.get(email=instructor_email))
+        GlobalStaff().add_users(User.objects.get(email=instructor_email))
 
         # unenroll and try again
         self.unenroll(self.course)
         self.assertTrue(self.enroll(self.course))
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_beta_period(self):
         """
         Check that beta-test access works.
@@ -366,9 +360,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertFalse(has_access(student_user, self.course, 'load'))
 
         # now add the student to the beta test group
-        group_name = course_beta_test_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(student_user)
+        CourseBetaTesterRole(self.course.location).add_users(student_user)
 
         # now the student should see it
         self.assertTrue(has_access(student_user, self.course, 'load'))

lms/djangoapps/instructor/access.py

@@ -10,13 +10,18 @@ TO DO sync instructor and staff flags
 """
 
 import logging
-from django.contrib.auth.models import Group
-from courseware.access import (get_access_group_name,
-                               course_beta_test_group_name)
 from django_comment_common.models import Role
 
+from courseware.roles import CourseBetaTesterRole, CourseInstructorRole, CourseStaffRole
+
 log = logging.getLogger(__name__)
 
+ROLES = {
+    'beta': CourseBetaTesterRole,
+    'instructor': CourseInstructorRole,
+    'staff': CourseStaffRole,
+}
+
 
 def list_with_level(course, level):
     """
@@ -26,16 +31,7 @@ def list_with_level(course, level):
     There could be other levels specific to the course.
     If there is no Group for that course-level, returns an empty list
     """
-    if level == 'beta':
-        grpname = course_beta_test_group_name(course.location)
-    else:
-        grpname = get_access_group_name(course, level)
-
-    try:
-        return Group.objects.get(name=grpname).user_set.all()
-    except Group.DoesNotExist:
-        log.info("list_with_level called with non-existant group named {}".format(grpname))
-        return []
+    return ROLES[level](course.location).users_with_role()
 
 
 def allow_access(course, user, level):
@@ -66,18 +62,15 @@ def _change_access(course, user, level, action):
     NOTE: will create a group if it does not yet exist.
     """
 
-    if level == 'beta':
-        grpname = course_beta_test_group_name(course.location)
-    elif level in ['instructor', 'staff']:
-        grpname = get_access_group_name(course, level)
-    else:
+    try:
+        role = ROLES[level](course.location)
+    except KeyError:
         raise ValueError("unrecognized level '{}'".format(level))
-    group, _ = Group.objects.get_or_create(name=grpname)
 
     if action == 'allow':
-        user.groups.add(group)
+        role.add_users(user)
     elif action == 'revoke':
-        user.groups.remove(group)
+        role.remove_users(user)
     else:
         raise ValueError("unrecognized action '{}'".format(action))
 

lms/djangoapps/instructor/tests/test_access.py

@@ -3,15 +3,14 @@ Test instructor.access
 """
 
 from nose.tools import raises
-from django.contrib.auth.models import Group
 from student.tests.factories import UserFactory
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from django.test.utils import override_settings
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseBetaTesterRole, CourseStaffRole
 
-from courseware.access import get_access_group_name
 from django_comment_common.models import (Role,
                                           FORUM_ROLE_MODERATOR)
 from instructor.access import (allow_access,
@@ -51,39 +50,29 @@ class TestInstructorAccessAllow(ModuleStoreTestCase):
     def test_allow(self):
         user = UserFactory()
         allow_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertIn(user, group.user_set.all())
+        self.assertTrue(CourseStaffRole(self.course.location).has_user(user))
 
     def test_allow_twice(self):
         user = UserFactory()
         allow_access(self.course, user, 'staff')
         allow_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertIn(user, group.user_set.all())
+        self.assertTrue(CourseStaffRole(self.course.location).has_user(user))
 
     def test_allow_beta(self):
         """ Test allow beta against list beta. """
         user = UserFactory()
         allow_access(self.course, user, 'beta')
-        self.assertIn(user, list_with_level(self.course, 'beta'))
+        self.assertTrue(CourseBetaTesterRole(self.course.location).has_user(user))
 
     @raises(ValueError)
     def test_allow_badlevel(self):
         user = UserFactory()
         allow_access(self.course, user, 'robot-not-a-level')
-        group = Group.objects.get(name=get_access_group_name(self.course, 'robot-not-a-level'))
-        self.assertIn(user, group.user_set.all())
 
     @raises(Exception)
     def test_allow_noneuser(self):
         user = None
         allow_access(self.course, user, 'staff')
-        group = Group.objects.get(name=get_access_group_name(self.course, 'staff'))
-        self.assertIn(user, group.user_set.all())
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -102,32 +91,22 @@ class TestInstructorAccessRevoke(ModuleStoreTestCase):
     def test_revoke(self):
         user = self.staff[0]
         revoke_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertNotIn(user, group.user_set.all())
+        self.assertFalse(CourseStaffRole(self.course.location).has_user(user))
 
     def test_revoke_twice(self):
         user = self.staff[0]
         revoke_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertNotIn(user, group.user_set.all())
+        self.assertFalse(CourseStaffRole(self.course.location).has_user(user))
 
     def test_revoke_beta(self):
         user = self.beta_testers[0]
         revoke_access(self.course, user, 'beta')
-        self.assertNotIn(user, list_with_level(self.course, 'beta'))
+        self.assertFalse(CourseBetaTesterRole(self.course.location).has_user(user))
 
     @raises(ValueError)
     def test_revoke_badrolename(self):
         user = UserFactory()
         revoke_access(self.course, user, 'robot-not-a-level')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'robot-not-a-level')
-        )
-        self.assertNotIn(user, group.user_set.all())
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)

lms/djangoapps/instructor/tests/test_api.py

@@ -183,7 +183,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         Ensure that a staff member can't access instructor endpoints.
         """
-        staff_member = StaffFactory(self.course)
+        staff_member = StaffFactory(course=self.course.location)
         CourseEnrollment.enroll(staff_member, self.course.id)
         self.client.login(username=staff_member.username, password='test')
         # Try to promote to forums admin - not working
@@ -212,7 +212,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         Ensure that an instructor member can access all endpoints.
         """
-        inst = InstructorFactory(self.course)
+        inst = InstructorFactory(course=self.course.location)
         CourseEnrollment.enroll(inst, self.course.id)
         self.client.login(username=inst.username, password='test')
 
@@ -249,7 +249,7 @@ class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase):
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.enrolled_student = UserFactory()
@@ -376,7 +376,7 @@ class TestInstructorAPILevelsAccess(ModuleStoreTestCase, LoginEnrollmentTestCase
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.other_instructor = UserFactory()
@@ -513,7 +513,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.students = [UserFactory() for _ in xrange(6)]
@@ -650,7 +650,7 @@ class TestInstructorAPIRegradeTask(ModuleStoreTestCase, LoginEnrollmentTestCase)
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.student = UserFactory()
@@ -794,7 +794,7 @@ class TestInstructorSendEmail(ModuleStoreTestCase, LoginEnrollmentTestCase):
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
         test_subject = u'\u1234 test subject'
         test_message = u'\u6824 test message'
@@ -916,7 +916,7 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase):
 
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.student = UserFactory()
@@ -1047,7 +1047,7 @@ class TestInstructorAPIAnalyticsProxy(ModuleStoreTestCase, LoginEnrollmentTestCa
 
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
     @patch.object(instructor.views.api.requests, 'get')

lms/djangoapps/instructor/tests/test_legacy_anon_csv.py

@@ -11,16 +11,15 @@ Notes for running by hand:
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
 from django.core.urlresolvers import reverse
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
-import xmodule.modulestore.django
 
 from mock import patch
 
@@ -46,10 +45,8 @@ class TestInstructorDashboardAnonCSV(ModuleStoreTestCase, LoginEnrollmentTestCas
         self.activate_user(self.instructor)
 
         def make_instructor(course):
-            """ Create an instructor for the course. """
-            group_name = _course_staff_group_name(course.location)
-            group = Group.objects.create(name=group_name)
-            group.user_set.add(User.objects.get(email=self.instructor))
+            """ Create an instructor for the course."""
+            CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
 
         make_instructor(self.toy)
 
@@ -68,4 +65,3 @@ class TestInstructorDashboardAnonCSV(ModuleStoreTestCase, LoginEnrollmentTestCas
         self.assertEqual(response['Content-Type'], 'text/csv')
         body = response.content.replace('\r', '')
         self.assertEqual(body, '"User ID","Anonymized user ID"\n"2","42"\n')
-

lms/djangoapps/instructor/tests/test_legacy_download_csv.py

@@ -11,16 +11,15 @@ Notes for running by hand:
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
 from django.core.urlresolvers import reverse
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
-import xmodule.modulestore.django
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -44,9 +43,7 @@ class TestInstructorDashboardGradeDownloadCSV(ModuleStoreTestCase, LoginEnrollme
 
         def make_instructor(course):
             """ Create an instructor for the course. """
-            group_name = _course_staff_group_name(course.location)
-            group = Group.objects.create(name=group_name)
-            group.user_set.add(User.objects.get(email=self.instructor))
+            CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
 
         make_instructor(self.toy)
 

lms/djangoapps/instructor/tests/test_legacy_enrollment.py

@@ -195,7 +195,7 @@ class TestInstructorEnrollsStudent(ModuleStoreTestCase, LoginEnrollmentTestCase)
         course = self.course
 
         # Create activated, but not enrolled, user
-        UserFactory.create(username="student3_0", email="student3_0@test.com")
+        UserFactory.create(username="student3_0", email="student3_0@test.com", first_name='Autoenrolled')
 
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
         response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'student3_0@test.com, student3_1@test.com, student3_2@test.com', 'auto_enroll': 'on', 'email_students': 'on'})
@@ -215,12 +215,12 @@ class TestInstructorEnrollsStudent(ModuleStoreTestCase, LoginEnrollmentTestCase)
         )
         self.assertEqual(
             mail.outbox[0].body,
-            "Dear Robot Test\n\nYou have been enrolled in Robot Super Course "
+            "Dear Autoenrolled Test\n\nYou have been enrolled in Robot Super Course "
             "at edx.org by a member of the course staff. "
             "The course should now appear on your edx.org dashboard.\n\n"
             "To start accessing course materials, please visit "
             "https://edx.org/courses/MITx/999/Robot_Super_Course\n\n"
-            "----\nThis email was automatically sent from edx.org to Robot Test"
+            "----\nThis email was automatically sent from edx.org to Autoenrolled Test"
         )
 
         self.assertEqual(

lms/djangoapps/instructor/tests/test_legacy_forum_admin.py

@@ -6,16 +6,16 @@ Unit tests for instructor dashboard forum administration
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
 from django.core.urlresolvers import reverse
 from django_comment_common.models import Role, FORUM_ROLE_ADMINISTRATOR, \
     FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_STUDENT
 from django_comment_client.utils import has_forum_access
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
@@ -54,9 +54,7 @@ class TestInstructorDashboardForumAdmin(ModuleStoreTestCase, LoginEnrollmentTest
         self.activate_user(self.student)
         self.activate_user(self.instructor)
 
-        group_name = _course_staff_group_name(self.toy.location)
-        g = Group.objects.create(name=group_name)
-        g.user_set.add(User.objects.get(email=self.instructor))
+        CourseStaffRole(self.toy.location).add_users(User.objects.get(email=self.instructor))
 
         self.logout()
         self.login(self.instructor, self.password)

lms/djangoapps/open_ended_grading/tests.py

@@ -8,7 +8,7 @@ import json
 import logging
 
 from django.conf import settings
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.test.utils import override_settings
 from mock import MagicMock, patch, Mock
@@ -22,11 +22,11 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.open_ended_grading_classes import peer_grading_service, controller_query_service
 from xmodule.tests import test_util_open_ended
 
-from courseware.access import _course_staff_group_name
 from courseware.tests import factories
 from courseware.tests.helpers import LoginEnrollmentTestCase, check_for_get_code, check_for_post_code
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from lms.lib.xblock.runtime import LmsModuleSystem
+from courseware.roles import CourseStaffRole
 from mitxmako.shortcuts import render_to_string
 from student.models import unique_id_for_user
 
@@ -52,9 +52,7 @@ def make_instructor(course, user_email):
     """
     Makes a given user an instructor in a course.
     """
-    group_name = _course_staff_group_name(course.location)
-    group = Group.objects.create(name=group_name)
-    group.user_set.add(User.objects.get(email=user_email))
+    CourseStaffRole(course.location).add_users(User.objects.get(email=user_email))
 
 
 class StudentProblemListMockQuery(object):