[FC-0118] docs: add ADR for introducing a row-level security layer - #38354
Conversation
…ble change (openedx#38188) Co-authored-by: Muhammad Faraz Maqsood <faraz.maqsood@A006-01130.local>
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Should this be an update to OEP-66 instead of an ADR?
How does this integrate with Casbin?
When the user requests a single object from a GET endpoint, does this still use a filter that could involve complicated/slow logic to construct, or does it just query the permissions of that one single object? i.e. is getting a single object more like "list all the courses the user is allowed to see and then select one" or is it like "check if the user is allowed to see course X".
Introduce an architectural decision record that standardizes how list endpoints separate three concerns: endpoint access (DRF permission classes), record visibility (RLS queryset scoping), and user-driven filtering (django-filter). This complements the existing bridgekeeper ADR (0003) by defining the view-layer pattern that any permission engine plugs into, rather than prescribing a specific engine. Includes code examples using CourseOverview, a reusable RLSQuerysetMixin, and a rollout plan targeting course listing, enrollment, and certificate endpoints first.
985f725 to
4ea6c8f
Compare
What we can do is taht take the pattern part (three-layer separation, |
|
I don't have a strong opinion on how you structure it; I'm just trying to understand how all the pieces fit together. |
| ``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) |
There was a problem hiding this comment.
We should also add a section with respect to https://github.com/openedx/openedx-authz/tree/main/docs/decisions
That repo is the future of AuthZ in the openedx-platform so we should talk about how we expect this to work with that.
| 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) |
There was a problem hiding this comment.
Where does this function get defined, the example seems incomplete.
|
|
||
| * **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 |
There was a problem hiding this comment.
Within the get_queryset(), how will permissions be checked? How will this work with the new permission framework?
adf2e7b to
ad4b7ba
Compare
|
hi @feanil @bradenmacdonald, thanks for the review and the questions on this. Based on Braden's first point, we agree this fits better as an update to OEP-66 rather than a standalone ADR. So we're going to close this PR and open a new one against OEP-66 with the same proposal, and we'll address the review comments raised here in that PR |
|
Gotcha, I think I misunderstood what you were saying in the meeting earlier. I'm good with closing this and opening the upstream OEP PR instead. |
|
Closing this PR in favour of openedx/openedx-proposals#802, since it's been moved to OEP-66. |
…t endpoints (#802) * docs: extend OEP-66 with row-level security pattern for list endpoints Adds a Django REST Framework subsection to OEP-66 covering the separation of three authorization concerns in list endpoints: endpoint access (permission_classes), record visibility (an RLS policy in get_queryset), and user-driven filtering (django-filter). Record visibility delegates to openedx-authz via get_scopes_for_subject_and_permission rather than per-row enforcement, and documents the list vs. single-object access paths and the modulestore fallback. Supersedes the standalone ADR proposed in openedx/openedx-platform#38354, moving the architectural pattern into OEP-66 per review feedback. * docs: link OEP-66 row-level security change history entry to PR #802 * docs: reframe OEP-66 pattern as queryset scoping and use CourseRun examples Addresses review on PR #802: - Rename "row-level security (RLS)" to "queryset scoping" throughout. The pattern is application-layer queryset filtering, not database-enforced RLS, and is bypassable via direct ORM access. Rename the ScopingPolicy/ScopedQuerysetMixin/scoping_policy abstractions, add a note contrasting the two, and link the PostgreSQL RLS docs. - Switch examples from CourseOverview to CourseRun. CourseRun has an internal integer pk, so the single-course scope matches on course_key, and the org scope joins through catalog_course__org__short_name since CourseRun has no org column. --------- Co-authored-by: Abdul Muqadim <abdul.muqadim@A006-01811.local>
5 participants
Adds ADR proposing a reusable row-level security layer to standardize how Open edX list endpoints handle record visibility, replacing the current ad-hoc pattern of inline has_access checks mixed with endpoint guards and user-driven filtering.
Currently, ~150 has_access call sites and helpers like has_studio_read_access(), has_course_author_access(), and get_course_with_access() are applied inline across views in contentstore, enrollments, discussion, and course blocks APIs, tangling three distinct concerns into single blocks of view logic. This makes access behavior inconsistent across endpoints, difficult to audit, and hard to test in isolation. This ADR documents the decision to separate every list endpoint into three layers, endpoint access via DRF permission classes, record visibility via RLS-scoped querysets in get_queryset(), and user-driven filtering via django-filter, backed by a shared RLSQuerysetMixin and pluggable RLSPolicy interface. The rollout targets high-impact ORM-backed endpoints first (course listing, enrollment, certificates) with in-memory filtering fallback for modulestore-backed endpoints like Course Blocks. This ADR is complementary to the existing bridgekeeper ADR (0003) , it defines the architectural pattern that any permission engine plugs into, rather than prescribing a specific engine.
issue:#38282