Repository navigation
Conversation
Declare the permission identifiers used by the Instructor Dashboard API v2. Temporary: the permissions not defined by openedx-authz depend on a separate issue that will define them there. They are declared here only for testing. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Add the shared infrastructure to check Instructor Dashboard permissions with openedx-authz, driven by the authz.enable_course_authoring flag. * With the flag enabled for the course only AuthZ is consulted, like in Studio. With the flag disabled the legacy check is used, so the previous behavior is preserved. * Add user_has_instructor_permission, get_instructor_authz_permissions and the InstructorAuthzPermission DRF permission class, logging which path (authz or legacy) resolved each check. * Add the legacy equivalent of each permission. * CourseMetadataView resolves the Course Info, Enrollments, Course Team, Grading, Date Extensions and Cohorts tabs through AuthZ, and returns the effective permissions in the new authz_permissions field. The existing fields do not change. * CourseEnrollmentsView requires courses.view_enrollments. * Add test helpers (InstructorAuthzTestMixin) and tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
* Add examples to "Migrating a view": a single permission, a permission per HTTP method, a custom legacy check, direct use of user_has_instructor_permission and the errors returned. * State in the cohort legacy checks that they follow the same rule as IsStaffOrAdmin. * Wrap the long line in has_cohort_view_access and move the "Shared with Authoring" note of COURSES_VIEW_GRADING_SETTINGS to the same line. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
* Add AuthzTestMixin with the code common to the course authoring and Instructor Dashboard test mixins: the flag patch, the policy seeding and cleanup, and assign_authz_role. * Add set_authz_enabled and set_authz_enabled_for to turn the flag off or enable it only for some courses, instead of ad hoc patches in each test. * CourseAuthoringAuthzTestMixin and InstructorAuthzTestMixin extend it and keep only what is specific to them. * Add docstrings to the Instructor Dashboard AuthZ tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…sions Make the default legacy check of each permission the rule of the endpoint that the MFE calls, and keep the tab visibility rules explicit. * view_course_info, view_grading_configuration and view_learner_extensions default to the VIEW_DASHBOARD permission. The Course Info and Grading tabs pass has_staff_access, as the Date Extensions tab passes has_instructor_access. * view_gradebook follows the gradebook API (course author access) and view_grading_settings follows the Studio grading settings (Studio read access). * The Grading tab is resolved with view_grading_configuration. * CourseMetadataView uses the default check of view_course_info, and documents that masquerading is resolved with that permission. * Use other permissions in the README example of a permission per HTTP method. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A tab is now shown when the user holds at least one of the permissions validated in it, instead of a single specific permission. * Add TAB_PERMISSIONS with the permissions of each tab. The Enrollments tab includes manage_course_team because it manages the beta testers. * Add user_has_any_instructor_permission. With the flag enabled it consults only AuthZ; with it disabled it uses the explicit legacy check, or the default one of each permission. * CourseInformationSerializerV2.get_tabs resolves the tabs through it. The explicit legacy checks (staff, instructor) are preserved. * Cover the helper and every (tab, permission) pair in the tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The Date Extensions tab is only shown when edx-when is enabled for the course, so the tab tests did not see it now that it is part of the migrated tabs. * Patch is_enabled_for_course for the whole CourseMetadataViewAuthzTest. * Legacy staff does not see Date Extensions, which requires instructor access when the flag is disabled. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
CourseEnrollmentsView goes back to the legacy InstructorPermission check, and its AuthZ tests are removed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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_permission 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). Fixes openedx-authz#492.
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
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