Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 11 additions & 38 deletions common/djangoapps/third_party_auth/pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,7 @@ def B(*args, **kwargs):
is_saml_provider,
user_exists,
)
from common.djangoapps.third_party_auth.toggles import (
is_saml_provider_site_fallback_enabled,
is_tpa_next_url_on_dispatch_enabled,
)
from common.djangoapps.third_party_auth.toggles import is_tpa_next_url_on_dispatch_enabled
from common.djangoapps.track import segment
from common.djangoapps.util.json_request import JsonResponse

Expand Down Expand Up @@ -363,10 +360,10 @@ def get_complete_url(backend_name):
ValueError: if no provider is enabled with the given backend_name.
"""
if not any(provider.Registry.get_enabled_by_backend_name(backend_name)):
# When the SAML site-fallback flag is on, the provider may not be visible to the
# site-filtered registry even though SAML auth already completed via a
# site-independent lookup. Allow get_complete_url to proceed in that case.
if not (is_saml_provider_site_fallback_enabled() and backend_name == 'tpa-saml'):
# For tpa-saml, the provider may not be visible to the site-filtered registry
# even though SAML auth already completed via a site-independent lookup.
# Allow get_complete_url to proceed in that case.
if backend_name != 'tpa-saml':
raise ValueError('Provider with backend %s not enabled' % backend_name)
Comment thread
jono-booth marked this conversation as resolved.
Resolved

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

if not user:
# Use only email for user existence check in case of saml provider
_is_saml = is_provider_saml()
_provider_obj = provider.Registry.get_from_pipeline({'backend': current_partial.backend, 'kwargs': kwargs})
logger.info(
'[THIRD_PARTY_AUTH] ensure_user_information: auth_entry=%s backend=%s is_provider_saml=%s '
'current_provider=%s skip_email_verification=%s send_to_registration_first=%s '
'email=%s kwargs_response_keys=%s',
auth_entry,
current_partial.backend,
_is_saml,
_provider_obj.provider_id if _provider_obj else None,
_provider_obj.skip_email_verification if _provider_obj else None,
_provider_obj.send_to_registration_first if _provider_obj else None,
details.get('email') if details else None,
list((kwargs.get('response') or {}).keys()),
)
if _is_saml:
# Use only email for user existence check in case of saml provider.
# Check the backend name directly rather than the site-filtered registry,
# since the provider may only be visible via the site-independent fallback.
if current_partial.backend == 'tpa-saml':
Comment thread
jono-booth marked this conversation as resolved.
Resolved
user_details = {'email': details.get('email')} if details else None
else:
user_details = details
_user_exists = user_exists(user_details or {})
logger.info(
'[THIRD_PARTY_AUTH] ensure_user_information: user_exists=%s user_details_email=%s',
_user_exists,
(user_details or {}).get('email'),
)
if _user_exists:
if user_exists(user_details or {}):
# User has not already authenticated and the details sent over from
# identity provider belong to an existing user.
logger.info('[THIRD_PARTY_AUTH] ensure_user_information: dispatching to login (user exists)')
Expand All @@ -658,12 +636,7 @@ def is_provider_saml():
elif auth_entry == AUTH_ENTRY_LOGIN:
# User has authenticated with the third party provider but we don't know which edX
# account corresponds to them yet, if any.
_force = should_force_account_creation()
logger.info(
'[THIRD_PARTY_AUTH] ensure_user_information: AUTH_ENTRY_LOGIN should_force_account_creation=%s',
_force,
)
if _force:
if should_force_account_creation():
return dispatch_to_register()
logger.info('[THIRD_PARTY_AUTH] ensure_user_information: dispatching to login (no force create)')
return dispatch_to_login()
Expand Down
3 changes: 1 addition & 2 deletions common/djangoapps/third_party_auth/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
SAMLConfiguration,
SAMLProviderConfig
)
from common.djangoapps.third_party_auth.toggles import is_saml_provider_site_fallback_enabled


class Registry:
Expand Down Expand Up @@ -104,7 +103,7 @@ def get_from_pipeline(cls, running_pipeline):
# won't yield it — but the SAML handshake already completed. Look up the provider
# directly by idp_name so that pipeline steps like should_force_account_creation()
# can still read provider flags.
if is_saml_provider_site_fallback_enabled() and running_pipeline.get('backend') == 'tpa-saml':
if running_pipeline.get('backend') == 'tpa-saml':
try:
idp_name = running_pipeline['kwargs']['response']['idp_name']
return SAMLProviderConfig.current(idp_name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,15 @@ def test_complete_url_raises_value_error_if_provider_not_enabled(self):
with pytest.raises(ValueError):
pipeline.get_complete_url(provider_name)

def test_complete_url_does_not_raise_for_tpa_saml_with_no_enabled_providers(self):
# tpa-saml is allowed to proceed even when no providers are visible in the
# site-filtered registry, because the SAML handshake may have completed via
# a site-independent lookup.
with mock.patch.object(provider.Registry, 'get_enabled_by_backend_name', return_value=iter([])):
url = pipeline.get_complete_url('tpa-saml')
assert url.startswith('/auth/complete')
assert 'tpa-saml' in url

def test_complete_url_returns_expected_format(self):
complete_url = pipeline.get_complete_url(self.enabled_provider.backend_name)

Expand Down Expand Up @@ -361,22 +370,19 @@ def test_redirect_for_saml_based_on_email_only(self, email, expected_redirect_ur
)
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
get_from_pipeline.return_value = saml_provider
with mock.patch(
'common.djangoapps.third_party_auth.pipeline.provider.Registry.get_enabled_by_backend_name'
) as enabled_saml_providers:
enabled_saml_providers.return_value = [saml_provider, ] if is_saml else []
with mock.patch('social_core.pipeline.partial.partial_prepare') as partial_prepare:
partial_prepare.return_value = mock.MagicMock(token='')
strategy = mock.MagicMock()
response = pipeline.ensure_user_information(
strategy=strategy,
backend=None,
auth_entry=pipeline.AUTH_ENTRY_LOGIN,
pipeline_index=0,
details={'username': self.user.username, 'email': email}
)
assert response.status_code == 302
assert response.url == expected_redirect_url
with mock.patch('social_core.pipeline.partial.partial_prepare') as partial_prepare:
backend = 'tpa-saml' if is_saml else 'oa2-google-oauth2'
partial_prepare.return_value = mock.MagicMock(token='', backend=backend)
strategy = mock.MagicMock()
response = pipeline.ensure_user_information(
strategy=strategy,
backend=None,
auth_entry=pipeline.AUTH_ENTRY_LOGIN,
pipeline_index=0,
details={'username': self.user.username, 'email': email}
)
Comment thread
jono-booth marked this conversation as resolved.
Resolved
assert response.status_code == 302
assert response.url == expected_redirect_url


@ddt.ddt
Expand Down
30 changes: 0 additions & 30 deletions common/djangoapps/third_party_auth/toggles.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,33 +65,3 @@ def is_tpa_next_url_on_dispatch_enabled():
when dispatching to login/register pages.
"""
return TPA_NEXT_URL_ON_DISPATCH_FLAG.is_enabled()


