Skip to content

feat: enforce AuthZ permissions in the Cohorts tab endpoints - #328

Draft
efortish wants to merge 1 commit into
bav/instructor-authz-shared-infrafrom
ks/issue-492-cohorts-authz
Draft

efortish wants to merge 1 commit into
bav/instructor-authz-shared-infrafrom
ks/issue-492-cohorts-authz

Conversation

@efortish

Copy link
Copy Markdown

Description

Stacked on top of openedx#39255 / this branch (not mergeable until that one lands, since it carries that PR's full diff). Migrates every endpoint behind the Cohorts tab to the shared Instructor Dashboard AuthZ infrastructure from openedx-authz#486, per the README's migration guide (lms/djangoapps/instructor/authz/README.rst), whose "legacy rule that is not a bridgekeeper permission" example is literally written against this tab.

Endpoints migrated

View Methods authz_permission Legacy fallback
CohortSettings (openedx.core.djangoapps.course_groups.views) GET / PUT view_cohorts / manage_cohorts default (see below)
CohortHandler GET / POST / PATCH view_cohorts / manage_cohorts / manage_cohorts default
CohortUsers GET / POST / DELETE view_cohorts / manage_cohorts / manage_cohorts default
course_cohort_settings_handler (legacy function view, still used by Studio) GET / PATCH view_cohorts / manage_cohorts custom, see below
CohortCSV (lms.djangoapps.instructor.views.api) POST manage_cohorts custom, see below

