Revert "AUT-31 code changes & implementation for redirection from Legacy page to new Authn MFE"#118
Conversation
…acy page to new Authn MFE"
There was a problem hiding this comment.
Pull request overview
This PR reverts changes from PR #60 that implemented redirection from legacy authentication pages to the new Authn MFE for enterprise customers with specific TPA and SAML configurations.
Changes:
- Removes enterprise/TPA/SAML conditional logic from the
should_redirect_to_authn_microfrontend()function - Reverts the boolean environment variable handling for
ENABLE_AUTHN_MICROFRONTENDfrom string comparison back to directos.environ.get()
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openedx/core/djangoapps/user_authn/toggles.py | Simplifies the authn MFE redirect logic by removing enterprise/TPA/SAML conditional checks |
| lms/envs/common.py | Changes environment variable handling for ENABLE_AUTHN_MICROFRONTEND setting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # .. toggle_warning: Also set settings.AUTHN_MICROFRONTEND_URL for rollout. This temporary feature | ||
| # toggle does not have a target removal date. | ||
| ENABLE_AUTHN_MICROFRONTEND = os.getenv("EDXAPP_ENABLE_AUTHN_MFE", "false").lower() == "true" | ||
| ENABLE_AUTHN_MICROFRONTEND = os.environ.get("EDXAPP_ENABLE_AUTHN_MFE", False) |
There was a problem hiding this comment.
The use of os.environ.get("EDXAPP_ENABLE_AUTHN_MFE", False) is problematic because when the environment variable is set, it returns a string (e.g., "true", "false", "1", "0"), not a boolean. In Python, any non-empty string is truthy, meaning even setting EDXAPP_ENABLE_AUTHN_MFE="false" would evaluate to True.
This differs from the code being reverted which used .lower() == "true" to properly convert the string to a boolean. The current implementation will incorrectly enable the authn microfrontend whenever the environment variable is set to any non-empty value.
Consider using a proper boolean conversion such as:
os.getenv("EDXAPP_ENABLE_AUTHN_MFE", "false").lower() == "true"- Or a helper function that safely converts string values to booleans
| ENABLE_AUTHN_MICROFRONTEND = os.environ.get("EDXAPP_ENABLE_AUTHN_MFE", False) | |
| ENABLE_AUTHN_MICROFRONTEND = os.getenv("EDXAPP_ENABLE_AUTHN_MFE", "false").lower() == "true" |
ferdis
deleted the
revert-60-AUT-31-code-change-identification-for-enterprise-applications
branch
| return configuration_helpers.get_value( | ||
| 'ENABLE_AUTHN_MICROFRONTEND', settings.FEATURES.get('ENABLE_AUTHN_MICROFRONTEND') | ||
| ) and not ( | ||
| configuration_helpers.get_value('ENABLE_ENTERPRISE_CUSTOMER', False) and | ||
| configuration_helpers.get_value('ENABLE_TPA_HINT_PROVIDER', False) and | ||
| configuration_helpers.get_value('ENABLE_SAML_PROVIDER', False) |
There was a problem hiding this comment.
Here is some example pseudo-code that would check a new additional toggle before entering the new logic.
# example new toggle to protect check of additional conditions
if AUTHN_MFE_ADDITIONAL_SKIP_CONDITIONS.is_enabled():
If configuration_helpers checks say we should skip:
return False
return configuration_helpers.get_value(
'ENABLE_AUTHN_MICROFRONTEND', settings.FEATURES.get('ENABLE_AUTHN_MICROFRONTEND')
)
Note that there were no unit tests for this, which could help ensure you are getting what you wish before getting this to any environment for testing.
Reverts #60