diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py index c66663cd208d..ea119ea895e0 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py @@ -23,6 +23,9 @@ class AdvancedCourseSettingsView(DeveloperErrorViewMixin, APIView): """ View for getting and setting the advanced settings for a course. + + Deprecated: use ``GET`` and ``PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/`` + instead. See https://github.com/openedx/openedx-platform/issues/39128. """ class FilterQuery(forms.Form): @@ -65,6 +68,12 @@ def get(self, request: Request, course_id: str): """ Get an object containing all the advanced settings in a course. + Deprecated: use ``GET /api/authoring/v1/courses/{course_key}/advanced_settings/``. + Equivalent v1 reads: without ``view`` = this endpoint with ``fetch_all=0``; + ``view=full`` = this endpoint's default; ``fields`` replaces ``filter_fields`` + and selects within the chosen view. + See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** GET /api/contentstore/v0/advanced_settings/{course_id} @@ -148,6 +157,9 @@ def patch(self, request: Request, course_id: str): """ Update a course's advanced settings. + Deprecated: use ``PATCH /api/authoring/v1/courses/{course_key}/advanced_settings/``, + which accepts this body unchanged. See https://github.com/openedx/openedx-platform/issues/39128. + **Example Request** PATCH /api/contentstore/v0/advanced_settings/{course_id} { diff --git a/cms/djangoapps/contentstore/rest_api/v1/advanced_settings_service.py b/cms/djangoapps/contentstore/rest_api/v1/advanced_settings_service.py new file mode 100644 index 000000000000..3a641e488000 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/advanced_settings_service.py @@ -0,0 +1,90 @@ +"""Reads and updates of a course's advanced settings.""" + +import logging + +from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.settings import api_settings + +from cms.djangoapps.contentstore.views.course import update_course_advanced_settings +from cms.djangoapps.contentstore.views.permissions import COURSE_NOT_FOUND_MESSAGE +from cms.djangoapps.models.settings.course_metadata import CourseMetadata +from xmodule.modulestore.django import modulestore + +log = logging.getLogger(__name__) + +# Reported under the name of each setting whose value the course refused. The +# validator's own explanation is logged, not returned. +INVALID_SETTING_MESSAGE = "The value is not valid for this setting." +# Reported when the settings could not be saved for a reason tied to no setting. +SETTINGS_NOT_SAVED_MESSAGE = "The settings could not be saved. No setting was changed." + + +def _course_block(course_key): + """Return the course block of ``course_key``, or raise ``NotFound`` when the modulestore has none.""" + course_block = modulestore().get_course(course_key) + if course_block is None: + raise NotFound(COURSE_NOT_FOUND_MESSAGE) + return course_block + + +def settings_authors_do_not_edit(course_key): + """Return the names of the course's settings that are not edited as advanced settings.""" + return frozenset(CourseMetadata.get_exclude_list_of_fields(course_key)) + + +def get_advanced_settings(course_key, full=False): + """ + Return the advanced settings of the course, keyed by setting name. + + By default only the settings course authors edit in Studio are returned; + with ``full`` every setting of the course is. + """ + course_block = _course_block(course_key) + if full: + return CourseMetadata.fetch_all(course_block) + return CourseMetadata.fetch(course_block) + + +def update_advanced_settings(course_key, changes, user): + """ + Apply ``changes`` (``{name: {"value": value}}``) to the course as ``user``. + + Every value is validated before any is stored; when one is refused, + nothing is stored and ``ValidationError`` maps each refused setting to + ``INVALID_SETTING_MESSAGE``. + Returns the settings course authors edit in Studio, as stored. + """ + course_block = _course_block(course_key) + try: + return update_course_advanced_settings(course_block, changes, user) + except ValidationError as error: + raise _errors_by_setting(course_key, error) from error + + +def _errors_by_setting(course_key, error): + """ + Return a ``ValidationError`` mapping each refused setting to a fixed message. + + The settings validator reports one entry per refused setting, carrying the + setting's name and its own explanation, which can be the text of a parsing + exception. The explanation is logged with the course and setting; the + caller gets ``INVALID_SETTING_MESSAGE`` under the setting's name. Any + failure not tied to a setting is logged and reported with + ``SETTINGS_NOT_SAVED_MESSAGE``. + """ + detail = error.detail if isinstance(error.detail, list) else [error.detail] + by_setting = {} + unattributed = [] + for entry in detail: + name = entry.get("key") if isinstance(entry, dict) else None + if name: + log.warning( + "Advanced setting %s of course %s was refused: %s", name, course_key, entry.get("message", ""), + ) + by_setting[str(name)] = [INVALID_SETTING_MESSAGE] + else: + unattributed.append(entry) + if unattributed: + log.warning("Advanced settings of course %s were not saved: %s", course_key, unattributed) + by_setting.setdefault(api_settings.NON_FIELD_ERRORS_KEY, []).append(SETTINGS_NOT_SAVED_MESSAGE) + return ValidationError(by_setting) diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py index 5a53ae650413..afea706611ba 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -7,6 +7,7 @@ YoutubeTranscriptChecksViewSet, YoutubeTranscriptImportsViewSet, ) +from cms.djangoapps.contentstore.rest_api.v1.views.advanced_settings import CourseAdvancedSettingsViewSet app_name = "authoring_v1" @@ -39,4 +40,9 @@ ), name="xblock_detail", ), + path( + "courses//advanced_settings/", + CourseAdvancedSettingsViewSet.as_view({"get": "retrieve", "patch": "partial_update"}), + name="course_advanced_settings", + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/advanced_settings.py new file mode 100644 index 000000000000..7090efd864b1 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/advanced_settings.py @@ -0,0 +1,144 @@ +"""Serializers for a course's advanced settings.""" + +from collections.abc import Mapping + +from drf_spectacular.extensions import OpenApiSerializerExtension +from drf_spectacular.plumbing import build_object_type, get_doc +from rest_framework import serializers +from rest_framework.exceptions import ValidationError +from rest_framework.settings import api_settings + +MINIMAL_VIEW = "minimal" +FULL_VIEW = "full" + + +class AdvancedSettingSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One advanced setting of a course, as Studio presents it for editing.""" + + value = serializers.JSONField( + allow_null=True, + help_text="Current value of the setting. Its type depends on the setting.", + ) + display_name = serializers.CharField(help_text="Name of the setting as Studio shows it.") + help = serializers.CharField( + allow_null=True, + help_text="Text explaining what the setting does, or null when the setting has none.", + ) + deprecated = serializers.BooleanField(help_text="Whether the setting is deprecated.") + hide_on_enabled_publisher = serializers.BooleanField( + help_text="Whether Studio hides the setting when the course is managed by a publishing service.", + ) + + class Meta: + ref_name = "AdvancedSetting.v1" + + +class AdvancedSettingUpdateSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The new value of one advanced setting.""" + + value = serializers.JSONField( + allow_null=True, + help_text="New value of the setting. Its type depends on the setting.", + ) + + class Meta: + ref_name = "AdvancedSettingUpdate.v1" + + +class _SettingsByNameSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + An object whose members are settings keyed by setting name. + + The set of names depends on the course, so the members cannot be declared + as fields; each one is handled by ``setting_serializer_class``. Names given + in the ``ignored_settings`` context entry are dropped before any member is + validated. + """ + + setting_serializer_class = None + + def to_representation(self, instance): + return { + name: self.setting_serializer_class(setting).data + for name, setting in instance.items() + } + + def to_internal_value(self, data): + if not isinstance(data, Mapping): + message = self.error_messages["invalid"].format(datatype=type(data).__name__) + raise ValidationError({api_settings.NON_FIELD_ERRORS_KEY: [message]}, code="invalid") + ignored = self.context.get("ignored_settings", frozenset()) + validated, errors = {}, {} + for name, item in data.items(): + if name in ignored: + continue + setting = self.setting_serializer_class(data=item) + if setting.is_valid(): + validated[name] = dict(setting.validated_data) + else: + errors[name] = [str(message) for messages in setting.errors.values() for message in messages] + if errors: + raise ValidationError(errors) + return validated + + +class CourseAdvancedSettingsSerializer(_SettingsByNameSerializer): # pylint: disable=abstract-method + """A course's advanced settings, keyed by setting name in the order the course declares them.""" + + setting_serializer_class = AdvancedSettingSerializer + + class Meta: + ref_name = "CourseAdvancedSettings.v1" + + +class CourseAdvancedSettingsUpdateSerializer(_SettingsByNameSerializer): # pylint: disable=abstract-method + """ + New values for some of a course's advanced settings, keyed by setting name. + + Settings that authors do not edit are ignored, whatever their entry holds; + settings the course does not have are ignored once their entry is valid. + """ + + setting_serializer_class = AdvancedSettingUpdateSerializer + + class Meta: + ref_name = "CourseAdvancedSettingsUpdate.v1" + + +class AdvancedSettingsQuerySerializer(serializers.Serializer): # pylint: disable=abstract-method + """Query parameters of a read of a course's advanced settings.""" + + view = serializers.ChoiceField( + choices=[MINIMAL_VIEW, FULL_VIEW], + default=MINIMAL_VIEW, + help_text=( + "Which settings to return. 'minimal' returns the settings course authors edit in Studio; " + "'full' returns every setting of the course, including those Studio manages on other pages " + "and those of features that are turned off." + ), + ) + fields = serializers.CharField( + required=False, + allow_blank=True, + help_text=( + "Comma-separated names of the settings to return, out of those the view returns. " + "Names the view does not return are ignored." + ), + ) + + class Meta: + ref_name = "AdvancedSettingsQuery.v1" + + +class SettingsByNameSerializerExtension(OpenApiSerializerExtension): + """Publish a settings-by-name serializer as an object whose members are its setting component.""" + + target_class = _SettingsByNameSerializer + match_subclasses = True + + def map_serializer(self, auto_schema, direction): + setting = auto_schema.resolve_serializer(self.target.setting_serializer_class, direction) + return build_object_type( + description=get_doc(self.target.__class__), + additionalProperties=setting.ref, + ) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/views/advanced_settings.py new file mode 100644 index 000000000000..cddb719044f9 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/advanced_settings.py @@ -0,0 +1,203 @@ +"""API views for a course's advanced settings.""" + +from django.core.exceptions import ImproperlyConfigured +from drf_spectacular.utils import OpenApiExample, OpenApiParameter, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from edx_rest_framework_extensions.errors import ErrorResponseSerializer +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from edx_rest_framework_extensions.shaping import project +from edx_rest_framework_extensions.url_converters import CourseKeyConverter +from rest_framework import viewsets +from rest_framework.permissions import IsAuthenticated +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service import ( + INVALID_SETTING_MESSAGE, + get_advanced_settings, + settings_authors_do_not_edit, + update_advanced_settings, +) +from cms.djangoapps.contentstore.rest_api.v1.serializers.advanced_settings import ( + FULL_VIEW, + MINIMAL_VIEW, + AdvancedSettingsQuerySerializer, + CourseAdvancedSettingsSerializer, + CourseAdvancedSettingsUpdateSerializer, +) +from cms.djangoapps.contentstore.views.permissions import ( + CanManageAdvancedSettings, + CanViewAdvancedSettings, + IsExistingCourse, +) + +_COURSE_KEY_PARAMETER = OpenApiParameter( + name="course_key", + description="Key of the course, such as course-v1:edX+DemoX+2026.", + location=OpenApiParameter.PATH, + type=str, + pattern=f"^{CourseKeyConverter.regex}$", +) + + +def _query_parameter(name, **kwargs): + return OpenApiParameter( + name=name, + description=AdvancedSettingsQuerySerializer().fields[name].help_text, + required=False, + location=OpenApiParameter.QUERY, + **kwargs, + ) + + +def _error(description): + return OpenApiResponse(response=ErrorResponseSerializer, description=description) + + +_SETTINGS_EXAMPLE = OpenApiExample( + "Advanced settings", + summary="Two of a course's settings", + response_only=True, + status_codes=["200"], + value={ + "display_name": { + "value": "Demonstration Course", + "display_name": "Course Display Name", + "help": "Enter the name of the course as it should appear in the course list.", + "deprecated": False, + "hide_on_enabled_publisher": False, + }, + "days_early_for_beta": { + "value": None, + "display_name": "Days Early for Beta Users", + "help": "Enter the number of days before the start date that beta users can access the course.", + "deprecated": False, + "hide_on_enabled_publisher": False, + }, + }, +) + +_UPDATE_EXAMPLE = OpenApiExample( + "New values", + summary="Turn on the calculator and set the advanced modules", + request_only=True, + value={ + "show_calculator": {"value": True}, + "advanced_modules": {"value": ["poll", "survey"]}, + }, +) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class CourseAdvancedSettingsViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + The advanced settings of one course: its policy settings keyed by name. + + By default (``view=minimal``) the settings course authors edit in Studio + are returned; with ``view=full`` every setting of the course is. A partial + update changes the values of the settings it names, all of them or none. + + Anyone who may view the course's advanced settings may read them; changing + them needs the permission to manage them. The course is looked up first, + so a signed-in caller learns that a course does not exist (404) before + being refused (403). + + A plain ``ViewSet``: the settings are fields of the course block in the + modulestore, not database rows. + """ + + # An inactive account's browser session stays authenticated, so the course permission decides: staff status + # admits it, and its roles count only on courses where policy-based permissions are on. + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = (IsAuthenticated, IsExistingCourse) + action_permission_classes = { + "retrieve": (CanViewAdvancedSettings,), + "partial_update": (CanManageAdvancedSettings,), + } + # A method with no route (answered 405) and OPTIONS serve no settings, so + # only the course is checked for them. + unrouted_actions = (None, "metadata") + serializer_class = CourseAdvancedSettingsSerializer + + def get_permissions(self): + """ + Check the course, then the permission the requested action needs. + + An action with no permission declared in ``action_permission_classes`` + is refused with a server error instead of being served unchecked. + """ + if self.action in self.unrouted_actions: + return super().get_permissions() + if self.action not in self.action_permission_classes: + raise ImproperlyConfigured(f"No permission is declared for the {self.action!r} action.") + specific = self.action_permission_classes[self.action] + return [*super().get_permissions(), *(permission() for permission in specific)] + + def get_serializer(self, *args, **kwargs): + """Instantiate the settings serializer declared on this viewset.""" + return self.serializer_class(*args, **kwargs) + + @extend_schema( + summary="Get a course's advanced settings", + description=( + "Returns the course's advanced settings keyed by setting name, each with its value, its name " + "as Studio shows it, its help text and whether it is deprecated. By default (view=minimal) the " + "settings course authors edit in Studio are returned; view=full returns every setting of the " + "course. fields narrows the response to the named settings." + ), + parameters=[ + _COURSE_KEY_PARAMETER, + _query_parameter("view", type=str, enum=[MINIMAL_VIEW, FULL_VIEW], default=MINIMAL_VIEW), + _query_parameter("fields", type=str), + ], + responses={ + 200: OpenApiResponse( + response=CourseAdvancedSettingsSerializer, + description="The course's advanced settings.", + ), + 400: _error("A query parameter has an invalid value."), + 401: _error("The requester is not authenticated."), + 403: _error("The requester may not view this course's advanced settings."), + 404: _error("The course does not exist."), + }, + examples=[_SETTINGS_EXAMPLE], + ) + def retrieve(self, request, course_key): + """Return the course's advanced settings, narrowed to ``fields`` when given.""" + query = AdvancedSettingsQuerySerializer(data=request.query_params) + query.is_valid(raise_exception=True) + params = query.validated_data + current = get_advanced_settings(course_key, full=params["view"] == FULL_VIEW) + return Response(project(self.get_serializer(current).data, params.get("fields"))) + + @extend_schema( + summary="Change some of a course's advanced settings", + description=( + "Sets the value of each setting named in the body. Every value is validated before any is " + "stored: when one is refused nothing is changed, and the error's errors member maps the name " + f"of each refused setting to the message '{INVALID_SETTING_MESSAGE}'. Settings that authors do " + "not edit are ignored, and so are settings the course does not have. Returns the settings " + "course authors edit in Studio, as stored." + ), + parameters=[_COURSE_KEY_PARAMETER], + request={"application/json": CourseAdvancedSettingsUpdateSerializer}, + responses={ + 200: OpenApiResponse( + response=CourseAdvancedSettingsSerializer, + description="The settings course authors edit in Studio, after the change.", + ), + 400: _error("The body is not an object of settings, or a value was refused."), + 401: _error("The requester is not authenticated."), + 403: _error("The requester may not change this course's advanced settings."), + 404: _error("The course does not exist."), + }, + examples=[_SETTINGS_EXAMPLE, _UPDATE_EXAMPLE], + ) + def partial_update(self, request, course_key): + """Store the new values and return the editable settings as stored.""" + changes = CourseAdvancedSettingsUpdateSerializer( + data=request.data, context={"ignored_settings": settings_authors_do_not_edit(course_key)}, + ) + changes.is_valid(raise_exception=True) + stored = update_advanced_settings(course_key, changes.validated_data, request.user) + return Response(self.get_serializer(stored).data) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/helpers.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/helpers.py new file mode 100644 index 000000000000..08e7c5748e43 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/helpers.py @@ -0,0 +1,254 @@ +"""Shared helpers for the authoring v1 endpoint tests.""" + +from contextlib import contextmanager +from functools import cache +from unittest.mock import patch + +import jsonschema +import pymongo +from crum import signals as crum_signals +from django.core import signals as core_signals +from django.db import connection +from django.db.models import signals as model_signals +from django.dispatch import Signal +from django.test.utils import CaptureQueriesContext +from edx_rest_framework_extensions.testing import assert_error_envelope +from openedx_authz.models.engine import PolicyCacheControl +from openedx_events.tooling import OpenEdxPublicSignal +from rest_framework.test import APIClient + +from cms.djangoapps.contentstore.rest_api.v2.views.tests.helpers import generate_cms_schema +from xmodule.modulestore.mixed import MixedModuleStore + +COURSE_NOT_FOUND = "The requested course does not exist." +UNKNOWN_COURSE_KEY = "course-v1:NoSuchOrg+NoSuchCourse+NoSuchRun" +VERSION_ONLY_KEY = "course-v1:version@0123456789abcdef01234567" + + +# --------------------------------------------------------------------------- +# Clients and refusals +# --------------------------------------------------------------------------- + +def api_client(user=None): + """An API client that returns server errors as responses, authenticated as ``user`` if given.""" + client = APIClient() + client.raise_request_exception = False + if user is not None: + client.force_authenticate(user=user) + return client + + +def session_client(user): + """A client signed in to a browser session as ``user``.""" + user.set_password("password") + user.save() + client = APIClient() + assert client.login(username=user.username, password="password") + return client + + +def assert_refused(response, status): + """Assert ``response`` is the error envelope for ``status``; a 404 names the missing course.""" + slug = {401: "authn", 403: "authz", 404: "not-found"}[status] + envelope = assert_error_envelope(response, expected_status=status, expected_type_slug=slug) + if status == 404: + assert envelope["detail"] == COURSE_NOT_FOUND + return envelope + + +# --------------------------------------------------------------------------- +# The published schema +# --------------------------------------------------------------------------- + +@cache +def published_schema(): + """The CMS authoring schema, generated once per test process.""" + return generate_cms_schema() + + +def as_json_schema(node): + """An OpenAPI 3.0 schema as JSON Schema: ``nullable`` becomes a null alternative.""" + if isinstance(node, list): + return [as_json_schema(item) for item in node] + if not isinstance(node, dict): + return node + converted = {key: as_json_schema(value) for key, value in node.items() if key != "nullable"} + if node.get("nullable"): + return {"anyOf": [converted, {"type": "null"}]} + return converted + + +def assert_matches_published_schema(body, component): + """Assert ``body`` is valid against the published schema component ``component``.""" + components = as_json_schema(published_schema()["components"]["schemas"]) + jsonschema.validate(body, {"$ref": f"#/components/schemas/{component}", "components": {"schemas": components}}) + + +# --------------------------------------------------------------------------- +# Comparing two response bodies +# --------------------------------------------------------------------------- + +VOLATILE_FIELDS = {"instance"} +_ABSENT = object() + + +def flatten_json(obj, prefix=""): + """ + A ``{json_path: value}`` map of every leaf of ``obj``, without volatile + members. An empty object or array is a leaf, so a container that empties + or changes kind is seen. + """ + flat = {} + if isinstance(obj, dict) and obj: + for key in sorted(obj): + if key in VOLATILE_FIELDS: + continue + flat.update(flatten_json(obj[key], f"{prefix}.{key}" if prefix else key)) + elif isinstance(obj, list) and obj: + for index, item in enumerate(obj): + flat.update(flatten_json(item, f"{prefix}[{index}]")) + else: + flat[prefix] = obj + return flat + + +def same_json(a, b): + """Whether two leaves are equal and of the same JSON type: true is not 1, 1.0 is not 1, {} is not [].""" + return type(a) is type(b) and a == b + + +def changed_paths(legacy, new): + """The json paths whose leaf differs between the two bodies.""" + a, b = flatten_json(legacy), flatten_json(new) + return {key for key in set(a) | set(b) if not same_json(a.get(key, _ABSENT), b.get(key, _ABSENT))} + + +def undeclared_paths(changed, intentional_differences): + """The ``changed`` paths that no ``(path, reason)`` entry of ``intentional_differences`` covers.""" + declared = [path for path, _ in intentional_differences] + return { + key for key in changed + if not any(key == path or key.startswith(f"{path}.") or key.startswith(f"{path}[") for path in declared) + } + + +# --------------------------------------------------------------------------- +# Side effects and costs +# --------------------------------------------------------------------------- + +# The one write the permission library makes on a database's first role or +# permission lookup, and the model signals its save sends. +FIRST_LOOKUP_WRITES = [("INSERT", PolicyCacheControl._meta.db_table)] # pylint: disable=protected-access + + +FIRST_LOOKUP_SIGNALS = [("pre_save", "PolicyCacheControl"), ("post_save", "PolicyCacheControl")] + + +# Signals every request sends without writing anything. +NEUTRAL_SIGNALS = ( + core_signals.request_started, + core_signals.request_finished, + model_signals.pre_init, + model_signals.post_init, + crum_signals.current_user_getter, + crum_signals.current_user_setter, +) + + +MODEL_SIGNAL_NAMES = { + model_signals.pre_save: "pre_save", + model_signals.post_save: "post_save", + model_signals.pre_delete: "pre_delete", + model_signals.post_delete: "post_delete", + model_signals.m2m_changed: "m2m_changed", +} + + +@contextmanager +def count_mongo_calls(): + """Count Mongo reads and writes made inside the block.""" + reads = ("find", "find_one", "count_documents", "aggregate") + writes = ("insert_one", "insert_many", "replace_one", "update_one", "update_many", "bulk_write", + "delete_one", "delete_many", "find_one_and_update", "find_one_and_replace", "find_one_and_delete") + counts = {"reads": 0, "writes": 0} + + def counting(kind, original): + def wrapper(self, *args, **kwargs): + counts[kind] += 1 + return original(self, *args, **kwargs) + return wrapper + + collection = pymongo.collection.Collection + with patch.multiple(collection, **{ + name: counting("reads" if name in reads else "writes", getattr(collection, name)) for name in reads + writes + }): + yield counts + + +def sql_writes(queries): + """``(statement, table)`` for each INSERT, UPDATE and DELETE among the captured ``queries``.""" + found = [] + for query in queries.captured_queries: + words = query["sql"].split() + statement = words[0].upper() + if statement in ("INSERT", "DELETE"): + found.append((statement, words[2].strip('"`'))) + elif statement == "UPDATE": + found.append((statement, words[1].strip('"`'))) + return found + + +@contextmanager +def record_effects(test_case): + """ + Record, inside the block, SQL writes, Mongo writes, modulestore updates, + every Django signal other than the neutral ones (as ``(name, sender)``) and + every openedx-event. Commit callbacks registered inside the block are run, + so anything sent on commit is seen. + """ + effects = {"signals": [], "events": [], "update_item": []} + send, send_robust, send_event = Signal.send, Signal.send_robust, OpenEdxPublicSignal.send_event + update_item = MixedModuleStore.update_item + + def record(signal, sender): + if any(signal is neutral for neutral in NEUTRAL_SIGNALS): + return + name = MODEL_SIGNAL_NAMES.get(signal, repr(signal)) + effects["signals"].append((name, getattr(sender, "__name__", repr(sender)))) + + def spy_send(signal, sender, **named): + record(signal, sender) + return send(signal, sender, **named) + + def spy_send_robust(signal, sender, **named): + record(signal, sender) + return send_robust(signal, sender, **named) + + def spy_send_event(signal, *args, **kwargs): + effects["events"].append(signal.event_type) + return send_event(signal, *args, **kwargs) + + def spy_update_item(store, xblock, *args, **kwargs): + effects["update_item"].append(str(xblock.location)) + return update_item(store, xblock, *args, **kwargs) + + with CaptureQueriesContext(connection) as queries, \ + count_mongo_calls() as mongo, \ + patch.object(Signal, "send", spy_send), \ + patch.object(Signal, "send_robust", spy_send_robust), \ + patch.object(OpenEdxPublicSignal, "send_event", spy_send_event), \ + patch.object(MixedModuleStore, "update_item", spy_update_item): + # Innermost, so the callbacks run on its exit while every spy above is installed. + with test_case.captureOnCommitCallbacks(execute=True): + yield effects + effects["sql_writes"] = sql_writes(queries) + effects["mongo_writes"] = mongo["writes"] + + +def measure(request): + """SQL queries, Mongo reads and Mongo writes of the second of two identical requests.""" + request() + with CaptureQueriesContext(connection) as queries, count_mongo_calls() as mongo: + response = request() + assert response.status_code == 200, response.content + return len(queries.captured_queries), mongo["reads"], mongo["writes"] diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py new file mode 100644 index 000000000000..17a31c5fe9ec --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_advanced_settings.py @@ -0,0 +1,1392 @@ +"""Tests for the v1 course advanced settings API.""" + +import json +import uuid +from datetime import UTC, datetime, timedelta +from types import SimpleNamespace +from unittest.mock import patch + +import ddt +import pytest +from django.core.exceptions import ImproperlyConfigured +from django.db import transaction +from django.dispatch import Signal +from django.test import SimpleTestCase, TestCase +from django.test.utils import override_settings +from django.urls import Resolver404, resolve, reverse +from edx_rest_framework_extensions.permissions import NotJwtRestrictedApplication +from edx_rest_framework_extensions.testing import assert_error_envelope +from opaque_keys.edx.keys import CourseKey +from openedx_authz.constants.roles import ( + COURSE_ADMIN, + COURSE_AUDITOR, + COURSE_BETA_TESTER, + COURSE_DATA_RESEARCHER, + COURSE_EDITOR, + COURSE_LIMITED_STAFF, + COURSE_STAFF, +) +from openedx_authz.models.engine import PolicyCacheControl +from rest_framework.permissions import IsAuthenticated +from rest_framework.test import APIClient, APIRequestFactory, force_authenticate +from rest_framework.utils.encoders import JSONEncoder + +from cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service import ( + INVALID_SETTING_MESSAGE, + SETTINGS_NOT_SAVED_MESSAGE, +) +from cms.djangoapps.contentstore.rest_api.v1.views.advanced_settings import CourseAdvancedSettingsViewSet +from cms.djangoapps.contentstore.rest_api.v1.views.tests.helpers import ( + FIRST_LOOKUP_SIGNALS, + FIRST_LOOKUP_WRITES, + UNKNOWN_COURSE_KEY, + VERSION_ONLY_KEY, + api_client, + assert_matches_published_schema, + assert_refused, + changed_paths, + measure, + published_schema, + record_effects, + session_client, + undeclared_paths, +) +from cms.djangoapps.contentstore.tests.utils import routed_actions +from cms.djangoapps.contentstore.views.permissions import ( + CanManageAdvancedSettings, + CanViewAdvancedSettings, + IsExistingCourse, +) +from cms.djangoapps.models.settings.course_metadata import CourseMetadata +from cms.lib.spectacular import LEGACY_MIGRATED_PATH_PREFIXES, SUPERSEDED_PATHS +from common.djangoapps.student.auth import check_course_advanced_settings_access, has_studio_advanced_settings_access +from common.djangoapps.student.models import CourseAccessRole +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, + OrgInstructorRole, + OrgStaffRole, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin +from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user +from openedx.core.djangoapps.oauth_dispatch.tests.factories import AccessTokenFactory, ApplicationFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.mixed import MixedModuleStore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory +from xmodule.tabs import InvalidTabsException + +URL_NAME = "authoring_v1:course_advanced_settings" +LEGACY_URL_NAME = "cms.djangoapps.contentstore:v0:course_advanced_settings" +LEGACY_VIEW = ("cms.djangoapps.contentstore.rest_api.v0.views.advanced_settings", "AdvancedCourseSettingsView") +SERVICE_LOGGER = "cms.djangoapps.contentstore.rest_api.v1.advanced_settings_service" +FULL = {"view": "full"} +LEGACY_DEFAULT = {"fetch_all": "0"} +# An integer setting authors edit, so a non-numeric value is refused by its field. +INTEGER_SETTING = "max_student_enrollments_allowed" +SETTING_MEMBERS = {"value", "display_name", "help", "deprecated", "hide_on_enabled_publisher"} +ACTIONS = ("read", "update") + + +def url(course_key): + """Return the address of the advanced settings of ``course_key``.""" + return reverse(URL_NAME, kwargs={"course_key": str(course_key)}) + + +def legacy_url(course_key): + """Return the deprecated advanced settings address of ``course_key``.""" + return reverse(LEGACY_URL_NAME, kwargs={"course_id": str(course_key)}) + + +def stored_course(course_key): + """The stored course block of ``course_key`` (a key or its string form), or None when there is none.""" + return modulestore().get_course(CourseKey.from_string(str(course_key))) + + +def stored_display_name(course_key): + """The display name stored for the course.""" + return stored_course(course_key).display_name + + +def editable_settings(course_key): + """The settings authors edit, as the settings model returns them for the stored course.""" + return CourseMetadata.fetch(stored_course(course_key)) + + +def every_setting(course_key): + """Every setting of the stored course, as the settings model returns them.""" + return CourseMetadata.fetch_all(stored_course(course_key)) + + +# --------------------------------------------------------------------------- +# The published schema +# --------------------------------------------------------------------------- + + +# --------------------------------------------------------------------------- +# Who may call the endpoint +# --------------------------------------------------------------------------- + +class _AccessMixin: + """ + Checks a caller's status on a read or an update against the expected one, + and that v0 decides the same for the same caller. + """ + + def assert_read(self, user, expected, course_key=None): + """Assert the status of a read by ``user``, its body or envelope, and agreement with v0.""" + course_key = course_key or self.course.id + response = api_client(user).get(url(course_key)) + legacy = api_client(user).get(legacy_url(course_key), LEGACY_DEFAULT) + assert response.status_code == expected, response.content + if expected == 200: + assert list(response.json()) == list(editable_settings(course_key)) + else: + assert_refused(response, expected) + assert legacy.status_code == expected, f"v0 decided {legacy.status_code} for the same caller" + + def assert_update(self, user, expected, course_key=None): + """Assert the status of an update by ``user``, what it stored, and agreement with v0.""" + course_key = course_key or self.course.id + course = stored_course(course_key) + before = course.display_name if course is not None else None + new_name = f"Renamed {uuid.uuid4().hex[:8]}" + response = api_client(user).patch(url(course_key), {"display_name": {"value": new_name}}, format="json") + assert response.status_code == expected, response.content + if expected == 200: + assert response.json()["display_name"]["value"] == new_name + assert stored_display_name(course_key) == new_name + else: + assert_refused(response, expected) + if course is not None: + assert stored_display_name(course_key) == before + legacy_name = f"Legacy {uuid.uuid4().hex[:8]}" + legacy = api_client(user).patch(legacy_url(course_key), {"display_name": {"value": legacy_name}}, format="json") + assert legacy.status_code == expected, f"v0 decided {legacy.status_code} for the same caller" + + def assert_access(self, user, action, expected, course_key=None): + """Assert ``expected`` for ``action`` (``read`` or ``update``) by ``user``.""" + getattr(self, f"assert_{action}")(user, expected, course_key) + + +@ddt.ddt +class AdvancedSettingsAccessLegacyPermissionsTest(_AccessMixin, SharedModuleStoreTestCase): + """Who may read and change the settings when policy-based authoring permissions are off.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + cls.other_course = CourseFactory.create() + + def _user_with(self, role): + user = UserFactory() + role.add_users(user) + return user + + @ddt.data(*ACTIONS) + def test_anonymous_caller_is_refused_with_401(self, action): + self.assert_access(None, action, 401) + + @ddt.data(*ACTIONS) + def test_anonymous_caller_of_an_unknown_course_is_refused_with_401(self, action): + self.assert_access(None, action, 401, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*ACTIONS) + def test_user_without_a_role_is_refused_with_403(self, action): + self.assert_access(UserFactory(), action, 403) + + @ddt.data(*ACTIONS) + def test_unknown_course_is_404_before_permission_is_checked(self, action): + self.assert_access(UserFactory(), action, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(*ACTIONS) + def test_limited_staff_is_refused(self, action): + self.assert_access(self._user_with(CourseLimitedStaffRole(self.course.id)), action, 403) + + @ddt.data(*ACTIONS) + def test_staff_and_instructors_of_another_course_are_refused(self, action): + self.assert_access(self._user_with(CourseStaffRole(self.other_course.id)), action, 403) + self.assert_access(self._user_with(CourseInstructorRole(self.other_course.id)), action, 403) + + @ddt.data(*ACTIONS) + def test_course_staff_is_allowed(self, action): + self.assert_access(self._user_with(CourseStaffRole(self.course.id)), action, 200) + + @ddt.data(*ACTIONS) + def test_course_instructor_is_allowed(self, action): + self.assert_access(self._user_with(CourseInstructorRole(self.course.id)), action, 200) + + @ddt.data(*ACTIONS) + def test_org_staff_and_org_instructor_are_allowed(self, action): + self.assert_access(self._user_with(OrgStaffRole(self.course.id.org)), action, 200) + self.assert_access(self._user_with(OrgInstructorRole(self.course.id.org)), action, 200) + + @ddt.data(*ACTIONS) + def test_global_staff_is_allowed(self, action): + self.assert_access(UserFactory(is_staff=True), action, 200) + + @ddt.data(*ACTIONS) + def test_a_superuser_is_allowed(self, action): + """Saving a superuser also makes it global staff, so it is allowed, as on v0.""" + self.assert_access(UserFactory(is_superuser=True, is_staff=False), action, 200) + + def test_a_version_only_course_key_is_404(self): + """The key parses but names no course run; v0 has no address that takes it.""" + user = UserFactory(is_staff=True) + assert_refused(api_client(user).get(url(VERSION_ONLY_KEY)), 404) + assert_refused( + api_client(user).patch(url(VERSION_ONLY_KEY), {"display_name": {"value": "x"}}, format="json"), 404, + ) + + def test_a_library_is_not_a_course(self): + library_key = LibraryFactory.create().location.library_key + user = UserFactory(is_staff=True) + assert_refused(api_client(user).get(url(library_key)), 404) + assert_refused(api_client(user).patch(url(library_key), {}, format="json"), 404) + + +@ddt.ddt +class AdvancedSettingsAccessAuthzTest(CourseAuthoringAuthzTestMixin, _AccessMixin, SharedModuleStoreTestCase): + """ + Who may read and change the settings when policy-based authoring + permissions are on: viewing needs the permission to view advanced + settings, changing them the permission to manage them. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + cls.other_course = CourseFactory.create() + + def _user_with(self, role, course_key=None): + user = UserFactory() + self.add_user_to_role_in_course(user, role, course_key or self.course.id) + return user + + @ddt.data(*ACTIONS) + def test_anonymous_caller_is_refused_with_401(self, action): + self.assert_access(None, action, 401) + + @ddt.data(*ACTIONS) + def test_user_without_a_role_is_refused_with_403(self, action): + self.assert_access(self.unauthorized_user, action, 403) + + @ddt.data(*ACTIONS) + def test_unknown_course_is_404_before_permission_is_checked(self, action): + self.assert_access(self.unauthorized_user, action, 404, course_key=UNKNOWN_COURSE_KEY) + + @ddt.data(COURSE_AUDITOR.external_key, COURSE_EDITOR.external_key) + def test_roles_that_may_only_view_may_read_but_not_update(self, role): + user = self._user_with(role) + self.assert_read(user, 200) + self.assert_update(user, 403) + + @ddt.data(*[(action, role) for action in ACTIONS for role in ( + COURSE_STAFF.external_key, COURSE_ADMIN.external_key, + )]) + @ddt.unpack + def test_roles_that_may_manage_may_read_and_update(self, action, role): + self.assert_access(self._user_with(role), action, 200) + + @ddt.data(*[(action, role) for action in ACTIONS for role in ( + COURSE_LIMITED_STAFF.external_key, COURSE_DATA_RESEARCHER.external_key, COURSE_BETA_TESTER.external_key, + )]) + @ddt.unpack + def test_other_roles_are_refused(self, action, role): + self.assert_access(self._user_with(role), action, 403) + + @ddt.data(*ACTIONS) + def test_a_role_on_another_course_is_refused(self, action): + user = self._user_with(COURSE_ADMIN.external_key, self.other_course.id) + self.assert_access(user, action, 200, course_key=self.other_course.id) + self.assert_access(user, action, 403) + + @ddt.data(*ACTIONS) + def test_legacy_course_staff_without_a_policy_role_is_refused(self, action): + user = UserFactory() + CourseAccessRole.objects.create(user=user, role="staff", course_id=self.course.id, org=self.course.id.org) + self.assert_access(user, action, 403) + + @ddt.data(*ACTIONS) + def test_staff_and_superuser_are_allowed_without_a_role(self, action): + self.assert_access(self.staff_user, action, 200) + self.assert_access(self.super_user, action, 200) + + def test_an_inactive_session_with_a_role_is_accepted(self): + """The policy does not look at whether the account is activated, on v1 as on v0.""" + user = self._user_with(COURSE_AUDITOR.external_key) + client = session_client(user) + user.is_active = False + user.save() + assert client.get(url(self.course.id)).status_code == 200 + assert client.get(legacy_url(self.course.id)).status_code == 200 + + +@override_settings(DISABLE_ADVANCED_SETTINGS=True) +class AdvancedSettingsSiteRestrictionLegacyPermissionsTest(_AccessMixin, SharedModuleStoreTestCase): + """ + With advanced settings restricted to staff site-wide and policy-based + permissions off, course staff still read and change them here, as on v0. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def setUp(self): + super().setUp() + self.course_staff = UserFactory() + CourseStaffRole(self.course.id).add_users(self.course_staff) + + def test_the_site_restriction_is_on(self): + """The Studio advanced settings page would refuse this caller.""" + assert has_studio_advanced_settings_access(self.course_staff) is False + assert check_course_advanced_settings_access(self.course_staff, self.course.id, "feature_restricted") is False + + def test_course_staff_may_still_read_and_update(self): + self.assert_read(self.course_staff, 200) + self.assert_update(self.course_staff, 200) + + def test_a_caller_without_a_role_is_still_refused(self): + self.assert_read(UserFactory(), 403) + self.assert_update(UserFactory(), 403) + + +@override_settings(DISABLE_ADVANCED_SETTINGS=True) +class AdvancedSettingsSiteRestrictionAuthzTest(CourseAuthoringAuthzTestMixin, _AccessMixin, SharedModuleStoreTestCase): + """ + With advanced settings restricted to staff site-wide and policy-based + permissions on, the policy alone still decides here, as on v0. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def _user_with(self, role): + user = UserFactory() + self.add_user_to_role_in_course(user, role, self.course.id) + return user + + def test_the_site_restriction_is_on(self): + """The restricted check that Studio pages use would refuse a course staff role.""" + user = self._user_with(COURSE_STAFF.external_key) + assert check_course_advanced_settings_access(user, self.course.id, "feature_restricted") is False + + def test_a_role_that_may_manage_may_still_read_and_update(self): + user = self._user_with(COURSE_STAFF.external_key) + self.assert_read(user, 200) + self.assert_update(user, 200) + + def test_a_role_that_may_only_view_still_may_not_update(self): + user = self._user_with(COURSE_EDITOR.external_key) + self.assert_read(user, 200) + self.assert_update(user, 403) + + +class AdvancedSettingsPermissionWiringTest(SimpleTestCase): + """Which permission each action checks, and which access each settings permission asks for.""" + + def test_each_action_checks_the_course_then_its_own_permission(self): + view = CourseAdvancedSettingsViewSet() + for action, expected in ( + ("retrieve", [IsAuthenticated, IsExistingCourse, CanViewAdvancedSettings]), + ("partial_update", [IsAuthenticated, IsExistingCourse, CanManageAdvancedSettings]), + (None, [IsAuthenticated, IsExistingCourse]), + ("metadata", [IsAuthenticated, IsExistingCourse]), + ): + view.action = action + # The JWT middleware appends NotJwtRestrictedApplication to views that take JWTs. + declared = [ + type(permission) for permission in view.get_permissions() + if not isinstance(permission, NotJwtRestrictedApplication) + ] + assert declared == expected, action + + def test_an_action_without_a_declared_permission_is_refused(self): + view = CourseAdvancedSettingsViewSet() + for action in ("destroy", "update", "list", "create"): + view.action = action + with pytest.raises(ImproperlyConfigured): + view.get_permissions() + + def test_a_routed_action_without_a_declared_permission_is_never_served(self): + """A request reaching such an action is answered with a server error, not the handler's body.""" + + class Unchecked(CourseAdvancedSettingsViewSet): + permission_classes = CourseAdvancedSettingsViewSet.permission_classes + + def destroy(self, request, course_key): # pylint: disable=unused-argument + raise AssertionError("the handler ran without a permission check") + + view = Unchecked.as_view({"delete": "destroy"}) + request = APIRequestFactory().delete(url(COURSE_KEY)) + force_authenticate(request, user=SimpleNamespace(is_authenticated=True, is_active=True)) + response = view(request, course_key=CourseKey.from_string(COURSE_KEY)) + assert_error_envelope(response, expected_status=500, expected_type_slug="internal") + + def test_the_settings_permissions_ask_for_read_and_write_access(self): + course_key = "course-v1:edX+Wiring+Run" + request = SimpleNamespace(user=object()) + view = SimpleNamespace(kwargs={"course_key": course_key}) + target = "cms.djangoapps.contentstore.views.permissions.check_course_advanced_settings_access" + for permission, access_type in ((CanViewAdvancedSettings, "read"), (CanManageAdvancedSettings, "write")): + for decision in (True, False): + with patch(target, return_value=decision) as check: + assert permission().has_permission(request, view) is decision + check.assert_called_once_with(request.user, course_key, access_type=access_type) + + def test_the_settings_permissions_refuse_a_view_without_a_course(self): + request = SimpleNamespace(user=object()) + view = SimpleNamespace(kwargs={}) + for permission in (CanViewAdvancedSettings, CanManageAdvancedSettings, IsExistingCourse): + assert permission().has_permission(request, view) is False + + +class AdvancedSettingsAuthenticationTest(SharedModuleStoreTestCase): + """How callers authenticate, with policy-based authoring permissions off.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def _course_staff(self): + user = UserFactory() + CourseStaffRole(self.course.id).add_users(user) + return user + + def test_a_jwt_caller_may_read_and_update(self): + header = {"HTTP_AUTHORIZATION": f"JWT {create_jwt_for_user(self._course_staff())}"} + assert APIClient().get(url(self.course.id), **header).status_code == 200 + response = APIClient().patch( + url(self.course.id), {"display_name": {"value": "By JWT"}}, format="json", **header, + ) + assert response.status_code == 200 + assert stored_display_name(self.course.id) == "By JWT" + + def test_a_session_caller_may_read_and_update(self): + client = session_client(self._course_staff()) + assert client.get(url(self.course.id)).status_code == 200 + response = client.patch(url(self.course.id), {"display_name": {"value": "By session"}}, format="json") + assert response.status_code == 200 + assert stored_display_name(self.course.id) == "By session" + + def test_an_inactive_session_is_authenticated(self): + """ + A session whose account is not activated is accepted, as on v0. Global + staff then pass; course staff are refused (403, not 401) because a + course role does not count while the account is inactive. + """ + for user, expected in ((UserFactory(is_staff=True), 200), (self._course_staff(), 403)): + client = session_client(user) + user.is_active = False + user.save() + response = client.get(url(self.course.id)) + assert response.status_code == expected, response.content + if expected == 403: + assert_refused(response, 403) + assert client.get(legacy_url(self.course.id)).status_code == expected + + def test_a_bearer_token_is_refused(self): + """Bearer tokens are not accepted here, though v0 takes them.""" + user = UserFactory(is_staff=True) + token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token + header = {"HTTP_AUTHORIZATION": f"Bearer {token}"} + assert_refused(APIClient().get(url(self.course.id), **header), 401) + assert APIClient().get(legacy_url(self.course.id), **header).status_code == 200 + + +# --------------------------------------------------------------------------- +# Reads +# --------------------------------------------------------------------------- + +@ddt.ddt +class AdvancedSettingsReadTest(SharedModuleStoreTestCase): + """What a read returns, by view and by named settings.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def setUp(self): + super().setUp() + reader = UserFactory() + CourseStaffRole(self.course.id).add_users(reader) + self.client = api_client(reader) + + def get(self, **params): + response = self.client.get(url(self.course.id), params) + assert response.status_code == 200, response.content + return response.json() + + def test_the_default_view_is_the_settings_authors_edit_in_course_order(self): + body = self.get() + assert list(body) == list(editable_settings(self.course.id)) + assert "display_name" in body + assert "tabs" not in body + + def test_the_full_view_is_every_setting_in_course_order(self): + body = self.get(**FULL) + assert list(body) == list(every_setting(self.course.id)) + assert "tabs" in body + + def test_the_default_view_leaves_out_exactly_the_excluded_settings(self): + default, full = self.get(), self.get(**FULL) + excluded = set(CourseMetadata.get_exclude_list_of_fields(self.course.id)) + assert set(full) - set(default) == set(full) & excluded + assert set(full) - set(default) != set() + + @ddt.data({}, FULL) + def test_every_setting_carries_the_five_members(self, params): + body = self.get(**params) + assert {name: set(setting) for name, setting in body.items()} == {name: SETTING_MEMBERS for name in body} + + @ddt.data({}, FULL) + def test_the_body_matches_the_published_schema(self, params): + assert_matches_published_schema(self.get(**params), "CourseAdvancedSettings.v1") + + def test_the_values_are_those_the_settings_model_reads(self): + body = self.get(**FULL) + assert {name: setting["value"] for name, setting in body.items()} == { + name: json.loads(json.dumps(setting["value"], cls=JSONEncoder)) + for name, setting in every_setting(self.course.id).items() + } + + def test_fields_narrows_to_the_named_settings_in_course_order(self): + named = {"days_early_for_beta", "display_name"} + body = self.get(fields="days_early_for_beta,display_name") + assert list(body) == [name for name in editable_settings(self.course.id) if name in named] + + def test_fields_ignores_names_outside_the_view(self): + assert self.get(fields="tabs,no_such_setting") == {} + assert list(self.get(fields="tabs,no_such_setting", **FULL)) == ["tabs"] + + def test_fields_ignores_spaces_around_names(self): + assert set(self.get(fields=" display_name , days_early_for_beta ")) == {"display_name", "days_early_for_beta"} + + def test_empty_fields_returns_the_whole_view(self): + assert list(self.get(fields="")) == list(editable_settings(self.course.id)) + + def test_the_minimal_view_is_the_default(self): + assert self.get(view="minimal") == self.get() + assert list(self.get(view="minimal")) == list(editable_settings(self.course.id)) + + def test_an_unknown_view_is_refused(self): + response = self.client.get(url(self.course.id), {"view": "default"}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"view": ['"default" is not a valid choice.']} + + def test_the_v0_query_parameters_are_ignored(self): + """fetch_all and filter_fields have no meaning here; view and fields replace them.""" + for params in ({}, FULL): + expected = self.get(**params) + assert self.get(**params, fetch_all="0", filter_fields="display_name") == expected + assert self.get(**params, fetch_all="1") == expected + + def test_head_answers_without_a_body(self): + for params in ({}, FULL): + response = self.client.head(url(self.course.id), params) + assert response.status_code == 200 + assert response.content == b"" + + +# --------------------------------------------------------------------------- +# Updates +# --------------------------------------------------------------------------- + +class AdvancedSettingsUpdateTest(ModuleStoreTestCase): + """What an update stores, returns and refuses.""" + + def setUp(self): + super().setUp() + self.course = CourseFactory.create(display_name="Original") + writer = UserFactory() + CourseStaffRole(self.course.id).add_users(writer) + self.writer = writer + self.client = api_client(writer) + + def patch(self, body): + return self.client.patch(url(self.course.id), body, format="json") + + def test_named_settings_are_stored_and_the_editable_settings_returned(self): + response = self.patch({"display_name": {"value": "Renamed"}, "days_early_for_beta": {"value": 3.0}}) + assert response.status_code == 200, response.content + course = modulestore().get_course(self.course.id) + assert (course.display_name, course.days_early_for_beta) == ("Renamed", 3.0) + body = response.json() + assert list(body) == list(editable_settings(self.course.id)) + assert body["display_name"]["value"] == "Renamed" + assert body["days_early_for_beta"]["value"] == 3.0 + assert_matches_published_schema(body, "CourseAdvancedSettings.v1") + + def test_members_other_than_value_are_ignored(self): + response = self.patch({"display_name": {"value": "Renamed", "display_name": "Not stored", "deprecated": True}}) + assert response.status_code == 200, response.content + setting = response.json()["display_name"] + assert (setting["value"], setting["deprecated"]) == ("Renamed", False) + assert setting["display_name"] == str(editable_settings(self.course.id)["display_name"]["display_name"]) + + def test_unknown_settings_and_settings_authors_do_not_edit_are_ignored(self): + start = modulestore().get_course(self.course.id).start + response = self.patch({"no_such_setting": {"value": 1}, "start": {"value": "2030-01-01T00:00:00Z"}}) + assert response.status_code == 200, response.content + assert "no_such_setting" not in response.json() + assert "start" not in response.json() + assert modulestore().get_course(self.course.id).start == start + + def test_a_null_value_is_stored(self): + self.patch({"days_early_for_beta": {"value": 3.0}}) + response = self.patch({"days_early_for_beta": {"value": None}}) + assert response.status_code == 200, response.content + assert modulestore().get_course(self.course.id).days_early_for_beta is None + + def test_a_refused_value_is_reported_with_a_fixed_message_and_nothing_is_stored(self): + body = {INTEGER_SETTING: {"value": "lots"}, "display_name": {"value": "Not stored"}} + with self.assertLogs(SERVICE_LOGGER, "WARNING") as logs: + response = self.patch(body) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {INTEGER_SETTING: [INVALID_SETTING_MESSAGE]} + assert stored_display_name(self.course.id) == "Original" + legacy = self.client.patch(legacy_url(self.course.id), body, format="json") + assert legacy.status_code == 400 + (validator_text,) = [ + entry["message"] for entry in legacy.json()["developer_message"] if entry["key"] == INTEGER_SETTING + ] + assert validator_text != INVALID_SETTING_MESSAGE + assert validator_text not in response.content.decode() + (record,) = logs.output + assert validator_text in record + assert INTEGER_SETTING in record + assert str(self.course.id) in record + + def test_several_refused_settings_are_each_named(self): + body = {INTEGER_SETTING: {"value": "lots"}, "days_early_for_beta": {"value": "soon"}} + with self.assertLogs(SERVICE_LOGGER, "WARNING") as logs: + response = self.patch(body) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == { + INTEGER_SETTING: [INVALID_SETTING_MESSAGE], "days_early_for_beta": [INVALID_SETTING_MESSAGE], + } + assert len(logs.output) == 2 + + def test_a_body_that_is_not_an_object_is_refused(self): + envelope = assert_error_envelope(self.patch([1, 2]), expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": ["Invalid data. Expected a dictionary, but got list."]} + + def test_each_entry_must_be_an_object_with_a_value(self): + response = self.patch({"display_name": "Renamed", "days_early_for_beta": {}}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == { + "display_name": ["Invalid data. Expected a dictionary, but got str."], + "days_early_for_beta": ["This field is required."], + } + assert stored_display_name(self.course.id) == "Original" + + def test_an_entry_that_is_not_an_object_is_refused_by_v0_too(self): + body = {"display_name": "Renamed"} + assert self.patch(body).status_code == 400 + assert self.client.patch(legacy_url(self.course.id), body, format="json").status_code == 400 + assert stored_display_name(self.course.id) == "Original" + + def test_a_malformed_entry_of_a_setting_authors_do_not_edit_is_ignored(self): + start = stored_course(self.course.id).start + response = self.patch({"tabs": "not tabs", "start": {}, "display_name": {"value": "Renamed"}}) + assert response.status_code == 200, response.content + assert stored_display_name(self.course.id) == "Renamed" + assert stored_course(self.course.id).start == start + + def test_a_failure_saving_the_tabs_is_reported_without_its_text(self): + target = "cms.djangoapps.contentstore.views.course._refresh_course_tabs" + with ( + patch(target, side_effect=InvalidTabsException("tab internals")), + self.assertLogs(SERVICE_LOGGER, "WARNING"), + ): + response = self.patch({"display_name": {"value": "Not stored"}}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": [SETTINGS_NOT_SAVED_MESSAGE]} + assert b"tab internals" not in response.content + assert b"Tabs Exception" not in response.content + assert stored_display_name(self.course.id) == "Original" + with patch(target, side_effect=InvalidTabsException("tab internals")): + legacy = self.client.patch(legacy_url(self.course.id), {"display_name": {"value": "x"}}, format="json") + assert legacy.status_code == 400 + + def test_an_unexpected_validation_failure_is_reported_without_its_text(self): + target = "cms.djangoapps.models.settings.course_metadata.CourseMetadata.validate_and_update_from_json" + with patch(target, side_effect=TypeError("validator internals")), self.assertLogs(SERVICE_LOGGER, "WARNING"): + response = self.patch({"display_name": {"value": "Not stored"}}) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": [SETTINGS_NOT_SAVED_MESSAGE]} + assert b"validator internals" not in response.content + assert stored_display_name(self.course.id) == "Original" + with patch(target, side_effect=TypeError("validator internals")): + legacy = self.client.patch(legacy_url(self.course.id), {"display_name": {"value": "x"}}, format="json") + assert legacy.status_code == 400 + assert b"validator internals" in legacy.content + + +# Requests v0 and v1 answer differently beyond the error body, as +# (request, v0 answer, v1 answer, test in AdvancedSettingsResponseDifferencesTest). +RESPONSE_DIFFERENCES = [ + ("read or update of a course the modulestore returns nothing for", "500", "404", + "test_a_course_the_modulestore_returns_nothing_for"), + ("update whose body is not an object", "500", "400", "test_an_update_whose_body_is_not_an_object"), + ("update entry without a value", "500", "400", "test_an_update_entry_without_a_value"), + ("read with view=default", "200", "400", "test_an_unknown_view"), + ("read or update with a Bearer token", "200", "401", "test_a_bearer_token"), + ("slash-separated course key", "404 JSON", "404 HTML", "test_a_slash_separated_course_key"), +] + + +class AdvancedSettingsResponseDifferencesTest(ModuleStoreTestCase): + """Each request v0 and v1 answer differently, both answers observed.""" + + def setUp(self): + super().setUp() + self.course = CourseFactory.create(display_name="Original") + writer = UserFactory() + CourseStaffRole(self.course.id).add_users(writer) + self.client = api_client(writer) + + def both(self, method, body=None, **params): + """The v0 and v1 responses to the same request.""" + call = getattr(self.client, method) + kwargs = {"format": "json"} if body is not None else {} + data = body if body is not None else params + return call(legacy_url(self.course.id), data, **kwargs), call(url(self.course.id), data, **kwargs) + + def test_every_listed_difference_has_its_test(self): + assert [name for *_, name in RESPONSE_DIFFERENCES if not hasattr(self, name)] == [] + + def test_a_course_the_modulestore_returns_nothing_for(self): + with patch.object(MixedModuleStore, "get_course", return_value=None): + for method, body in (("get", None), ("patch", {"display_name": {"value": "x"}})): + legacy, new = self.both(method, body) + assert legacy.status_code == 500, method + assert_refused(new, 404) + + def test_an_update_whose_body_is_not_an_object(self): + legacy, new = self.both("patch", [1, 2]) + assert legacy.status_code == 500 + envelope = assert_error_envelope(new, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": ["Invalid data. Expected a dictionary, but got list."]} + + def test_an_update_entry_without_a_value(self): + legacy, new = self.both("patch", {"display_name": {}}) + assert legacy.status_code == 500 + envelope = assert_error_envelope(new, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"display_name": ["This field is required."]} + assert stored_display_name(self.course.id) == "Original" + + def test_an_unknown_view(self): + legacy, new = self.both("get", view="default") + assert legacy.status_code == 200 + assert_error_envelope(new, expected_status=400, expected_type_slug="validation") + + def test_a_bearer_token(self): + user = UserFactory(is_staff=True) + token = AccessTokenFactory(user=user, application=ApplicationFactory(user=user)).token + header = {"HTTP_AUTHORIZATION": f"Bearer {token}"} + assert APIClient().get(legacy_url(self.course.id), **header).status_code == 200 + assert_refused(APIClient().get(url(self.course.id), **header), 401) + body = {"display_name": {"value": "By Bearer"}} + assert APIClient().patch(legacy_url(self.course.id), body, format="json", **header).status_code == 200 + assert_refused(APIClient().patch(url(self.course.id), body, format="json", **header), 401) + + def test_a_slash_separated_course_key(self): + """v0 routes the slash form and reports the course unknown in JSON; v1 has no route for it.""" + legacy = self.client.get("/api/contentstore/v0/advanced_settings/edX/NoSuch/Course") + new = self.client.get("/api/authoring/v1/courses/edX/NoSuch/Course/advanced_settings/") + assert legacy.status_code == new.status_code == 404 + assert legacy.json()["error_code"] == "course_does_not_exist" + assert new["Content-Type"].startswith("text/html") + + +@ddt.ddt +@override_settings( + ENABLE_PROCTORED_EXAMS=True, + PROCTORING_BACKENDS={"DEFAULT": "test_proctoring_provider", "valid_provider": {}}, + PARTNER_SUPPORT_EMAIL="support@example.com", +) +class AdvancedSettingsProctoringProviderTest(ModuleStoreTestCase): + """ + Once a course has started, only global staff may change its proctoring + provider, on v1 as on v0. Before it starts, course staff may. + """ + + CHANGE = {"proctoring_provider": {"value": "valid_provider"}} + + def _course(self, started): + offset = timedelta(days=-1 if started else 30) + return CourseFactory.create(start=datetime.now(UTC) + offset) + + def _course_staff(self, course): + user = UserFactory() + CourseStaffRole(course.id).add_users(user) + return user + + def _patch(self, version, user, course): + address = url(course.id) if version == "v1" else legacy_url(course.id) + return api_client(user).patch(address, self.CHANGE, format="json") + + @ddt.data("v1", "v0") + def test_course_staff_may_not_change_it_after_the_course_starts(self, version): + course = self._course(started=True) + before = stored_course(course.id).proctoring_provider + assert before != "valid_provider" + response = self._patch(version, self._course_staff(course), course) + assert response.status_code == 400, response.content + if version == "v1": + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert list(envelope["errors"]) == ["proctoring_provider"] + else: + assert [entry["key"] for entry in response.json()["developer_message"]] == ["proctoring_provider"] + assert stored_course(course.id).proctoring_provider == before + + @ddt.data("v1", "v0") + def test_global_staff_may_change_it_after_the_course_starts(self, version): + course = self._course(started=True) + response = self._patch(version, UserFactory(is_staff=True), course) + assert response.status_code == 200, response.content + assert response.json()["proctoring_provider"]["value"] == "valid_provider" + assert stored_course(course.id).proctoring_provider == "valid_provider" + + @ddt.data("v1", "v0") + def test_course_staff_may_change_it_before_the_course_starts(self, version): + course = self._course(started=False) + response = self._patch(version, self._course_staff(course), course) + assert response.status_code == 200, response.content + assert stored_course(course.id).proctoring_provider == "valid_provider" + + +# --------------------------------------------------------------------------- +# Parity with v0 +# --------------------------------------------------------------------------- + +# Members of a response body that differ between v0 and v1 for the same +# request. The comparison asserts nothing else differs, and that each of these +# occurs in the error cases below. +INTENTIONAL_DIFFERENCES = [ + ("type", "errors use the standardized envelope"), + ("title", "errors use the standardized envelope"), + ("status", "errors use the standardized envelope"), + ("detail", "errors use the standardized envelope"), + ("errors", "a refused update maps the name of each refused setting to INVALID_SETTING_MESSAGE, where v0 " + "lists the validator's own text under developer_message"), + ("developer_message", "the v0 error body carries developer_message; the envelope does not"), + ("error_code", "the v0 not-found body carries error_code; the envelope does not"), +] + +# Reads of the two addresses that return the same settings. Their parameters +# differ by design: the default view is v0's fetch_all=0 response, view=full +# is v0's default, and fields replaces filter_fields. +EQUIVALENT_READS = [ + ({}, LEGACY_DEFAULT), + (FULL, {}), + ( + {"fields": "display_name,days_early_for_beta"}, + {**LEGACY_DEFAULT, "filter_fields": "display_name,days_early_for_beta"}, + ), + ({**FULL, "fields": "tabs,display_name"}, {"filter_fields": "tabs,display_name"}), +] + + +def _undeclared(changed): + """The changed paths no declared intentional difference covers.""" + return undeclared_paths(changed, INTENTIONAL_DIFFERENCES) + + +def test_the_comparison_sees_types_order_free_members_and_empty_containers(): + assert changed_paths({"a": {"b": []}}, {"a": {}}) == {"a", "a.b"} + assert changed_paths({"x": True}, {"x": 1}) == {"x"} + assert changed_paths({"x": 1.0}, {"x": 1}) == {"x"} + assert changed_paths({"x": {}}, {"x": []}) == {"x"} + assert changed_paths({"x": None, "y": 1}, {"y": 1}) == {"x"} + assert changed_paths({"instance": "/a"}, {"instance": "/b"}) == set() + assert changed_paths({"x": [1, {"y": False}]}, {"x": [1, {"y": False}]}) == set() + + +def test_only_declared_differences_are_tolerated(): + assert _undeclared({"type", "errors.display_name[0]", "developer_message[0].key"}) == set() + assert _undeclared({"display_name.value", "typed"}) == {"display_name.value", "typed"} + + +class _ParityMixin: + """ + v1 against v0 for the same course and caller: reads and updates return + identical bodies, and errors differ only by the declared differences. + """ + + def reader_for(self, course): + """Return a user who may read and change the course's settings.""" + raise NotImplementedError + + def outsider(self): + """Return a signed-in user with no access to the course.""" + raise NotImplementedError + + def setUp(self): + super().setUp() + self.course = CourseFactory.create(display_name="Parity") + self.reader = self.reader_for(self.course) + self.observed = set() + + def compare(self, legacy, new, expected_status): + """Assert both statuses and that the bodies differ only as declared; return both bodies.""" + assert legacy.status_code == expected_status, legacy.content + assert new.status_code == expected_status, new.content + legacy_body, new_body = json.loads(legacy.content or b"null"), json.loads(new.content or b"null") + changed = changed_paths(legacy_body, new_body) + assert _undeclared(changed) == set(), f"undeclared differences: legacy={legacy_body} new={new_body}" + self.observed |= changed + return legacy_body, new_body + + def test_equivalent_reads_return_identical_bodies(self): + client = api_client(self.reader) + for new_params, legacy_params in EQUIVALENT_READS: + legacy_body, new_body = self.compare( + client.get(legacy_url(self.course.id), legacy_params), + client.get(url(self.course.id), new_params), + 200, + ) + assert changed_paths(legacy_body, new_body) == set(), (new_params, legacy_params) + assert list(new_body) == list(legacy_body) + assert new_body != {} + + def test_the_same_parameters_return_different_settings(self): + """The default view is not v0's default: the difference is exactly the excluded settings.""" + client = api_client(self.reader) + legacy_body = client.get(legacy_url(self.course.id)).json() + new_body = client.get(url(self.course.id)).json() + assert set(legacy_body) - set(new_body) == set(every_setting(self.course.id)) - set( + editable_settings(self.course.id) + ) + assert set(new_body) < set(legacy_body) + assert {name: legacy_body[name] for name in new_body} == new_body + + def test_an_update_returns_an_identical_body(self): + body = {"display_name": {"value": "Changed by both"}, "days_early_for_beta": {"value": 2.0}} + client = api_client(self.reader) + new = client.patch(url(self.course.id), body, format="json") + legacy = client.patch(legacy_url(self.course.id), body, format="json") + legacy_body, new_body = self.compare(legacy, new, 200) + assert changed_paths(legacy_body, new_body) == set() + assert new_body["display_name"]["value"] == "Changed by both" + + def _error_pairs(self): + """``(legacy, new, status)`` for each error both addresses produce.""" + refused = {INTEGER_SETTING: {"value": "lots"}} + cases = [ + (api_client(), self.course.id, "get", None, 401), + (api_client(self.outsider()), self.course.id, "get", None, 403), + (api_client(self.outsider()), UNKNOWN_COURSE_KEY, "get", None, 404), + (api_client(self.reader), self.course.id, "patch", refused, 400), + ] + for client, course_key, method, body, status in cases: + call = getattr(client, method) + kwargs = {"format": "json"} if body is not None else {} + legacy = call(legacy_url(course_key), body, **kwargs) + new = call(url(course_key), body, **kwargs) + yield legacy, new, status + + def test_errors_differ_only_by_the_envelope(self): + for legacy, new, status in self._error_pairs(): + self.compare(legacy, new, status) + + def test_a_refused_update_names_the_settings_v0_names_with_a_fixed_message(self): + *_, (legacy, new, status) = self._error_pairs() + legacy_body, new_body = self.compare(legacy, new, status) + validator_texts = {entry["key"]: entry["message"] for entry in legacy_body["developer_message"]} + assert new_body["errors"] == {name: [INVALID_SETTING_MESSAGE] for name in validator_texts} + assert INVALID_SETTING_MESSAGE not in validator_texts.values() + assert [text for text in validator_texts.values() if text in new.content.decode()] == [] + + def test_a_malformed_entry_of_a_setting_authors_do_not_edit_is_ignored_by_both(self): + client = api_client(self.reader) + for body in ({"tabs": "not tabs"}, {"start": {}}, {"tabs": "not tabs", "display_name": {"value": "Both"}}): + new = client.patch(url(self.course.id), body, format="json") + legacy = client.patch(legacy_url(self.course.id), body, format="json") + legacy_body, new_body = self.compare(legacy, new, 200) + assert changed_paths(legacy_body, new_body) == set(), body + + def test_every_declared_difference_occurs(self): + for legacy, new, status in self._error_pairs(): + self.compare(legacy, new, status) + missing = [ + path for path, _ in INTENTIONAL_DIFFERENCES + if not any(key == path or key.startswith(f"{path}.") or key.startswith(f"{path}[") for key in self.observed) + ] + assert missing == [] + + +class AdvancedSettingsParityLegacyPermissionsTest(_ParityMixin, ModuleStoreTestCase): + """Parity read and written by course staff, with policy-based authoring permissions off.""" + + def reader_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + return reader + + def outsider(self): + return UserFactory() + + +class AdvancedSettingsParityAuthzTest(_ParityMixin, CourseAuthoringAuthzTestMixin, ModuleStoreTestCase): + """Parity read and written by a course staff role, with policy-based authoring permissions on.""" + + def reader_for(self, course): + reader = UserFactory() + self.add_user_to_role_in_course(reader, COURSE_STAFF.external_key, course.id) + return reader + + def outsider(self): + return self.unauthorized_user + + +# --------------------------------------------------------------------------- +# Side effects of reads +# --------------------------------------------------------------------------- + + +READS = [("get", {}), ("get", FULL), ("get", {"fields": "display_name"}), ("head", {}), ("head", FULL)] + + +class _ReadsAreSideEffectFreeMixin: + """ + Every read (default, full, named settings, and HEAD on both views) writes + no row and no Mongo document, stores nothing through the modulestore and + sends no signal or event. The reads start on a database that has never + served a role or permission lookup, so the first read may insert the + permission library's one policy version row and nothing else; v0 inserts + the same row. + """ + + def reader_for(self, course): + """Return a user who may read ``course``, leaving the database as no lookup had touched it.""" + raise NotImplementedError + + def setUp(self): + super().setUp() + self.course = CourseFactory.create() + + def test_reads_have_no_side_effects(self): + client = api_client(self.reader_for(self.course)) + assert not PolicyCacheControl.objects.exists() + for index, (method, params) in enumerate(READS): + with record_effects(self) as effects: + response = getattr(client, method)(url(self.course.id), params) + read = (index, method, params) + first = index == 0 + assert response.status_code == 200, (read, response.content) + assert effects["sql_writes"] == (FIRST_LOOKUP_WRITES if first else []), read + assert effects["signals"] == (FIRST_LOOKUP_SIGNALS if first else []), read + assert effects["events"] == [], read + assert effects["mongo_writes"] == 0, read + assert effects["update_item"] == [], read + + def test_the_v0_read_makes_the_same_first_lookup_write(self): + client = api_client(self.reader_for(self.course)) + with record_effects(self) as effects: + response = client.get(legacy_url(self.course.id), LEGACY_DEFAULT) + assert response.status_code == 200, response.content + assert effects["sql_writes"] == FIRST_LOOKUP_WRITES + assert effects["signals"] == FIRST_LOOKUP_SIGNALS + assert effects["mongo_writes"] == 0 + + def test_the_recorder_sees_writes_signals_and_modulestore_updates(self): + """The checks above would notice a write: one made inside the recorder is seen.""" + with record_effects(self) as effects: + UserFactory() + course = modulestore().get_course(self.course.id) + modulestore().update_item(course, self.user.id) + assert ("INSERT", "auth_user") in effects["sql_writes"] + assert ("post_save", "User") in effects["signals"] + assert effects["mongo_writes"] > 0 + assert effects["update_item"] == [str(course.location)] + + def test_the_recorder_sees_what_commit_callbacks_do(self): + """Work deferred to the end of the transaction is recorded too.""" + probe = Signal() + + class Sender: + pass + + with record_effects(self) as effects: + transaction.on_commit(lambda: probe.send(sender=Sender)) + transaction.on_commit(lambda: UserFactory(username="made_on_commit")) + assert (repr(probe), "Sender") in effects["signals"] + assert ("INSERT", "auth_user") in effects["sql_writes"] + assert ("post_save", "User") in effects["signals"] + + +class AdvancedSettingsReadSideEffectsLegacyPermissionsTest(_ReadsAreSideEffectFreeMixin, ModuleStoreTestCase): + """Reads by course staff with policy-based authoring permissions off.""" + + def reader_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + PolicyCacheControl.objects.all().delete() + return reader + + +class AdvancedSettingsReadSideEffectsAuthzTest( + _ReadsAreSideEffectFreeMixin, CourseAuthoringAuthzTestMixin, ModuleStoreTestCase, +): + """ + Reads by a course auditor with policy-based authoring permissions on. The + role is assigned through the permission library, which creates the policy + version row; it is deleted before the reads. + """ + + def reader_for(self, course): + reader = UserFactory() + self.add_user_to_role_in_course(reader, COURSE_AUDITOR.external_key, course.id) + PolicyCacheControl.objects.all().delete() + return reader + + +# --------------------------------------------------------------------------- +# Query counts +# --------------------------------------------------------------------------- + + +class _QueryCountMixin: + """Each read costs no more SQL queries or Mongo reads than the v0 read returning the same settings.""" + + def reader_for(self, course): + raise NotImplementedError + + def test_reads_cost_no_more_than_the_equivalent_v0_reads(self): + course = CourseFactory.create() + client = api_client(self.reader_for(course)) + for new_params, legacy_params in EQUIVALENT_READS[:2]: + legacy = measure(lambda params=legacy_params: client.get(legacy_url(course.id), params)) + new = measure(lambda params=new_params: client.get(url(course.id), params)) + assert new[0] <= legacy[0], (new_params, new, legacy) + assert new[1] <= legacy[1], (new_params, new, legacy) + assert new[2] == legacy[2] == 0, (new_params, new, legacy) + + +class AdvancedSettingsQueryCountLegacyPermissionsTest(_QueryCountMixin, ModuleStoreTestCase): + """Costs read by course staff, with policy-based authoring permissions off.""" + + def reader_for(self, course): + reader = UserFactory() + CourseStaffRole(course.id).add_users(reader) + return reader + + +class AdvancedSettingsQueryCountAuthzTest(CourseAuthoringAuthzTestMixin, _QueryCountMixin, ModuleStoreTestCase): + """Costs read by a course auditor, with policy-based authoring permissions on.""" + + def reader_for(self, course): + reader = UserFactory() + self.add_user_to_role_in_course(reader, COURSE_AUDITOR.external_key, course.id) + return reader + + +# --------------------------------------------------------------------------- +# Addresses and methods +# --------------------------------------------------------------------------- + +COURSE_KEY = "course-v1:edX+DemoX+Demo_Course" + + +@ddt.ddt +class AdvancedSettingsRoutingTest(SharedModuleStoreTestCase): + """The address, the v0 address, and requests no route or method serves.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def test_the_address(self): + assert url(COURSE_KEY) == f"/api/authoring/v1/courses/{COURSE_KEY}/advanced_settings/" + match = resolve(url(COURSE_KEY)) + assert match.func.cls is CourseAdvancedSettingsViewSet + assert routed_actions(match.func) == {"get": "retrieve", "patch": "partial_update"} + + def test_the_v0_address_is_unchanged(self): + assert legacy_url(COURSE_KEY) == f"/api/contentstore/v0/advanced_settings/{COURSE_KEY}" + view = resolve(legacy_url(COURSE_KEY)).func.cls + assert (view.__module__, view.__name__) == LEGACY_VIEW + + def test_v0_still_takes_a_slash_separated_key_that_v1_does_not(self): + view = resolve("/api/contentstore/v0/advanced_settings/edX/DemoX/Demo_Course").func.cls + assert (view.__module__, view.__name__) == LEGACY_VIEW + with pytest.raises(Resolver404): + resolve("/api/authoring/v1/courses/edX/DemoX/Demo_Course/advanced_settings/") + + def test_the_url_names_are_unique_in_the_namespace(self): + # Imported here: at module import the URL converters are not registered yet. + from cms.djangoapps.contentstore.rest_api.v1 import authoring_urls # noqa: PLC0415 + names = [pattern.name for pattern in authoring_urls.urlpatterns] + assert len(names) == len(set(names)) + assert "course_advanced_settings" in names + + # The error library does not catalogue MethodNotAllowed or NotAcceptable, so + # these answer with the envelope typed internal. Change the expected type + # here once it does. + + @ddt.data("post", "put", "delete") + def test_other_methods_are_not_allowed(self, method): + response = getattr(api_client(UserFactory(is_staff=True)), method)(url(self.course.id), {}, format="json") + assert_error_envelope(response, expected_status=405, expected_type_slug="internal") + + def test_unsupported_accept_is_refused_with_406(self): + response = api_client(UserFactory(is_staff=True)).get(url(self.course.id), HTTP_ACCEPT="application/xml") + assert_error_envelope(response, expected_status=406, expected_type_slug="internal") + + # Nor ParseError or UnsupportedMediaType: a malformed or unsupported body is + # answered with its own status but typed internal. Change the expected type + # here once the library catalogues them. + + def test_a_malformed_json_body_is_a_400_typed_internal(self): + response = api_client(UserFactory(is_staff=True)).patch( + url(self.course.id), data="{", content_type="application/json", + ) + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="internal") + assert envelope["title"] == "Internal Server Error" + + def test_an_unsupported_media_type_is_a_415_typed_internal(self): + response = api_client(UserFactory(is_staff=True)).patch( + url(self.course.id), data="display_name=Renamed", content_type="text/plain", + ) + envelope = assert_error_envelope(response, expected_status=415, expected_type_slug="internal") + assert envelope["title"] == "Internal Server Error" + assert stored_display_name(self.course.id) != "Renamed" + + # These addresses match no route, so the service's HTML 404 page answers + # them. Change these to the error envelope once a catch-all route under the + # mount answers unknown addresses. + + @ddt.data("not-a-course-key", "edX/DemoX/Demo_Course", "edX+DemoX+Demo_Course") + def test_an_unparsable_or_slash_form_key_is_an_html_404(self, key): + response = api_client(UserFactory(is_staff=True)).get(f"/api/authoring/v1/courses/{key}/advanced_settings/") + assert response.status_code == 404 + assert response["Content-Type"].startswith("text/html") + + def test_the_address_without_its_trailing_slash_redirects_to_it(self): + """No route answers the address without its slash; the service redirects a read to the slashed one.""" + address = f"/api/authoring/v1/courses/{self.course.id}/advanced_settings" + response = api_client(UserFactory(is_staff=True)).get(address) + assert response.status_code == 301 + assert response["Location"] == url(self.course.id) + with pytest.raises(Resolver404): + resolve(address) + + +# --------------------------------------------------------------------------- +# The published schema +# --------------------------------------------------------------------------- + +class AdvancedSettingsSchemaTest(TestCase): + """The read and the update as published in the CMS authoring schema.""" + + PATH = "/api/authoring/v1/courses/{course_key}/advanced_settings/" + LEGACY = "/api/contentstore/v0/advanced_settings/{course_id}" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.schema = published_schema() + cls.components = cls.schema["components"]["schemas"] + + @staticmethod + def _ref(node): + return node["content"]["application/json"]["schema"]["$ref"].rsplit("/", 1)[-1] + + def test_the_read_and_the_update_are_published_for_the_sdk(self): + operations = self.schema["paths"][self.PATH] + assert sorted(operations) == ["get", "patch"] + assert operations["get"]["operationId"] == "v1_courses_advanced_settings_retrieve" + assert operations["patch"]["operationId"] == "v1_courses_advanced_settings_partial_update" + for operation in operations.values(): + assert operation["tags"] == ["openedx-platform-sdk"] + assert "deprecated" not in operation + assert self._ref(operation["responses"]["200"]) == "CourseAdvancedSettings.v1" + for status in ("400", "401", "403", "404"): + assert self._ref(operation["responses"][status]) == "ErrorResponse", status + + def test_the_read_parameters(self): + parameters = {p["name"]: p for p in self.schema["paths"][self.PATH]["get"]["parameters"]} + assert set(parameters) == {"course_key", "view", "fields"} + assert parameters["course_key"]["in"] == "path" + assert parameters["course_key"]["schema"] == {"type": "string", "pattern": "^[^/]+$"} + assert parameters["view"]["in"] == parameters["fields"]["in"] == "query" + assert not parameters["view"].get("required") + assert not parameters["fields"].get("required") + assert parameters["view"]["schema"] == {"type": "string", "enum": ["full", "minimal"], "default": "minimal"} + assert parameters["fields"]["schema"] == {"type": "string"} + + def test_the_settings_are_an_object_keyed_by_setting_name(self): + settings = self.components["CourseAdvancedSettings.v1"] + assert settings["type"] == "object" + assert settings["additionalProperties"] == {"$ref": "#/components/schemas/AdvancedSetting.v1"} + assert "properties" not in settings + setting = self.components["AdvancedSetting.v1"] + assert set(setting["properties"]) == SETTING_MEMBERS + assert set(setting["required"]) == SETTING_MEMBERS + nullable = {name for name, field in setting["properties"].items() if field.get("nullable")} + assert nullable == {"value", "help"} + + def test_the_update_body_is_json_settings_each_with_a_value(self): + body = self.schema["paths"][self.PATH]["patch"]["requestBody"] + assert list(body["content"]) == ["application/json"] + assert self._ref(body) == "PatchedCourseAdvancedSettingsUpdate.v1" + update = self.components[self._ref(body)] + assert update["type"] == "object" + assert update["additionalProperties"] == {"$ref": "#/components/schemas/AdvancedSettingUpdate.v1"} + entry = self.components["AdvancedSettingUpdate.v1"] + assert set(entry["properties"]) == {"value"} + assert entry["required"] == ["value"] + assert entry["properties"]["value"]["nullable"] is True + + def test_every_member_is_described(self): + for name in ("CourseAdvancedSettings.v1", "AdvancedSetting.v1", "AdvancedSettingUpdate.v1"): + component = self.components[name] + assert component.get("description"), name + properties = component.get("properties", {}) + assert [field for field, spec in properties.items() if not spec.get("description")] == [], name + + def test_the_examples_are_valid_bodies(self): + get = self.schema["paths"][self.PATH]["get"] + patch_ = self.schema["paths"][self.PATH]["patch"] + response_example = get["responses"]["200"]["content"]["application/json"]["examples"]["AdvancedSettings"] + assert_matches_published_schema(response_example["value"], "CourseAdvancedSettings.v1") + request_example = patch_["requestBody"]["content"]["application/json"]["examples"]["NewValues"] + assert_matches_published_schema(request_example["value"], self._ref(patch_["requestBody"])) + + def test_the_v0_operations_are_deprecated_and_left_out_of_the_sdk(self): + operations = self.schema["paths"][self.LEGACY] + assert sorted(operations) == ["get", "patch"] + assert operations["get"]["operationId"] == "v0_advanced_settings_retrieve" + assert operations["patch"]["operationId"] == "v0_advanced_settings_partial_update" + for operation in operations.values(): + assert operation["deprecated"] is True + assert "openedx-platform-sdk" not in operation.get("tags", []) + + def test_only_superseded_or_migrated_addresses_are_deprecated(self): + """No operation is marked deprecated except by the legacy-address lists, so a stray mark is caught.""" + deprecated = { + (path, method) + for path, operations in self.schema["paths"].items() + for method, operation in operations.items() + if isinstance(operation, dict) and operation.get("deprecated") + } + assert {(self.LEGACY, "get"), (self.LEGACY, "patch")} <= deprecated + assert {(self.PATH, "get"), (self.PATH, "patch")} & deprecated == set() + stray = sorted( + (path, method) for path, method in deprecated + if path not in SUPERSEDED_PATHS and not path.startswith(LEGACY_MIGRATED_PATH_PREFIXES) + ) + assert stray == [] diff --git a/cms/djangoapps/contentstore/views/permissions.py b/cms/djangoapps/contentstore/views/permissions.py index 6450a3bfcc3e..3701d7149481 100644 --- a/cms/djangoapps/contentstore/views/permissions.py +++ b/cms/djangoapps/contentstore/views/permissions.py @@ -3,13 +3,21 @@ """ from openedx_authz.constants.permissions import COURSES_VIEW_PAGES_AND_RESOURCES +from rest_framework.exceptions import NotFound from rest_framework.permissions import BasePermission -from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access +from common.djangoapps.student.auth import ( + check_course_advanced_settings_access, + has_studio_read_access, + has_studio_write_access, +) from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission from openedx.core.djangoapps.authz.decorators import user_has_course_permission +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.view_utils import validate_course_key +COURSE_NOT_FOUND_MESSAGE = "The requested course does not exist." + class HasStudioWriteAccess(BasePermission): """ @@ -58,3 +66,60 @@ def has_permission(self, request, view): if course_key is None: return False return has_studio_read_access(request.user, course_key) + + +class IsExistingCourse(BasePermission): + """ + Answer 404 when the course in the path does not exist. + + Reads the parsed ``course_key`` URL argument. A key that names only a + content version parses, but no course run is behind it, so it is answered + as an unknown course. List this class after the authentication check, so + an anonymous caller learns nothing about which courses exist, and before + any class that checks roles on the course, so a signed-in caller is told + that a course is missing before being refused. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + if course_key.org is None or not CourseOverview.course_exists(course_key): + raise NotFound(COURSE_NOT_FOUND_MESSAGE) + return True + + +class CanViewAdvancedSettings(BasePermission): + """ + Allow callers who may view the advanced settings of the course in the path. + + Reads the parsed ``course_key`` URL argument. On courses where + policy-based authoring permissions are enabled, only that policy is + consulted; elsewhere the caller needs Studio read access to the course. + Advanced settings being restricted to staff site-wide does not apply here: + course teams also change page and resource toggles through these settings. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return check_course_advanced_settings_access(request.user, course_key, access_type="read") + + +class CanManageAdvancedSettings(BasePermission): + """ + Allow callers who may change the advanced settings of the course in the path. + + Reads the parsed ``course_key`` URL argument. On courses where + policy-based authoring permissions are enabled, only that policy is + consulted; elsewhere the caller needs Studio write access to the course. + Advanced settings being restricted to staff site-wide does not apply here, + as for ``CanViewAdvancedSettings``. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return check_course_advanced_settings_access(request.user, course_key, access_type="write") diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 662cf80c65f0..b4292d975e96 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -4,15 +4,17 @@ from drf_spectacular.openapi import AutoSchema -# Legacy operations superseded by a conforming address under /api/authoring/. -# Each is listed in the truncated spelling as well as the full one: the routes -# make the final character optional, so both addresses resolve, and which one -# the generator emits depends on how it renders that regex. +# Legacy operations superseded by a conforming address under /api/authoring/, +# matched on the exact path. A route that makes its final character optional +# resolves with and without it, and the generator may emit either spelling, so +# such a route is listed in both. SUPERSEDED_PATHS = ( "/api/contentstore/v0/youtube_transcripts/{course_id}/chec", "/api/contentstore/v0/youtube_transcripts/{course_id}/check", "/api/contentstore/v0/youtube_transcripts/{course_id}/uploa", "/api/contentstore/v0/youtube_transcripts/{course_id}/upload", + # → /api/authoring/v1/courses/{course_key}/advanced_settings/ + "/api/contentstore/v0/advanced_settings/{course_id}", ) # Legacy addresses of APIs migrated to /api/authoring/, marked deprecated for diff --git a/cms/lib/tests/test_spectacular.py b/cms/lib/tests/test_spectacular.py index 4581f23a6148..ba7732a260e6 100644 --- a/cms/lib/tests/test_spectacular.py +++ b/cms/lib/tests/test_spectacular.py @@ -76,6 +76,7 @@ def test_both_spellings_of_each_superseded_path_are_listed(self): "/api/contentstore/v0/youtube_transcripts/{course_id}/check", "/api/contentstore/v0/youtube_transcripts/{course_id}/uploa", "/api/contentstore/v0/youtube_transcripts/{course_id}/upload", + "/api/contentstore/v0/advanced_settings/{course_id}", } def test_the_conforming_paths_are_left_unmarked(self): @@ -83,8 +84,18 @@ def test_the_conforming_paths_are_left_unmarked(self): result = self._run({"paths": {conforming: {"get": {"operationId": "x"}}}}) assert result["paths"][conforming]["get"] == {"operationId": "x"} + def test_the_advanced_settings_operations_are_marked_and_their_successor_is_not(self): + legacy = "/api/contentstore/v0/advanced_settings/{course_id}" + conforming = "/api/authoring/v1/courses/{course_key}/advanced_settings/" + result = self._run({"paths": { + legacy: {"get": {}, "patch": {}}, + conforming: {"get": {}, "patch": {}}, + }}) + assert result["paths"][legacy] == {"get": {"deprecated": True}, "patch": {"deprecated": True}} + assert result["paths"][conforming] == {"get": {}, "patch": {}} + def test_an_unrelated_contentstore_path_is_left_unmarked(self): - other = "/api/contentstore/v0/advanced_settings/{course_id}" + other = "/api/contentstore/v0/heartbeat" result = self._run({"paths": {other: {"get": {}}}}) assert "deprecated" not in result["paths"][other]["get"]