Repository navigation
Conversation
This was referenced Oct 21, 2026
BryanttV
force-pushed
the
bav/instructor-authz-shared-infra
branch
2 times, most recently
from
October 22, 2026 15:56
c7012a1 to
7d48414
Compare
2 tasks
efortish
force-pushed
the
ks/issue-492-cohorts-authz
branch
2 times, most recently
from
October 24, 2026 20:41
cdf3ea4 to
3d61f4f
Compare
BryanttV
force-pushed
the
bav/instructor-authz-shared-infra
branch
2 times, most recently
from
October 26, 2026 21:46
e521b17 to
df2efbe
Compare
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
force-pushed
the
ks/issue-492-cohorts-authz
branch
from
October 29, 2026 17:37
3d61f4f to
9f73824
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
authz_permissionCohortSettings(openedx.core.djangoapps.course_groups.views)view_cohorts/manage_cohortsCohortHandlerview_cohorts/manage_cohorts/manage_cohortsCohortUsersview_cohorts/manage_cohorts/manage_cohortscourse_cohort_settings_handler(legacy function view, still used by Studio)view_cohorts/manage_cohortsCohortCSV(lms.djangoapps.instructor.views.api)manage_cohortsThe three DRF views (
CohortSettings/CohortHandler/CohortUsers) don't setlegacy_permission_checkorpermission_nameat all. That's deliberate: with both left unset,user_has_instructor_permission's fallback resolves each method's own default fromLEGACY_PERMISSION_CHECKS(keyed byauthz_permission, not by view), andhas_cohort_view_access/has_cohort_manage_access(already built in openedx#39255'slegacy.py) already reproduceIsStaffOrAdmin's exact logic, GET included (staff/instructor plus forum admin/moderator/community TA), writes staff/instructor only. Setting a singlelegacy_permission_checkhere would have pinned one check across every method, undoing the per-method distinction the mapping is supposed to give.IsStaffOrAdminitself is now dead code (this migration was its only remaining caller) and I deleted it along withcourse_groups/permissions.py.course_cohort_settings_handlerkeeps its own, narrower legacy behavior: it only ever requiredget_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.CohortCSVpreviously usedIsAdminUser(Django's globalis_staff), not a course-level check at all, unlike every other cohorts endpoint. I preserved that exact behavior via a customlegacy_permission_checkrather than widening it tohas_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 bemanage_cohortslong-term, flagging it here rather than deciding unilaterally.Shared infra change
InstructorAuthzPermission.has_permissionhardcoded the course-key URL kwarg ascourse_id, true for every other Instructor Dashboard v2 view, but the cohorts API usescourse_key_string. Added an optionalcourse_id_url_kwargview 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 getsmanage_cohortsthrough AuthZ) does not get it through the legacy path, since that path was never broadened.test_custom_course_id_url_kwargin the sharedtest_permissions.py, for the newcourse_id_url_kwargattribute.Also ran, unmodified:
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.course_groups/+instructor/authz/: 191 passed.lms/djangoapps/instructor/: 1027 passed.ruff,pylint,lint-importsclean.Deadline
None