Merge pull request #844 from MITx/feature/cdodge/cms-unified-user-database

adhere to the naming conventions of user groups (instructor_* and staff_...

cms/djangoapps/auth/authz.py

@@ -6,21 +6,33 @@ from django.core.exceptions import PermissionDenied
 
 from xmodule.modulestore import Location
 
+'''
+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
+'''
+
 # define a couple of simple roles, we just need ADMIN and EDITOR now for our purposes
-ADMIN_ROLE_NAME = 'admin'
-EDITOR_ROLE_NAME = 'editor'
+INSTRUCTOR_ROLE_NAME = 'instructor'
+STAFF_ROLE_NAME = 'staff'
 
 # 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_course_groupname_for_role(location, role):
     loc = Location(location)
-    groupname = loc.course_id  + ':' + role
+    # hack: check for existence of a group name in the legacy LMS format <role>_<course>
+    # if it exists, then use that one, otherwise use a <role>_<course_id> which contains
+    # more information
+    groupname = '{0}_{1}'.format(role, loc.course)
+
+    if len(Group.objects.filter(name = groupname)) == 0:
+        groupname = '{0}_{1}'.format(role,loc.course_id)
+
     return groupname
 
 def get_users_in_course_group_by_role(location, role):
     groupname = get_course_groupname_for_role(location, role)
-    group = Group.objects.get(name=groupname)
+    (group, created) = Group.objects.get_or_create(name=groupname)
     return group.user_set.all()
 
 
@@ -28,8 +40,8 @@ def get_users_in_course_group_by_role(location, role):
 Create all permission groups for a new course and subscribe the caller into those roles
 '''
 def create_all_course_groups(creator, location):
-    create_new_course_group(creator, location, ADMIN_GROUP_NAME)
-    create_new_course_group(creator, location, EDITOR_GROUP_NAME)
+    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):
@@ -46,7 +58,7 @@ def create_new_course_group(creator, location, role):
 
 def add_user_to_course_group(caller, user, location, role):
     # only admins can add/remove other users
-    if not is_user_in_course_group_role(caller, location, ADMIN_ROLE_NAME):
+    if not is_user_in_course_group_role(caller, location, INSTRUCTOR_ROLE_NAME):
         raise PermissionDenied
 
     if user.is_active and user.is_authenticated:
@@ -73,7 +85,7 @@ def get_user_by_email(email):
 
 def remove_user_from_course_group(caller, user, location, role):
     # only admins can add/remove other users
-    if not is_user_in_course_group_role(caller, location, ADMIN_ROLE_NAME):
+    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

cms/djangoapps/contentstore/views.py

@@ -42,7 +42,7 @@ from xmodule.contentstore.content import StaticContent
 from cache_toolbox.core import set_cached_content, get_cached_content, del_cached_content
 from auth.authz import is_user_in_course_group_role, get_users_in_course_group_by_role
 from auth.authz import get_user_by_email, add_user_to_course_group, remove_user_from_course_group
-from auth.authz import ADMIN_ROLE_NAME, EDITOR_ROLE_NAME
+from auth.authz import INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME
 from .utils import get_course_location_for_item, get_lms_link_for_item
 
 from xmodule.templates import all_templates
@@ -98,11 +98,22 @@ def index(request):
 
 # ==== Views with per-item permissions================================
 
-def has_access(user, location, role=EDITOR_ROLE_NAME):
-    '''Return True if user allowed to access this piece of data'''
-    '''Note that the CMS permissions model is with respect to courses'''
-    '''There is a super-admin permissions if user.is_staff is set'''
-    return is_user_in_course_group_role(user, get_course_location_for_item(location), role)
+def has_access(user, location, role=STAFF_ROLE_NAME):
+    '''
+    Return True if user allowed to access this piece of data
+    Note that the CMS permissions model is with respect to courses
+    There is a super-admin permissions if user.is_staff is set
+    Also, since we're unifying the user database between LMS and CAS, 
+    I'm presuming that the course instructor (formally known as admin)
+    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
+    '''
+    course_location = get_course_location_for_item(location)
+    _has_access = is_user_in_course_group_role(user, course_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, course_location, INSTRUCTOR_ROLE_NAME)
+    return _has_access
 
 
 @login_required
@@ -591,15 +602,16 @@ This view will return all CMS users who are editors for the specified course
 '''
 @login_required
 @ensure_csrf_cookie
-def manage_users(request, org, course, name):
-    location = ['i4x', org, course, 'course', name]
+def manage_users(request, location):
     
     # check that logged in user has permissions to this item
-    if not has_access(request.user, location, role=ADMIN_ROLE_NAME):
+    if not has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME):
         raise PermissionDenied()
 
     return render_to_response('manage_users.html', {
-        'editors': get_users_in_course_group_by_role(location, EDITOR_ROLE_NAME)
+        'staff': get_users_in_course_group_by_role(location, STAFF_ROLE_NAME),
+        'add_user_postback_url' : reverse('add_user', args=[location]).rstrip('/'),
+        'remove_user_postback_url' : reverse('remove_user', args=[location]).rstrip('/')
     })
     
 
