From 68945fdc77264c939fb65a1cf52f2065681f4453 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 9 Oct 2012 13:14:31 -0400 Subject: [PATCH 1/2] adhere to the naming conventions of user groups (instructor_* and staff_* of the LMS AuthZ implementation). While we're keeping our authz.py implementation - which is duplicative of LMS's access.py - in order to minimize code churn/risk-of-regression, there is data compatibility so the two systems will interpret the data the same way --- cms/djangoapps/auth/authz.py | 21 ++++--- cms/djangoapps/contentstore/views.py | 50 +++++++++------- cms/envs/dev.py | 2 +- cms/templates/manage_users.html | 87 +++++++++++++++++++++------- cms/urls.py | 7 ++- 5 files changed, 113 insertions(+), 54 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 2f6f15cf63..90c51e45a2 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -6,21 +6,26 @@ 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 + groupname = role + '_' + loc.course 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 +33,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 +51,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 +78,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 diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index c1c3fc92d0..190b463383 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/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() diff --git a/cms/envs/dev.py b/cms/envs/dev.py index dd0e0337f6..d692d202fd 100644 --- a/cms/envs/dev.py +++ b/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", } } diff --git a/cms/templates/manage_users.html b/cms/templates/manage_users.html index 3887b4cbcb..b525080123 100644 --- a/cms/templates/manage_users.html +++ b/cms/templates/manage_users.html @@ -1,38 +1,81 @@ <%inherit file="base.html" /> -<%block name="title">Course Editor Manager +<%block name="title">Course Staff Manager <%include file="widgets/header.html"/> <%block name="content">
-

Course Editors

- +

Course Staff

+
+

+ 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. +

+
+
+
    + % for user in staff: +
  1. ${user.email} (${user.username}) remove
    2. + % endfor +
+
-
- - -
+ + + add as course staff +
+ +<%block name="jsextra"> + + + \ No newline at end of file diff --git a/cms/urls.py b/cms/urls.py index 44f42343f3..890e9e2671 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -22,10 +22,11 @@ urlpatterns = ('', 'contentstore.views.preview_dispatch', name='preview_dispatch'), url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/upload_asset$', 'contentstore.views.upload_asset', name='upload_asset'), - url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/manage_users$', - 'contentstore.views.manage_users', name='manage_users'), - url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/add_user$', + url(r'^manage_users/(?P.*?)$', 'contentstore.views.manage_users', name='manage_users'), + url(r'^add_user/(?P.*?)$', 'contentstore.views.add_user', name='add_user'), + url(r'^remove_user/(?P.*?)$', + 'contentstore.views.remove_user', name='remove_user'), url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/remove_user$', 'contentstore.views.remove_user', name='remove_user'), url(r'^assets/(?P.*?)$', 'contentstore.views.asset_index', name='asset_index'), From a80e8ce3d54e88320679b7a535ba894027ca6ff4 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 9 Oct 2012 15:11:07 -0400 Subject: [PATCH 2/2] respond to Cale pull request comments. Update access.py (LMS) to use the same 'check for legacy group name first' logic. Add SECRET_KEY setting to the CAS project so that auth tokens from CAS are accepted in the LMS --- cms/djangoapps/auth/authz.py | 9 ++++++++- cms/envs/dev.py | 3 +++ cms/templates/manage_users.html | 16 ---------------- lms/djangoapps/courseware/access.py | 25 +++++++++++++++++++++++-- 4 files changed, 34 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 90c51e45a2..659588f6f6 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -20,7 +20,14 @@ STAFF_ROLE_NAME = 'staff' # of those two variables def get_course_groupname_for_role(location, role): loc = Location(location) - groupname = role + '_' + loc.course + # hack: check for existence of a group name in the legacy LMS format _ + # if it exists, then use that one, otherwise use a _ 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): diff --git a/cms/envs/dev.py b/cms/envs/dev.py index d692d202fd..8401fa5c15 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -97,3 +97,6 @@ CACHES = { # Make the keyedcache startup warnings go away CACHE_TIMEOUT = 0 + +# Dummy secret key for dev +SECRET_KEY = '85920908f28904ed733fe576320db18cabd7b6cd' diff --git a/cms/templates/manage_users.html b/cms/templates/manage_users.html index b525080123..b647d629b8 100644 --- a/cms/templates/manage_users.html +++ b/cms/templates/manage_users.html @@ -27,22 +27,6 @@
- - diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 2612efedb9..4d49684ffe 100644 --- a/lms/djangoapps/courseware/access.py +++ b/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):