From ad3140a6e6743426afe1c6c2de697f073c5e89f1 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 8 Jan 2014 14:36:45 -0500 Subject: [PATCH] Cleaned up modules to reflect centralizing auth --- CHANGELOG.rst | 2 + cms/djangoapps/auth/__init__.py | 0 cms/djangoapps/auth/tests/__init__.py | 0 .../management/commands/clone_course.py | 2 +- .../management/commands/populate_creators.py | 17 +++++-- cms/djangoapps/contentstore/views/user.py | 51 +++++++++++-------- cms/envs/common.py | 1 - cms/templates/manage_users.html | 5 +- .../djangoapps/student}/tests/test_authz.py | 0 .../djangoapps/student}/tests/test_roles.py | 0 10 files changed, 47 insertions(+), 31 deletions(-) delete mode 100644 cms/djangoapps/auth/__init__.py delete mode 100644 cms/djangoapps/auth/tests/__init__.py rename {cms/djangoapps/auth => common/djangoapps/student}/tests/test_authz.py (100%) rename {lms/djangoapps/courseware => common/djangoapps/student}/tests/test_roles.py (100%) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b98865e301..569b194908 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -24,6 +24,8 @@ Studio: Newly-created courses default to being published on Jan 1, 2030 Studio: Added pagination to the Files & Uploads page. +Common: Centralized authorization mechanisms and removed the app-specific ones. + Blades: Video player improvements: - Disable edX controls on iPhone/iPod (native controls are used). - Disable unsupported controls (volume, playback rate) on iPad/Android. diff --git a/cms/djangoapps/auth/__init__.py b/cms/djangoapps/auth/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/cms/djangoapps/auth/tests/__init__.py b/cms/djangoapps/auth/tests/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/cms/djangoapps/contentstore/management/commands/clone_course.py b/cms/djangoapps/contentstore/management/commands/clone_course.py index 67f23d20eb..1336b2f0d1 100644 --- a/cms/djangoapps/contentstore/management/commands/clone_course.py +++ b/cms/djangoapps/contentstore/management/commands/clone_course.py @@ -10,7 +10,7 @@ from student.roles import CourseInstructorRole, CourseStaffRole # -# To run from command line: rake cms:clone SOURCE_LOC=edX/111/Foo1 DEST_LOC=edX/135/Foo3 +# To run from command line: ./manage.py cms clone_course --settings=dev master/300/cough edx/111/foo # class Command(BaseCommand): """Clone a MongoDB-backed course to another location""" diff --git a/cms/djangoapps/contentstore/management/commands/populate_creators.py b/cms/djangoapps/contentstore/management/commands/populate_creators.py index 37d647fd1a..900d1960cc 100644 --- a/cms/djangoapps/contentstore/management/commands/populate_creators.py +++ b/cms/djangoapps/contentstore/management/commands/populate_creators.py @@ -3,14 +3,14 @@ Script for granting existing course instructors course creator privileges. This script is only intended to be run once on a given environment. """ -from auth.authz import get_users_with_instructor_role, get_users_with_staff_role from course_creators.views import add_user_with_status_granted, add_user_with_status_unrequested from django.core.management.base import BaseCommand from django.contrib.auth.models import User from django.db.utils import IntegrityError +from student.roles import CourseInstructorRole, CourseStaffRole - +#------------ to run: ./manage.py cms populate_creators --settings=dev class Command(BaseCommand): """ Script for granting existing course instructors course creator privileges. @@ -32,13 +32,13 @@ class Command(BaseCommand): # the admin user will already exist. admin = User.objects.get(username=username, email=email) - for user in get_users_with_instructor_role(): + for user in get_users_with_role(CourseInstructorRole.ROLE): add_user_with_status_granted(admin, user) # Some users will be both staff and instructors. Those folks have been # added with status granted above, and add_user_with_status_unrequested # will not try to add them again if they already exist in the course creator database. - for user in get_users_with_staff_role(): + for user in get_users_with_role(CourseStaffRole.ROLE): add_user_with_status_unrequested(user) # There could be users who are not in either staff or instructor (they've @@ -46,3 +46,12 @@ class Command(BaseCommand): # when they first go to their dashboard. admin.delete() + + +#============================================================================================================= +# Because these are expensive and far-reaching, I moved them here +def get_users_with_role(role_prefix): + """ + An expensive operation which finds all users in the db with the given role prefix + """ + return User.objects.filter(groups__name__startswith=role_prefix) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index 468263d8d6..fb590b4450 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -133,17 +133,10 @@ def _course_team_user(request, locator, email): return JsonResponse(msg, 400) if request.method == "DELETE": - # remove all roles in this course from this user: but fail if the user - # is the last instructor in the course team - instructors = CourseInstructorRole(locator) - if instructors.has_user(user): - if instructors.users_with_role().count() == 1: - msg = { - "error": _("You may not remove the last instructor from a course") - } - return JsonResponse(msg, 400) - else: - instructors.remove_users(request.user, user) + try: + try_remove_instructor(request, locator, user) + except CannotOrphanCourse as oops: + return JsonResponse(oops.msg, 400) auth.remove_users(request.user, CourseStaffRole(locator), user) return JsonResponse() @@ -167,19 +160,33 @@ def _course_team_user(request, locator, email): # add to staff regardless (can't do after removing from instructors as will no longer # be allowed) auth.add_users(request.user, CourseStaffRole(locator), user) - # if we're trying to downgrade a user from "instructor" to "staff", - # make sure we have at least one other instructor in the course team. - instructors = CourseInstructorRole(locator) - if instructors.has_user(user): - if instructors.users_with_role().count() == 1: - msg = { - "error": _("You may not remove the last instructor from a course") - } - return JsonResponse(msg, 400) - else: - instructors.remove_users(request.user, user) + try: + try_remove_instructor(request, locator, user) + except CannotOrphanCourse as oops: + return JsonResponse(oops.msg, 400) # auto-enroll the course creator in the course so that "View Live" will work. CourseEnrollment.enroll(user, old_location.course_id) return JsonResponse() + + +class CannotOrphanCourse(Exception): + """ + Exception raised if an attempt is made to remove all responsible instructors from course. + """ + def __init__(self, msg): + self.msg = msg + Exception.__init__(self) + + +def try_remove_instructor(request, locator, user): + # remove all roles in this course from this user: but fail if the user + # is the last instructor in the course team + instructors = CourseInstructorRole(locator) + if instructors.has_user(user): + if instructors.users_with_role().count() == 1: + msg = {"error":_("You may not remove the last instructor from a course")} + raise CannotOrphanCourse(msg) + else: + auth.remove_users(request.user, instructors, user) diff --git a/cms/envs/common.py b/cms/envs/common.py index 999b344332..c5e5545384 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -409,7 +409,6 @@ INSTALLED_APPS = ( # For CMS 'contentstore', - 'auth', 'course_creators', 'student', # misleading name due to sharing with lms 'course_groups', # not used in cms (yet), but tests run diff --git a/cms/templates/manage_users.html b/cms/templates/manage_users.html index 5ad8aaea26..bac136ab04 100644 --- a/cms/templates/manage_users.html +++ b/cms/templates/manage_users.html @@ -2,7 +2,6 @@ <%! from django.utils.translation import ugettext as _ %> <%! from django.core.urlresolvers import reverse %> <%! from student.roles import CourseInstructorRole %> -<%! from student.auth import has_access %> <%! from xmodule.modulestore.django import loc_mapper %> <%inherit file="base.html" /> <%block name="title">${_("Course Team Settings")} @@ -66,7 +65,7 @@
  • - <% is_instuctor = has_access(user, CourseInstructorRole(context_course.location)) %> + <% is_instuctor = CourseInstructorRole(context_course.location).has_user(user) %> % if is_instuctor: @@ -121,7 +120,7 @@ % endfor - <% user_is_instuctor = has_access(request.user, CourseInstructorRole(context_course.location)) %> + <% user_is_instuctor = CourseInstructorRole(context_course.location).has_user(request.user) %> % if user_is_instuctor and len(staff) == 1:
    diff --git a/cms/djangoapps/auth/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py similarity index 100% rename from cms/djangoapps/auth/tests/test_authz.py rename to common/djangoapps/student/tests/test_authz.py diff --git a/lms/djangoapps/courseware/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py similarity index 100% rename from lms/djangoapps/courseware/tests/test_roles.py rename to common/djangoapps/student/tests/test_roles.py