Skip to content

Commit feaf970

Browse files
committed
chore: Remove waffle flag and logs for saml tpa redirect
1 parent 8aa3081 commit feaf970

4 files changed

Lines changed: 34 additions & 86 deletions

File tree

common/djangoapps/third_party_auth/pipeline.py

Lines changed: 11 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -102,10 +102,7 @@ def B(*args, **kwargs):
102102
is_saml_provider,
103103
user_exists,
104104
)
105-
from common.djangoapps.third_party_auth.toggles import (
106-
is_saml_provider_site_fallback_enabled,
107-
is_tpa_next_url_on_dispatch_enabled,
108-
)
105+
from common.djangoapps.third_party_auth.toggles import is_tpa_next_url_on_dispatch_enabled
109106
from common.djangoapps.track import segment
110107
from common.djangoapps.util.json_request import JsonResponse
111108

@@ -363,10 +360,10 @@ def get_complete_url(backend_name):
363360
ValueError: if no provider is enabled with the given backend_name.
364361
"""
365362
if not any(provider.Registry.get_enabled_by_backend_name(backend_name)):
366-
# When the SAML site-fallback flag is on, the provider may not be visible to the
367-
# site-filtered registry even though SAML auth already completed via a
368-
# site-independent lookup. Allow get_complete_url to proceed in that case.
369-
if not (is_saml_provider_site_fallback_enabled() and backend_name == 'tpa-saml'):
363+
# For tpa-saml, the provider may not be visible to the site-filtered registry
364+
# even though SAML auth already completed via a site-independent lookup.
365+
# Allow get_complete_url to proceed in that case.
366+
if backend_name != 'tpa-saml':
370367
raise ValueError('Provider with backend %s not enabled' % backend_name)
371368

372369
return _get_url('social:complete', backend_name)
@@ -621,33 +618,14 @@ def is_provider_saml():
621618
strategy.storage.partial.store(current_partial)
622619

623620
if not user:
624-
# Use only email for user existence check in case of saml provider
625-
_is_saml = is_provider_saml()
626-
_provider_obj = provider.Registry.get_from_pipeline({'backend': current_partial.backend, 'kwargs': kwargs})
627-
logger.info(
628-
'[THIRD_PARTY_AUTH] ensure_user_information: auth_entry=%s backend=%s is_provider_saml=%s '
629-
'current_provider=%s skip_email_verification=%s send_to_registration_first=%s '
630-
'email=%s kwargs_response_keys=%s',
631-
auth_entry,
632-
current_partial.backend,
633-
_is_saml,
634-
_provider_obj.provider_id if _provider_obj else None,
635-
_provider_obj.skip_email_verification if _provider_obj else None,
636-
_provider_obj.send_to_registration_first if _provider_obj else None,
637-
details.get('email') if details else None,
638-
list((kwargs.get('response') or {}).keys()),
639-
)
640-
if _is_saml:
621+
# Use only email for user existence check in case of saml provider.
622+
# Check the backend name directly rather than the site-filtered registry,
623+
# since the provider may only be visible via the site-independent fallback.
624+
if current_partial.backend == 'tpa-saml':
641625
user_details = {'email': details.get('email')} if details else None
642626
else:
643627
user_details = details
644-
_user_exists = user_exists(user_details or {})
645-
logger.info(
646-
'[THIRD_PARTY_AUTH] ensure_user_information: user_exists=%s user_details_email=%s',
647-
_user_exists,
648-
(user_details or {}).get('email'),
649-
)
650-
if _user_exists:
628+
if user_exists(user_details or {}):
651629
# User has not already authenticated and the details sent over from
652630
# identity provider belong to an existing user.
653631
logger.info('[THIRD_PARTY_AUTH] ensure_user_information: dispatching to login (user exists)')
@@ -658,12 +636,7 @@ def is_provider_saml():
658636
elif auth_entry == AUTH_ENTRY_LOGIN:
659637
# User has authenticated with the third party provider but we don't know which edX
660638
# account corresponds to them yet, if any.
661-
_force = should_force_account_creation()
662-
logger.info(
663-
'[THIRD_PARTY_AUTH] ensure_user_information: AUTH_ENTRY_LOGIN should_force_account_creation=%s',
664-
_force,
665-
)
666-
if _force:
639+
if should_force_account_creation():
667640
return dispatch_to_register()
668641
logger.info('[THIRD_PARTY_AUTH] ensure_user_information: dispatching to login (no force create)')
669642
return dispatch_to_login()

common/djangoapps/third_party_auth/provider.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
SAMLConfiguration,
1717
SAMLProviderConfig
1818
)
19-
from common.djangoapps.third_party_auth.toggles import is_saml_provider_site_fallback_enabled
2019

2120

2221
class Registry:
@@ -104,7 +103,7 @@ def get_from_pipeline(cls, running_pipeline):
104103
# won't yield it — but the SAML handshake already completed. Look up the provider
105104
# directly by idp_name so that pipeline steps like should_force_account_creation()
106105
# can still read provider flags.
107-
if is_saml_provider_site_fallback_enabled() and running_pipeline.get('backend') == 'tpa-saml':
106+
if running_pipeline.get('backend') == 'tpa-saml':
108107
try:
109108
idp_name = running_pipeline['kwargs']['response']['idp_name']
110109
return SAMLProviderConfig.current(idp_name)

common/djangoapps/third_party_auth/tests/test_pipeline_integration.py

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,15 @@ def test_complete_url_raises_value_error_if_provider_not_enabled(self):
159159
with pytest.raises(ValueError):
160160
pipeline.get_complete_url(provider_name)
161161

162+
def test_complete_url_does_not_raise_for_tpa_saml_with_no_enabled_providers(self):
163+
# tpa-saml is allowed to proceed even when no providers are visible in the
164+
# site-filtered registry, because the SAML handshake may have completed via
165+
# a site-independent lookup.
166+
with mock.patch.object(provider.Registry, 'get_enabled_by_backend_name', return_value=iter([])):
167+
url = pipeline.get_complete_url('tpa-saml')
168+
assert url.startswith('/auth/complete')
169+
assert 'tpa-saml' in url
170+
162171
def test_complete_url_returns_expected_format(self):
163172
complete_url = pipeline.get_complete_url(self.enabled_provider.backend_name)
164173

@@ -361,22 +370,19 @@ def test_redirect_for_saml_based_on_email_only(self, email, expected_redirect_ur
361370
)
362371
with mock.patch('common.djangoapps.third_party_auth.pipeline.provider.Registry.get_from_pipeline') as get_from_pipeline: # lint-amnesty, pylint: disable=line-too-long
363372
get_from_pipeline.return_value = saml_provider
364-
with mock.patch(
365-
'common.djangoapps.third_party_auth.pipeline.provider.Registry.get_enabled_by_backend_name'
366-
) as enabled_saml_providers:
367-
enabled_saml_providers.return_value = [saml_provider, ] if is_saml else []
368-
with mock.patch('social_core.pipeline.partial.partial_prepare') as partial_prepare:
369-
partial_prepare.return_value = mock.MagicMock(token='')
370-
strategy = mock.MagicMock()
371-
response = pipeline.ensure_user_information(
372-
strategy=strategy,
373-
backend=None,
374-
auth_entry=pipeline.AUTH_ENTRY_LOGIN,
375-
pipeline_index=0,
376-
details={'username': self.user.username, 'email': email}
377-
)
378-
assert response.status_code == 302
379-
assert response.url == expected_redirect_url
373+
with mock.patch('social_core.pipeline.partial.partial_prepare') as partial_prepare:
374+
backend = 'tpa-saml' if is_saml else 'oa2-google-oauth2'
375+
partial_prepare.return_value = mock.MagicMock(token='', backend=backend)
376+
strategy = mock.MagicMock()
377+
response = pipeline.ensure_user_information(
378+
strategy=strategy,
379+
backend=None,
380+
auth_entry=pipeline.AUTH_ENTRY_LOGIN,
381+
pipeline_index=0,
382+
details={'username': self.user.username, 'email': email}
383+
)
384+
assert response.status_code == 302
385+
assert response.url == expected_redirect_url
380386

381387

382388
@ddt.ddt

common/djangoapps/third_party_auth/toggles.py

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -65,33 +65,3 @@ def is_tpa_next_url_on_dispatch_enabled():
6565
when dispatching to login/register pages.
6666
"""
6767
return TPA_NEXT_URL_ON_DISPATCH_FLAG.is_enabled()
68-
69-
70-
# .. toggle_name: third_party_auth.saml_provider_site_fallback
71-
# .. toggle_implementation: WaffleFlag
72-
# .. toggle_default: False
73-
# .. toggle_description: When enabled, Registry.get_from_pipeline() will fall back to a
74-
# site-independent SAMLProviderConfig lookup when the site-filtered registry returns no
75-
# match for a running SAML pipeline. This handles cases where the SAMLProviderConfig or
76-
# SAMLConfiguration is associated with a different Django site than the one currently
77-
# serving the request, while SAML auth itself already completed (SAMLAuthBackend.get_idp()
78-
# has no site check). Without this flag, pipeline steps such as should_force_account_creation()
79-
# cannot read provider flags (e.g. send_to_registration_first), causing new users to land on
80-
# the login page instead of registration.
81-
# .. toggle_use_cases: temporary
82-
# .. toggle_creation_date: 2026-02-19
83-
# .. toggle_target_removal_date: 2026-06-01
84-
# .. toggle_warning: The underlying site configuration mismatch should still be fixed in Django
85-
# admin (SAMLConfiguration and SAMLProviderConfig must reference the correct site). This flag
86-
# is a temporary workaround until that is resolved.
87-
SAML_PROVIDER_SITE_FALLBACK_FLAG = WaffleFlag(
88-
f'{THIRD_PARTY_AUTH_NAMESPACE}.saml_provider_site_fallback', __name__
89-
)
90-
91-
92-
def is_saml_provider_site_fallback_enabled():
93-
"""
94-
Returns True if get_from_pipeline() should fall back to a site-independent
95-
SAMLProviderConfig lookup when the site-filtered registry finds no match.
96-
"""
97-
return SAML_PROVIDER_SITE_FALLBACK_FLAG.is_enabled()

0 commit comments

Comments
 (0)