Saml redirect mfe #36197
Saml redirect mfe #36197
Conversation
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.
…edx#36156) * feat!: Removing the long-deprecated legacy course_modes chooser `course_modes/choose.html` (and its corresponding `_upgrade_button.html`) were specifically only used for the edge case where an enterprise user found themselves in the non-enterprise learner dashboard, and attempted to enroll in a course outside of the enterprise flow. Once upon a time, in a 2U-only workflow, the commerce system would apply specific discounts for users within the said case. That's no longer true, and it has never been true outside of this one company. Removing this template cleans up a legacy version of a legacy page that was, realistically, exclusively seen by employees of 2U, and nobody else. Removes: * The corresponding testsfor behavior only seen in the legacy page. * A waffle flag since all cases route as if the flag is set: `VALUE_PROP_TRACK_SELECTION_FLAG`: `course_modes.use_new_track_selection` * Some variables set in `CourseModeView` which were only ever rendered in the legacy template (`title_content`, `has_credit_upsell`) have been removed from the class. * There is a high likelihood that the class is still a target for re-factoring now that the legacy view is gone, but I'm hesitant to touch something which is not covered by previously existing tests, because the logic around what template gets rendered when is complex. FIXES: APER-3779 FIXES: openedx#36090
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
Adds the concept of "downstream-only" fields to the XBlock upstream sync logic. Downstream-only fields are customizable fields set only on the downstream XBlock -- we don't keep track of the upstream field value anywhere on the downstream XBlock. Changes made to these fields in the upstream block are ignored, and if the link to the upstream block is severed, the downstream changes are preserved (not reset back to defaults, like the upstream-tracked customizable fields are). The fields chosen as "downstream-only" are those related to scoring and grading. The `max_attempts` field was previously a customizable field that tracked the upstream value. However, because it is scoring-related, it has been converted to a "downstream-only" field. This change impacts course authors' use of library content in their courses.
…#36172) * fix: catch a possible exception in beta course configuration when the learner is a beta tester, and the beta test has been set up with a large duration, the courseware djangoapp can return an overflow error on a timedelta call. In those circumstances, this defaults to returning an unmodified date. FIXES: APER-3848
This command dumps the current Django settings to JSON for debugging/diagnostics. The output of this command is for *humans*... it is NOT suitable for consumption by production systems. In particular, we are introducing this command as part of a series of refactorings to the Django settings files lms/envs/* and cms/envs/*. We want to ensure that these refactorings do not introduce any unexpected breaking changes, so the dump_settings command will both help us manually verify our refactorings and help operators verify that our refactorings behave expectedly when using their custom python/yaml settings files. Related to: openedx#36131
I'm letting autoformat hit this file to make it match our current standards before I actually make any code changes.
`notify_credentials` has 2 ways of running. 1. The manual, one-off method which uses `--args_from_database` to specify what should be sent. 2. [The automated method](https://github.com/openedx/edx-platform/blob/7316111b35c8db0b93665e00aa4071685d772ab3/openedx/core/djangoapps/credentials/management/commands/notify_credentials.py#L157-L159), which runs on a regular schedule, to catch anything which fell through the cracks. The automated method does a certain amount of time/date math in order to calculate the entry point of the window based on the current runtime. This is, I assume, why it has some hardcoded logic; it's not at all simple to have a `cron`-run management command running on a regular cadence that can do the same time logic. ```py if options['auto']: options['end_date'] = datetime.now().replace(minute=0, second=0, microsecond=0) options['start_date'] = options['end_date'] - timedelta(hours=4) ``` However, it is not ideal that the actual time window of 4 hours is hardcoded directly into `edx-platform`. This fix * creates an overridable `NOTIFY_CREDENTIALS_FREQUENCY` that defaults to 14400 seconds (4 hours). * pulls that frequency into the quoted code Using seconds allows maximum flexibility. FIXES: APER-3383
The `dump_settings` command currently prints out the raw `repr(...)`s for
defined functions, e.g.:
"WIKI_CAN_ASSIGN": "<function CAN_ASSIGN at 0x74ce5e9b2020>",
In addition to being uninformative, these `at 0x74ce...` hashes change every
run, so they appear in the diff as having "changed" every time. With this
commit, here's what `dump_settings` will print out for a function instead:
"WIKI_CAN_ASSIGN": {
"module": "lms.djangoapps.course_wiki.settings",
"qualname": "CAN_ASSIGN"
},
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
* fix: enable pylint warnings
This reintroduces commit 1593923, which was reverted due to a typo. The typo is fixed in the commit immediately following this one. Co-Authored-By: Feanil Patel <[email protected]>
In the near term, we wish to precisely preserve the existing values of all Django settings exposed by lms/envs/production.py in order to avoid breaking legacy Django plugins without a proper announcement. That includes preserving the behavior of these old, redundant dicts: * ENV_TOKENS * AUTH_TOKENS * ENV_FEATURES * ENV_CELERY_QUEUES * ALTERNATE_QUEUE_ENVS Particularly, it means we need to ensure that updates to Django settings are reflected in these dicts. The most reliable way to do that is to change the yaml-loading logic so that these values are aliased to the corresponding values in the global namespace rather than deep-copied. Finally, we remove KEYS_WITH_MERGED_VALUES from the global namespace, and inline the remaining list. We have modified the list (specifically, we dropped the no-op MKTG_URL_OVERRIDES). Plugins should not be counting on the value of the list, so we remove it.
* fix: swagger docs ref_name conflicts * fix: swagger auto doc errors * chore: bumps openedx-learning==0.18.2 --------- Co-authored-by: Jillian Vogel <[email protected]>
…36146) Fixes the styles for the advanced editors (poll, survey, LTI Provider, etc). Updated the code if `xblock_v2/xblock_iframe.html` to use `course-unit-mfe-iframe-bundle.scss`
|
Thanks for the pull request, @leoaulasneo98! 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.
|
openedx-webhooks
added
the
open-source-contribution
|
Hi, @leoaulasneo98. While we do a code review of this change, could you look into getting a signed Contributor Agreement? That's necessarily a blocker before we can merge. Thank you, and welcome! |
deborahgu
added
the
waiting for eng review
This test validates the conditional redirection to the authentication microfrontend (MFE) for enterprise and SAML authentication scenarios. The test covers different combinations of: - Enterprise customer presence - Third-party authentication provider - SAML provider status - Redirection setting Ensures that enterprise customers with SAML providers are not redirected to the authentication MFE, while other scenarios follow the standard redirection rules.
leoaulasneo98
force-pushed
the
saml-redirect-mfe
branch
from
9f8c0a9 to
967a981
Compare
Hello, thank you very much, I will then follow the indicated steps |
@leoaulasneo98 you can be added to the existing entity CLA (agreement) we have with Aulasneo. Please ask your manager to send the request to [email protected]. Thanks! |
mphilbrick211
added
needs test run
|
Description
This pull request fixes a bug in the authentication microfrontend (MFE) redirection logic. It ensures that enterprise users who are authenticating via a SAML identity provider are not incorrectly redirected to the MFE.
The change involves modifying the should_redirect_to_authn_microfrontend() function to add an additional check for the presence of an enterprise customer and a SAML-based third-party authentication provider. If both conditions are met, the user will not be redirected to the MFE, and the authentication flow will be handled within the LMS.
This change impacts "Developer" and "Operator" roles, as it affects the authentication and authorization logic within the Open edX platform.
Supporting information
The issue was discovered during internal testing and is not currently reported in any public issue trackers. The change is based on the analysis and solution proposed in the following commits:
Commit 1: Redirect non-enterprise SAML to authn MFE
Commit 2: Add test for enterprise SAML authentication MFE redirection logic
Testing instructions
To test this change:
Ensure the authentication MFE is enabled in your Open edX environment.
Create a test enterprise customer in the platform.
Configure a SAML identity provider for the enterprise customer.
Authenticate as a user who belongs to the enterprise customer and is using the SAML provider.
Verify that the user is not redirected to the authentication MFE, but instead the authentication flow is handled within the LMS.
Repeat the test with a non-enterprise user who is authenticating via SAML. Ensure they are redirected to the authentication MFE.
Deadline
There is no urgent deadline for this change. It can be included in the next scheduled release of the Open edX platform.
Other information
This change does not depend on any other changes elsewhere in the codebase. There are no special concerns or limitations, and the change does not involve any database migrations. CopyRetryClaude does not have internet access. Links provided may not be accurate or up to date.