diff --git a/openedx/core/djangoapps/content_libraries/admin.py b/openedx/core/djangoapps/content_libraries/admin.py index d48782809a..165a88a330 100644 --- a/openedx/core/djangoapps/content_libraries/admin.py +++ b/openedx/core/djangoapps/content_libraries/admin.py @@ -10,7 +10,7 @@ class ContentLibraryPermissionInline(admin.TabularInline): Inline form for a content library's permissions """ model = ContentLibraryPermission - raw_id_fields = ("user", ) + raw_id_fields = ("user", "group", ) extra = 0 diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 238dafe79d..9dc6570894 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -4,11 +4,12 @@ Python API for content libraries. Unless otherwise specified, all APIs in this file deal with the DRAFT version of the content library. """ - from uuid import UUID import logging import attr +from django.contrib.auth.models import AbstractUser, Group +from django.core.exceptions import PermissionDenied from django.core.validators import validate_unicode_slug from django.db import IntegrityError from lxml import etree @@ -18,7 +19,9 @@ import six from xblock.core import XBlock from xblock.exceptions import XBlockNotFoundError +from openedx.core.djangoapps.content_libraries import permissions from openedx.core.djangoapps.content_libraries.library_bundle import LibraryBundle +from openedx.core.djangoapps.content_libraries.models import ContentLibrary, ContentLibraryPermission from openedx.core.djangoapps.xblock.api import get_block_display_name, load_block from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl from openedx.core.djangoapps.xblock.runtime.olx_parsing import XBlockInclude @@ -36,7 +39,6 @@ from openedx.core.lib.blockstore_api import ( ) from openedx.core.djangolib import blockstore_cache from openedx.core.djangolib.blockstore_cache import BundleCache -from .models import ContentLibrary, ContentLibraryPermission log = logging.getLogger(__name__) @@ -80,6 +82,30 @@ class ContentLibraryMetadata(object): # has_unpublished_deletes will be true when the draft version of the library's bundle # contains deletes of any XBlocks that were in the most recently published version has_unpublished_deletes = attr.ib(False) + # Allow any user (even unregistered users) to view and interact directly + # with this library's content in the LMS + allow_public_learning = attr.ib(False) + # Allow any user with Studio access to view this library's content in + # Studio, use it in their courses, and copy content out of this library. + allow_public_read = attr.ib(False) + + +class AccessLevel(object): + """ Enum defining library access levels/permissions """ + ADMIN_LEVEL = ContentLibraryPermission.ADMIN_LEVEL + AUTHOR_LEVEL = ContentLibraryPermission.AUTHOR_LEVEL + READ_LEVEL = ContentLibraryPermission.READ_LEVEL + NO_ACCESS = None + + +@attr.s +class ContentLibraryPermissionEntry(object): + """ + A user or group granted permission to use a content library. + """ + user = attr.ib(type=AbstractUser, default=None) + group = attr.ib(type=Group, default=None) + access_level = attr.ib(AccessLevel.NO_ACCESS) @attr.s @@ -117,23 +143,32 @@ class LibraryXBlockType(object): display_name = attr.ib("") -class AccessLevel(object): - """ Enum defining library access levels/permissions """ - ADMIN_LEVEL = ContentLibraryPermission.ADMIN_LEVEL - AUTHOR_LEVEL = ContentLibraryPermission.AUTHOR_LEVEL - READ_LEVEL = ContentLibraryPermission.READ_LEVEL - NO_ACCESS = None +def list_libraries_for_user(user): + """ + Lists up to 50 content libraries that the user has permission to view. + + This method makes at least one HTTP call per library so should only be used + for development until we have something more efficient. + """ + qs = ContentLibrary.objects.all() + filtered_qs = permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs) + return [get_library(ref.library_key) for ref in filtered_qs[:50]] -def list_libraries(): +def require_permission_for_library_key(library_key, user, permission): """ - TEMPORARY method for testing. Lists all content libraries. - This should be replaced with a method for listing all libraries that belong - to a particular user, and/or has permission to view. This method makes at - least one HTTP call per library so should only be used for development. + Given any of the content library permission strings defined in + openedx.core.djangoapps.content_libraries.permissions, + check if the given user has that permission for the library with the + specified library ID. + + Raises django.core.exceptions.PermissionDenied if the user doesn't have + permission. """ - refs = ContentLibrary.objects.all()[:1000] - return [get_library(ref.library_key) for ref in refs] + assert isinstance(library_key, LibraryLocatorV2) + library_obj = ContentLibrary.objects.get_by_key(library_key) + if not user.has_perm(permission, obj=library_obj): + raise PermissionDenied def get_library(library_key): @@ -154,12 +189,14 @@ def get_library(library_key): title=bundle_metadata.title, description=bundle_metadata.description, version=bundle_metadata.latest_version, + allow_public_learning=ref.allow_public_learning, + allow_public_read=ref.allow_public_read, has_unpublished_changes=has_unpublished_changes, has_unpublished_deletes=has_unpublished_deletes, ) -def create_library(collection_uuid, org, slug, title, description): +def create_library(collection_uuid, org, slug, title, description, allow_public_learning, allow_public_read): """ Create a new content library. @@ -171,6 +208,10 @@ def create_library(collection_uuid, org, slug, title, description): description: description of this library + allow_public_learning: Allow anyone to read/learn from blocks in the LMS + + allow_public_read: Allow anyone to view blocks (including source) in Studio? + Returns a ContentLibraryMetadata instance. """ assert isinstance(collection_uuid, UUID) @@ -189,8 +230,8 @@ def create_library(collection_uuid, org, slug, title, description): org=org, slug=slug, bundle_uuid=bundle.uuid, - allow_public_learning=True, - allow_public_read=True, + allow_public_learning=allow_public_learning, + allow_public_read=allow_public_read, ) except IntegrityError: delete_bundle(bundle.uuid) @@ -201,9 +242,22 @@ def create_library(collection_uuid, org, slug, title, description): title=title, description=description, version=0, + allow_public_learning=ref.allow_public_learning, + allow_public_read=ref.allow_public_read, ) +def get_library_team(library_key): + """ + Get the list of users/groups granted permission to use this library. + """ + ref = ContentLibrary.objects.get_by_key(library_key) + return [ + ContentLibraryPermissionEntry(user=entry.user, group=entry.group, access_level=entry.access_level) + for entry in ref.permission_grants.all() + ] + + def set_library_user_permissions(library_key, user, access_level): """ Change the specified user's level of access to this library. @@ -212,12 +266,39 @@ def set_library_user_permissions(library_key, user, access_level): """ ref = ContentLibrary.objects.get_by_key(library_key) if access_level is None: - ref.authorized_users.filter(user=user).delete() + ref.permission_grants.filter(user=user).delete() else: - ContentLibraryPermission.objects.update_or_create(user=user, library=ref, access_level=access_level) + ContentLibraryPermission.objects.update_or_create( + library=ref, + user=user, + defaults={"access_level": access_level}, + ) -def update_library(library_key, title=None, description=None): +def set_library_group_permissions(library_key, group, access_level): + """ + Change the specified group's level of access to this library. + + access_level should be one of the AccessLevel values defined above. + """ + ref = ContentLibrary.objects.get_by_key(library_key) + if access_level is None: + ref.permission_grants.filter(group=group).delete() + else: + ContentLibraryPermission.objects.update_or_create( + library=ref, + group=group, + defaults={"access_level": access_level}, + ) + + +def update_library( + library_key, + title=None, + description=None, + allow_public_learning=None, + allow_public_read=None, +): """ Update a library's title or description. (Slug cannot be changed as it would break IDs throughout the system.) @@ -225,6 +306,17 @@ def update_library(library_key, title=None, description=None): A value of None means "don't change". """ ref = ContentLibrary.objects.get_by_key(library_key) + # Update MySQL model: + changed = False + if allow_public_learning is not None: + ref.allow_public_learning = allow_public_learning + changed = True + if allow_public_read is not None: + ref.allow_public_read = allow_public_read + changed = True + if changed: + ref.save() + # Update Blockstore: fields = { # We don't ever read the "slug" value from the Blockstore bundle, but # we might as well always do our best to keep it in sync with the "slug" diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index 21c1c09598..f7d6ac010a 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -4,6 +4,9 @@ Definition of "Library" as a learning context. import logging +from django.core.exceptions import PermissionDenied + +from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.library_bundle import ( LibraryBundle, bundle_uuid_for_library_key, @@ -30,29 +33,45 @@ class LibraryContextImpl(LearningContext): def can_edit_block(self, user, usage_key): """ Does the specified usage key exist in its context, and if so, does the - specified user (which may be an AnonymousUser) have permission to edit - it? + specified user have permission to edit it (make changes to the authored + data store)? + + user: a Django User object (may be an AnonymousUser) + + usage_key: the UsageKeyV2 subclass used for this learning context Must return a boolean. """ + try: + api.require_permission_for_library_key(usage_key.lib_key, user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + except PermissionDenied: + return False def_key = self.definition_for_usage(usage_key) if not def_key: return False - # TODO: implement permissions return True def can_view_block(self, user, usage_key): """ Does the specified usage key exist in its context, and if so, does the - specified user (which may be an AnonymousUser) have permission to view - it and interact with it (call handlers, save user state, etc.)? + specified user have permission to view it and interact with it (call + handlers, save user state, etc.)? + + user: a Django User object (may be an AnonymousUser) + + usage_key: the UsageKeyV2 subclass used for this learning context Must return a boolean. """ + try: + api.require_permission_for_library_key( + usage_key.lib_key, user, permissions.CAN_LEARN_FROM_THIS_CONTENT_LIBRARY, + ) + except PermissionDenied: + return False def_key = self.definition_for_usage(usage_key) if not def_key: return False - # TODO: implement permissions return True def definition_for_usage(self, usage_key, **kwargs): diff --git a/openedx/core/djangoapps/content_libraries/migrations/0002_group_permissions.py b/openedx/core/djangoapps/content_libraries/migrations/0002_group_permissions.py new file mode 100644 index 0000000000..f2bdeb7d76 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/migrations/0002_group_permissions.py @@ -0,0 +1,46 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.26 on 2019-12-11 19:20 +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('auth', '0008_alter_user_username_max_length'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('content_libraries', '0001_initial'), + ] + + operations = [ + migrations.AlterModelOptions( + name='contentlibrarypermission', + options={'ordering': ('user__username', 'group__name')}, + ), + migrations.RemoveField( + model_name='contentlibrary', + name='authorized_users', + ), + migrations.AddField( + model_name='contentlibrarypermission', + name='group', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to='auth.Group', blank=True), + ), + migrations.AlterField( + model_name='contentlibrarypermission', + name='library', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='permission_grants', to='content_libraries.ContentLibrary'), + ), + migrations.AlterField( + model_name='contentlibrarypermission', + name='user', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL, blank=True), + ), + migrations.AlterUniqueTogether( + name='contentlibrarypermission', + unique_together=set([('library', 'user'), ('library', 'group')]), + ), + ] diff --git a/openedx/core/djangoapps/content_libraries/models.py b/openedx/core/djangoapps/content_libraries/models.py index 71f0653e66..7b122d736e 100644 --- a/openedx/core/djangoapps/content_libraries/models.py +++ b/openedx/core/djangoapps/content_libraries/models.py @@ -1,11 +1,10 @@ """ Models for new Content Libraries """ - - from django.contrib.auth import get_user_model +from django.contrib.auth.models import Group +from django.core.exceptions import ValidationError from django.db import models -from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ from opaque_keys.edx.locator import LibraryLocatorV2 from organizations.models import Organization @@ -26,7 +25,6 @@ class ContentLibraryManager(models.Manager): return self.get(org__short_name=library_key.org, slug=library_key.slug) -@python_2_unicode_compatible class ContentLibrary(models.Model): """ A Content Library is a collection of content (XBlocks and/or static assets) @@ -68,8 +66,6 @@ class ContentLibrary(models.Model): """), ) - authorized_users = models.ManyToManyField(User, through='ContentLibraryPermission') - class Meta: verbose_name_plural = "Content Libraries" unique_together = ("org", "slug") @@ -85,14 +81,15 @@ class ContentLibrary(models.Model): return "ContentLibrary ({})".format(six.text_type(self.library_key)) -@python_2_unicode_compatible class ContentLibraryPermission(models.Model): """ Row recording permissions for a content library """ - library = models.ForeignKey(ContentLibrary, on_delete=models.CASCADE) - user = models.ForeignKey(User, on_delete=models.CASCADE) - # TODO: allow permissions to be assign to a group, not just a user + library = models.ForeignKey(ContentLibrary, on_delete=models.CASCADE, related_name="permission_grants") + # One of the following must be set (but not both): + user = models.ForeignKey(User, null=True, blank=True, on_delete=models.CASCADE) + group = models.ForeignKey(Group, null=True, blank=True, on_delete=models.CASCADE) + # What level of access is granted to the above user or group: ADMIN_LEVEL = 'admin' AUTHOR_LEVEL = 'author' READ_LEVEL = 'read' @@ -103,5 +100,25 @@ class ContentLibraryPermission(models.Model): ) access_level = models.CharField(max_length=30, choices=ACCESS_LEVEL_CHOICES) + class Meta: + ordering = ('user__username', 'group__name') + unique_together = [ + ('library', 'user'), + ('library', 'group'), + ] + + def save(self, *args, **kwargs): # pylint: disable=arguments-differ + """ + Validate any constraints on the model. + + We can remove this and replace it with a proper database constraint + once we're upgraded to Django 2.2+ + """ + # if both are nonexistent or both are existing, error + if (not self.user) == (not self.group): + raise ValidationError(_("One and only one of 'user' and 'group' must be set.")) + return super().save(*args, **kwargs) + def __str__(self): - return "ContentLibraryPermission ({} for {})".format(self.access_level, self.user.username) + who = self.user.username if self.user else self.group.name + return "ContentLibraryPermission ({} for {})".format(self.access_level, who) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py new file mode 100644 index 0000000000..c2acfcf47f --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -0,0 +1,98 @@ +""" +Permissions for Content Libraries (v2, Blockstore-based) +""" +from bridgekeeper import perms, rules +from bridgekeeper.rules import Attribute, ManyRelation, Relation, in_current_groups +from django.contrib.auth.models import Group + +from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission + +# Is the user active (and their email verified)? +is_user_active = rules.is_authenticated & rules.is_active +# Is the user global staff? +is_global_staff = is_user_active & rules.is_staff + +# Helper rules used to define the permissions below + +# Does the user have at least read permission for the specified library? +has_explicit_read_permission_for_library = ( + ManyRelation( + # In newer versions of bridgekeeper, the 1st and 3rd arguments below aren't needed. + 'permission_grants', 'contentlibrarypermission', ContentLibraryPermission, + Attribute('user', lambda user: user) | Relation('group', Group, in_current_groups) + ) + # We don't check 'access_level' here because all access levels grant read permission +) +# Does the user have at least author permission for the specified library? +has_explicit_author_permission_for_library = ( + ManyRelation( + 'permission_grants', 'contentlibrarypermission', ContentLibraryPermission, + (Attribute('user', lambda user: user) | Relation('group', Group, in_current_groups)) & ( + Attribute('access_level', ContentLibraryPermission.AUTHOR_LEVEL) | + Attribute('access_level', ContentLibraryPermission.ADMIN_LEVEL) + ) + ) +) +# Does the user have admin permission for the specified library? +has_explicit_admin_permission_for_library = ( + ManyRelation( + 'permission_grants', 'contentlibrarypermission', ContentLibraryPermission, + (Attribute('user', lambda user: user) | Relation('group', Group, in_current_groups)) & + Attribute('access_level', ContentLibraryPermission.ADMIN_LEVEL) + ) +) + + +########################### Permissions ########################### + +# Is the user allowed to view XBlocks from the specified content library +# directly in the LMS, and interact with them? +# Note there is no is_authenticated/is_active check for this one - we allow +# anonymous users to learn if the library allows public learning. +CAN_LEARN_FROM_THIS_CONTENT_LIBRARY = 'content_libraries.learn_from_library' +perms[CAN_LEARN_FROM_THIS_CONTENT_LIBRARY] = ( + # Global staff can learn from any library: + is_global_staff | + # Regular users can learn if the library allows public learning: + Attribute('allow_public_learning', True) | + # Users/groups who are explicitly granted permission can learn from the library: + has_explicit_read_permission_for_library +) + +# Is the user allowed to create content libraries? +CAN_CREATE_CONTENT_LIBRARY = 'content_libraries.create_library' +perms[CAN_CREATE_CONTENT_LIBRARY] = is_user_active + +# Is the user allowed to view the specified content library in Studio, +# including to view the raw OLX and asset files? +CAN_VIEW_THIS_CONTENT_LIBRARY = 'content_libraries.view_library' +perms[CAN_VIEW_THIS_CONTENT_LIBRARY] = is_user_active & ( + # Global staff can access any library + is_global_staff | + # Some libraries allow anyone to view them in Studio: + Attribute('allow_public_read', True) | + # Otherwise the user must be part of the library's team + has_explicit_read_permission_for_library +) + +# Is the user allowed to edit the specified content library? +CAN_EDIT_THIS_CONTENT_LIBRARY = 'content_libraries.edit_library' +perms[CAN_EDIT_THIS_CONTENT_LIBRARY] = is_user_active & ( + is_global_staff | has_explicit_author_permission_for_library +) + +# Is the user allowed to view the users/permissions of the specified content library? +CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM = 'content_libraries.view_library_team' +perms[CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM] = perms[CAN_EDIT_THIS_CONTENT_LIBRARY] + +# Is the user allowed to edit the users/permissions of the specified content library? +CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM = 'content_libraries.edit_library_team' +perms[CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM] = is_user_active & ( + is_global_staff | has_explicit_admin_permission_for_library +) + +# Is the user allowed to delete the specified content library? +CAN_DELETE_THIS_CONTENT_LIBRARY = 'content_libraries.delete_library' +perms[CAN_DELETE_THIS_CONTENT_LIBRARY] = is_user_active & ( + is_global_staff | has_explicit_admin_permission_for_library +) diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 781c66c37c..998df00881 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -2,11 +2,10 @@ Serializers for the content libraries REST API """ # pylint: disable=abstract-method - - from django.core.validators import validate_unicode_slug from rest_framework import serializers +from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission from openedx.core.lib import blockstore_api @@ -16,7 +15,9 @@ class ContentLibraryMetadataSerializer(serializers.Serializer): """ # We rename the primary key field to "id" in the REST API since API clients # often implement magic functionality for fields with that name, and "key" - # is a reserved prop name in React + # is a reserved prop name in React. This 'id' field is a string that + # begins with 'lib:'. (The numeric ID of the ContentLibrary object in MySQL + # is not exposed via this API.) id = serializers.CharField(source="key", read_only=True) org = serializers.SlugField(source="key.org") slug = serializers.CharField(source="key.slug", validators=(validate_unicode_slug, )) @@ -25,6 +26,8 @@ class ContentLibraryMetadataSerializer(serializers.Serializer): title = serializers.CharField() description = serializers.CharField(allow_blank=True) version = serializers.IntegerField(read_only=True) + allow_public_learning = serializers.BooleanField(default=False) + allow_public_read = serializers.BooleanField(default=False) has_unpublished_changes = serializers.BooleanField(read_only=True) has_unpublished_deletes = serializers.BooleanField(read_only=True) @@ -36,6 +39,27 @@ class ContentLibraryUpdateSerializer(serializers.Serializer): # These are the only fields that support changes: title = serializers.CharField() description = serializers.CharField() + allow_public_learning = serializers.BooleanField() + allow_public_read = serializers.BooleanField() + + +class ContentLibraryPermissionLevelSerializer(serializers.Serializer): + """ + Serializer for the "Access Level" of a ContentLibraryPermission object. + + This is used when updating a user or group's permissions re some content + library. + """ + access_level = serializers.ChoiceField(choices=ContentLibraryPermission.ACCESS_LEVEL_CHOICES) + + +class ContentLibraryPermissionSerializer(ContentLibraryPermissionLevelSerializer): + """ + Serializer for a ContentLibraryPermission object, which grants either a user + or a group permission to view a content library. + """ + user_id = serializers.IntegerField(source="user.id", allow_null=True) + group_name = serializers.CharField(source="group.name", allow_null=True, allow_blank=False, default=None) class LibraryXBlockMetadataSerializer(serializers.Serializer): diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index fb2a38f316..63d8a7fa8e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -2,12 +2,12 @@ """ Tests for Blockstore-based Content Libraries """ - +from contextlib import contextmanager import unittest from django.conf import settings from organizations.models import Organization -from rest_framework.test import APITestCase +from rest_framework.test import APITestCase, APIClient import six from student.tests.factories import UserFactory @@ -22,6 +22,9 @@ URL_LIB_DETAIL = URL_PREFIX + '{lib_key}/' # Get data about a library, update o URL_LIB_BLOCK_TYPES = URL_LIB_DETAIL + 'block_types/' # Get the list of XBlock types that can be added to this library URL_LIB_COMMIT = URL_LIB_DETAIL + 'commit/' # Commit (POST) or revert (DELETE) all pending changes to this library URL_LIB_BLOCKS = URL_LIB_DETAIL + 'blocks/' # Get the list of XBlocks in this library, or add a new one +URL_LIB_TEAM = URL_LIB_DETAIL + 'team/' # Get the list of users/groups authorized to use this library +URL_LIB_TEAM_USER = URL_LIB_TEAM + 'user/{user_id}/' # Add/edit/remove a user's permission to use this library +URL_LIB_TEAM_GROUP = URL_LIB_TEAM + 'group/{group_name}/' # Add/edit/remove a group's permission to use this library URL_LIB_BLOCK = URL_PREFIX + 'blocks/{block_key}/' # Get data about a block, or delete it URL_LIB_BLOCK_OLX = URL_LIB_BLOCK + 'olx/' # Get or set the OLX of the specified XBlock URL_LIB_BLOCK_ASSETS = URL_LIB_BLOCK + 'assets/' # List the static asset files of the specified XBlock @@ -76,8 +79,21 @@ class ContentLibrariesRestApiTest(APITestCase): def setUp(self): super(ContentLibrariesRestApiTest, self).setUp() + self.clients_by_user = {} self.client.login(username=self.user.username, password="edx") + # Assertions + + def assertDictContainsEntries(self, big_dict, subset_dict): + """ + Assert that the first dict contains at least all of the same entries as + the second dict. + + Like python 2's assertDictContainsSubset, but with the arguments in the + correct order. + """ + self.assertGreaterEqual(big_dict.items(), subset_dict.items()) + # API helpers def _api(self, method, url, data, expect_response): @@ -91,6 +107,19 @@ class ContentLibrariesRestApiTest(APITestCase): ) return response.data + @contextmanager + def as_user(self, user): + """ + Context manager to call the REST API as a user other than self.user + """ + old_client = self.client + if user not in self.clients_by_user: + client = self.clients_by_user[user] = APIClient() + client.force_authenticate(user=user) + self.client = self.clients_by_user[user] # pylint: disable=attribute-defined-outside-init + yield + self.client = old_client # pylint: disable=attribute-defined-outside-init + def _create_library(self, slug, title, description="", expect_response=200): """ Create a library """ return self._api('post', URL_LIB_CREATE, { @@ -105,25 +134,45 @@ class ContentLibrariesRestApiTest(APITestCase): """ Get a library """ return self._api('get', URL_LIB_DETAIL.format(lib_key=lib_key), None, expect_response) - def _update_library(self, lib_key, **data): + def _update_library(self, lib_key, expect_response=200, **data): """ Update an existing library """ - return self._api('patch', URL_LIB_DETAIL.format(lib_key=lib_key), data=data, expect_response=200) + return self._api('patch', URL_LIB_DETAIL.format(lib_key=lib_key), data, expect_response) def _delete_library(self, lib_key, expect_response=200): """ Delete an existing library """ return self._api('delete', URL_LIB_DETAIL.format(lib_key=lib_key), None, expect_response) - def _commit_library_changes(self, lib_key): + def _commit_library_changes(self, lib_key, expect_response=200): """ Commit changes to an existing library """ - return self._api('post', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response=200) + return self._api('post', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response) - def _revert_library_changes(self, lib_key): + def _revert_library_changes(self, lib_key, expect_response=200): """ Revert pending changes to an existing library """ - return self._api('delete', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response=200) + return self._api('delete', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response) - def _get_library_blocks(self, lib_key): + def _get_library_team(self, lib_key, expect_response=200): + """ Get the list of users/groups authorized to use this library """ + return self._api('get', URL_LIB_TEAM.format(lib_key=lib_key), None, expect_response) + + def _set_user_access_level(self, lib_key, user_id, access_level, expect_response=200): + """ Change the specified user's access level """ + url = URL_LIB_TEAM_USER.format(lib_key=lib_key, user_id=user_id) + if access_level is None: + return self._api('delete', url, None, expect_response) + else: + return self._api('put', url, {"access_level": access_level}, expect_response) + + def _set_group_access_level(self, lib_key, group_name, access_level, expect_response=200): + """ Change the specified group's access level """ + url = URL_LIB_TEAM_GROUP.format(lib_key=lib_key, group_name=group_name) + if access_level is None: + return self._api('delete', url, None, expect_response) + else: + return self._api('put', url, {"access_level": access_level}, expect_response) + + def _get_library_blocks(self, lib_key, expect_response=200): """ Get the list of XBlocks in the library """ - return self._api('get', URL_LIB_BLOCKS.format(lib_key=lib_key), None, expect_response=200) + return self._api('get', URL_LIB_BLOCKS.format(lib_key=lib_key), None, expect_response) def _add_block_to_library(self, lib_key, block_type, slug, parent_block=None, expect_response=200): """ Add a new XBlock to the library """ diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 5d5b5b264e..59ad24819c 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -2,11 +2,13 @@ """ Tests for Blockstore-based Content Libraries """ - import unittest from uuid import UUID +from django.contrib.auth.models import Group + from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from student.tests.factories import UserFactory class ContentLibrariesTest(ContentLibrariesRestApiTest): @@ -49,18 +51,18 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): "has_unpublished_changes": False, "has_unpublished_deletes": False, } - self.assertDictContainsSubset(expected_data, lib) + self.assertDictContainsEntries(lib, expected_data) # Check that bundle_uuid looks like a valid UUID UUID(lib["bundle_uuid"]) # will raise an exception if not valid # Read: lib2 = self._get_library(lib["id"]) - self.assertDictContainsSubset(expected_data, lib2) + self.assertDictContainsEntries(lib2, expected_data) # Update: lib3 = self._update_library(lib["id"], title="New Title") expected_data["title"] = "New Title" - self.assertDictContainsSubset(expected_data, lib3) + self.assertDictContainsEntries(lib3, expected_data) # Delete: self._delete_library(lib["id"]) @@ -94,12 +96,12 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): # Add a 'problem' XBlock to the library: block_data = self._add_block_to_library(lib_id, "problem", "problem1") - self.assertDictContainsSubset({ + self.assertDictContainsEntries(block_data, { "id": "lb:CL-TEST:testlib1:problem:problem1", "display_name": "Blank Advanced Problem", "block_type": "problem", "has_unpublished_changes": True, - }, block_data) + }) block_id = block_data["id"] # Confirm that the result contains a definition key, but don't check its value, # which for the purposes of these tests is an implementation detail. @@ -114,7 +116,7 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): self.assertEqual(self._get_library(lib_id)["has_unpublished_changes"], False) # And now the block information should also show that block has no unpublished changes: block_data["has_unpublished_changes"] = False - self.assertDictContainsSubset(block_data, self._get_library_block(block_id)) + self.assertDictContainsEntries(self._get_library_block(block_id), block_data) self.assertEqual(self._get_library_blocks(lib_id), [block_data]) # Now update the block's OLX: @@ -138,10 +140,10 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): # now reading it back, we should get that exact OLX (no change to whitespace etc.): self.assertEqual(self._get_library_block_olx(block_id), new_olx) # And the display name and "unpublished changes" status of the block should be updated: - self.assertDictContainsSubset({ + self.assertDictContainsEntries(self._get_library_block(block_id), { "display_name": "New Multi Choice Question", "has_unpublished_changes": True, - }, self._get_library_block(block_id)) + }) # Now view the XBlock's student_view (including draft changes): fragment = self._render_block_view(block_id, "student_view") @@ -209,3 +211,147 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): # We cannot add a duplicate ID to the library, either at the top level or as a child: self._add_block_to_library(lib_id, "problem", "problem1", expect_response=400) self._add_block_to_library(lib_id, "problem", "problem1", parent_block=unit_block["id"], expect_response=400) + + # Test that permissions are enforced for content libraries + + def test_library_permissions(self): # pylint: disable=too-many-statements + """ + Test that permissions are enforced for content libraries, and that + permissions can be read and manipulated using the REST API (which in + turn tests the python API). + + This is a single giant test case, because that optimizes for the fastest + test run time, even though it can make debugging failures harder. + """ + # Create a few users to use for all of these tests: + admin = UserFactory.create(username="Admin", email="admin@example.com") + author = UserFactory.create(username="Author", email="author@example.com") + reader = UserFactory.create(username="Reader", email="reader@example.com") + group = Group.objects.create(name="group1") + author_group_member = UserFactory.create(username="GroupMember", email="groupmember@example.com") + author_group_member.groups.add(group) + random_user = UserFactory.create(username="Random", email="random@example.com") + + # Library CRUD ######################################################### + + # Create a library, owned by "Admin" + with self.as_user(admin): + lib = self._create_library(slug="permtest", title="Permission Test Library", description="Testing") + lib_id = lib["id"] + # By default, "public learning" and public read access are disallowed. + self.assertEqual(lib["allow_public_learning"], False) + self.assertEqual(lib["allow_public_read"], False) + + # By default, the creator of a new library is the only admin + data = self._get_library_team(lib_id) + self.assertEqual(len(data), 1) + self.assertDictContainsEntries(data[0], {"user_id": admin.pk, "group_name": None, "access_level": "admin"}) + + # Add the other users to the content library: + self._set_user_access_level(lib_id, author.pk, access_level="author") + self._set_user_access_level(lib_id, reader.pk, access_level="read") + self._set_group_access_level(lib_id, group.name, access_level="author") + + team_response = self._get_library_team(lib_id) + self.assertEqual(len(team_response), 4) + # The response should also always be sorted in a specific order (by username and group name): + expected_response = [ + {"user_id": None, "group_name": "group1", "access_level": "author"}, + {"user_id": admin.pk, "group_name": None, "access_level": "admin"}, + {"user_id": author.pk, "group_name": None, "access_level": "author"}, + {"user_id": reader.pk, "group_name": None, "access_level": "read"}, + ] + for entry, expected in zip(team_response, expected_response): + self.assertDictContainsEntries(entry, expected) + + # A random user cannot get the library nor its team: + with self.as_user(random_user): + self._get_library(lib_id, expect_response=403) + self._get_library_team(lib_id, expect_response=403) + + # But every authorized user can: + for user in [admin, author, author_group_member]: + with self.as_user(user): + self._get_library(lib_id) + data = self._get_library_team(lib_id) + self.assertEqual(data, team_response) + + # A user with only read permission can get data about the library but not the team: + with self.as_user(reader): + self._get_library(lib_id) + self._get_library_team(lib_id, expect_response=403) + + # Users without admin access cannot delete the library nor change its team: + for user in [author, reader, author_group_member, random_user]: + with self.as_user(user): + self._delete_library(lib_id, expect_response=403) + self._set_user_access_level(lib_id, author.pk, access_level="admin", expect_response=403) + self._set_user_access_level(lib_id, admin.pk, access_level=None, expect_response=403) + self._set_user_access_level(lib_id, random_user.pk, access_level="read", expect_response=403) + + # Users with author access (or higher) can edit the library's properties: + with self.as_user(author): + self._update_library(lib_id, description="Revised description") + with self.as_user(author_group_member): + self._update_library(lib_id, title="New Library Title") + # But other users cannot: + with self.as_user(reader): + self._update_library(lib_id, description="Prohibited description", expect_response=403) + with self.as_user(random_user): + self._update_library(lib_id, title="I can't set this title", expect_response=403) + # Verify the permitted changes were made: + with self.as_user(admin): + data = self._get_library(lib_id) + self.assertEqual(data["description"], "Revised description") + self.assertEqual(data["title"], "New Library Title") + + # Library XBlock editing ############################################### + + # users with read permission or less cannot add blocks: + for user in [reader, random_user]: + with self.as_user(user): + self._add_block_to_library(lib_id, "problem", "problem1", expect_response=403) + # But authors and admins can: + with self.as_user(admin): + self._add_block_to_library(lib_id, "problem", "problem1") + with self.as_user(author): + self._add_block_to_library(lib_id, "problem", "problem2") + with self.as_user(author_group_member): + block3_data = self._add_block_to_library(lib_id, "problem", "problem3") + block3_key = block3_data["id"] + + # At this point, the library contains 3 draft problem XBlocks. + + # A random user cannot read OLX nor assets (this library has allow_public_read False): + with self.as_user(random_user): + self._get_library_block_olx(block3_key, expect_response=403) + self._get_library_block_assets(block3_key, expect_response=403) + self._get_library_block_asset(block3_key, file_name="whatever.png", expect_response=403) + # But if we grant allow_public_read, then they can: + with self.as_user(admin): + self._update_library(lib_id, allow_public_read=True) + self._set_library_block_asset(block3_key, "whatever.png", b"data") + with self.as_user(random_user): + self._get_library_block_olx(block3_key) + self._get_library_block_assets(block3_key) + self._get_library_block_asset(block3_key, file_name="whatever.png") + + # Users without authoring permission cannot edit nor delete XBlocks (this library has allow_public_read False): + for user in [reader, random_user]: + with self.as_user(user): + self._set_library_block_olx(block3_key, "", expect_response=403) + self._set_library_block_asset(block3_key, "test.txt", b"data", expect_response=403) + self._delete_library_block(block3_key, expect_response=403) + self._commit_library_changes(lib_id, expect_response=403) + self._revert_library_changes(lib_id, expect_response=403) + + # But users with author permission can: + with self.as_user(author_group_member): + olx = self._get_library_block_olx(block3_key) + self._set_library_block_olx(block3_key, olx) + self._get_library_block_assets(block3_key) + self._set_library_block_asset(block3_key, "test.txt", b"data") + self._get_library_block_asset(block3_key, file_name="test.txt") + self._delete_library_block(block3_key) + self._commit_library_changes(lib_id) + self._revert_library_changes(lib_id) # This is a no-op after the commit, but should still have 200 response diff --git a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py index 92c742e95d..2dd2c3284d 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py @@ -2,7 +2,6 @@ """ Test the Blockstore-based XBlock runtime and content libraries together. """ - import json from completion.test_utils import CompletionWaffleTestMixin @@ -50,6 +49,8 @@ class ContentLibraryContentTestMixin(object): slug=cls.__name__, title=(cls.__name__ + " Test Lib"), description="", + allow_public_learning=True, + allow_public_read=False, ) @@ -81,6 +82,8 @@ class ContentLibraryRuntimeTest(ContentLibraryContentTestMixin, TestCase): slug="idolx", title=("Identical OLX Test Lib 2"), description="", + allow_public_learning=True, + allow_public_read=False, ) unit_block2_key = library_api.create_library_block(library2.key, "unit", "u1").usage_key library_api.create_library_block_child(unit_block2_key, "problem", "p1") diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 8df317561f..2c08f87086 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -1,8 +1,6 @@ """ URL configuration for Studio's Content Libraries REST API """ - - from django.conf.urls import include, url from . import views @@ -26,6 +24,12 @@ urlpatterns = [ url(r'^blocks/$', views.LibraryBlocksView.as_view()), # Commit (POST) or revert (DELETE) all pending changes to this library: url(r'^commit/$', views.LibraryCommitView.as_view()), + # Get the list of users/groups who have permission to view/edit/administer this library: + url(r'^team/$', views.LibraryTeamView.as_view()), + # Add/Edit (PUT) or remove (DELETE) a user's permission to use this library + url(r'^team/user/(?P\d+)/$', views.LibraryTeamUserView.as_view()), + # Add/Edit (PUT) or remove (DELETE) a group's permission to use this library + url(r'^team/group/(?P[^/]+)/$', views.LibraryTeamGroupView.as_view()), ])), url(r'^blocks/(?P[^/]+)/', include([ # Get metadata about a specific XBlock in this library, or delete the block: diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 913f8eca15..6decb85803 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -1,23 +1,26 @@ """ REST API for Blockstore-based content libraries """ - from functools import wraps import logging +from django.contrib.auth import get_user_model +from django.contrib.auth.models import Group +from django.shortcuts import get_object_or_404 from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from organizations.models import Organization from rest_framework import status -from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError from rest_framework.parsers import MultiPartParser from rest_framework.response import Response from rest_framework.views import APIView -from openedx.core.lib.api.view_utils import view_auth_classes -from . import api -from .serializers import ( +from openedx.core.djangoapps.content_libraries import api, permissions +from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryMetadataSerializer, ContentLibraryUpdateSerializer, + ContentLibraryPermissionLevelSerializer, + ContentLibraryPermissionSerializer, LibraryXBlockCreationSerializer, LibraryXBlockMetadataSerializer, LibraryXBlockTypeSerializer, @@ -25,7 +28,9 @@ from .serializers import ( LibraryXBlockStaticFileSerializer, LibraryXBlockStaticFilesSerializer, ) +from openedx.core.lib.api.view_utils import view_auth_classes +User = get_user_model() log = logging.getLogger(__name__) @@ -62,16 +67,19 @@ class LibraryRootView(APIView): def get(self, request): """ - Return a list of all content libraries. This is a temporary view for - development. + Return a list of all content libraries that the user has permission to + view. This is a temporary view for development and returns at most 50 + libraries. """ - result = api.list_libraries() + result = api.list_libraries_for_user(request.user) return Response(ContentLibraryMetadataSerializer(result, many=True).data) def post(self, request): """ Create a new content library. """ + if not request.user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY): + raise PermissionDenied serializer = ContentLibraryMetadataSerializer(data=request.data) serializer.is_valid(raise_exception=True) data = serializer.validated_data @@ -103,6 +111,7 @@ class LibraryDetailsView(APIView): Get a specific content library """ key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) result = api.get_library(key) return Response(ContentLibraryMetadataSerializer(result).data) @@ -112,6 +121,7 @@ class LibraryDetailsView(APIView): Update a content library """ key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) serializer = ContentLibraryUpdateSerializer(data=request.data, partial=True) serializer.is_valid(raise_exception=True) api.update_library(key, **serializer.validated_data) @@ -124,10 +134,97 @@ class LibraryDetailsView(APIView): Delete a content library """ key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_DELETE_THIS_CONTENT_LIBRARY) api.delete_library(key) return Response({}) +@view_auth_classes() +class LibraryTeamView(APIView): + """ + View to get the list of users/groups who can access and edit the content + library. + + Note also the 'allow_public_' settings which can be edited by PATCHing the + library itself (LibraryDetailsView.patch). + """ + @convert_exceptions + def get(self, request, lib_key_str): + """ + Get the list of users and groups who have permissions to view and edit + this library. + """ + key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM) + team = api.get_library_team(key) + return Response(ContentLibraryPermissionSerializer(team, many=True).data) + + +@view_auth_classes() +class LibraryTeamUserView(APIView): + """ + View to add/remove/edit an individual user's permissions for a content + library. + """ + @convert_exceptions + def put(self, request, lib_key_str, user_id): + """ + Add a user to this content library, with permissions specified in the + request body. + """ + key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM) + serializer = ContentLibraryPermissionLevelSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + user = get_object_or_404(User, pk=int(user_id)) + api.set_library_user_permissions(key, user, access_level=serializer.validated_data["access_level"]) + return Response({}) + + @convert_exceptions + def delete(self, request, lib_key_str, user_id): + """ + Remove the specified user's permission to access or edit this content + library. + """ + key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM) + user = get_object_or_404(User, pk=int(user_id)) + api.set_library_user_permissions(key, user, access_level=None) + return Response({}) + + +@view_auth_classes() +class LibraryTeamGroupView(APIView): + """ + View to add/remove/edit a group's permissions for a content library. + """ + @convert_exceptions + def put(self, request, lib_key_str, group_name): + """ + Add a group to this content library, with permissions specified in the + request body. + """ + key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM) + serializer = ContentLibraryPermissionLevelSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + group = get_object_or_404(Group, name=group_name) + api.set_library_group_permissions(key, group, access_level=serializer.validated_data["access_level"]) + return Response({}) + + @convert_exceptions + def delete(self, request, lib_key_str, user_id): + """ + Remove the specified user's permission to access or edit this content + library. + """ + key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM) + group = get_object_or_404(Group, pk=int(user_id)) + api.set_library_group_permissions(key, group, access_level=None) + return Response({}) + + @view_auth_classes() class LibraryBlockTypesView(APIView): """ @@ -139,6 +236,7 @@ class LibraryBlockTypesView(APIView): Get the list of XBlock types that can be added to this library """ key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) result = api.get_allowed_block_types(key) return Response(LibraryXBlockTypeSerializer(result, many=True).data) @@ -155,6 +253,7 @@ class LibraryCommitView(APIView): descendants. """ key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) api.publish_changes(key) return Response({}) @@ -165,6 +264,7 @@ class LibraryCommitView(APIView): descendants. Restore it to the last published version """ key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) api.revert_changes(key) return Response({}) @@ -180,6 +280,7 @@ class LibraryBlocksView(APIView): Get the list of all top-level blocks in this content library """ key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) result = api.get_library_blocks(key) return Response(LibraryXBlockMetadataSerializer(result, many=True).data) @@ -189,6 +290,7 @@ class LibraryBlocksView(APIView): Add a new XBlock to this content library """ library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) serializer = LibraryXBlockCreationSerializer(data=request.data) serializer.is_valid(raise_exception=True) parent_block_usage_str = serializer.validated_data.pop("parent_block", None) @@ -215,6 +317,7 @@ class LibraryBlockView(APIView): Get metadata about an existing XBlock in the content library """ key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) result = api.get_library_block(key) return Response(LibraryXBlockMetadataSerializer(result).data) @@ -232,6 +335,7 @@ class LibraryBlockView(APIView): be deleted but the link and the linked bundle will be unaffected. """ key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) api.delete_library_block(key) return Response({}) @@ -247,6 +351,7 @@ class LibraryBlockOlxView(APIView): Get the block's OLX """ key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) xml_str = api.get_library_block_olx(key) return Response(LibraryXBlockOlxSerializer({"olx": xml_str}).data) @@ -259,6 +364,7 @@ class LibraryBlockOlxView(APIView): Very little validation is done. """ key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) serializer = LibraryXBlockOlxSerializer(data=request.data) serializer.is_valid(raise_exception=True) new_olx_str = serializer.validated_data["olx"] @@ -280,6 +386,7 @@ class LibraryBlockAssetListView(APIView): List the static asset files belonging to this block. """ key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) files = api.get_library_block_static_asset_files(key) return Response(LibraryXBlockStaticFilesSerializer({"files": files}).data) @@ -297,6 +404,7 @@ class LibraryBlockAssetView(APIView): Get a static asset file belonging to this block. """ key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) files = api.get_library_block_static_asset_files(key) for f in files: if f.path == file_path: @@ -309,6 +417,9 @@ class LibraryBlockAssetView(APIView): Replace a static asset file belonging to this block. """ usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key( + usage_key.lib_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + ) file_wrapper = request.data['content'] if file_wrapper.size > 20 * 1024 * 1024: # > 20 MiB # In the future, we need a way to use file_wrapper.chunks() to read @@ -323,11 +434,14 @@ class LibraryBlockAssetView(APIView): return Response(LibraryXBlockStaticFileSerializer(result).data) @convert_exceptions - def delete(self, request, usage_key_str, file_path): # pylint: disable=unused-argument + def delete(self, request, usage_key_str, file_path): """ Delete a static asset file belonging to this block. """ usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key( + usage_key.lib_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + ) try: api.delete_library_block_static_asset_file(usage_key, file_path) except ValueError: diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 000477e4fa..a3449a198f 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -62,14 +62,9 @@ def load_block(usage_key, user): # Is this block part of a course, a library, or what? # Get the Learning Context Implementation based on the usage key context_impl = get_learning_context_impl(usage_key) - # Now, using the LearningContext and the Studio/LMS-specific logic, check if - # the block exists in this context and if the user has permission to render - # this XBlock view: - if get_xblock_app_config().require_edit_permission: - authorized = context_impl.can_edit_block(user, usage_key) - else: - authorized = context_impl.can_view_block(user, usage_key) - if not authorized: + # Now, check if the block exists in this context and if the user has + # permission to render this XBlock view: + if user is not None and not context_impl.can_view_block(user, usage_key): # We do not know if the block was not found or if the user doesn't have # permission, but we want to return the same result in either case: raise NotFound("XBlock {} does not exist, or you don't have permission to view it.".format(usage_key)) diff --git a/openedx/core/djangoapps/xblock/apps.py b/openedx/core/djangoapps/xblock/apps.py index 04d9e76a00..27add726ef 100644 --- a/openedx/core/djangoapps/xblock/apps.py +++ b/openedx/core/djangoapps/xblock/apps.py @@ -17,10 +17,6 @@ class XBlockAppConfig(AppConfig): verbose_name = 'New XBlock Runtime' label = 'xblock_new' # The name 'xblock' is already taken by ORA2's 'openassessment.xblock' app :/ - # If this is True, users must have 'edit' permission to be allowed even to - # view content. (It's only true in Studio) - require_edit_permission = False - def get_runtime_system_params(self): """ Get the XBlockRuntimeSystem parameters appropriate for viewing and/or @@ -72,8 +68,6 @@ class StudioXBlockAppConfig(XBlockAppConfig): """ Studio-specific configuration of the XBlock Runtime django app. """ - # In Studio, users must have 'edit' permission to be allowed even to view content - require_edit_permission = True BLOCKSTORE_DRAFT_NAME = "studio_draft" diff --git a/openedx/core/djangoapps/xblock/learning_context/learning_context.py b/openedx/core/djangoapps/xblock/learning_context/learning_context.py index 976abbe761..16ac21e1b0 100644 --- a/openedx/core/djangoapps/xblock/learning_context/learning_context.py +++ b/openedx/core/djangoapps/xblock/learning_context/learning_context.py @@ -26,7 +26,8 @@ class LearningContext(object): def can_edit_block(self, user, usage_key): # pylint: disable=unused-argument """ Does the specified usage key exist in its context, and if so, does the - specified user have permission to edit it? + specified user have permission to edit it (make changes to the authored + data store)? user: a Django User object (may be an AnonymousUser) diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index 821a2232d9..8b3558bced 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -120,6 +120,13 @@ class BlockstoreXBlockRuntime(XBlockRuntime): "The Blockstore runtime does not support saving changes to blockstore without a draft. " "Are you making changes to UserScope.NONE fields from the LMS rather than Studio?" ) + # Verify that the user has permission to write to authored data in this + # learning context: + if self.user is not None: + learning_context = get_learning_context_impl(block.scope_ids.usage_id) + if not learning_context.can_edit_block(self.user, block.scope_ids.usage_id): + log.warning("User %s does not have permission to edit %s", self.user.username, block.scope_ids.usage_id) + raise RuntimeError("You do not have permission to edit this XBlock") olx_str, static_files = serialize_xblock(block) # Write the OLX file to the bundle: draft_uuid = blockstore_api.get_or_create_bundle_draft(