From 27016465f435807277f1502dcc9d9b529f249fef Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Oct 2012 09:18:50 -0400 Subject: [PATCH 1/6] support some simple authorization scheme to view/edit courses --- cms/djangoapps/auth/__init__.py | 0 cms/djangoapps/auth/authz.py | 70 +++++++++++++++++ cms/djangoapps/contentstore/utils.py | 31 ++++++++ cms/djangoapps/contentstore/views.py | 112 ++++++++++++++++++++++++--- cms/envs/common.py | 1 + cms/templates/manage_users.html | 38 +++++++++ cms/urls.py | 6 ++ 7 files changed, 248 insertions(+), 10 deletions(-) create mode 100644 cms/djangoapps/auth/__init__.py create mode 100644 cms/djangoapps/auth/authz.py create mode 100644 cms/djangoapps/contentstore/utils.py create mode 100644 cms/templates/manage_users.html diff --git a/cms/djangoapps/auth/__init__.py b/cms/djangoapps/auth/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py new file mode 100644 index 0000000000..710ea47182 --- /dev/null +++ b/cms/djangoapps/auth/authz.py @@ -0,0 +1,70 @@ +import logging +import sys + +from django.contrib.auth.models import User, Group + +from xmodule.modulestore import Location + +# 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 + 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) + return group.user_set.all() + + +def add_user_to_course_group(caller, user, location, role): + # @todo: make sure caller has 'admin' permissions in the course + if user.is_active and user.is_authenticated: + groupname = get_course_groupname_for_role(location, role) + + # see if the group exists, or create if new + (group, created) = Group.objects.get_or_create(name=groupname) + if created: + # if newly created, then we have to save it + group.save() + + user.groups.add(group) + user.save() + return True + + return False + +def get_user_by_email(email): + user = None + # try to look up user + try: + user = User.objects.get(email=email) + except: + pass + + return user + + +def remove_user_from_course_group(caller, user, location, role): + # @todo: make sure caller has 'admin' permissions in the course + + if is_user_in_course_group_role(user, location, role) == True: + groupname = get_course_groupname_for_role(location, role) + + # make sure the group actually exists + group = Group.objects.get(name=groupname) + + if group is not None: + user.groups.remove(group) + user.save() + + +def is_user_in_course_group_role(user, location, role): + if user.is_active and user.is_authenticated: + return user.groups.filter(name=get_course_groupname_for_role(location,role)).count() > 0 + + return False + + diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py new file mode 100644 index 0000000000..fc801ac684 --- /dev/null +++ b/cms/djangoapps/contentstore/utils.py @@ -0,0 +1,31 @@ +from xmodule.modulestore import Location +from xmodule.modulestore.django import modulestore + +''' +cdodge: for a given Xmodule, return the course that it belongs to +NOTE: This makes a lot of assumptions about the format of the course location +Also we have to assert that this module maps to only one course item - it'll throw an +assert if not +''' +def get_course_location_for_item(location): + item_loc = Location(location) + + # check to see if item is already a course, if so we can skip this + if item_loc.category != 'course': + # @hack! We need to find the course location however, we don't + # know the 'name' parameter in this context, so we have + # to assume there's only one item in this query even though we are not specifying a name + course_search_location = ['i4x', item_loc.org, item_loc.course, 'course', None] + courses = modulestore().get_items(course_search_location) + + # make sure we found exactly one match on this above course search + found_cnt = len(courses) + if found_cnt == 0: + raise BaseException('Could not find course at {0}'.format(course_search_location)) + + if found_cnt > 1: + raise BaseException('Found more than one course at {0}. There should only be one!!!'.format(course_search_location)) + + location = courses[0].location + + return location diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 3223bfda29..c4b86e3d71 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -5,6 +5,7 @@ import logging import sys import mimetypes import StringIO +import exceptions from collections import defaultdict # to install PIL on MacOSX: 'easy_install http://dist.repoze.org/PIL-1.1.6.tar.gz' @@ -12,6 +13,7 @@ from PIL import Image from django.http import HttpResponse, Http404, HttpResponseBadRequest, HttpResponseForbidden from django.contrib.auth.decorators import login_required +from django.core.exceptions import PermissionDenied from django.core.context_processors import csrf from django_future.csrf import ensure_csrf_cookie from django.core.urlresolvers import reverse @@ -36,9 +38,10 @@ from operator import attrgetter from xmodule.contentstore.django import contentstore from xmodule.contentstore.content import StaticContent -#from django.core.cache import cache - 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 +from .utils import get_course_location_for_item log = logging.getLogger(__name__) @@ -72,6 +75,10 @@ def index(request): List all courses available to the logged in user """ courses = modulestore().get_items(['i4x', None, None, 'course', None]) + + # filter out courses that we don't have access to + courses = filter(lambda course: has_access(request.user, course.location), courses) + return render_to_response('index.html', { 'courses': [(course.metadata.get('display_name'), reverse('course_index', args=[ @@ -84,10 +91,10 @@ def index(request): # ==== Views with per-item permissions================================ -def has_access(user, location): +def has_access(user, location, role='editor'): '''Return True if user allowed to access this piece of data''' - # TODO (vshnayder): actually check perms - return user.is_active and user.is_authenticated + '''Note that the CMS permissions model is with respect to courses''' + return is_user_in_course_group_role(user, get_course_location_for_item(location), role) @login_required @@ -99,8 +106,10 @@ def course_index(request, org, course, name): org, course, name: Attributes of the Location for the item to edit """ location = ['i4x', org, course, 'course', name] + + # check that logged in user has permissions to this item if not has_access(request.user, location): - raise Http404 # TODO (vshnayder): better error + raise PermissionDenied() # TODO (cpennington): These need to be read in from the active user _course = modulestore().get_item(location) @@ -134,10 +143,12 @@ def edit_item(request): id: A Location URL """ - # TODO (vshnayder): change name from id to location in coffee+html as well. + item_location = request.GET['id'] + + # check that we have permissions to edit this item if not has_access(request.user, item_location): - raise Http404 # TODO (vshnayder): better error + raise PermissionDenied() item = modulestore().get_item(item_location) item.get_html = wrap_xmodule(item.get_html, item, "xmodule_edit.html") @@ -359,8 +370,10 @@ def get_module_previews(request, descriptor): @expect_json def save_item(request): item_location = request.POST['id'] + + # check permissions for this user within this course if not has_access(request.user, item_location): - raise Http404 # TODO (vshnayder): better error + raise PermissionDenied() if request.POST['data']: data = request.POST['data'] @@ -407,7 +420,7 @@ def clone_item(request): display_name = request.POST['name'] if not has_access(request.user, parent_location): - raise Http404 # TODO (vshnayder): better error + raise PermissionDenies() parent = modulestore().get_item(parent_location) dest_location = parent_location._replace(category=template.category, name=Location.clean_for_url_name(display_name)) @@ -513,3 +526,82 @@ def upload_asset(request, org, course, coursename): return HttpResponse('Upload completed') +''' +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] + + # check that logged in user has permissions to this item + if not has_access(request.user, location): + raise PermissionDenied() + + return render_to_response('manage_users.html', { + 'editors': get_users_in_course_group_by_role(location, 'editor') + }) + + +def create_json_response(errmsg = None): + if errmsg is not None: + resp = HttpResponse(json.dumps({'Status': 'Failed', 'ErrMsg' : errmsg})) + else: + resp = HttpResponse(json.dumps({'Status': 'OK'})) + + return resp + +''' +This POST-back view will add a user - specified by email - to the list of editors for +the specified course +''' +@login_required +@ensure_csrf_cookie +def add_user(request, org, course, name): + 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 permissions to this item + if not has_access(request.user, location): + raise PermissionDenied() + + user = get_user_by_email(email) + + # user doesn't exist?!? Return error. + if user is None: + return create_json_response('Could not find user by email address \'{0}\'.'.format(email)) + + # user exists, but hasn't activated account?!? + if not user.is_active: + 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') + + return create_json_response() + +''' +This POST-back view will remove a user - specified by email - from the list of editors for +the specified course +''' +@login_required +@ensure_csrf_cookie +def remove_user(request, org, course, name): + email = request.POST["email"] + + location = ['i4x', org, course, 'course', name] + + # check that logged in user has permissions to this item + if not has_access(request.user, location): + raise PermissionDenied() + + user = get_user_by_email(email) + if user is not None: + remove_user_from_course_group(request.user, user, location, 'editor') + + return create_json_response() + diff --git a/cms/envs/common.py b/cms/envs/common.py index 7190ba9e51..d80c705fed 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -325,6 +325,7 @@ INSTALLED_APPS = ( # For CMS 'contentstore', + 'auth', 'github_sync', 'student', # misleading name due to sharing with lms diff --git a/cms/templates/manage_users.html b/cms/templates/manage_users.html new file mode 100644 index 0000000000..3887b4cbcb --- /dev/null +++ b/cms/templates/manage_users.html @@ -0,0 +1,38 @@ +<%inherit file="base.html" /> +<%block name="title">Course Editor Manager +<%include file="widgets/header.html"/> + +<%block name="content"> +
+ +

