From 1b301486e2f5344e4d1466f3acd1955e2db75aa4 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Thu, 15 Oct 2026 03:41:09 +0500 Subject: [PATCH 1/2] feat: add /api/user/v2/ users, user_preferences and time_zones The user directory, the stored-preference listings and the time-zone reference list were the least conforming endpoints of the User API: the only gate was the deprecated X-Edx-Api-Key header, which ignores request.user entirely, so any holder of the key could read every user's email address and full preference mapping. The listings were addressed by database id, two of them put the parent key in the path instead of the query string, the time-zone list returned an unbounded array, the stored preference row embedded its owner's entire record (one extra query per row), and the forum-role listing created the role it was asked about on every GET. Each of those is a contract change, so they land on a new version and /api/user/v1/ is untouched and keeps serving them. What callers get instead: GET /api/user/v2/users/ (also ?preference_key=, ?forum_role=&course_key=) GET /api/user/v2/users/{username}/ GET /api/user/v2/user_preferences/ GET /api/user/v2/user_preferences/{username},{key}/ GET /api/user/v2/time_zones/ Authenticated staff callers rather than an API key, the standard error envelope, the standard pagination envelope on all three lists, filter sets and a deterministic ordering, and the ?view= and ?fields= response presets. The forum-role filter no longer creates a role, and an unparsable course key is a validation error rather than a 500. Being refused is part of the contract, so the refusals are explicit. A deactivated staff account is turned away on both paths rather than only one: the session authenticator already rejected it, but IsAdminUser reads only is_staff and the JWT authenticator does not read is_active, so such an account could read the whole directory until its token expired. These addresses require an active user. An unrecognised query parameter is a 400 rather than silence. A client porting from a superseded address by changing the version segment alone would otherwise pass a filter this version does not define, have it ignored, and receive every row believing the page was filtered. The allowlist is derived from the paginator, the ordering backend, the filter set and the query serializer, so it cannot drift away from the filters it describes. Shared building blocks are used rather than rewritten: StandardizedErrorMixin, DefaultPagination, IterablePaginationMixin, ScopedQuerysetMixin with FullScopePolicy, MinimalViewMixin and project. The two local classes each supply one predicate the library does not, and each docstring says which. The tests pin behaviour rather than structure: every allowed and every refused caller with the error type asserted, value-level parity with each superseded address, query counts that stay constant across 1, 5 and 25 rows where the superseded listing pays per row, SQL captures proving no GET writes, and the URL literals each name reverses to. --- lms/urls.py | 1 + .../core/djangoapps/user_api/v2/__init__.py | 1 + openedx/core/djangoapps/user_api/v2/errors.py | 36 + .../core/djangoapps/user_api/v2/filters.py | 178 +++ .../core/djangoapps/user_api/v2/paginators.py | 57 + .../djangoapps/user_api/v2/permissions.py | 20 + openedx/core/djangoapps/user_api/v2/schema.py | 31 + .../djangoapps/user_api/v2/serializers.py | 206 +++ .../djangoapps/user_api/v2/tests/__init__.py | 1 + .../user_api/v2/tests/test_parity.py | 374 +++++ .../user_api/v2/tests/test_query_counts.py | 178 +++ .../user_api/v2/tests/test_schema.py | 531 +++++++ .../user_api/v2/tests/test_url_mount.py | 68 + .../djangoapps/user_api/v2/tests/test_urls.py | 296 ++++ .../user_api/v2/tests/test_views.py | 1401 +++++++++++++++++ openedx/core/djangoapps/user_api/v2/urls.py | 71 + openedx/core/djangoapps/user_api/v2/views.py | 593 +++++++ 17 files changed, 4043 insertions(+) create mode 100644 openedx/core/djangoapps/user_api/v2/__init__.py create mode 100644 openedx/core/djangoapps/user_api/v2/errors.py create mode 100644 openedx/core/djangoapps/user_api/v2/filters.py create mode 100644 openedx/core/djangoapps/user_api/v2/paginators.py create mode 100644 openedx/core/djangoapps/user_api/v2/permissions.py create mode 100644 openedx/core/djangoapps/user_api/v2/schema.py create mode 100644 openedx/core/djangoapps/user_api/v2/serializers.py create mode 100644 openedx/core/djangoapps/user_api/v2/tests/__init__.py create mode 100644 openedx/core/djangoapps/user_api/v2/tests/test_parity.py create mode 100644 openedx/core/djangoapps/user_api/v2/tests/test_query_counts.py create mode 100644 openedx/core/djangoapps/user_api/v2/tests/test_schema.py create mode 100644 openedx/core/djangoapps/user_api/v2/tests/test_url_mount.py create mode 100644 openedx/core/djangoapps/user_api/v2/tests/test_urls.py create mode 100644 openedx/core/djangoapps/user_api/v2/tests/test_views.py create mode 100644 openedx/core/djangoapps/user_api/v2/urls.py create mode 100644 openedx/core/djangoapps/user_api/v2/views.py diff --git a/lms/urls.py b/lms/urls.py index 35a1b6cdabcd..82526e9a83ef 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -142,6 +142,7 @@ path('api/courses/', include('lms.djangoapps.course_api.urls')), # User API endpoints + path('api/user/v2/', include('openedx.core.djangoapps.user_api.v2.urls')), path('api/user/', include('openedx.core.djangoapps.user_api.urls')), # Note: these are older versions of the User API that will eventually be # subsumed by api/user listed above. diff --git a/openedx/core/djangoapps/user_api/v2/__init__.py b/openedx/core/djangoapps/user_api/v2/__init__.py new file mode 100644 index 000000000000..333c1a588afe --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/__init__.py @@ -0,0 +1 @@ +"""Version 2 of the User API.""" diff --git a/openedx/core/djangoapps/user_api/v2/errors.py b/openedx/core/djangoapps/user_api/v2/errors.py new file mode 100644 index 000000000000..223f7d59e69a --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/errors.py @@ -0,0 +1,36 @@ +"""Error types this version of the User API classifies for itself.""" + +from edx_rest_framework_extensions.errors import register_error_type +from rest_framework.exceptions import MethodNotAllowed, NotAcceptable + + +class RefusedMethod(MethodNotAllowed): + """A request method this API does not serve at the address it was sent to.""" + + +class UnsatisfiableAccept(NotAcceptable): + """An ``Accept`` header naming no representation this API can produce.""" + + +register_error_type(RefusedMethod, "method-not-allowed", "Method Not Allowed") +register_error_type(UnsatisfiableAccept, "not-acceptable", "Not Acceptable") + + +class CatalogedRequestErrorsMixin: + """ + Names the error type of a refused method and of an unanswerable ``Accept`` header. + + The shared catalog classifies neither, so both would reach the caller + described as a server fault at a 4xx status. Raising this API's own + subclasses of them names each error without changing how the same failure + is classified for any other API in the process; once the shared catalog + covers them, the subclasses can give way to it. + """ + + def handle_exception(self, exc): + """Classify the two request failures the shared catalog leaves unnamed.""" + if isinstance(exc, MethodNotAllowed) and not isinstance(exc, RefusedMethod): + exc = RefusedMethod(self.request.method, detail=exc.detail) + elif isinstance(exc, NotAcceptable) and not isinstance(exc, UnsatisfiableAccept): + exc = UnsatisfiableAccept(detail=exc.detail) + return super().handle_exception(exc) diff --git a/openedx/core/djangoapps/user_api/v2/filters.py b/openedx/core/djangoapps/user_api/v2/filters.py new file mode 100644 index 000000000000..741aaee5fc12 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/filters.py @@ -0,0 +1,178 @@ +"""Filter sets for version 2 of the User API.""" + +import logging + +from django import forms +from django.contrib.auth.models import User # pylint: disable=imported-auth-user +from django.core.exceptions import ValidationError as FormValidationError +from django_filters import rest_framework as filters +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework.exceptions import ValidationError +from rest_framework.filters import OrderingFilter + +from openedx.core.djangoapps.user_api.models import UserPreference + +log = logging.getLogger(__name__) + + +class StrictOrderingFilter(OrderingFilter): + """ + Refuses a sort field the view does not offer, and completes the sort it accepts. + + The default backend drops an unusable field silently and answers 200 in + the default order, which a caller reads as a page sorted the way it asked. + It also replaces the view's own order rather than adding to it, so a sort + on a column several rows share leaves their relative order to the database + and the pages of one listing need not agree on it; the fields named by the + view's ``ordering_tiebreak`` are appended to settle those rows. + """ + + def get_ordering(self, request, queryset, view): + """Return the requested sort, followed by the fields that make it total.""" + ordering = super().get_ordering(request, queryset, view) + if not ordering: + return ordering + chosen = {field.lstrip("-") for field in ordering} + tiebreak = [field for field in getattr(view, "ordering_tiebreak", ()) if field not in chosen] + return [*ordering, *tiebreak] + + def remove_invalid_fields(self, queryset, fields, view, request): + """Return the requested sort fields, refusing the request when one is not offered.""" + allowed = super().remove_invalid_fields(queryset, fields, view, request) + rejected = [field for field in fields if field not in allowed] + if rejected: + log.info("Sort refused on %s: %r", type(view).__name__, rejected) + offered = sorted( + name for name, _label in self.get_valid_fields(queryset, view, {"request": request}) + ) + raise ValidationError({ + "ordering": [ + f"Sort by any of: {', '.join(offered)}, each optionally prefixed with '-'.", + ], + }) + return allowed + + +class _BooleanQueryField(forms.NullBooleanField): + """Reads a boolean out of the query string, refusing a value that names neither side.""" + + def to_python(self, value): + """Return the boolean this value names, or refuse the request.""" + if value in self.empty_values: + return None + parsed = super().to_python(value) + if parsed is None: + raise FormValidationError("Enter true or false.", code="invalid") + return parsed + + +class StrictBooleanFilter(filters.BooleanFilter): + """ + Refuses a value that names neither true nor false instead of ignoring it. + + The default field reads an unrecognised value as "no preference" and + answers 200 with the filter dropped, handing the caller the rows it asked + to leave out, which is the opposite of what it asked for. + """ + + field_class = _BooleanQueryField + + def __init__(self, *args, **kwargs): + """Pass the raw value through to the field, which is what judges it.""" + # The default widget maps anything it does not recognise to "no + # preference" before the field ever sees it; a text widget hands the + # value over untouched. + kwargs.setdefault("widget", forms.TextInput) + super().__init__(*args, **kwargs) + + +class UserFilterSet(filters.FilterSet): + """ + Narrows the user directory. + + ``forum_role`` and ``course_key`` name a role within one course and are + only meaningful together, so either on its own is rejected. A role that no + course has defined matches no users and is not created. + """ + + username = filters.CharFilter(help_text="Return the user with exactly this username.") + email = filters.CharFilter(help_text="Return the user with exactly this email address.") + is_active = StrictBooleanFilter( + help_text="Return only users whose account is active, or only inactive ones. true or false.", + ) + preference_key = filters.CharFilter( + method="filter_preference_key", + help_text="Return only users who have stored a preference with this key.", + ) + forum_role = filters.CharFilter( + method="filter_forum_role", + help_text="Return only users holding this course forum role. Requires course_key.", + ) + course_key = filters.CharFilter( + method="filter_course_key", + help_text=( + "Course the forum role belongs to, percent-encoded, for example " + "course-v1:edX%2BDemoX%2BDemo. A plus sign that is not encoded arrives as a " + "space and the key is refused. Requires forum_role." + ), + ) + + class Meta: + model = User + fields = ("username", "email", "is_active", "preference_key", "forum_role", "course_key") + + def filter_queryset(self, queryset): + """Reject a forum role without its course, or a course without its role, before filtering.""" + cleaned = self.form.cleaned_data + forum_role = cleaned.get("forum_role") + course_key = cleaned.get("course_key") + if forum_role and not course_key: + raise ValidationError({"course_key": ["This parameter is required when forum_role is supplied."]}) + if course_key and not forum_role: + raise ValidationError({"forum_role": ["This parameter is required when course_key is supplied."]}) + return super().filter_queryset(queryset) + + def filter_preference_key(self, queryset, name, value): # pylint: disable=unused-argument + """Keep users who have stored a preference under ``value``.""" + return queryset.filter(preferences__key=value) + + def filter_forum_role(self, queryset, name, value): # pylint: disable=unused-argument + """ + Keep users holding the role ``value`` in the course named by ``course_key``. + + Nothing stops a course from carrying the same role under several rows, + and a user may hold any number of them, so the join is collapsed to one + entry per user. + """ + raw_course_key = self.form.cleaned_data.get("course_key") + try: + course_key = CourseKey.from_string(raw_course_key) + except InvalidKeyError as exc: + log.info("User directory filtered with an unparsable course key: %r", raw_course_key) + raise ValidationError({"course_key": ["Enter a valid course key."]}) from exc + return queryset.filter(roles__name=value, roles__course_id=course_key).distinct() + + def filter_course_key(self, queryset, name, value): # pylint: disable=unused-argument + """Narrowing happens in ``filter_forum_role``, which needs both values at once.""" + return queryset + + +class UserPreferenceFilterSet(filters.FilterSet): + """ + Narrows the stored-preference list by preference name or by owner. + + An owner is named by username: the list is keyed the way the addresses of + its members are, and publishes no database identifier for a caller to + filter on. + """ + + key = filters.CharFilter(help_text="Return only preferences stored under exactly this key.") + username = filters.CharFilter( + field_name="user__username", + help_text="Return only preferences belonging to this user.", + ) + + class Meta: + model = UserPreference + fields = ("key", "username") diff --git a/openedx/core/djangoapps/user_api/v2/paginators.py b/openedx/core/djangoapps/user_api/v2/paginators.py new file mode 100644 index 000000000000..5e796180d3cb --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/paginators.py @@ -0,0 +1,57 @@ +"""Pagination for version 2 of the User API.""" + +from edx_rest_framework_extensions.paginators import DefaultPagination + + +class UserApiPagination(DefaultPagination): + """ + Page-number pagination whose published schema matches the body it returns. + + ``DefaultPagination`` answers with ``count``, ``num_pages``, + ``current_page``, ``start``, ``next``, ``previous`` and ``results`` but + inherits the four-member schema of DRF's ``PageNumberPagination``, so a + generated client would not know about the three page-position members. + This subclass only restates the response schema; the body is unchanged. + """ + + def get_paginated_response_schema(self, schema): + """Describe the full envelope the paginator returns.""" + return { + "type": "object", + "required": ["count", "num_pages", "current_page", "start", "results"], + "properties": { + "count": { + "type": "integer", + "example": 123, + "description": "Total number of items across every page.", + }, + "num_pages": { + "type": "integer", + "example": 13, + "description": "Total number of pages.", + }, + "current_page": { + "type": "integer", + "example": 2, + "description": "Number of the page in this response, counting from one.", + }, + "start": { + "type": "integer", + "example": 10, + "description": "Index of the first item of this page within the full result set.", + }, + "next": { + "type": "string", + "nullable": True, + "format": "uri", + "description": "URL of the next page, or null on the last page.", + }, + "previous": { + "type": "string", + "nullable": True, + "format": "uri", + "description": "URL of the previous page, or null on the first page.", + }, + "results": schema, + }, + } diff --git a/openedx/core/djangoapps/user_api/v2/permissions.py b/openedx/core/djangoapps/user_api/v2/permissions.py new file mode 100644 index 000000000000..80e78b269541 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/permissions.py @@ -0,0 +1,20 @@ +"""Permission classes for version 2 of the User API.""" + +from rest_framework.permissions import BasePermission + + +class IsActiveUser(BasePermission): + """ + Admits only a caller whose account is still active. + + The authenticated and administrator checks read ``is_authenticated`` and + ``is_staff``, and neither reads ``is_active``. A token stays valid until it + expires and is accepted without the account being re-read for it, so an + account deactivated after a token was issued keeps every access that token + carries unless the account's state is checked here. edx-drf-extensions + ships no equivalent class, so this one stands in for it. + """ + + def has_permission(self, request, view): + """Return whether the caller's account is still active.""" + return bool(request.user and request.user.is_active) diff --git a/openedx/core/djangoapps/user_api/v2/schema.py b/openedx/core/djangoapps/user_api/v2/schema.py new file mode 100644 index 000000000000..1db961213b16 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/schema.py @@ -0,0 +1,31 @@ +"""Schema helpers for version 2 of the User API.""" + +from drf_spectacular.serializers import PolymorphicProxySerializerExtension +from drf_spectacular.utils import PolymorphicProxySerializer + + +class ResponseShapes(PolymorphicProxySerializer): + """ + The set of bodies one operation returns for the same status code. + + An endpoint whose caller can reduce the response answers with the default + object, with a preset, or with a selection of properties. A reduced body + satisfies more than one of those descriptions, so they are published as + alternatives a body may match in any combination. + """ + + def __init__(self, component_name, serializers, **kwargs): + # The generator re-instantiates a declaration from the arguments it + # retained, so the discriminator is only supplied when it is missing. + kwargs.setdefault("resource_type_field_name", None) + super().__init__(component_name=component_name, serializers=serializers, **kwargs) + + +class ResponseShapesExtension(PolymorphicProxySerializerExtension): + """Publishes the alternatives of a ``ResponseShapes`` declaration.""" + + target_class = ResponseShapes + + def map_serializer(self, auto_schema, direction): + """Return the alternatives as a set a body may match more than one of.""" + return {"anyOf": super().map_serializer(auto_schema, direction)["oneOf"]} diff --git a/openedx/core/djangoapps/user_api/v2/serializers.py b/openedx/core/djangoapps/user_api/v2/serializers.py new file mode 100644 index 000000000000..8c418252640d --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/serializers.py @@ -0,0 +1,206 @@ +"""Serializers for version 2 of the User API.""" + +from django.contrib.auth.models import User # pylint: disable=imported-auth-user +from django.urls import reverse +from drf_spectacular.utils import extend_schema_field +from edx_rest_framework_extensions.errors import ErrorResponseSerializer +from rest_framework import serializers + +from openedx.core.djangoapps.user_api.models import UserPreference + +#: Value of the ``view`` query parameter that embeds the owning user in each preference. +FULL_VIEW = "full" + +#: Value of the ``view`` query parameter that reduces a user to its identifying fields. +MINIMAL_VIEW = "minimal" + +#: Properties the ``minimal`` preset keeps of a directory entry. +USER_MINIMAL_FIELDS = ("id", "username", "email") + +#: Properties the ``minimal`` preset keeps of a stored preference. +PREFERENCE_MINIMAL_FIELDS = ("username", "key", "value") + + +class ErrorResponse(ErrorResponseSerializer): # pylint: disable=abstract-method + """ + The body of every unsuccessful response. + + ``type`` is a URI naming the kind of failure, ``title`` names it in one + line, ``status`` repeats the HTTP status code and ``detail`` explains what + happened; ``errors`` lists the per-field messages of a rejected parameter. + """ + + +class _KeptFieldsMixin: + """ + Describes an object reduced to the properties named in ``kept_fields``. + + The response presets reduce an already serialized object, so a preset's + shape has a describing serializer of its own rather than a serializing one. + """ + + #: Names of the properties the preset returns. + kept_fields = () + + def get_fields(self): + """Return the declared fields narrowed to the ones the preset returns.""" + fields = super().get_fields() + return {name: field for name, field in fields.items() if name in self.kept_fields} + + +class _OptionalFieldsMixin: + """ + Describes an object whose every property is optional. + + A caller that selects properties with ``fields`` decides which ones come + back, so none of them is guaranteed to be present. + """ + + def get_fields(self): + """Return the declared fields, none of them mandatory.""" + fields = super().get_fields() + for field in fields.values(): + field.required = False + field.read_only = False + return fields + + +def full_view_requested(request): + """Return True when the caller asked for the embedded-user response preset.""" + if request is None: + return False + params = getattr(request, "query_params", None) + if params is None: + params = getattr(request, "GET", {}) + return params.get("view") == FULL_VIEW + + +class UserDirectorySerializer(serializers.ModelSerializer): + """A user of the platform directory, addressed by username.""" + + id = serializers.IntegerField( + read_only=True, + help_text="Database identifier of the user. Addresses in this API use the username instead.", + ) + url = serializers.HyperlinkedIdentityField( + view_name="user_v2:user_detail", + lookup_field="username", + help_text="Absolute URL of this user's directory entry.", + ) + email = serializers.EmailField(read_only=True, help_text="Registered email address of the user.") + username = serializers.CharField(read_only=True, help_text="Unique name the user is addressed by.") + name = serializers.SerializerMethodField( + help_text="Full name recorded on the user's profile, or null when the user has no profile.", + ) + preferences = serializers.SerializerMethodField( + help_text="The user's stored preferences, as a mapping of preference key to value.", + ) + + class Meta: + model = User + fields = ("id", "url", "email", "name", "username", "preferences") + read_only_fields = fields + + @extend_schema_field(serializers.CharField(allow_null=True)) + def get_name(self, user): + """Return the profile name, or None for a user who has no profile row.""" + profile = getattr(user, "profile", None) + return profile.name if profile is not None else None + + @extend_schema_field(serializers.DictField(child=serializers.CharField())) + def get_preferences(self, user): + """Return the user's preferences as a key-to-value mapping.""" + return UserPreference.get_all_preferences(user) + + +class UserDirectoryMinimalSerializer(_KeptFieldsMixin, UserDirectorySerializer): + """A directory entry reduced to the properties that identify the user.""" + + kept_fields = USER_MINIMAL_FIELDS + + +class UserDirectoryProjectionSerializer(_OptionalFieldsMixin, UserDirectorySerializer): + """A directory entry reduced to the properties the caller selected.""" + + +class UserPreferenceRecordSerializer(serializers.ModelSerializer): + """ + A stored preference, identified by the user that owns it and its key. + + ``user`` carries the owner's whole directory entry and is included only + when the caller asks for the ``full`` response preset; by default the + owner appears as the ``username`` string alone. + """ + + url = serializers.SerializerMethodField(help_text="Absolute URL of this preference.") + username = serializers.CharField( + source="user.username", + read_only=True, + help_text="Username of the user this preference belongs to.", + ) + key = serializers.CharField(read_only=True, help_text="Name of the preference.") + value = serializers.CharField(read_only=True, help_text="Stored value of the preference.") + user = serializers.SerializerMethodField( + help_text="Directory entry of the owning user; null unless the full response preset is asked for.", + ) + + class Meta: + model = UserPreference + fields = ("username", "key", "value", "url", "user") + read_only_fields = fields + + @extend_schema_field(serializers.URLField()) + def get_url(self, preference): + """Return the member address of this preference.""" + path = reverse( + "user_v2:user_preference_detail", + kwargs={"username": preference.user.username, "key": preference.key}, + ) + request = self.context.get("request") + return request.build_absolute_uri(path) if request is not None else path + + @extend_schema_field(UserDirectorySerializer(allow_null=True)) + def get_user(self, preference): + """Embed the owner only when the caller asked for the full preset.""" + if not full_view_requested(self.context.get("request")): + return None + return UserDirectorySerializer(preference.user, context=self.context).data + + +class UserPreferenceRecordMinimalSerializer(_KeptFieldsMixin, UserPreferenceRecordSerializer): + """A stored preference reduced to its owner, its name and its value.""" + + kept_fields = PREFERENCE_MINIMAL_FIELDS + + +class UserPreferenceRecordProjectionSerializer(_OptionalFieldsMixin, UserPreferenceRecordSerializer): + """A stored preference reduced to the properties the caller selected.""" + + +class TimeZoneSerializer(serializers.Serializer): # pylint: disable=abstract-method + """A time zone and the way it is displayed to a learner.""" + + time_zone = serializers.CharField(help_text="Name of the time zone, for example ``US/Pacific``.") + description = serializers.CharField( + help_text="Display form of the time zone, for example ``US/Pacific (PST, UTC-0800)``.", + ) + + +class TimeZoneQuerySerializer(serializers.Serializer): # pylint: disable=abstract-method + """Query parameters accepted by the time-zone list.""" + + country_code = serializers.RegexField( + r"^[A-Za-z]{2}$", + required=False, + # An empty value narrows nothing, the way an empty value of any other + # parameter does, and is accepted rather than held to the pattern. + allow_blank=True, + error_messages={ + "invalid": "Enter a two-letter ISO 3166-1 alpha-2 country code, for example FR.", + }, + help_text=( + "Two-letter ISO 3166-1 alpha-2 country code, case-insensitive. " + "Every time zone is returned when it is omitted or left empty, and when " + "the code names a country with no time zones of its own." + ), + ) diff --git a/openedx/core/djangoapps/user_api/v2/tests/__init__.py b/openedx/core/djangoapps/user_api/v2/tests/__init__.py new file mode 100644 index 000000000000..61ab32b09c15 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/tests/__init__.py @@ -0,0 +1 @@ +"""Tests for version 2 of the User API.""" diff --git a/openedx/core/djangoapps/user_api/v2/tests/test_parity.py b/openedx/core/djangoapps/user_api/v2/tests/test_parity.py new file mode 100644 index 000000000000..38a75fe6bd13 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/tests/test_parity.py @@ -0,0 +1,374 @@ +"""Behaviour parity between the version 1 and version 2 User API addresses.""" + +import json + +import pytest +from django.contrib.auth.models import User # pylint: disable=imported-auth-user +from django.core.exceptions import ObjectDoesNotExist +from django.test.utils import override_settings +from django.urls import reverse +from edx_rest_framework_extensions.testing import assert_error_envelope +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework import status +from rest_framework.test import APITestCase + +from common.djangoapps.student.tests.factories import AdminFactory, UserFactory +from openedx.core.djangoapps.django_comment_common.models import Role +from openedx.core.djangoapps.user_api.tests.factories import UserPreferenceFactory +from openedx.core.djangolib.testing.utils import skip_unless_lms + +PASSWORD = "Password1234" +API_KEY = "parity-test-api-key" +COURSE_KEY = CourseKey.from_string("course-v1:edX+DemoX+Demo") + +#: Fields that legitimately differ per request and are ignored in the diff. +VOLATILE_FIELDS = {"instance", "next", "previous"} + +#: Differences the plan declares intentional, per case. Each entry is a +#: (json path, reason) pair and the diff must contain all of them and nothing else. +INTENTIONAL_DIFFERENCES = { + "user_list": [ + ("results[0].url", "the member address is keyed by username instead of database id"), + ("results[1].url", "the member address is keyed by username instead of database id"), + ], + "user_detail": [ + ("url", "the member address is keyed by username instead of database id"), + ], + "preference_list": [ + ( + "results[0].user", + "the embedded owner is a member of every row and is null unless the full preset is asked for", + ), + ("results[0].username", "the owner's username is carried in place of the embedded user object"), + ("results[0].url", "the member address is keyed by username and preference key"), + ( + "results[1].user", + "the embedded owner is a member of every row and is null unless the full preset is asked for", + ), + ("results[1].username", "the owner's username is carried in place of the embedded user object"), + ("results[1].url", "the member address is keyed by username and preference key"), + ], + "preference_detail": [ + ("user", "the embedded owner is a member of every row and is null unless the full preset is asked for"), + ("username", "the owner's username is carried in place of the embedded user object"), + ("url", "the member address is keyed by username and preference key"), + ], + "preference_key_filter": [ + ("results[0].url", "the member address is keyed by username instead of database id"), + ], + "forum_role_filter": [ + ("results[0].url", "the member address is keyed by username instead of database id"), + ], +} + + +def normalize(obj, path=""): + """Sort keys, drop volatile fields, and collect a {json path: value} map for diffing.""" + flat = {} + if isinstance(obj, dict): + for key in sorted(obj): + if key in VOLATILE_FIELDS: + continue + flat.update(normalize(obj[key], f"{path}.{key}" if path else key)) + elif isinstance(obj, list): + for index, item in enumerate(obj): + flat.update(normalize(item, f"{path}[{index}]")) + else: + flat[path] = obj + return flat + + +def diff(legacy, new): + """Return (every changed path, the changed paths no declared difference covers).""" + left, right = normalize(legacy), normalize(new) + return {key for key in set(left) | set(right) if left.get(key, "") != right.get(key, "")} + + +def unexpected(changed, intentional): + """Return the changed paths that no declared intentional difference covers.""" + declared = {path for path, _reason in intentional} + return { + key for key in changed + if not any(key == path or key.startswith(path + ".") or key.startswith(path + "[") for path in declared) + } + + +@skip_unless_lms +@override_settings(EDX_API_KEY=API_KEY) +class ParityTests(APITestCase): + """The same fixtures through the legacy and the conforming address.""" + + def setUp(self): + super().setUp() + self.admin = AdminFactory.create(username="parity-admin", password=PASSWORD) + self.user = UserFactory.create( + username="parity-user", email="parity-user@example.com", password=PASSWORD, + ) + self.preference = UserPreferenceFactory.create(user=self.user, key="pref-lang", value="en") + role = Role.objects.create(course_id=COURSE_KEY, name="Moderator") + role.users.add(self.user) + + def legacy(self, address, params=None): + """Call a version 1 address with the API key it is gated by.""" + return self.client.get(address, params or {}, HTTP_X_EDX_API_KEY=API_KEY) + + def conforming(self, address, params=None): + """Call a version 2 address as the administrator it is gated by.""" + assert self.client.login(username=self.admin.username, password=PASSWORD) + response = self.client.get(address, params or {}) + self.client.logout() + return response + + def assert_parity(self, legacy, new, case, expected_status=status.HTTP_200_OK): + """ + Assert the two bodies differ only where the case declares they do. + + Rows are compared where each version put them, so a page whose rows + move is a difference like any other and has to be declared. + """ + assert legacy.status_code == expected_status, legacy.content + assert new.status_code == expected_status, new.content + legacy_body = json.loads(legacy.content) + new_body = json.loads(new.content) + changed = diff(legacy_body, new_body) + intentional = INTENTIONAL_DIFFERENCES[case] + assert not unexpected(changed, intentional), ( + f"undeclared differences: {sorted(unexpected(changed, intentional))}\n" + f"legacy={legacy_body}\nnew={new_body}" + ) + for path, reason in intentional: + assert any( + key == path or key.startswith(path + ".") or key.startswith(path + "[") + for key in changed + ), f"declared difference did not occur: {path} ({reason})" + + def test_user_list_parity(self): + legacy = self.legacy("/api/user/v1/users/", {"page_size": 100}) + new = self.conforming(reverse("user_v2:user_list"), {"page_size": 100}) + self.assert_parity(legacy, new, "user_list") + + def test_the_conforming_page_is_ordered_by_username_whatever_the_creation_order(self): + """Version 1 applies no ordering, so only the conforming side can be pinned to a sequence.""" + for username in ("parity-zulu", "parity-alpha"): + UserFactory.create(username=username, email=f"{username}@example.com", password=PASSWORD) + + new = self.conforming(reverse("user_v2:user_list"), {"page_size": 100}) + usernames = [row["username"] for row in json.loads(new.content)["results"]] + assert usernames == ["parity-admin", "parity-alpha", "parity-user", "parity-zulu"] + + legacy = self.legacy("/api/user/v1/users/", {"page_size": 100}) + legacy_usernames = [row["username"] for row in json.loads(legacy.content)["results"]] + assert sorted(legacy_usernames) == usernames + + def test_the_api_key_admits_at_the_legacy_addresses_only(self): + """The header the version 1 addresses are gated by is not a credential at version 2.""" + legacy = self.legacy("/api/user/v1/users/", {"page_size": 100}) + assert legacy.status_code == status.HTTP_200_OK, legacy.content + assert sorted(row["username"] for row in json.loads(legacy.content)["results"]) == [ + "parity-admin", "parity-user", + ] + + for address in ( + reverse("user_v2:user_list"), + reverse("user_v2:user_detail", kwargs={"username": "parity-user"}), + reverse("user_v2:user_preference_list"), + reverse( + "user_v2:user_preference_detail", + kwargs={"username": "parity-user", "key": "pref-lang"}, + ), + ): + refused = self.client.get(address, HTTP_X_EDX_API_KEY=API_KEY) + envelope = assert_error_envelope( + refused, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn", + ) + assert envelope["detail"] == "Authentication credentials were not provided.", address + + def test_user_detail_parity(self): + legacy = self.legacy(f"/api/user/v1/users/{self.user.id}/") + new = self.conforming(reverse("user_v2:user_detail", kwargs={"username": "parity-user"})) + self.assert_parity(legacy, new, "user_detail") + + def test_preference_list_parity(self): + """More than one row, so the comparison pairs rows rather than reading a single one.""" + UserPreferenceFactory.create(user=self.user, key="pref-time", value="UTC") + legacy = self.legacy("/api/user/v1/user_prefs/", {"page_size": 100}) + new = self.conforming(reverse("user_v2:user_preference_list"), {"page_size": 100}) + assert len(json.loads(new.content)["results"]) == 2 + self.assert_parity(legacy, new, "preference_list") + + def test_the_conforming_preference_page_is_ordered_by_owner_and_key(self): + """Version 1 applies no ordering, so only the conforming side can be pinned to a sequence.""" + UserPreferenceFactory.create(user=self.user, key="pref-time", value="UTC") + UserPreferenceFactory.create(user=self.admin, key="pref-zone", value="UTC") + + new = self.conforming(reverse("user_v2:user_preference_list"), {"page_size": 100}) + rows = [(row["username"], row["key"]) for row in json.loads(new.content)["results"]] + assert rows == [ + ("parity-admin", "pref-zone"), + ("parity-user", "pref-lang"), + ("parity-user", "pref-time"), + ] + + legacy = self.legacy("/api/user/v1/user_prefs/", {"page_size": 100}) + legacy_rows = [ + (row["user"]["username"], row["key"]) for row in json.loads(legacy.content)["results"] + ] + assert legacy_rows == [ + ("parity-user", "pref-lang"), + ("parity-user", "pref-time"), + ("parity-admin", "pref-zone"), + ] + assert sorted(legacy_rows) == rows + + def test_preference_detail_parity(self): + legacy = self.legacy(f"/api/user/v1/user_prefs/{self.preference.id}/") + new = self.conforming( + reverse( + "user_v2:user_preference_detail", + kwargs={"username": "parity-user", "key": "pref-lang"}, + ), + ) + self.assert_parity(legacy, new, "preference_detail") + + def test_preference_key_filter_parity(self): + legacy = self.legacy("/api/user/v1/preferences/pref-lang/users/", {"page_size": 100}) + new = self.conforming( + reverse("user_v2:user_list"), {"preference_key": "pref-lang", "page_size": 100}, + ) + self.assert_parity(legacy, new, "preference_key_filter") + + def test_forum_role_filter_parity(self): + legacy = self.legacy( + "/api/user/v1/forum_roles/Moderator/users/", + {"course_id": str(COURSE_KEY), "page_size": 100}, + ) + new = self.conforming( + reverse("user_v2:user_list"), + {"forum_role": "Moderator", "course_key": str(COURSE_KEY), "page_size": 100}, + ) + self.assert_parity(legacy, new, "forum_role_filter") + + def test_full_preset_reproduces_the_legacy_embedded_user(self): + legacy = self.legacy(f"/api/user/v1/user_prefs/{self.preference.id}/") + new = self.conforming( + reverse( + "user_v2:user_preference_detail", + kwargs={"username": "parity-user", "key": "pref-lang"}, + ), + {"view": "full"}, + ) + legacy_user = json.loads(legacy.content)["user"] + new_user = json.loads(new.content)["user"] + assert set(legacy_user) == set(new_user) + for field in ("id", "email", "name", "username", "preferences"): + assert legacy_user[field] == new_user[field], field + assert legacy_user["url"].endswith(f"/user_api/v1/users/{self.user.id}/") + assert new_user["url"].endswith("/api/user/v2/users/parity-user/") + + def test_a_user_without_a_profile_is_a_row_here_and_a_failure_at_version_one(self): + """Declared difference: the version 1 entry cannot render a user who has no profile row.""" + profileless = User.objects.create_user("parity-profileless", "pp@example.com", PASSWORD) + with pytest.raises(ObjectDoesNotExist): + self.legacy(f"/api/user/v1/users/{profileless.id}/") + + new = self.conforming( + reverse("user_v2:user_detail", kwargs={"username": "parity-profileless"}), + ) + assert new.status_code == status.HTTP_200_OK + body = json.loads(new.content) + assert body["name"] is None + assert body["username"] == "parity-profileless" + + def test_time_zone_rows_are_unchanged_inside_the_new_envelope(self): + legacy = json.loads(self.legacy("/api/user/v1/preferences/time_zones/").content) + new = json.loads(self.conforming(reverse("user_v2:time_zone_list"), {"page_size": 100}).content) + assert isinstance(legacy, list) + assert new["count"] == len(legacy) + assert new["results"] == legacy[:100] + + def test_country_filtered_time_zones_are_unchanged(self): + legacy = json.loads( + self.legacy("/api/user/v1/preferences/time_zones/", {"country_code": "FR"}).content, + ) + new = json.loads( + self.conforming(reverse("user_v2:time_zone_list"), {"country_code": "FR"}).content, + ) + assert new["results"] == legacy + + def test_unauthenticated_bodies_differ_as_declared(self): + legacy = self.client.get("/api/user/v1/users/") + assert legacy.status_code == status.HTTP_401_UNAUTHORIZED + assert json.loads(legacy.content) == { + "detail": "Authentication credentials were not provided.", + } + + new = self.client.get(reverse("user_v2:user_list")) + envelope = assert_error_envelope( + new, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn", + ) + assert envelope["title"] == "Authentication Required" + assert envelope["detail"] == "Authentication credentials were not provided." + assert envelope["instance"] == "/api/user/v2/users/" + + def test_forbidden_bodies_differ_as_declared(self): + assert self.client.login(username=self.user.username, password=PASSWORD) + legacy = self.client.get("/api/user/v1/users/") + assert legacy.status_code == status.HTTP_403_FORBIDDEN + assert json.loads(legacy.content) == { + "detail": "You do not have permission to perform this action.", + } + + new = self.client.get(reverse("user_v2:user_list")) + envelope = assert_error_envelope( + new, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz", + ) + assert envelope["title"] == "Permission Denied" + assert envelope["detail"] == "You do not have permission to perform this action." + + def test_a_missing_course_parameter_keeps_its_status_and_changes_its_body(self): + """The legacy address already refused this caller; only the shape of the refusal moves.""" + legacy = self.legacy("/api/user/v1/forum_roles/Moderator/users/") + assert legacy.status_code == status.HTTP_400_BAD_REQUEST + assert json.loads(legacy.content) == {"detail": "course_id must be specified"} + + new = self.conforming(reverse("user_v2:user_list"), {"forum_role": "Moderator"}) + envelope = assert_error_envelope( + new, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "course_key": ["This parameter is required when forum_role is supplied."], + } + + def test_an_unparsable_course_parameter_is_answered_rather_than_raised(self): + """Declared difference: the version 1 address lets the key error escape the handler.""" + with pytest.raises(InvalidKeyError): + self.legacy("/api/user/v1/forum_roles/Moderator/users/", {"course_id": "not a course"}) + + new = self.conforming( + reverse("user_v2:user_list"), {"forum_role": "Moderator", "course_key": "not a course"}, + ) + envelope = assert_error_envelope( + new, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == {"course_key": ["Enter a valid course key."]} + + def test_a_database_identifier_is_refused_instead_of_narrowing_nothing(self): + """The directory is keyed by username, and says so rather than answering an unfiltered page.""" + refused = self.conforming(reverse("user_v2:user_list"), {"id": self.user.id, "page_size": 100}) + envelope = assert_error_envelope( + refused, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert sorted(envelope["errors"]) == ["id"] + + def test_an_identifier_held_by_a_caller_resolves_to_the_username_the_entry_is_keyed_by(self): + """The step an integration holding identifiers takes before addressing an entry.""" + assert self.client.login(username=self.admin.username, password=PASSWORD) + resolved = self.client.get("/api/user/v1/accounts", {"lms_user_id": self.user.id}) + self.client.logout() + assert resolved.status_code == status.HTTP_200_OK + assert [account["username"] for account in json.loads(resolved.content)] == ["parity-user"] + + entry = self.conforming(reverse("user_v2:user_detail", kwargs={"username": "parity-user"})) + assert entry.status_code == status.HTTP_200_OK + assert json.loads(entry.content)["id"] == self.user.id diff --git a/openedx/core/djangoapps/user_api/v2/tests/test_query_counts.py b/openedx/core/djangoapps/user_api/v2/tests/test_query_counts.py new file mode 100644 index 000000000000..e8cc522fa8f1 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/tests/test_query_counts.py @@ -0,0 +1,178 @@ +"""Query-count guards for the version 2 User API list and member addresses.""" + +import pytest +from django.test.utils import override_settings +from django.urls import reverse + +from common.djangoapps.student.tests.factories import AdminFactory, UserFactory +from openedx.core.djangoapps.django_comment_common.models import Role +from openedx.core.djangoapps.user_api.tests.factories import UserPreferenceFactory +from openedx.core.djangolib.testing.utils import skip_unless_lms + +PASSWORD = "Password1234" +COURSE_KEY = "course-v1:edX+DemoX+Demo" + +#: Row counts a per-row query would show up at. +ROW_COUNTS = [1, 5, 25] + +#: Page sizes a per-item query would show up at, for the list whose rows are +#: not database rows and whose size is therefore set by the page alone. +PAGE_SIZES = [1, 10, 100] + +USER_LIST_QUERIES = 16 +USER_DETAIL_QUERIES = 15 +PREFERENCE_LIST_QUERIES = 15 +PREFERENCE_LIST_FULL_VIEW_QUERIES = 16 +PREFERENCE_DETAIL_QUERIES = 14 +TIME_ZONE_LIST_QUERIES = 13 + +#: Address of the stored-preference list this version replaces, and the key it +#: is gated by, so the cost of the two can be compared in one measurement. +EARLIER_PREFERENCE_LIST = "/api/user/v1/user_prefs/" +EARLIER_API_KEY = "query-count-api-key" + +#: What that list costs for a page holding two rows per user, measured the same +#: way as the counts above: a constant part plus five queries for every user on +#: the page, because each row fetches its owner, the owner's profile and the +#: owner's own preferences again. +EARLIER_PREFERENCE_LIST_QUERIES = {1: 15, 5: 35, 25: 135} + + +@pytest.fixture(name="directory") +@skip_unless_lms +def directory_fixture(request, client, django_user_model): # pylint: disable=unused-argument + """Sign an administrator in and populate the directory with ``request.param`` users.""" + admin = AdminFactory.create(username="count-admin", password=PASSWORD) + users = [UserFactory.create(username=f"counted{index}") for index in range(request.param)] + for user in users: + UserPreferenceFactory.create(user=user, key="pref-lang", value="en") + UserPreferenceFactory.create(user=user, key="time_zone", value="UTC") + role = Role.objects.create(course_id=COURSE_KEY, name="Moderator") + role.users.add(*users) + assert client.login(username=admin.username, password=PASSWORD) + # The first authenticated request of a session populates caches the rest + # of the session reuses; the counts below are the steady-state ones. + client.get(reverse("user_v2:user_list")) + return users + + +@pytest.mark.django_db +@pytest.mark.parametrize("directory", ROW_COUNTS, indirect=True) +@skip_unless_lms +def test_user_list_query_count_does_not_grow_with_rows(client, directory, django_assert_num_queries): + url = reverse("user_v2:user_list") + with django_assert_num_queries(USER_LIST_QUERIES): + response = client.get(url, {"page_size": 100}) + assert len(response.json()["results"]) == len(directory) + 1 + + +@pytest.mark.django_db +@pytest.mark.parametrize("directory", ROW_COUNTS, indirect=True) +@skip_unless_lms +def test_preference_key_filter_query_count_does_not_grow_with_rows( + client, directory, django_assert_num_queries, +): + url = reverse("user_v2:user_list") + with django_assert_num_queries(USER_LIST_QUERIES): + response = client.get(url, {"preference_key": "pref-lang", "page_size": 100}) + assert len(response.json()["results"]) == len(directory) + + +@pytest.mark.django_db +@pytest.mark.parametrize("directory", ROW_COUNTS, indirect=True) +@skip_unless_lms +def test_forum_role_filter_query_count_does_not_grow_with_rows( + client, directory, django_assert_num_queries, +): + url = reverse("user_v2:user_list") + with django_assert_num_queries(USER_LIST_QUERIES): + response = client.get( + url, {"forum_role": "Moderator", "course_key": COURSE_KEY, "page_size": 100}, + ) + assert len(response.json()["results"]) == len(directory) + + +@pytest.mark.django_db +@pytest.mark.parametrize("directory", ROW_COUNTS, indirect=True) +@skip_unless_lms +def test_preference_list_query_count_does_not_grow_with_rows( + client, directory, django_assert_num_queries, +): + url = reverse("user_v2:user_preference_list") + with django_assert_num_queries(PREFERENCE_LIST_QUERIES): + response = client.get(url, {"page_size": 100}) + assert len(response.json()["results"]) == 2 * len(directory) + + +@pytest.mark.django_db +@pytest.mark.parametrize("directory", ROW_COUNTS, indirect=True) +@skip_unless_lms +def test_preference_list_full_view_query_count_does_not_grow_with_rows( + client, directory, django_assert_num_queries, +): + url = reverse("user_v2:user_preference_list") + with django_assert_num_queries(PREFERENCE_LIST_FULL_VIEW_QUERIES): + response = client.get(url, {"page_size": 100, "view": "full"}) + rows = response.json()["results"] + assert len(rows) == 2 * len(directory) + assert rows[0]["user"]["preferences"] == {"pref-lang": "en", "time_zone": "UTC"} + + +@pytest.mark.django_db +@pytest.mark.parametrize("page_size", PAGE_SIZES) +@pytest.mark.parametrize("directory", [1], indirect=True) +@skip_unless_lms +def test_time_zone_list_query_count_does_not_grow_with_page_size( + client, directory, django_assert_num_queries, page_size, +): # pylint: disable=unused-argument + """The rows come from the time-zone database, so only the page size varies them.""" + url = reverse("user_v2:time_zone_list") + with django_assert_num_queries(TIME_ZONE_LIST_QUERIES): + response = client.get(url, {"page_size": page_size}) + assert len(response.json()["results"]) == page_size + + +@pytest.mark.django_db +@pytest.mark.parametrize("directory", [5], indirect=True) +@skip_unless_lms +def test_member_query_counts(client, directory, django_assert_num_queries): + with django_assert_num_queries(USER_DETAIL_QUERIES): + response = client.get(reverse("user_v2:user_detail", kwargs={"username": "counted0"})) + assert response.json()["username"] == "counted0" + + with django_assert_num_queries(PREFERENCE_DETAIL_QUERIES): + response = client.get( + reverse( + "user_v2:user_preference_detail", + kwargs={"username": "counted0", "key": "pref-lang"}, + ), + ) + assert response.json()["value"] == "en" + + +@pytest.mark.django_db +@pytest.mark.parametrize("directory", ROW_COUNTS, indirect=True) +@skip_unless_lms +def test_the_earlier_preference_list_pays_per_row_where_this_one_does_not( + client, directory, django_assert_num_queries, +): + """ + Both lists are measured over the same rows, so the saving cannot drift apart. + + The earlier row embeds its owner without a join and re-reads that owner's + own preferences, so a page of it costs five queries per user; this one + reads the same page at a count that does not move with it. + """ + with django_assert_num_queries(PREFERENCE_LIST_QUERIES): + current = client.get(reverse("user_v2:user_preference_list"), {"page_size": 100}) + assert len(current.json()["results"]) == 2 * len(directory) + + client.logout() + with override_settings(EDX_API_KEY=EARLIER_API_KEY): + with django_assert_num_queries(EARLIER_PREFERENCE_LIST_QUERIES[len(directory)]): + earlier = client.get( + EARLIER_PREFERENCE_LIST, + {"page_size": 100}, + HTTP_X_EDX_API_KEY=EARLIER_API_KEY, + ) + assert len(earlier.json()["results"]) == 2 * len(directory) diff --git a/openedx/core/djangoapps/user_api/v2/tests/test_schema.py b/openedx/core/djangoapps/user_api/v2/tests/test_schema.py new file mode 100644 index 000000000000..de29bdae413e --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/tests/test_schema.py @@ -0,0 +1,531 @@ +"""Tests for what version 2 of the User API publishes in the OpenAPI schema.""" + +import json +from importlib import import_module + +import jsonschema +import pytest +from django.conf import settings +from django.urls import reverse +from drf_spectacular.generators import SchemaGenerator +from drf_spectacular.settings import patched_settings + +from common.djangoapps.student.tests.factories import AdminFactory +from lms.lib.spectacular import SUPERSEDED_USER_PATHS, lms_mark_legacy_paths_deprecated +from openedx.core.apidocs import get_api_docs_settings +from openedx.core.djangoapps.user_api.tests.factories import UserPreferenceFactory +from openedx.core.djangoapps.user_api.v2.serializers import TimeZoneQuerySerializer +from openedx.core.djangoapps.user_api.v2.views import PREFERENCE_KEY_PATTERN_PARTIAL +from openedx.core.djangolib.testing.utils import skip_unless_lms + +PASSWORD = "Password1234" + +USER_MEMBER_PATH = "/api/user/v2/users/{username}/" +PREFERENCE_MEMBER_PATH = "/api/user/v2/user_preferences/{username},{key}/" +TIME_ZONE_LIST_PATH = "/api/user/v2/time_zones/" + +#: The addresses that answer with a page of rows, and the ones that answer with +#: a single record. +LIST_PATHS = { + "/api/user/v2/users/", + "/api/user/v2/user_preferences/", + TIME_ZONE_LIST_PATH, +} +MEMBER_PATHS = {USER_MEMBER_PATH, PREFERENCE_MEMBER_PATH} + +VERSION_TWO_PATHS = LIST_PATHS | MEMBER_PATHS + +PAGINATION_MEMBERS = {"count", "num_pages", "current_page", "start", "next", "previous", "results"} + +#: Where this version is mounted in the service's URL configuration. +VERSION_TWO_MOUNT = "api/user/v2/" + +#: The components this version contributes to the document. +VERSION_TWO_COMPONENTS = ( + "ErrorResponse", + "PaginatedTimeZoneList", + "PaginatedUserDirectoryResponseList", + "PaginatedUserPreferenceRecordResponseList", + "TimeZone", + "UserDirectory", + "UserDirectoryMinimal", + "UserDirectoryProjection", + "UserDirectoryResponse", + "UserPreferenceRecord", + "UserPreferenceRecordMinimal", + "UserPreferenceRecordProjection", + "UserPreferenceRecordResponse", +) + +#: Every body these endpoints return: the address and parameters that produce +#: it, the published path it is documented under, and the rows it carries. +SHAPE_CASES = ( + ("/api/user/v2/users/", "/api/user/v2/users/", {}), + ("/api/user/v2/users/", "/api/user/v2/users/", {"view": "minimal"}), + ("/api/user/v2/users/", "/api/user/v2/users/", {"fields": "username,email"}), + ("/api/user/v2/users/", "/api/user/v2/users/", {"view": "minimal", "fields": "username"}), + ("/api/user/v2/users/{username}/", "/api/user/v2/users/shape-admin/", {}), + ("/api/user/v2/users/{username}/", "/api/user/v2/users/shape-admin/", {"view": "minimal"}), + ("/api/user/v2/users/{username}/", "/api/user/v2/users/shape-admin/", {"fields": "id,url"}), + ("/api/user/v2/user_preferences/", "/api/user/v2/user_preferences/", {}), + ("/api/user/v2/user_preferences/", "/api/user/v2/user_preferences/", {"view": "full"}), + ("/api/user/v2/user_preferences/", "/api/user/v2/user_preferences/", {"view": "minimal"}), + ("/api/user/v2/user_preferences/", "/api/user/v2/user_preferences/", {"fields": "key,value"}), + ( + "/api/user/v2/user_preferences/{username},{key}/", + "/api/user/v2/user_preferences/shape-admin,pref-lang/", + {}, + ), + ( + "/api/user/v2/user_preferences/{username},{key}/", + "/api/user/v2/user_preferences/shape-admin,pref-lang/", + {"view": "full"}, + ), + ( + "/api/user/v2/user_preferences/{username},{key}/", + "/api/user/v2/user_preferences/shape-admin,pref-lang/", + {"view": "minimal"}, + ), + ( + "/api/user/v2/user_preferences/{username},{key}/", + "/api/user/v2/user_preferences/shape-admin,pref-lang/", + {"fields": "value"}, + ), + ("/api/user/v2/time_zones/", "/api/user/v2/time_zones/", {"country_code": "FR"}), +) + + +def as_json_schema(node): + """ + Return a published schema node as a JSON Schema document. + + The document describes an absent value with ``nullable``, which JSON + Schema spells as an alternative type, so a validator reading the published + node verbatim would reject a member the API is documented to omit. + """ + 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 published_body_schema(schema, path): + """Return what a successful body of ``path`` must satisfy, components included.""" + body = schema["paths"][path]["get"]["responses"]["200"]["content"]["application/json"]["schema"] + return as_json_schema({**body, "components": schema["components"]}) + +#: The version 1 addresses this change supersedes, written out independently of +#: the constant the hook reads so that changing either one shows up here. +SUPERSEDED_USER_ADDRESSES = { + "/api/user/v1/users/": "/api/user/v2/users/", + "/api/user/v1/users/{id}/": "/api/user/v2/users/{username}/", + "/api/user/v1/user_prefs/": "/api/user/v2/user_preferences/", + "/api/user/v1/user_prefs/{id}/": "/api/user/v2/user_preferences/{username},{key}/", + "/api/user/v1/preferences/{pref_key}/users/": "/api/user/v2/users/", + "/api/user/v1/forum_roles/{name}/users/": "/api/user/v2/users/", + "/api/user/v1/preferences/time_zones/": "/api/user/v2/time_zones/", +} + +#: Legacy enrollment addresses this schema already marked before the user API joined it. +SUPERSEDED_ENROLLMENT_PATHS = { + "/api/enrollment/v2/course/{course_id}", + "/api/enrollment/v2/enrollment/{course_id}", + "/api/enrollment/v2/enrollment/{username},{course_id}", + "/api/enrollment/v2/enrollments", +} + +#: Enrollment operations whose ids must survive the widened path prefix. +ENROLLMENT_OPERATION_IDS = { + "v1_enrollments_list", + "v1_enrollment_create", + "v2_enrollments_list", + "v2_enrollments_list_legacy", + "v2_enrollment_,_retrieve", + "v2_roles_retrieve", +} + + +@pytest.fixture(name="schema") +@skip_unless_lms +def schema_fixture(db): # pylint: disable=invalid-name, unused-argument + """The schema published under the service's own drf-spectacular settings.""" + return SchemaGenerator().get_schema(request=None, public=True) + + +def service_wide_schema(patterns=None): + """Return the document the service publishes for browsing its whole surface.""" + with patched_settings(get_api_docs_settings()): + return SchemaGenerator(patterns=patterns).get_schema(request=None, public=True) + + +def patterns_without_this_version(): + """Return the service's URL configuration with this version's mount taken out.""" + configured = import_module(settings.ROOT_URLCONF).urlpatterns + remaining = [entry for entry in configured if str(entry.pattern) != VERSION_TWO_MOUNT] + assert len(remaining) == len(configured) - 1, "this version is not mounted where it is looked for" + return remaining + + +def published_names(schema): + """Return every component name and operation id a document carries.""" + return set(schema["components"]["schemas"]) | { + operation["operationId"] for _path, _method, operation in operations(schema) + } + + +def operations(schema): + """Yield every (path, method, operation) the schema documents.""" + for path, path_item in schema["paths"].items(): + for method, operation in path_item.items(): + if isinstance(operation, dict) and method in {"get", "post", "put", "patch", "delete"}: + yield path, method, operation + + +@skip_unless_lms +def test_version_two_addresses_are_published(schema): + assert VERSION_TWO_PATHS <= set(schema["paths"]) + + +@skip_unless_lms +def test_version_two_operations_carry_the_area_tag(schema): + tags = { + operation["operationId"]: operation.get("tags") + for path, _method, operation in operations(schema) + if path in VERSION_TWO_PATHS + } + assert tags == { + "v2_users_list": ["user"], + "v2_users_retrieve": ["user"], + "v2_user_preferences_list": ["user"], + "v2_user_preferences_retrieve": ["user"], + "v2_time_zones_list": ["user"], + } + + +@skip_unless_lms +def test_enrollment_operation_ids_survive_the_widened_prefix(schema): + published = {operation["operationId"] for _path, _method, operation in operations(schema)} + assert ENROLLMENT_OPERATION_IDS <= published + + +@skip_unless_lms +def test_only_superseded_addresses_are_marked_deprecated(schema): + deprecated = { + path for path, _method, operation in operations(schema) if operation.get("deprecated") + } + assert deprecated == SUPERSEDED_ENROLLMENT_PATHS | set(SUPERSEDED_USER_ADDRESSES) + + +@skip_unless_lms +def test_only_this_version_and_the_addresses_it_supersedes_are_published(schema): + """A user address this change neither serves nor replaces stays out of the document.""" + published = {path for path in schema["paths"] if path.startswith("/api/user/")} + assert published == VERSION_TWO_PATHS | set(SUPERSEDED_USER_ADDRESSES) + + +@skip_unless_lms +def test_addresses_owned_by_other_apps_are_not_published(schema): + """Neighbours of the mounts this change adds, including one under its own prefix.""" + for path in ( + "/api/user/v1/accounts", + "/api/user/v1/me", + "/api/user/v1/preferences/{username}", + "/api/user/v1/account/registration/", + "/api/user/v2/account/registration/", + ): + assert path not in schema["paths"], path + + +@skip_unless_lms +def test_pagination_envelope_is_published_in_full(schema): + for component in ( + "PaginatedUserDirectoryResponseList", + "PaginatedUserPreferenceRecordResponseList", + "PaginatedTimeZoneList", + ): + published = schema["components"]["schemas"][component] + assert set(published["properties"]) == PAGINATION_MEMBERS, component + + +@pytest.mark.django_db +@skip_unless_lms +def test_published_pagination_envelope_matches_the_response_body(client, schema): + admin = AdminFactory.create(username="schema-admin", password=PASSWORD) + UserPreferenceFactory.create(user=admin, key="pref-lang", value="en") + assert client.login(username=admin.username, password=PASSWORD) + body = client.get(reverse("user_v2:user_preference_list")).json() + published = schema["components"]["schemas"]["PaginatedUserPreferenceRecordResponseList"] + assert set(body) == set(published["properties"]) + + +@skip_unless_lms +def test_error_responses_reference_the_shared_envelope(schema): + for path, method, operation in operations(schema): + if path not in VERSION_TWO_PATHS: + continue + for code, response in operation["responses"].items(): + if not code.startswith("4"): + continue + schema_ref = response["content"]["application/json"]["schema"] + assert schema_ref == {"$ref": "#/components/schemas/ErrorResponse"}, (path, method, code) + + +@skip_unless_lms +def test_an_operation_that_takes_a_response_preset_publishes_the_refusal(schema): + """A preset the operation does not answer to is a 400, so each of them documents one.""" + takes_a_preset = [ + (path, method) + for path, method, operation in operations(schema) + if path in VERSION_TWO_PATHS + and any(parameter["name"] == "view" for parameter in operation.get("parameters", [])) + and "400" in operation["responses"] + ] + assert sorted(takes_a_preset) == [ + ("/api/user/v2/user_preferences/", "get"), + ("/api/user/v2/user_preferences/{username},{key}/", "get"), + ("/api/user/v2/users/", "get"), + ("/api/user/v2/users/{username}/", "get"), + ] + + +@skip_unless_lms +def test_user_list_publishes_its_filters(schema): + parameters = { + parameter["name"]: parameter + for parameter in schema["paths"]["/api/user/v2/users/"]["get"]["parameters"] + } + assert set(parameters) == { + "course_key", "email", "fields", "forum_role", "is_active", "ordering", + "page", "page_size", "preference_key", "username", "view", + } + # The filter that refuses an unparsable value is still published as a boolean. + assert parameters["is_active"]["schema"] == {"type": "boolean"} + + +@skip_unless_lms +def test_preference_list_publishes_its_filters(schema): + parameters = { + parameter["name"]: parameter + for parameter in schema["paths"]["/api/user/v2/user_preferences/"]["get"]["parameters"] + } + assert set(parameters) == {"fields", "key", "ordering", "page", "page_size", "username", "view"} + # Every published parameter is one the list reads, and one it keeps. + assert [name for name, parameter in parameters.items() if parameter.get("deprecated")] == [] + + +@skip_unless_lms +def test_enum_post_processing_runs_before_the_deprecation_hook(): + hooks = settings.SPECTACULAR_SETTINGS["POSTPROCESSING_HOOKS"] + assert hooks[0] == "drf_spectacular.hooks.postprocess_schema_enums" + assert "lms.lib.spectacular.lms_mark_legacy_paths_deprecated" in hooks[1:] + + +@skip_unless_lms +def test_hook_leaves_other_resources_alone(): + """The hook marks only the addresses it lists, even among their neighbours.""" + untouched = [ + "/api/user/v1/accounts", + "/api/user/v1/me", + "/api/user/v1/preferences/email_opt_in/", + "/api/user/v2/account/registration/", + "/user_api/v1/users/", + "/user_api/v1/preferences/time_zones/", + "/api/enrollment/v2/enrollments/", + "/api/courses/v1/courses/", + ] + marked = ["/api/user/v1/users/", "/api/enrollment/v2/enrollments"] + result = {"paths": {path: {"get": {"operationId": path}} for path in untouched + marked}} + + lms_mark_legacy_paths_deprecated(result, generator=None, request=None, public=True) + + for path in untouched: + assert "deprecated" not in result["paths"][path]["get"], path + for path in marked: + assert result["paths"][path]["get"]["deprecated"] is True, path + + +@skip_unless_lms +def test_every_superseded_address_has_a_published_replacement(schema): + assert set(SUPERSEDED_USER_ADDRESSES) == set(SUPERSEDED_USER_PATHS) + for superseded, replacement in SUPERSEDED_USER_ADDRESSES.items(): + assert superseded in schema["paths"] + assert replacement in schema["paths"] + + +@pytest.mark.django_db +@skip_unless_lms +def test_this_version_only_adds_to_the_service_wide_document(): + """ + Nothing published for browsing the whole service is lost when this version joins it. + + Enum post-processing runs over every operation the service publishes and + renames both sides of a name collision, so a component this version never + declared can be renamed out of the document, and a client generated from it + loses the matching model. The document is therefore read with this version + mounted and with its mount taken out, and the second has to be contained in + the first. + """ + without_this_version = published_names(service_wide_schema(patterns_without_this_version())) + whole = service_wide_schema() + missing = without_this_version - published_names(whole) + assert missing == set(), sorted(missing) + + components = whole["components"]["schemas"] + assert "user_api.User" in components + assert {"UserDirectory", "UserPreferenceRecord", "TimeZone"} <= set(components) + assert "CountryTimeZone" in components + + +@skip_unless_lms +def test_every_response_shape_is_published(schema): + """The reduced bodies the presets return are described, not only the full one.""" + components = schema["components"]["schemas"] + assert components["UserDirectoryResponse"]["anyOf"] == [ + {"$ref": "#/components/schemas/UserDirectory"}, + {"$ref": "#/components/schemas/UserDirectoryMinimal"}, + {"$ref": "#/components/schemas/UserDirectoryProjection"}, + ] + assert components["UserPreferenceRecordResponse"]["anyOf"] == [ + {"$ref": "#/components/schemas/UserPreferenceRecord"}, + {"$ref": "#/components/schemas/UserPreferenceRecordMinimal"}, + {"$ref": "#/components/schemas/UserPreferenceRecordProjection"}, + ] + assert sorted(components["UserDirectory"]["required"]) == [ + "email", "id", "name", "preferences", "url", "username", + ] + assert sorted(components["UserDirectoryMinimal"]["required"]) == ["email", "id", "username"] + assert sorted(components["UserPreferenceRecordMinimal"]["required"]) == ["key", "username", "value"] + for component in ("UserDirectoryProjection", "UserPreferenceRecordProjection"): + published = components[component] + assert "required" not in published, component + assert set(published["properties"]) == set( + components[component.replace("Projection", "")]["properties"] + ), component + + +@pytest.mark.django_db +@skip_unless_lms +def test_published_shapes_accept_every_body_the_endpoints_return(client, schema): + """Every preset and selection the parameters offer returns a body the document describes.""" + admin = AdminFactory.create(username="shape-admin", password=PASSWORD) + UserPreferenceFactory.create(user=admin, key="pref-lang", value="en") + assert client.login(username=admin.username, password=PASSWORD) + for published_path, address, params in SHAPE_CASES: + case = (address, params) + response = client.get(address, params) + assert response.status_code == 200, case + body = response.json() + if "results" in body: + assert body["count"] == 1, case + validator = jsonschema.Draft202012Validator(published_body_schema(schema, published_path)) + assert [error.message for error in validator.iter_errors(body)] == [], case + + +@skip_unless_lms +def test_sort_parameter_publishes_the_fields_it_accepts(schema): + """The sort allow-list and the order used when none is asked for are published.""" + for path, accepted, default in ( + ("/api/user/v2/users/", ["-date_joined", "-username", "date_joined", "username"], ["username"]), + ("/api/user/v2/user_preferences/", ["-key", "-username", "key", "username"], ["username", "key"]), + ): + parameter = next( + published for published in schema["paths"][path]["get"]["parameters"] + if published["name"] == "ordering" + ) + assert parameter["schema"]["items"]["enum"] == accepted, path + assert parameter["schema"]["default"] == default, path + + +@skip_unless_lms +def test_every_list_publishes_the_page_that_does_not_exist(schema): + """Asking for a page past the end of a list is answered 404, so each list documents it.""" + assert LIST_PATHS | MEMBER_PATHS == set(VERSION_TWO_PATHS) + documented = { + path: sorted(operation["responses"]) + for path, _method, operation in operations(schema) + if path in LIST_PATHS + } + assert documented == { + "/api/user/v2/users/": ["200", "400", "401", "403", "404"], + "/api/user/v2/user_preferences/": ["200", "400", "401", "403", "404"], + TIME_ZONE_LIST_PATH: ["200", "400", "404"], + } + + +@skip_unless_lms +def test_member_addresses_describe_the_parts_they_are_built_from(schema): + """Each part of a member address is published with its meaning and what its route matches.""" + username_schema = {"type": "string", "pattern": rf"^{settings.USERNAME_REGEX_PARTIAL}$"} + published = { + (path, parameter["name"]): parameter + for path, _method, operation in operations(schema) + if path in MEMBER_PATHS + for parameter in operation["parameters"] + if parameter["in"] == "path" + } + assert published == { + (USER_MEMBER_PATH, "username"): { + "in": "path", + "name": "username", + "required": True, + "schema": username_schema, + "description": "Unique name of the user whose directory entry is returned.", + }, + (PREFERENCE_MEMBER_PATH, "username"): { + "in": "path", + "name": "username", + "required": True, + "schema": username_schema, + "description": ( + "Username of the user the preference belongs to: the part of the address " + "before the comma." + ), + }, + (PREFERENCE_MEMBER_PATH, "key"): { + "in": "path", + "name": "key", + "required": True, + "schema": {"type": "string", "pattern": rf"^{PREFERENCE_KEY_PATTERN_PARTIAL}$"}, + "description": ( + "Name the preference is stored under: the part of the address after the comma." + ), + }, + } + + +@skip_unless_lms +def test_the_country_parameter_publishes_the_constraint_it_is_held_to(schema): + """The parameter comes from the serializer that validates it, so the two cannot disagree.""" + field = TimeZoneQuerySerializer().fields["country_code"] + enforced = next( + validator.regex.pattern for validator in field.validators if hasattr(validator, "regex") + ) + parameter = next( + published for published in schema["paths"][TIME_ZONE_LIST_PATH]["get"]["parameters"] + if published["name"] == "country_code" + ) + assert parameter["description"] == field.help_text + assert parameter["schema"] == { + "oneOf": [{"type": "string", "pattern": enforced}, {"type": "string", "maxLength": 0}], + } + assert "required" not in parameter + + +@skip_unless_lms +def test_published_text_is_written_for_the_clients_that_read_it(schema): + """Descriptions reach every consumer, so they carry no governance references.""" + published = json.dumps({ + "paths": {path: schema["paths"][path] for path in VERSION_TWO_PATHS}, + "components": { + name: schema["components"]["schemas"][name] for name in VERSION_TWO_COMPONENTS + }, + }) + for reference in ("ADR", "OEP-", "drf-spectacular", "extend_schema"): + assert reference not in published, reference + assert schema["components"]["schemas"]["ErrorResponse"]["description"].startswith( + "The body of every unsuccessful response." + ) diff --git a/openedx/core/djangoapps/user_api/v2/tests/test_url_mount.py b/openedx/core/djangoapps/user_api/v2/tests/test_url_mount.py new file mode 100644 index 000000000000..019d3c535434 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/tests/test_url_mount.py @@ -0,0 +1,68 @@ +"""Addresses of other apps that share the prefix version 2 of the User API is mounted on.""" + +from django.test import SimpleTestCase +from django.urls import resolve, reverse + +from openedx.core.djangolib.testing.utils import skip_unless_lms + +AUTHN_LOGIN_VIEWS = "openedx.core.djangoapps.user_authn.views.login" +AUTHN_REGISTER_VIEWS = "openedx.core.djangoapps.user_authn.views.register" + + +@skip_unless_lms +class SharedPrefixTests(SimpleTestCase): + """ + The authentication app keeps the two addresses it serves under this prefix. + + The mount for this version is matched before the include that carries the + rest of the user surface and before the authentication app's own patterns, + so it is first in line for every address that begins with + ``/api/user/v2/``. Those two addresses reach the authentication app only + because no pattern here matches them first. + """ + + def assert_resolves_to(self, address, module, class_name, url_name, kwargs=None): + """Assert an address reaches the named view through the named route.""" + match = resolve(address) + view_class = match.func.view_class + assert (view_class.__module__, view_class.__name__) == (module, class_name), address + assert match.url_name == url_name, address + assert match.kwargs == (kwargs or {}), address + + def test_registration_still_reaches_the_authentication_app(self): + self.assert_resolves_to( + "/api/user/v2/account/registration/", + AUTHN_REGISTER_VIEWS, + "RegistrationView", + "user_api_registration_v2", + ) + + def test_login_session_still_reaches_the_authentication_app(self): + self.assert_resolves_to( + "/api/user/v2/account/login_session/", + AUTHN_LOGIN_VIEWS, + "LoginSessionView", + "user_api_login_session", + {"api_version": "v2"}, + ) + + def test_the_version_one_addresses_beside_them_are_unaffected(self): + self.assert_resolves_to( + "/api/user/v1/account/registration/", + AUTHN_REGISTER_VIEWS, + "RegistrationView", + "user_api_registration", + ) + self.assert_resolves_to( + "/api/user/v1/account/login_session/", + AUTHN_LOGIN_VIEWS, + "LoginSessionView", + "user_api_login_session", + {"api_version": "v1"}, + ) + + def test_both_shared_prefix_addresses_still_reverse_to_themselves(self): + assert reverse("user_api_registration_v2") == "/api/user/v2/account/registration/" + assert reverse( + "user_api_login_session", kwargs={"api_version": "v2"}, + ) == "/api/user/v2/account/login_session/" diff --git a/openedx/core/djangoapps/user_api/v2/tests/test_urls.py b/openedx/core/djangoapps/user_api/v2/tests/test_urls.py new file mode 100644 index 000000000000..c51ceab78a13 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/tests/test_urls.py @@ -0,0 +1,296 @@ +"""Tests for the version 2 User API URL surface.""" + +import re + +import pytest +from django.core.exceptions import ImproperlyConfigured +from django.urls import get_resolver, resolve, reverse +from edx_rest_framework_extensions.testing import assert_error_envelope +from rest_framework import status +from rest_framework.request import Request +from rest_framework.test import APIRequestFactory, APITestCase + +from common.djangoapps.student.tests.factories import AdminFactory +from openedx.core.djangoapps.user_api.v2 import urls as v2_urls +from openedx.core.djangoapps.user_api.v2.views import UserPreferenceViewSet, UserViewSet +from openedx.core.djangolib.testing.utils import skip_unless_lms + +PASSWORD = "Password1234" + +V2_VIEW_MODULE = "openedx.core.djangoapps.user_api.v2.views" +V1_VIEW_MODULE = "openedx.core.djangoapps.user_api.views" +V2_NAMESPACE = "user_v2" + +#: A route name is lower case, starts with a letter and separates words with +#: underscores; nothing else is accepted. +SNAKE_CASE_NAME = re.compile(r"[a-z][a-z0-9_]*\Z") +VERSION_IN_NAME = re.compile(r"v\d") + +EXPECTED_ADDRESSES = { + "user_v2:user_list": ({}, "/api/user/v2/users/"), + "user_v2:user_detail": ({"username": "alice"}, "/api/user/v2/users/alice/"), + "user_v2:user_preference_list": ({}, "/api/user/v2/user_preferences/"), + "user_v2:user_preference_detail": ( + {"username": "alice", "key": "time_zone"}, + "/api/user/v2/user_preferences/alice,time_zone/", + ), + "user_v2:time_zone_list": ({}, "/api/user/v2/time_zones/"), +} + + +def registered_route_names(): + """ + Return the route names the mounted URLconf registers under this namespace. + + These are the names ``reverse()`` accepts; they are read from the resolver + rather than from this module so that a route added or renamed in the + URLconf is seen here. + """ + _prefix, namespace_resolver = get_resolver().namespace_dict[V2_NAMESPACE] + return {key for key in namespace_resolver.reverse_dict if isinstance(key, str)} + + +def resolved_route_names(): + """Return the route names ``resolve()`` reports for the published addresses.""" + return {resolve(address).url_name for _kwargs, address in EXPECTED_ADDRESSES.values()} + + +def routed_actions(view_func): + """ + Return the method-to-action map a viewset mount was registered with. + + DRF adds a ``head`` entry to a mount's ``actions`` the first time it serves + a GET, so the raw attribute depends on what the test suite requested first. + """ + return {method: action for method, action in view_func.actions.items() if method != "head"} + + +def view_identity(view_func): + """ + Return the module and class name of the view a route is mounted on. + + A viewset mount carries the class name on the wrapper itself; Django's + plain class-based views record the class separately instead. + """ + view_class = getattr(view_func, "view_class", None) + name = view_class.__name__ if view_class is not None else view_func.__name__ + return (view_func.__module__, name) + + +@skip_unless_lms +class UrlLiteralTests(APITestCase): + """Every name reverses to the address the API publishes.""" + + def test_reverse_literals(self): + for name, (kwargs, expected) in EXPECTED_ADDRESSES.items(): + assert reverse(name, kwargs=kwargs) == expected, name + + def test_names_are_snake_case_and_carry_no_version(self): + registered = registered_route_names() + assert registered == {name.split(":")[1] for name in EXPECTED_ADDRESSES} + for name in registered: + assert SNAKE_CASE_NAME.match(name), name + assert not VERSION_IN_NAME.search(name), name + + def test_resolved_names_are_the_names_that_reverse(self): + resolved = resolved_route_names() + assert resolved == registered_route_names() + for address, name in ( + ("/api/user/v2/users/", "user_list"), + ("/api/user/v2/users/alice/", "user_detail"), + ("/api/user/v2/user_preferences/", "user_preference_list"), + ("/api/user/v2/user_preferences/alice,time_zone/", "user_preference_detail"), + ("/api/user/v2/time_zones/", "time_zone_list"), + ): + match = resolve(address) + assert match.url_name == name, address + assert match.view_name == f"{V2_NAMESPACE}:{name}", address + + def test_viewsets_build_action_addresses_under_this_mount(self): + assert UserViewSet(basename="user").reverse_action("list") == "/api/user/v2/users/" + assert UserViewSet(basename="user").reverse_action( + "detail", kwargs={"username": "alice"}, + ) == "/api/user/v2/users/alice/" + preferences = UserPreferenceViewSet(basename="user_preference") + assert preferences.reverse_action("list") == "/api/user/v2/user_preferences/" + assert preferences.reverse_action( + "detail", kwargs={"username": "alice", "key": "time_zone"}, + ) == "/api/user/v2/user_preferences/alice,time_zone/" + + def test_action_addresses_follow_the_namespace_the_request_arrived_on(self): + request = Request(APIRequestFactory().get("/api/user/v2/users/")) + request.resolver_match = resolve("/api/user/v2/users/") + view = UserViewSet(basename="user", request=request) + assert view.reverse_action("list") == "http://testserver/api/user/v2/users/" + + def test_addresses_resolve_to_the_version_two_viewsets(self): + expected = { + "/api/user/v2/users/": ("UserViewSet", {"get": "list"}), + "/api/user/v2/users/alice/": ("UserViewSet", {"get": "retrieve"}), + "/api/user/v2/user_preferences/": ("UserPreferenceViewSet", {"get": "list"}), + "/api/user/v2/user_preferences/alice,time_zone/": ( + "UserPreferenceViewSet", {"get": "retrieve"}, + ), + "/api/user/v2/time_zones/": ("CountryTimeZoneViewSet", {"get": "list"}), + } + for address, (class_name, actions) in expected.items(): + match = resolve(address) + assert view_identity(match.func) == (V2_VIEW_MODULE, class_name), address + assert routed_actions(match.func) == actions, address + + def test_username_with_a_dot_resolves_to_the_member_route(self): + match = resolve("/api/user/v2/users/ada.lovelace/") + assert view_identity(match.func) == (V2_VIEW_MODULE, "UserViewSet") + assert match.kwargs == {"username": "ada.lovelace"} + + def test_preference_member_address_splits_into_username_and_key(self): + match = resolve("/api/user/v2/user_preferences/ada.lovelace,pref-lang/") + assert match.kwargs == {"username": "ada.lovelace", "key": "pref-lang"} + + def test_a_key_containing_a_comma_belongs_to_the_key(self): + """A preference name is free-form text, and the username half cannot contain a comma.""" + match = resolve("/api/user/v2/user_preferences/ada.lovelace,a,b/") + assert match.kwargs == {"username": "ada.lovelace", "key": "a,b"} + assert reverse( + "user_v2:user_preference_detail", kwargs={"username": "ada.lovelace", "key": "a,b"}, + ) == "/api/user/v2/user_preferences/ada.lovelace,a,b/" + + def test_a_username_outside_the_allowed_pattern_does_not_reach_the_viewset(self): + match = resolve("/api/user/v2/user_preferences/al!ce,pref-lang/") + assert view_identity(match.func) == (V2_VIEW_MODULE, "UnknownAddressView") + + def test_no_database_id_member_address_is_served(self): + match = resolve("/api/user/v2/user_preferences/42/") + assert view_identity(match.func) == (V2_VIEW_MODULE, "UnknownAddressView") + + +@skip_unless_lms +class RouteShapeTests(APITestCase): + """ + Every address this version serves is named and ends in a slash. + + The mount also carries one pattern that is neither: the fallback matched + after all of them, which answers an address no route serves. It addresses + no resource and reverses to nothing, which is why it has no name, and + pinning it here keeps a second wildcard from being added quietly beside + it. + """ + + def test_every_address_is_named_and_ends_in_a_slash(self): + addresses = [pattern for pattern in v2_urls.urlpatterns if pattern.name] + assert sorted(pattern.name for pattern in addresses) == [ + "time_zone_list", + "user_detail", + "user_list", + "user_preference_detail", + "user_preference_list", + ] + for pattern in addresses: + assert SNAKE_CASE_NAME.match(pattern.name), pattern.name + assert str(pattern.pattern).endswith("/$"), pattern.name + + def test_the_router_route_that_is_replaced_has_to_exist(self): + """Dropping a route the router does not generate is refused instead of passing silently.""" + routes = v2_urls.router.urls + assert v2_urls.REPLACED_ROUTER_ROUTE in {route.name for route in routes} + kept = v2_urls.without_route(routes, v2_urls.REPLACED_ROUTER_ROUTE) + assert [route.name for route in kept] == [ + "user_list", + "user_detail", + "user_preference_list", + "time_zone_list", + ] + with pytest.raises(ImproperlyConfigured): + v2_urls.without_route(routes, "user_preference_detail_renamed") + + def test_the_one_unnamed_pattern_is_the_fallback_and_it_is_matched_last(self): + unnamed = [pattern for pattern in v2_urls.urlpatterns if not pattern.name] + assert len(unnamed) == 1 + assert unnamed[0] is v2_urls.urlpatterns[-1] + assert view_identity(unnamed[0].callback) == (V2_VIEW_MODULE, "UnknownAddressView") + + +@skip_unless_lms +class LegacyAddressTests(APITestCase): + """Mounting version 2 leaves the version 1 surface exactly where it was.""" + + def test_version_one_addresses_still_reach_the_version_one_views(self): + expected = { + "/api/user/v1/users/": "UserViewSet", + "/api/user/v1/users/42/": "UserViewSet", + "/api/user/v1/user_prefs/": "UserPreferenceViewSet", + "/api/user/v1/user_prefs/42/": "UserPreferenceViewSet", + "/api/user/v1/preferences/pref-lang/users/": "PreferenceUsersListView", + "/api/user/v1/forum_roles/Moderator/users/": "ForumRoleUsersListView", + "/api/user/v1/preferences/time_zones/": "CountryTimeZoneListView", + } + for address, class_name in expected.items(): + match = resolve(address) + assert view_identity(match.func) == (V1_VIEW_MODULE, class_name), address + + def test_version_one_url_names_still_reverse(self): + assert reverse("accounts_api", kwargs={"username": "alice"}) == "/api/user/v1/accounts/alice" + assert reverse("own_username_api") == "/api/user/v1/me" + assert reverse("preferences_api", kwargs={"username": "alice"}) == "/api/user/v1/preferences/alice" + + def test_unversioned_twins_still_resolve(self): + match = resolve("/user_api/v1/users/") + assert view_identity(match.func) == (V1_VIEW_MODULE, "UserViewSet") + + +@skip_unless_lms +class MalformedAddressTests(APITestCase): + """An address that matches no route is refused in the body shape this API answers with.""" + + def setUp(self): + super().setUp() + self.admin = AdminFactory.create(username="url-admin", password=PASSWORD) + assert self.client.login(username=self.admin.username, password=PASSWORD) + + def assert_refused_in_json(self, response): + """Assert the refusal is the standard error body and not a page meant for a browser.""" + assert response["Content-Type"].startswith("application/json"), response["Content-Type"] + assert "\w+)", + re.IGNORECASE, +) + +#: The platform's ``SafeSessionMiddleware`` puts the signed-in caller's e-mail +#: into the session the first time it finds it missing, which rewrites that one +#: row once per session; the deployed configuration keeps sessions in the cache +#: and only the test settings put them in a table at all. It is framework +#: bookkeeping rather than a change to the data this API serves, and it is the +#: only table left out of the captures below. +SESSION_TABLE = "django_session" + + +def write_target(sql): + """Return the table a statement writes to, or ``None`` if it cannot be read.""" + match = WRITE_TARGET.match(sql) + return match.group("table") if match else None + + +def write_queries(captured): + """ + Return the captured statements that would change the data this API serves. + + Only the statement's own target table decides: a write to a domain table is + reported however it mentions the session table elsewhere, and a write whose + target cannot be read is reported rather than assumed harmless. + """ + return [ + query["sql"] + for query in captured + if WRITE_VERB.match(query["sql"]) and write_target(query["sql"]) != SESSION_TABLE + ] + + +class WriteCaptureFilterTests(SimpleTestCase): + """The capture filter reports writes by their target table and nothing else.""" + + PREFERENCE_INSERT = ( + 'INSERT INTO "user_api_userpreference" ("user_id", "key", "value") ' + "VALUES (1, 'pref-lang', 'django_session')" + ) + PREFERENCE_DELETE = ( + 'DELETE FROM "user_api_userpreference" WHERE "user_id" IN ' + '(SELECT "user_id" FROM "django_session" WHERE "session_key" = \'abc\')' + ) + ROLE_INSERT = 'INSERT INTO "django_comment_common_role" ("name") VALUES (\'Moderator\')' + SESSION_UPDATE = 'UPDATE "django_session" SET "session_data" = \'x\' WHERE "session_key" = \'abc\'' + SESSION_INSERT = 'INSERT INTO "django_session" ("session_key", "session_data") VALUES (\'abc\', \'x\')' + SESSION_DELETE = 'DELETE FROM "django_session" WHERE "session_key" = \'abc\'' + USER_SELECT = 'SELECT "auth_user"."id" FROM "auth_user" ORDER BY "auth_user"."username" ASC' + UNREADABLE_WRITE = 'UPDATE /* hint */ "user_api_userpreference" SET "value" = \'en\'' + + def captured(self, *statements): + """Return the statements in the shape ``CaptureQueriesContext`` records them.""" + return [{"sql": statement, "time": "0.000"} for statement in statements] + + def test_domain_writes_are_reported_even_when_they_name_the_session_table(self): + reported = write_queries( + self.captured(self.PREFERENCE_INSERT, self.PREFERENCE_DELETE, self.ROLE_INSERT), + ) + assert reported == [self.PREFERENCE_INSERT, self.PREFERENCE_DELETE, self.ROLE_INSERT] + + def test_session_bookkeeping_is_the_only_statement_dropped(self): + reported = write_queries( + self.captured( + self.SESSION_INSERT, + self.SESSION_UPDATE, + self.SESSION_DELETE, + self.USER_SELECT, + self.ROLE_INSERT, + ), + ) + assert reported == [self.ROLE_INSERT] + + def test_reads_are_not_writes(self): + assert write_queries(self.captured(self.USER_SELECT)) == [] + + def test_a_write_whose_target_cannot_be_read_is_still_reported(self): + assert write_target(self.UNREADABLE_WRITE) is None + assert write_queries(self.captured(self.UNREADABLE_WRITE)) == [self.UNREADABLE_WRITE] + + def test_the_target_table_is_read_through_any_backend_quoting(self): + assert write_target(self.ROLE_INSERT) == "django_comment_common_role" + assert write_target("UPDATE `django_session` SET `session_data` = 'x'") == SESSION_TABLE + assert write_target(" insert into user_api_userpreference (value) values ('x')") == ( + "user_api_userpreference" + ) + assert write_target("INSERT OR IGNORE INTO `user_api_userpreference` (`value`) VALUES ('en')") == ( + "user_api_userpreference" + ) + assert write_target(self.USER_SELECT) is None + + +class UserApiV2TestCase(APITestCase): + """Fixtures shared by the version 2 User API tests.""" + + def setUp(self): + super().setUp() + self.admin = AdminFactory.create(username="admin-caller", password=PASSWORD) + self.learner = UserFactory.create(username="learner-caller", password=PASSWORD) + self.alice = UserFactory.create(username="alice", email="alice@example.com", password=PASSWORD) + self.bob = UserFactory.create(username="bob", email="bob@example.com", password=PASSWORD) + UserPreferenceFactory.create(user=self.alice, key="pref-lang", value="en") + UserPreferenceFactory.create(user=self.bob, key="time_zone", value="UTC") + + def login(self, user): + """Sign the given user in through the session the platform default accepts.""" + assert self.client.login(username=user.username, password=PASSWORD) + + def usernames_in(self, response): + """Return the usernames carried by a paginated directory page.""" + return [row["username"] for row in response.data["results"]] + + +@skip_unless_lms +class UserListViewTests(UserApiV2TestCase): + """GET /api/user/v2/users/""" + + def setUp(self): + super().setUp() + self.url = reverse("user_v2:user_list") + + def test_anonymous_caller_is_unauthenticated(self): + response = self.client.get(self.url) + assert_error_envelope(response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn") + + def test_non_admin_caller_is_forbidden(self): + self.login(self.learner) + response = self.client.get(self.url) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_non_admin_cannot_list_another_users_preference_holders(self): + self.login(self.learner) + response = self.client.get(self.url, {"preference_key": "pref-lang"}) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_admin_caller_receives_the_directory(self): + self.login(self.admin) + response = self.client.get(self.url, {"page_size": 100}) + assert response.status_code == status.HTTP_200_OK + assert set(response.data) == PAGINATION_KEYS + assert self.usernames_in(response) == ["admin-caller", "alice", "bob", "learner-caller"] + assert response.data["count"] == 4 + assert response.data["num_pages"] == 1 + assert response.data["current_page"] == 1 + assert response.data["start"] == 0 + + def test_row_shape_matches_the_declared_fields(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice"}) + row = response.data["results"][0] + assert set(row) == {"id", "url", "email", "name", "username", "preferences"} + assert row["id"] == self.alice.id + assert row["email"] == "alice@example.com" + assert row["name"] == self.alice.profile.name + assert row["preferences"] == {"pref-lang": "en"} + assert row["url"].endswith("/api/user/v2/users/alice/") + + def test_second_page_reports_its_position(self): + self.login(self.admin) + response = self.client.get(self.url, {"page_size": 2, "page": 2}) + assert response.status_code == status.HTTP_200_OK + assert self.usernames_in(response) == ["bob", "learner-caller"] + assert response.data["current_page"] == 2 + assert response.data["num_pages"] == 2 + assert response.data["start"] == 2 + assert response.data["previous"] is not None + assert response.data["next"] is None + + def test_a_page_beyond_the_last_one_is_not_found(self): + self.login(self.admin) + response = self.client.get(self.url, {"page": 9999}) + assert_error_envelope(response, expected_status=status.HTTP_404_NOT_FOUND, expected_type_slug="not-found") + + def test_a_page_number_that_is_not_a_number_is_not_found(self): + self.login(self.admin) + response = self.client.get(self.url, {"page": "abc"}) + assert_error_envelope(response, expected_status=status.HTTP_404_NOT_FOUND, expected_type_slug="not-found") + + def test_ordering_can_be_reversed(self): + self.login(self.admin) + response = self.client.get(self.url, {"ordering": "-username", "page_size": 100}) + assert self.usernames_in(response) == ["learner-caller", "bob", "alice", "admin-caller"] + + def test_ordering_by_join_date(self): + self.alice.date_joined = datetime(2020, 1, 1, tzinfo=UTC) + self.alice.save() + self.bob.date_joined = datetime(2019, 1, 1, tzinfo=UTC) + self.bob.save() + self.login(self.admin) + response = self.client.get(self.url, {"ordering": "date_joined", "page_size": 100}) + assert self.usernames_in(response)[-2:] == ["bob", "alice"] + + def test_unknown_ordering_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url, {"ordering": "password", "page_size": 100}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "ordering": ["Sort by any of: date_joined, username, each optionally prefixed with '-'."], + } + + def test_one_unknown_field_refuses_the_whole_sort(self): + self.login(self.admin) + response = self.client.get(self.url, {"ordering": "username,password", "page_size": 100}) + assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + + def test_filter_by_email(self): + self.login(self.admin) + response = self.client.get(self.url, {"email": "bob@example.com"}) + assert self.usernames_in(response) == ["bob"] + + def test_filter_by_is_active(self): + self.alice.is_active = False + self.alice.save() + self.login(self.admin) + response = self.client.get(self.url, {"is_active": "false", "page_size": 100}) + assert self.usernames_in(response) == ["alice"] + + def test_filter_by_is_active_keeps_only_the_active_accounts(self): + self.alice.is_active = False + self.alice.save() + self.login(self.admin) + response = self.client.get(self.url, {"is_active": "true", "page_size": 100}) + assert self.usernames_in(response) == ["admin-caller", "bob", "learner-caller"] + + def test_an_is_active_value_that_names_no_side_is_refused(self): + """A mistyped value must be refused, not read as "no preference" and answered unfiltered.""" + self.alice.is_active = False + self.alice.save() + self.login(self.admin) + response = self.client.get(self.url, {"is_active": "maybe", "page_size": 100}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == {"is_active": ["Enter true or false."]} + + def test_an_empty_is_active_value_narrows_nothing(self): + self.alice.is_active = False + self.alice.save() + self.login(self.admin) + response = self.client.get(self.url, {"is_active": "", "page_size": 100}) + assert response.status_code == status.HTTP_200_OK + assert self.usernames_in(response) == ["admin-caller", "alice", "bob", "learner-caller"] + + def test_filter_by_preference_key(self): + self.login(self.admin) + response = self.client.get(self.url, {"preference_key": "pref-lang", "page_size": 100}) + assert self.usernames_in(response) == ["alice"] + + def test_filter_by_unknown_preference_key_returns_no_rows(self): + self.login(self.admin) + response = self.client.get(self.url, {"preference_key": "nope", "page_size": 100}) + assert response.data["results"] == [] + assert response.data["count"] == 0 + + def give_alice_the_moderator_role(self): + """Make alice a moderator of the course the forum-role tests narrow by.""" + role = Role.objects.create(course_id=COURSE_KEY, name="Moderator") + role.users.add(self.alice) + return role + + def test_filter_by_forum_role_and_course(self): + self.give_alice_the_moderator_role() + other_role = Role.objects.create(course_id=OTHER_COURSE_KEY, name="Moderator") + other_role.users.add(self.bob) + self.login(self.admin) + response = self.client.get( + self.url, {"forum_role": "Moderator", "course_key": str(COURSE_KEY), "page_size": 100}, + ) + assert self.usernames_in(response) == ["alice"] + + def test_anonymous_caller_cannot_list_a_courses_forum_role_holders(self): + self.give_alice_the_moderator_role() + response = self.client.get( + self.url, {"forum_role": "Moderator", "course_key": str(COURSE_KEY)}, + ) + assert_error_envelope(response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn") + + def test_non_admin_cannot_list_a_courses_forum_role_holders(self): + self.give_alice_the_moderator_role() + self.login(self.learner) + response = self.client.get( + self.url, {"forum_role": "Moderator", "course_key": str(COURSE_KEY)}, + ) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_holding_the_role_does_not_let_a_learner_list_its_holders(self): + self.give_alice_the_moderator_role() + self.login(self.alice) + response = self.client.get( + self.url, {"forum_role": "Moderator", "course_key": str(COURSE_KEY)}, + ) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_the_published_course_key_example_can_be_pasted_into_a_query_string(self): + """A caller that copies the documented key into a query string reaches the role holders.""" + example = re.search( + r"for example (course-v1:[^\s.]+)", + UserFilterSet.base_filters["course_key"].extra["help_text"], + ).group(1) + role = Role.objects.create(course_id=COURSE_KEY, name="Moderator") + role.users.add(self.alice) + self.login(self.admin) + response = self.client.get(f"{self.url}?forum_role=Moderator&course_key={example}") + assert response.status_code == status.HTTP_200_OK + assert self.usernames_in(response) == ["alice"] + + def test_a_course_key_whose_plus_signs_are_not_encoded_is_refused(self): + """An unencoded plus sign arrives as a space, which no course key may contain.""" + self.login(self.admin) + response = self.client.get( + f"{self.url}?forum_role=Moderator&course_key=course-v1:edX+DemoX+Demo", + ) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == {"course_key": ["Enter a valid course key."]} + + def test_a_user_holding_the_role_through_several_rows_is_listed_once(self): + """Nothing stops a course from carrying one role name under several rows.""" + for _ in range(2): + role = Role.objects.create(course_id=COURSE_KEY, name="Moderator") + role.users.add(self.alice) + self.login(self.admin) + response = self.client.get( + self.url, {"forum_role": "Moderator", "course_key": str(COURSE_KEY), "page_size": 100}, + ) + assert self.usernames_in(response) == ["alice"] + assert response.data["count"] == 1 + + def test_forum_role_without_course_key_is_rejected(self): + self.login(self.admin) + response = self.client.get(self.url, {"forum_role": "Moderator"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "course_key": ["This parameter is required when forum_role is supplied."], + } + + def test_course_key_without_forum_role_is_rejected(self): + self.login(self.admin) + response = self.client.get(self.url, {"course_key": str(COURSE_KEY)}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "forum_role": ["This parameter is required when course_key is supplied."], + } + + def test_malformed_course_key_is_rejected(self): + self.login(self.admin) + response = self.client.get(self.url, {"forum_role": "Moderator", "course_key": "not a course"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == {"course_key": ["Enter a valid course key."]} + + def test_unknown_forum_role_returns_no_rows_and_creates_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + response = self.client.get( + self.url, {"forum_role": "Inquisitor", "course_key": str(COURSE_KEY)}, + ) + assert response.status_code == status.HTTP_200_OK + assert response.data["results"] == [] + assert Role.objects.filter(name="Inquisitor").count() == 0 + assert write_queries(captured.captured_queries) == [] + + def test_minimal_view_returns_only_identifying_fields(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice", "view": "minimal"}) + row = response.data["results"][0] + assert set(row) == {"id", "username", "email"} + assert row["username"] == "alice" + + def test_fields_parameter_projects_the_row(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice", "fields": "username,email"}) + assert response.data["results"] == [{"username": "alice", "email": "alice@example.com"}] + + def test_fields_parameter_narrows_the_minimal_view(self): + self.login(self.admin) + response = self.client.get( + self.url, {"username": "alice", "view": "minimal", "fields": "username"}, + ) + assert response.data["results"] == [{"username": "alice"}] + + def test_a_field_name_the_row_does_not_carry_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url, {"fields": "user_name"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "fields": [ + "No such field: user_name. " + "Select any of: email, id, name, preferences, url, username.", + ], + } + + def test_a_field_the_minimal_preset_drops_is_refused(self): + """The preset decides what the row carries, so the selection is judged against it.""" + self.login(self.admin) + response = self.client.get(self.url, {"view": "minimal", "fields": "name"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "fields": ["No such field: name. Select any of: email, id, username."], + } + + def test_every_unknown_field_name_is_named_at_once(self): + self.login(self.admin) + response = self.client.get(self.url, {"fields": "username,e_mail,joined"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"]["fields"][0].startswith("No such field: e_mail, joined.") + + def test_a_field_name_with_surrounding_space_is_accepted(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice", "fields": " username , email "}) + assert response.data["results"] == [{"username": "alice", "email": "alice@example.com"}] + + def test_an_unknown_field_does_not_answer_an_anonymous_caller(self): + response = self.client.get(self.url, {"fields": "user_name"}) + assert_error_envelope(response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn") + + def test_an_unknown_response_preset_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url, {"view": "nonsense"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "view": ["Ask for any of: minimal, or omit this parameter for the default response."], + } + + def test_the_preset_that_belongs_to_the_preference_list_is_refused_here(self): + self.login(self.admin) + response = self.client.get(self.url, {"view": "full"}) + assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + + def test_an_empty_response_preset_returns_the_default_shape(self): + """A query string built from an empty variable narrows nothing rather than failing.""" + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice", "view": ""}) + assert response.status_code == status.HTTP_200_OK + assert set(response.data["results"][0]) == { + "id", "url", "email", "name", "username", "preferences", + } + + def test_an_unknown_preset_does_not_answer_an_anonymous_caller(self): + response = self.client.get(self.url, {"view": "nonsense"}) + assert_error_envelope(response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn") + + def test_listing_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + self.client.get(self.url, {"page_size": 100}) + assert write_queries(captured.captured_queries) == [] + + def test_head_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + response = self.client.head(self.url) + assert response.status_code == status.HTTP_200_OK + assert write_queries(captured.captured_queries) == [] + + +@skip_unless_lms +class UserDetailViewTests(UserApiV2TestCase): + """GET /api/user/v2/users/{username}/""" + + def url_for(self, username): + """Return the member address of the named user.""" + return reverse("user_v2:user_detail", kwargs={"username": username}) + + def test_anonymous_caller_is_unauthenticated(self): + response = self.client.get(self.url_for("alice")) + assert_error_envelope(response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn") + + def test_non_admin_caller_is_forbidden(self): + self.login(self.learner) + response = self.client.get(self.url_for("alice")) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_owner_who_is_not_an_admin_is_forbidden(self): + self.login(self.alice) + response = self.client.get(self.url_for("alice")) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_admin_caller_receives_the_entry(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice")) + assert response.status_code == status.HTTP_200_OK + assert response.data["username"] == "alice" + assert response.data["email"] == "alice@example.com" + assert response.data["preferences"] == {"pref-lang": "en"} + + def test_unknown_username_is_not_found(self): + self.login(self.admin) + response = self.client.get(self.url_for("nobody")) + assert_error_envelope(response, expected_status=status.HTTP_404_NOT_FOUND, expected_type_slug="not-found") + + def test_username_containing_a_dot_resolves(self): + UserFactory.create(username="ada.lovelace", email="ada@example.com", password=PASSWORD) + self.login(self.admin) + response = self.client.get(self.url_for("ada.lovelace")) + assert response.status_code == status.HTTP_200_OK + assert response.data["username"] == "ada.lovelace" + + def test_minimal_view_returns_only_identifying_fields(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice"), {"view": "minimal"}) + assert set(response.data) == {"id", "username", "email"} + + def test_a_field_name_the_entry_does_not_carry_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice"), {"fields": "user_name"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "fields": [ + "No such field: user_name. " + "Select any of: email, id, name, preferences, url, username.", + ], + } + + def test_fields_parameter_projects_the_entry(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice"), {"fields": "username"}) + assert response.data == {"username": "alice"} + + def test_an_unknown_response_preset_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice"), {"view": "nonsense"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "view": ["Ask for any of: minimal, or omit this parameter for the default response."], + } + + def test_retrieval_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + self.client.get(self.url_for("alice")) + assert write_queries(captured.captured_queries) == [] + + def test_head_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + response = self.client.head(self.url_for("alice")) + assert response.status_code == status.HTTP_200_OK + assert write_queries(captured.captured_queries) == [] + + +@skip_unless_lms +class ProfilelessUserTests(UserApiV2TestCase): + """A user with no profile row is listed with a null name rather than failing.""" + + def test_user_without_a_profile_has_a_null_name(self): + User.objects.create_user("profileless", "profileless@example.com", PASSWORD) + self.login(self.admin) + response = self.client.get( + reverse("user_v2:user_detail", kwargs={"username": "profileless"}), + ) + assert response.status_code == status.HTTP_200_OK + assert response.data["name"] is None + + +@skip_unless_lms +class UserPreferenceListViewTests(UserApiV2TestCase): + """GET /api/user/v2/user_preferences/""" + + def setUp(self): + super().setUp() + self.url = reverse("user_v2:user_preference_list") + + def keys_in(self, response): + """Return the (username, key) pairs carried by a page of preferences.""" + return [(row["username"], row["key"]) for row in response.data["results"]] + + def test_anonymous_caller_is_unauthenticated(self): + response = self.client.get(self.url) + assert_error_envelope(response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn") + + def test_non_admin_caller_is_forbidden(self): + self.login(self.learner) + response = self.client.get(self.url) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_owner_who_is_not_an_admin_is_forbidden(self): + self.login(self.alice) + response = self.client.get(self.url, {"username": "alice"}) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_admin_caller_receives_every_preference(self): + self.login(self.admin) + response = self.client.get(self.url, {"page_size": 100}) + assert response.status_code == status.HTTP_200_OK + assert set(response.data) == PAGINATION_KEYS + assert self.keys_in(response) == [("alice", "pref-lang"), ("bob", "time_zone")] + + def test_row_shape_is_flat_by_default(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice"}) + row = response.data["results"][0] + assert set(row) == {"username", "key", "value", "url", "user"} + assert row["username"] == "alice" + assert row["key"] == "pref-lang" + assert row["value"] == "en" + assert row["user"] is None + assert row["url"].endswith("/api/user/v2/user_preferences/alice,pref-lang/") + + def test_full_view_embeds_the_owning_user(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice", "view": "full"}) + row = response.data["results"][0] + assert set(row["user"]) == {"id", "url", "email", "name", "username", "preferences"} + assert row["user"]["username"] == "alice" + assert row["user"]["preferences"] == {"pref-lang": "en"} + + def test_minimal_view_drops_the_address_and_owner(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice", "view": "minimal"}) + assert response.data["results"] == [{"username": "alice", "key": "pref-lang", "value": "en"}] + + def test_a_field_name_the_row_does_not_carry_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url, {"fields": "preference_key"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "fields": [ + "No such field: preference_key. " + "Select any of: key, url, user, username, value.", + ], + } + + def test_a_page_beyond_the_last_one_is_not_found(self): + self.login(self.admin) + response = self.client.get(self.url, {"page": 9999}) + assert_error_envelope(response, expected_status=status.HTTP_404_NOT_FOUND, expected_type_slug="not-found") + + def test_fields_parameter_projects_the_row(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice", "fields": "key,value"}) + assert response.data["results"] == [{"key": "pref-lang", "value": "en"}] + + def test_filter_by_key(self): + UserPreferenceFactory.create(user=self.bob, key="pref-lang", value="fr") + self.login(self.admin) + response = self.client.get(self.url, {"key": "pref-lang", "page_size": 100}) + assert self.keys_in(response) == [("alice", "pref-lang"), ("bob", "pref-lang")] + + def test_filter_by_username(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "bob", "page_size": 100}) + assert self.keys_in(response) == [("bob", "time_zone")] + + def test_the_owner_id_the_earlier_list_took_is_refused_rather_than_ignored(self): + """A caller that ported by changing the version segment is told, not quietly unfiltered.""" + self.login(self.admin) + response = self.client.get(self.url, {"user": self.alice.id, "page_size": 100}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "user": [f"No such query parameter. Send any of: {PREFERENCE_QUERY_PARAMS}."], + } + + def test_an_answered_page_announces_no_deprecated_parameter(self): + self.login(self.admin) + response = self.client.get(self.url, {"username": "alice"}) + assert response.status_code == status.HTTP_200_OK + assert "Deprecation" not in response + + def test_ordering_by_key_descending(self): + self.login(self.admin) + response = self.client.get(self.url, {"ordering": "-key", "page_size": 100}) + assert self.keys_in(response) == [("bob", "time_zone"), ("alice", "pref-lang")] + + def test_ordering_takes_the_fields_in_the_order_they_are_named(self): + UserPreferenceFactory.create(user=self.bob, key="pref-lang", value="fr") + self.login(self.admin) + response = self.client.get(self.url, {"ordering": "key,-username", "page_size": 100}) + assert self.keys_in(response) == [ + ("bob", "pref-lang"), ("alice", "pref-lang"), ("bob", "time_zone"), + ] + + def test_unknown_ordering_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url, {"ordering": "value", "page_size": 100}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "ordering": ["Sort by any of: key, username, each optionally prefixed with '-'."], + } + + def test_listing_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + self.client.get(self.url, {"page_size": 100}) + assert write_queries(captured.captured_queries) == [] + + def test_full_view_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + self.client.get(self.url, {"page_size": 100, "view": "full"}) + assert write_queries(captured.captured_queries) == [] + + def test_head_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + response = self.client.head(self.url) + assert response.status_code == status.HTTP_200_OK + assert write_queries(captured.captured_queries) == [] + + def test_an_unknown_response_preset_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url, {"view": "nonsense"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "view": [ + "Ask for any of: full, minimal, or omit this parameter for the default response.", + ], + } + + +@skip_unless_lms +class UserPreferenceDetailViewTests(UserApiV2TestCase): + """GET /api/user/v2/user_preferences/{username},{key}/""" + + def url_for(self, username, key): + """Return the member address of one stored preference.""" + return reverse("user_v2:user_preference_detail", kwargs={"username": username, "key": key}) + + def test_anonymous_caller_is_unauthenticated(self): + response = self.client.get(self.url_for("alice", "pref-lang")) + assert_error_envelope(response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn") + + def test_non_admin_caller_is_forbidden(self): + self.login(self.learner) + response = self.client.get(self.url_for("alice", "pref-lang")) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_owner_who_is_not_an_admin_is_forbidden(self): + self.login(self.alice) + response = self.client.get(self.url_for("alice", "pref-lang")) + assert_error_envelope(response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz") + + def test_admin_caller_receives_the_preference(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice", "pref-lang")) + assert response.status_code == status.HTTP_200_OK + assert set(response.data) == {"username", "key", "value", "url", "user"} + assert response.data["username"] == "alice" + assert response.data["key"] == "pref-lang" + assert response.data["value"] == "en" + assert response.data["user"] is None + + def test_full_view_embeds_the_owning_user(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice", "pref-lang"), {"view": "full"}) + assert response.data["user"]["username"] == "alice" + + def test_a_field_name_the_preference_does_not_carry_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice", "pref-lang"), {"fields": "preference_key"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "fields": [ + "No such field: preference_key. " + "Select any of: key, url, user, username, value.", + ], + } + + def test_unknown_key_is_not_found(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice", "nope")) + assert_error_envelope(response, expected_status=status.HTTP_404_NOT_FOUND, expected_type_slug="not-found") + + def test_key_belonging_to_another_user_is_not_found(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice", "time_zone")) + assert_error_envelope(response, expected_status=status.HTTP_404_NOT_FOUND, expected_type_slug="not-found") + + def test_an_unknown_response_preset_is_refused(self): + self.login(self.admin) + response = self.client.get(self.url_for("alice", "pref-lang"), {"view": "nonsense"}) + assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + + def test_retrieval_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + self.client.get(self.url_for("alice", "pref-lang")) + assert write_queries(captured.captured_queries) == [] + + def test_head_writes_nothing(self): + self.login(self.admin) + with CaptureQueriesContext(connection) as captured: + response = self.client.head(self.url_for("alice", "pref-lang")) + assert response.status_code == status.HTTP_200_OK + assert write_queries(captured.captured_queries) == [] + + +@skip_unless_lms +class TimeZoneListViewTests(UserApiV2TestCase): + """GET /api/user/v2/time_zones/""" + + def setUp(self): + super().setUp() + self.url = reverse("user_v2:time_zone_list") + + def test_anonymous_caller_receives_the_list(self): + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + assert set(response.data) == PAGINATION_KEYS + assert len(response.data["results"]) == 10 + assert set(response.data["results"][0]) == {"time_zone", "description"} + + def test_authenticated_caller_receives_the_same_count(self): + anonymous_count = self.client.get(self.url).data["count"] + self.login(self.learner) + response = self.client.get(self.url) + assert response.data["count"] == anonymous_count + + def test_country_code_narrows_the_list(self): + response = self.client.get(self.url, {"country_code": "FR", "page_size": 100}) + assert [row["time_zone"] for row in response.data["results"]] == ["Europe/Paris"] + + def test_country_code_is_case_insensitive(self): + upper = self.client.get(self.url, {"country_code": "FR", "page_size": 100}).data["results"] + lower = self.client.get(self.url, {"country_code": "fr", "page_size": 100}).data["results"] + assert upper == lower + + def test_country_without_time_zones_falls_back_to_every_zone(self): + every = self.client.get(self.url).data["count"] + response = self.client.get(self.url, {"country_code": "BV"}) + assert response.data["count"] == every + + def test_an_empty_country_code_returns_every_zone(self): + """A query string built from an empty variable narrows nothing rather than failing.""" + every = self.client.get(self.url).data["count"] + response = self.client.get(self.url, {"country_code": ""}) + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == every + + def test_a_page_beyond_the_last_one_is_not_found(self): + """The list is public, so its most common failure is answered to anyone.""" + response = self.client.get(self.url, {"page": 9999}) + assert_error_envelope(response, expected_status=status.HTTP_404_NOT_FOUND, expected_type_slug="not-found") + + def test_malformed_country_code_is_rejected(self): + response = self.client.get(self.url, {"country_code": "FRA"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == { + "country_code": ["Enter a two-letter ISO 3166-1 alpha-2 country code, for example FR."], + } + + def test_listing_writes_nothing(self): + with CaptureQueriesContext(connection) as captured: + self.client.get(self.url, {"page_size": 100}) + assert write_queries(captured.captured_queries) == [] + + def test_head_writes_nothing(self): + with CaptureQueriesContext(connection) as captured: + response = self.client.head(self.url) + assert response.status_code == status.HTTP_200_OK + assert write_queries(captured.captured_queries) == [] + + +@skip_unless_lms +class PageSizeCapTests(UserApiV2TestCase): + """ + No caller can ask any of the lists for more rows than one maximum page. + + The time-zone list is the one a caller reaches without signing in, and it + is long enough to be worth asking for whole, so the cap is asserted against + live rows on every list rather than read off the paginator. + """ + + MAX_PAGE_SIZE = 100 + + def test_the_directory_page_is_capped(self): + User.objects.bulk_create( + User(username=f"extra-{index:03d}", email=f"extra-{index:03d}@example.com") + for index in range(self.MAX_PAGE_SIZE) + ) + self.login(self.admin) + response = self.client.get(reverse("user_v2:user_list"), {"page_size": 1000}) + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == self.MAX_PAGE_SIZE + 4 + assert len(response.data["results"]) == self.MAX_PAGE_SIZE + assert response.data["num_pages"] == 2 + assert response.data["next"] is not None + + def test_the_stored_preference_page_is_capped(self): + UserPreference.objects.bulk_create( + UserPreference(user=self.alice, key=f"extra-{index:03d}", value="x") + for index in range(self.MAX_PAGE_SIZE) + ) + self.login(self.admin) + response = self.client.get(reverse("user_v2:user_preference_list"), {"page_size": 1000}) + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == self.MAX_PAGE_SIZE + 2 + assert len(response.data["results"]) == self.MAX_PAGE_SIZE + assert response.data["num_pages"] == 2 + + def test_the_time_zone_page_is_capped_for_an_anonymous_caller(self): + every_zone = len(get_country_time_zones()) + assert every_zone > self.MAX_PAGE_SIZE, every_zone + response = self.client.get(reverse("user_v2:time_zone_list"), {"page_size": 1000}) + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == every_zone + assert len(response.data["results"]) == self.MAX_PAGE_SIZE + assert response.data["num_pages"] == ceil(every_zone / self.MAX_PAGE_SIZE) + + +@skip_unless_lms +class RefusedRequestTests(UserApiV2TestCase): + """A method this API does not serve, and an Accept header it cannot answer.""" + + #: Public, standardized, and served by another API: what its refused method + #: is called must not change because this one names its own. + OTHER_API_URL = "/api/enrollment/v2/courses/course-v1:edX+DemoX+Demo/" + + def setUp(self): + super().setUp() + self.public_url = reverse("user_v2:time_zone_list") + + def test_a_refused_method_is_named_for_an_anonymous_caller(self): + response = self.client.post(self.public_url, {}, format="json") + envelope = assert_error_envelope( + response, + expected_status=status.HTTP_405_METHOD_NOT_ALLOWED, + expected_type_slug="method-not-allowed", + ) + assert envelope["title"] == "Method Not Allowed" + assert envelope["detail"] == 'Method "POST" not allowed.' + assert response["Content-Type"] == "application/json" + + def test_an_accept_header_this_api_cannot_answer_is_named(self): + response = self.client.get(self.public_url, HTTP_ACCEPT="application/xml") + envelope = assert_error_envelope( + response, + expected_status=status.HTTP_406_NOT_ACCEPTABLE, + expected_type_slug="not-acceptable", + ) + assert envelope["title"] == "Not Acceptable" + assert envelope["detail"] == "Could not satisfy the request Accept header." + assert response["Content-Type"] == "application/json" + + def test_a_refused_method_is_named_on_the_gated_collections(self): + self.login(self.admin) + for url_name in ("user_v2:user_list", "user_v2:user_preference_list"): + response = self.client.delete(reverse(url_name)) + envelope = assert_error_envelope( + response, + expected_status=status.HTTP_405_METHOD_NOT_ALLOWED, + expected_type_slug="method-not-allowed", + ) + assert envelope["detail"] == 'Method "DELETE" not allowed.' + + def test_a_refused_method_is_named_on_the_gated_members(self): + self.login(self.admin) + response = self.client.put(reverse("user_v2:user_detail", args=[self.alice.username])) + envelope = assert_error_envelope( + response, + expected_status=status.HTTP_405_METHOD_NOT_ALLOWED, + expected_type_slug="method-not-allowed", + ) + assert envelope["detail"] == 'Method "PUT" not allowed.' + + def test_another_api_is_classified_exactly_as_the_shared_catalog_classifies_it(self): + """The catalog names no refused method, and nothing here changes that for anyone else.""" + response = self.client.post(self.OTHER_API_URL, {}, format="json") + envelope = assert_error_envelope( + response, + expected_status=status.HTTP_405_METHOD_NOT_ALLOWED, + expected_type_slug="internal", + ) + assert envelope["title"] == "Internal Server Error" + + +@skip_unless_lms +class UnknownQueryParameterTests(UserApiV2TestCase): + """Each endpoint refuses a query parameter it reads nothing from, and takes the rest.""" + + def setUp(self): + super().setUp() + self.user_list = reverse("user_v2:user_list") + self.user_detail = reverse("user_v2:user_detail", kwargs={"username": "alice"}) + self.preference_list = reverse("user_v2:user_preference_list") + self.preference_detail = reverse( + "user_v2:user_preference_detail", kwargs={"username": "alice", "key": "pref-lang"}, + ) + self.time_zone_list = reverse("user_v2:time_zone_list") + + def assert_refused(self, response, name, offered): + """Assert the response named the parameter it would not take, and the ones it takes.""" + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert envelope["errors"] == {name: [f"No such query parameter. Send any of: {offered}."]} + + def assert_each_is_answered(self, address, parameter_sets): + """Assert the endpoint answers each of these query strings.""" + for params in parameter_sets: + with self.subTest(params=params): + response = self.client.get(address, params) + assert response.status_code == status.HTTP_200_OK, response.content + + def test_the_directory_refuses_a_parameter_it_does_not_read(self): + self.login(self.admin) + response = self.client.get(self.user_list, {"lms_user_id": self.alice.id, "page_size": 100}) + self.assert_refused(response, "lms_user_id", USER_QUERY_PARAMS) + + def test_a_mistyped_filter_refuses_the_page_instead_of_returning_every_row(self): + self.login(self.admin) + response = self.client.get(self.user_list, {"usernme": "alice", "page_size": 100}) + self.assert_refused(response, "usernme", USER_QUERY_PARAMS) + + def test_every_unrecognised_parameter_is_named_at_once(self): + self.login(self.admin) + response = self.client.get(self.user_list, {"sort": "username", "limit": "10"}) + envelope = assert_error_envelope( + response, expected_status=status.HTTP_400_BAD_REQUEST, expected_type_slug="validation", + ) + assert sorted(envelope["errors"]) == ["limit", "sort"] + + def test_a_directory_entry_refuses_a_parameter_it_does_not_read(self): + self.login(self.admin) + response = self.client.get(self.user_detail, {"expand": "profile"}) + self.assert_refused(response, "expand", USER_QUERY_PARAMS) + + def test_the_preference_list_refuses_a_parameter_it_does_not_read(self): + self.login(self.admin) + response = self.client.get(self.preference_list, {"value": "en", "page_size": 100}) + self.assert_refused(response, "value", PREFERENCE_QUERY_PARAMS) + + def test_a_stored_preference_refuses_a_parameter_it_does_not_read(self): + self.login(self.admin) + response = self.client.get(self.preference_detail, {"value": "en"}) + self.assert_refused(response, "value", PREFERENCE_QUERY_PARAMS) + + def test_the_time_zone_list_refuses_a_parameter_it_does_not_read(self): + response = self.client.get(self.time_zone_list, {"country": "FR"}) + self.assert_refused(response, "country", TIME_ZONE_QUERY_PARAMS) + + def test_the_time_zone_list_refuses_a_sort_it_does_not_offer(self): + """This list is ordered by its display form and reads no sort parameter.""" + response = self.client.get(self.time_zone_list, {"ordering": "country_code"}) + self.assert_refused(response, "ordering", TIME_ZONE_QUERY_PARAMS) + + def test_an_unknown_parameter_does_not_answer_an_anonymous_caller(self): + response = self.client.get(self.user_list, {"lms_user_id": self.alice.id}) + assert_error_envelope( + response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn", + ) + + def test_an_unknown_parameter_does_not_answer_a_non_admin_caller(self): + self.login(self.learner) + response = self.client.get(self.preference_list, {"lms_user_id": self.alice.id}) + assert_error_envelope( + response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz", + ) + + def test_the_directory_answers_every_parameter_it_reads(self): + self.login(self.admin) + self.assert_each_is_answered(self.user_list, [ + {"page": 1}, + {"page_size": 2}, + {"ordering": "-username"}, + {"view": "minimal"}, + {"fields": "username"}, + {"format": "json"}, + {"username": "alice"}, + {"email": "alice@example.com"}, + {"is_active": "true"}, + {"preference_key": "pref-lang"}, + {"forum_role": "Moderator", "course_key": str(COURSE_KEY)}, + ]) + + def test_a_directory_entry_answers_every_parameter_it_reads(self): + self.login(self.admin) + self.assert_each_is_answered(self.user_detail, [ + {"view": "minimal"}, {"fields": "username"}, {"format": "json"}, {"ordering": "username"}, + ]) + + def test_the_preference_list_answers_every_parameter_it_reads(self): + self.login(self.admin) + self.assert_each_is_answered(self.preference_list, [ + {"page": 1}, + {"page_size": 2}, + {"ordering": "-key"}, + {"view": "full"}, + {"fields": "key"}, + {"format": "json"}, + {"key": "pref-lang"}, + {"username": "alice"}, + ]) + + def test_a_stored_preference_answers_every_parameter_it_reads(self): + self.login(self.admin) + self.assert_each_is_answered(self.preference_detail, [ + {"view": "full"}, {"fields": "key"}, {"format": "json"}, + ]) + + def test_the_time_zone_list_answers_every_parameter_it_reads(self): + self.assert_each_is_answered(self.time_zone_list, [ + {"country_code": "FR"}, {"page": 1}, {"page_size": 5}, {"format": "json"}, + ]) + + def test_a_page_of_the_directory_is_still_narrowed_by_the_filters_it_takes(self): + """The refusal guards the parameters, not the filtering they ask for.""" + self.login(self.admin) + response = self.client.get(self.user_list, {"username": "alice", "page_size": 100}) + assert self.usernames_in(response) == ["alice"] + + +@skip_unless_lms +class TokenCallerTests(UserApiV2TestCase): + """The staff-gated endpoints answer a bearer of a staff token and refuse everyone else.""" + + def setUp(self): + super().setUp() + self.addresses = { + "user_list": reverse("user_v2:user_list"), + "user_detail": reverse("user_v2:user_detail", kwargs={"username": "alice"}), + "preference_list": reverse("user_v2:user_preference_list"), + "preference_detail": reverse( + "user_v2:user_preference_detail", kwargs={"username": "alice", "key": "pref-lang"}, + ), + } + + def token_headers(self, user): + """Return the request headers that carry a token identifying the given user.""" + return {"HTTP_AUTHORIZATION": f"JWT {create_jwt_for_user(user)}"} + + def test_a_staff_token_reaches_the_directory(self): + response = self.client.get( + self.addresses["user_list"], {"page_size": 100}, **self.token_headers(self.admin), + ) + assert response.status_code == status.HTTP_200_OK + assert self.usernames_in(response) == ["admin-caller", "alice", "bob", "learner-caller"] + assert response.data["count"] == 4 + + def test_a_staff_token_reaches_every_staff_gated_address(self): + headers = self.token_headers(self.admin) + detail = self.client.get(self.addresses["user_detail"], **headers) + assert detail.status_code == status.HTTP_200_OK + assert detail.data["username"] == "alice" + + preferences = self.client.get( + self.addresses["preference_list"], {"username": "alice"}, **headers, + ) + assert preferences.status_code == status.HTTP_200_OK + assert [(row["username"], row["key"]) for row in preferences.data["results"]] == [ + ("alice", "pref-lang"), + ] + + preference = self.client.get(self.addresses["preference_detail"], **headers) + assert preference.status_code == status.HTTP_200_OK + assert preference.data["value"] == "en" + + def test_the_token_and_not_a_session_identifies_the_caller(self): + token = create_jwt_for_user(self.admin) + response = self.client.get( + self.addresses["user_list"], {"username": "alice"}, HTTP_AUTHORIZATION=f"JWT {token}", + ) + assert response.status_code == status.HTTP_200_OK + assert response.wsgi_request.user == self.admin + assert response.wsgi_request.auth == token + assert "sessionid" not in response.wsgi_request.COOKIES + + def test_a_learners_token_is_forbidden_on_every_staff_gated_address(self): + headers = self.token_headers(self.learner) + for name, address in self.addresses.items(): + with self.subTest(address=name): + response = self.client.get(address, **headers) + assert_error_envelope( + response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz", + ) + + def test_the_owner_of_the_record_is_still_not_staff(self): + response = self.client.get( + self.addresses["preference_detail"], **self.token_headers(self.alice), + ) + assert_error_envelope( + response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz", + ) + + def test_a_token_that_does_not_decode_is_unauthenticated(self): + response = self.client.get( + self.addresses["user_list"], HTTP_AUTHORIZATION="JWT not.a.token", + ) + assert_error_envelope( + response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn", + ) + + +@skip_unless_lms +class SharedSortValueTests(UserApiV2TestCase): + """Users that compare equal on the sorted column are ordered the same way on every page.""" + + def setUp(self): + super().setUp() + self.url = reverse("user_v2:user_list") + joined = datetime(2024, 3, 1, tzinfo=UTC) + for username in ("zoe-shared", "mia-shared", "ada-shared"): + UserFactory.create(username=username, date_joined=joined) + self.login(self.admin) + + def test_users_sharing_a_join_date_are_ordered_by_username(self): + response = self.client.get(self.url, {"ordering": "date_joined", "page_size": 100}) + shared = [name for name in self.usernames_in(response) if name.endswith("-shared")] + assert shared == ["ada-shared", "mia-shared", "zoe-shared"] + + def test_the_sort_the_caller_asked_for_still_decides_the_order(self): + """The tie-break settles rows that compare equal; the requested sort places them.""" + response = self.client.get(self.url, {"ordering": "-date_joined", "page_size": 100}) + assert self.usernames_in(response)[:3] == ["ada-shared", "mia-shared", "zoe-shared"] + + def test_paging_through_a_shared_join_date_lists_each_user_once(self): + listed = [] + for page in (1, 2, 3): + response = self.client.get( + self.url, {"ordering": "date_joined", "page_size": 3, "page": page}, + ) + listed.extend(self.usernames_in(response)) + assert sorted(listed) == [ + "ada-shared", "admin-caller", "alice", "bob", "learner-caller", "mia-shared", "zoe-shared", + ] + + +@skip_unless_lms +class DeactivatedStaffCallerTests(UserApiV2TestCase): + """ + A staff account whose user record has been deactivated is refused by both halves of + the default authentication pair: the session half stops accepting it, and the active + account these views require refuses a token issued before the account was deactivated. + """ + + def setUp(self): + super().setUp() + self.url = reverse("user_v2:user_list") + self.staff = AdminFactory.create(username="deactivated-admin", password=PASSWORD) + self.staff_gated = { + "user_list": self.url, + "user_detail": reverse("user_v2:user_detail", kwargs={"username": "alice"}), + "preference_list": reverse("user_v2:user_preference_list"), + "preference_detail": reverse( + "user_v2:user_preference_detail", kwargs={"username": "alice", "key": "pref-lang"}, + ), + } + + def deactivate(self, user): + """Clear an account's active flag the way an operator does, leaving its staff flag.""" + user.is_active = False + user.save() + + def test_a_session_held_by_a_deactivated_staff_account_is_refused(self): + self.login(self.staff) + assert self.client.get(self.url).status_code == status.HTTP_200_OK + self.deactivate(self.staff) + response = self.client.get(self.url) + assert_error_envelope( + response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn", + ) + + def test_a_token_held_by_a_deactivated_staff_account_is_refused(self): + headers = {"HTTP_AUTHORIZATION": f"JWT {create_jwt_for_user(self.staff)}"} + assert self.client.get(self.url, **headers).status_code == status.HTTP_200_OK + self.deactivate(self.staff) + response = self.client.get(self.url, {"page_size": 100}, **headers) + assert_error_envelope( + response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz", + ) + + def test_a_deactivated_staff_token_is_refused_at_every_staff_gated_address(self): + self.deactivate(self.staff) + headers = {"HTTP_AUTHORIZATION": f"JWT {create_jwt_for_user(self.staff)}"} + for name, address in self.staff_gated.items(): + with self.subTest(address=name): + response = self.client.get(address, **headers) + assert_error_envelope( + response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz", + ) + + def test_a_deactivated_staff_session_is_refused_at_every_staff_gated_address(self): + self.login(self.staff) + self.deactivate(self.staff) + for name, address in self.staff_gated.items(): + with self.subTest(address=name): + response = self.client.get(address) + assert_error_envelope( + response, expected_status=status.HTTP_401_UNAUTHORIZED, expected_type_slug="authn", + ) + + def test_the_directory_does_not_list_the_account_that_was_refused(self): + """The refusal is the whole response: no row of the page leaks past it.""" + self.deactivate(self.staff) + response = self.client.get( + self.url, {"page_size": 100}, HTTP_AUTHORIZATION=f"JWT {create_jwt_for_user(self.staff)}", + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "results" not in response.data + + def test_an_active_staff_token_still_reaches_the_directory(self): + response = self.client.get( + self.url, {"page_size": 100}, HTTP_AUTHORIZATION=f"JWT {create_jwt_for_user(self.staff)}", + ) + assert response.status_code == status.HTTP_200_OK + assert "deactivated-admin" in self.usernames_in(response) + + def test_a_token_held_by_a_deactivated_learner_is_still_forbidden(self): + self.deactivate(self.learner) + response = self.client.get( + self.url, HTTP_AUTHORIZATION=f"JWT {create_jwt_for_user(self.learner)}", + ) + assert_error_envelope( + response, expected_status=status.HTTP_403_FORBIDDEN, expected_type_slug="authz", + ) + + +@skip_unless_lms +class PreferenceSerializerTests(UserApiV2TestCase): + """The preference address is built with or without a request to make it absolute.""" + + def test_url_is_relative_without_a_request(self): + preference = self.alice.preferences.get(key="pref-lang") + data = UserPreferenceRecordSerializer(preference).data + assert data["url"] == "/api/user/v2/user_preferences/alice,pref-lang/" + assert data["user"] is None diff --git a/openedx/core/djangoapps/user_api/v2/urls.py b/openedx/core/djangoapps/user_api/v2/urls.py new file mode 100644 index 000000000000..911fb321af69 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/urls.py @@ -0,0 +1,71 @@ +"""URLs for version 2 of the User API.""" + +from django.conf import settings +from django.core.exceptions import ImproperlyConfigured +from django.urls import re_path +from rest_framework.routers import SimpleRouter + +from .views import ( + PREFERENCE_KEY_PATTERN_PARTIAL, + CountryTimeZoneViewSet, + UnknownAddressView, + UserPreferenceViewSet, + UserViewSet, +) + +app_name = "user_v2" + +#: A preference is identified by its owner and its key, so the router's +#: database-id member route is dropped and replaced by the address declared +#: below under the same name. +REPLACED_ROUTER_ROUTE = "user_preference_detail" + + +def without_route(routes, name): + """ + Return ``routes`` without the one registered under ``name``. + + A name that matches nothing means the router stopped generating that route + under that name: the address it serves would then be published alongside + the one declared here instead of being replaced by it, so the mismatch is + refused here rather than discovered at the address. + """ + kept = [route for route in routes if route.name != name] + if len(kept) == len(routes): + raise ImproperlyConfigured(f"The router registers no route named {name!r} to replace.") + return kept + + +class SnakeCaseRouter(SimpleRouter): + """ + A router that names its routes ``_``. + + The names a route is registered under are the ones ``resolve()`` reports + and the ones ``reverse()`` accepts, so they are generated in the published + form rather than rewritten afterwards. + """ + + routes = [route._replace(name=route.name.replace("-", "_")) for route in SimpleRouter.routes] + + +router = SnakeCaseRouter() +router.register(r"users", UserViewSet, basename="user") +router.register(r"user_preferences", UserPreferenceViewSet, basename="user_preference") +router.register(r"time_zones", CountryTimeZoneViewSet, basename="time_zone") + + +urlpatterns = [ + *without_route(router.urls, REPLACED_ROUTER_ROUTE), + # The username half carries the pattern a username is allowed to match and + # the key half takes the rest of the segment. + re_path( + rf"^user_preferences/(?P{settings.USERNAME_REGEX_PARTIAL})" + rf",(?P{PREFERENCE_KEY_PATTERN_PARTIAL})/$", + UserPreferenceViewSet.as_view({"get": "retrieve"}), + name="user_preference_detail", + ), + # The authentication app serves two addresses under this prefix, which this + # mount is matched before; everything else under it belongs to this version. + # The route answers rather than reverses, so it carries no name. + re_path(r"^(?!account/).*$", UnknownAddressView.as_view()), +] diff --git a/openedx/core/djangoapps/user_api/v2/views.py b/openedx/core/djangoapps/user_api/v2/views.py new file mode 100644 index 000000000000..6022c5dcec40 --- /dev/null +++ b/openedx/core/djangoapps/user_api/v2/views.py @@ -0,0 +1,593 @@ +"""Views for version 2 of the User API.""" + +import logging + +from django.conf import settings +from django.contrib.auth.models import User # pylint: disable=imported-auth-user +from django.db.models import F +from django.http import Http404 +from django_filters.rest_framework import DjangoFilterBackend +from drf_spectacular.utils import OpenApiParameter, OpenApiResponse, extend_schema, extend_schema_view +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from edx_rest_framework_extensions.paginators import IterablePaginationMixin +from edx_rest_framework_extensions.scoping import FullScopePolicy, ScopedQuerysetMixin +from edx_rest_framework_extensions.shaping import MinimalViewMixin, project +from rest_framework import permissions, viewsets +from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.reverse import reverse +from rest_framework.settings import api_settings +from rest_framework.views import APIView + +from openedx.core.djangoapps.user_api.models import UserPreference +from openedx.core.djangoapps.user_api.preferences.api import get_country_time_zones + +from .errors import CatalogedRequestErrorsMixin +from .filters import StrictOrderingFilter, UserFilterSet, UserPreferenceFilterSet +from .paginators import UserApiPagination +from .permissions import IsActiveUser +from .schema import ResponseShapes +from .serializers import ( + FULL_VIEW, + MINIMAL_VIEW, + PREFERENCE_MINIMAL_FIELDS, + USER_MINIMAL_FIELDS, + ErrorResponse, + TimeZoneQuerySerializer, + TimeZoneSerializer, + UserDirectoryMinimalSerializer, + UserDirectoryProjectionSerializer, + UserDirectorySerializer, + UserPreferenceRecordMinimalSerializer, + UserPreferenceRecordProjectionSerializer, + UserPreferenceRecordSerializer, + full_view_requested, +) + +log = logging.getLogger(__name__) + +USER_TAG = "user" + +#: Fields the user directory sorts on, and the order it uses when asked for none. +USER_SORT_FIELDS = ("username", "date_joined") +USER_DEFAULT_SORT = ("username",) + +#: Fields the stored-preference list sorts on, and the order it uses when asked for none. +PREFERENCE_SORT_FIELDS = ("key", "username") +PREFERENCE_DEFAULT_SORT = ("username", "key") + +#: The part of a preference's address that carries its key. The key is +#: free-form text that may itself contain a comma, so it takes the rest of the +#: segment; the route and the published address are built from this one pattern. +PREFERENCE_KEY_PATTERN_PARTIAL = "[^/]+" + + +def _query_param(name, description, **kwargs): + """Build an optional query parameter for the published schema.""" + return OpenApiParameter( + name=name, + description=description, + required=False, + location=OpenApiParameter.QUERY, + **kwargs, + ) + + +def _path_param(name, description, pattern): + """Build a path parameter for the published schema, carrying what its route matches.""" + return OpenApiParameter( + name=name, + description=description, + required=True, + location=OpenApiParameter.PATH, + type=str, + pattern=pattern, + ) + + +_USERNAME_PATTERN = rf"^{settings.USERNAME_REGEX_PARTIAL}$" + +_USER_PATH_PARAM = _path_param( + "username", + "Unique name of the user whose directory entry is returned.", + _USERNAME_PATTERN, +) +_PREFERENCE_USERNAME_PATH_PARAM = _path_param( + "username", + "Username of the user the preference belongs to: the part of the address before the comma.", + _USERNAME_PATTERN, +) +_PREFERENCE_KEY_PATH_PARAM = _path_param( + "key", + "Name the preference is stored under: the part of the address after the comma.", + rf"^{PREFERENCE_KEY_PATTERN_PARTIAL}$", +) + +#: Parameters that shape a response rather than select the rows in it. +VIEW_PARAM = "view" +FIELDS_PARAM = "fields" +RESPONSE_SHAPING_PARAMS = (VIEW_PARAM, FIELDS_PARAM) + +#: The framework's own representation selector. This deployment reads nothing +#: from it, since ``URL_FORMAT_OVERRIDE`` is unset and JSON is the only +#: renderer, and a caller that sends it anyway is answered rather than refused. +FORMAT_PARAM = "format" + +_FIELDS_PARAM = _query_param( + FIELDS_PARAM, + "Comma-separated list of top-level field names to keep in each returned object. " + "A name the response cannot carry is refused.", +) +#: The presets each list answers to, published as the parameter's enum and +#: accepted by the view from the same declaration. +USER_PRESETS = (MINIMAL_VIEW,) +PREFERENCE_PRESETS = (FULL_VIEW, MINIMAL_VIEW) + +_MINIMAL_VIEW_PARAM = _query_param( + VIEW_PARAM, + "Response preset. `minimal` reduces each user to its identifying fields " + "(`id`, `username`, `email`). Omit it to receive the full entry.", + enum=list(USER_PRESETS), +) +_PREFERENCE_VIEW_PARAM = _query_param( + VIEW_PARAM, + "Response preset. `full` embeds the owning user's whole directory entry in each " + "preference; `minimal` returns only `username`, `key` and `value`. Omit it to " + "receive the owner's username alongside the preference.", + enum=list(PREFERENCE_PRESETS), +) + + +def _ordering_param(sort_fields, default): + """Build the sort parameter of a list, naming the fields it accepts.""" + return _query_param( + "ordering", + "Fields to sort by, in order of precedence, each optionally prefixed with `-` to sort " + "descending. A field that is not listed here is refused.", + many=True, + enum=[*sort_fields, *(f"-{field}" for field in sort_fields)], + default=list(default), + style="form", + explode=False, + ) + + +_USER_ORDERING_PARAM = _ordering_param(USER_SORT_FIELDS, USER_DEFAULT_SORT) +_PREFERENCE_ORDERING_PARAM = _ordering_param(PREFERENCE_SORT_FIELDS, PREFERENCE_DEFAULT_SORT) + +_RESP_UNAUTHENTICATED = OpenApiResponse(ErrorResponse, description="The requester is not authenticated.") +_RESP_FORBIDDEN = OpenApiResponse(ErrorResponse, description="The requester may not use this endpoint.") +_RESP_NOT_FOUND = OpenApiResponse(ErrorResponse, description="No such resource.") +_RESP_NO_SUCH_PAGE = OpenApiResponse(ErrorResponse, description="There is no such page of results.") +_RESP_BAD_REQUEST = OpenApiResponse( + ErrorResponse, description="A query parameter is missing, invalid, or not one this endpoint reads.", +) + +#: A directory entry comes back whole, as the identifying fields the ``minimal`` +#: preset keeps, or as the properties ``fields`` selected. +_USER_SHAPES = ResponseShapes( + component_name="UserDirectoryResponse", + serializers=[ + UserDirectorySerializer, + UserDirectoryMinimalSerializer, + UserDirectoryProjectionSerializer, + ], +) + +#: A stored preference comes back whole, as the three fields the ``minimal`` +#: preset keeps, or as the properties ``fields`` selected. +_PREFERENCE_SHAPES = ResponseShapes( + component_name="UserPreferenceRecordResponse", + serializers=[ + UserPreferenceRecordSerializer, + UserPreferenceRecordMinimalSerializer, + UserPreferenceRecordProjectionSerializer, + ], +) + + +class _MemberLookupMixin: + """Report a missing member through the standard error envelope.""" + + def get_object(self): + try: + return super().get_object() + except Http404 as exc: + raise NotFound from exc + + +class _ActionAddressMixin: + """ + Builds the address of an action from the snake_case names these routes carry. + + A router names an action ``-`` by default, and that name + belongs to an earlier mount of the same resource, so the default lookup + would hand out the older address instead of this one. + """ + + #: The namespace these routes are mounted under, read when an address is + #: asked for outside a request and there is no resolver match to take it from. + url_namespace = "user_v2" + + def reverse_action(self, url_name, *args, **kwargs): + """Return the address of one of this viewset's actions.""" + request = getattr(self, "request", None) + namespace = self.url_namespace + if request is not None and request.resolver_match is not None: + namespace = request.resolver_match.namespace or namespace + kwargs.setdefault("request", request) + return reverse(f"{namespace}:{self.basename}_{url_name}", *args, **kwargs) + + +def _requested_field_names(raw): + """Return the top-level property names a ``fields`` value asks for.""" + if not raw: + return [] + return [name.strip().split(".", 1)[0] for name in raw.split(",") if name.strip()] + + +class _StrictQueryParametersMixin: + """ + Refuses a query parameter the view reads nothing from. + + An unknown parameter is otherwise dropped in silence: a caller that + mistyped a filter, or kept one an earlier version of the endpoint + answered, receives the whole collection and reads it as the narrowed page + it asked for. The names a view answers to are assembled from what it + declares (its paginator, its sort backend, its filter set, and the + serializer that validates its query string), so a parameter that is added + or renamed cannot fall out of step with this check. + """ + + #: Names the view reads that none of the declarations below account for. + extra_query_params = () + + def known_query_params(self): + """Return the names of the query parameters this view reads.""" + names = {FORMAT_PARAM, api_settings.URL_FORMAT_OVERRIDE, *self.extra_query_params} + pagination_class = getattr(self, "pagination_class", None) + if pagination_class is not None: + names.update({pagination_class.page_query_param, pagination_class.page_size_query_param}) + for backend in getattr(self, "filter_backends", ()): + names.add(getattr(backend, "ordering_param", None)) + filterset_class = getattr(self, "filterset_class", None) + if filterset_class is not None: + names.update(filterset_class.base_filters) + query_serializer_class = getattr(self, "query_serializer_class", None) + if query_serializer_class is not None: + names.update(query_serializer_class().fields) + return names - {None} + + def initial(self, request, *args, **kwargs): + """Check the query string once the caller has been admitted.""" + super().initial(request, *args, **kwargs) + known = self.known_query_params() + unknown = sorted(set(request.query_params) - known) + if unknown: + log.info("Query refused on %s: %r", type(self).__name__, unknown) + offered = ", ".join(sorted(known)) + raise ValidationError({ + name: [f"No such query parameter. Send any of: {offered}."] + for name in unknown + }) + + +class _ResponsePresetMixin(MinimalViewMixin): + """ + Applies the ``view`` and ``fields`` presets to an already serialized payload. + + A view lists the presets it answers to, and a ``view`` value outside that + list is refused: a caller that mistypes a preset asked for a smaller + payload and would otherwise be handed the full one without being told. + The same holds for a field name the response cannot carry, which would + otherwise select nothing and leave the caller with empty objects. + """ + + #: Presets this view answers to. An empty ``view`` narrows nothing, the way + #: an empty value of any other parameter does. + response_presets = () + + def initial(self, request, *args, **kwargs): + """Check the requested preset and fields once the caller has been admitted.""" + super().initial(request, *args, **kwargs) + preset = request.query_params.get(VIEW_PARAM) + if preset and preset not in self.response_presets: + raise ValidationError({ + VIEW_PARAM: [ + f"Ask for any of: {', '.join(self.response_presets)}, " + "or omit this parameter for the default response.", + ], + }) + self.check_requested_fields(request) + + def shapeable_fields(self): + """Return the names of the properties a response of this view can carry.""" + if self.is_minimal_view_requested(): + return set(self.minimal_fields) + return set(self.get_serializer_class()().fields) + + def check_requested_fields(self, request): + """Refuse a selected field the response cannot carry, naming the ones it can.""" + available = self.shapeable_fields() + unknown = sorted(set(_requested_field_names(request.query_params.get(FIELDS_PARAM))) - available) + if unknown: + raise ValidationError({ + FIELDS_PARAM: [ + f"No such field: {', '.join(unknown)}. " + f"Select any of: {', '.join(sorted(available))}.", + ], + }) + + def shape(self, data, request): + """Return ``data`` reduced to the preset and the fields the caller asked for.""" + data = self.shape_minimal(data, request) + requested_fields = request.query_params.get(FIELDS_PARAM) + if not requested_fields: + return data + if isinstance(data, list): + return [project(item, requested_fields) for item in data] + return project(data, requested_fields) + + def shape_page(self, response, request): + """Shape the items of a paginated response in place.""" + response.data["results"] = self.shape(response.data["results"], request) + return response + + +@extend_schema(tags=[USER_TAG]) +@extend_schema_view( + list=extend_schema( + summary="List users", + description=( + "Returns a page of the user directory. Supply `preference_key` to keep only the " + "users who have stored a preference under that key, or `forum_role` together with " + "`course_key` to keep only the holders of a course forum role." + ), + parameters=[_MINIMAL_VIEW_PARAM, _FIELDS_PARAM, _USER_ORDERING_PARAM], + responses={ + 200: _USER_SHAPES, + 400: _RESP_BAD_REQUEST, + 401: _RESP_UNAUTHENTICATED, + 403: _RESP_FORBIDDEN, + 404: _RESP_NO_SUCH_PAGE, + }, + ), + retrieve=extend_schema( + summary="Get one user", + description="Returns the directory entry of the user with this username.", + parameters=[_USER_PATH_PARAM, _MINIMAL_VIEW_PARAM, _FIELDS_PARAM], + responses={ + 200: _USER_SHAPES, + 400: _RESP_BAD_REQUEST, + 401: _RESP_UNAUTHENTICATED, + 403: _RESP_FORBIDDEN, + 404: _RESP_NOT_FOUND, + }, + ), +) +class UserViewSet( + StandardizedErrorMixin, + CatalogedRequestErrorsMixin, + _ActionAddressMixin, + _MemberLookupMixin, + _ResponsePresetMixin, + _StrictQueryParametersMixin, + ScopedQuerysetMixin, + viewsets.ReadOnlyModelViewSet, +): + """ + Read-only directory of platform users, addressed by username. + + Access is layered: ``permission_classes`` admits only platform + administrators whose account is still active, the scoping policy decides + which rows such an administrator may see, and the filter set narrows those + rows further by the caller's query parameters. + """ + + permission_classes = (permissions.IsAuthenticated, IsActiveUser, permissions.IsAdminUser) + serializer_class = UserDirectorySerializer + pagination_class = UserApiPagination + queryset = User.objects.all().prefetch_related("preferences").select_related("profile") + # Platform administrators may read every entry, so the policy keeps the + # queryset whole; a narrower policy can replace it without touching the + # filtering below. + scoping_policy = FullScopePolicy() + lookup_field = "username" + # The router's default lookup pattern excludes dots, which usernames allow. + lookup_value_regex = settings.USERNAME_REGEX_PARTIAL + filter_backends = (DjangoFilterBackend, StrictOrderingFilter) + filterset_class = UserFilterSet + ordering_fields = USER_SORT_FIELDS + ordering = USER_DEFAULT_SORT + # A sort the caller asks for is completed with these so that it is total: + # rows that compare equal would otherwise be ordered differently from one + # page to the next, and a client reading the pages in turn would see some + # of them twice and miss others. + ordering_tiebreak = ("username",) + minimal_fields = USER_MINIMAL_FIELDS + response_presets = USER_PRESETS + extra_query_params = RESPONSE_SHAPING_PARAMS + + def list(self, request, *args, **kwargs): + """Return a page of directory entries, shaped as the caller asked.""" + return self.shape_page(super().list(request, *args, **kwargs), request) + + def retrieve(self, request, *args, **kwargs): + """Return one directory entry, shaped as the caller asked.""" + response = super().retrieve(request, *args, **kwargs) + response.data = self.shape(response.data, request) + return response + + +@extend_schema(tags=[USER_TAG]) +@extend_schema_view( + list=extend_schema( + summary="List stored preferences", + description=( + "Returns a page of stored user preferences. Supply `key` to keep only the " + "preferences stored under that name, or `username` to keep only one user's." + ), + parameters=[_PREFERENCE_VIEW_PARAM, _FIELDS_PARAM, _PREFERENCE_ORDERING_PARAM], + responses={ + 200: _PREFERENCE_SHAPES, + 400: _RESP_BAD_REQUEST, + 401: _RESP_UNAUTHENTICATED, + 403: _RESP_FORBIDDEN, + 404: _RESP_NO_SUCH_PAGE, + }, + ), + retrieve=extend_schema( + summary="Get one stored preference", + description=( + "Returns the preference stored under this key for this user. A preference is " + "identified by the pair, which the address carries as the username, a comma and " + "the key, for example `alice,time_zone`." + ), + operation_id="v2_user_preferences_retrieve", + parameters=[ + _PREFERENCE_USERNAME_PATH_PARAM, + _PREFERENCE_KEY_PATH_PARAM, + _PREFERENCE_VIEW_PARAM, + _FIELDS_PARAM, + ], + responses={ + 200: _PREFERENCE_SHAPES, + 400: _RESP_BAD_REQUEST, + 401: _RESP_UNAUTHENTICATED, + 403: _RESP_FORBIDDEN, + 404: _RESP_NOT_FOUND, + }, + ), +) +class UserPreferenceViewSet( + StandardizedErrorMixin, + CatalogedRequestErrorsMixin, + _ActionAddressMixin, + _ResponsePresetMixin, + _StrictQueryParametersMixin, + ScopedQuerysetMixin, + viewsets.ReadOnlyModelViewSet, +): + """ + Read-only list of stored user preferences, each addressed by its owner and key. + + A preference is identified by the pair, so the member address is + ``{username},{key}`` rather than a database id. Access is layered the same + way as the user directory: permission classes, then the scoping policy, + then the filter set. + """ + + permission_classes = (permissions.IsAuthenticated, IsActiveUser, permissions.IsAdminUser) + serializer_class = UserPreferenceRecordSerializer + pagination_class = UserApiPagination + queryset = ( + UserPreference.objects.all() + .select_related("user", "user__profile") + .annotate(username=F("user__username")) + ) + scoping_policy = FullScopePolicy() + filter_backends = (DjangoFilterBackend, StrictOrderingFilter) + filterset_class = UserPreferenceFilterSet + ordering_fields = PREFERENCE_SORT_FIELDS + ordering = PREFERENCE_DEFAULT_SORT + # The owner and the key identify a row, so they complete any sort the + # caller asks for and keep the order of a page stable. + ordering_tiebreak = ("username", "key") + minimal_fields = PREFERENCE_MINIMAL_FIELDS + response_presets = PREFERENCE_PRESETS + extra_query_params = RESPONSE_SHAPING_PARAMS + + def filter_queryset(self, queryset): + """Fetch each owner's own preferences too when they are about to be embedded.""" + queryset = super().filter_queryset(queryset) + if full_view_requested(getattr(self, "request", None)): + queryset = queryset.prefetch_related("user__preferences") + return queryset + + def get_object(self): + """Look the preference up by the username and key that identify it.""" + try: + preference = self.get_queryset().get( + user__username=self.kwargs["username"], key=self.kwargs["key"], + ) + except UserPreference.DoesNotExist as exc: + raise NotFound from exc + self.check_object_permissions(self.request, preference) + return preference + + def list(self, request, *args, **kwargs): + """Return a page of stored preferences, shaped as the caller asked.""" + return self.shape_page(super().list(request, *args, **kwargs), request) + + def retrieve(self, request, *args, **kwargs): + """Return one stored preference, shaped as the caller asked.""" + response = super().retrieve(request, *args, **kwargs) + response.data = self.shape(response.data, request) + return response + + +@extend_schema(tags=[USER_TAG]) +class CountryTimeZoneViewSet( + StandardizedErrorMixin, + CatalogedRequestErrorsMixin, + _ActionAddressMixin, + _StrictQueryParametersMixin, + IterablePaginationMixin, + viewsets.ViewSet, +): + """ + Time zones commonly used in a country, or every time zone when no country is named. + + The rows come from the time-zone database rather than from a queryset, so + this is a plain ViewSet whose one query parameter is validated by a + serializer. The reference data it returns is the same for every caller and + is public on purpose. + """ + + permission_classes = (permissions.AllowAny,) + serializer_class = TimeZoneSerializer + #: Validates the query string, and names the parameters this view reads. + query_serializer_class = TimeZoneQuerySerializer + pagination_class = UserApiPagination + + def get_serializer(self, *args, **kwargs): + """Build the output serializer; a plain ViewSet has no generic machinery to do it.""" + return self.serializer_class(*args, **kwargs) + + @extend_schema( + summary="List time zones", + description=( + "Returns a page of time zones, ordered by their display form. Supply " + "`country_code` to receive the time zones commonly used in one country." + ), + # The parameters are published from the serializer that validates them, + # so the constraint a caller is held to is the one it reads. + parameters=[TimeZoneQuerySerializer], + responses={ + 200: TimeZoneSerializer, + 400: _RESP_BAD_REQUEST, + 404: _RESP_NO_SUCH_PAGE, + }, + ) + def list(self, request): + """Return a page of time zones, for one country or for the world.""" + query = self.query_serializer_class(data=request.query_params) + query.is_valid(raise_exception=True) + time_zones = get_country_time_zones(query.validated_data.get("country_code")) + return self.paginate_iterable(request, time_zones) + + +@extend_schema(exclude=True) +class UnknownAddressView(StandardizedErrorMixin, APIView): + """ + Refuses every address under this version of the API that no route serves. + + It serves no resource: it exists so that a mistyped address is answered + with the same JSON body as every other failure instead of the service's + HTML error page, which a client that asked for JSON cannot read. + """ + + permission_classes = (permissions.AllowAny,) + + def initial(self, request, *args, **kwargs): + """Refuse the address before a handler is chosen, whatever the method.""" + raise NotFound From a8eecf25d307bdadd993e3c4e8752a96be83a7a8 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Thu, 15 Oct 2026 03:41:09 +0500 Subject: [PATCH 2/2] feat: publish the user API in the LMS schema The LMS drf-spectacular pre-processing hook is an allowlist and admitted only /api/enrollment/vN/, so everything under /api/user/ generated, routed and tested while being invisible in the published document. The new v2 addresses would have shipped undocumented and the schema gate would have reported the prefix missing with no clue as to why. The filter now admits exactly what this change owns: the v2 addresses, matched by their view package rather than by path prefix, plus the seven legacy paths they supersede, so each superseded address appears beside its replacement. It is deliberately not a /api/user/vN/ prefix. Admitting the whole prefix pulled thirty-eight further v1 operations into the document -- undocumented request bodies and internal docstrings published as client-facing text -- none of which this change owns or documents. SCHEMA_PATH_PREFIX becomes a two-branch pattern, stripped only to derive tags and operationIds. Every existing enrollment operationId was compared before and after and is unchanged, and so is every operationId, path and component of the service-wide /api-docs document: a generated client derives a module per operationId and a model per component, so a rename there deletes a module as surely as a removal does. The title follows the contents, and the three comments that named the old title follow it. Deprecation is marked where it belongs, outside the frozen modules: the existing post-processing hook gains a tuple of the seven exact legacy paths this change supersedes. Exact paths, not a prefix, so an address whose replacement nobody has written yet can never be marked -- the hook test includes neighbouring user addresses, the unversioned twins and another area's paths and asserts they are left alone. POSTPROCESSING_HOOKS is unchanged: setting it replaces drf-spectacular's defaults, so the enum hook stays restated and stays first. --- lms/envs/common.py | 11 +- lms/envs/devstack.py | 2 +- lms/envs/production.py | 2 +- lms/lib/spectacular.py | 84 +++++++++++-- lms/lib/tests/test_spectacular.py | 193 ++++++++++++++++++++++++++++-- openedx/core/apidocs.py | 2 +- 6 files changed, 264 insertions(+), 30 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index e4bbaa9ff5a2..e8f4d54ac0b5 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2157,22 +2157,21 @@ add_optional_apps(OPTIONAL_APPS, INSTALLED_APPS) # noqa: F405 -###################### drf-spectacular (LMS enrollment schema) ###################### +###################### drf-spectacular (LMS API schema) ###################### SPECTACULAR_SETTINGS = { - 'TITLE': 'LMS Enrollment API', + 'TITLE': 'LMS API', 'VERSION': '0.1.0', 'SERVE_INCLUDE_SCHEMA': False, 'PREPROCESSING_HOOKS': ['lms.lib.spectacular.lms_api_filter'], - # Mark legacy slashless enrollment addresses deprecated. The enum hook is - # drf-spectacular's default, restated because setting this key replaces - # the default list. + # Mark superseded addresses deprecated. The enum hook is drf-spectacular's + # default, restated because setting this key replaces the default list. 'POSTPROCESSING_HOOKS': [ 'drf_spectacular.hooks.postprocess_schema_enums', 'lms.lib.spectacular.lms_mark_legacy_paths_deprecated', ], # Stripped from each path to derive its tags and operationId, so changing # it renames every operationId. Paths themselves are emitted in full. - 'SCHEMA_PATH_PREFIX': '/api/enrollment', + 'SCHEMA_PATH_PREFIX': r'/api/(enrollment|user)', # Serve the Swagger UI and ReDoc assets from drf-spectacular-sidecar rather # than drf-spectacular's default unpinned jsdelivr CDN URLs, keeping them # self-hosted and version-pinned as the drf-yasg bundles were. diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index 789945a32c40..e74a18aaf8ce 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -583,7 +583,7 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs 'COMPLETE', ] -###################### drf-spectacular (LMS enrollment schema) ###################### +###################### drf-spectacular (LMS API schema) ###################### SPECTACULAR_SETTINGS['SERVERS'] = [ # noqa: F405 {'url': LMS_ROOT_URL, 'description': 'Local'}, # noqa: F405 ] diff --git a/lms/envs/production.py b/lms/envs/production.py index 24350824c391..1defebe32992 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -398,7 +398,7 @@ def get_env_setting(setting): MIDDLEWARE.extend(_YAML_TOKENS.get('EXTRA_MIDDLEWARE_CLASSES', [])) # noqa: F405 -###################### drf-spectacular (LMS enrollment schema) ###################### +###################### drf-spectacular (LMS API schema) ###################### SPECTACULAR_SETTINGS['SERVERS'] = [ # noqa: F405 {'url': LMS_ROOT_URL, 'description': 'Local'}, # noqa: F405 ] diff --git a/lms/lib/spectacular.py b/lms/lib/spectacular.py index 942199deac69..cfd310de404a 100644 --- a/lms/lib/spectacular.py +++ b/lms/lib/spectacular.py @@ -11,21 +11,70 @@ # addresses already have distinct ids and keep them. COLLIDING_LEGACY_PATH = "/api/enrollment/v2/enrollments" +# Addresses listed here are superseded by a conforming address this schema also +# publishes. Each entry is spelled out rather than matched by prefix, so an +# address whose replacement has not been written yet is never marked. +SUPERSEDED_USER_PATHS = ( + "/api/user/v1/users/", + "/api/user/v1/users/{id}/", + "/api/user/v1/user_prefs/", + "/api/user/v1/user_prefs/{id}/", + "/api/user/v1/preferences/{pref_key}/users/", + "/api/user/v1/forum_roles/{name}/users/", + "/api/user/v1/preferences/time_zones/", +) + +PUBLISHED_PATH_PATTERN = re.compile(r"^/api/enrollment/v\d+/") + +# The user addresses this schema documents are the ones the version below +# serves, and beside each of those the address it supersedes. Apps older than +# that version are mounted under the same ``/api/user/v2/`` prefix, so matching +# the prefix alone would publish endpoints described nowhere here; the package +# a view comes from decides instead. +PUBLISHED_USER_PREFIX = "/api/user/v2/" +PUBLISHED_USER_VIEW_MODULE = "openedx.core.djangoapps.user_api.v2." + +# Every user address, whatever its version, belongs to one tag group. A tag is +# the first path segment left once the configured prefix is removed, which in a +# document that trims ``/api/user`` is the version, so user operations would be +# grouped with the enrollment operations that carry the same version. A document +# that trims nothing already derives a tag naming an area, and regrouping its +# operations would move them in every client generated from it. +USER_PATH_PREFIX = "/api/user/" +USER_TAG = "user" + +_PATH_PARAMETER = re.compile(r"\{([\w-]+)\}") +_VERSION_TAG = re.compile(r"v\d+") + +# The superseded addresses with their parameter names removed. The generator +# renames a ``{pk}`` parameter to ``{id}`` only after this filter has run, so +# the comparison cannot depend on how a parameter is spelled. +_SUPERSEDED_USER_SHAPES = frozenset(_PATH_PARAMETER.sub("{}", path) for path in SUPERSEDED_USER_PATHS) + def _is_legacy_path(path): """Conforming routes always end in a slash, so a slashless v2 path is a legacy address.""" return path.startswith(LEGACY_V2_PREFIX) and not path.endswith("/") +def _is_published_user_path(path, callback): + """A user address is documented when this version serves it, or when this version replaces it.""" + if _PATH_PARAMETER.sub("{}", path) in _SUPERSEDED_USER_SHAPES: + return True + if not path.startswith(PUBLISHED_USER_PREFIX): + return False + view = getattr(callback, "cls", None) + return getattr(view, "__module__", "").startswith(PUBLISHED_USER_VIEW_MODULE) + + def lms_api_filter(endpoints): """ - Pre-processing hook: keep only enrollment v2 endpoints tagged for the SDK. + Pre-processing hook: keep only the endpoints this schema documents. """ filtered = [] - ENROLLMENT_PATH_PATTERN = re.compile(r"^/api/enrollment/v\d+/") for path, path_regex, method, callback in endpoints: - if ENROLLMENT_PATH_PATTERN.match(path): + if PUBLISHED_PATH_PATTERN.match(path) or _is_published_user_path(path, callback): filtered.append((path, path_regex, method, callback)) return filtered @@ -33,26 +82,37 @@ def lms_api_filter(endpoints): class LmsAutoSchema(AutoSchema): """ - Give the colliding legacy Enrollment v2 address a distinct operationId. - - It and its conforming twin tokenize to the same operationId, so - drf-spectacular would otherwise break the tie with a numeral suffix in - registration order. The conforming address keeps the clean id; the - deprecated one is suffixed ``_legacy``. + Keep the one colliding address distinct, and group each operation under its own area. + + The slashless Enrollment v2 address tokenizes to the id of its conforming + twin, so the two would be told apart only by a numeral suffix handed out + in registration order, which says nothing about the operation and moves + when the routes move; it is suffixed ``_legacy`` instead, leaving the + conforming address the clean id. Every other address keeps the id and the + tag the generator derives for it, because a client generated from a + document takes a module from each operationId and groups it by tag, so + renaming either drops what a consumer already imports. """ def get_operation_id(self): - """Suffix the colliding legacy address so the pair never shares an operationId.""" + """Suffix the one colliding address so the pair never shares an operationId.""" operation_id = super().get_operation_id() if self.path == COLLIDING_LEGACY_PATH: return f"{operation_id}_legacy" return operation_id + def get_tags(self): + """Name the area whenever the tag derived for a user address would name a version.""" + tags = super().get_tags() + if self.path.startswith(USER_PATH_PREFIX) and tags and _VERSION_TAG.fullmatch(tags[0]): + return [USER_TAG] + return tags + def lms_mark_legacy_paths_deprecated(result, generator, request, public): # pylint: disable=unused-argument - """Mark the legacy slashless Enrollment v2 addresses ``deprecated: true``.""" + """Mark every superseded address ``deprecated: true``.""" for path, path_item in result.get("paths", {}).items(): - if not _is_legacy_path(path): + if not (_is_legacy_path(path) or path in SUPERSEDED_USER_PATHS): continue for operation in path_item.values(): if isinstance(operation, dict): diff --git a/lms/lib/tests/test_spectacular.py b/lms/lib/tests/test_spectacular.py index b611c8ad7776..66c193901f33 100644 --- a/lms/lib/tests/test_spectacular.py +++ b/lms/lib/tests/test_spectacular.py @@ -1,20 +1,33 @@ """Tests for the LMS drf-spectacular hooks and schema class.""" +import re +from collections import defaultdict +from types import SimpleNamespace from unittest import mock -from django.test import SimpleTestCase +from django.test import SimpleTestCase, TestCase from django.urls import path from drf_spectacular.generators import SchemaGenerator -from drf_spectacular.settings import spectacular_settings +from drf_spectacular.settings import patched_settings, spectacular_settings from rest_framework import serializers, viewsets +from rest_framework.permissions import AllowAny from rest_framework.response import Response from rest_framework.settings import api_settings from lms.lib.spectacular import LmsAutoSchema, lms_api_filter, lms_mark_legacy_paths_deprecated +from openedx.core.apidocs import get_api_docs_settings +ENROLLMENT_VIEWS = "openedx.core.djangoapps.enrollments.views" +USER_API_V1_VIEWS = "openedx.core.djangoapps.user_api.views" +USER_API_V2_VIEWS = "openedx.core.djangoapps.user_api.v2.views" +USER_AUTHN_VIEWS = "openedx.core.djangoapps.user_authn.views.register" +COURSE_VIEWS = "lms.djangoapps.course_api.views" -def _endpoint(route): - return (route, route, "GET", None) + +def _endpoint(route, view_module=COURSE_VIEWS): + """An endpoint tuple as the generator hands it to the hook, served from ``view_module``.""" + callback = SimpleNamespace(cls=type("_View", (), {"__module__": view_module})) + return (route, route, "GET", callback) def _schema(*routes): @@ -22,19 +35,45 @@ def _schema(*routes): class LmsApiFilterTest(SimpleTestCase): - """The pre-processing hook keeps only enrollment endpoints.""" + """The hook keeps the enrollment mounts, this version of the user mounts, and what they supersede.""" - def test_keeps_enrollment_drops_the_rest(self): + def test_keeps_enrollment_this_version_and_the_addresses_it_supersedes(self): kept = lms_api_filter([ - _endpoint("/api/enrollment/v2/enrollments/"), - _endpoint("/api/enrollment/v1/enrollment"), - _endpoint("/api/user/v1/accounts/"), + _endpoint("/api/enrollment/v2/enrollments/", ENROLLMENT_VIEWS), + _endpoint("/api/enrollment/v1/enrollment", ENROLLMENT_VIEWS), + _endpoint("/api/user/v2/users/", USER_API_V2_VIEWS), + _endpoint("/api/user/v2/user_preferences/{username},{key}/", USER_API_V2_VIEWS), + # Superseded, and spelled with the parameter name the generator has + # not renamed to ``{id}`` yet. + _endpoint("/api/user/v1/users/{pk}/", USER_API_V1_VIEWS), + _endpoint("/api/user/v1/preferences/time_zones/", USER_API_V1_VIEWS), + # Neither served nor superseded by this version. + _endpoint("/api/user/v2/account/registration/", USER_AUTHN_VIEWS), + _endpoint("/api/user/v1/accounts", USER_API_V1_VIEWS), + _endpoint("/api/user/v1/me", USER_API_V1_VIEWS), + _endpoint("/api/user/v1/preferences/{username}", USER_API_V1_VIEWS), + _endpoint("/api/courses/v1/courses/", COURSE_VIEWS), + _endpoint("/api/user_tours/v1/tours/"), + _endpoint("/user_api/v1/users/", USER_API_V1_VIEWS), ]) assert [path for path, *_ in kept] == [ "/api/enrollment/v2/enrollments/", "/api/enrollment/v1/enrollment", + "/api/user/v2/users/", + "/api/user/v2/user_preferences/{username},{key}/", + "/api/user/v1/users/{pk}/", + "/api/user/v1/preferences/time_zones/", ] + def test_under_the_version_prefix_the_view_package_decides(self): + """Other apps are mounted under the same prefix, so the path alone cannot admit an address.""" + route = "/api/user/v2/account/registration/" + admitted = { + view_module: [path for path, *_ in lms_api_filter([_endpoint(route, view_module)])] + for view_module in (USER_AUTHN_VIEWS, USER_API_V2_VIEWS) + } + assert admitted == {USER_AUTHN_VIEWS: [], USER_API_V2_VIEWS: [route]} + class LmsMarkLegacyPathsDeprecatedTest(SimpleTestCase): """Slashless v2 paths are legacy and deprecated; slashed ones are conforming.""" @@ -54,6 +93,25 @@ def test_only_slashless_v2_paths_are_deprecated(self): assert "deprecated" not in paths["/api/enrollment/v2/courses/{course_key}/"]["get"] assert "deprecated" not in paths["/api/enrollment/v1/enrollment"]["get"] + def test_only_the_listed_user_addresses_are_deprecated(self): + result = lms_mark_legacy_paths_deprecated(_schema( + "/api/user/v1/users/", + "/api/user/v1/users/{id}/", + "/api/user/v1/preferences/time_zones/", + "/api/user/v1/accounts", + "/api/user/v1/preferences/{username}", + "/api/user/v2/users/", + "/user_api/v1/users/", + ), None, None, False) + paths = result["paths"] + assert paths["/api/user/v1/users/"]["get"]["deprecated"] is True + assert paths["/api/user/v1/users/{id}/"]["get"]["deprecated"] is True + assert paths["/api/user/v1/preferences/time_zones/"]["get"]["deprecated"] is True + assert "deprecated" not in paths["/api/user/v1/accounts"]["get"] + assert "deprecated" not in paths["/api/user/v1/preferences/{username}"]["get"] + assert "deprecated" not in paths["/api/user/v2/users/"]["get"] + assert "deprecated" not in paths["/user_api/v1/users/"]["get"] + def test_paths_are_left_as_full_urls(self): """Paths are not trimmed, so every key resolves against LMS_ROOT_URL.""" result = lms_mark_legacy_paths_deprecated(_schema("/api/enrollment/v2/enrollments/"), None, None, False) @@ -99,3 +157,120 @@ def test_only_the_colliding_legacy_address_is_suffixed(self): assert ids["/api/enrollment/v2/courses/{id}/"] == "v2_courses_retrieve" # Already distinct from every conforming id, so it keeps its name. assert ids["/api/enrollment/v2/enrollment/{id}"] == "v2_enrollment_retrieve" + + +class _UserApiViewSet(viewsets.GenericViewSet): + """Stand-in for a user view read both as a collection and as a member.""" + + serializer_class = _EmptySerializer + permission_classes = (AllowAny,) + + def list(self, request): + return Response([]) + + def retrieve(self, request, **kwargs): + return Response({}) + + +def _user_routes(schema_class): + """User, enrollment and non-``/api/`` addresses, inspected by ``schema_class``.""" + view = type("_InspectedViewSet", (_UserApiViewSet,), {"schema": schema_class()}) + return [ + path("api/user/v1/accounts", view.as_view({"get": "retrieve"})), + path("api/user/v1/accounts/", view.as_view({"get": "retrieve"})), + path("api/user/v1/preferences/", view.as_view({"get": "retrieve"})), + path( + "api/user/v1/preferences//", + view.as_view({"get": "retrieve"}), + ), + path("api/user/v1/users/", view.as_view({"get": "list"})), + path("api/user/v2/users/", view.as_view({"get": "list"})), + path("api/enrollment/v1/enrollments/", view.as_view({"get": "list"})), + # Outside ``/api/``, so a document that derives its prefix from the + # routes it covers derives the one the service-wide document has. + path("user_api/v1/preferences/", view.as_view({"get": "retrieve"})), + ] + + +def _named_operations(schema_class, patches): + """The ``{address: (operationId, tags)}`` ``schema_class`` produces under ``patches``.""" + with patched_settings({"PREPROCESSING_HOOKS": [], **patches}): + schema = SchemaGenerator(patterns=_user_routes(schema_class)).get_schema(request=None, public=True) + return { + route: (operation["operationId"], tuple(operation["tags"])) + for route, path_item in schema["paths"].items() + for operation in path_item.values() + } + + +class UserOperationIdTest(SimpleTestCase): + """User addresses keep the names the generator derives; only their tag group is chosen here.""" + + def test_the_service_wide_document_keeps_every_name_the_generator_derives(self): + """Each of these is the name that document already published, and a client imports it by name.""" + assert _named_operations(LmsAutoSchema, get_api_docs_settings()) == { + "/api/user/v1/accounts": ("api_user_v1_accounts_retrieve", ("api",)), + "/api/user/v1/accounts/{username}": ("api_user_v1_accounts_retrieve_2", ("api",)), + "/api/user/v1/preferences/{username}": ("api_user_v1_preferences_retrieve", ("api",)), + "/api/user/v1/preferences/{username}/{preference_key}": + ("api_user_v1_preferences_retrieve_2", ("api",)), + "/api/user/v1/users/": ("api_user_v1_users_list", ("api",)), + "/api/user/v2/users/": ("api_user_v2_users_list", ("api",)), + "/api/enrollment/v1/enrollments/": ("api_enrollment_v1_enrollments_list", ("api",)), + "/user_api/v1/preferences/{username}": ("user_api_v1_preferences_retrieve", ("user_api",)), + } + + def test_a_user_address_is_grouped_by_area_where_the_derived_tag_would_name_a_version(self): + """In the document that trims the area off, the derived tag is the version, and ids are untouched.""" + assert _named_operations(LmsAutoSchema, {}) == { + "/api/user/v1/accounts": ("v1_accounts_retrieve", ("user",)), + "/api/user/v1/accounts/{username}": ("v1_accounts_retrieve_2", ("user",)), + "/api/user/v1/preferences/{username}": ("v1_preferences_retrieve", ("user",)), + "/api/user/v1/preferences/{username}/{preference_key}": ("v1_preferences_retrieve_2", ("user",)), + "/api/user/v1/users/": ("v1_users_list", ("user",)), + "/api/user/v2/users/": ("v2_users_list", ("user",)), + "/api/enrollment/v1/enrollments/": ("v1_enrollments_list", ("v1",)), + "/user_api/v1/preferences/{username}": ("user_api_v1_preferences_retrieve", ("user_api",)), + } + + +class PublishedDocumentTest(TestCase): + """What the service actually publishes: distinct ids, and one area per tag group.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.schema = SchemaGenerator().get_schema(request=None, public=True) + + @classmethod + def operations(cls): + """Yield every (path, operation) the published document carries.""" + for route, path_item in cls.schema["paths"].items(): + for operation in path_item.values(): + if isinstance(operation, dict) and "operationId" in operation: + yield route, operation + + def test_every_operation_id_is_unique(self): + ids = [operation["operationId"] for _route, operation in self.operations()] + assert sorted(ids) == sorted(set(ids)) + + def test_only_the_one_enrollment_pair_is_told_apart_by_a_numeral_suffix(self): + """A numeral suffix is assigned in route order, so it is not a stable name.""" + suffixed = { + route: operation["operationId"] + for route, operation in self.operations() + if re.fullmatch(r".+_\d+", operation["operationId"]) + } + # Renaming this one would change an id the document already published, so it stays. + assert suffixed == {"/api/enrollment/v1/enrollment/{course_id}": "v1_enrollment_retrieve_2"} + + def test_no_tag_group_holds_two_areas(self): + areas = defaultdict(set) + for route, operation in self.operations(): + for tag in operation.get("tags", []): + areas[tag].add(route.split("/")[2]) + assert {tag: sorted(members) for tag, members in areas.items()} == { + "v1": ["enrollment"], + "openedx-platform-sdk": ["enrollment"], + "user": ["user"], + } diff --git a/openedx/core/apidocs.py b/openedx/core/apidocs.py index 4c96c8cce109..acf4de94ce16 100644 --- a/openedx/core/apidocs.py +++ b/openedx/core/apidocs.py @@ -21,7 +21,7 @@ # These are passed as ``custom_settings`` to SpectacularAPIView rather than # living in ``SPECTACULAR_SETTINGS``, because that global is already claimed by # a deliberately narrow schema in each service -- the Authoring API -# (``/authoring-api/``) in CMS and the Enrollment API (``/lms-api/``) in LMS. +# (``/authoring-api/``) in CMS and the LMS API (``/lms-api/``) in LMS. # Both filter the surface down via ``PREPROCESSING_HOOKS`` and trim a path # prefix, so ``/api-docs`` must switch that filtering off explicitly to cover # the whole service.