From 2d9fe6b3a2228fde1edc57c4795096a068d11be4 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 21:29:20 +0200 Subject: [PATCH 1/5] chore: update documentation link --- templates/post_logout/index.html.twig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/post_logout/index.html.twig b/templates/post_logout/index.html.twig index 025585e4..2adcb81b 100644 --- a/templates/post_logout/index.html.twig +++ b/templates/post_logout/index.html.twig @@ -101,7 +101,7 @@
- ITK Development Docs + ITK Development Docs

Developer and PM tips & tricks

From 494535880bb4a71377dde6675cc97a8c8f1daef6 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 21:32:11 +0200 Subject: [PATCH 2/5] chore: update openid-connect-bundle to 6.0 --- .env | 2 ++ composer.json | 2 +- composer.lock | 18 +++++++++++------- config/packages/itkdev_openid_connect.yaml | 3 +++ config/reference.php | 15 ++++++++++++++- 5 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.env b/.env index 41d88877..a5fceb90 100644 --- a/.env +++ b/.env @@ -50,6 +50,8 @@ MESSENGER_TRANSPORT_DSN_FAILED='doctrine://default?queue_name=failed' AZURE_AZ_OIDC_METADATA_URL=AZURE_AZ_APP_METADATA_URL AZURE_AZ_OIDC_CLIENT_ID=AZURE_AZ_APP_CLIENT_ID AZURE_AZ_OIDC_CLIENT_SECRET=AZURE_AZ_APP_CLIENT_SECRET +# Date the Azure client secret expires (any strtotime-parseable value) +AZURE_AZ_OIDC_CLIENT_SECRET_EXPIRES_AT=2027-01-31 AZURE_AZ_OIDC_REDIRECT_URI=AZURE_AZ_APP_REDIRECT_URI AZURE_AZ_OIDC_ALLOW_HTTP=false AZURE_AZ_OIDC_LEEWAY=10 diff --git a/composer.json b/composer.json index 252f8530..c6b2dc4f 100644 --- a/composer.json +++ b/composer.json @@ -14,7 +14,7 @@ "doctrine/doctrine-migrations-bundle": "^4.0", "doctrine/orm": "^3.0", "easycorp/easyadmin-bundle": "^5.0", - "itk-dev/openid-connect-bundle": "^5.0", + "itk-dev/openid-connect-bundle": "^6.0", "itk-dev/vault-bundle": "^1.0.0", "nelmio/cors-bundle": "^2.2", "ocramius/doctrine-batch-utils": "^2.8", diff --git a/composer.lock b/composer.lock index cbd47e74..c1087136 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "ceb76d3549aace47bff7b8e9260728fe", + "content-hash": "18003b642aa58da510f8036137cbdfee", "packages": [ { "name": "api-platform/core", @@ -1985,16 +1985,16 @@ }, { "name": "itk-dev/openid-connect-bundle", - "version": "5.0.0", + "version": "6.0.0", "source": { "type": "git", "url": "https://github.com/itk-dev/openid-connect-bundle.git", - "reference": "af57823b41629ad5bc1abc50eeb388c7b4b6dbb4" + "reference": "79be9eb80767e6c0f7945de8cee9bbfe2a4b5f3d" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/itk-dev/openid-connect-bundle/zipball/af57823b41629ad5bc1abc50eeb388c7b4b6dbb4", - "reference": "af57823b41629ad5bc1abc50eeb388c7b4b6dbb4", + "url": "https://api.github.com/repos/itk-dev/openid-connect-bundle/zipball/79be9eb80767e6c0f7945de8cee9bbfe2a4b5f3d", + "reference": "79be9eb80767e6c0f7945de8cee9bbfe2a4b5f3d", "shasum": "" }, "require": { @@ -2003,7 +2003,10 @@ "ext-openssl": "*", "itk-dev/openid-connect": "^5.0", "php": "^8.3", + "psr/log": "^3.0", "symfony/cache": "^6.4 || ^7.0 || ^8.0", + "symfony/clock": "^6.4 || ^7.0 || ^8.0", + "symfony/event-dispatcher": "^6.4 || ^7.0 || ^8.0", "symfony/framework-bundle": "^6.4.13 || ^7.0 || ^8.0", "symfony/security-bundle": "^6.4.13 || ^7.0 || ^8.0", "symfony/uid": "^6.4 || ^7.0 || ^8.0", @@ -2012,6 +2015,7 @@ "require-dev": { "ergebnis/composer-normalize": "^2.28", "friendsofphp/php-cs-fixer": "^3.11", + "infection/infection": "*", "phpstan/phpstan": "^2.1.41", "phpstan/phpstan-deprecation-rules": "^2.0", "phpstan/phpstan-phpunit": "^2.0", @@ -2044,9 +2048,9 @@ "description": "Symfony bundle for openid-connect", "support": { "issues": "https://github.com/itk-dev/openid-connect-bundle/issues", - "source": "https://github.com/itk-dev/openid-connect-bundle/tree/5.0.0" + "source": "https://github.com/itk-dev/openid-connect-bundle/tree/6.0.0" }, - "time": "2026-06-02T11:15:20+00:00" + "time": "2026-08-25T19:09:29+00:00" }, { "name": "itk-dev/vault", diff --git a/config/packages/itkdev_openid_connect.yaml b/config/packages/itkdev_openid_connect.yaml index 854a6d3e..61943447 100644 --- a/config/packages/itkdev_openid_connect.yaml +++ b/config/packages/itkdev_openid_connect.yaml @@ -15,6 +15,9 @@ itkdev_openid_connect: metadata_url: '%env(string:AZURE_AZ_OIDC_METADATA_URL)%' client_id: '%env(AZURE_AZ_OIDC_CLIENT_ID)%' client_secret: '%env(AZURE_AZ_OIDC_CLIENT_SECRET)%' + # Date the client secret expires. Lets the bundle warn before it + # breaks every login, as it did on 2026-08-12. + client_secret_expires_at: '%env(string:AZURE_AZ_OIDC_CLIENT_SECRET_EXPIRES_AT)%' # Specify redirect URI redirect_uri: '%env(string:AZURE_AZ_OIDC_REDIRECT_URI)%' allow_http: '%env(bool:AZURE_AZ_OIDC_ALLOW_HTTP)%' diff --git a/config/reference.php b/config/reference.php index 0367b0fe..74538485 100644 --- a/config/reference.php +++ b/config/reference.php @@ -1741,19 +1741,32 @@ * route?: scalar|Param|null, // Return route for CLI login * }, * user_provider?: scalar|Param|null, // The User Provider to inject // Default: null + * logging_options?: array{ + * logger?: scalar|Param|null, // Service id of the PSR-3 logger to receive this bundle's failure logs, e.g. "monolog.logger.openid_connect". Defaults to the application logger, which Symfony always provides. Set "itkdev_openid_connect.null_logger" to turn logging off. // Default: null + * }, + * audit_options?: array{ + * enabled?: bool|Param, // Write an authentication audit trail (logins, failures, CLI token issuance). Off by default: audit records identify people, so an existing installation must opt in rather than start logging personal data on upgrade. // Default: false + * logger?: scalar|Param|null, // Service id of the PSR-3 logger to receive audit records, e.g. "monolog.logger.openid_connect_audit". Defaults to the application logger. Keep this separate from logging_options.logger: an operational threshold of "error" would otherwise discard the whole trail. // Default: null + * identifier?: "raw"|"hashed"|Param, // Record user identifiers as-is ("raw") or pseudonymised ("hashed"). Hashing is keyed with the application secret, so records still correlate. Cannot come from an environment variable; use environment-specific configuration instead. // Default: "raw" + * }, + * secret_expiry_options?: array{ + * warning_days?: int|Param, // How many days before a client secret expires the bundle starts warning (default: 30) // Default: 30 + * }, * openid_providers?: list, + * callback_path?: scalar|Param|null, // Optional. The request path the callback arrives on, for a proxy that rewrites it without sending X-Forwarded-Prefix. Include any base path. Defaults to the path of redirect_uri, or of the generated redirect_route; a trusted X-Forwarded-Prefix or a subdirectory deployment is already accounted for without this. * allow_http?: bool|Param, // Whether to allow http or not (default: false) // Default: false * http_client_options?: array{ // Options forwarded to the underlying Guzzle HTTP client. league/oauth2-client only forwards: timeout, proxy, verify (verify is only consulted when proxy is set). - * timeout?: float|Param, // Total request timeout in seconds + * timeout?: float|Param, // Total request timeout in seconds. Defaults to 30; set to 0 to wait indefinitely (Guzzle's own default). // Default: 30.0 * proxy?: scalar|Param|null, // HTTP proxy URI * verify?: bool|Param, // Verify TLS certificates (only consulted by Guzzle when proxy is set) * }, From 0b764183dd45e4de49de2e07b772a9fa2deb6de5 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 21:32:11 +0200 Subject: [PATCH 3/5] feat: render a failed OIDC login as a page --- .env.test | 5 ++ CHANGELOG.md | 12 ++++ .../OpenIdConnectFailureListener.php | 64 ++++++++++++++++++ .../error/openid_connect_failed.html.twig | 67 +++++++++++++++++++ .../OpenIdConnectFailureListenerTest.php | 48 +++++++++++++ 5 files changed, 196 insertions(+) create mode 100644 src/EventListener/OpenIdConnectFailureListener.php create mode 100644 templates/error/openid_connect_failed.html.twig create mode 100644 tests/EventListener/OpenIdConnectFailureListenerTest.php diff --git a/.env.test b/.env.test index 022bf682..af091794 100644 --- a/.env.test +++ b/.env.test @@ -4,3 +4,8 @@ APP_SECRET='$ecretf0rt3st' SYMFONY_DEPRECATIONS_HELPER=999999 PANTHER_APP_ENV=panther PANTHER_ERROR_SCREENSHOT_DIR=./var/error-screenshots + +# The path of this URI is the only path the OIDC authenticator treats as a +# callback (openid-connect-bundle 6.0), so it has to be the app's own callback +# route for a callback to reach the authenticator at all. +AZURE_AZ_OIDC_REDIRECT_URI=https://itksites.example.org/openid-connect/generic diff --git a/CHANGELOG.md b/CHANGELOG.md index 71f41cfd..e0409c3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- [#92](https://github.com/itk-dev/devops_itksites/pull/92) Update openid-connect-bundle to 6.0 + - Bump `itk-dev/openid-connect-bundle` to `^6.0` + - A failed OIDC callback now raises an error instead of redirecting to the + identity provider again, so an expired client secret can no longer put the + site in a login loop + - Only the provider's callback path is treated as a callback; the configured + `redirect_uri` covers this, no `callback_path` needed + - Set `client_secret_expires_at` for the `azure_az` provider from the new + `AZURE_AZ_OIDC_CLIENT_SECRET_EXPIRES_AT` variable, so the bundle warns + before the secret expires + - Render a failed login as a page saying so, instead of an unhandled + exception, in `OpenIdConnectFailureListener` - [#91](https://github.com/itk-dev/devops_itksites/pull/91) Health endpoints - Add `/health/live`, `/health/ready` and `/health/detail` endpoints - Add health checks for database, RabbitMQ transport and detection result freshness diff --git a/src/EventListener/OpenIdConnectFailureListener.php b/src/EventListener/OpenIdConnectFailureListener.php new file mode 100644 index 00000000..6a9dd3c8 --- /dev/null +++ b/src/EventListener/OpenIdConnectFailureListener.php @@ -0,0 +1,64 @@ +isMainRequest() || !$event->getThrowable() instanceof AuthenticationFailedException) { + return; + } + + // Deliberately without the exception message: it carries the identity + // provider's own error text, which belongs in the log, not in a browser. + $content = $this->twig->render('error/openid_connect_failed.html.twig', [ + 'login_url' => $this->urlGenerator->generate('itkdev_openid_connect_login', ['providerKey' => 'azure_az']), + ]); + + // Still a 500: the most likely cause is on this side of the login, and a + // failed login should read as an error in the log and in monitoring. + $response = new Response($content, Response::HTTP_INTERNAL_SERVER_ERROR); + $response->headers->set('Cache-Control', 'no-store, private'); + + $event->setResponse($response); + } +} diff --git a/templates/error/openid_connect_failed.html.twig b/templates/error/openid_connect_failed.html.twig new file mode 100644 index 00000000..e95fb0c1 --- /dev/null +++ b/templates/error/openid_connect_failed.html.twig @@ -0,0 +1,67 @@ +{# + Shown by App\EventListener\OpenIdConnectFailureListener when an OIDC login + could not be completed. + + Self-contained on purpose: an error page should not depend on the asset + build, and "Try again" is a link the user clicks rather than a redirect — + the bundle stopped redirecting to the identity provider precisely to end + the loop a broken login used to cause. +#} + + + + + + + + Login failed - ITKsites + + + + + +
+

