Release ulmo 3 - #38969
Open
thaipro23 wants to merge 45 commits into
Open
Conversation
The Course Info Blocks API endpoint has been known to be rather slow to return the response. Previous investigation showed that the major time sink was the get_course_blocks function, which is called three times in a single request. This commit aims to improve the response times by reducing the number of times that this function is called. Solution Summary The first time the function get_course_blocks is called, the result (transformed course blocks) is stored in the current WSGI request object. Later in the same request, before the second get_course_blocks call is triggered, the already transformed course blocks are taken from the request object, and if they are available, get_course_blocks is not called (if not, it is called as a fallback). Later in the request, the function is called again as before (see Optimization Strategy and Difficulties). Optimization Strategy and Difficulties The original idea was to fetch and transform the course blocks once and reuse them in all three cases, which would reduce get_course_blocks call count to 1. However, this did not turn out to be a viable solution because of the arguments passed to get_course_blocks. Notably, the allow_start_dates_in_future boolean flag affects the behavior of StartDateTransformer, which is a filtering transformer modifying the block structure returned. The first two times allow_start_dates_in_future is False, the third time it is True. Setting it to True in all three cases would mean that some blocks would be incorrectly included in the response. This left us with one option - optimize the first two calls. The difference between the first two calls is the non-filtering transformers, however the second call applies a subset of transformers from the first call, so it was safe to apply the superset of transformers in both cases. This allowed to reduce the number of function calls to 2. However, the cached structure may be further mutated by filters downstream, which means we need to cache a copy of the course structure (not the structure itself). The copy method itself is quite heavy (it calls deepcopy three times), making the benefits of this solution much less tangible. In fact, another potential optimization that was considered was to reuse the collected block structure (pre-transformation), but since calling copy on a collected structure proved to be more time-consuming than calling get_collected, this change was discarded, considering that the goal is to improve performance. Revised Solution To achieve a more tangible performance improvement, it was decided to modify the previous strategy as follows: * Pass a for_blocks_view parameter to the get_blocks function to make sure the new caching logic only affects the blocks view. * Collect and cache course blocks with future dates included. * Include start key in requested fields. * Reuse the cached blocks in the third call, which is in get_course_assignments * Before returning the response, filter out any blocks with a future start date, and also remove the start key if it was not in requested fields
fix: validation API for certificates
This pulls in publishing dependency changes from: openedx/openedx-core#369 This fixes a bug where publishing a Content Library v2 container would publish only its direct children instead of publishing all ancestors. Backports: 190a8b8 Co-authored-by: Kyle McCormick <kyle@axim.org>
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
…nedx#37622) (openedx#37629) Calls `LIBRARY_CONTAINER_PUBLISHED` when publishing a container that is child of another container.
fix: validation API for certificates (backport - Ulmo)
…cement points (openedx#37633) * feat: filter libraries based on user-role scopes (openedx#37564) (cherry picked from commit 6c6fc5d) * feat: add openedx-authz to user_can_create_library and require_permission_for_library_key (openedx#37501) * feat: add the authz check to the library api function feat: add the authz publish check in rest_api blocks and containers feat: add the authz checks in libraries and refactor feat: add collections checks feat: update enforcement in serializer file refactor: refactor the permission check functions fix: fix value error fix: calling the queries twice * test: add structure for test and apply feedback refactor: refactor the tests and apply feedback fix: apply feedback Revert "refactor: refactor the tests and apply feedback" This reverts commit aa0bd52. refactor: use constants and avoid mapping test: fix the test to have them in order docs: about we rely on bridgekeeper and the old check for two cases docs: update openedx/core/djangoapps/content_libraries/api/libraries.py Co-authored-by: Maria Grimaldi (Majo) <maria.grimaldi@edunext.co> refactor: use global scope wildcard instead of * refactor: allow receiving PermissionData objects refactor: do not inherit from BaseRolesTestCase to favor CL setup methods If both BaseRolesTestCase and ContentLibrariesRestApiTest define a method with the same name (e.g., setUp()), Python will use the one found first in the MRO, which is the one in BaseRolesTestCase because it is listed first in the class definition leading to unexpected behavior. refactor: remove unnecessary imports and indent * chore: bump openedx-authz version (cherry picked from commit f4f14a6) * feat: Upgrade Python dependency openedx-authz (openedx#37652) * feat: Upgrade Python dependency openedx-authz handle cache invalidation Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` * fix: update the num of queries in tests --------- Co-authored-by: MaferMazu <35668326+MaferMazu@users.noreply.github.com> Co-authored-by: Maria Fernanda Magallanes Zubillaga <maria.magallanes@edunext.co> (cherry picked from commit 122b4e0) * chore: update requirements to fix the inconsistency --------- Co-authored-by: Maria Grimaldi (Majo) <maria.grimaldi@edunext.co> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was originally permitted for forum moderators and course staff as a way to fight spam, but it was decided that this functionality was too dangerous to open up that widely. There are some tentative plans around how to make this a more fully supported feature, but until then, we're restricting this to global staff on the Ulmo release branch as an interim measure. The frontend was already disabled in the Ulmo release, meaning that this will be a backend-only API (i.e. if you really know what you're doing and absolutely need this functionality). The assumption is that this feature will continue to be developed on the master branch and will be in better shape for Verawood.
…edx#37644) (openedx#37679) - Fix the issue described in openedx/frontend-app-authoring#2626 - Publish components and containers after migrate (cherry picked from commit b9e5683)
…) (openedx#37688) Prevents notification failures with MySQL backend by ensuring signals are only sent after database transactions commit. This fixes race conditions where Celery workers couldn't see newly created threads. - Added send_signal_after_commit() helper function - Updated both thread creation paths to use the helper Co-authored-by: Taimoor Ahmed <taimoor.ahmed@A006-01711.local>
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
chore(logging): add additional logs for LTI launch flow
* fix: Course search pill not cleared when text deleted * chore: fix spacing Co-authored-by: Feanil Patel <feanil@axim.org> --------- Co-authored-by: Feanil Patel <feanil@axim.org>
… (openedx#37661) This reverts commit 7cd4170.
There is no way to resume either the backup or restore library actions, i.e. if you navigate away from it, you have to do it again. This is a limitation of the current UI because we wanted to get something quick and simple in for Ulmo, but it also reflects the fact that library backup/restore should be much faster than course import/export has historically been. In any case, sending an email for a 5-10 second task is unnecessary and distracting, so this commit suppresses the email. Note: I'm using local imports to get around the fact that the content_libraries public API is used by content_libraries/tasks.py which defines the tasks. I can't import from content_libraries/tasks.py directly, because that would violate import linter rules forbidding other apps from importing things outside of api.py. This isn't ideal, but it keeps the fix small and it keeps the logic in the content_libraries app.
We previously fixed this when the CourseLimitedStaffRole was applied to a course but did not handle the case where the role is applied to a user for a whole org. The underlying issue is that the CourseLimitedStaffRole is a subclass of the CourseStaffRole and much of the system assumes that subclesses are for giving more access not less access. To prevent that from happening for the case of the CourseLimitedStaffRole, when we do CourseStaffRole access checks, we use the strict_role_checking context manager to ensure that we're not accidentally granting the limited_staff role too much access.
The "overview" and "about_sidebar_html" fields in the
CoursewareInformation view (/api/courseware/course/{courseId}) were
returning unsanitized HTML and relying on the client to sanitize it.
This commit shifts that work to the server side (clean_dangerous_html)
to remove potentially dangerous tags when generating the response. The
source of this data is modified in the "Settings and Details" section
of a course in Studio.
…ff_fix fix: CourseLimitedStaffRole should not be able to access studio.
Prior to this, if ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES was enabled, we would not return the orgs that someone had course creator rights on, even if ENABLE_CREATOR_GROUP was enabled. (For the moment, we are conflating "can create courses" with "can create libraries" for a given org, even though we should probably eventually split those apart.)
Re-compilation and upgrade-package should be able to run without updating the common_constraints.txt file. We do this all the time when backporting fixes to older releases. We shouldn't pull in the latest common_constraints.txt in those cases as they may not be compatible with older releases.
…t_backport build: Don't update common_constraints.txt on re-compilation.
…7796) For legacy library_content references in courses, this PR: - **Removes the spurious sync after updating a reference to a migrated library**, so that users don't need to "update" their content _after_ updating their reference, _unless_ there were real content edits that happened since they last synced. We do this by correctly associating a DraftChangeLogRecord with the ModulestoreBlockSource migration artifact, and then comparing that version information before offering a sync. (related issue: openedx/frontend-app-authoring#2626). - **Prompts users to update a reference to a migrated library with higher priority than prompting them to sync legacy content updates for that reference**, so that users don't end up needing to accept legacy content updates in order to get a to a point where they can update to V2 content. - **Ensures the library references in courses always follow the correct migration,** as defined by the data `forwarded` fields in the data model, which are populated based on the REST API spec and the stated product UI requirements. For the migration itself, this PR: - **Allows non-admins to migrate libraries**, fixing: openedx#37774 - **When triggered via the UI, ensures the migration uses nice title-based target slugs instead of ugly source-hash-based slugs.** We've had this as an option for a long time, but preserve_url_slugs defaulted to True instead of False in the REST API serializer, so we weren't taking advantage of it. - **Unifies logic between single-source and bulk migration**. These were implement as two separate code paths, with drift in their implementations. In particular, the collection update-vs-create-new logic was completely different for single-souce vs. bulk. - **When using the Skip or Update strategies for repeats, it consistently follows mappings established by the latest successful migration** rather than following mappings across arbitrary previous migrations. - **We log unexpected exceptions more often**, although there is so much more room for improvement here. - **Adds more validation to the REST API** so that client mistakes more often become 400s with validation messages rather than 500s. For developers, this PR: - Adds unit tests to the REST API - Ensures that all migration business logic now goes through a general-purpose Python API. - Ensures that the data model (specifically `forwarded`, and `change_log_record`) is now populated and respected. - Adds more type annotations. Backports: 91e521e Backport note: Compared to the original commit, this backport commit excludes the REST APIs which were not defined at the time of the Ulmo cutoff: * /api/v1/modulestore_migrator/libraries * /api/v1/modulestore_migrator/migration_info * /api/v1/modulestore_migrator/migration_blocks
This is to fix an issue in the following common migration situation: 1. An existing course references content in a legacy content library. 2. The legacy content library is migrated to the new library system. 3. The user clicks on "Update reference" from the Randomized Content Block in the course. This action is supposed to update the children of the LibraryContentBlock (usually ProblemBlocks) so that the "upstream" attribute is set to point at the UsageKeys of the content in the new libraries they were migrated to. What was happening instead was that the upstream entries for these child blocks were left blank, breaking the upstream/sync connection and making it so that the courses did not receive any updates from the migrated libraries. There were two issues: 1. get_forwarding_for_blocks() was being called with the child UsageKeys in the course, when it should have been called with the v1 library usage keys instead (since those are the things being forwarded). 2. We were checking that the target_key was a v2 Library key, but really the upstream target_key is supposed to be a LibraryUsageLocatorV2, i.e. the key of the specific piece of content, not the library it ended up in. Note on testing: Although there were unit tests for the migration of legacy content libraries, there were not any unit tests for the migration of legacy library *blocks*. This commit adds a minimal test, which would have caught the bug we're fixing. It would be good to add more comprehensive testing unit testing for this part of the migration flow. --------- Co-authored-by: Kyle McCormick <kyle@axim.org>
There is a singleton SplitMongoModuleStore instance that is returned whenever we call the ubiquitous modulestore() function (wrapped in a MixedModuleStore). During initialization, SplitMongoModuleStore sets up a small handful of XBlock runtime services that are intended to be shared globally: i18n, fs, cache. When we get an individual block back from the store using get_item(), SplitMongoModuleStore creates a SplitModuleStoreRuntime using SplitMongoModuleStore.create_runtime(). These runtimes are intended to be modified on a per-item, and later per-user basis (using prepare_runtime_for_user()). Prior to this commit, the create_runtime() method was assigning the globally shared SplitMongoModuleStore.services dict directly to the newly instantiated SplitModuleStoreRuntime. This meant that even though each block had its own _services dict, they were all in fact pointing to the same underlying object. This exposed us to a risk of multiple threads contaminating each other's SplitModuleStoreRuntime services when deployed under load in multithreaded mode. We believe this led to a race condition that caused student submissions to be mis-scored in some cases. This commit makes a copy of the SplitMongoModuleStore.services dict for each SplitModuleStoreRuntime. The baseline global services are still shared, but other per-item and per-user services are now better isolated from each other. This commit also includes a small modification to the PartitionService, which up until this point had relied on the (incorrect) shared instance behavior. The details are provided in the comments in the PartitionService __init__(). It's worth noting that the historical rationale for having a singleton ModuleStore instance is that the ModuleStore used to be extremely expensive to initialize. This was because at one point, the init process required reading entire XML-based courses into memory, or pre-computing complex field inheritance caches. This is no longer the case, and SplitMongoModuleStore initialization is in the 1-2 ms range, with most of that being for PyMongo's connection setup. We should try to fully remove the global singleton in the Verawood release cycle in order to make this kind of bug less likely.
This commit updates the logic in the build_block_structure function to ensure that block locations are consistently normalized by removing branch and version information. This change addresses issues when creating a BlockStructure from modulestore using the published_only branch. Without this change, we end up comparing versioned keys to unversioned ones later on, which always yields False.
This update is required for the Catalog MFE to function, as the new `unstable/v0/course_list_search/` endpoint was released in `4.4.0`.
django security fixes Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
…penedx#37991) (openedx#37992) We hope this will fix an error where loremipsum is using pkg_resources, which setuptools dropped support for as of v82 (released yesterday).
…i-logging chore(logging): add additional logs for LTI launch flow (backport)
The activation_key field was exposed in /api/user/v1/accounts/{username},
allowing an attacker to bypass email verification by combining two behaviors:
1. OAuth2 password grant issues tokens to inactive users (intentional)
2. activation_key returned in API response (the vulnerability)
An attacker could register, get an OAuth2 token, read the activation_key
from the API, then GET /activate/{key} to activate without email access.
Fix: remove activation_key from UserReadOnlySerializer.to_representation()
and from ACCOUNT_VISIBILITY_CONFIGURATION["admin_fields"] (which controls
the field whitelist in _filter_fields — listed fields default to None even
if absent from the serializer data dict).
Reported by Daniel Baillo via the Open edX security working group.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* build: Fix the docs build. Our use of `fs` means we have a runtime dependency on setuptools. The newest version is not compatible with the current version of `fs` so we've added a constraint. But the docs build didn't previously install our specified version of setuptools. This is to fix that. Co-authored-by: Sarina Canelake <sarina@axim.org>
The view_survey endpoint accepted a redirect_url GET parameter and passed
it directly to HttpResponseRedirect() with no validation. If a non-existent
survey name was requested, this produced an immediate 302 to an
attacker-controlled URL. If a valid survey was requested, the same URL was
embedded in a hidden _redirect_url form field; after submission, submit_answers
echoed it back in JSON and client-side JS used it as location.href — a second
unvalidated redirect path.
Fix both by ignoring user-supplied redirect URLs entirely:
- view_survey no longer reads redirect_url from GET params
- submit_answers always redirects to reverse('dashboard') rather than
reading _redirect_url from the POST body
Note: view_student_survey retains its redirect_url parameter because it is
also called from the courseware view (courseware/views/views.py), which passes
a server-controlled course_home_url. That call path is unaffected.
Fixes: GHSA-2843-x998-f8r2
BREAKING CHANGE: The redirect_url GET parameter on /survey/<name>/ is no longer
honored. Requests that previously redirected to a caller-specified URL after
survey completion will now always redirect to the dashboard.
Backports: 42e4e0d Likely fixes: openedx#38027 Co-authored-by: María Fernanda Magallanes <35668326+MaferMazu@users.noreply.github.com>
`clean_thread_html_body()` was missing `<style>` from its tag denylist, allowing arbitrary CSS to survive sanitization and be rendered via the `|safe` filter in email templates. This enabled CSS-based email tracking (IP disclosure via background-image/import), content spoofing, and phishing via pseudo-elements. Uses `decompose()` rather than `unwrap()` so the CSS text content is also removed, not just the tag wrapper. Ref: GHSA-4xv3-5j4x-q8g4 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds `validate_saml_metadata_url()` to `third_party_auth/utils.py` which enforces HTTPS and blocks requests to loopback, link-local, and reserved addresses unconditionally, and RFC 1918 private ranges by default (configurable via `SAML_METADATA_URL_ALLOW_PRIVATE_IPS`). The validator is called in three places where user-supplied metadata URLs are fetched: `fetch_metadata_xml()` in utils.py, the inline fetch in `tasks.py::fetch_saml_metadata`, and `sync_provider_data` in the SAMLProviderDataViewSet. A 30-second timeout is also added to all `requests.get()` calls that were previously unbounded. Fixes GHSA-328g-7h4g-r2m9. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
thaipro23
requested review from
a team,
farhan,
irtazaakram and
salman2013
as code owners
August 11, 2026 03:26
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @thaipro23! 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:
🔘 Submit a signed contributor agreement (CLA)
If you've signed an agreement in the past, you may need to re-sign. Once you've signed the CLA, please allow 1 business day for it to be processed. 🔘 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. |
|
Hi @thaipro23! Welcome, and thank you for this contribution! In order for your CLA check to turn green, you'll need to submit a CLA form. If you are contributing as an individual, please fill out the individual CLA form here. If you are contributing on behalf of an organization, please have your manager reach out to oscm@axim.org so you may be added to your org's existing entity agreement. When you have a moment, please also add a description of your changes to the top of the pull request. Please let me know if you have any questions. Thanks! |
mphilbrick211
moved this from Needs Triage
to Needs Tests Run or CLA Signed
in Contributions
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
20 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
Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.