Repository navigation
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @BryanttV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
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>
f76840a to
d6c794e
Compare
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>
d6c794e to
5469bc0
Compare
* 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>
9f9304f to
84ab8fb
Compare
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
84ab8fb to
21acf19
Compare
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>
With authz.enable_course_authoring enabled, the Instructor course tab of the LMS and CourseMetadataView grant access when the user holds at least one Instructor Dashboard permission, instead of only view_course_info. With the flag disabled they keep requiring the legacy VIEW_DASHBOARD permission. InstructorAuthzPermission accepts a collection of permissions in authz_permission, and bridgekeeper_check evaluates the permission against the course key without loading the course. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
efortish
left a comment
efortish
left a comment
There was a problem hiding this comment.
Went through this end to end (helpers.py, permissions.py, legacy.py, the CourseMetadataView/serializer changes, and the tests) and it looks solid. The design answers exactly the open question #486 left: InstructorAuthzPermission plus user_has_instructor_permission/user_has_any_instructor_permission give every per-tab subtask (#487-492) a drop-in combined check, and the README's migration examples are clear enough to follow without reading the implementation first.
For #492 specifically: has_cohort_view_access/has_cohort_manage_access in legacy.py already reproduce IsStaffOrAdmin's current logic (forum admin/moderator/community TA on read, staff/instructor on write), which is exactly the legacy equivalent that issue needed. The README's own "legacy rule that is not a bridgekeeper permission" example is even written against cohorts. That part of #492 is unblocked now.
One thing worth flagging before the per-tab subtasks start wiring this in: courses.view_cohorts/courses.manage_cohorts (and, it looks like, every permission except view_course_team/manage_course_team/view_grading_settings) aren't defined in the openedx-authz schema yet, this PR's own docstring says so (constants.py: "TEMPORARY... declared here only for testing"). So whoever picks up #487-492 will need those added to course_permissions.yaml (and granted to the right roles) before the AuthZ path actually works outside a test, not just this infra PR. Not a blocker on this PR itself, just flagging it so it doesn't surprise the next person.
Ran lms/djangoapps/instructor/authz/tests and test_api_v2_authz.py locally: the authz/tests ones pass, but test_api_v2_authz.py fails in my devstack with KeyError: 'instructor_api_v2' on reverse(). That's my CMS container missing the LMS urlconf (same gap I hit with an unrelated gradebook test earlier), not something in this PR. Your lms-1..5 CI shards are all green, so I'm trusting those over my local run.
Nice work, this is exactly the shape #486 needed.
The permission check no longer loads the course, so a user without access gets 403 for a non-existent course and only global staff reaches the view and its 404. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ea156bb to
14004a7
Compare
This branch has not been deployed
3 participants
Description
The Instructor Dashboard API v2 (
/api/instructor/v2) only checks legacy course roles, so users granted granularopenedx-authzroles cannot be authorized in the new dashboard. This PR adds the shared infrastructure to check Instructor Dashboard permissions withopenedx-authz, and wires it into the first endpoints.The behavior is driven by the
authz.enable_course_authoringwaffle flag, evaluated per course, with the same semantics as Studio:openedx-authzalready grants the global staff and superuser bypass).Every decision is logged with the path that resolved it (
authzorlegacy) inaccess_path.Main changes, all under
lms/djangoapps/instructor/authz/:constants.py: the permission identifiers used by the dashboard.helpers.py:user_has_instructor_permissionandget_instructor_authz_permissions.permissions.py:InstructorAuthzPermission, a DRF permission class. Views declareauthz_permission(a permission or a per-method mapping) next to the legacypermission_name.legacy.py: the pre-RBAC equivalent of each permission.tests/mixins.py:InstructorAuthzTestMixinto assign AuthZ roles in tests. It shares the flag and policy setup with the course authoring tests through the newAuthzTestMixin(openedx/core/djangoapps/authz/tests/mixins.py).README.rst: how to migrate a view.Endpoints migrated in
views/api_v2.py:CourseMetadataViewresolves the Course Info, Enrollments, Course Team, Grading, Date Extensions, and Cohorts tabs through AuthZ (requirescourses.view_course_info) and returns the effective permissions in the newauthz_permissionsfield. Existing response fields do not change.Affected roles: Course Instructor, Course Staff, Operator, and any user assigned an AuthZ role on a course.
Supporting Information
Testing Instructions
Add the new permissions to the Course Staff and Course Admin roles. They are not defined in
openedx-authzyet, so download the following script to thescripts/folder of youropenedx-platformcheckout and run it inside the LMS (use--removeto revoke the permissions afterwards):seed_instructor_dashboard_authz_policies.sh
chmod +x scripts/seed_instructor_dashboard_authz_policies.sh tutor dev exec lms ./scripts/seed_instructor_dashboard_authz_policies.shIn the LMS, enable the
authz.enable_course_authoringwaffle flag for a test course (or globally).Create a user with no legacy course role and assign them an AuthZ role that includes
courses.view_course_info(e.g., Course Staff).As that user, request
GET /api/instructor/v2/courses/<course_id>. Expect200, with anauthz_permissionsfield listing the effective value of each permission.As a user with only a legacy Course Staff role and no AuthZ role, repeat step 4. Expect
403while the flag is enabled.Disable the flag for the course and repeat step 5. Expect
200(legacy behavior preserved).Note: Users who were assigned an AuthZ role still have access when the flag is disabled, because of the compatibility layer between AuthZ roles and legacy roles. This is expected behavior.
Run the automated tests:
Deadline
Willow
Other Information
authz/constants.pyare temporary. Onlycourses.manage_course_team,courses.view_course_teamandcourses.view_grading_settingsare defined byopenedx-authz; the rest depend on a separate issue and are declared here only for testing. They should be removed once released inopenedx-authz.CourseMetadataViewis migrated here; the remaining v2 endpoints are follow-up work using the same infrastructure.