diff --git a/lms/djangoapps/course_api/blocks/forms.py b/lms/djangoapps/course_api/blocks/forms.py
index 17dee8491949565d9afc666e95f2a677a654f16c..96c7946e0b85fc439aecfa8d446713ac93aa42bd 100644
--- a/lms/djangoapps/course_api/blocks/forms.py
+++ b/lms/djangoapps/course_api/blocks/forms.py
@@ -3,8 +3,7 @@ Course API Forms
"""
-import six
-from django.contrib.auth.models import User
+from django.contrib.auth.models import AnonymousUser, User
from django.core.exceptions import ValidationError
from django.forms import CharField, ChoiceField, Form, IntegerField
from django.http import Http404
@@ -49,7 +48,7 @@ class BlockListGetForm(Form):
try:
return int(value)
except ValueError:
- raise ValidationError(u"'{}' is not a valid depth value.".format(value))
+ raise ValidationError("'{}' is not a valid depth value.".format(value))
def clean_requested_fields(self):
"""
@@ -76,7 +75,7 @@ class BlockListGetForm(Form):
try:
usage_key = UsageKey.from_string(usage_key)
except InvalidKeyError:
- raise ValidationError(u"'{}' is not a valid usage key.".format(six.text_type(usage_key)))
+ raise ValidationError("'{}' is not a valid usage key.".format(str(usage_key)))
return usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key))
@@ -106,38 +105,86 @@ class BlockListGetForm(Form):
cleaned_data['user'] = self._clean_requested_user(cleaned_data, usage_key.course_key)
return cleaned_data
+ def clean_username(self):
+ """
+ Return cleaned username.
+
+ Overrides the default behaviour that maps an empty string to None. This
+ allows us to differentiate between no username being provided (None) vs
+ an empty username being provided ('').
+ """
+ # In case all_blocks is specified, ignore the username.
+ if self.cleaned_data.get('all_blocks', False):
+ return None
+
+ # See if 'username' was provided as a parameter in the raw data.
+ # If so, we return the already-cleaned version of that, otherwise we
+ # return None
+ if 'username' in self.data:
+ return self.cleaned_data['username']
+ return None
+
def _clean_requested_user(self, cleaned_data, course_key):
"""
Validates and returns the requested_user, while checking permissions.
"""
requesting_user = self.initial['requesting_user']
requested_username = cleaned_data.get('username', None)
+ all_blocks = cleaned_data.get('all_blocks', False)
+
+ if requested_username is None and not all_blocks:
+ raise ValidationError({'username': ["This field is required unless all_blocks is requested."]})
+
+ if requesting_user.is_anonymous:
+ return self._verify_anonymous_user(requested_username, course_key, all_blocks)
- if not requested_username:
- return self._verify_no_user(requesting_user, cleaned_data, course_key)
+ if all_blocks:
+ return self._verify_all_blocks(requesting_user, course_key)
elif requesting_user.username.lower() == requested_username.lower():
return self._verify_requesting_user(requesting_user, course_key)
else:
return self._verify_other_user(requesting_user, requested_username, course_key)
@staticmethod
- def _verify_no_user(requesting_user, cleaned_data, course_key):
+ def _verify_anonymous_user(username, course_key, all_blocks):
"""
- Verifies form for when no username is specified, including permissions.
+ Verifies form for when the requesting user is anonymous.
"""
- # Verify that access to all blocks is requested
- # (and not unintentionally requested).
- if not cleaned_data.get('all_blocks', None):
- raise ValidationError({'username': ['This field is required unless all_blocks is requested.']})
+ if all_blocks:
+ raise PermissionDenied(
+ "Anonymous users do not have permission to access all blocks in '{course_key}'.".format(
+ course_key=str(course_key),
+ )
+ )
+
+ # Check for '' and explicitly '' since the only valid option for anonymous users is
+ # an empty string that corresponds to an anonymous user.
+ if username != '':
+ raise PermissionDenied("Anonymous users cannot access another user's blocks.")
+
+ if not permissions.is_course_public(course_key):
+ raise PermissionDenied(
+ "Course blocks for '{course_key}' cannot be accessed anonymously.".format(
+ course_key=course_key,
+ )
+ )
+
+ return AnonymousUser()
+ @staticmethod
+ def _verify_all_blocks(requesting_user, course_key): # pylint: disable=useless-return
+ """
+ Verifies form for when no username is specified, including permissions.
+ """
# Verify all blocks can be accessed for the course.
if not permissions.can_access_all_blocks(requesting_user, course_key):
raise PermissionDenied(
- u"'{requesting_username}' does not have permission to access all blocks in '{course_key}'."
- .format(requesting_username=requesting_user.username, course_key=six.text_type(course_key))
+ "'{requesting_username}' does not have permission to access all blocks in '{course_key}'.".format(
+ requesting_username=requesting_user.username,
+ course_key=str(course_key),
+ )
)
- # return None for user
return None
@staticmethod
@@ -147,8 +194,9 @@ class BlockListGetForm(Form):
"""
if not permissions.can_access_self_blocks(requesting_user, course_key):
raise PermissionDenied(
- u"Course blocks for '{requesting_username}' cannot be accessed."
- .format(requesting_username=requesting_user.username)
+ "Course blocks for '{requesting_username}' cannot be accessed.".format(
+ requesting_username=requesting_user.username,
+ )
)
return requesting_user
@@ -158,11 +206,18 @@ class BlockListGetForm(Form):
Verifies whether the requesting user can access another user's view of
the blocks in the course.
"""
+ # If accessing a public course, and requesting only content available publicly,
+ # we can allow the request.
+ if requested_username == '' and permissions.is_course_public(course_key):
+ return AnonymousUser()
+
# Verify requesting user can access the user's blocks.
if not permissions.can_access_others_blocks(requesting_user, course_key):
raise PermissionDenied(
- u"'{requesting_username}' does not have permission to access view for '{requested_username}'."
- .format(requesting_username=requesting_user.username, requested_username=requested_username)
+ "'{requesting_username}' does not have permission to access view for '{requested_username}'.".format(
+ requesting_username=requesting_user.username,
+ requested_username=requested_username,
+ )
)
# Verify user exists.
@@ -170,5 +225,7 @@ class BlockListGetForm(Form):
return User.objects.get(username=requested_username)
except User.DoesNotExist:
raise Http404(
- u"Requested user '{requested_username}' does not exist.".format(requested_username=requested_username)
+ "Requested user '{requested_username}' does not exist.".format(
+ requested_username=requested_username,
+ )
)
diff --git a/lms/djangoapps/course_api/blocks/permissions.py b/lms/djangoapps/course_api/blocks/permissions.py
index e2acbea2bc097c96da9eb6fcad24e8958d849879..3d0e2c467037feeff0ec232b88209846a04d5f32 100644
--- a/lms/djangoapps/course_api/blocks/permissions.py
+++ b/lms/djangoapps/course_api/blocks/permissions.py
@@ -1,11 +1,16 @@
"""
Encapsulates permissions checks for Course Blocks API
"""
-
+from django.contrib.auth.models import User
+from opaque_keys.edx.keys import CourseKey
from lms.djangoapps.courseware.access import has_access
+from lms.djangoapps.courseware.access_response import AccessResponse
+from lms.djangoapps.courseware.access_utils import ACCESS_DENIED, ACCESS_GRANTED, check_public_access
+from lms.djangoapps.courseware.courses import get_course
from student.models import CourseEnrollment
from student.roles import CourseStaffRole
+from xmodule.course_module import COURSE_VISIBILITY_PUBLIC
def can_access_all_blocks(requesting_user, course_key):
@@ -24,11 +29,25 @@ def can_access_others_blocks(requesting_user, course_key):
return has_access(requesting_user, CourseStaffRole.ROLE, course_key)
-def can_access_self_blocks(requesting_user, course_key):
+def can_access_self_blocks(requesting_user: User, course_key: CourseKey) -> AccessResponse:
"""
Returns whether the requesting_user can access own blocks.
"""
- return (
+ user_is_enrolled_or_staff = ( # pylint: disable=consider-using-ternary
(requesting_user.id and CourseEnrollment.is_enrolled(requesting_user, course_key)) or
has_access(requesting_user, CourseStaffRole.ROLE, course_key)
)
+ if user_is_enrolled_or_staff:
+ return ACCESS_GRANTED
+ try:
+ return is_course_public(course_key)
+ except ValueError:
+ return ACCESS_DENIED
+
+
+def is_course_public(course_key: CourseKey) -> AccessResponse:
+ """
+ This checks if a course is publicly accessible or not.
+ """
+ course = get_course(course_key, depth=0)
+ return check_public_access(course, [COURSE_VISIBILITY_PUBLIC])
diff --git a/lms/djangoapps/course_api/blocks/tests/test_views.py b/lms/djangoapps/course_api/blocks/tests/test_views.py
index 811cbc87c90b14e63105a14a27f79694f5751f90..cbb9325f62aad60f1b8f2d2f4f9ae8d644439c7e 100644
--- a/lms/djangoapps/course_api/blocks/tests/test_views.py
+++ b/lms/djangoapps/course_api/blocks/tests/test_views.py
@@ -4,9 +4,11 @@ Tests for Blocks Views
from datetime import datetime
+from unittest import mock
+from unittest.mock import Mock
+from urllib.parse import urlencode, urlunparse
-import six
-from six.moves.urllib.parse import urlencode, urlunparse
+from django.conf import settings
from django.urls import reverse
from opaque_keys.edx.locator import CourseLocator
@@ -38,7 +40,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
cls.course_usage_key = cls.store.make_course_usage_key(cls.course_key)
cls.non_orphaned_block_usage_keys = set(
- six.text_type(item.location)
+ str(item.location)
for item in cls.store.get_items(cls.course_key)
# remove all orphaned items in the course, except for the root 'course' block
if cls.store.get_parent_location(item.location) or item.category == 'course'
@@ -49,17 +51,18 @@ class TestBlocksView(SharedModuleStoreTestCase):
# create and enroll user in the toy course
self.user = UserFactory.create()
+ self.admin_user = AdminFactory.create()
self.client.login(username=self.user.username, password='test')
CourseEnrollmentFactory.create(user=self.user, course_id=self.course_key)
# default values for url and query_params
self.url = reverse(
'blocks_in_block_tree',
- kwargs={'usage_key_string': six.text_type(self.course_usage_key)}
+ kwargs={'usage_key_string': str(self.course_usage_key)}
)
self.query_params = {'depth': 'all', 'username': self.user.username}
- def verify_response(self, expected_status_code=200, params=None, url=None):
+ def verify_response(self, expected_status_code=200, params=None, url=None, cacheable=False):
"""
Ensure that sending a GET request to the specified URL returns the
expected status code.
@@ -77,7 +80,13 @@ class TestBlocksView(SharedModuleStoreTestCase):
if params:
self.query_params.update(params)
response = self.client.get(url or self.url, self.query_params)
- self.assertEqual(response.status_code, expected_status_code)
+ self.assertEqual(response.status_code, expected_status_code, str(response.content))
+ if cacheable:
+ assert response.get('Cache-Control', None) == 'max-age={}'.format(
+ settings.CACHE_MIDDLEWARE_SECONDS
+ )
+ else:
+ assert response.get('Cache-Control', None) is None
return response
def verify_response_block_list(self, response):
@@ -94,7 +103,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
Verify that the response contains the expected blocks
"""
self.assertSetEqual(
- set(six.iterkeys(response.data['blocks'])),
+ set(response.data['blocks'].keys()),
self.non_orphaned_block_usage_keys,
)
@@ -103,7 +112,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
Verify the response has the expected structure
"""
self.verify_response_block_dict(response)
- for block_key_string, block_data in six.iteritems(response.data['blocks']):
+ for block_key_string, block_data in response.data['blocks'].items():
block_key = deserialize_usage_key(block_key_string, self.course_key)
xblock = self.store.get_item(block_key)
@@ -116,7 +125,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
if xblock.has_children:
self.assertSetEqual(
- set(six.text_type(child.location) for child in xblock.get_children()),
+ set(str(child.location) for child in xblock.get_children()),
set(block_data['children']),
)
@@ -148,19 +157,97 @@ class TestBlocksView(SharedModuleStoreTestCase):
else:
self.assertFalse(expression)
- def test_not_authenticated(self):
+ def test_not_authenticated_non_public_course_with_other_username(self):
+ """
+ Verify behaviour when accessing course blocks of a non-public course for another user anonymously.
+ """
+ self.client.logout()
+ self.verify_response(403)
+
+ def test_not_authenticated_non_public_course_with_all_blocks(self):
+ """
+ Verify behaviour when accessing all course blocks of a non-public course anonymously.
+ """
self.client.logout()
- self.verify_response(401)
+ self.query_params.pop('username')
+ self.query_params['all_blocks'] = True
+ self.verify_response(403)
- def test_not_enrolled(self):
+ def test_not_authenticated_non_public_course_with_blank_username(self):
+ """
+ Verify behaviour when accessing course blocks of a non-public course for anonymous user anonymously.
+ """
+ self.client.logout()
+ self.query_params['username'] = ''
+ self.verify_response(403)
+
+ @mock.patch("course_api.blocks.forms.permissions.is_course_public", Mock(return_value=True))
+ def test_not_authenticated_public_course_with_other_username(self):
+ """
+ Verify behaviour when accessing course blocks of a public course for another user anonymously.
+ """
+ self.client.logout()
+ self.verify_response(403)
+
+ @mock.patch("course_api.blocks.forms.permissions.is_course_public", Mock(return_value=True))
+ def test_not_authenticated_public_course_with_all_blocks(self):
+ """
+ Verify behaviour when accessing all course blocks of a public course anonymously.
+ """
+ self.client.logout()
+ self.query_params.pop('username')
+ self.query_params['all_blocks'] = True
+ self.verify_response(403)
+
+ @mock.patch("course_api.blocks.forms.permissions.is_course_public", Mock(return_value=True))
+ def test_not_authenticated_public_course_with_blank_username(self):
+ """
+ Verify behaviour when accessing course blocks of a public course for anonymous user anonymously.
+ """
+ self.client.logout()
+ self.query_params['username'] = ''
+ self.verify_response(cacheable=True)
+
+ def test_not_enrolled_non_public_course(self):
+ """
+ Verify behaviour when accessing course blocks for a non-public course as a user not enrolled in course.
+ """
CourseEnrollment.unenroll(self.user, self.course_key)
self.verify_response(403)
+ @mock.patch("course_api.blocks.forms.permissions.is_course_public", Mock(return_value=True))
+ def test_not_enrolled_public_course(self):
+ """
+ Verify behaviour when accessing course blocks for a public course as a user not enrolled in course.
+ """
+ self.query_params['username'] = ''
+ CourseEnrollment.unenroll(self.user, self.course_key)
+ self.verify_response(cacheable=True)
+
+ @mock.patch("course_api.blocks.forms.permissions.is_course_public", Mock(return_value=True))
+ def test_public_course_all_blocks_and_empty_username(self):
+ """
+ Verify behaviour when specifying both all_blocks and username='', and ensure the response is not cached.
+ """
+ self.query_params['username'] = ''
+ self.query_params['all_blocks'] = True
+ # Verify response for a regular user.
+ self.verify_response(403, cacheable=False)
+ # Verify response for an unenrolled user.
+ CourseEnrollment.unenroll(self.user, self.course_key)
+ self.verify_response(403, cacheable=False)
+ # Verify response for an anonymous user.
+ self.client.logout()
+ self.verify_response(403, cacheable=False)
+ # Verify response for a staff user.
+ self.client.login(username=self.admin_user.username, password='test')
+ self.verify_response(cacheable=False)
+
def test_non_existent_course(self):
usage_key = self.store.make_course_usage_key(CourseLocator('non', 'existent', 'course'))
url = reverse(
'blocks_in_block_tree',
- kwargs={'usage_key_string': six.text_type(usage_key)}
+ kwargs={'usage_key_string': str(usage_key)}
)
self.verify_response(403, url=url)
@@ -174,16 +261,16 @@ class TestBlocksView(SharedModuleStoreTestCase):
self.verify_response(400)
def test_no_user_staff_all_blocks(self):
- self.client.login(username=AdminFactory.create().username, password='test')
+ self.client.login(username=self.admin_user.username, password='test')
self.query_params.pop('username')
self.query_params['all_blocks'] = True
self.verify_response()
def test_basic(self):
response = self.verify_response()
- self.assertEqual(response.data['root'], six.text_type(self.course_usage_key))
+ self.assertEqual(response.data['root'], str(self.course_usage_key))
self.verify_response_block_dict(response)
- for block_key_string, block_data in six.iteritems(response.data['blocks']):
+ for block_key_string, block_data in response.data['blocks'].items():
block_key = deserialize_usage_key(block_key_string, self.course_key)
self.assertEqual(block_data['id'], block_key_string)
self.assertEqual(block_data['type'], block_key.block_type)
@@ -196,7 +283,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
def test_block_counts_param(self):
response = self.verify_response(params={'block_counts': ['course', 'chapter']})
self.verify_response_block_dict(response)
- for block_data in six.itervalues(response.data['blocks']):
+ for block_data in response.data['blocks'].values():
self.assertEqual(
block_data['block_counts']['course'],
1 if block_data['type'] == 'course' else 0,
@@ -215,7 +302,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
'student_view_data': self.BLOCK_TYPES_WITH_STUDENT_VIEW_DATA + ['chapter']
})
self.verify_response_block_dict(response)
- for block_data in six.itervalues(response.data['blocks']):
+ for block_data in response.data['blocks'].values():
self.assert_in_iff(
'student_view_data',
block_data,
@@ -233,12 +320,13 @@ class TestBlocksView(SharedModuleStoreTestCase):
- other_course_settings
- course_visibility
"""
+ self.client.login(username=self.admin_user.username, password='test')
response = self.verify_response(params={
'all_blocks': True,
'requested_fields': ['other_course_settings', 'course_visibility'],
})
self.verify_response_block_dict(response)
- for block_data in six.itervalues(response.data['blocks']):
+ for block_data in response.data['blocks'].values():
self.assert_in_iff(
'other_course_settings',
block_data,
@@ -264,12 +352,13 @@ class TestBlocksView(SharedModuleStoreTestCase):
- other_course_settings
- course_visibility
"""
+ self.client.login(username=self.admin_user.username, password='test')
response = self.verify_response(params={
'all_blocks': True,
'requested_fields': ['course_visibility'],
})
self.verify_response_block_dict(response)
- for block_data in six.itervalues(response.data['blocks']):
+ for block_data in response.data['blocks'].values():
self.assertNotIn(
'other_course_settings',
block_data
@@ -284,7 +373,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
def test_navigation_param(self):
response = self.verify_response(params={'nav_depth': 10})
self.verify_response_block_dict(response)
- for block_data in six.itervalues(response.data['blocks']):
+ for block_data in response.data['blocks'].values():
self.assertIn('descendants', block_data)
def test_requested_fields_param(self):
@@ -314,7 +403,7 @@ class TestBlocksInCourseView(TestBlocksView):
def setUp(self):
super(TestBlocksInCourseView, self).setUp()
self.url = reverse('blocks_in_course')
- self.query_params['course_id'] = six.text_type(self.course_key)
+ self.query_params['course_id'] = str(self.course_key)
def test_no_course_id(self):
self.query_params.pop('course_id')
@@ -324,4 +413,4 @@ class TestBlocksInCourseView(TestBlocksView):
self.verify_response(400, params={'course_id': 'invalid_course_id'})
def test_non_existent_course(self):
- self.verify_response(403, params={'course_id': six.text_type(CourseLocator('non', 'existent', 'course'))})
+ self.verify_response(403, params={'course_id': str(CourseLocator('non', 'existent', 'course'))})
diff --git a/lms/djangoapps/course_api/blocks/views.py b/lms/djangoapps/course_api/blocks/views.py
index b9f1b9eb2e836bcc1ad97ff40a6f3133fe14c731..6f2421fcde94fb0b41f1c5ba393dc752df5e70da 100644
--- a/lms/djangoapps/course_api/blocks/views.py
+++ b/lms/djangoapps/course_api/blocks/views.py
@@ -7,6 +7,7 @@ import six
from django.core.exceptions import ValidationError
from django.db import transaction
from django.http import Http404
+from django.utils.cache import patch_response_headers
from django.utils.decorators import method_decorator
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
@@ -22,8 +23,8 @@ from .api import get_blocks
from .forms import BlockListGetForm
-@view_auth_classes()
@method_decorator(transaction.non_atomic_requests, name='dispatch')
+@view_auth_classes(is_authenticated=False)
class BlocksView(DeveloperErrorViewMixin, ListAPIView):
"""
**Use Case**
@@ -53,12 +54,16 @@ class BlocksView(DeveloperErrorViewMixin, ListAPIView):
* username: (string) Required, unless ``all_blocks`` is specified.
Specify the username for the user whose course blocks are requested.
- Only users with course staff permissions can specify other users'
- usernames. If a username is specified, results include blocks that
- are visible to that user, including those based on group or cohort
- membership or randomized content assigned to that user.
+ A blank/empty username can be used to request the blocks accessible
+ to anonymous users (for public courses). Only users with course staff
+ permissions can specify other users' usernames. If a username is
+ specified, results include blocks that are visible to that user,
+ including those based on group or cohort membership or randomized
+ content assigned to that user.
Example: username=anjali
+ username=''
+ username
* student_view_data: (list) Indicates for which block types to return
student_view_data.
@@ -211,7 +216,7 @@ class BlocksView(DeveloperErrorViewMixin, ListAPIView):
raise ValidationError(params.errors)
try:
- return Response(
+ response = Response(
get_blocks(
request,
params.cleaned_data['usage_key'],
@@ -226,11 +231,17 @@ class BlocksView(DeveloperErrorViewMixin, ListAPIView):
hide_access_denials=hide_access_denials,
)
)
+ # If the username is an empty string, and not None, then we are requesting
+ # data about the anonymous view of a course, which can be cached. In this
+ # case we add the usual caching headers to the response.
+ if params.cleaned_data.get('username', None) == '':
+ patch_response_headers(response)
+ return response
except ItemNotFoundError as exception:
raise Http404(u"Block not found: {}".format(text_type(exception)))
-@view_auth_classes()
+@view_auth_classes(is_authenticated=False)
class BlocksInCourseView(BlocksView):
"""
**Use Case**