Repository navigation
Conversation
The library container and xblock embed endpoints used by the "review pending changes" modal only checked direct library permissions, so a Course Auditor (or any role holding courses.view_library_updates) got a 403 even though they should be able to review changes from their course. Accept an optional course_id query param on these three endpoints and grant access via courses.view_library_updates when it points to a course the user can review from, falling back to the existing library-level check otherwise. Same pattern already used for the sync endpoint in openedx#39009 and openedx#39055. Related to openedx/openedx-authz#441
mypy needs check_permission annotated as CheckPerm | None explicitly, since the first assignment alone made it infer plain CheckPerm. Move the embed view's authz bypass test out of test_runtime.py (a plain TestCase, missing CORS_ORIGIN_WHITELIST in the CMS test environment) into test_embed_block.py, next to the other embed view tests, using the same override_settings(CORS_ORIGIN_WHITELIST=[]) workaround already in place there for the same reason.
The course_id bypass only checked that the user held courses.view_library_updates in the given course, never that the course actually used the requested library resource. That meant holding the permission in any course was enough to read any library container or block in the system. Require the query param to be a real downstream usage key, resolve its UpstreamLink, and only grant access when that link points at the resource being requested. Same approach already used by upstream_sync_block.py/upstream_sync_container.py for the sync endpoint. Also: - add regression tests for the mismatched-downstream case and for existing library-permission callers who never pass course_id - fix cms/envs/test.py to actually define CORS_ORIGIN_WHITELIST instead of overriding it per test class, embed_block_view reads it directly with no fallback Addresses review from @mariajgrimaldi and @BryanttV.
user_has_course_permission_for_upstream imported UpstreamLink from cms.lib.xblock.upstream_sync, which import-linter's layers contract places above content_libraries, content_staging and xblock. Those apps import authz.decorators, so the import violated the contract even though it was function-local, and broke CI's Lint Python Imports job for all three. Read the downstream block's upstream field directly instead of going through UpstreamLink, and parse it with LibraryUsageLocatorV2 or LibraryContainerLocator from opaque_keys, which isn't subject to the layers contract. Updated the tests to match: they now use real library keys and mock modulestore().get_item() to return an object with a plain upstream attribute, instead of mocking UpstreamLink.
Under ModuleStoreTestCase, ATOMIC_REQUESTS wraps each request in a savepoint nested inside the test's own outer transaction. When a view returns a DRF PermissionDenied, DRF marks that savepoint for rollback, and response middleware that runs a query afterward (lang_pref's process_response, checking the user's language preference) hits TransactionManagementError because the savepoint hasn't been rolled back yet. Wrap the requests that intentionally get a 403 in self.allow_transaction_exception(), the same pattern already used in test_clipboard.py for the same kind of test.
Function-level imports hid the dependency until the function ran and could produce import-order surprises later. Moving opaque_keys locators and xmodule.modulestore to the top is safe here: opaque_keys is already imported at the top of this module, xmodule is available in both LMS and CMS, other core apps already import modulestore at module level, and lint-imports reports the same dependency graph either way since grimp counts function-level imports too. Update test_decorators.py to patch the new module-level name (openedx.core.djangoapps.authz.decorators.modulestore) and move the local ItemNotFoundError import in test_downstream_not_found_denies to the top of the test module.
The param was never actually a course id, it's always been the downstream block's usage key (the decorator derives the course from it after verifying the upstream link), so the name was misleading to API consumers. Matches the frontend-app-authoring PR's own rename of the prop from courseId to downstreamBlockId for the same reason. No call site passes param_name explicitly, so this only changes the function's default and the tests that exercise it directly.
…ponent The legacy Studio unit page's preview-changes modal only sent upstreamBlockId and old to the comparison iframe, dropping the downstreamBlockId it already had on hand. Without that param, the course-permission bypass in embed_block_view never triggers for either version, so a course auditor reviewing a single library component (as opposed to a section, subsection, or unit) gets denied instead of falling back to the regular library check, which they don't hold. Also add the same user_has_course_permission_for_upstream bypass to LibraryBlockView.get that LibraryContainerView.get already has: even with downstream_block_id correctly forwarded, the block metadata endpoint had no way to honor it.
… into ks/issue-441-library-review-authz
CI flagged the new authz.decorators import as out of alphabetical order relative to content_libraries.
Static assets referenced inside reviewed library content (images, etc.) still returned 403 for a Course Auditor, even after openedx#39107, because the asset URL is generated during content rendering (OpenedXContentRuntime's _absolute_url_for_asset) and resolved directly by the browser, not through a request the frontend controls. Thread downstream_block_id from embed_block_view into the runtime via load_block/get_runtime, so _absolute_url_for_asset can bake it into every asset URL it generates. LibraryComponentAssetView (used by both the published and draft asset endpoints) now does the same user_has_course_permission_for_upstream check the other endpoints use, resolving the component version back to its usage key via library_component_usage_key to verify the downstream is actually linked to it. Fixes openedx-authz#473.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @efortish! 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. |
This branch has not been deployed
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
3 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
Builds on #39107 (this PR depends on it) to close the remaining gap in the course-review AuthZ bypass. Even after #39107 fixed the surrounding block and container endpoints, static assets (e.g., images) referenced by reviewed library content still returned
403 Forbiddenfor Course Auditors.Root Cause
Asset URLs are generated during content rendering by
OpenedXContentRuntime._absolute_url_for_asset()and resolved directly by the browser. Since these requests are not controlled by the frontend, there was no way to attachdownstream_block_idat request time.Solution
Propagate
downstream_block_idthrough the rendering pipeline so asset requests can be authorized against the downstream course context:load_blockandget_runtime(openedx/core/djangoapps/xblock/api.py) now acceptdownstream_block_idand pass it to the runtime.embed_block_viewforwards the existingdownstream_block_idquery parameter toload_block, reusing the context already used by its own authorization bypass check.OpenedXContentRuntimestores the downstream block ID and appends it as a query parameter to every asset URL it generates.get_component_version_asset, shared byLibraryComponentAssetViewandLibraryComponentDraftAssetView, resolves the component version to its usage key viaapi.library_component_usage_keyand applies the sameuser_has_course_permission_for_upstreamcheck used by the other endpoints. This ensures assets are served only when the component is actually linked to the specified downstream context.Fixes [openedx-authz#473](openedx/openedx-authz#473).
Testing
LibraryComponentAssetAuthzBypassTesttotest_embed_block.py, covering:downstream_block_idis missing.xblock,content_libraries,content_stagingclipboard, andcontent/searchtest suites locally: 416 passed, 9 skipped, 0 failed.ruff,pylint, andlint-importsall pass.Dependencies
Depends on #39107, which must be merged before this PR can be merged.
Deadline
None.