Skip to content

Commit 38b5af9

Browse files
pwnage101claude
andcommitted
feat: remove enterprise imports from third_party_auth
- 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>
1 parent 5935899 commit 38b5af9

14 files changed

Lines changed: 178 additions & 294 deletions

File tree

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
# lint-amnesty, pylint: disable=missing-module-docstring
22

33
from django.apps import AppConfig
4-
from django.conf import settings
54

65

76
class ThirdPartyAuthConfig(AppConfig): # lint-amnesty, pylint: disable=missing-class-docstring
@@ -11,12 +10,3 @@ class ThirdPartyAuthConfig(AppConfig): # lint-amnesty, pylint: disable=missing-
1110
def ready(self):
1211
# Import signal handlers to register them
1312
from .signals import handlers # noqa: F401 pylint: disable=unused-import
14-
15-
# Note: Third-party auth settings are now defined statically in lms/envs/common.py
16-
# However, the enterprise pipeline step must be inserted dynamically because
17-
# it requires checking if enterprise is enabled, which can't be done at
18-
# settings load time.
19-
# Only insert enterprise elements if SOCIAL_AUTH_PIPELINE exists (LMS only, not CMS).
20-
if hasattr(settings, 'SOCIAL_AUTH_PIPELINE'):
21-
from openedx.features.enterprise_support.api import insert_enterprise_pipeline_elements
22-
insert_enterprise_pipeline_elements(settings.SOCIAL_AUTH_PIPELINE)

common/djangoapps/third_party_auth/models.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,10 @@ class ProviderConfig(ConfigurationModel):
237237
help_text="Use the presence of a profile from a trusted third party as proof of identity verification.",
238238
)
239239

