From bf633f9c5d298ce8435999c2f9d96c8a216ff12a Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Fri, 20 Sep 2013 10:26:15 -0400 Subject: [PATCH 1/6] Perform auth checks in middleware Includes static server tests. --- common/djangoapps/contentserver/middleware.py | 14 ++- .../contentserver/tests/__init__.py | 0 common/djangoapps/contentserver/tests/test.py | 118 ++++++++++++++++++ lms/envs/test.py | 7 ++ 4 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 common/djangoapps/contentserver/tests/__init__.py create mode 100644 common/djangoapps/contentserver/tests/test.py diff --git a/common/djangoapps/contentserver/middleware.py b/common/djangoapps/contentserver/middleware.py index d89e3fdd23..0174c39fea 100644 --- a/common/djangoapps/contentserver/middleware.py +++ b/common/djangoapps/contentserver/middleware.py @@ -1,4 +1,6 @@ from django.http import HttpResponse, HttpResponseNotModified +from django.shortcuts import redirect +from student.models import CourseEnrollment from xmodule.contentstore.django import contentstore from xmodule.contentstore.content import StaticContent, XASSET_LOCATION_TAG @@ -20,7 +22,8 @@ class StaticContentServer(object): return response # first look in our cache so we don't have to round-trip to the DB - content = get_cached_content(loc) + #content = get_cached_content(loc) + content = None if content is None: # nope, not in cache, let's fetch from DB try: @@ -41,6 +44,15 @@ class StaticContentServer(object): # NOP here, but we may wish to add a "cache-hit" counter in the future pass + # Check that user has access to content + if getattr(content, "locked", False): + if not hasattr(request, "user") or not request.user.is_authenticated(): + return redirect('root') + course_id = "/".join([loc.org, loc.course, loc.name]) + if not CourseEnrollment.is_enrolled(request.user, course_id): + return redirect('dashboard') + + # see if the last-modified at hasn't changed, if not return a 302 (Not Modified) # convert over the DB persistent last modified timestamp to a HTTP compatible diff --git a/common/djangoapps/contentserver/tests/__init__.py b/common/djangoapps/contentserver/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/contentserver/tests/test.py b/common/djangoapps/contentserver/tests/test.py new file mode 100644 index 0000000000..d084494be3 --- /dev/null +++ b/common/djangoapps/contentserver/tests/test.py @@ -0,0 +1,118 @@ +""" +Tests for StaticContentServer +""" +import copy +import logging +from uuid import uuid4 +from path import path +from pymongo import MongoClient + +from django.contrib.auth.models import User +from django.conf import settings +from django.core.urlresolvers import reverse +from django.test.client import Client +from django.test.utils import override_settings + +from student.models import CourseEnrollment + +from xmodule.contentstore.django import contentstore, _CONTENTSTORE +from xmodule.modulestore import Location +from xmodule.contentstore.content import StaticContent +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import (studio_store_config, + ModuleStoreTestCase) +from xmodule.modulestore.xml_importer import import_from_xml + +log = logging.getLogger(__name__) + +TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) +TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex + +TEST_MODULESTORE = studio_store_config(settings.TEST_ROOT / "data") + + +@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE) +class ContentStoreToyCourseTest(ModuleStoreTestCase): + """ + Tests that use the toy course. + """ + + def setUp(self): + """ + Create user and login. + """ + + settings.MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') + settings.MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') + + self.client = Client() + + loc = Location('c4x', 'edX', 'toy', 'asset', 'sample_static.txt' ) + self.loc = loc + + rel_url = StaticContent.get_url_path_from_location(loc) + base = "http://127.0.0.1:8000" + + self.contentstore = contentstore() + import_from_xml(modulestore('direct'), 'common/test/data/', ['toy'], + static_content_store=self.contentstore, verbose=True) + self.url = base + rel_url + + def tearDown(self): + + MongoClient().drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) + _CONTENTSTORE.clear() + + def test_aunlocked_asset(self): + """ + Test that unlocked assets are being served. + """ + # Unlock the asset + self.contentstore.set_attr(self.loc, 'locked', False) + resp = self.client.get(self.url) + self.assertEqual(resp.status_code, 200) + + + def test_locked_asset(self): + """ + Test that locked assets behave appropriately in case: + (1) User is not logged in + (2) User is logged in in but not registerd for the course + (3) User is logged in and registered + """ + + # Lock the asset + self.contentstore.set_attr(self.loc, 'locked', True) + + + # Case (1) + resp = self.client.get(self.url) + self.assertEqual(resp.status_code, 302) + self.assertTrue(resp.has_header("LOCATION")) + + # Case (2) + # Create user and login + uname = 'testuser' + email = 'test+courses@edx.org' + password = 'foo' + user = User.objects.create_user(uname, email, password) + user.is_active = True + user.save() + self.client.login(username=uname, password=password) + log.debug("User logged in") + + resp = self.client.get(self.url) + log.debug("Received response %s", resp) + self.assertEqual(resp.status_code, 302) + self.assertTrue(resp.has_header("LOCATION")) + self.assertIn("dashboard", resp["LOCATION"]) + + # Case (3) + # Enroll student + course_id = "/".join([self.loc.org, self.loc.course, self.loc.name]) + self.assertTrue(CourseEnrollment.enroll(user, course_id)) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + + resp = self.client.get(self.url) + self.assertEqual(resp.status_code, 200) + diff --git a/lms/envs/test.py b/lms/envs/test.py index 94feffdf3e..45e934ec1f 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -97,6 +97,13 @@ MODULESTORE = { } } +CONTENTSTORE = { + 'ENGINE': 'xmodule.contentstore.mongo.MongoContentStore', + 'OPTIONS': { + 'host': 'localhost', + 'db': 'xcontent', + } +} DATABASES = { 'default': { From 7a8d463260d96900c1c11e8e428a3b960077c9ce Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Fri, 20 Sep 2013 17:22:51 -0400 Subject: [PATCH 2/6] Put static server middleware after the middleware it requires --- lms/envs/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index 81b01179d7..466ec262f9 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -542,7 +542,6 @@ TEMPLATE_LOADERS = ( ) MIDDLEWARE_CLASSES = ( - 'contentserver.middleware.StaticContentServer', 'request_cache.middleware.RequestCache', 'django_comment_client.middleware.AjaxExceptionMiddleware', 'django.middleware.common.CommonMiddleware', @@ -551,6 +550,7 @@ MIDDLEWARE_CLASSES = ( # Instead of AuthenticationMiddleware, we use a cached backed version #'django.contrib.auth.middleware.AuthenticationMiddleware', 'cache_toolbox.middleware.CacheBackedAuthenticationMiddleware', + 'contentserver.middleware.StaticContentServer', 'django.contrib.messages.middleware.MessageMiddleware', 'track.middleware.TrackMiddleware', From 268b5b47aeca7315650c42dafcff086bf03904eb Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Fri, 20 Sep 2013 17:25:43 -0400 Subject: [PATCH 3/6] Remove cache workaround --- common/djangoapps/contentserver/middleware.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/contentserver/middleware.py b/common/djangoapps/contentserver/middleware.py index 0174c39fea..8d81929693 100644 --- a/common/djangoapps/contentserver/middleware.py +++ b/common/djangoapps/contentserver/middleware.py @@ -22,8 +22,7 @@ class StaticContentServer(object): return response # first look in our cache so we don't have to round-trip to the DB - #content = get_cached_content(loc) - content = None + content = get_cached_content(loc) if content is None: # nope, not in cache, let's fetch from DB try: @@ -47,7 +46,7 @@ class StaticContentServer(object): # Check that user has access to content if getattr(content, "locked", False): if not hasattr(request, "user") or not request.user.is_authenticated(): - return redirect('root') + return redirect('login') course_id = "/".join([loc.org, loc.course, loc.name]) if not CourseEnrollment.is_enrolled(request.user, course_id): return redirect('dashboard') From b4e6a8b209e45b641a112e569684f57049c6131a Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Mon, 23 Sep 2013 10:52:05 -0400 Subject: [PATCH 4/6] Fix order-dependency in tests --- common/djangoapps/contentserver/tests/test.py | 41 +++++++++++-------- .../test/data/toy/static/another_static.txt | 17 ++++++++ 2 files changed, 41 insertions(+), 17 deletions(-) create mode 100644 common/test/data/toy/static/another_static.txt diff --git a/common/djangoapps/contentserver/tests/test.py b/common/djangoapps/contentserver/tests/test.py index d084494be3..340092b4d3 100644 --- a/common/djangoapps/contentserver/tests/test.py +++ b/common/djangoapps/contentserver/tests/test.py @@ -45,31 +45,42 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): settings.MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') settings.MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') - self.client = Client() - - loc = Location('c4x', 'edX', 'toy', 'asset', 'sample_static.txt' ) - self.loc = loc - - rel_url = StaticContent.get_url_path_from_location(loc) base = "http://127.0.0.1:8000" - + self.client = Client() self.contentstore = contentstore() + + # A locked the asset + loc = Location('c4x', 'edX', 'toy', 'asset', 'sample_static.txt' ) + self.loc = loc + rel_url = StaticContent.get_url_path_from_location(loc) + self.url = base + rel_url + + # An unlocked asset + loc2 = Location('c4x', 'edX', 'toy', 'asset', 'another_static.txt' ) + self.loc2 = loc2 + rel_url2 = StaticContent.get_url_path_from_location(loc2) + self.url2 = base + rel_url2 + + import_from_xml(modulestore('direct'), 'common/test/data/', ['toy'], static_content_store=self.contentstore, verbose=True) - self.url = base + rel_url + + self.contentstore.set_attr(self.loc, 'locked', True) + def tearDown(self): MongoClient().drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) _CONTENTSTORE.clear() - def test_aunlocked_asset(self): + def test_unlocked_asset(self): """ Test that unlocked assets are being served. """ - # Unlock the asset - self.contentstore.set_attr(self.loc, 'locked', False) - resp = self.client.get(self.url) + # Logout user + self.client.logout() + + resp = self.client.get(self.url2) self.assertEqual(resp.status_code, 200) @@ -81,10 +92,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): (3) User is logged in and registered """ - # Lock the asset - self.contentstore.set_attr(self.loc, 'locked', True) - - # Case (1) resp = self.client.get(self.url) self.assertEqual(resp.status_code, 302) @@ -110,7 +117,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # Case (3) # Enroll student course_id = "/".join([self.loc.org, self.loc.course, self.loc.name]) - self.assertTrue(CourseEnrollment.enroll(user, course_id)) + CourseEnrollment.enroll(user, course_id) self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) resp = self.client.get(self.url) diff --git a/common/test/data/toy/static/another_static.txt b/common/test/data/toy/static/another_static.txt new file mode 100644 index 0000000000..83e560736d --- /dev/null +++ b/common/test/data/toy/static/another_static.txt @@ -0,0 +1,17 @@ + + _ + | | + _____ ____ _ _ __ ___ _ __ | | ___ + / _ \ \/ / _` | '_ ` _ \| '_ \| |/ _ \ +| __/> < (_| | | | | | | |_) | | __/ + \___/_/\_\__,_|_| |_| |_| .__/|_|\___| + | | + |_| + _ _ _ + | | | | (_) + ___| |_ __ _| |_ _ ___ + / __| __/ _` | __| |/ __| + \__ \ || (_| | |_| | (__ + |___/\__\__,_|\__|_|\___| + + From e5c90d33fc551cc6871a13c5c5d5b34b9530ae58 Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Mon, 23 Sep 2013 14:21:24 -0400 Subject: [PATCH 5/6] Fix middleware order in CMS. And include Don's fix for partial course_id lookup. --- cms/envs/common.py | 2 +- common/djangoapps/contentserver/middleware.py | 8 +++--- common/djangoapps/contentserver/tests/test.py | 9 +++---- common/djangoapps/student/models.py | 25 +++++++++++++++++++ 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 38913f537a..c79cfe7aa7 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -140,7 +140,6 @@ TEMPLATE_LOADERS = ( ) MIDDLEWARE_CLASSES = ( - 'contentserver.middleware.StaticContentServer', 'request_cache.middleware.RequestCache', 'django.middleware.cache.UpdateCacheMiddleware', 'django.middleware.common.CommonMiddleware', @@ -150,6 +149,7 @@ MIDDLEWARE_CLASSES = ( # Instead of AuthenticationMiddleware, we use a cache-backed version 'cache_toolbox.middleware.CacheBackedAuthenticationMiddleware', + 'contentserver.middleware.StaticContentServer', 'django.contrib.messages.middleware.MessageMiddleware', 'track.middleware.TrackMiddleware', diff --git a/common/djangoapps/contentserver/middleware.py b/common/djangoapps/contentserver/middleware.py index 8d81929693..30ab977b96 100644 --- a/common/djangoapps/contentserver/middleware.py +++ b/common/djangoapps/contentserver/middleware.py @@ -46,10 +46,10 @@ class StaticContentServer(object): # Check that user has access to content if getattr(content, "locked", False): if not hasattr(request, "user") or not request.user.is_authenticated(): - return redirect('login') - course_id = "/".join([loc.org, loc.course, loc.name]) - if not CourseEnrollment.is_enrolled(request.user, course_id): - return redirect('dashboard') + return HttpResponse('Unauthorized', status=403) + course_partial_id = "/".join([loc.org, loc.course]) + if not CourseEnrollment.is_enrolled_by_partial(request.user, course_partial_id): + return HttpResponse('Unauthorized', status=403) # see if the last-modified at hasn't changed, if not return a 302 (Not Modified) diff --git a/common/djangoapps/contentserver/tests/test.py b/common/djangoapps/contentserver/tests/test.py index 340092b4d3..7313ba3f9b 100644 --- a/common/djangoapps/contentserver/tests/test.py +++ b/common/djangoapps/contentserver/tests/test.py @@ -94,8 +94,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # Case (1) resp = self.client.get(self.url) - self.assertEqual(resp.status_code, 302) - self.assertTrue(resp.has_header("LOCATION")) + self.assertEqual(resp.status_code, 403) # Case (2) # Create user and login @@ -110,13 +109,11 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): resp = self.client.get(self.url) log.debug("Received response %s", resp) - self.assertEqual(resp.status_code, 302) - self.assertTrue(resp.has_header("LOCATION")) - self.assertIn("dashboard", resp["LOCATION"]) + self.assertEqual(resp.status_code, 403) # Case (3) # Enroll student - course_id = "/".join([self.loc.org, self.loc.course, self.loc.name]) + course_id = "/".join([self.loc.org, self.loc.course, '2012_Fall']) CourseEnrollment.enroll(user, course_id) self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 71cff4183b..8cda698d8b 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -843,6 +843,31 @@ class CourseEnrollment(models.Model): except cls.DoesNotExist: return False + @classmethod + def is_enrolled_by_partial(cls, user, course_id_partial): + """ + Returns `True` if the user is enrolled in a course that starts with + `course_id_partial`. Otherwise, returns False. + + Can be used to determine whether a student is enrolled in a course + whose run name is unknown. + + `user` is a Django User object. If it hasn't been saved yet (no `.id` + attribute), this method will automatically save it before + adding an enrollment for it. + + `course_id_partial` is a starting substring for a fully qualified + course_id (e.g. "edX/Test101/"). + """ + try: + return CourseEnrollment.objects.filter( + user=user, + course_id__startswith=course_id_partial, + is_active=1 + ).exists() + except cls.DoesNotExist: + return False + @classmethod def enrollment_mode_for_user(cls, user, course_id): """ From 1a0b752a812b04b85be38bdd9feeaab4c61732c5 Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Tue, 24 Sep 2013 11:17:11 -0400 Subject: [PATCH 6/6] Review fixes --- common/djangoapps/contentserver/middleware.py | 14 +-- common/djangoapps/contentserver/tests/test.py | 108 ++++++++++-------- common/djangoapps/student/models.py | 8 +- common/djangoapps/student/tests/tests.py | 11 ++ 4 files changed, 82 insertions(+), 59 deletions(-) diff --git a/common/djangoapps/contentserver/middleware.py b/common/djangoapps/contentserver/middleware.py index 30ab977b96..b9c14cd537 100644 --- a/common/djangoapps/contentserver/middleware.py +++ b/common/djangoapps/contentserver/middleware.py @@ -1,5 +1,5 @@ -from django.http import HttpResponse, HttpResponseNotModified -from django.shortcuts import redirect +from django.http import (HttpResponse, HttpResponseNotModified, + HttpResponseForbidden) from student.models import CourseEnrollment from xmodule.contentstore.django import contentstore @@ -46,13 +46,11 @@ class StaticContentServer(object): # Check that user has access to content if getattr(content, "locked", False): if not hasattr(request, "user") or not request.user.is_authenticated(): - return HttpResponse('Unauthorized', status=403) + return HttpResponseForbidden('Unauthorized') course_partial_id = "/".join([loc.org, loc.course]) - if not CourseEnrollment.is_enrolled_by_partial(request.user, course_partial_id): - return HttpResponse('Unauthorized', status=403) - - - # see if the last-modified at hasn't changed, if not return a 302 (Not Modified) + if not request.user.is_staff and not CourseEnrollment.is_enrolled_by_partial( + request.user, course_partial_id): + return HttpResponseForbidden('Unauthorized') # convert over the DB persistent last modified timestamp to a HTTP compatible # timestamp, so we can simply compare the strings diff --git a/common/djangoapps/contentserver/tests/test.py b/common/djangoapps/contentserver/tests/test.py index 7313ba3f9b..94a614c85a 100644 --- a/common/djangoapps/contentserver/tests/test.py +++ b/common/djangoapps/contentserver/tests/test.py @@ -9,7 +9,6 @@ from pymongo import MongoClient from django.contrib.auth.models import User from django.conf import settings -from django.core.urlresolvers import reverse from django.test.client import Client from django.test.utils import override_settings @@ -20,7 +19,7 @@ from xmodule.modulestore import Location from xmodule.contentstore.content import StaticContent from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import (studio_store_config, - ModuleStoreTestCase) + ModuleStoreTestCase) from xmodule.modulestore.xml_importer import import_from_xml log = logging.getLogger(__name__) @@ -45,28 +44,39 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): settings.MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') settings.MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') - base = "http://127.0.0.1:8000" self.client = Client() self.contentstore = contentstore() - # A locked the asset - loc = Location('c4x', 'edX', 'toy', 'asset', 'sample_static.txt' ) - self.loc = loc - rel_url = StaticContent.get_url_path_from_location(loc) - self.url = base + rel_url + # A locked asset + self.loc_locked = Location('c4x', 'edX', 'toy', 'asset', 'sample_static.txt') + self.url_locked = StaticContent.get_url_path_from_location(self.loc_locked) # An unlocked asset - loc2 = Location('c4x', 'edX', 'toy', 'asset', 'another_static.txt' ) - self.loc2 = loc2 - rel_url2 = StaticContent.get_url_path_from_location(loc2) - self.url2 = base + rel_url2 - + self.loc_unlocked = Location('c4x', 'edX', 'toy', 'asset', 'another_static.txt') + self.url_unlocked = StaticContent.get_url_path_from_location(self.loc_unlocked) import_from_xml(modulestore('direct'), 'common/test/data/', ['toy'], static_content_store=self.contentstore, verbose=True) - self.contentstore.set_attr(self.loc, 'locked', True) + self.contentstore.set_attr(self.loc_locked, 'locked', True) + # Create user + self.usr = 'testuser' + self.pwd = 'foo' + email = 'test+courses@edx.org' + self.user = User.objects.create_user(self.usr, email, self.pwd) + self.user.is_active = True + self.user.save() + + # Create staff user + self.staff_usr = 'stafftestuser' + self.staff_pwd = 'foo' + staff_email = 'stafftest+courses@edx.org' + self.staff_user = User.objects.create_user(self.staff_usr, staff_email, + self.staff_pwd) + self.staff_user.is_active = True + self.staff_user.is_staff = True + self.staff_user.save() def tearDown(self): @@ -77,46 +87,50 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): """ Test that unlocked assets are being served. """ - # Logout user self.client.logout() + resp = self.client.get(self.url_unlocked) + self.assertEqual(resp.status_code, 200) #pylint: disable=E1103 - resp = self.client.get(self.url2) - self.assertEqual(resp.status_code, 200) - - - def test_locked_asset(self): + def test_locked_asset_not_logged_in(self): """ - Test that locked assets behave appropriately in case: - (1) User is not logged in - (2) User is logged in in but not registerd for the course - (3) User is logged in and registered + Test that locked assets behave appropriately in case the user is not + logged in. """ + self.client.logout() + resp = self.client.get(self.url_locked) + self.assertEqual(resp.status_code, 403) #pylint: disable=E1103 - # Case (1) - resp = self.client.get(self.url) - self.assertEqual(resp.status_code, 403) + def test_locked_asset_not_registered(self): + """ + Test that locked assets behave appropriately in case user is logged in + in but not registered for the course. + """ + self.client.login(username=self.usr, password=self.pwd) + resp = self.client.get(self.url_locked) + self.assertEqual(resp.status_code, 403) #pylint: disable=E1103 - # Case (2) - # Create user and login - uname = 'testuser' - email = 'test+courses@edx.org' - password = 'foo' - user = User.objects.create_user(uname, email, password) - user.is_active = True - user.save() - self.client.login(username=uname, password=password) - log.debug("User logged in") + def test_locked_asset_registered(self): + """ + Test that locked assets behave appropriately in case user is logged in + and registered for the course. + """ + #pylint: disable=E1101 + course_id = "/".join([self.loc_locked.org, self.loc_locked.course, '2012_Fall']) + CourseEnrollment.enroll(self.user, course_id) + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) - resp = self.client.get(self.url) - log.debug("Received response %s", resp) - self.assertEqual(resp.status_code, 403) + self.client.login(username=self.usr, password=self.pwd) + resp = self.client.get(self.url_locked) + self.assertEqual(resp.status_code, 200) #pylint: disable=E1103 - # Case (3) - # Enroll student - course_id = "/".join([self.loc.org, self.loc.course, '2012_Fall']) - CourseEnrollment.enroll(user, course_id) - self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + def test_locked_asset_staff(self): + """ + Test that locked assets behave appropriately in case user is staff. + """ + #pylint: disable=E1101 + course_id = "/".join([self.loc_locked.org, self.loc_locked.course, '2012_Fall']) - resp = self.client.get(self.url) - self.assertEqual(resp.status_code, 200) + self.client.login(username=self.staff_usr, password=self.staff_pwd) + resp = self.client.get(self.url_locked) + self.assertEqual(resp.status_code, 200) #pylint: disable=E1103 diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 8cda698d8b..da16a2fda2 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -861,10 +861,10 @@ class CourseEnrollment(models.Model): """ try: return CourseEnrollment.objects.filter( - user=user, - course_id__startswith=course_id_partial, - is_active=1 - ).exists() + user=user, + course_id__startswith=course_id_partial, + is_active=1 + ).exists() except cls.DoesNotExist: return False diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 397816ec00..a6466ee9f9 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -213,23 +213,34 @@ class EnrollInCourseTest(TestCase): def test_enrollment(self): user = User.objects.create_user("joe", "joe@joe.com", "password") course_id = "edX/Test101/2013" + course_id_partial = "edX/Test101" # Test basic enrollment self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user, + course_id_partial)) CourseEnrollment.enroll(user, course_id) self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + self.assertTrue(CourseEnrollment.is_enrolled_by_partial(user, + course_id_partial)) # Enrolling them again should be harmless CourseEnrollment.enroll(user, course_id) self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + self.assertTrue(CourseEnrollment.is_enrolled_by_partial(user, + course_id_partial)) # Now unenroll the user CourseEnrollment.unenroll(user, course_id) self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user, + course_id_partial)) # Unenrolling them again should also be harmless CourseEnrollment.unenroll(user, course_id) self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user, + course_id_partial)) # The enrollment record should still exist, just be inactive enrollment_record = CourseEnrollment.objects.get(