fix: allow non-enterprise, saml users to complete registration in authn MFE #35916
Conversation
This file contains 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
This kicks off the Quince release! We follow the instructions from: https://openedx.atlassian.net/wiki/spaces/COMM/pages/19662426/Process+to+Create+an+Open+edX+Release
Quince django42 upgrade
With this change, the platform users who access content via LTI will be automatically linked to their platform account instead of the new (anonymous) one. The following conditions need to be met: * The `LtiConsumer` should be configured to auto-link the users via email. * The LTI Consumer should share the user's email using the `lis_person_contact_email_primary` parameter in the LTI Launch POST data. This also replaces the one-to-one relationship of the `User` and `LtiUser` with one-to-many. This way, multiple `LtiUser` objects can refer to the same `edx_user`. With the auto-linking, multiple LTI Consumers can create independent `LtiUser` objects with the same `edx_user`. Co-authored-by: Piotr Surowiec <[email protected]> (cherry picked from commit 5b2f012)
…kport-lti-provider-user-auto-linking [Quince Backport] feat: link LTI Provider launches to authenticated users
POST requests to the LMS are failing systematically when HTTPS is enabled. This issue is observed in the Quince release branch. Here is the root cause analysis: - CorsCSRFMiddleware overrides the `is_secure` attribute by setting it to "false". - CorsCSRFMiddleware calls the parent `process_view` method, from the CsrfViewMiddleware. - CsrfViewMiddleware checks the Origin header, including the scheme. It is equal to "https://LMSHOST". But because the request is not considered secure, the expected origin is "http://LMSHOST". - The check fails with "Origin checking failed" We resolve this issue by running the CsrfViewMiddleware *before* the custom CorsCSRFMiddleware. After a successful check of the CsrfViewMiddleware, the request has the "csrf_processing_done = True" attribute, and CorsCSRFMiddleware is short-circuited. This issue did not happen in the following environments: - in Palm because the CsrfViewMiddleware did not check the "Origin" header in Django 3. - in the Studio, because the Studio already runs the CsrfViewMiddleware before the CorsCSRFMiddleware. - in the master branch because the master branch does not yet run on Django 4. But the issue will happen in the master branch without this proposed change. To bypass this issue in the master branch, it was proposed that we add "https://LMSHOST" to CSRF_TRUSTED_ORIGINS. This would effectily bypass CSRF checking entirely for all requests that originate from the LMS. Such a solution would not be acceptable, as we would lose the security guarantees offered by CSRF. See discussion: openedx/wg-build-test-release#325
…penedx#33724) (requires the contentstore.enable_copy_paste_units waffle flag)
* test: test for length validation
…it-backport Backport to Quince: copy/paste unit from within a unit in Studio
chore: update Django to 4.2.8 for Quince - Security Patch
This test broke on January 1, 2024.
(cherry picked from commit 019888f)
…allback-jwt-restricted-application-check-quince fix(quince): add `JwtRestrictedApplication` check to XBlock callback
…) (openedx#34170) * fix: add missing function import in certificate template * test: add test case to check certificates generated when GA4 is enabled (cherry picked from commit d0a49d1)
The details are described in this discussion: https://discuss.openedx.org/t/ora-grading-returns-error/11482
…ce-4.2.10 chore: update Django to 4.2.10 for Quince - Security Patch
fix: removed waffle switch around oep-50 filter
Authored-by: Dima Alipov <[email protected]>
The url was renamed from session_language to update_language but it was still referred to in some html templates
…#34404) Co-authored-by: Dima Alipov <[email protected]>
…ce-4.2.11 chore: update Django to 4.2.11 for Quince - Security Patch
…tadata response Currently, openedx/frontend-app-authoring#517 faces an issue when the progress graph toggle is enabled/disabled but the settings are not respected, the disable_progress_graph attribute will allow the frontend-app-learning repo to use this attribute to respect the settings authored from frontend-app-course-authoring and ultimately fix openedx/frontend-app-authoring#517.
…s-graph feat: Adds disable_progress_graph attribute to the returned course_me…
…nedx#34485) Co-authored-by: Dima Alipov <[email protected]>
* fix: Social link parsing approach changed * fix: fix tests * fix: better approach
…enedx#34466) "Course organization display string" option in Advanced settings doesn't influence certificate. Co-authored-by: Dima Alipov <[email protected]>
… discussion is enabled (openedx#34426) Co-authored-by: Jason Wesson <[email protected]>
Open edX implements its a JwtAuthentication class in edx-drf-extensions (in edx_rest_framework_extensions.auth.jwt.authentication). This class updates the local User database entry to match certain values in the token. It's used as a way to automatically provision and update users with their LMS user information on other Open edX services like ecommerce. Since LMS and Studio keep the record of truth in its database tables, they should *not* update their database user information based on the JWT. Doing so would allow stale JWTs to incorrectly reset user values after they had been changed in the LMS. This is done by having the EDX_DRF_EXTENSIONS['JWT_PAYLOAD_USER_ATTRIBUTE_MAPPING'] setting be an empty dictionary, and was set correctly for the LMS in its common.py env settings module. Unfortunately, this was *not* being set for Studio. This commit adds the same setting to Studio's common settings module. Prior to this commit, it was possible for a stale JWT to reset user attributes if the user hit a Studio API endpoint that used JWT for auth (e.g. endpoints used by the Course Authoring MFE). This opened up a potential security issue where a global staff user (is_staff=True) that had their global staff status removed (is_staff=False) could have up to a one hour window in which they could use their stale-but-still-valid global-staff JWT token to regain global staff status by calling a Studio endpoint with their browser.
…ge-url fix: course progress url based on whether the user has learning MFE enabled (for Quince)
…) to quince (openedx#35225) * fix: disable submit button for archived courses (openedx#34920)
…edx#35138) (openedx#35378) * chore: provide logo url from backend for batch enrollment email
* feat: course about page markup and styles improvements * test: update tests according to changes * fix: relocate course organization and return removed prerequisites info * fix: display org info above the course title --------- Co-authored-by: Eugene Dyudyunov <[email protected]> Co-authored-by: ihor-romaniuk <[email protected]>
The original request was that enterprise users with tpa hint and SAML should not be redirected to MFE. The current condition also excludes regular non-enterprise users with SAML authentication from the MFE.
|
Thanks for the pull request, @angonz! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by 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. |
openedx-webhooks
added
the
open-source-contribution
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
PR #27431 included a condition that was ment to prevent enterprise users with tpa-hint enabled and coming from SAML IdP to be redirected to the authn MFE, as it "doesn't support a co-branded login/register screen with the enterprise customer (e.g., enterprise logo, custom messaging, etc.)."
However the condition also prevented non-enterprise, SAML users to be redirected to the authn MFE.
Supporting information
It was discussed in a slack conversation.
Testing instructions
To test this use case:
Tests run as part of PR #27431 should also pass:
Enterprise customer with tpa-hint and SAML IdP should be redirected to the legacy registration page.
Deadline
ASAP.
Other information
We happen to have a client that requested SAML authentication, and a number of customizations in the authn MFE including custom fields. Users are then redirected to the legacy register page, which is based on Backbone. These pages are not theme-able, so porting the customizations is extremely complex and not scalable.