feat: add TPA pipeline step and signal handler for enterprise#2549
feat: add TPA pipeline step and signal handler for enterprise#2549
Conversation
a1edcbb to
80e2b97
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2549 +/- ##
==========================================
+ Coverage 85.96% 86.01% +0.04%
==========================================
Files 249 249
Lines 16612 16674 +62
Branches 1645 1655 +10
==========================================
+ Hits 14281 14342 +61
- Misses 1997 1998 +1
Partials 334 334
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bb11c4b to
ff7139b
Compare
There was a problem hiding this comment.
Pull request overview
Adds enterprise-specific hooks into the third-party-auth (TPA) flow to better support enterprise SSO behavior in the Open edX platform, including a new pipeline step for conditional email association and a platform signal handler to unlink enterprise users on SAML disconnect.
Changes:
- Add
enterprise_associate_by_emailpipeline step and inject it intoSOCIAL_AUTH_PIPELINEvia enterprise plugin settings. - Add a platform-consumed signal handler for SAML social-auth disconnects to unlink enterprise user associations.
- Add/adjust dependencies and tests; bump package version and changelog entry.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
enterprise/tpa_pipeline.py |
Adds enterprise email-association pipeline step for SAML flows. |
enterprise/settings/common.py |
Injects enterprise pipeline steps into the platform’s social-auth pipeline. |
enterprise/platform_signal_handlers.py |
Adds SAML disconnect handler + unlink helper. |
enterprise/apps.py |
Connects the platform’s SAMLAccountDisconnected signal to the new handler. |
enterprise/utils.py |
Adds get_user_from_email helper and type hints for email lookup utilities. |
enterprise/settings/test.py |
Enables ENABLE_ENTERPRISE_INTEGRATION in test settings. |
tests/test_tpa_pipeline.py |
Adds unit tests for enterprise_associate_by_email. |
tests/test_settings.py |
Adds tests ensuring pipeline injection placement and idempotency. |
tests/test_platform_signal_handlers.py |
Adds tests for unlink helper + disconnect signal handler behavior. |
requirements/base.in |
Adds python3-saml to base dependencies. |
requirements/test*.txt, requirements/doc.txt, requirements/dev.txt |
Locks transitive deps for SAML support (python3-saml, xmlsec, isodate, etc.). |
enterprise/__init__.py |
Bumps version to 7.0.5. |
CHANGELOG.rst |
Adds 7.0.5 release entry for the new feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
abe8d19 to
21cb498
Compare
| """ | ||
| from enterprise.signals import handle_user_post_save # pylint: disable=import-outside-toplevel | ||
|
|
||
| from django.db.models.signals import post_save, pre_migrate # pylint: disable=C0415, # isort:skip |
There was a problem hiding this comment.
As far as I can tell, importing post_save and pre_migrate never needed to be deferred. In fact, importing signals generally doesn't need to be deferred. It's just the handlers that need to be deferred because they often import models.
| # with an EnterpriseCustomer when applicable. This it the unique identifier | ||
| # used to ensure that signal receiver is only called once. | ||
| USER_POST_SAVE_DISPATCH_UID = "user_post_save_upgrade_pending_enterprise_customer_user" | ||
| USER_POST_SAVE_DISPATCH_UID = "enterprise.user_post_save_upgrade_pending_enterprise_customer_user" |
There was a problem hiding this comment.
Seems like good practice to prefix the dispatch UID with something unique.
ff7d54f to
d5bc766
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3ba4e30 to
2a8db44
Compare
|
TODO: Come up with stage validation plan. Audit logging and potentially come up with a datadog dashboard to monitor rollout. |
5514661 to
26f1d96
Compare
|
@kiram15 I did just make a major fix to the final instructions printed to the console at the end of the provisioning script: diff --git a/provision-tpa.py b/provision-tpa.py
index ebfb8600..9ba49475 100644
--- a/provision-tpa.py
+++ b/provision-tpa.py
@@ -38,7 +38,9 @@ SAML_SLUG = os.environ['SAML_SLUG']
OID_EMAIL = os.environ['OID_EMAIL']
OID_GIVEN_NAME = os.environ['OID_GIVEN_NAME']
OID_SURNAME = os.environ['OID_SURNAME']
+TEST_USERNAME = os.environ['TEST_USERNAME']
TEST_EMAIL = os.environ['TEST_EMAIL']
+TEST_PASSWORD = os.environ['TEST_PASSWORD']
TEST_FIRST_NAME = os.environ['TEST_FIRST_NAME']
TEST_LAST_NAME = os.environ['TEST_LAST_NAME']
LMS_USERNAME = os.environ['LMS_USERNAME']
@@ -191,4 +193,5 @@ print(f'EnterpriseCustomerUser {action}: active={ecu.active}')
# ---------------------------------------------------------------------------
print('\n=== LMS TPA provisioning complete ===')
print(f'SAML login URL: http://localhost:18000/auth/login/tpa-saml/?auth_entry=login&idp={SAML_SLUG}')
-print(f'Test user: {LMS_USERNAME} / {LMS_PASSWORD} (email: {TEST_EMAIL})')
+print(f'Keycloak login: {TEST_USERNAME} / {TEST_PASSWORD}')
+print(f'LMS login: {TEST_EMAIL} / {LMS_PASSWORD}')Basically, I was telling the user the wrong username/password to use on the IdP (keycloak). In practice, I think the flow involves logging into both keycloak AND the LMS, so both sets of usernames/passwords are needed. |
|
Here's what should happen: Screen.Recording.2026-04-23.at.12.47.48.mov |
|
UGH---my unclean devstack led me to believe there should have been an LMS login prompt, when actually the whole point of the injected pipeline step ( Screen.Recording.2026-04-23.at.14.05.12.movApplied this fix to the docs to reflect my findings: diff --git a/docs/saml_testing.rst b/docs/saml_testing.rst
index 9ed9b376..848bd45b 100644
--- a/docs/saml_testing.rst
+++ b/docs/saml_testing.rst
@@ -83,8 +83,13 @@ Testing the SAML login flow
Password ``testpass``
========= =========================
-4. After successful authentication you should be redirected back to the LMS
- learner dashboard, logged in as ``keycloak_test_learner``.
+4. Validate that you were **not** prompted to log into the existing LMS user.
+ The ``enterprise_associate_by_email`` pipeline step should discover that the
+ pre-provisioned LMS learner is already associated with the SAML-enabled
+ enterprise customer, so LMS authentication is skipped.
+
+5. Validate that you have been redirected to the LMS learner dashboard and are
+ logged in as ``keycloak_test_learner``.
Stopping Keycloak
-----------------
diff --git a/provision-tpa.py b/provision-tpa.py
index 9ba49475..2d3bb5bf 100644
--- a/provision-tpa.py
+++ b/provision-tpa.py
@@ -194,4 +194,3 @@ print(f'EnterpriseCustomerUser {action}: active={ecu.active}')
print('\n=== LMS TPA provisioning complete ===')
print(f'SAML login URL: http://localhost:18000/auth/login/tpa-saml/?auth_entry=login&idp={SAML_SLUG}')
print(f'Keycloak login: {TEST_USERNAME} / {TEST_PASSWORD}')
-print(f'LMS login: {TEST_EMAIL} / {LMS_PASSWORD}')Here's my cleanup script (run in the LMS) for posterity: Cleanup Script
from django.contrib.auth import get_user_model
from social_django.models import UserSocialAuth
from common.djangoapps.student.models import UserProfile
from common.djangoapps.third_party_auth.models import (
SAMLConfiguration,
SAMLProviderConfig,
SAMLProviderData,
)
from enterprise.models import (
EnterpriseCustomerIdentityProvider,
EnterpriseCustomerUser,
)
User = get_user_model()
# Constants from keycloak-devstack.env
SAML_SLUG = 'keycloak-devstack'
IDP_ENTITY_ID = 'http://edx.devstack.keycloak:8080/realms/devstack'
PROVIDER_ID = f'saml-{SAML_SLUG}'
LMS_USERNAME = 'keycloak_test_learner'
# Step 8: Remove pre-linked enterprise learner
try:
learner = User.objects.get(username=LMS_USERNAME)
UserSocialAuth.objects.filter(user=learner).delete()
EnterpriseCustomerUser.objects.filter(user_id=learner.id).delete()
UserProfile.objects.filter(user=learner).delete()
learner.delete()
print(f'Deleted user {LMS_USERNAME} and related records')
except User.DoesNotExist:
print(f'User {LMS_USERNAME} not found, skipping')
# Step 4: Remove EnterpriseCustomerIdentityProvider
d = EnterpriseCustomerIdentityProvider.objects.filter(provider_id=PROVIDER_ID).delete()
print(f'EnterpriseCustomerIdentityProvider: {d}')
# Steps 5-6: Remove SAMLProviderData (fetched metadata)
d = SAMLProviderData.objects.filter(entity_id=IDP_ENTITY_ID).delete()
print(f'SAMLProviderData: {d}')
# Step 3: Remove SAMLProviderConfig versions
d = SAMLProviderConfig.objects.filter(slug=SAML_SLUG).delete()
print(f'SAMLProviderConfig: {d}')
# Step 2: Remove SAMLConfiguration (global SP config)
# Note: this is shared across all SAML providers. Skip if you have other providers.
d = SAMLConfiguration.objects.filter(slug='default').delete()
print(f'SAMLConfiguration: {d}')
# Step 1 (seed_enterprise_devstack_data) is left intact -- the
# EnterpriseCustomer and other seed data may be used elsewhere.
print('\nDone. Run provisioning again with: docker compose down -v keycloak && make dev.up.keycloak && make dev.provision.keycloak') |
26f1d96 to
eff75a8
Compare
iloveagent57
left a comment
There was a problem hiding this comment.
Awesome job on the keycloak stuff
eff75a8 to
f568b2b
Compare
f568b2b to
7a9f6f6
Compare
|
Updated testing docs to cover additional "unlink from IdP" integration test. diff --git a/docs/saml_testing.rst b/docs/saml_testing.rst
index 848bd45b..84ad8474 100644
--- a/docs/saml_testing.rst
+++ b/docs/saml_testing.rst
@@ -91,6 +91,68 @@ Testing the SAML login flow
5. Validate that you have been redirected to the LMS learner dashboard and are
logged in as ``keycloak_test_learner``.
+Testing the SAML disconnect flow
+--------------------------------
+
+Triggering the disconnect via the Account MFE
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+1. Bring up the Account MFE container (devstack runs it on port 1997):
+
+ .. code-block:: bash
+
+ $ make dev.up.frontend-app-account
+
+2. In the same browser session where you completed the SAML login, navigate to
+ the Linked Accounts section:
+
+ ``http://localhost:1997/#linked-accounts``
+
+3. Find the ``Keycloak Devstack IdP`` entry (matches
+ ``SAMLProviderConfig.name``) and click **Unlink Keycloak Devstack IdP
+ account**.
+
+4. The button should settle into the "unconnected" state with a "Sign in with
+ Keycloak Devstack IdP" link. indicating the MFE received a successful
+ disconnect response.
+
+Verifying the disconnect
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+1. **LMS logs** -- tail the LMS container and grep for the new debug lines:
+
+ .. code-block:: bash
+
+ $ docker logs --tail 500 edx.devstack.lms 2>&1 | grep -E 'SAMLAccountDisconnected|_unlink_enterprise_user_from_idp|successfully unlinked'
+
+ You should see all three lines, in order::
+
+ [THIRD_PARTY_AUTH] Emitting SAMLAccountDisconnected signal for user_id=<id>, backend=tpa-saml
+ [ENTERPRISE] _unlink_enterprise_user_from_idp called for user_id=<id>, backend=tpa-saml
+ Enterprise learner {keycloak_learner@example.com} successfully unlinked from Enterprise Customer {<name>}
+
+Resetting state to repeat the test
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The simplest reset is to re-run provisioning:
+
+.. code-block:: bash
+
+ $ make dev.provision.keycloak
+
+Then navigate to the SAML login URL again to re-link:
+
+ ``http://localhost:18000/auth/login/tpa-saml/?auth_entry=login&idp=keycloak-devstack``
+
+Note: re-running provisioning is necessary because when you clicked the
+**Unlink Keycloak Devstack IdP account** button, the SAML disconnect handler
+did more than just disconnect from the IdP, it also unlinked the
+EnterpriseCustomerUser. This is only recoverable by an admin or system
+operator, hence the need to use the provision script. Yes, that means in prod
+if a learner accidentally clicks the unlink-from-IdP button, they ALSO get
+unlinked from the enterprise itself and need to reach out to their admin to get
+re-linked to the enterprise.
+
Stopping Keycloak
-----------------
@@ -120,3 +182,7 @@ Troubleshooting
The Keycloak admin console is available at
``http://localhost:8080/admin/master/console/`` with credentials
``admin`` / ``admin``.
+
+**Account MFE shows no linked providers**
+ ``UserSocialAuth`` likely has no row for this user -- complete the SAML
+ login first.
diff --git a/enterprise/signals.py b/enterprise/signals.py
index 5019fde9..4577b483 100644
--- a/enterprise/signals.py
+++ b/enterprise/signals.py
@@ -497,6 +497,13 @@ def _unlink_enterprise_user_from_idp(request: HttpRequest, user: AbstractUser, i
"""
Un-links learner from their enterprise identity provider.
+ Note: despite the name, this also flips ``EnterpriseCustomerUser.linked``
+ to ``False``, severing the learner's enterprise membership entirely -- not
+ just their IdP association. Re-linking is not learner-self-service and
+ requires admin intervention via the enterprise customer API or Django
+ admin. This is legacy behavior preserved during a refactor; whether it was
+ a conscious product decision is unclear.
+
Args:
request: The current HTTP request.
user (User): User who initiated disconnect request. |
- Add enterprise_associate_by_email pipeline step, replacing the removed pipeline step from platform. - Add signal handler to unlink enterprise users from an IdP (signal: SAMLAccountDisconnected) - Inject all enterprise-specific TPA pipeline steps via plugin_settings. ENT-11566 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
John Nagro originally leveraged Keycloak to provide an identity provider for testing SAML-based third party auth (TPA) in edx devstack, but it was not broadly usable because it lacked scripts for keycloak provisioning, LMS TPA provisioning, and other necessary scaffolding to tie it all together. ENT-11566
7a9f6f6 to
2201424
Compare
2 commits: One to update business logic, and one to enhance local SAML testing tooling.
ENT-11566
blocks:
Testing
Do stuff to make sure devstack has the following branches cloned:
pwnage101/ENT-11566pwnage101/ENT-11566-edxFrom the devstack repo:
make lms-up/edx/src/edx-enterpriseas editable in the LMS container.From the edx-enterprise repo:
make dev.up.keycloakmake dev.provision.keycloakFrom your MacBook:
/etc/hosts:127.0.0.1 edx.devstack.keycloakhttp://edx.devstack.keycloak:8080/realms/...)keycloak_learner/testpassenterprise_associate_by_emailshould successfully discover that the existing LMS learner is already associated with the SAML-enabled customer, so LMS authentication can be skipped.keycloak_test_learner