Course Editors

+ + +
+ + +
+
+ + + +
+ diff --git a/cms/urls.py b/cms/urls.py index ddd54adc65..8868e923de 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -20,6 +20,12 @@ 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$', + 'contentstore.views.add_user', name='add_user'), + url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/remove_user$', + 'contentstore.views.add_user', name='remove_user') ) # User creation and updating views From 580e620b2236681b3568764fd7acb3187ead36f9 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Oct 2012 10:38:35 -0400 Subject: [PATCH 2/6] add distinct 'admin' role separate from 'editors'. Only 'admins' should be able to add/remove editors. --- cms/djangoapps/auth/authz.py | 52 ++++++++++++++++++++-------- cms/djangoapps/contentstore/views.py | 20 +++++------ 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 710ea47182..fec25c5ba2 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -2,9 +2,14 @@ import logging import sys from django.contrib.auth.models import User, Group +from django.core.exceptions import PermissionDenied from xmodule.modulestore import Location +# define a couple of simple roles, we just need ADMIN and EDITOR now for our purposes +ADMIN_ROLE_NAME = 'admin' +EDITOR_ROLE_NAME = 'editor' + # 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 @@ -19,26 +24,45 @@ def get_users_in_course_group_by_role(location, role): return group.user_set.all() +''' +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) + + +def create_new_course_group(creator, location, role): + groupname = get_course_groupname_for_role(location, role) + (group, created) =Group.get_or_create(name=groupname) + if created: + group.save() + + creator.groups.add(group) + creator.save() + + return + + def add_user_to_course_group(caller, user, location, role): - # @todo: make sure caller has 'admin' permissions in the course + # only admins can add/remove other users + if not is_user_in_course_group_role(caller, location, ADMIN_ROLE_NAME): + raise PermissionDenied + if user.is_active and user.is_authenticated: groupname = get_course_groupname_for_role(location, role) - # see if the group exists, or create if new - (group, created) = Group.objects.get_or_create(name=groupname) - if created: - # if newly created, then we have to save it - group.save() - + group = Group.objects.get(name=groupname) user.groups.add(group) user.save() return True return False + def get_user_by_email(email): user = None - # try to look up user + # try to look up user, return None if not found try: user = User.objects.get(email=email) except: @@ -48,17 +72,17 @@ def get_user_by_email(email): def remove_user_from_course_group(caller, user, location, role): - # @todo: make sure caller has 'admin' permissions in the course + # only admins can add/remove other users + if not is_user_in_course_group_role(caller, location, ADMIN_ROLE_NAME): + raise PermissionDenied + # see if the user is actually in that role, if not then we don't have to do anything if is_user_in_course_group_role(user, location, role) == True: groupname = get_course_groupname_for_role(location, role) - # make sure the group actually exists group = Group.objects.get(name=groupname) - - if group is not None: - user.groups.remove(group) - user.save() + user.groups.remove(group) + user.save() def is_user_in_course_group_role(user, location, role): diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index c4b86e3d71..8d5c32b78d 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -40,7 +40,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 +from auth.authz import get_user_by_email, add_user_to_course_group, ADMIN_ROLE_NAME, EDITOR_ROLE_NAME from .utils import get_course_location_for_item log = logging.getLogger(__name__) @@ -91,7 +91,7 @@ def index(request): # ==== Views with per-item permissions================================ -def has_access(user, location, role='editor'): +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''' return is_user_in_course_group_role(user, get_course_location_for_item(location), role) @@ -535,11 +535,11 @@ def manage_users(request, org, course, name): location = ['i4x', org, course, 'course', name] # check that logged in user has permissions to this item - if not has_access(request.user, location): + if not has_access(request.user, location, role=ADMIN_ROLE_NAME): raise PermissionDenied() return render_to_response('manage_users.html', { - 'editors': get_users_in_course_group_by_role(location, 'editor') + 'editors': get_users_in_course_group_by_role(location, EDITOR_ROLE_NAME) }) @@ -565,8 +565,8 @@ def add_user(request, org, course, name): location = ['i4x', org, course, 'course', name] - # check that logged in user has permissions to this item - if not has_access(request.user, location): + # check that logged in user has admin permissions to this course + if not has_access(request.user, location, role=ADMIN_ROLE_NAME): raise PermissionDenied() user = get_user_by_email(email) @@ -580,7 +580,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') + add_user_to_course_group(request.user, user, location, EDITOR_ROLE_NAME) return create_json_response() @@ -595,13 +595,13 @@ def remove_user(request, org, course, name): location = ['i4x', org, course, 'course', name] - # check that logged in user has permissions to this item - if not has_access(request.user, location): + # check that logged in user has admin permissions on this course + if not has_access(request.user, location, role=ADMIN_ROLE_NAME): raise PermissionDenied() user = get_user_by_email(email) if user is not None: - remove_user_from_course_group(request.user, user, location, 'editor') + remove_user_from_course_group(request.user, user, location, EDITOR_ROLE_NAME) return create_json_response() From ea8e875d7d26333b030d8c037df88ba9cc74ccce Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Oct 2012 10:46:10 -0400 Subject: [PATCH 3/6] fix url mapping bug in url.py and include import reference to remove_user_from_course_group --- cms/djangoapps/contentstore/views.py | 5 +++-- cms/urls.py | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 8d5c32b78d..e8e359f0f1 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -40,7 +40,8 @@ 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, ADMIN_ROLE_NAME, EDITOR_ROLE_NAME +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 .utils import get_course_location_for_item log = logging.getLogger(__name__) @@ -598,7 +599,7 @@ def remove_user(request, org, course, name): # check that logged in user has admin permissions on this course if not has_access(request.user, location, role=ADMIN_ROLE_NAME): raise PermissionDenied() - + user = get_user_by_email(email) if user is not None: remove_user_from_course_group(request.user, user, location, EDITOR_ROLE_NAME) diff --git a/cms/urls.py b/cms/urls.py index 8868e923de..141abac2ac 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -25,7 +25,8 @@ urlpatterns = ('', url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/add_user$', 'contentstore.views.add_user', name='add_user'), url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/remove_user$', - 'contentstore.views.add_user', name='remove_user') + 'contentstore.views.remove_user', name='remove_user') + ) # User creation and updating views From d241d5553c04ddd2ed7ca465f8081c060fa4acaf Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Oct 2012 10:48:57 -0400 Subject: [PATCH 4/6] add error response when trying to remove a user which does not exist --- cms/djangoapps/contentstore/views.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index e8e359f0f1..9a9653a0c1 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -601,8 +601,10 @@ def remove_user(request, org, course, name): raise PermissionDenied() user = get_user_by_email(email) - if user is not None: - remove_user_from_course_group(request.user, user, location, EDITOR_ROLE_NAME) + 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) return create_json_response() From 116c7ed31d58c89e4d0aa9b6b781e95f8b86be24 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Oct 2012 10:52:43 -0400 Subject: [PATCH 5/6] fix typo bug on PermissionDenied exception raising --- cms/djangoapps/contentstore/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 9a9653a0c1..425b29f8bc 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -421,7 +421,7 @@ def clone_item(request): display_name = request.POST['name'] if not has_access(request.user, parent_location): - raise PermissionDenies() + raise PermissionDenied() parent = modulestore().get_item(parent_location) dest_location = parent_location._replace(category=template.category, name=Location.clean_for_url_name(display_name)) From f17b48fff304d1327462fe7b403ca1a6a9c64a25 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Oct 2012 15:19:05 -0400 Subject: [PATCH 6/6] fix problem introduced when rebasing master and manually resolving conflicts --- cms/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/urls.py b/cms/urls.py index 141abac2ac..bf391eb8e9 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -19,7 +19,7 @@ urlpatterns = ('', url(r'^preview/modx/(?P[^/]*)/(?P.*?)/(?P[^/]*)$', 'contentstore.views.preview_dispatch', name='preview_dispatch'), url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/upload_asset$', - 'contentstore.views.upload_asset', name='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$',