Update openid-connect-bundle to 6.1 - #92
Conversation
API Specification - Non-breaking changesNo changelog changes |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #92 +/- ##
=============================================
+ Coverage 37.14% 38.46% +1.31%
- Complexity 948 1119 +171
=============================================
Files 133 148 +15
Lines 2972 3541 +569
=============================================
+ Hits 1104 1362 +258
- Misses 1868 2179 +311
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nect-bundle-6.0 # Conflicts: # CHANGELOG.md # composer.lock # config/reference.php
…nect-bundle-6.0 # Conflicts: # CHANGELOG.md # composer.lock
|
Related: #97 replaces the Raised separately since it is a development-environment change rather than part of the 6.0 upgrade, but it is worth knowing about while reviewing this one: with the bypass in place, none of the callback handling this PR changes — the failure page, Either can merge first. |
The bundle now distinguishes a provider that refuses the authorization request — a cancelled login screen, an expired session at Azure, a tenant policy — from a callback that fails validation, and states the status the answer should have. Two changes here to make use of that: AzureOIDCAuthenticator chains the cause when it re-raises as Symfony's AuthenticationException. Without `previous` the bundle cannot tell the two apart and every failure arrives as a 500. OpenIdConnectFailureListener answers with the status the exception asks for — 403 where the user or a policy declined, 503 where Azure reports its own trouble, 500 otherwise — and the page says the login was declined rather than that something went wrong. It still repeats nothing the provider sent. PKCE comes with the bundle and needs no configuration. itk-dev/openid-connect moves to 5.1, which drops robrichards/xmlseclibs.
turegjorup
changed the title
2 participants
Link to ticket
TODO: no ticket linked yet — please add before review.
Description
Updates
itk-dev/openid-connect-bundlefrom 5.0 to 6.1 and adapts the application to it.What 6.0 changed. A failed callback used to be caught by the firewall, which handed control back to the entry point and redirected to Azure again — the loop that kept the site down for as long as the client secret was expired. It now escapes the firewall as
AuthenticationFailedExceptionand stops instead of retrying.OpenIdConnectFailureListenerturns that into a page saying the login could not be completed, with a link to try again. The provider's own error text stays in the log; the page repeats none of it.The other 6.0 breaking changes need nothing here. Every provider must declare where its callback arrives, which
azure_azalready does viaredirect_uri, and only that path counts as a callback —/openid-connect/generic, matching the app's own route, so nocallback_path. The removedItkOpenIdConnectBundleExceptionandUserDoesNotExistExceptionwere not in use.What 6.1 adds. The bundle now tells a provider that refuses the request — a cancelled login screen, an expired session at Azure, a tenant policy — from a callback that fails validation, and states the status the answer should have. Two changes here to use that:
AzureOIDCAuthenticatorchains the cause when it re-raises as Symfony'sAuthenticationException. Withoutpreviousthe bundle cannot tell the two apart and every failure arrives as a 500.OpenIdConnectFailureListeneranswers with the status the exception asks for — 403 where the user or a policy declined, 503 where Azure reports its own trouble, 500 otherwise — and the page says the login was declined rather than that something went wrong.A user who clicks Cancel now gets a page telling them so, and nobody is paged for it.
PKCE (RFC 7636, S256) comes with the bundle and needs no configuration; it was verified against the real Azure AD B2C tenant on this branch before 6.1 was tagged.
itk-dev/openid-connectmoves to 5.1, which dropsrobrichards/xmlseclibs.Also carried over from the 5.1 work on this branch:
client_secret_expires_atforazure_az, so the bundle warns before a client secret expires rather than after. Monitoring sees it too — anoidc_client_secretcheck joins the database, transport and ingest-freshness ones on/health/detail. It reports degraded only once the date has passed, since a readiness endpoint that goes red 30 days early is one nobody believes by the time it matters, sodays_remainingin the payload is what to watch before then. The configured date stays an indicator rather than an authority: rotate a secret without updating it and the check reports expired while logins succeed, which is a false degraded answer we accept, because either way someone has to look.Screenshot of the result
Screenshot to follow — two pages now: the existing "We could not complete your login", and a declined variant for a cancelled login.
Checklist
64 tests, 148 assertions. Three are new, on
OpenIdConnectFailureListenerTest: a refusal answers 403 with noLocationheader, the page says the login was declined and repeats neither the error code nor the provider's description, and a refusal carrying a forged state is answered as an ordinary failure with none of the sender's text on the page.Additional comments or questions
Deployment needs a new variable.
AZURE_AZ_OIDC_CLIENT_SECRET_EXPIRES_AThas to be set wherever the real secret lives, or the provider reportsunknown, the health check reportsskipped, and nothing is watching the secret..envcarries a placeholder date, which is deliberately not the answer.Ordinary failures still answer 500, and a stale browser tab gets one, which is the cost of not being able to tell that apart from a real fault at that point. Only a refusal the provider names gets a 4xx.
.env.testsetsAZURE_AZ_OIDC_REDIRECT_URI. Without it the test environment derives its callback path from the placeholder in.env, which is not a route, so the router answers 404 before the authenticator is asked and the tests would prove nothing.Merged
develop, which brings the local mock identity provider from #97. Local development now exercises this code rather than running with the firewall switched off, so the failure and refusal pages can both be seen without deploying.