@@ -615,18 +627,17 @@ def create_json_response(errmsg = None):
 This POST-back view will add a user - specified by email - to the list of editors for
 the specified course
 '''
+@expect_json
 @login_required
 @ensure_csrf_cookie
-def add_user(request, org, course, name):
+def add_user(request, location):
     email = request.POST["email"]
 
     if email=='':
         return create_json_response('Please specify an email address.')
     
-    location = ['i4x', org, course, 'course', name]
-    
     # check that logged in user has admin permissions to this course
-    if not has_access(request.user, location, role=ADMIN_ROLE_NAME):
+    if not has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME):
         raise PermissionDenied()
     
     user = get_user_by_email(email)
@@ -640,7 +651,7 @@ def add_user(request, org, course, name):
         return create_json_response('User {0} has registered but has not yet activated his/her account.'.format(email))
 
     # ok, we're cool to add to the course group
-    add_user_to_course_group(request.user, user, location, EDITOR_ROLE_NAME)
+    add_user_to_course_group(request.user, user, location, STAFF_ROLE_NAME)
 
     return create_json_response()
 
@@ -648,22 +659,21 @@ def add_user(request, org, course, name):
 This POST-back view will remove a user - specified by email - from the list of editors for
 the specified course
 '''
+@expect_json
 @login_required
 @ensure_csrf_cookie
-def remove_user(request, org, course, name):
+def remove_user(request, location):
     email = request.POST["email"]
     
-    location = ['i4x', org, course, 'course', name]
-    
     # check that logged in user has admin permissions on this course
-    if not has_access(request.user, location, role=ADMIN_ROLE_NAME):
+    if not has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME):
         raise PermissionDenied()
 
     user = get_user_by_email(email)
     if user is None:
         return create_json_response('Could not find user by email address \'{0}\'.'.format(email))
 
-    remove_user_from_course_group(request.user, user, location, EDITOR_ROLE_NAME)
+    remove_user_from_course_group(request.user, user, location, STAFF_ROLE_NAME)
 
     return create_json_response()
 

cms/envs/dev.py

