Skip to content
Open
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
63 changes: 62 additions & 1 deletion common/djangoapps/third_party_auth/saml.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""

import logging
from urllib.parse import unquote
from copy import deepcopy

import requests
Expand All @@ -15,6 +16,7 @@
from social_core.backends.saml import OID_EDU_PERSON_ENTITLEMENT, SAMLAuth, SAMLIdentityProvider
from social_core.exceptions import AuthForbidden, AuthInvalidParameter, AuthMissingParameter

from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect
from common.djangoapps.third_party_auth.exceptions import IncorrectConfigurationException
from openedx.core.djangoapps.theming.helpers import get_current_request

Expand Down Expand Up @@ -88,12 +90,71 @@ def auth_complete(self, *args, **kwargs):
"""
Handle exceptions that happen during SAML authentication
"""
# For IdP-initiated flows (where the user doesn't first hit /auth/login/...),
# allow callers to provide a post-auth redirect by packing it into RelayState.
# Store it in the session so the rest of the pipeline behaves consistently.
try:
request = get_current_request()
# Allow RelayState to carry both IdP slug and a post-auth destination.
# Format: "<idp_slug>|<next>", where <next> is typically a relative LMS path.
self._maybe_set_next_url_from_relay_state(request)
except Exception: # pylint: disable=broad-exception-caught # pragma: no cover
# Never fail auth due to redirect bookkeeping.
pass

try:
return super().auth_complete(*args, **kwargs)
# We are seeing errors of MultiValueDictKeyError looking for the parameter 'RelayState'.
# We would like to have a more specific error to handle for observability purposes.
except MultiValueDictKeyError as e:
raise AuthMissingParameter(self.name, e.args[0]) from e
raise AuthMissingParameter(self.name, e.args[0] if e.args else '') from e

@staticmethod
def _maybe_set_next_url_from_relay_state(request):
"""Optionally extract a safe `next` from RelayState and rewrite RelayState to the IdP slug.

This is specifically to support IdP-initiated flows where Auth0 (and some IdPs) can only
reliably influence the SAML POST via RelayState.
"""
if request is None or not hasattr(request, 'POST'):
return
if not hasattr(request, 'session'):
return

relay_state = None
try:
relay_state = request.POST.get('RelayState')
except Exception: # pylint: disable=broad-exception-caught # pragma: no cover
relay_state = None

if not relay_state or '|' not in str(relay_state):
return

slug_part, next_part = str(relay_state).split('|', 1)
slug_part = slug_part.strip()
next_part = next_part.strip()
if not slug_part or not next_part:
return

# URL-decode next (Auth0 or callers may URL-encode it).
next_decoded = unquote(next_part)

# Only store next if it's safe per existing Open edX redirect policy.
if is_safe_login_or_logout_redirect(
redirect_to=next_decoded,
request_host=request.get_host(),
dot_client_id=(request.GET.get('client_id') if hasattr(request, 'GET') else None),
require_https=request.is_secure(),
):
request.session['next'] = next_decoded
else:
# RelayState included an unsafe destination; clear any stale 'next' value
request.session.pop('next', None)
Comment on lines +143 to +152

# Always rewrite RelayState to just the IdP slug so the SAML backend can locate the provider.
post_copy = request.POST.copy()
post_copy['RelayState'] = slug_part
request._post = post_copy # pylint: disable=protected-access

def get_user_id(self, details, response):
"""
Expand Down
56 changes: 56 additions & 0 deletions common/djangoapps/third_party_auth/tests/test_saml.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@

from unittest import mock

from django.test import RequestFactory
from django.utils.datastructures import MultiValueDictKeyError
from django.contrib.sessions.middleware import SessionMiddleware
from social_core.exceptions import AuthMissingParameter

from common.djangoapps.third_party_auth.dummy import DummyBackend
Expand Down Expand Up @@ -41,6 +43,14 @@ def test_get_user_details(self):
class TestSAMLAuthBackend(SAMLTestCase):
""" Tests for the SAML backend. """

@staticmethod
def _add_session(request):
"""Attach a Django session to a RequestFactory request."""
middleware = SessionMiddleware(lambda req: None)
middleware.process_request(request)
request.session.save()
return request

@mock.patch('common.djangoapps.third_party_auth.saml.SAMLAuth.auth_complete')
def test_saml_auth_complete(self, super_auth_complete):
super_auth_complete.side_effect = MultiValueDictKeyError('RelayState')
Expand All @@ -49,3 +59,49 @@ def test_saml_auth_complete(self, super_auth_complete):
backend.auth_complete()

assert cm.exception.parameter == 'RelayState'

@mock.patch('common.djangoapps.third_party_auth.saml.get_current_request')
@mock.patch('common.djangoapps.third_party_auth.saml.SAMLAuth.auth_complete')
def test_relaystate_splits_and_sets_next_when_safe(self, super_auth_complete, get_current_request_mock):
"""RelayState may include both the IdP slug and a safe `next` destination."""
rf = RequestFactory()
request = rf.post(
'/auth/complete/tpa-saml/',
data={
'SAMLResponse': 'ignored',
'RelayState': 'example-idp|/courses/course-v1:edX+DemoX+Demo_Course/course/',
},
HTTP_HOST=self.hostname,
)
self._add_session(request)
get_current_request_mock.return_value = request

super_auth_complete.return_value = 'ok'
backend = SAMLAuthBackend()
assert backend.auth_complete() == 'ok'

assert request.POST.get('RelayState') == 'example-idp'
assert request.session.get('next') == '/courses/course-v1:edX+DemoX+Demo_Course/course/'

@mock.patch('common.djangoapps.third_party_auth.saml.get_current_request')
@mock.patch('common.djangoapps.third_party_auth.saml.SAMLAuth.auth_complete')
def test_relaystate_drops_unsafe_next(self, super_auth_complete, get_current_request_mock):
"""If RelayState contains an unsafe `next`, it is ignored but the slug is preserved."""
rf = RequestFactory()
request = rf.post(
'/auth/complete/tpa-saml/',
data={
'SAMLResponse': 'ignored',
'RelayState': 'example-idp|https%3A%2F%2Fevil.example.com%2Fpwn',
},
HTTP_HOST=self.hostname,
)
self._add_session(request)
get_current_request_mock.return_value = request

super_auth_complete.return_value = 'ok'
backend = SAMLAuthBackend()
assert backend.auth_complete() == 'ok'

assert request.POST.get('RelayState') == 'example-idp'
assert request.session.get('next') is None