feat: remove enterprise imports from third_party_auth by pwnage101 · Pull Request #246 · edx/edx-platform

pwnage101 · 2026-04-22T23:58:34Z

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.

Copilot

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 in lms/envs/common.py and delete third_party_auth/settings.py.
Remove enterprise-specific pipeline/linking utilities from platform TPA code and enterprise_support API/tests.
Add SAMLAccountDisconnected signal 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.

+    @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


Copilot

Pull request overview

This PR decouples third_party_auth from enterprise-specific code so edx-enterprise can be an optional dependency, replacing direct enterprise hooks with platform-defined settings and a new disconnect signal.

Changes:

Remove enterprise imports/logic from third_party_auth utilities/pipeline and delete enterprise-specific helpers from enterprise_support.api.
Move core python-social-auth settings (pipeline, middleware, context processors, etc.) into lms/envs/common.py (static settings instead of apply_settings() mutation).
Add a new SAMLAccountDisconnected signal and update SAML disconnect behavior and integration tests accordingly; add ADR documentation for the migration.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
openedx/features/enterprise_support/tests/test_api.py	Removes tests tied to enterprise pipeline injection and IdP unlink helper.
openedx/features/enterprise_support/api.py	Drops enterprise unlink/pipeline-injection helpers and associated imports.
lms/envs/common.py	Defines social-auth settings/pipeline and adds required middleware/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	Updates ADR wording around enterprise/plugin dependency direction.
common/djangoapps/third_party_auth/utils.py	Removes enterprise model imports and the enterprise-specific helper.
common/djangoapps/third_party_auth/tests/test_utils.py	Removes tests that depended on enterprise factories/models.
common/djangoapps/third_party_auth/tests/test_settings.py	Reworks tests to assert settings now defined in `lms/envs/common.py`.
common/djangoapps/third_party_auth/tests/specs/test_testshib.py	Updates unlink/disconnect integration test to validate signal emission instead of enterprise cleanup.
common/djangoapps/third_party_auth/signals/signals.py	Introduces `SAMLAccountDisconnected` signal definition.
common/djangoapps/third_party_auth/signals/init.py	Exposes `SAMLAccountDisconnected` from the signals package.
common/djangoapps/third_party_auth/settings.py	Removes legacy `apply_settings()` implementation.
common/djangoapps/third_party_auth/saml.py	Emits `SAMLAccountDisconnected` during SAML disconnect.
common/djangoapps/third_party_auth/pipeline.py	Makes `associate_by_email_if_saml` a deprecated no-op for backward compatibility.
common/djangoapps/third_party_auth/models.py	Adds clarification comments about the enterprise-only `disable_for_enterprise_sso` field.
common/djangoapps/third_party_auth/apps.py	Removes dynamic settings enablement from AppConfig startup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…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>

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- 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>

Copilot AI review requested due to automatic review settings April 22, 2026 23:58

Copilot started reviewing on behalf of pwnage101 April 22, 2026 23:59 View session

This was referenced Apr 22, 2026

feat: remove enterprise imports from third_party_auth openedx/openedx-platform#38103

Merged

feat: add TPA pipeline step and signal handler for enterprise openedx/edx-enterprise#2549

Merged

pwnage101 changed the title ~~Pwnage101/ent 11566 edx~~ feat: remove enterprise imports from third_party_auth Apr 23, 2026

Copilot AI reviewed Apr 23, 2026

View reviewed changes

pwnage101 force-pushed the pwnage101/ENT-11566-edx branch from 6fe695a to b98346d Compare April 23, 2026 19:03

iloveagent57 approved these changes Apr 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings April 30, 2026 23:07

pwnage101 force-pushed the pwnage101/ENT-11566-edx branch from b98346d to 297dd7b Compare April 30, 2026 23:07

Copilot started reviewing on behalf of pwnage101 April 30, 2026 23:08 View session

pwnage101 force-pushed the pwnage101/ENT-11566-edx branch from 297dd7b to e762650 Compare April 30, 2026 23:10

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread lms/envs/common.py

Comment thread common/djangoapps/third_party_auth/tests/test_settings.py

feanil and others added 3 commits May 5, 2026 14:29

fix: Apply suggestions from code review

04ad053

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>

pwnage101 force-pushed the pwnage101/ENT-11566-edx branch from e762650 to d997fab Compare May 5, 2026 21:31

pwnage101 closed this May 5, 2026

pwnage101 reopened this May 5, 2026

Copilot AI review requested due to automatic review settings May 5, 2026 22:05

pwnage101 force-pushed the pwnage101/ENT-11566-edx branch from d997fab to cc373ac Compare May 5, 2026 22:05

Copilot started reviewing on behalf of pwnage101 May 5, 2026 22:05 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

pwnage101 enabled auto-merge May 5, 2026 22:15

pwnage101 force-pushed the pwnage101/ENT-11566-edx branch from cc373ac to 6474903 Compare May 6, 2026 02:07

pwnage101 closed this May 6, 2026

auto-merge was automatically disabled May 6, 2026 02:08
Pull request was closed

pwnage101 reopened this May 6, 2026

pwnage101 closed this May 6, 2026

pwnage101 reopened this May 6, 2026

pwnage101 merged commit 9e6ef2e into release-ulmo May 6, 2026
116 of 192 checks passed

pwnage101 deleted the pwnage101/ENT-11566-edx branch May 6, 2026 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove enterprise imports from third_party_auth#246

feat: remove enterprise imports from third_party_auth#246
pwnage101 merged 4 commits intorelease-ulmofrom
pwnage101/ENT-11566-edx

pwnage101 commented Apr 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

pwnage101 commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pwnage101 commented Apr 22, 2026 •

edited

Loading