240+
# Enterprise-only field: excludes this provider from the EnterpriseCustomer Django admin IDP
241+
# dropdown. Added in ENT-1366 after social auth providers (Facebook, Google, etc.) were linked
242+
# as enterprise IDPs, incorrectly associating all their users with an enterprise. Should ideally
243+
# be migrated into the enterprise plugin.
240244
disable_for_enterprise_sso = models.BooleanField(
241245
default=False,
242246
verbose_name='Disabled for Enterprise TPA',

common/djangoapps/third_party_auth/pipeline.py

Lines changed: 8 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,7 @@ def B(*args, **kwargs):
8686
from common.djangoapps.edxmako.shortcuts import render_to_string
8787
from common.djangoapps.third_party_auth.utils import (
8888
get_associated_user_by_email_response,
89-
get_user_from_email,
90-
is_enterprise_customer_user,
9189
is_oauth_provider,
92-
is_saml_provider,
9390
user_exists,
9491
)
9592
from common.djangoapps.track import segment
@@ -781,80 +778,18 @@ def associate_by_email_if_oauth(auth_entry, backend, details, user, strategy, *a
781778
return association_response
782779

783780

784-
@partial.partial
785781
def associate_by_email_if_saml(auth_entry, backend, details, user, strategy, *args, **kwargs):
786782
"""
787-
This pipeline step associates the current social auth with the user with the
788-
same email address in the database. It defers to the social library's associate_by_email
789-
implementation, which verifies that only a single database user is associated with the email.
783+
Deprecated — enterprise SAML email association moved to
784+
enterprise.tpa_pipeline.enterprise_associate_by_email.
790785
791-
This association is done ONLY if the user entered the pipeline belongs to SAML provider.
786+
Retained as a no-op for backwards compatibility with custom pipeline configs.
792787
"""
793-
from openedx.features.enterprise_support.api import enterprise_is_enabled
794-
795-
def get_user():
796-
"""
797-
This is the helper method to get the user from system by matching email.
798-
"""
799-
user_details = {'email': details.get('email')} if details else None
800-
return get_user_from_email(user_details or {})
801-
802-
@enterprise_is_enabled()
803-
def associate_by_email_if_enterprise_user():
804-
"""
805-
If the learner arriving via SAML is already linked to the enterprise customer linked to the same IdP,
806-
they should not be prompted for their edX password.
807-
"""
808-
try:
809-
enterprise_customer_user = is_enterprise_customer_user(current_provider.provider_id, current_user)
810-
logger.info(
811-
'[Multiple_SSO_SAML_Accounts_Association_to_User] Enterprise user verification:' # noqa: UP032
812-
'User Email: {email}, User ID: {user_id}, Provider ID: {provider_id},'
813-
' is_enterprise_customer_user: {enterprise_customer_user}'.format(
814-
email=current_user.email,
815-
user_id=current_user.id,
816-
provider_id=current_provider.provider_id,
817-
enterprise_customer_user=enterprise_customer_user,
818-
)
819-
)
820-
821-
if enterprise_customer_user:
822-
# this is python social auth pipeline default method to automatically associate social accounts
823-
# if the email already matches a user account.
824-
association_response, user_is_active = get_associated_user_by_email_response(
825-
backend, details, user, *args, **kwargs)
826-
827-
if not user_is_active:
828-
logger.info(
829-
'[Multiple_SSO_SAML_Accounts_Association_to_User] User association account is not' # noqa: UP032 # pylint: disable=line-too-long
830-
' active: User Email: {email}, User ID: {user_id}, Provider ID: {provider_id},'
831-
' is_enterprise_customer_user: {enterprise_customer_user}'.format(
832-
email=current_user.email,
833-
user_id=current_user.id,
834-
provider_id=current_provider.provider_id,
835-
enterprise_customer_user=enterprise_customer_user
836-
)
837-
)
838-
return None
839-
840-
return association_response
841-
842-
except Exception as ex: # pylint: disable=broad-except
843-
logger.exception('[Multiple_SSO_SAML_Accounts_Association_to_User] Error in'
844-
' saml multiple accounts association: User ID: %s, User Email: %s:,'
845-
'Provider ID: %s, Exception: %s', current_user.id, current_user.email,
846-
current_provider.provider_id, ex)
847-
848-
saml_provider, current_provider = is_saml_provider(strategy.request.backend.name, kwargs)
849-
850-
if saml_provider:
851-
# get the user by matching email if the pipeline user is not available.
852-
current_user = user if user else get_user()
853-
854-
# Verify that the user linked to enterprise customer of current identity provider and an active user
855-
associate_response = associate_by_email_if_enterprise_user() if current_user else None
856-
if associate_response:
857-
return associate_response
788+
logger.warning(
789+
"associate_by_email_if_saml is deprecated and is now a no-op. "
790+
"Enterprise SAML email association has moved to "
791+
"enterprise.tpa_pipeline.enterprise_associate_by_email."
792+
)
858793

859794

860795
def user_details_force_sync(auth_entry, strategy, details, user=None, *args, **kwargs): # lint-amnesty, pylint: disable=keyword-arg-before-vararg

common/djangoapps/third_party_auth/saml.py

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -141,11 +141,17 @@ def auth_url(self):
141141

142142
def disconnect(self, *args, **kwargs):
143143
"""
144-
Override of SAMLAuth.disconnect to unlink the learner from enterprise customer if associated.
145-
"""
146-
from openedx.features.enterprise_support.api import unlink_enterprise_user_from_idp
147-
user = kwargs.get('user', None)
148-
unlink_enterprise_user_from_idp(self.strategy.request, user, self.name)
144+
Override of SAMLAuth.disconnect to emit a signal when a user disconnects their SAML account.
145+
"""
146+
from common.djangoapps.third_party_auth.signals import SAMLAccountDisconnected
147+
# Emit the signal before super().disconnect() so that handlers (e.g. enterprise
148+
# user unlinking) run while the social auth record still exists.
149+
SAMLAccountDisconnected.send(
150+
sender=self.__class__,
151+
request=self.strategy.request,
152+
user=kwargs.get('user', None),
153+
saml_backend=self,
154+
)
149155
return super().disconnect(*args, **kwargs)
150156

151157
def _check_entitlements(self, idp, attributes):
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
11
# Signal handlers for third_party_auth app
2+
3+
from common.djangoapps.third_party_auth.signals.signals import SAMLAccountDisconnected # noqa: F401
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
"""
2+
Signals defined by the third_party_auth app.
3+
"""
4+
from django.dispatch import Signal
5+
6+
# Signal fired when a user disconnects a SAML account.
7+
#
8+
# Keyword arguments sent with this signal:
9+
# request (HttpRequest): The HTTP request during which the disconnect occurred.
10+
# user (User): The Django User disconnecting the social auth account.
11+
# saml_backend: The SAMLAuth backend instance (has a ``name`` attribute).
12+
SAMLAccountDisconnected = Signal()

common/djangoapps/third_party_auth/tests/specs/test_testshib.py

Lines changed: 20 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,7 @@
1212

1313
import ddt
1414
import httpretty
15-
from django.conf import settings
1615
from django.contrib import auth
17-
from enterprise.models import EnterpriseCustomerIdentityProvider, EnterpriseCustomerUser
1816
from freezegun import freeze_time
1917
from social_core import actions
2018
from social_django import views as social_views
@@ -25,11 +23,11 @@
2523
from common.djangoapps.third_party_auth.exceptions import IncorrectConfigurationException
2624
from common.djangoapps.third_party_auth.saml import SapSuccessFactorsIdentityProvider
2725
from common.djangoapps.third_party_auth.saml import log as saml_log
26+
from common.djangoapps.third_party_auth.signals import SAMLAccountDisconnected
2827
from common.djangoapps.third_party_auth.tasks import fetch_saml_metadata
2928
from common.djangoapps.third_party_auth.tests import testutil, utils
3029
from common.test.utils import assert_dict_contains_subset
3130
from openedx.core.djangoapps.user_authn.views.login import login_user
32-
from openedx.features.enterprise_support.tests.factories import EnterpriseCustomerFactory
3331

3432
from .base import IntegrationTestMixin
3533

@@ -220,13 +218,7 @@ class TestShibIntegrationTest(SamlIntegrationTestUtilities, IntegrationTestMixin
220218
"session_index": "1",
221219
}
222220

223-
@patch("openedx.features.enterprise_support.api.enterprise_customer_for_request")
224-
@patch("openedx.features.enterprise_support.utils.third_party_auth.provider.Registry.get")
225-
def test_full_pipeline_succeeds_for_unlinking_testshib_account(
226-
self,
227-
mock_auth_provider,
228-
mock_enterprise_customer_for_request,
229-
):
221+
def test_full_pipeline_succeeds_for_unlinking_testshib_account(self):
230222

231223
# First, create, the request and strategy that store pipeline state,
232224
# configure the backend, and mock out wire traffic.
@@ -245,30 +237,6 @@ def test_full_pipeline_succeeds_for_unlinking_testshib_account(
245237
# We're already logged in, so simulate that the cookie is set correctly
246238
self.set_logged_in_cookies(request)
247239

248-
# linking a learner with enterprise customer.
249-
enterprise_customer = EnterpriseCustomerFactory()
250-
assert EnterpriseCustomerUser.objects.count() == 0, "Precondition check: no link records should exist"
251-
EnterpriseCustomerUser.objects.link_user(enterprise_customer, user.email)
252-
assert (
253-
EnterpriseCustomerUser.objects.filter(enterprise_customer=enterprise_customer, user_id=user.id).count() == 1
254-
)
255-
EnterpriseCustomerIdentityProvider.objects.get_or_create(
256-
enterprise_customer=enterprise_customer, provider_id=self.provider.provider_id
257-
)
258-
259-
enterprise_customer_data = {
260-
"uuid": enterprise_customer.uuid,
261-
"name": enterprise_customer.name,
262-
"identity_provider": "saml-default",
263-
"identity_providers": [
264-
{
265-
"provider_id": "saml-default",
266-
}
267-
],
268-
}
269-
mock_auth_provider.return_value.backend_name = "tpa-saml"
270-
mock_enterprise_customer_for_request.return_value = enterprise_customer_data
271-
272240
# Instrument the pipeline to get to the dashboard with the full expected state.
273241
self.client.get(pipeline.get_login_url(self.provider.provider_id, pipeline.AUTH_ENTRY_LOGIN))
274242

@@ -285,31 +253,35 @@ def test_full_pipeline_succeeds_for_unlinking_testshib_account(
285253
# First we expect that we're in the linked state, with a backend entry.
286254
self.assert_social_auth_exists_for_user(request.user, strategy)
287255

288-
FEATURES_WITH_ENTERPRISE_ENABLED = settings.FEATURES.copy()
289-
FEATURES_WITH_ENTERPRISE_ENABLED["ENABLE_ENTERPRISE_INTEGRATION"] = True
290-
with patch.dict("django.conf.settings.FEATURES", FEATURES_WITH_ENTERPRISE_ENABLED):
256+
# Track signal emissions to verify the disconnect signal is sent.
257+
signal_calls = []
258+
259+
def signal_receiver(sender, **kwargs):
260+
signal_calls.append(kwargs)
261+
262+
SAMLAccountDisconnected.connect(signal_receiver)
263+
try:
291264
# Fire off the disconnect pipeline without the user information.
292265
actions.do_disconnect(
293266
request.backend, None, None, redirect_field_name=auth.REDIRECT_FIELD_NAME, request=request
294267
)
295-
assert (
296-
EnterpriseCustomerUser.objects.filter(enterprise_customer=enterprise_customer, user_id=user.id).count()
297-
!= 0
298-
)
299268

300269
# Fire off the disconnect pipeline to unlink.
301270
self.assert_redirect_after_pipeline_completes(
302271
actions.do_disconnect(
303272
request.backend, user, None, redirect_field_name=auth.REDIRECT_FIELD_NAME, request=request
304273
)
305274
)
306-
# Now we expect to be in the unlinked state, with no backend entry.
307-
self.assert_third_party_accounts_state(request, linked=False)
308-
self.assert_social_auth_does_not_exist_for_user(user, strategy)
309-
assert (
310-
EnterpriseCustomerUser.objects.filter(enterprise_customer=enterprise_customer, user_id=user.id).count()
311-
== 0
312-
)
275+
finally:
276+
SAMLAccountDisconnected.disconnect(signal_receiver)
277+
278+
# Now we expect to be in the unlinked state, with no backend entry.
279+
self.assert_third_party_accounts_state(request, linked=False)
280+
self.assert_social_auth_does_not_exist_for_user(user, strategy)
281+
# Verify that the SAMLAccountDisconnected signal was emitted.
282+
# The actual enterprise user unlinking is handled by edx-enterprise's
283+
# signal handler, not by openedx-platform.
284+
assert len(signal_calls) == 2, f"Expected 2 signal emissions, got {len(signal_calls)}"
313285

314286
def get_response_data(self):
315287
"""Gets dict (string -> object) of merged data about the user."""

common/djangoapps/third_party_auth/tests/test_utils.py

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,11 @@
1515
create_or_update_bulk_saml_provider_data,
1616
get_associated_user_by_email_response,
1717
get_user_from_email,
18-
is_enterprise_customer_user,
1918
is_oauth_provider,
2019
parse_metadata_xml,
2120
user_exists,
2221
)
2322
from openedx.core.djangolib.testing.utils import skip_unless_lms
24-
from openedx.features.enterprise_support.tests.factories import (
25-
EnterpriseCustomerIdentityProviderFactory,
26-
EnterpriseCustomerUserFactory,
27-
)
2823

2924

3025
@ddt.ddt
@@ -55,26 +50,6 @@ def test_get_user(self):
5550
assert get_user_from_email({'email': 'test_user@example.com'})
5651
assert not get_user_from_email({'email': 'invalid@example.com'})
5752

58-
def test_is_enterprise_customer_user(self):
59-
"""
60-
Verify that if user is an enterprise learner.
61-
"""
62-
# Create users from factory
63-
64-
user = UserFactory(username='test_user', email='test_user@example.com')
65-
other_user = UserFactory(username='other_user', email='other_user@example.com')
66-
customer_idp = EnterpriseCustomerIdentityProviderFactory.create(
67-
provider_id='the-provider',
68-
)
69-
customer = customer_idp.enterprise_customer
70-
EnterpriseCustomerUserFactory.create(
71-
enterprise_customer=customer,
72-
user_id=user.id,
73-
)
74-
75-
assert is_enterprise_customer_user('the-provider', user)
76-
assert not is_enterprise_customer_user('the-provider', other_user)
77-
7853
@ddt.data(
7954
('saml-farkle', False),
8055
('oa2-fergus', True),

common/djangoapps/third_party_auth/utils.py

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import dateutil.parser
99
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user
1010
from django.utils.timezone import now
11-
from enterprise.models import EnterpriseCustomerIdentityProvider, EnterpriseCustomerUser
1211
from lxml import etree
1312
from onelogin.saml2.utils import OneLogin_Saml2_Utils
1413
from social_core.pipeline.social_auth import associate_by_email
@@ -164,14 +163,6 @@ def is_saml_provider(backend, kwargs):
164163
current_provider.slug in [saml_provider.slug for saml_provider in saml_providers_list]), current_provider
165164

166165

167-
def is_enterprise_customer_user(provider_id, user):
168-
""" Verify that the user linked to enterprise customer of current identity provider"""
169-
enterprise_idp = EnterpriseCustomerIdentityProvider.objects.get(provider_id=provider_id)
170-
171-
return EnterpriseCustomerUser.objects.filter(enterprise_customer=enterprise_idp.enterprise_customer,
172-
user_id=user.id).exists()
173-
174-
175166
def is_oauth_provider(backend_name, **kwargs):
176167
"""
177168
Verify that the third party provider uses oauth

docs/decisions/0025-saml-admin-views-in-enterprise-plugin.rst

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,11 @@ edx-enterprise repository:
4040
* ``enterprise/api/v1/views/saml_provider_config.py``
4141
* ``enterprise/api/v1/views/saml_provider_data.py``
4242

43-
They will be registered in ``enterprise/api/v1/urls.py`` under the same
44-
``auth/saml/v0/`` prefix, so the API contract is preserved for existing clients.
43+
They will be registered under the same ``auth/saml/v0/`` prefix, so the API
44+
contract is preserved for existing clients.
4545

4646
The underlying ``SAMLProviderConfig``, ``SAMLProviderData``, and
47-
``SAMLConfiguration`` models remain in
48-
``common/djangoapps/third_party_auth/models.py`` because they are
47+
``SAMLConfiguration`` models remain in openedx-platform because they are
4948
general-purpose SAML models used by the platform's authentication layer
5049
regardless of whether the enterprise plugin is installed.
5150

@@ -58,10 +57,9 @@ Consequences
5857

5958
* **Dependency direction**: edx-enterprise now imports ``SAMLProviderConfig``,
6059
``SAMLProviderData``, and several utility functions from
61-
``common.djangoapps.third_party_auth``. This is the correct dependency
62-
direction (plugin depends on platform, not the reverse).
63-
64-
* **Deployments without edx-enterprise**: The SAML admin API endpoints will not
65-
be available in deployments that do not install edx-enterprise. This is
66-
acceptable because the endpoints ONLY serve frontend-app-admin-portal
67-
which doesn't work without edx-enterprise installed anyway.
60+
``common.djangoapps.third_party_auth``. This makes the plugin more sensitive to
61+
platform code changes in these models/functions. But this isn't worse than
62+
before, which was just the same problem in the opposite direction. Ideally
63+
there'd be a cleaner and more stable set of hooks into TPA that enterprise
64+
can leverage, but that's a major enhancement which is not in scope for this
65+
phase of the migration work.

0 commit comments

Comments
 (0)