Skip to content

Fixes #22346: Return a clean error message & redirect on SSO auth failure#22420

Open
jeremystretch wants to merge 1 commit into
mainfrom
22346-sso-failure-error-message
Open

Fixes #22346: Return a clean error message & redirect on SSO auth failure#22420
jeremystretch wants to merge 1 commit into
mainfrom
22346-sso-failure-error-message

Conversation

@jeremystretch

Copy link
Copy Markdown
Member

Closes: #22346

  • Introduce & employ SocialAuthExceptionMiddleware to return a clean error message in the event of an SSO authentication failure
  • Set SOCIAL_AUTH_LOGIN_ERROR_URL and SOCIAL_AUTH_RAISE_EXCEPTIONS in `settings.py
  • Add tests

@jeremystretch

Copy link
Copy Markdown
Member Author

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jeremystretch's task in 3m 6s —— View job


Review

Two issues worth considering:


1. SOCIAL_AUTH_LOGIN_ERROR_URL = 'login' may produce a broken redirect

settings.py, line 732:

SOCIAL_AUTH_LOGIN_ERROR_URL = 'login'

The parent SocialAuthExceptionMiddleware.get_redirect_uri() returns this value via strategy.setting('LOGIN_ERROR_URL') and passes it directly to strategy.redirect(url), which in DjangoStrategy is just HttpResponseRedirect(url). No URL reversal occurs. Passing a URL name like 'login' (without a leading slash) produces a relative redirect — so if the SSO callback fires at /complete/saml/, the browser would follow the redirect to /complete/login (which 404s) rather than /login/.

The setting should likely be /login/ (an absolute path) rather than the URL name 'login'.

Fix this →


2. Test mock doesn't exercise the real URL resolution

test_authentication.py, FakeStrategy.setting():

'LOGIN_ERROR_URL': '/login/',  # absolute path

vs the actual setting:

SOCIAL_AUTH_LOGIN_ERROR_URL = 'login'  # URL name

test_redirect_on_failure asserts response.url == '/login/' using the mock strategy's value, not the real DjangoStrategy. If the redirect URL in settings is wrong (see above), this test would still pass. The mock should match the real setting value so the test catches a misconfiguration.


@jeremystretch

Copy link
Copy Markdown
Member Author
  1. Set SOCIAL_AUTH_LOGIN_ERROR_URL to LOGIN_URL (the path does get reversed).
  2. The test mock now reads the real SOCIAL_AUTH_* settings so it'll catch a misconfigured URL.

@jeremystretch jeremystretch force-pushed the 22346-sso-failure-error-message branch from 4fecca2 to 19f0312 Compare June 9, 2026 19:39
@jeremystretch jeremystretch marked this pull request as ready for review June 9, 2026 19:39
@jeremystretch jeremystretch requested review from a team and jnovinger and removed request for a team June 9, 2026 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Render SSO/SAML authentication failures as a login-page message instead of an HTTP 500 "Server Error"

1 participant