# .. toggle_name: third_party_auth.saml_provider_site_fallback
# .. toggle_implementation: WaffleFlag
# .. toggle_default: False
# .. toggle_description: When enabled, Registry.get_from_pipeline() will fall back to a
# site-independent SAMLProviderConfig lookup when the site-filtered registry returns no
# match for a running SAML pipeline. This handles cases where the SAMLProviderConfig or
# SAMLConfiguration is associated with a different Django site than the one currently
# serving the request, while SAML auth itself already completed (SAMLAuthBackend.get_idp()
# has no site check). Without this flag, pipeline steps such as should_force_account_creation()
# cannot read provider flags (e.g. send_to_registration_first), causing new users to land on
# the login page instead of registration.
# .. toggle_use_cases: temporary
# .. toggle_creation_date: 2026-02-19
# .. toggle_target_removal_date: 2026-06-01
# .. toggle_warning: The underlying site configuration mismatch should still be fixed in Django
# admin (SAMLConfiguration and SAMLProviderConfig must reference the correct site). This flag
# is a temporary workaround until that is resolved.
SAML_PROVIDER_SITE_FALLBACK_FLAG = WaffleFlag(
f'{THIRD_PARTY_AUTH_NAMESPACE}.saml_provider_site_fallback', __name__
)


def is_saml_provider_site_fallback_enabled():
"""
Returns True if get_from_pipeline() should fall back to a site-independent
SAMLProviderConfig lookup when the site-filtered registry finds no match.
"""
return SAML_PROVIDER_SITE_FALLBACK_FLAG.is_enabled()
15 changes: 12 additions & 3 deletions openedx/core/djangoapps/user_authn/views/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -577,10 +577,13 @@ def post(self, request):
HttpResponse: 403 operation not allowed
"""
should_be_rate_limited = getattr(request, 'limited', False)
if should_be_rate_limited:
pipeline_data = pipeline.get(request)
pipeline_running = pipeline_data is not None
pipeline_backend = pipeline_data.get('backend') if pipeline_data else None
Comment thread
jono-booth marked this conversation as resolved.
Resolved
if should_be_rate_limited and (not pipeline_running or pipeline_backend != 'tpa-saml'):
Comment on lines +580 to +583

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this would read better and be more self-documenting if refactored:

Suggested change
pipeline_data = pipeline.get(request)
pipeline_running = pipeline_data is not None
pipeline_backend = pipeline_data.get('backend') if pipeline_data else None
if should_be_rate_limited and (not pipeline_running or pipeline_backend != 'tpa-saml'):
pipeline_data = pipeline.get(request)
pipeline_running = pipeline_data is not None
saml_pipeline_running = pipeline_running and pipeline_data.get('backend') == 'tpa-saml'
# Bypass rate limit when a SAML pipeline is running.
if should_be_rate_limited and not saml_pipeline_running:

return JsonResponse({'error_code': 'forbidden-request'}, status=403)

if is_require_third_party_auth_enabled() and not pipeline.running(request):
if is_require_third_party_auth_enabled() and not pipeline_running:
# if request is not running a third-party auth pipeline
return HttpResponseForbidden(
"Third party authentication is required to register. Username and password were received instead."
Expand Down Expand Up @@ -875,7 +878,7 @@ def country_handler(self, request):
}

@method_decorator(
ratelimit(key=REAL_IP_KEY, rate=settings.REGISTRATION_VALIDATION_RATELIMIT, method='POST', block=True)
ratelimit(key=REAL_IP_KEY, rate=settings.REGISTRATION_VALIDATION_RATELIMIT, method='POST', block=False)
)
def post(self, request):
"""
Expand All @@ -897,6 +900,12 @@ def post(self, request):
can get extra verification checks if entered along with others,
like when the password may not equal the username.
"""
pipeline_data = pipeline.get(request)
if getattr(request, 'limited', False) and not (
pipeline_data and pipeline_data.get('backend') == 'tpa-saml'
):
return Response(status=403)

field_key = request.data.get('form_field_key')
validation_decisions = {}

Expand Down
64 changes: 64 additions & 0 deletions openedx/core/djangoapps/user_authn/views/tests/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -2068,6 +2068,40 @@ def _assert_social_auth_provider_present(self, field_settings, backend):
"defaultValue": backend.name
})

@mock.patch('django_ratelimit.decorators.is_ratelimited', return_value=True)
def test_rate_limiting_exempted_for_saml_pipeline(self, _mock_ratelimited):
"""
Confirm that a registration POST with an active SAML/TPA pipeline is not
blocked by IP-based rate limiting, even after the limit is exhausted.

