diff --git a/docs/decisions/0025-standardize-serializer-usage.rst b/docs/decisions/0025-standardize-serializer-usage.rst new file mode 100644 index 000000000000..780e7ed22ecc --- /dev/null +++ b/docs/decisions/0025-standardize-serializer-usage.rst @@ -0,0 +1,113 @@ +Standardize Serializer Usage Across APIs +======================================== + +:Status: Proposed +:Date: 2026-03-09 +:Deciders: API Working Group +:Technical Story: Open edX REST API Standards - Serializer standardization for consistency + +Context +------- + +Many Open edX platform API endpoints manually construct JSON responses using Python dictionaries instead of Django REST Framework (DRF) serializers. This leads to inconsistent schema responses, makes validation errors harder to manage, and creates unpredictable formats that AI and third-party systems struggle with. + +Decision +-------- + +We will standardize all Open edX REST APIs to use **DRF serializers** for request and response handling. + +Implementation requirements: + +* All API views MUST define explicit serializers for request and response handling. +* Replace manual JSON construction with serializer-based responses. +* Use serializers for both input validation and output formatting. +* Ensure serializers are properly documented with field descriptions and validation rules. +* Maintain backward compatibility for all APIs during migration. While the goal is fully compatible DRF serializers, if that is not possible and we must make a backwards incompatible change, that change MUST be handled by creating a new version of the API and transitioning to that API using the deprecation process. + +Relevance in edx-platform +------------------------- + +Current patterns that should be migrated: + +* **Certificates API** (``/api/certificates/v0/``) constructs JSON manually with nested dictionaries. +* **Enrollment API** endpoints manually build response objects without serializers. +* **Course API** views use hand-coded JSON responses instead of structured serializers. + +Code example (target serializer usage) +-------------------------------------- + +**Example serializer and APIView using DRF best practices:** + +.. code-block:: python + + # serializers.py + from rest_framework import serializers + + class CertificateSerializer(serializers.Serializer): + username = serializers.CharField( + help_text="The username of the certificate holder" + ) + course_id = serializers.CharField( + help_text="The course identifier" + ) + status = serializers.CharField( + help_text="The certificate status (e.g., downloadable, generating)" + ) + grade = serializers.FloatField( + help_text="The final grade achieved" + ) + + # views.py + from rest_framework.views import APIView + from rest_framework.response import Response + from rest_framework import status + + class CertificateAPIView(APIView): + def get(self, request): + data = { + "username": "john_doe", + "course_id": "course-v1:edX+DemoX+1T2024", + "status": "downloadable", + "grade": 0.95, + } + serializer = CertificateSerializer(data) + return Response(serializer.data, status=status.HTTP_200_OK) + +Consequences +------------ + +Positive +~~~~~~~~ + +* Simplifies validation and ensures consistent response contracts. +* Improves AI compatibility through predictable data structures. +* Enables automatic schema generation and documentation. +* Reduces code duplication and maintenance overhead. + +Negative / Trade-offs +~~~~~~~~~~~~~~~~~~~~~ + +* Requires refactoring existing endpoints that manually construct JSON. +* Initial development overhead for creating comprehensive serializers. +* May require updates to existing client code that expects legacy formats. + +Alternatives Considered +----------------------- + +* **Keep manual JSON construction**: rejected due to inconsistency and maintenance burden. +* **Use DRF defaults only**: rejected because explicit serializers provide better validation and documentation. +* **Use newer ways of managing API responses such as dataclasses or pydantic**: rejected due to complexity and unknowns in transitioning from two existing patterns (manual JSON and DRF serializers) to a third approach. While these python libraries offer better ergonomics, migration would require checking nested serializers, complex validation, and ModelSerializer-heavy endpoints. To move to some new format, we would want to prevent using the basic DRF Serializers any more than we do right now, but preventing new DRF serializers via linting is more complex than anticipated. This work can be revisited in the future once the platform is a bit more consistent. + +Rollout Plan +------------ + +1. Audit existing endpoints to identify those using manual JSON construction. +2. Create a library of common serializers for shared data structures. +3. Migrate high-impact endpoints first (certificates, enrollment, courses). +4. Update tests to validate serializer-based responses. +5. Update API documentation to reflect new serializer-based contracts. + +References +---------- + +* Open edX REST API Standards: "Serializer Usage" recommendations for API consistency. diff --git a/docs/decisions/0038-introduce-row-level-security-layer.rst b/docs/decisions/0038-introduce-row-level-security-layer.rst new file mode 100644 index 000000000000..e7e8c1e11dc6 --- /dev/null +++ b/docs/decisions/0038-introduce-row-level-security-layer.rst @@ -0,0 +1,199 @@ +Introduce a Row-Level Security Layer +==================================== + +:Status: Proposed +:Date: 2026-04-14 +:Deciders: API Working Group +:Technical Story: Open edX REST API Standards – Row-level security for list endpoints + +Context +------- + +List endpoints must return only items a user is allowed to see. Today, many APIs rely on ad-hoc +``has_access`` checks inside views, mixing three distinct concerns — endpoint access control, +record visibility filtering, and user-driven filtering — into a single block of view logic. This +makes behavior inconsistent across APIs, harder to audit, and more difficult to test. + +The platform already contains ~150 call sites for ``has_access`` +(``lms/djangoapps/courseware/access.py``) and several similar helpers such as +``has_studio_read_access()``, ``has_course_author_access()``, and ``get_course_with_access()``. +These are applied inline in views rather than through a reusable, composable layer. + +Relevance in edx-platform +-------------------------- + +Current patterns that should be refactored: + +* **CMS contentstore views** (``cms/djangoapps/contentstore/views/course.py``) call + ``has_studio_read_access()`` and ``has_course_author_access()`` inline to decide which courses a + user can see or modify, mixing endpoint-level guards with per-record visibility. +* **Enrollment API** (``openedx/core/djangoapps/enrollments/views.py``) performs inline + ``user_has_role`` checks against ``CourseStaffRole`` and ``GlobalStaff`` rather than separating + permission classes from queryset scoping. +* **Discussion REST API** (``lms/djangoapps/discussion/rest_api/views.py``) embeds per-record + access decisions directly in view logic alongside user-driven filters. +* **Course Blocks API** (``lms/djangoapps/course_api/blocks/views.py``) imports + ``lms.djangoapps.courseware.access.has_access`` to filter blocks by user after retrieval rather + than scoping the data source upfront. + +Relationship to ADR-0003 (Bridgekeeper) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +An existing ADR (``lms/djangoapps/courseware/docs/decisions/0003-permissions-via-bridgekeeper.rst``) +proposes converting ``has_access`` calls to ``bridgekeeper``-based named permissions with queryset +filtering support. The current ADR is **complementary, not competing**: ADR-0003 addresses the +*permission engine* (how individual access checks are expressed and evaluated), while this ADR +defines the *architectural pattern* that views must follow (how endpoint access, record visibility, +and user-driven filtering are separated into distinct layers). + +If bridgekeeper is adopted, its queryset-capable rules can serve as the implementation behind the +RLS scoping layer described here. If bridgekeeper is not adopted or is replaced by another +mechanism, the three-layer separation of concerns still applies. This ADR is intentionally +engine-agnostic. + +Decision +-------- + +1. Define a reusable **Row-Level Security (RLS)** layer that governs record visibility by user and + role. +2. Separate three concerns in every list endpoint: + + * **Endpoint access** — DRF permission classes (``permission_classes``). Answers: "Can this + user call this endpoint at all?" + * **Record visibility** — RLS policy applied in ``get_queryset()``. Answers: "Which rows is + this user allowed to see?" + * **User-driven filtering** — ``django-filter`` FilterSets or DRF filter backends. Answers: + "Which subset of visible rows did the user request?" + +3. Require that list endpoints use RLS-scoped querysets by default. + +Code example (target view structure) +------------------------------------- + +**Separation of concerns with a Django ORM-backed endpoint:** + +.. code-block:: python + + # permissions.py — Endpoint access: DRF permission class + from rest_framework.permissions import BasePermission, IsAuthenticated + from common.djangoapps.student.auth import has_studio_read_access + + class HasCourseStaffAccess(BasePermission): + """Checks that the user has staff-level read access to the course.""" + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return has_studio_read_access(request.user, course_key) + +.. code-block:: python + + # views.py — Record visibility via RLS-scoped queryset + user-driven filtering + from django_filters.rest_framework import DjangoFilterBackend + from rest_framework import viewsets + from rest_framework.permissions import IsAuthenticated + + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview + from .permissions import HasCourseStaffAccess + from .serializers import CourseOverviewSerializer + + class CourseOverviewViewSet(viewsets.ReadOnlyModelViewSet): + """ + List and retrieve course overviews visible to the requesting user. + """ + serializer_class = CourseOverviewSerializer + permission_classes = [IsAuthenticated, HasCourseStaffAccess] + filter_backends = [DjangoFilterBackend] + filterset_fields = ["org"] + + def get_queryset(self): + # RLS layer: scope the queryset to rows this user may see. + qs = CourseOverview.objects.all() + return rls_scope_courses(qs, self.request.user) + +**Reusable RLS mixin (target utility):** + +.. code-block:: python + + class RLSQuerysetMixin: + """ + Mixin that delegates queryset scoping to a pluggable ``rls_policy``. + + Subclasses set ``rls_policy`` to an object that implements + ``scope(queryset, user) -> queryset``. + """ + rls_policy = None # Must be set by the concrete view + + def get_queryset(self): + qs = super().get_queryset() + if self.rls_policy is None: + raise ImproperlyConfigured( + f"{self.__class__.__name__} must define rls_policy." + ) + return self.rls_policy.scope(qs, self.request.user) + +Consequences +------------ + +Positive +~~~~~~~~ + +* Clear, auditable security model: each concern is testable in isolation. +* Consistent results across APIs — every list endpoint follows the same pattern. +* Performance improvement for list views: queryset-level scoping avoids the current + pattern of loading all records, then filtering in Python. +* Easier onboarding for new contributors who can follow a single, documented pattern. + +Negative / Trade-offs +~~~~~~~~~~~~~~~~~~~~~ + +* Requires shared abstractions (mixin, policy interface) to be built and maintained. +* Migration work for existing list endpoints that interleave access checks with view logic. +* Not all data sources are backed by the Django ORM (e.g., modulestore XBlocks). For non-ORM + sources, the RLS policy degrades to an in-memory filter rather than a queryset annotation, but + the architectural separation still applies. +* ``django-filter`` is already a transitive dependency of edx-platform; this ADR promotes it to a + first-class, required pattern for user-driven filtering, which increases coupling to that + library. + +Alternatives Considered +----------------------- + +* **Keep ad-hoc ``has_access`` checks inside views**: rejected because the current pattern mixes + three distinct concerns, produces inconsistent behavior across APIs, and is difficult to audit + or test. +* **Rely solely on bridgekeeper (ADR-0003)**: bridgekeeper provides a permission engine with + queryset support but does not prescribe how views should be structured. Adopting bridgekeeper + alone would not guarantee that every list endpoint cleanly separates endpoint access, record + visibility, and user-driven filtering. This ADR layers an architectural pattern on top of + whatever permission engine is in use. +* **Use Django-Guardian for object-level permissions**: rejected because Django-Guardian requires + per-object permission rows in the database, which does not scale well for the volume of + course content in a typical Open edX installation. RLS queryset scoping derives visibility + from existing role and enrollment data without additional storage. + +Rollout Plan +------------ + +1. Build shared ``RLSQuerysetMixin`` and define the ``RLSPolicy`` interface (``scope(qs, user)``). +2. Audit existing list endpoints to catalogue how each one currently handles access and filtering. +3. Migrate high-impact, ORM-backed endpoints first +4. For modulestore-backed endpoints (e.g., Course Blocks), apply the same architectural + separation using in-memory filtering behind the same policy interface. +5. Add linting or CI checks to enforce that new list endpoints include an ``rls_policy`` or + equivalent scoping in ``get_queryset()``. +6. Update tests to validate that each endpoint returns only the records the requesting user + should see. +7. Update API documentation to reflect the standardized pattern. + +References +---------- + +* Existing ADR: ``lms/djangoapps/courseware/docs/decisions/0003-permissions-via-bridgekeeper.rst`` + — permission engine migration from ``has_access`` to named permissions. +* "Homogenize row level security" recommendation in the Open edX REST API standardization notes. +* Django REST Framework filtering documentation: + https://www.django-rest-framework.org/api-guide/filtering/ +* ``django-filter`` DRF integration: + https://django-filter.readthedocs.io/en/stable/guide/rest_framework.html \ No newline at end of file