@@ -42,7 +42,7 @@ CONTENTSTORE = {
 DATABASES = {
     'default': {
         'ENGINE': 'django.db.backends.sqlite3',
-        'NAME': ENV_ROOT / "db" / "cms.db",
+        'NAME': ENV_ROOT / "db" / "mitx.db",
     }
 }
 
@@ -97,3 +97,6 @@ CACHES = {
 
 # Make the keyedcache startup warnings go away
 CACHE_TIMEOUT = 0
+
+# Dummy secret key for dev
+SECRET_KEY = '85920908f28904ed733fe576320db18cabd7b6cd'

cms/templates/manage_users.html

 <%inherit file="base.html" />
-<%block name="title">Course Editor Manager</%block>
+<%block name="title">Course Staff Manager</%block>
 <%include file="widgets/header.html"/>
 
 <%block name="content">
 <section class="main-container">
 
-  <h2>Course Editors</h2>
-  <ul>
-    % for user in editors:
-    <li>${user.email} (${user.username})</li>
+  <h2>Course Staff</h2>
+  <div>
+    <p>
+      The following list of users have been designated as course staff. This means that these users will 
+      have permissions to modify course content. You may add additional source staff below. Please note that they
+      must have already registered and verified their account.
+    </p>
+  </div>
+  <div>
+    <ol>
+      % for user in staff:
+      <li>${user.email} (${user.username}) <a href='#' class='remove-user' data-id='${user.email}'>remove</a></li>
       % endfor
-  </ul>
+    </ol>
+  </div>
+
+  
+  <label>email:&nbsp;</label><input type="text" id="email" autocomplete="Off" placeholder="email@example.com..." size="40"/>
+  <a href="#" id="add_user" class="button" />add as course staff</a>
   
-  <form action="add_user" id="addEditorsForm">
-    <label>email:&nbsp;</label><input type="text" name="email" placeholder="email@example.com..." />
-    <input type="submit" value="add editor" />
-  </form>
   <div id="result"></div>
 
-<script>
-  $("#addEditorsForm").submit(function(event) {
-  event.preventDefault();
-  var $form = $(this),
-  email = $form.find('input[name="email"]').val(),
-  url = $form.attr('action');
-  $.post(url, {email:email},
-  function(data) {
-  if(data['Status'] != 'OK')
-  $("#result").empty().append(data['ErrMsg']);
+</section>
+</%block>
+
+<%block name="jsextra">
+
+<script type="text/javascript">
+  $(document).ready(function() {
+      $("#add_user").click(function() {
+        $.ajax({
+          url: "${add_user_postback_url}",
+          type: "POST",
+          dataType: "json",
+          contentType: "application/json",
+          data:JSON.stringify({ 'email': $('#email').val()}),
+        }).done(function(data) {
+          if (data.ErrMsg != undefined)
+            $("#result").empty().append(data.ErrMsg);
           else
             location.reload();
+        })
+      })
+
+      $(".remove-user").click(function() {
+        $.ajax({
+          url: "${remove_user_postback_url}",
+          type: "POST",
+          dataType: "json",
+          contentType: "application/json",
+          data:JSON.stringify({ 'email': $(this).data('id')}),          
+          }).done(function() {
+            location.reload();
+          })
       });
   });
 </script>
-
-</section>
 </%block>
\ No newline at end of file

cms/urls.py

@@ -22,10 +22,11 @@ urlpatterns = ('',
         'contentstore.views.preview_dispatch', name='preview_dispatch'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)/upload_asset$', 
         'contentstore.views.upload_asset', name='upload_asset'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)/manage_users$',
-        'contentstore.views.manage_users', name='manage_users'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)/add_user$',
+    url(r'^manage_users/(?P<location>.*?)$', 'contentstore.views.manage_users', name='manage_users'),
+    url(r'^add_user/(?P<location>.*?)$',
         'contentstore.views.add_user', name='add_user'),
+    url(r'^remove_user/(?P<location>.*?)$',
+        'contentstore.views.remove_user', name='remove_user'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)/remove_user$',
         'contentstore.views.remove_user', name='remove_user'),
     url(r'^assets/(?P<location>.*?)$', 'contentstore.views.asset_index', name='asset_index'),

lms/djangoapps/courseware/access.py

@@ -6,6 +6,7 @@ import logging
 import time
 
 from django.conf import settings
+from django.contrib.auth.models import Group
 
 from xmodule.course_module import CourseDescriptor
 from xmodule.error_module import ErrorDescriptor
@@ -306,13 +307,25 @@ def _dispatch(table, action, user, obj):
     raise ValueError("Unknown action for object type '{0}': '{1}'".format(
         type(obj), action))
 
+
+def _does_course_group_name_exist(name):
+    return len(Group.objects.filter(name = name)) > 0
+
 def _course_staff_group_name(location):
     """
     Get the name of the staff group for a location.  Right now, that's staff_COURSE.
 
     location: something that can passed to Location.
+    
+    cdodge: We're changing the name convention of the group to better epxress different runs of courses by 
+    using course_id rather than just the course number. So first check to see if the group name exists
     """
-    return 'staff_%s' % Location(location).course
+    loc = Location(location)
+    legacy_name = 'staff_%s' % loc.course
+    if _does_course_group_name_exist(legacy_name):
+        return legacy_name
+
+    return 'staff_%s' & loc.course_id
 
 
 def _course_instructor_group_name(location):
@@ -321,8 +334,16 @@ def _course_instructor_group_name(location):
     A course instructor has all staff privileges, but also can manage list of course staff (add, remove, list).
 
     location: something that can passed to Location.
+
+    cdodge: We're changing the name convention of the group to better epxress different runs of courses by 
+    using course_id rather than just the course number. So first check to see if the group name exists
     """
-    return 'instructor_%s' % Location(location).course
+    loc = Location(location)
+    legacy_name = 'instructor_%s' % loc.course
+    if _does_course_group_name_exist(legacy_name):
+        return legacy_name
+    
+    return 'instructor_%s' % loc.course_id
 
 
 def _has_global_staff_access(user):