The three DRF views (CohortSettings/CohortHandler/CohortUsers) don't set legacy_permission_check or permission_name at all. That's deliberate: with both left unset, user_has_instructor_permission's fallback resolves each method's own default from LEGACY_PERMISSION_CHECKS (keyed by authz_permission, not by view), and has_cohort_view_access/has_cohort_manage_access (already built in openedx#39255's legacy.py) already reproduce IsStaffOrAdmin's exact logic, GET included (staff/instructor plus forum admin/moderator/community TA), writes staff/instructor only. Setting a single legacy_permission_check here would have pinned one check across every method, undoing the per-method distinction the mapping is supposed to give. IsStaffOrAdmin itself is now dead code (this migration was its only remaining caller) and I deleted it along with course_groups/permissions.py.

course_cohort_settings_handler keeps its own, narrower legacy behavior: it only ever required get_course_with_access(user, 'staff', course_key) for both GET and PATCH (no forum-role carve-out, unlike the DRF views). I didn't want to silently broaden it, so it gets an explicit legacy check (_legacy_cohort_staff_access) that reproduces that exact call instead of falling back to the broader default.

CohortCSV previously used IsAdminUser (Django's global is_staff), not a course-level check at all, unlike every other cohorts endpoint. I preserved that exact behavior via a custom legacy_permission_check rather than widening it to has_cohort_manage_access, since that would grant CSV upload to course staff/instructors who couldn't do it before. Worth a product/security conversation if that's actually supposed to be manage_cohorts long-term, flagging it here rather than deciding unilaterally.

Shared infra change

InstructorAuthzPermission.has_permission hardcoded the course-key URL kwarg as course_id, true for every other Instructor Dashboard v2 view, but the cohorts API uses course_key_string. Added an optional course_id_url_kwarg view attribute (defaults to "course_id", so every already-migrated view is unaffected) and documented it in the README, since any future subtask with a non-standard kwarg name will hit the same thing.

Fixes openedx-authz#492.

Testing

Added:

  • CohortsAuthzTest (course_groups/tests/test_views_authz.py): the three DRF views, across every GET/write endpoint, covering Course Admin, Course Staff, no permission, AuthZ denying a legacy instructor, the legacy path with the flag off, and that AuthZ isn't queried when the flag is off.
  • CohortSettingsHandlerAuthzTest: the legacy function view, same coverage, plus a dedicated check that a course instructor keeps exactly the access it had before (not more, not less).
  • CohortCSVAuthzTest: Course Admin and no-permission cases, plus a dedicated test that Django-staff keeps access while a course instructor (who gets manage_cohorts through AuthZ) does not get it through the legacy path, since that path was never broadened.
  • test_custom_course_id_url_kwarg in the shared test_permissions.py, for the new course_id_url_kwarg attribute.

Also ran, unmodified:

  • The existing openedx/core/djangoapps/course_groups/ suite (102 tests, including the OAuth-authenticated staff/non-staff cases and the CSV upload test), to confirm the legacy path is byte-for-byte unchanged.
  • Full course_groups/ + instructor/authz/: 191 passed.
  • Full lms/djangoapps/instructor/: 1027 passed.
  • ruff, pylint, lint-imports clean.

Deadline

None

@BryanttV
BryanttV force-pushed the bav/instructor-authz-shared-infra branch 2 times, most recently from c7012a1 to 7d48414 Compare October 22, 2026 15:56
@BryanttV BryanttV linked an issue Oct 22, 2026 that may be closed by this pull request
2 tasks
@efortish
efortish force-pushed the ks/issue-492-cohorts-authz branch 2 times, most recently from cdf3ea4 to 3d61f4f Compare October 24, 2026 20:41
@BryanttV
BryanttV force-pushed the bav/instructor-authz-shared-infra branch 2 times, most recently from e521b17 to df2efbe Compare October 26, 2026 21:46
Migrate the cohorts API views to InstructorAuthzPermission, following
openedx-authz#486's shared infrastructure:

- CohortSettings, CohortHandler and CohortUsers (openedx.core.djangoapps.
  course_groups.views) now require courses.view_cohorts for GET and
  courses.manage_cohorts for every write method, through a per-method
  authz_permissions mapping. No legacy_permission_check is set for these,
  so each method falls back to the matching default registered in
  LEGACY_PERMISSION_CHECKS (has_cohort_view_access/has_cohort_manage_access,
  already built in openedx#486 to reproduce IsStaffOrAdmin), which is what varies
  the legacy fallback by method without any new code.

- CohortCSV (lms.djangoapps.instructor.views.api) now requires
  courses.manage_cohorts, with a custom legacy_permission_check that
  keeps its pre-existing is_staff-only fallback: unlike the rest of the
  cohorts endpoints, this one was never gated on course staff access.

- course_cohort_settings_handler, the legacy function-based view still
  used by Studio, gets the same permission check via
  user_has_instructor_permission directly, with an explicit legacy check
  reproducing its existing get_course_with_access(user, 'staff', ...)
  call so GET and PATCH keep requiring the same access level they always
  did (the DRF views' IsStaffOrAdmin-derived checks allow broader GET
  access, which this endpoint never had).

Along the way:

- InstructorAuthzPermission assumed the URL kwarg was always named
  course_id. The cohorts API uses course_key_string, so it gained an
  optional course_id_url_kwarg attribute (defaults to "course_id"),
  documented in the README for the other per-tab subtasks too.

- Deleted course_groups.permissions.IsStaffOrAdmin: it was this
  migration's only remaining caller, and legacy.has_cohort_view_access/
  has_cohort_manage_access already reproduce its exact logic.

Add CohortsAuthzTest, CohortSettingsHandlerAuthzTest and CohortCSVAuthzTest
covering Course Admin, Course Staff, a user with no permission, AuthZ
denying a legacy instructor, and the legacy path with the flag off
(including CohortCSV's narrower is_staff-only fallback).

Rebased onto the latest instructor-authz-shared-infra (authz_permission
renamed to authz_permissions, always a collection) and updated the
cohorts views and tests to match.

Closes openedx-authz#492.
@BryanttV
BryanttV force-pushed the ks/issue-492-cohorts-authz branch from 3d61f4f to 9f73824 Compare October 29, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforcement: Cohorts tab (course_groups API)

1 participant