From 2df3a6ef11b85ece2cc36a770a3942dc28bd7105 Mon Sep 17 00:00:00 2001
From: Victor Shnayder <victor@mitx.mit.edu>
Date: Tue, 14 Aug 2012 16:10:07 -0400
Subject: [PATCH] Big access control refactor
* All access control logic is now in access.py
* It exports a single method for general use: has_access(user, object, action)
- possible actions depend on object type (e.g. 'see_exists', 'enroll', 'staff')
* Removed DARK_LAUNCH feature flag--it is now the default behavior
* Replaced check_course with three separate more focused functions that use has_access
Minor things:
* note on using pdb in testing
* moved time parsing helper into timeparse.py
* x_modules now have a .start attribute (None if not in metadata)
---
common/djangoapps/student/views.py | 33 +--
common/lib/xmodule/xmodule/course_module.py | 26 +-
common/lib/xmodule/xmodule/timeparse.py | 11 +
common/lib/xmodule/xmodule/x_module.py | 23 +-
doc/development.md | 1 +
lms/djangoapps/courseware/access.py | 266 ++++++++++++++++++++
lms/djangoapps/courseware/courses.py | 122 +++------
lms/djangoapps/courseware/module_render.py | 20 +-
lms/djangoapps/courseware/tests/tests.py | 18 +-
lms/djangoapps/courseware/views.py | 50 ++--
lms/djangoapps/simplewiki/views.py | 26 +-
lms/djangoapps/staticbook/views.py | 8 +-
lms/envs/common.py | 5 +-
lms/templates/course_navigation.html | 4 +-
lms/templates/dashboard.html | 11 +-
lms/templates/portal/course_about.html | 9 +-
16 files changed, 414 insertions(+), 219 deletions(-)
create mode 100644 common/lib/xmodule/xmodule/timeparse.py
create mode 100644 lms/djangoapps/courseware/access.py
diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py
index ea1770109bf..b6aa62e03d0 100644
--- a/common/djangoapps/student/views.py
+++ b/common/djangoapps/student/views.py
@@ -38,8 +38,8 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
from datetime import date
from collections import namedtuple
-from courseware.courses import (course_staff_group_name, has_staff_access_to_course,
- get_courses_by_university)
+from courseware.courses import get_courses_by_university
+from courseware.access import has_access
log = logging.getLogger("mitx.student")
Article = namedtuple('Article', 'title url author image deck publication publish_date')
@@ -166,22 +166,6 @@ def change_enrollment_view(request):
"""Delegate to change_enrollment to actually do the work."""
return HttpResponse(json.dumps(change_enrollment(request)))
-def enrollment_allowed(user, course):
- """If the course has an enrollment period, check whether we are in it.
- Also respects the DARK_LAUNCH setting"""
- now = time.gmtime()
- start = course.enrollment_start
- end = course.enrollment_end
-
- if (start is None or now > start) and (end is None or now < end):
- # in enrollment period.
- return True
-
- if settings.MITX_FEATURES['DARK_LAUNCH']:
- if has_staff_access_to_course(user, course):
- # if dark launch, staff can enroll outside enrollment window
- return True
- return False
def change_enrollment(request):
@@ -209,18 +193,7 @@ def change_enrollment(request):
.format(user.username, enrollment.course_id))
return {'success': False, 'error': 'The course requested does not exist.'}
- if settings.MITX_FEATURES.get('ACCESS_REQUIRE_STAFF_FOR_COURSE'):
- # require that user be in the staff_* group (or be an
- # overall admin) to be able to enroll eg staff_6.002x or
- # staff_6.00x
- if not has_staff_access_to_course(user, course):
- staff_group = course_staff_group_name(course)
- log.debug('user %s denied enrollment to %s ; not in %s' % (
- user, course.location.url(), staff_group))
- return {'success': False,
- 'error' : '%s membership required to access course.' % staff_group}
-
- if not enrollment_allowed(user, course):
+ if not has_access(user, course, 'enroll'):
return {'success': False,
'error': 'enrollment in {} not allowed at this time'
.format(course.display_name)}
diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py
index e7d480f4e95..2c440cba2fd 100644
--- a/common/lib/xmodule/xmodule/course_module.py
+++ b/common/lib/xmodule/xmodule/course_module.py
@@ -1,12 +1,12 @@
from fs.errors import ResourceNotFoundError
import time
-import dateutil.parser
import logging
from xmodule.util.decorators import lazyproperty
from xmodule.graders import load_grading_policy
from xmodule.modulestore import Location
from xmodule.seq_module import SequenceDescriptor, SequenceModule
+from xmodule.timeparse import parse_time
log = logging.getLogger(__name__)
@@ -19,7 +19,7 @@ class CourseDescriptor(SequenceDescriptor):
msg = None
try:
- self.start = time.strptime(self.metadata["start"], "%Y-%m-%dT%H:%M")
+ self.start = parse_time(self.metadata["start"])
except KeyError:
msg = "Course loaded without a start date. id = %s" % self.id
except ValueError as e:
@@ -31,25 +31,8 @@ class CourseDescriptor(SequenceDescriptor):
log.critical(msg)
system.error_tracker(msg)
- def try_parse_time(key):
- """
- Parse an optional metadata key: if present, must be valid.
- Return None if not present.
- """
- if key in self.metadata:
- try:
- return time.strptime(self.metadata[key], "%Y-%m-%dT%H:%M")
- except ValueError as e:
- msg = "Course %s loaded with a bad metadata key %s '%s'" % (
- self.id, self.metadata[key], e)
- log.warning(msg)
- return None
-
- self.enrollment_start = try_parse_time("enrollment_start")
- self.enrollment_end = try_parse_time("enrollment_end")
-
-
-
+ self.enrollment_start = self._try_parse_time("enrollment_start")
+ self.enrollment_end = self._try_parse_time("enrollment_end")
def has_started(self):
return time.gmtime() > self.start
@@ -154,6 +137,7 @@ class CourseDescriptor(SequenceDescriptor):
@property
def id(self):
+ """Return the course_id for this course"""
return self.location_to_id(self.location)
@property
diff --git a/common/lib/xmodule/xmodule/timeparse.py b/common/lib/xmodule/xmodule/timeparse.py
new file mode 100644
index 00000000000..28a437bd5f3
--- /dev/null
+++ b/common/lib/xmodule/xmodule/timeparse.py
@@ -0,0 +1,11 @@
+"""
+Helper functions for handling time in the format we like.
+"""
+import time
+
+def parse_time(time_str):
+ """
+ Takes a time string in our format ("%Y-%m-%dT%H:%M"), and returns
+ it as a time_struct. Raises ValueError if the string is not in the right format.
+ """
+ return time.strptime(time_str, "%Y-%m-%dT%H:%M")
diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py
index 071e4539010..94f8010e100 100644
--- a/common/lib/xmodule/xmodule/x_module.py
+++ b/common/lib/xmodule/xmodule/x_module.py
@@ -8,8 +8,9 @@ from lxml import etree
from lxml.etree import XMLSyntaxError
from pprint import pprint
-from xmodule.modulestore import Location
from xmodule.errortracker import exc_info_to_str
+from xmodule.modulestore import Location
+from xmodule.timeparse import parse_time
log = logging.getLogger('mitx.' + __name__)
@@ -384,6 +385,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
self.category = self.location.category
self.shared_state_key = kwargs.get('shared_state_key')
+ # look for a start time, setting to None if not present
+ self.start = self._try_parse_time('start')
+
self._child_instances = None
self._inherited_metadata = set()
@@ -596,6 +600,23 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
metadata=self.metadata
))
+ # ================================ Internal helpers =======================
+
+ def _try_parse_time(self, key):
+ """
+ Parse an optional metadata key containing a time: if present, must be valid.
+ Return None if not present.
+ """
+ if key in self.metadata:
+ try:
+ parse_time(self.metadata[key])
+ except ValueError as e:
+ msg = "Descriptor {} loaded with a bad metadata key '{}': '{}'".format(
+ self.location.url(), self.metadata[key], e)
+ log.warning(msg)
+ return None
+
+
class DescriptorSystem(object):
def __init__(self, load_item, resources_fs, error_tracker, **kwargs):
diff --git a/doc/development.md b/doc/development.md
index 44965cb0ded..590a935405f 100644
--- a/doc/development.md
+++ b/doc/development.md
@@ -65,3 +65,4 @@ To run a single nose test:
nosetests common/lib/xmodule/xmodule/tests/test_stringify.py:test_stringify
+Very handy: if you uncomment the `--pdb` argument in `NOSE_ARGS` in `lms/envs/test.py`, it will drop you into pdb on error. This lets you go up and down the stack and see what the values of the variables are. Check out http://docs.python.org/library/pdb.html
diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py
new file mode 100644
index 00000000000..a8145aa1cea
--- /dev/null
+++ b/lms/djangoapps/courseware/access.py
@@ -0,0 +1,266 @@
+"""This file contains (or should), all access control logic for the courseware.
+Ideally, it will be the only place that needs to know about any special settings
+like DISABLE_START_DATES"""
+
+import logging
+import time
+
+from django.conf import settings
+
+from xmodule.course_module import CourseDescriptor
+from xmodule.modulestore import Location
+from xmodule.timeparse import parse_time
+from xmodule.x_module import XModule, XModuleDescriptor
+
+
+DEBUG_ACCESS = True
+
+log = logging.getLogger(__name__)
+
+def debug(*args, **kwargs):
+ # to avoid overly verbose output, this is off by default
+ if DEBUG_ACCESS:
+ log.debug(*args, **kwargs)
+
+def has_access(user, obj, action):
+ """
+ Check whether a user has the access to do action on obj. Handles any magic
+ switching based on various settings.
+
+ Things this module understands:
+ - start dates for modules
+ - DISABLE_START_DATES
+ - different access for staff, course staff, and students.
+
+ user: a Django user object. May be anonymous.
+
+ obj: The object to check access for. For now, a module or descriptor.
+
+ action: A string specifying the action that the client is trying to perform.
+
+ actions depend on the obj type, but include e.g. 'enroll' for courses. See the
+ type-specific functions below for the known actions for that type.
+
+ Returns a bool. It is up to the caller to actually deny access in a way
+ that makes sense in context.
+ """
+ # delegate the work to type-specific functions.
+ # (start with more specific types, then get more general)
+ if isinstance(obj, CourseDescriptor):
+ return _has_access_course_desc(user, obj, action)
+
+ if isinstance(obj, XModuleDescriptor):
+ return _has_access_descriptor(user, obj, action)
+
+ if isinstance(obj, XModule):
+ return _has_access_xmodule(user, obj, action)
+
+ if isinstance(obj, Location):
+ return _has_access_location(user, obj, action)
+
+ # Passing an unknown object here is a coding error, so rather than
+ # returning a default, complain.
+ raise TypeError("Unknown object type in has_access(). Object type: '{}'"
+ .format(type(obj)))
+
+# ================ Implementation helpers ================================
+
+def _has_access_course_desc(user, course, action):
+ """
+ Check if user has access to a course descriptor.
+
+ Valid actions:
+
+ 'load' -- load the courseware, see inside the course
+ 'enroll' -- enroll. Checks for enrollment window,
+ ACCESS_REQUIRE_STAFF_FOR_COURSE,
+ 'see_exists' -- can see that the course exists.
+ 'staff' -- staff access to course.
+ """
+ def can_load():
+ "Can this user load this course?"
+ # delegate to generic descriptor check
+ return _has_access_descriptor(user, course, action)
+
+ def can_enroll():
+ """
+ If the course has an enrollment period, check whether we are in it.
+ (staff can always enroll)
+ """
+
+ now = time.gmtime()
+ start = course.enrollment_start
+ end = course.enrollment_end
+
+ if (start is None or now > start) and (end is None or now < end):
+ # in enrollment period, so any user is allowed to enroll.
+ return True
+
+ # otherwise, need staff access
+ return _has_staff_access_to_descriptor(user, course)
+
+ def see_exists():
+ """
+ Can see if can enroll, but also if can load it: if user enrolled in a course and now
+ it's past the enrollment period, they should still see it.
+
+ TODO (vshnayder): This means that courses with limited enrollment periods will not appear
+ to non-staff visitors after the enrollment period is over. If this is not what we want, will
+ need to change this logic.
+ """
+ # VS[compat] -- this setting should go away once all courses have
+ # properly configured enrollment_start times (if course should be
+ # staff-only, set enrollment_start far in the future.)
+ if settings.MITX_FEATURES.get('ACCESS_REQUIRE_STAFF_FOR_COURSE'):
+ # if this feature is on, only allow courses that have ispublic set to be
+ # seen by non-staff
+ if course.metadata.get('ispublic'):
+ return True
+ return _has_staff_access_to_descriptor(user, course)
+
+ return can_enroll() or can_load()
+
+ checkers = {
+ 'load': can_load,
+ 'enroll': can_enroll,
+ 'see_exists': see_exists,
+ 'staff': lambda: _has_staff_access_to_descriptor(user, course)
+ }
+
+ return _dispatch(checkers, action, user, course)
+
+
+def _has_access_descriptor(user, descriptor, action):
+ """
+ Check if user has access to this descriptor.
+
+ Valid actions:
+ 'load' -- load this descriptor, showing it to the user.
+ 'staff' -- staff access to descriptor.
+
+ NOTE: This is the fallback logic for descriptors that don't have custom policy
+ (e.g. courses). If you call this method directly instead of going through
+ has_access(), it will not do the right thing.
+ """
+ def can_load():
+ # If start dates are off, can always load
+ if settings.MITX_FEATURES['DISABLE_START_DATES']:
+ return True
+
+ # Check start date
+ if descriptor.start is not None:
+ now = time.gmtime()
+ if now > descriptor.start:
+ # after start date, everyone can see it
+ return True
+ # otherwise, need staff access
+ return _has_staff_access_to_descriptor(user, descriptor)
+
+ # No start date, so can always load.
+ return True
+
+ checkers = {
+ 'load': can_load,
+ 'staff': lambda: _has_staff_access_to_descriptor(user, descriptor)
+ }
+
+ return _dispatch(checkers, action, user, descriptor)
+
+
+
+
+def _has_access_xmodule(user, xmodule, action):
+ """
+ Check if user has access to this xmodule.
+
+ Valid actions:
+ - same as the valid actions for xmodule.descriptor
+ """
+ # Delegate to the descriptor
+ return has_access(user, xmodule.descriptor, action)
+
+
+def _has_access_location(user, location, action):
+ """
+ Check if user has access to this location.
+
+ Valid actions:
+ 'staff' : True if the user has staff access to this location
+
+ NOTE: if you add other actions, make sure that
+
+ has_access(user, location, action) == has_access(user, get_item(location), action)
+
+ And in general, prefer checking access on loaded items, rather than locations.
+ """
+ checkers = {
+ 'staff': lambda: _has_staff_access_to_location(user, location)
+ }
+
+ return _dispatch(checkers, action, user, location)
+
+
+##### Internal helper methods below
+
+def _dispatch(table, action, user, obj):
+ """
+ Helper: call table[action], raising a nice pretty error if there is no such key.
+
+ user and object passed in only for error messages and debugging
+ """
+ if action in table:
+ result = table[action]()
+ debug("%s user %s, object %s, action %s",
+ 'ALLOWED' if result else 'DENIED',
+ user, obj, action)
+ return result
+
+ raise ValueError("Unknown action for object type '{}': '{}'".format(
+ type(obj), action))
+
+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.
+ """
+ return 'staff_%s' % Location(location).course
+
+def _has_staff_access_to_location(user, location):
+ '''
+ Returns True if the given user has staff access to a location. For now this
+ is equivalent to having staff access to the course location.course.
+
+ This means that user is in the staff_* group, or is an overall admin.
+
+ TODO (vshnayder): this needs to be changed to allow per-course_id permissions, not per-course
+ (e.g. staff in 2012 is different from 2013, but maybe some people always have access)
+
+ course is a string: the course field of the location being accessed.
+ '''
+ if user is None or (not user.is_authenticated()):
+ return False
+ if user.is_staff:
+ return True
+
+ # If not global staff, is the user in the Auth group for this class?
+ user_groups = [x[1] for x in user.groups.values_list()]
+ staff_group = _course_staff_group_name(location)
+ if staff_group in user_groups:
+ return True
+ return False
+
+def _has_staff_access_to_course_id(user, course_id):
+ """Helper method that takes a course_id instead of a course name"""
+ loc = CourseDescriptor.id_to_location(course_id)
+ return _has_staff_access_to_location(user, loc)
+
+
+def _has_staff_access_to_descriptor(user, descriptor):
+ """Helper method that checks whether the user has staff access to
+ the course of the location.
+
+ location: something that can be passed to Location
+ """
+ return _has_staff_access_to_location(user, descriptor.location)
+
diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py
index ffdf0650a66..2e748537609 100644
--- a/lms/djangoapps/courseware/courses.py
+++ b/lms/djangoapps/courseware/courses.py
@@ -12,49 +12,51 @@ from xmodule.modulestore import Location
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.exceptions import ItemNotFoundError
from static_replace import replace_urls, try_staticfiles_lookup
+from courseware.access import has_access
log = logging.getLogger(__name__)
-def check_course(user, course_id, course_must_be_open=True, course_required=True):
+def get_course_by_id(course_id):
"""
- Given a django user and a course_id, this returns the course
- object. By default, if the course is not found or the course is
- not open yet, this method will raise a 404.
+ Given a course id, return the corresponding course descriptor.
- If course_must_be_open is False, the course will be returned
- without a 404 even if it is not open.
-
- If course_required is False, a course_id of None is acceptable. The
- course returned will be None. Even if the course is not required,
- if a course_id is given that does not exist a 404 will be raised.
-
- This behavior is modified by MITX_FEATURES['DARK_LAUNCH']:
- if dark launch is enabled, course_must_be_open is ignored for
- users that have staff access.
+ If course_id is not valid, raises a 404.
"""
- course = None
- if course_required or course_id:
- try:
- course_loc = CourseDescriptor.id_to_location(course_id)
- course = modulestore().get_item(course_loc)
+ try:
+ course_loc = CourseDescriptor.id_to_location(course_id)
+ return modulestore().get_item(course_loc)
+ except (KeyError, ItemNotFoundError):
+ raise Http404("Course not found.")
- except (KeyError, ItemNotFoundError):
- raise Http404("Course not found.")
- started = course.has_started() or settings.MITX_FEATURES['DISABLE_START_DATES']
- must_be_open = course_must_be_open
- if (settings.MITX_FEATURES['DARK_LAUNCH'] and
- has_staff_access_to_course(user, course)):
- must_be_open = False
-
- if must_be_open and not started:
- raise Http404("This course has not yet started.")
+def get_course_with_access(user, course_id, action):
+ """
+ Given a course_id, look up the corresponding course descriptor,
+ check that the user has the access to perform the specified action
+ on the course, and return the descriptor.
+ Raises a 404 if the course_id is invalid, or the user doesn't have access.
+ """
+ course = get_course_by_id(course_id)
+ if not has_access(user, course, action):
+ # Deliberately return a non-specific error message to avoid
+ # leaking info about access control settings
+ raise Http404("Course not found.")
return course
+def get_opt_course_with_access(user, course_id, action):
+ """
+ Same as get_course_with_access, except that if course_id is None,
+ return None without performing any access checks.
+ """
+ if course_id is None:
+ return None
+ return get_course_with_access(user, course_id, action)
+
+
def course_image_url(course):
"""Try to look up the image url for the course. If it's not found,
log an error and return the dead link"""
@@ -140,66 +142,10 @@ def get_course_info_section(course, section_key):
raise KeyError("Invalid about key " + str(section_key))
-def course_staff_group_name(course):
- '''
- course should be either a CourseDescriptor instance, or a string (the
- .course entry of a Location)
- '''
- if isinstance(course, str) or isinstance(course, unicode):
- coursename = course
- else:
- # should be a CourseDescriptor, so grab its location.course:
- coursename = course.location.course
- return 'staff_%s' % coursename
-
-def has_staff_access_to_course(user, course):
- '''
- Returns True if the given user has staff access to the course.
- This means that user is in the staff_* group, or is an overall admin.
- TODO (vshnayder): this needs to be changed to allow per-course_id permissions, not per-course
- (e.g. staff in 2012 is different from 2013, but maybe some people always have access)
-
- course is the course field of the location being accessed.
- '''
- if user is None or (not user.is_authenticated()) or course is None:
- return False
- if user.is_staff:
- return True
-
- # note this is the Auth group, not UserTestGroup
- user_groups = [x[1] for x in user.groups.values_list()]
- staff_group = course_staff_group_name(course)
- if staff_group in user_groups:
- return True
- return False
-
-def has_staff_access_to_course_id(user, course_id):
- """Helper method that takes a course_id instead of a course name"""
- loc = CourseDescriptor.id_to_location(course_id)
- return has_staff_access_to_course(user, loc.course)
-
-
-def has_staff_access_to_location(user, location):
- """Helper method that checks whether the user has staff access to
- the course of the location.
-
- location: something that can be passed to Location
- """
- return has_staff_access_to_course(user, Location(location).course)
-
-def has_access_to_course(user, course):
- '''course is the .course element of a location'''
- if course.metadata.get('ispublic'):
- return True
- return has_staff_access_to_course(user,course)
-
def get_courses_by_university(user):
'''
Returns dict of lists of courses available, keyed by course.org (ie university).
Courses are sorted by course.number.
-
- if ACCESS_REQUIRE_STAFF_FOR_COURSE then list only includes those accessible
- to user.
'''
# TODO: Clean up how 'error' is done.
# filter out any courses that errored.
@@ -208,9 +154,7 @@ def get_courses_by_university(user):
courses = sorted(courses, key=lambda course: course.number)
universities = defaultdict(list)
for course in courses:
- if settings.MITX_FEATURES.get('ACCESS_REQUIRE_STAFF_FOR_COURSE'):
- if not has_access_to_course(user,course):
- continue
- universities[course.org].append(course)
+ if has_access(user, course, 'see_exists'):
+ universities[course.org].append(course)
return universities
diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py
index 5184cd9866e..768139c2f07 100644
--- a/lms/djangoapps/courseware/module_render.py
+++ b/lms/djangoapps/courseware/module_render.py
@@ -2,25 +2,25 @@ import json
import logging
from django.conf import settings
+from django.contrib.auth.models import User
from django.core.urlresolvers import reverse
from django.http import Http404
from django.http import HttpResponse
from django.views.decorators.csrf import csrf_exempt
+from requests.auth import HTTPBasicAuth
+
from capa.xqueue_interface import XQueueInterface
-from django.contrib.auth.models import User
-from xmodule.modulestore.django import modulestore
+from courseware.access import has_access
from mitxmako.shortcuts import render_to_string
from models import StudentModule, StudentModuleCache
from static_replace import replace_urls
from xmodule.exceptions import NotFoundError
+from xmodule.modulestore import Location
+from xmodule.modulestore.django import modulestore
from xmodule.x_module import ModuleSystem
from xmodule_modifiers import replace_static_urls, add_histogram, wrap_xmodule
-from courseware.courses import (has_staff_access_to_course,
- has_staff_access_to_location)
-from requests.auth import HTTPBasicAuth
-
log = logging.getLogger("mitx.courseware")
@@ -140,6 +140,8 @@ def get_module(user, request, location, student_module_cache, position=None):
Returns: xmodule instance
'''
+ # has_access below needs an actual Location
+ location = Location(location)
descriptor = modulestore().get_item(location)
#TODO Only check the cache if this module can possibly have state
@@ -163,7 +165,7 @@ def get_module(user, request, location, student_module_cache, position=None):
# TODO (vshnayder): fix hardcoded urls (use reverse)
# Setup system context for module instance
- ajax_url = reverse('modx_dispatch',
+ ajax_url = reverse('modx_dispatch',
kwargs=dict(course_id=descriptor.location.course_id,
id=descriptor.location.url(),
dispatch=''),
@@ -208,7 +210,7 @@ def get_module(user, request, location, student_module_cache, position=None):
# a module is coming through get_html and is therefore covered
# by the replace_static_urls code below
replace_urls=replace_urls,
- is_staff=has_staff_access_to_location(user, location),
+ is_staff=has_access(user, location, 'staff'),
node_path=settings.NODE_PATH
)
# pass position specified in URL to module through ModuleSystem
@@ -223,7 +225,7 @@ def get_module(user, request, location, student_module_cache, position=None):
)
if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF'):
- if has_staff_access_to_course(user, module.location.course):
+ if has_access(user, module, 'staff'):
module.get_html = add_histogram(module.get_html, module, user)
return module
diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py
index daffa44d2ae..8eb9c225844 100644
--- a/lms/djangoapps/courseware/tests/tests.py
+++ b/lms/djangoapps/courseware/tests/tests.py
@@ -18,8 +18,10 @@ from override_settings import override_settings
import xmodule.modulestore.django
+# Need access to internal func to put users in the right group
+from courseware.access import _course_staff_group_name
+
from student.models import Registration
-from courseware.courses import course_staff_group_name
from xmodule.modulestore.django import modulestore
from xmodule.modulestore import Location
from xmodule.modulestore.xml_importer import import_from_xml
@@ -310,7 +312,7 @@ class TestViewAuth(PageLoader):
self.check_for_get_code(404, url)
# Make the instructor staff in the toy course
- group_name = course_staff_group_name(self.toy)
+ group_name = _course_staff_group_name(self.toy.location)
g = Group.objects.create(name=group_name)
g.user_set.add(user(self.instructor))
@@ -340,25 +342,22 @@ class TestViewAuth(PageLoader):
def run_wrapped(self, test):
"""
- test.py turns off start dates. Enable them and DARK_LAUNCH.
+ test.py turns off start dates. Enable them.
Because settings is global, be careful not to mess it up for other tests
(Can't use override_settings because we're only changing part of the
MITX_FEATURES dict)
"""
oldDSD = settings.MITX_FEATURES['DISABLE_START_DATES']
- oldDL = settings.MITX_FEATURES['DARK_LAUNCH']
try:
settings.MITX_FEATURES['DISABLE_START_DATES'] = False
- settings.MITX_FEATURES['DARK_LAUNCH'] = True
test()
finally:
settings.MITX_FEATURES['DISABLE_START_DATES'] = oldDSD
- settings.MITX_FEATURES['DARK_LAUNCH'] = oldDL
def test_dark_launch(self):
- """Make sure that when dark launch is on, students can't access course
+ """Make sure that before course start, students can't access course
pages, but instructors can"""
self.run_wrapped(self._do_test_dark_launch)
@@ -378,7 +377,6 @@ class TestViewAuth(PageLoader):
self.assertFalse(self.toy.has_started())
self.assertFalse(self.full.has_started())
self.assertFalse(settings.MITX_FEATURES['DISABLE_START_DATES'])
- self.assertTrue(settings.MITX_FEATURES['DARK_LAUNCH'])
def reverse_urls(names, course):
"""Reverse a list of course urls"""
@@ -444,7 +442,7 @@ class TestViewAuth(PageLoader):
print '=== Testing course instructor access....'
# Make the instructor staff in the toy course
- group_name = course_staff_group_name(self.toy)
+ group_name = _course_staff_group_name(self.toy.location)
g = Group.objects.create(name=group_name)
g.user_set.add(user(self.instructor))
@@ -494,7 +492,7 @@ class TestViewAuth(PageLoader):
print '=== Testing course instructor access....'
# Make the instructor staff in the toy course
- group_name = course_staff_group_name(self.toy)
+ group_name = _course_staff_group_name(self.toy.location)
g = Group.objects.create(name=group_name)
g.user_set.add(user(self.instructor))
diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py
index 00ab45e605f..f54a76da1f6 100644
--- a/lms/djangoapps/courseware/views.py
+++ b/lms/djangoapps/courseware/views.py
@@ -15,21 +15,19 @@ from mitxmako.shortcuts import render_to_response, render_to_string
from django_future.csrf import ensure_csrf_cookie
from django.views.decorators.cache import cache_control
-from module_render import toc_for_course, get_module, get_section
+from courseware import grades
+from courseware.access import has_access
+from courseware.courses import (get_course_with_access, get_courses_by_university)
from models import StudentModuleCache
+from module_render import toc_for_course, get_module, get_section
from student.models import UserProfile
+from student.models import UserTestGroup, CourseEnrollment
+from util.cache import cache, cache_if_anonymous
+from xmodule.course_module import CourseDescriptor
from xmodule.modulestore import Location
-from xmodule.modulestore.search import path_to_location
-from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
from xmodule.modulestore.django import modulestore
-from xmodule.course_module import CourseDescriptor
-
-from util.cache import cache, cache_if_anonymous
-from student.models import UserTestGroup, CourseEnrollment
-from courseware import grades
-from courseware.courses import (check_course, get_courses_by_university,
- has_staff_access_to_course_id)
-
+from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
+from xmodule.modulestore.search import path_to_location
log = logging.getLogger("mitx.courseware")
@@ -93,7 +91,8 @@ def render_accordion(request, course, chapter, section):
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
def index(request, course_id, chapter=None, section=None,
position=None):
- ''' Displays courseware accordion, and any associated content.
+ """
+ Displays courseware accordion, and any associated content.
If course, chapter, and section aren't all specified, just returns
the accordion. If they are specified, returns an error if they don't
point to a valid module.
@@ -109,8 +108,8 @@ def index(request, course_id, chapter=None, section=None,
Returns:
- HTTPresponse
- '''
- course = check_course(request.user, course_id)
+ """
+ course = get_course_with_access(request.user, course_id, 'load')
registered = registered_for_course(course, request.user)
if not registered:
# TODO (vshnayder): do course instructors need to be registered to see course?
@@ -204,7 +203,7 @@ def course_info(request, course_id):
Assumes the course_id is in a valid format.
"""
- course = check_course(request.user, course_id)
+ course = get_course_with_access(request.user, course_id, 'load')
return render_to_response('info.html', {'course': course})
@@ -221,7 +220,7 @@ def registered_for_course(course, user):
@ensure_csrf_cookie
@cache_if_anonymous
def course_about(request, course_id):
- course = check_course(request.user, course_id, course_must_be_open=False)
+ course = get_course_with_access(request.user, course_id, 'see_exists')
registered = registered_for_course(course, request.user)
return render_to_response('portal/course_about.html', {'course': course, 'registered': registered})
@@ -253,14 +252,14 @@ def profile(request, course_id, student_id=None):
Course staff are allowed to see the profiles of students in their class.
"""
- course = check_course(request.user, course_id)
+ course = get_course_with_access(request.user, course_id, 'load')
if student_id is None or student_id == request.user.id:
# always allowed to see your own profile
student = request.user
else:
# Requesting access to a different student's profile
- if not has_staff_access_to_course_id(request.user, course_id):
+ if not has_access(request.user, course, 'staff'):
raise Http404
student = User.objects.get(id=int(student_id))
@@ -297,10 +296,7 @@ def gradebook(request, course_id):
- only displayed to course staff
- shows students who are enrolled.
"""
- if not has_staff_access_to_course_id(request.user, course_id):
- raise Http404
-
- course = check_course(request.user, course_id)
+ course = get_course_with_access(request.user, course_id, 'staff')
enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username')
@@ -322,10 +318,7 @@ def gradebook(request, course_id):
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
def grade_summary(request, course_id):
"""Display the grade summary for a course."""
- if not has_staff_access_to_course_id(request.user, course_id):
- raise Http404
-
- course = check_course(request.user, course_id)
+ course = get_course_with_access(request.user, course_id, 'staff')
# For now, just a static page
context = {'course': course }
@@ -335,11 +328,10 @@ def grade_summary(request, course_id):
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
def instructor_dashboard(request, course_id):
"""Display the instructor dashboard for a course."""
- if not has_staff_access_to_course_id(request.user, course_id):
+ course = get_course_with_access(request.user, course_id, 'staff')
+ if not has_access(request.user, course, 'staff'):
raise Http404
- course = check_course(request.user, course_id)
-
# For now, just a static page
context = {'course': course }
return render_to_response('instructor_dashboard.html', context)
diff --git a/lms/djangoapps/simplewiki/views.py b/lms/djangoapps/simplewiki/views.py
index a6bb192fd7c..2ee76a18685 100644
--- a/lms/djangoapps/simplewiki/views.py
+++ b/lms/djangoapps/simplewiki/views.py
@@ -9,7 +9,7 @@ from django.utils import simplejson
from django.utils.translation import ugettext_lazy as _
from mitxmako.shortcuts import render_to_response
-from courseware.courses import check_course
+from courseware.courses import get_opt_course_with_access
from xmodule.course_module import CourseDescriptor
from xmodule.modulestore.django import modulestore
@@ -51,7 +51,7 @@ def update_template_dictionary(dictionary, request=None, course=None, article=No
def view(request, article_path, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
(article, err) = get_article(request, article_path, course)
if err:
@@ -67,7 +67,7 @@ def view(request, article_path, course_id=None):
def view_revision(request, revision_number, article_path, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
(article, err) = get_article(request, article_path, course)
if err:
@@ -91,7 +91,7 @@ def view_revision(request, revision_number, article_path, course_id=None):
def root_redirect(request, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
#TODO: Add a default namespace to settings.
namespace = course.wiki_namespace if course else "edX"
@@ -109,7 +109,7 @@ def root_redirect(request, course_id=None):
def create(request, article_path, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
article_path_components = article_path.split('/')
@@ -170,7 +170,7 @@ def create(request, article_path, course_id=None):
def edit(request, article_path, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
(article, err) = get_article(request, article_path, course)
if err:
@@ -218,7 +218,7 @@ def edit(request, article_path, course_id=None):
def history(request, article_path, page=1, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
(article, err) = get_article(request, article_path, course)
if err:
@@ -300,7 +300,7 @@ def history(request, article_path, page=1, course_id=None):
def revision_feed(request, page=1, namespace=None, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
page_size = 10
@@ -333,7 +333,7 @@ def revision_feed(request, page=1, namespace=None, course_id=None):
def search_articles(request, namespace=None, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
# blampe: We should check for the presence of other popular django search
# apps and use those if possible. Only fall back on this as a last resort.
@@ -382,7 +382,7 @@ def search_articles(request, namespace=None, course_id=None):
def search_add_related(request, course_id, slug, namespace):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
(article, err) = get_article(request, slug, namespace if namespace else course_id)
if err:
@@ -415,7 +415,7 @@ def search_add_related(request, course_id, slug, namespace):
def add_related(request, course_id, slug, namespace):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
(article, err) = get_article(request, slug, namespace if namespace else course_id)
if err:
@@ -439,7 +439,7 @@ def add_related(request, course_id, slug, namespace):
def remove_related(request, course_id, namespace, slug, related_id):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
(article, err) = get_article(request, slug, namespace if namespace else course_id)
@@ -462,7 +462,7 @@ def remove_related(request, course_id, namespace, slug, related_id):
def random_article(request, course_id=None):
- course = check_course(request.user, course_id, course_required=False)
+ course = get_opt_course_with_access(request.user, course_id, 'load')
from random import randint
num_arts = Article.objects.count()
diff --git a/lms/djangoapps/staticbook/views.py b/lms/djangoapps/staticbook/views.py
index e63619756ca..aec3fb1448e 100644
--- a/lms/djangoapps/staticbook/views.py
+++ b/lms/djangoapps/staticbook/views.py
@@ -1,15 +1,17 @@
from django.contrib.auth.decorators import login_required
from mitxmako.shortcuts import render_to_response
-from courseware.courses import check_course
+from courseware.courses import get_course_with_access
from lxml import etree
@login_required
def index(request, course_id, page=0):
- course = check_course(request.user, course_id)
+ course = get_course_with_access(request.user, course_id, 'load')
raw_table_of_contents = open('lms/templates/book_toc.xml', 'r') # TODO: This will need to come from S3
table_of_contents = etree.parse(raw_table_of_contents).getroot()
- return render_to_response('staticbook.html', {'page': int(page), 'course': course, 'table_of_contents': table_of_contents})
+ return render_to_response('staticbook.html',
+ {'page': int(page), 'course': course,
+ 'table_of_contents': table_of_contents})
def index_shifted(request, course_id, page):
diff --git a/lms/envs/common.py b/lms/envs/common.py
index 54c4e6e8bd4..303e73ea812 100644
--- a/lms/envs/common.py
+++ b/lms/envs/common.py
@@ -48,7 +48,6 @@ MITX_FEATURES = {
## DO NOT SET TO True IN THIS FILE
## Doing so will cause all courses to be released on production
'DISABLE_START_DATES': False, # When True, all courses will be active, regardless of start date
- 'DARK_LAUNCH': False, # When True, courses will be active for staff only
'ENABLE_TEXTBOOK' : True,
'ENABLE_DISCUSSION' : True,
@@ -95,12 +94,12 @@ system_node_path = os.environ.get("NODE_PATH", None)
if system_node_path is None:
system_node_path = "/usr/local/lib/node_modules"
-node_paths = [COMMON_ROOT / "static/js/vendor",
+node_paths = [COMMON_ROOT / "static/js/vendor",
COMMON_ROOT / "static/coffee/src",
system_node_path
]
NODE_PATH = ':'.join(node_paths)
-
+
################################## MITXWEB #####################################
# This is where we stick our compiled template files. Most of the app uses Mako
# templates
diff --git a/lms/templates/course_navigation.html b/lms/templates/course_navigation.html
index ea889ce6c72..dd1c8d93b9c 100644
--- a/lms/templates/course_navigation.html
+++ b/lms/templates/course_navigation.html
@@ -7,7 +7,7 @@ def url_class(url):
return ""
%>
<%! from django.core.urlresolvers import reverse %>
-<%! from courseware.courses import has_staff_access_to_course_id %>
+<%! from courseware.access import has_access %>
<nav class="${active_page} course-material">
<div class="inner-wrapper">
@@ -28,7 +28,7 @@ def url_class(url):
% if user.is_authenticated():
<li class="profile"><a href="${reverse('profile', args=[course.id])}" class="${url_class('profile')}">Profile</a></li>
% endif
-% if has_staff_access_to_course_id(user, course.id):
+% if has_access(user, course, 'staff'):
<li class="instructor"><a href="${reverse('instructor_dashboard', args=[course.id])}" class="${url_class('instructor')}">Instructor</a></li>
% endif
diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html
index fc8e9abf300..cfe8a0953cb 100644
--- a/lms/templates/dashboard.html
+++ b/lms/templates/dashboard.html
@@ -1,6 +1,7 @@
-<%!
- from django.core.urlresolvers import reverse
+<%!
+ from django.core.urlresolvers import reverse
from courseware.courses import course_image_url, get_course_about_section
+ from courseware.access import has_access
%>
<%inherit file="main.html" />
@@ -17,7 +18,7 @@
$("#unenroll_course_number").text( $(event.target).data("course-number") );
});
-
+
$(document).delegate('#unenroll_form', 'ajax:success', function(data, json, xhr) {
if(json.success) {
location.href="${reverse('dashboard')}";
@@ -33,7 +34,7 @@
</%block>
<section class="container dashboard">
-
+
%if message:
<section class="dashboard-banner">
${message}
@@ -66,7 +67,7 @@
<article class="my-course">
<%
- if course.has_started() or settings.MITX_FEATURES['DISABLE_START_DATES']:
+ if has_access(user, course, 'load'):
course_target = reverse('info', args=[course.id])
else:
course_target = reverse('about_course', args=[course.id])
diff --git a/lms/templates/portal/course_about.html b/lms/templates/portal/course_about.html
index 36ec33607fd..4931f1fed65 100644
--- a/lms/templates/portal/course_about.html
+++ b/lms/templates/portal/course_about.html
@@ -1,6 +1,7 @@
-<%!
- from django.core.urlresolvers import reverse
+<%!
+ from django.core.urlresolvers import reverse
from courseware.courses import course_image_url, get_course_about_section
+ from courseware.access import has_access
%>
<%namespace name='static' file='../static_content.html'/>
@@ -15,7 +16,7 @@
$(".register").click(function() {
$("#class_enroll_form").submit();
});
-
+
$(document).delegate('#class_enroll_form', 'ajax:success', function(data, json, xhr) {
if(json.success) {
location.href="${reverse('dashboard')}";
@@ -64,7 +65,7 @@
%if registered:
<%
## TODO: move this logic into a view
- if course.has_started() or settings.MITX_FEATURES['DISABLE_START_DATES']:
+ if has_access(user, course, 'load'):
course_target = reverse('info', args=[course.id])
else:
course_target = reverse('about_course', args=[course.id])
--
GitLab