grant xblock edit access via authz edit_course_content permission - #39050
Draft
jacobo-dominguez-wgu wants to merge 16 commits into
Draft
grant xblock edit access via authz edit_course_content permission#39050jacobo-dominguez-wgu wants to merge 16 commits into
jacobo-dominguez-wgu wants to merge 16 commits into
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @jacobo-dominguez-wgu! 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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where 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. |
…all courses or specific courses (openedx#38676) Add a management command for generating a csv report of xblocks used in courses --------- Co-authored-by: Samuel Allan <samuel@opencraft.com>
Add optional start_date_on_or_after/start_date_on_or_before query params to CourseOverview.get_all_courses(), threaded through get_courses_accessible_to_user() and documented on HomePageCoursesViewV2. Adds get_date_param() (validates the date params, 400 on bad input) and a db_index on CourseOverview.start. See openedx/openedx-core#669 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add openedx_learning to INSTALLED_APPS in both lms/envs/common.py and cms/envs/common.py, so the CompetencyTaxonomy model and its migrations are active. Add openedx_learning to the isolated_apps import-linter contract, restricting other code to importing only its api/models_api/data modules, and to root_packages, which import-linter needs to build a graph node for the app at all. CompetencyTaxonomy already carries a `.. no_pii:` annotation in its own docstring, so no PII safe-list entry is needed here. The openedx-core requirements pin (1.3.0) already contains the app; no requirements change needed. See openedx#38958 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* docs: add ADR for standardizing REST API URL structure Open edX REST URLs follow no consistent pattern. The /api/ prefix, the position of the version, pluralisation, word separators, and trailing slashes all vary, in places between adjacent lines of a single URLconf. The FC-0118 ADRs (0025-0037) standardize what happens inside an endpoint but never its address. Add docs/decisions/0038-standardize-rest-api-url-structure.rst as an accepted ADR defining twelve rules: a leading /api/ prefix, singular API names with plural collections, domain-based rather than app-based naming, lowercase snake_case segments, mounts that declare their own prefix, exact-match routes with a required trailing slash, version position and form, hierarchy capped at one level of nesting, opaque-key identifiers resolved by shared path converters, verb-free resource paths, snake_case Django URL names, and a single URL namespace shared by the LMS and Studio. Existing endpoints migrate under OEP-21 with the conforming path mounted alongside the legacy one. This restates the still-applicable rules from the Open edX REST API Conventions wiki that OEP-49 defers to, settles what that page left as TBD, and adds a CI conformance check so the convention is enforced rather than remembered. Findings recorded in the ADR: - /api/courses/ is mounted in both services on unrelated implementations, both at v1, which blocks the endpoint-by-endpoint combined headless LMS+CMS migration. - /api/enrollment/v1/enrollment and /api/enrollment/v1/enrollments/ are different views, so pluralisation is load-bearing today. - Deprecated Org/Course/Run course keys contain slashes, so nesting one mid-path requires a shared path converter; the platform has three, in two apps, none reusable. - Django resolves re_path with re.search, so unanchored patterns in course_experience and learner_home match under arbitrary prefixes. * docs: address review feedback on the URL structure ADR Four review comments on PR openedx#39003: - Fix the RST formatting in rule 11. RST does not nest inline markup, so **``snake_case``**, ... rendered with visible asterisks and backticks. Rule 11 now keeps the literal outside the bold lead-in. The same construct in the kebab-case rejected-alternative bullet is fixed the same way. - Stop requiring slash-tolerant course keys. openedx-platform#31134 removed Old Mongo create and update operations, leaving only read-only access to static assets and the root CourseBlock, so no new deprecated-key course can be authored. New and migrated APIs now accept non-deprecated keys only, which covers course-v1: and ccx-v1: and refuses Org/Course/Run. The converter regex becomes a plain "no slash" match with the check made on course_key.deprecated, and mid-path nesting is unambiguous as a result. The now-invalid "breaks on keys containing /" clause is dropped from the deep-nesting rejected alternative. Only endpoints still serving pre-existing Old Mongo courses need the slash-tolerant pattern. - Record that content_staging, olx_rest_api, content_libraries, and lms.djangoapps.instructor are core apps rather than optional extensions. All four are absent from the static INSTALLED_APPS lists and arrive through get_plugin_apps(), so the ADR recommends moving them into INSTALLED_APPS and mounting them explicitly under their own prefix, which is what rule 5 asks of any API. The conformance check still walks the composed resolver, because third-party plugins will always contribute routes the project URLconfs cannot show. - Answer where the path converters belong: they are generic and need only edx-opaque-keys, so they go in edx-drf-extensions alongside the pagination and JWT classes, with openedx/core/lib shown as the interim home. * docs: point ADR 0038 at the shipped edx-drf-extensions converters The shared opaque-key path converters now live in edx-drf-extensions (edx_rest_framework_extensions/url_converters.py, registered via register_url_converters()), so drop the paragraph deferring that extraction and update the code example and Implementation Notes to reference the library instead of the interim openedx/core/lib/ path.
…nedx#39024) * fix: recover from orphaned Mongo course index on course creation The split modulestore course index is read from MySQL but written to both MySQL and Mongo's active_versions. Mongo writes are not covered by ATOMIC_REQUESTS, so if a request creates a course and then fails, the MySQL row is rolled back while the Mongo doc survives. That course key is then permanently unusable: has_course() reads MySQL and reports the course as absent, so creation is retried, and the follower write to Mongo raises DuplicateKeyError against UNIQUE(org, course, run). Every retry fails identically, and the rollback leaves no MySQL trace of why. Clear any stale Mongo doc before the follower write. This is safe because it runs after new_index.save() has succeeded, which means no MySQL row existed for the key, which means any Mongo doc for it is stale. * fix: only clean up the stale Mongo doc when the insert conflicts Deleting unconditionally added a Mongo round-trip to every course and library creation, which broke check_mongo_calls in TestLibraries::test_create_library (expected 3 calls, 4 were made). Insert first and clean up only on DuplicateKeyError, so the happy path keeps its original call count and the extra work happens only in the rare case where a stale doc is actually present. --------- Co-authored-by: Peter Pinch <pdpinch@mit.edu>
…on-requirements-394de1f chore: Upgrade Python requirements
…e-country-database-33466049745 Update GeoLite Database
* feat: update tpa config * fix: silent TPA error messages in MFE account * feat: pass provider name as dynamic error_code param in account redirect * feat: surface TPA errors to Account MFE via endpoint instead of query param * fix: preserve unrelated Django messages in TPA error endpoint Reading django.contrib.messages marks the whole session storage as consumed, not just the message we want. Without re-queuing the rest, any unrelated message queued in the same session (from some other, unrelated flow) was being silently dropped instead of shown wherever it was actually meant to appear. Now only the first social-auth message is consumed; everything else is re-queued. Also add a drf_yasg schema to the endpoint, matching the convention already used elsewhere in this app, and expand test coverage: message preservation, multiple queued social-auth messages, unrecognized exceptions, and the full set of TPA exception types for both the redirect-dispatch and message-queuing paths. * fix: address pylint failures in new tests - use-implicit-booleaness-not-comparison: "list(...) == []" -> "not list(...)" - comparison-with-callable: match.func == view_function needs an explicit disable, same pattern already used elsewhere in this codebase (e.g. common/djangoapps/util/date_utils.py). * fix: limit test account path only to lms
mphilbrick211
added
the
mao-onboarding
…signments (openedx#38984) * fix: RoleCache legacy compat layer ignores platform-wide glob role assignments authz_get_all_course_assignments_for_user() only fetched CourseOverviewData and OrgCourseOverviewGlobData scopes, never PlatformCourseOverviewGlobData (course-v1:*). Even if it had, _get_org_and_course_id_from_authz_scope() had no branch for it, since a platform-wide scope doesn't map to a single org the way course/org-wide scopes do. These assignments feed RoleCache/BulkRoleCache, which back has_access()/ CourseRole.has_user() legacy checks. A user whose only role assignment was a platform-wide glob got an empty RoleCache for every course, silently denying access despite the AuthZ assignment existing. Fix: a platform-wide grant applies to every org, so represent it as an org-wide grant (org, course_id=None) repeated for every registered org (reusing organizations.api.get_organizations(), the same helper used for the analogous fix in openedx-authz#380). This is exactly the shape an org-wide grant already produces, so it's picked up by the existing OrgRole-based legacy checks (has_staff_roles, get_user_permissions, which already check org-level and course-level access separately) with no changes needed to RoleCache/OrgRole/CourseRole. Fixes openedx/openedx-authz#379 * refactor: use plain assert instead of self.assertTrue in new tests Per review feedback from @BryanttV on openedx#38984. * refactor: use build_external_key() instead of hardcoded 'course-v1:*' Per review feedback from @BryanttV on openedx#38984. * refactor: use real role assignment in platform-glob tests, drop dead org check Per mariajgrimaldi's review: the two platform-glob tests built a RoleAssignmentData by hand and mocked get_user_role_assignments instead of exercising the real assignment and policy load path, so switched both to assign_role_to_user_in_scope + AuthzEnforcer.get_enforcer().load_policy(), same pattern already used elsewhere in this file. Also dropped the `scope.org is not None` check in _get_orgs_and_course_ids_from_authz_scope. An OrgCourseOverviewGlobData instance can only come from the ScopeData factory, which validates the org name before selecting that class, so org can't actually be None there.
openedx#39013) * feat: adding authz edit permission check on xblock wrapper * feat: adding manage tags check
…dX/openedx-platform into course-editor-content
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
11 participants
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
In Studio, the per-component "Edit" action on an XBlock card is controlled by the
can_editflag inxblock_view_handler. Previouslycan_editwas derived solely from the legacyhas_studio_write_accesscheck.Under the authz course-authoring rollout, a user can be granted
courses.edit_course_contentwithout holding legacy studio write access. Those users were incorrectly denied the "Edit" action on component cards, even though the authzpermission is meant to grant exactly that capability.
This change makes
can_editthe union of the legacy check and the authz path:The
is_authz_authoring_enabled/authz_can_edit_course_contentflags are already resolved for this request via_get_authz_permissions_flags, so no additional permission lookups are introduced. When the authz flag is off, behavior is unchanged.Supporting information
Fixes openedx/openedx-authz#418
Testing instructions
Automated:
Run the regression tests added in
TestXBlockViewHandlerHeaderActionsAuthz:They cover the component edit button on a leaf
htmlcomponent preview:edit_course_contentgranted + no legacy write access → edit button shownManual:
courses.edit_course_contentwithout legacy studio write access.Demo
User with edit_course_content permission must be able to edit course content.
Screen.Recording.2026-08-31.at.3.16.16.PM.mov
AI usage notice
Used Copilot with auto model mode (Claude) to assist on the modification and creation of unit tests.
Important
Needs to be reviewed after #39013, it contains code from that pr.