ITKsites

+ +

+ We could not complete your login. +

+ +

+ This happens if the login was left half-finished for too long — in that + case try again. +

+ +

+ If it keeps failing, the login setup itself needs attention: tell ITK Dev, + and mention the time you tried. The details are in the log. +

+
+ + diff --git a/tests/EventListener/OpenIdConnectFailureListenerTest.php b/tests/EventListener/OpenIdConnectFailureListenerTest.php new file mode 100644 index 00000000..a9d0207f --- /dev/null +++ b/tests/EventListener/OpenIdConnectFailureListenerTest.php @@ -0,0 +1,48 @@ +request('GET', '/openid-connect/generic?state=bogus&code=bogus'); + + $this->assertResponseStatusCodeSame(Response::HTTP_INTERNAL_SERVER_ERROR); + + $content = (string) $client->getResponse()->getContent(); + $this->assertStringContainsString('could not complete your login', $content); + $this->assertStringContainsString('/openidconnect/login/azure_az', $content); + } + + /** + * The identity provider's own error text says why a login failed and + * belongs in the log. The page says none of it. + */ + public function testErrorPageDisclosesNothingAboutTheFailure(): void + { + $client = static::createClient(); + $client->request('GET', '/openid-connect/generic?state=bogus&code=bogus'); + + $content = (string) $client->getResponse()->getContent(); + $this->assertStringNotContainsString('Error occurred validating openid login', $content); + $this->assertStringContainsString( + 'no-store', + (string) $client->getResponse()->headers->get('Cache-Control'), + ); + } +} From ea180290ec7db5e1c5d17f236e32633462fa9291 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 10:08:30 +0200 Subject: [PATCH 4/5] feat: report OIDC client secret expiry in health checks --- CHANGELOG.md | 2 + README.md | 15 ++- .../Check/ClientSecretExpiryHealthCheck.php | 85 ++++++++++++++ tests/Controller/HealthControllerTest.php | 2 +- .../ClientSecretExpiryHealthCheckTest.php | 109 ++++++++++++++++++ 5 files changed, 208 insertions(+), 5 deletions(-) create mode 100644 src/Health/Check/ClientSecretExpiryHealthCheck.php create mode 100644 tests/Health/Check/ClientSecretExpiryHealthCheckTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e0409c3c..b55e604e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 before the secret expires - Render a failed login as a page saying so, instead of an unhandled exception, in `OpenIdConnectFailureListener` + - Add an `oidc_client_secret` health check, so a client secret nearing its + expiry shows up in `/health/detail` instead of in a login loop - [#91](https://github.com/itk-dev/devops_itksites/pull/91) Health endpoints - Add `/health/live`, `/health/ready` and `/health/detail` endpoints - Add health checks for database, RabbitMQ transport and detection result freshness diff --git a/README.md b/README.md index 64b54161..e9c18749 100644 --- a/README.md +++ b/README.md @@ -81,14 +81,21 @@ read `/health/detail` when it goes red: curl --silent https://itksites.local.itkdev.dk/health/detail | jq ``` -The checks cover the database, the RabbitMQ messenger transport and the -freshness of the most recent detection result. The last one catches an ingest -pipeline that has stopped while the application itself is still serving -requests. +The checks cover the database, the RabbitMQ messenger transport, the freshness +of the most recent detection result and the expiry of the OIDC client secret. +The freshness check catches an ingest pipeline that has stopped while the +application itself is still serving requests. The client secret check catches +the expiry that breaks every login at once. `HEALTH_INGEST_MAX_AGE` sets how old the most recent detection result may be before ingest is reported as degraded. +`AZURE_AZ_OIDC_CLIENT_SECRET_EXPIRES_AT` is where the client secret check reads +the date. It reports degraded only once that date has passed, so watch +`days_remaining` in the detail payload rather than waiting for it to go red. With +no date configured the check reports skipped, which means nothing is watching the +secret. + Results are cached for `HEALTH_CACHE_TTL` seconds so that polling does not turn into load on the dependencies. The cache is the dedicated, filesystem-backed `cache.health` pool in `config/packages/cache.yaml` – it has to keep working diff --git a/src/Health/Check/ClientSecretExpiryHealthCheck.php b/src/Health/Check/ClientSecretExpiryHealthCheck.php new file mode 100644 index 00000000..ca0f56c6 --- /dev/null +++ b/src/Health/Check/ClientSecretExpiryHealthCheck.php @@ -0,0 +1,85 @@ +expiryChecker->getAllStatuses(); + + if ([] === $statuses) { + return HealthCheckResult::skipped($this->getName(), 'No OIDC providers are configured.'); + } + + $details = []; + $expired = []; + $unknown = []; + + foreach ($statuses as $providerKey => $expiry) { + $details[$providerKey.'.status'] = $expiry->status->value; + $details[$providerKey.'.expires_at'] = $expiry->expiresAt?->format(\DATE_ATOM); + $details[$providerKey.'.days_remaining'] = $expiry->daysRemaining; + + if ($expiry->isExpired()) { + $expired[] = $providerKey; + } elseif (ClientSecretExpiryStatus::Unknown === $expiry->status) { + $unknown[] = $providerKey; + } + } + + if ([] !== $expired) { + return HealthCheckResult::degraded( + $this->getName(), + \sprintf('Client secret past its configured expiry: %s.', implode(', ', $expired)), + $details + ); + } + + // No date anywhere is the state an installation is in before it + // configures any: nothing is being monitored, which is not the same as + // nothing being wrong, so it is reported as skipped rather than ok. + if (\count($unknown) === \count($statuses)) { + return HealthCheckResult::skipped($this->getName(), 'No client secret expiry dates are configured.'); + } + + return HealthCheckResult::ok($this->getName(), $details); + } +} diff --git a/tests/Controller/HealthControllerTest.php b/tests/Controller/HealthControllerTest.php index b8adf86d..2bb63a17 100644 --- a/tests/Controller/HealthControllerTest.php +++ b/tests/Controller/HealthControllerTest.php @@ -77,7 +77,7 @@ public function testDetailReportsEveryCheck(): void $this->assertArrayHasKey('checks', $payload); $this->assertEqualsCanonicalizing( - ['database', 'rabbitmq', 'ingest_freshness'], + ['database', 'rabbitmq', 'ingest_freshness', 'oidc_client_secret'], array_keys($payload['checks']) ); diff --git a/tests/Health/Check/ClientSecretExpiryHealthCheckTest.php b/tests/Health/Check/ClientSecretExpiryHealthCheckTest.php new file mode 100644 index 00000000..3ad226b2 --- /dev/null +++ b/tests/Health/Check/ClientSecretExpiryHealthCheckTest.php @@ -0,0 +1,109 @@ +check(['azure_az' => '2026-08-01']); + + self::assertSame(HealthStatus::Degraded, $result->status); + self::assertSame('Client secret past its configured expiry: azure_az.', $result->message); + self::assertSame('expired', $result->details['azure_az.status']); + self::assertLessThan(0, $result->details['azure_az.days_remaining']); + } + + /** + * Expiring soon stays ok. Thirty days of 503 would train everyone to ignore + * the endpoint before the day it matters; the remaining days are in the + * payload for whatever watches it. + */ + public function testExpiringSoonIsOkWithDaysRemaining(): void + { + $result = $this->check(['azure_az' => '2026-09-05']); + + self::assertSame(HealthStatus::Ok, $result->status); + self::assertSame('expiring_soon', $result->details['azure_az.status']); + // 2026-08-26 12:00 UTC to midnight on 2026-09-05 is 9.5 days, floored. + self::assertSame(9, $result->details['azure_az.days_remaining']); + } + + public function testHealthySecretIsOk(): void + { + $result = $this->check(['azure_az' => '2027-01-31']); + + self::assertSame(HealthStatus::Ok, $result->status); + self::assertSame('ok', $result->details['azure_az.status']); + self::assertSame('2027-01-31T00:00:00+00:00', $result->details['azure_az.expires_at']); + } + + /** + * An unconfigured date means nothing is being monitored, which must not read + * as healthy and must not fail readiness either. + */ + public function testUnconfiguredDateIsSkipped(): void + { + $result = $this->check(['azure_az' => null]); + + self::assertSame(HealthStatus::Skipped, $result->status); + self::assertSame('No client secret expiry dates are configured.', $result->message); + } + + public function testNoProvidersIsSkipped(): void + { + $result = $this->check([]); + + self::assertSame(HealthStatus::Skipped, $result->status); + self::assertSame('No OIDC providers are configured.', $result->message); + } + + /** + * One expired provider degrades the check even when another is fine, and + * both stay visible in the payload. + */ + public function testExpiredProviderDegradesAlongsideAHealthyOne(): void + { + $result = $this->check(['azure_az' => '2027-01-31', 'legacy' => '2026-08-01']); + + self::assertSame(HealthStatus::Degraded, $result->status); + self::assertSame('Client secret past its configured expiry: legacy.', $result->message); + self::assertSame('ok', $result->details['azure_az.status']); + self::assertSame('expired', $result->details['legacy.status']); + } + + /** + * @param array $expiryDates + */ + private function check(array $expiryDates): HealthCheckResult + { + $checker = new ClientSecretExpiryChecker( + new MockClock('2026-08-26 12:00:00', 'UTC'), + $expiryDates, + self::WARNING_DAYS, + new NullLogger(), + ); + + return (new ClientSecretExpiryHealthCheck($checker))->check(); + } +} From 984543a5c778d8a9e8c95be4411518b7b9264fcd Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 16:33:24 +0200 Subject: [PATCH 5/5] feat: update openid-connect-bundle to 6.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- composer.json | 2 +- composer.lock | 77 +++++-------------- config/reference.php | 2 + .../OpenIdConnectFailureListener.php | 38 ++++++--- src/Security/AzureOIDCAuthenticator.php | 5 +- .../error/openid_connect_failed.html.twig | 35 ++++++--- .../OpenIdConnectFailureListenerTest.php | 75 ++++++++++++++++++ 7 files changed, 151 insertions(+), 83 deletions(-) diff --git a/composer.json b/composer.json index 999ddaff..fdfdcc75 100644 --- a/composer.json +++ b/composer.json @@ -14,7 +14,7 @@ "doctrine/doctrine-migrations-bundle": "^4.0", "doctrine/orm": "^3.0", "easycorp/easyadmin-bundle": "^5.0", - "itk-dev/openid-connect-bundle": "^6.0", + "itk-dev/openid-connect-bundle": "^6.1", "itk-dev/vault-bundle": "^1.0.0", "nelmio/cors-bundle": "^2.2", "ocramius/doctrine-batch-utils": "^2.8", diff --git a/composer.lock b/composer.lock index 4a8701e8..4c17a85c 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "f78e1c30f5af0b778bf9f5779631a156", + "content-hash": "64cb1b11b5e4c3da1bdc1f83e64b414f", "packages": [ { "name": "api-platform/core", @@ -1928,31 +1928,31 @@ }, { "name": "itk-dev/openid-connect", - "version": "5.0.0", + "version": "5.1.0", "source": { "type": "git", "url": "https://github.com/itk-dev/openid-connect.git", - "reference": "f241f6794a2e74eab8c4808bc22f341a98e96f0b" + "reference": "950758b97397ab529f1df787c377ca54b321af79" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/itk-dev/openid-connect/zipball/f241f6794a2e74eab8c4808bc22f341a98e96f0b", - "reference": "f241f6794a2e74eab8c4808bc22f341a98e96f0b", + "url": "https://api.github.com/repos/itk-dev/openid-connect/zipball/950758b97397ab529f1df787c377ca54b321af79", + "reference": "950758b97397ab529f1df787c377ca54b321af79", "shasum": "" }, "require": { "ext-json": "*", "ext-openssl": "*", "firebase/php-jwt": "^7.0", - "league/oauth2-client": "^2.6", + "league/oauth2-client": "^2.8.1", "php": "^8.3", "psr/cache": "^2.0 || ^3.0", - "psr/http-client": "^1.0", - "robrichards/xmlseclibs": "^3.1.5" + "psr/http-client": "^1.0" }, "require-dev": { "ergebnis/composer-normalize": "^2.50", "friendsofphp/php-cs-fixer": "^3.75", + "infection/infection": "^0.35.2", "mockery/mockery": "^1.6.12", "phpstan/phpstan": "^2.1.41", "phpstan/phpstan-mockery": "^2.0", @@ -1986,29 +1986,29 @@ "description": "OpenID connect configuration package", "support": { "issues": "https://github.com/itk-dev/openid-connect/issues", - "source": "https://github.com/itk-dev/openid-connect/tree/5.0.0" + "source": "https://github.com/itk-dev/openid-connect/tree/5.1.0" }, - "time": "2026-06-02T09:01:56+00:00" + "time": "2026-08-26T12:01:21+00:00" }, { "name": "itk-dev/openid-connect-bundle", - "version": "6.0.0", + "version": "6.1.0", "source": { "type": "git", "url": "https://github.com/itk-dev/openid-connect-bundle.git", - "reference": "79be9eb80767e6c0f7945de8cee9bbfe2a4b5f3d" + "reference": "c990bd64fca599de02766671b99eab4cc260915b" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/itk-dev/openid-connect-bundle/zipball/79be9eb80767e6c0f7945de8cee9bbfe2a4b5f3d", - "reference": "79be9eb80767e6c0f7945de8cee9bbfe2a4b5f3d", + "url": "https://api.github.com/repos/itk-dev/openid-connect-bundle/zipball/c990bd64fca599de02766671b99eab4cc260915b", + "reference": "c990bd64fca599de02766671b99eab4cc260915b", "shasum": "" }, "require": { "doctrine/orm": "^2.8 || ^3.0", "ext-json": "*", "ext-openssl": "*", - "itk-dev/openid-connect": "^5.0", + "itk-dev/openid-connect": "^5.1", "php": "^8.3", "psr/log": "^3.0", "symfony/cache": "^6.4 || ^7.0 || ^8.0", @@ -2022,6 +2022,7 @@ "require-dev": { "ergebnis/composer-normalize": "^2.28", "friendsofphp/php-cs-fixer": "^3.11", + "igor-php/igor-php": "^0.9", "infection/infection": "*", "phpstan/phpstan": "^2.1.41", "phpstan/phpstan-deprecation-rules": "^2.0", @@ -2055,9 +2056,9 @@ "description": "Symfony bundle for openid-connect", "support": { "issues": "https://github.com/itk-dev/openid-connect-bundle/issues", - "source": "https://github.com/itk-dev/openid-connect-bundle/tree/6.0.0" + "source": "https://github.com/itk-dev/openid-connect-bundle/tree/6.1.0" }, - "time": "2026-08-25T19:09:29+00:00" + "time": "2026-08-26T14:22:26+00:00" }, { "name": "itk-dev/vault", @@ -3149,48 +3150,6 @@ }, "time": "2019-03-08T08:55:37+00:00" }, - { - "name": "robrichards/xmlseclibs", - "version": "3.1.5", - "source": { - "type": "git", - "url": "https://github.com/robrichards/xmlseclibs.git", - "reference": "03062be78178cbb5e8f605cd255dc32a14981f92" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/robrichards/xmlseclibs/zipball/03062be78178cbb5e8f605cd255dc32a14981f92", - "reference": "03062be78178cbb5e8f605cd255dc32a14981f92", - "shasum": "" - }, - "require": { - "ext-openssl": "*", - "php": ">= 5.4" - }, - "type": "library", - "autoload": { - "psr-4": { - "RobRichards\\XMLSecLibs\\": "src" - } - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "BSD-3-Clause" - ], - "description": "A PHP library for XML Security", - "homepage": "https://github.com/robrichards/xmlseclibs", - "keywords": [ - "security", - "signature", - "xml", - "xmldsig" - ], - "support": { - "issues": "https://github.com/robrichards/xmlseclibs/issues", - "source": "https://github.com/robrichards/xmlseclibs/tree/3.1.5" - }, - "time": "2026-03-13T10:31:56+00:00" - }, { "name": "symfony/amqp-messenger", "version": "v8.1.0", diff --git a/config/reference.php b/config/reference.php index fa1bf93c..db69eea8 100644 --- a/config/reference.php +++ b/config/reference.php @@ -1771,6 +1771,8 @@ * client_secret_expires_at?: scalar|Param|null, // Optional. Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. Set it and the bundle warns before the secret expires; leave it unset and the provider reports "unknown" and is not monitored. Set it where the real secret lives — a date carried in a committed default is a date nobody maintains. * leeway?: int|Param, // Leeway in seconds to account for clock skew between server and provider // Default: 10 * cache_duration?: int|Param, // Cache duration in seconds for the OIDC discovery document and JWKS (default: 86400 — 24 hours) // Default: 86400 + * scopes?: Param|string|list, + * pkce?: bool|Param, // Send a PKCE challenge (RFC 7636, S256) with the authorization request // Default: true * redirect_uri?: scalar|Param|null, // Redirect URI registered at identity provider * redirect_route?: scalar|Param|null, // Redirect route registered at identity provider (must not be set if redirect_uri is set) * redirect_route_parameters?: array, diff --git a/src/EventListener/OpenIdConnectFailureListener.php b/src/EventListener/OpenIdConnectFailureListener.php index 6a9dd3c8..f2338854 100644 --- a/src/EventListener/OpenIdConnectFailureListener.php +++ b/src/EventListener/OpenIdConnectFailureListener.php @@ -5,9 +5,11 @@ namespace App\EventListener; use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; use Symfony\Component\EventDispatcher\Attribute\AsEventListener; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\ExceptionEvent; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; use Twig\Environment; @@ -15,12 +17,16 @@ /** * Renders a failed OIDC login as a page rather than as an unhandled exception. * - * Since openid-connect-bundle 6.0 a failed callback throws - * `AuthenticationFailedException`, which is deliberately not an - * `AuthenticationException`: the firewall no longer catches it and sends the - * browser back to Azure, which is what turned the expired client secret into a - * redirect loop. What escapes the firewall instead is an unhandled exception, - * so without this listener a failed login shows the default error page. + * A failed callback throws `AuthenticationFailedException`, which is deliberately + * not an `AuthenticationException`: the firewall does not catch it and does not + * send the browser back to Azure, which is what turned the expired client secret + * into a redirect loop. What escapes the firewall instead is an unhandled + * exception, so without this listener a failed login shows the default error page. + * + * `ProviderErrorException` is the subclass Azure sends when it refuses the request + * — a cancelled consent screen, an expired session there, a tenant policy. It + * carries the status the answer should have, and the page says the login was + * declined rather than that something went wrong. * * Only `AuthenticationFailedException` is handled. The bundle's other failures * are configuration bugs — an unknown provider key, an unreachable @@ -44,19 +50,31 @@ public function __construct( public function onKernelException(ExceptionEvent $event): void { - if (!$event->isMainRequest() || !$event->getThrowable() instanceof AuthenticationFailedException) { + $exception = $event->getThrowable(); + + if (!$event->isMainRequest() || !$exception instanceof AuthenticationFailedException) { return; } // Deliberately without the exception message: it carries the identity // provider's own error text, which belongs in the log, not in a browser. + // `declined` is the one thing worth telling the user, and it is the error + // code rather than that text. $content = $this->twig->render('error/openid_connect_failed.html.twig', [ 'login_url' => $this->urlGenerator->generate('itkdev_openid_connect_login', ['providerKey' => 'azure_az']), + 'declined' => $exception instanceof ProviderErrorException + && ProviderErrorException::ACCESS_DENIED === $exception->getError(), ]); - // Still a 500: the most likely cause is on this side of the login, and a - // failed login should read as an error in the log and in monitoring. - $response = new Response($content, Response::HTTP_INTERNAL_SERVER_ERROR); + // A refusal states its own status — 403 where the user or a policy said + // no, 503 where Azure reports its own trouble. Everything else is a 500: + // the likely cause is on this side of the login, and it should read as an + // error in the log and in monitoring. + $status = $exception instanceof HttpExceptionInterface + ? $exception->getStatusCode() + : Response::HTTP_INTERNAL_SERVER_ERROR; + + $response = new Response($content, $status); $response->headers->set('Cache-Control', 'no-store, private'); $event->setResponse($response); diff --git a/src/Security/AzureOIDCAuthenticator.php b/src/Security/AzureOIDCAuthenticator.php index d2ab5d12..3d6198b1 100644 --- a/src/Security/AzureOIDCAuthenticator.php +++ b/src/Security/AzureOIDCAuthenticator.php @@ -63,7 +63,10 @@ public function authenticate(Request $request): Passport return new SelfValidatingPassport(new UserBadge($user->getUserIdentifier())); } catch (OpenIdConnectExceptionInterface $exception) { - throw new CustomUserMessageAuthenticationException($exception->getMessage()); + // Chained: the bundle reads the cause back in onAuthenticationFailure() + // to decide what the user is shown. Dropping it turns a refusal the + // user caused into an unexplained 500. + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); } } diff --git a/templates/error/openid_connect_failed.html.twig b/templates/error/openid_connect_failed.html.twig index e95fb0c1..1141b94a 100644 --- a/templates/error/openid_connect_failed.html.twig +++ b/templates/error/openid_connect_failed.html.twig @@ -14,7 +14,7 @@ - Login failed - ITKsites + {% if declined %}Login declined{% else %}Login failed{% endif %} - ITKsites