feat: remove enterprise imports from third_party_auth#246
Open
pwnage101 wants to merge 4 commits intorelease-ulmofrom
Open
feat: remove enterprise imports from third_party_auth#246pwnage101 wants to merge 4 commits intorelease-ulmofrom
pwnage101 wants to merge 4 commits intorelease-ulmofrom
Conversation
…onfig This refactoring moves all third-party authentication settings from runtime configuration during AppConfig.ready() to static definitions in lms/envs/common.py. ## Problem The third_party_auth app used an `apply_settings()` function called during Django's app initialization to modify Django settings. This pattern caused issues: 1. Settings were modified after Django initialization when they should be finalized, making debugging difficult 2. Operators couldn't override these settings in their YAML config files because apply_settings() would overwrite their values ## Why the ENABLE_THIRD_PARTY_AUTH conditional was removed The settings are now defined unconditionally because: 1. These settings are inert when social auth backends aren't configured in AUTHENTICATION_BACKENDS - they have no effect 2. ENABLE_THIRD_PARTY_AUTH still controls what matters: registration of authentication backends and exposure of auth URLs 3. This follows standard Django patterns where middleware and settings exist but are no-ops when their feature isn't active ## Enterprise pipeline integration The enterprise pipeline step (handle_enterprise_logistration) is included statically in SOCIAL_AUTH_PIPELINE rather than being inserted dynamically. This step handles its own runtime checks and returns early if enterprise is not configured, making it safe to include always. This avoids complexity with Derived() functions that would need to import Django models at settings load time (before apps are ready). ## Operator impact Operators can now properly override any of these settings in their YAML configuration files, including SOCIAL_AUTH_PIPELINE for custom flows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ise pipeline The settings tests were failing when run with CMS settings because the third_party_auth settings (SOCIAL_AUTH_PIPELINE, ExceptionMiddleware, etc.) are now defined only in lms/envs/common.py. Investigation confirmed CMS does not need these settings: - CMS has no social auth URL endpoints (third_party_auth URLs only included in LMS when ENABLE_THIRD_PARTY_AUTH is true) - CMS uses EdxDjangoStrategy for OAuth2 SSO with LMS, not ConfigurationModelStrategy for third-party identity providers - CMS authentication backends are EdXOAuth2 (LMS SSO) and LtiAuthenticationBackend, not social auth backends - The third_party_auth app is only in cms/envs/test.py INSTALLED_APPS to avoid import errors from indirect dependencies (like enterprise) Changes: - Added @skip_unless_lms decorator to SettingsUnitTest class - Added hasattr guard for SOCIAL_AUTH_PIPELINE in apps.py to prevent AttributeError when running under CMS (which doesn't have this setting) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Don't add a new reference to the old FEATURES dictionary and drop an unncessary test. Co-authored-by: Taylor Payne <taylor.payne2@wgu.edu> Co-authored-by: Feanil Patel <feanil@axim.org>
This was referenced Apr 22, 2026
pwnage101
changed the title
There was a problem hiding this comment.
Pull request overview
Decouples third_party_auth from edx-enterprise by removing enterprise-specific imports/logic, moving social-auth settings into LMS settings, and replacing direct unlink calls with a new SAML disconnect signal (documented via ADRs).
Changes:
- Define python-social-auth (
SOCIAL_AUTH_*) settings statically inlms/envs/common.pyand deletethird_party_auth/settings.py. - Remove enterprise-specific pipeline/linking utilities from platform TPA code and enterprise_support API/tests.
- Add
SAMLAccountDisconnectedsignal and emit it from the SAML backend disconnect flow; update integration tests and add ADR 0026.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| openedx/features/enterprise_support/tests/test_api.py | Removes tests tied to deleted enterprise/TPA coupling helpers. |
| openedx/features/enterprise_support/api.py | Drops enterprise unlink/pipeline helper functions and related model imports. |
| lms/envs/common.py | Adds static python-social-auth settings, pipeline definition, middleware, and context processors. |
| docs/decisions/0026-enterprise-decoupled-from-third-party-auth.rst | New ADR documenting the decoupling approach (pipeline injection + new signal). |
| docs/decisions/0025-saml-admin-views-in-enterprise-plugin.rst | Edits ADR text for clarity/accuracy as part of the migration narrative. |
| common/djangoapps/third_party_auth/utils.py | Removes enterprise model imports and is_enterprise_customer_user. |
| common/djangoapps/third_party_auth/tests/test_utils.py | Removes unit tests for deleted enterprise-dependent utility. |
| common/djangoapps/third_party_auth/tests/test_settings.py | Updates tests to validate new static settings in lms/envs/common.py. |
| common/djangoapps/third_party_auth/tests/specs/test_testshib.py | Updates integration test to verify signal emission instead of enterprise unlinking. |
| common/djangoapps/third_party_auth/signals/signals.py | Introduces SAMLAccountDisconnected signal definition. |
| common/djangoapps/third_party_auth/signals/init.py | Exposes the new signal at the package level. |
| common/djangoapps/third_party_auth/settings.py | Deletes legacy dynamic settings applicator. |
| common/djangoapps/third_party_auth/saml.py | Emits SAMLAccountDisconnected during SAML disconnect instead of calling enterprise unlink directly. |
| common/djangoapps/third_party_auth/pipeline.py | Makes associate_by_email_if_saml a deprecated no-op (enterprise step migrated). |
| common/djangoapps/third_party_auth/models.py | Adds context explaining disable_for_enterprise_sso as legacy enterprise-only field. |
| common/djangoapps/third_party_auth/apps.py | Attempts to mutate pipeline in ready() (currently broken and conflicts with new approach). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # | ||
| # Keyword arguments sent with this signal: | ||
| # request (HttpRequest): The HTTP request during which the disconnect occurred. | ||
| # user (User): The Django User disconnecting the social auth account. |
Comment on lines
+65
to
+70
| @override_settings(FEATURES={'ENABLE_UNICODE_USERNAME': False}) | ||
| def test_social_auth_clean_usernames_default(self): | ||
| """Verify SOCIAL_AUTH_CLEAN_USERNAMES is True when unicode usernames disabled.""" | ||
| # Note: SOCIAL_AUTH_CLEAN_USERNAMES is a Derived setting, computed at settings load time. | ||
| # This test verifies the default behavior (unicode usernames disabled). | ||
| assert settings.SOCIAL_AUTH_CLEAN_USERNAMES is True |
- Remove enterprise pipeline functions and insert_enterprise_pipeline_elements; enterprise pipeline steps are now injected by the enterprise plugin. - Add SAMLAccountDisconnected signal in SAMLAuth.disconnect() to replace the unlink_enterprise_user_from_idp import (moved to the enterprise plugin). - Added an ADR to help explain the migration. ENT-11566 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pwnage101
force-pushed
the
pwnage101/ENT-11566-edx
branch
from
6fe695a to
b98346d
Compare
iloveagent57
approved these changes
Apr 24, 2026
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
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.
This is the 2U fork version of openedx#38103
BTW, I was forced to cherry-pick 3 upstream commits to get this working, hence the commits from feanil.