The decorator rate is baked in at import time so @override_settings cannot
change it. Instead we mock is_ratelimited to always return True, which is
equivalent to every request arriving after the limit is exhausted.
"""
# With the limit exhausted and no SAML pipeline, the request is blocked
response = self.client.post(self.url, {
"email": self.EMAIL,
"name": self.NAME,
"username": self.USERNAME,
"password": self.PASSWORD,
"honor_code": "true",
})
assert response.status_code == 403
assert response.json().get('error_code') == 'forbidden-request'

# With an active SAML/TPA pipeline, the same IP is not blocked by rate limiting
with simulate_running_pipeline(
'openedx.core.djangoapps.user_authn.views.register.pipeline', 'tpa-saml'
):
response = self.client.post(self.url, {
"email": self.EMAIL,
"name": self.NAME,
"username": self.USERNAME,
"password": self.PASSWORD,
"honor_code": "true",
})
assert response.json().get('error_code') != 'forbidden-request'


@ddt.ddt
class RegistrationViewTestV2(RegistrationViewTestV1):
Expand Down Expand Up @@ -3028,6 +3062,36 @@ def test_rate_limiting_registration_view(self):
response = self.request_without_auth('post', self.path)
assert response.status_code == 403

@override_settings(
CACHES={
'default': {
'BACKEND': 'django.core.cache.backends.locmem.LocMemCache',
'LOCATION': 'validation_ratelimit_tpa',
}
}
)
def test_rate_limiting_exempted_for_saml_pipeline(self):
"""
Confirm that validation requests with an active SAML/TPA pipeline are not
blocked by IP-based rate limiting, even after the limit is exhausted.
"""
# Exhaust the real rate limit. The rate is baked into the decorator at import
# time, so @override_settings cannot change it — we must read the live value.
for _ in range(int(settings.REGISTRATION_VALIDATION_RATELIMIT.split('/')[0])):
response = self.request_without_auth('post', self.path)
assert response.status_code != 403
# Without a pipeline, the next request from the same IP is rate limited
response = self.request_without_auth('post', self.path)
assert response.status_code == 403

# With an active SAML/TPA pipeline, the same IP is not blocked by rate limiting
with simulate_running_pipeline(
'openedx.core.djangoapps.user_authn.views.register.pipeline', 'tpa-saml'
):
Comment thread
jono-booth marked this conversation as resolved.
Resolved
response = self.request_without_auth('post', self.path)
self.assertHttpOK(response)
assert response.json().get('validation_decisions') == {}

Comment thread
jono-booth marked this conversation as resolved.
Resolved
def test_single_field_validation(self):
"""
Test that if `is_authn_mfe` is provided in request along with form_field_key, only
Expand Down
Loading