feat: apply OEP-66 queryset-scoping pattern to Enrollments v2 API - #39037
feat: apply OEP-66 queryset-scoping pattern to Enrollments v2 API#39037Abdul-Muqadim-Arbisoft wants to merge 1 commit into
Conversation
78d3f46 to
3678f4c
Compare
| def test_filter_by_username_narrows(self): | ||
| """User-driven filter (filter_queryset) still narrows by username.""" | ||
| self.client.force_authenticate(user=self.admin) | ||
| response = self.client.get(self.url, {"username": self.learner_b.username}) |
There was a problem hiding this comment.
This should only work for admins correct? we should have the inverted test validating that users can't see eachothers enrollments by passing eachothers names in.
There was a problem hiding this comment.
yup that should be admin only. Just added test_non_admin_cannot_list_another_users_enrollments for it
|
|
||
| def scope(self, queryset, subject): | ||
| # Platform admins (IsAdminUser) see all enrollments; nothing to narrow yet. | ||
| return queryset |
There was a problem hiding this comment.
This is a lot of boilerplate for essentially a no-op on scoping, can we simplify this? Maybe we have some sort of a "FullScopePolicy" that can be used in many places for things like this?
There was a problem hiding this comment.
I think FullScopePolicy would be a good to add since any admin only list that adopts the scoping layer would otherwise end up hand writing the same pass through class, and since its a reusable DRF tooling it should be placed next to ScopingPolicy / ScopedQuerysetMixin so plugins can use it too.
I've opened openedx/edx-drf-extensions#578 adding it there, and this PR now imports it from the library. CI here will stay red until that's released; once you approve, merge and publish 10.9.0, I'll bump the pin in this PR and it should be good to merge.
Wire EnrollmentsAdminListView through ScopedQuerysetMixin from edx-drf-extensions so endpoint access, record visibility and user-driven filtering are kept separate. Platform admins may see every enrollment, so the view uses the library's FullScopePolicy for now; a narrower policy can replace it later without touching the view. The form-based filtering moves from get_queryset() into filter_queryset(), so it only narrows the scoped queryset. No behavior change. Adds regression tests for the admin list: 401/403, a regular user filtering by someone else's username gets 403 with no data, admins see all rows, the course_key/username filters, the 400 on invalid params, and the ADR 0033 Deprecation header. Requires edx-drf-extensions 10.9.0 (edx-drf-extensions#578). Follows up openedx#38847, whose shared scoping tooling moved to edx-drf-extensions in #569.
3678f4c to
b106b61
Compare
2 participants
Adopt the OEP-66 "Separating Authorization Concerns in List Endpoints" record-visibility layer on the admin enrollment list, using the shared building blocks now published in edx-drf-extensions.
EnrollmentsAdminListView is an ORM-backed ListAPIView, so it wires the three authorization concerns separately:
Bumps edx-drf-extensions 10.6.0 -> 10.7.0, the release that adds the reusable ScopingPolicy (a typing.Protocol) and ScopedQuerysetMixin. Adds TestEnrollmentsAdminListView regression tests covering endpoint access (401/403), pass-through scoping (admin sees all rows), the course_key/username filters, the 400-on-invalid-params path, the ADR 0033 Deprecation header, and the scoping-policy pass-through.
Follows up the closed PR #38847: per review, the shared ScopingPolicy / ScopedQuerysetMixin tooling moved to edx-drf-extensions (#569) as a subject-based typing.Protocol with a duck-typed mixin check, and this change consumes it rather than defining it locally.