diff --git a/.github/workflows/changelog.yaml b/.github/workflows/changelog.yaml index 63638c2..71da908 100644 --- a/.github/workflows/changelog.yaml +++ b/.github/workflows/changelog.yaml @@ -18,7 +18,7 @@ jobs: fail-fast: false steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@v7 with: fetch-depth: 2 diff --git a/.github/workflows/composer.yaml b/.github/workflows/composer.yaml index 9752a66..8c27ac6 100644 --- a/.github/workflows/composer.yaml +++ b/.github/workflows/composer.yaml @@ -44,7 +44,7 @@ jobs: matrix: prefer: [prefer-lowest, prefer-stable] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -58,7 +58,7 @@ jobs: strategy: fail-fast: false steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -73,7 +73,7 @@ jobs: strategy: fail-fast: false steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/.github/workflows/github_build_release.yml b/.github/workflows/github_build_release.yml index b2c083f..abb8cd8 100644 --- a/.github/workflows/github_build_release.yml +++ b/.github/workflows/github_build_release.yml @@ -16,7 +16,7 @@ jobs: APP_ENV: prod steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Create a release in GitHub run: gh release create ${{ github.ref_name }} --verify-tag --generate-notes diff --git a/.github/workflows/markdown.yaml b/.github/workflows/markdown.yaml index 8f0fc25..b0f74c5 100644 --- a/.github/workflows/markdown.yaml +++ b/.github/workflows/markdown.yaml @@ -34,7 +34,7 @@ jobs: fail-fast: false steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/.github/workflows/php.yaml b/.github/workflows/php.yaml index f1d2939..8807a29 100644 --- a/.github/workflows/php.yaml +++ b/.github/workflows/php.yaml @@ -15,7 +15,7 @@ jobs: name: PHP - Check Coding Standards runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -26,10 +26,41 @@ jobs: docker compose run --rm phpfpm vendor/bin/php-cs-fixer fix --dry-run --diff phpstan: + # Analysed on the highest supported PHP, not the default 8.3 service: Symfony 8.1 + # requires PHP >= 8.4.1, so on 8.3 composer cannot install the Symfony 8 that + # composer.json advertises and the analysis never sees it. phpstan.neon pins the + # analysis to the declared php ^8.3 range, so the floor stays protected from here. name: PHPStan runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 + + - name: Create docker network + run: | + docker network create frontend + + - run: | + docker compose run --rm phpfpm85 composer install + docker compose run --rm phpfpm85 vendor/bin/phpstan + + phpstan-lowest: + # The declared dependency floor, analysed with current dev tooling. A plain + # --prefer-lowest also downgrades PHPUnit and phpstan-phpunit, and then nearly + # every error reported is an artefact of that rather than a statement about + # Symfony 6.4 — 29 of 30, when this was measured. Lowering only what `require` + # names keeps the analysis about the runtime the bundle claims to support. + # + # Runs on the PHP floor too, in the default 8.3 service, as the counterpart to the + # phpstan job's ceiling on 8.5. + # + # Analysis only: lowering the runtime packages drags shared Symfony components + # (finder, console) down with them, and php-cs-fixer built on those emits nonsense + # such as `previous : $e`. Never add a formatter to this job, and do not reuse the + # resulting vendor/ for anything but PHPStan. + name: PHPStan (lowest runtime dependencies) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -37,6 +68,8 @@ jobs: - run: | docker compose run --rm phpfpm composer install + docker compose run --rm phpfpm sh -c \ + 'composer update --prefer-lowest --with-all-dependencies $(composer show --direct --no-dev --name-only)' docker compose run --rm phpfpm vendor/bin/phpstan unit-tests: @@ -65,7 +98,7 @@ jobs: php: "8.5" prefer: prefer-stable steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -96,7 +129,7 @@ jobs: php: "8.3" prefer: prefer-stable steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/.github/workflows/yaml.yaml b/.github/workflows/yaml.yaml index 299d4e1..80d0a23 100644 --- a/.github/workflows/yaml.yaml +++ b/.github/workflows/yaml.yaml @@ -31,7 +31,7 @@ jobs: yaml-lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index ab2818b..a7e8c3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,52 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [6.0.0] - 2026-08-25 + +See [UPGRADE-6.0.md](UPGRADE-6.0.md). + +### Changed (BREAKING) + +- A failed OpenID Connect callback throws `AuthenticationFailedException` instead of + Symfony's `AuthenticationException`, so it escapes the firewall rather than + redirecting to the identity provider again. `CliLoginTokenAuthenticator` is + unchanged. +- `getPrevious()` on that exception is the underlying OpenID Connect exception. +- A callback is recognised only on the provider's callback path, not on any URL + carrying `state` and `code` (#63). +- Each provider must declare `redirect_uri`, `redirect_route` or `callback_path`. + +### Added + +- `callback_path` per provider, for a proxy that rewrites the path without announcing + it. +- `OpenIdLoginAuthenticator::getSupportedProviderKeys()`, to narrow an authenticator to + named providers. Defaults to all of them. +- `OpenIdLoginAuthenticator::createTargetPathRedirect()`, returning the user to the + page that sent them to log in. +- `?target_path=` on the login route, validated as a path within the application. +- `OpenIdConfigurationProviderManager::getRedirectUriPaths()` and `isCallbackPath()`. + +### Changed + +- `client_secret_expires_at` remains optional, and the 5.1 deprecation for leaving it + unset is gone. Unset reports `unknown`; a value that is set must be a string and + parseable. +- `UPGRADE-6.0.md` rewritten and linked from `README.md`. +- Static analysis runs against both ends of the supported dependency range. +- `actions/checkout` updated to v7. + +### Fixed + +- `logging_options.logger` no longer depends on bundle registration order. +- `getContainerExtension()` no longer returns `mixed` on Symfony 6.4. + +### Removed (BREAKING) + +- `ItkOpenIdConnectBundleException`. Catch `OpenIdConnectBundleExceptionInterface`. +- `UserDoesNotExistException`. Use Symfony's `UserNotFoundException`. +- `symfony/deprecation-contracts` from `require`. + ## [5.1.1] - 2026-08-19 ### Fixed @@ -281,7 +327,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `itk-dev/openid-connect` 1.0.0 to 2.1.0 - OpenId Connect Bundle: Added CLI login feature. -[unreleased]: https://github.com/itk-dev/openid-connect-bundle/compare/5.1.1...HEAD +[unreleased]: https://github.com/itk-dev/openid-connect-bundle/compare/6.0.0...HEAD +[6.0.0]: https://github.com/itk-dev/openid-connect-bundle/compare/5.1.1...6.0.0 [5.1.1]: https://github.com/itk-dev/openid-connect-bundle/compare/5.1.0...5.1.1 [5.1.0]: https://github.com/itk-dev/openid-connect-bundle/compare/5.0.0...5.1.0 [5.0.0]: https://github.com/itk-dev/openid-connect-bundle/compare/4.2.0...5.0.0 diff --git a/README.md b/README.md index db3301e..2b0930c 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,9 @@ Symfony bundle for authorization via OpenID Connect. > If your application needs browser-based OIDC login, this bundle is still > required. +Upgrading from an earlier major? See [UPGRADE-6.0.md](UPGRADE-6.0.md) and +[UPGRADE-5.0.md](UPGRADE-5.0.md). + ## Installation To install run @@ -115,12 +118,18 @@ itkdev_openid_connect: metadata_url: '%env(string:ADMIN_OIDC_METADATA_URL)%' client_id: '%env(string:ADMIN_OIDC_CLIENT_ID)%' client_secret: '%env(string:ADMIN_OIDC_CLIENT_SECRET)%' - # Date the client secret expires. An expired secret breaks every login, - # so setting this lets the bundle warn while there is still time to - # rotate. Will be REQUIRED in 6.0. See "Client secret expiry" below. + # Optional: date the client secret expires. Set it and the bundle warns + # before the secret expires; unset means the provider is not + # monitored and reports "unknown". Set it where the real secret + # lives. See "Client secret expiry" below. client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' # Specify redirect URI redirect_uri: '%env(string:ADMIN_OIDC_REDIRECT_URI)%' + # Optional: the path the callback arrives on, for a proxy that rewrites it + # without sending X-Forwarded-Prefix. Defaults to the path of + # redirect_uri, or of the generated redirect_route. See "Which + # requests count as a callback" below. + callback_path: '/auth/callback' # Optional: Specify leeway (seconds) to account for clock skew between provider and hosting # Defaults to 10 leeway: '%env(int:ADMIN_OIDC_LEEWAY)%' @@ -226,13 +235,18 @@ For a genuinely expired secret that means the login still fails, at the callback with `invalid_client` — but the `critical` record here and the failure record from the callback together name the cause without anyone having to reproduce it. -Until the date is configured a provider sits in `unknown`, where none of the above -applies and nothing is reported. +`client_secret_expires_at` is optional, and where you set it matters more than that +you set it. Put it with the real secret — the production secret store, or a `when@prod` +block. A date in a committed `.env` default is a date nobody maintains: it reports `ok` +while measuring nothing, which is worse than the `unknown` you get by leaving it out. -> [!NOTE] -> `client_secret_expires_at` is optional in 5.x and **will be required in 6.0**. -> Providers without it emit a deprecation warning, because the bundle cannot warn -> about an expiry it does not know about. +Quote it: YAML reads an unquoted `2027-01-31` as a number, and a value that is not a +string is rejected while the container compiles. + +A provider still reaches `unknown` at runtime when the value resolves to something +unusable — an environment variable that is set but blank, or a date +`DateTimeImmutable` cannot parse — and that is reported at `error`, because an +unmonitored secret is no better than not having this feature. ##### Monitoring expiry @@ -406,6 +420,10 @@ default for services in `config/services.yaml`. With autoconfiguration disabled the authenticator falls back to a `NullLogger` and logs nothing, while the rest of the bundle keeps logging. +A configured logger also takes precedence over a `setLogger()` call on the +authenticator's own service definition. Disabling autoconfiguration is the way to +wire a logger yourself. + #### Audit logging Separately from the failure logging above, the bundle can write an @@ -638,7 +656,8 @@ class SomeAuthenticator extends OpenIdLoginAuthenticator public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response { - // TODO: Implement onAuthenticationSuccess() method. + // Back to whatever the user was trying to reach, or your default. + return $this->createTargetPathRedirect($request, $firewallName, '/'); } public function start(Request $request, AuthenticationException $authException = null) @@ -663,6 +682,85 @@ security: entry_point: App\Security\ExampleAuthenticator ``` +With one authenticator per provider, override `getSupportedProviderKeys()` in each so +it only answers its own provider's callback: + +```php +protected function getSupportedProviderKeys(): array +{ + return ['admin']; +} +``` + +Without the override every authenticator supports every callback path, Symfony asks +them in the order above, and the session's provider key decides which provider +validates the callback — which is how existing setups already work. + +#### Which requests count as a callback + +A request is treated as an OpenID Connect callback when it carries both `state` and +`code` **and** arrives on a provider's configured callback path — the path of +`redirect_uri`, of the generated `redirect_route`, or `callback_path` when set. Every +provider must declare one of the three. + +`?state=…&code=…` on any other URL is ignored by the authenticator, and the firewall +handles the request as it would without them: an anonymous visitor is sent to your +entry point, a logged-in one gets the page. + +The path is matched against `getBaseUrl()` plus `getPathInfo()`, so an application +deployed in a subdirectory, or behind a proxy that sends `X-Forwarded-Prefix` with +Symfony's [trusted proxies](https://symfony.com/doc/current/deployment/proxies.html) +configured, matches without further configuration: the prefix is part of the base URL +on the way in and part of `redirect_uri` on the way out. + +Set `callback_path` when a proxy rewrites the path **without** announcing it — an +external `https://app.example.org/prefix/auth/callback` that arrives here as +`/auth/callback`. Nothing in the request says where the prefix went, so the path has to +be declared: + +```yaml +callback_path: '/auth/callback' +``` + +Give it the path as this application receives it, including any base path of its own. + +#### Returning to the originally requested page + +`createTargetPathRedirect()` sends the user back to the page that triggered the login, +falling back to a URL of your choosing when there is nothing to go back to: + +```php +public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response +{ + return $this->createTargetPathRedirect($request, $firewallName, $this->router->generate('dashboard')); +} +``` + +Symfony saves the requested page when your entry point fires, so this works both for +applications that redirect straight to the identity provider and for those that show a +login screen with a provider link on it. The fallback covers a user who went to the +login link directly. The saved page is cleared on use, so a later visit to that link +does not replay it. + +For a login link on a public page, where nothing was denied and so nothing was saved, +name the destination on the link itself: + +```twig +Log in +``` + +The value must be a path within the application: a single leading `/`, no backslash, +no `://`, no control characters. Anything else is dropped and logged at `warning`, +because it would otherwise turn the login route into an open redirect. When a page was +also denied, that page wins — it is what the user was actually stopped from reaching. + +Only pages that exist and are access-controlled return this way, and that is by +design. Routing runs before security — `RouterListener` on `kernel.request` at +priority 32, the firewall at 8 — so a link to a URL with no route is a 404 before the +firewall is reached: no entry point fires, nothing is saved, and there is nothing to +come back to. A link to a page that exists but is public simply loads. Neither is +affected by the login flow. + #### Example authenticator functions Here is an example using a `User` with a name and email property. First we @@ -708,7 +806,7 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator private readonly UrlGeneratorInterface $router, private readonly OpenIdConfigurationProviderManager $providerManager ) { - parent::__construct($providerManager, $requestStack); + parent::__construct($providerManager); } /** @inheritDoc */ @@ -745,7 +843,11 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator /** @inheritDoc */ public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response { - return new RedirectResponse($this->router->generate('homepage_authenticated')); + return $this->createTargetPathRedirect( + $request, + $firewallName, + $this->router->generate('homepage_authenticated') + ); } /** @inheritDoc */ @@ -887,6 +989,9 @@ We use [SemVer](http://semver.org/) for versioning. For the versions available, see the [tags on this repository](https://github.com/itk-dev/openid-connect/tags). +Upgrading across a major: [UPGRADE-6.0.md](UPGRADE-6.0.md), +[UPGRADE-5.0.md](UPGRADE-5.0.md). [CHANGELOG.md](CHANGELOG.md) has the rest. + ## License This project is licensed under the MIT License - see the diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md new file mode 100644 index 0000000..82b6d71 --- /dev/null +++ b/UPGRADE-6.0.md @@ -0,0 +1,80 @@ +# Upgrading from 5.x to 6.0 + +```sh +composer require itk-dev/openid-connect-bundle:^6.0 +``` + +`require`, not `update`: `composer update` will not cross the major your +`composer.json` pins. Coming from 4.x, do [UPGRADE-5.0.md](UPGRADE-5.0.md) first. + +## Every provider must declare where its callback arrives + +Set one of `redirect_uri`, `redirect_route` or `callback_path` per provider, or the +container will not compile: + +```text +Invalid configuration for path "itkdev_openid_connect.openid_providers.admin.options": +One of redirect_uri, redirect_route or callback_path must be set: it is how a callback +is recognised. +``` + +Most applications already set `redirect_uri` and need no change. + +## A callback is only recognised on that path + +`?state=…&code=…` on any other URL is left to the firewall. If a reverse proxy rewrites +the path without sending `X-Forwarded-Prefix`, declare the path the application +receives: + +```yaml +openid_providers: + admin: + options: + redirect_uri: 'https://app.example.org/prefix/auth/callback' + callback_path: '/auth/callback' +``` + +A subdirectory deployment, or a proxy sending `X-Forwarded-Prefix` with trusted proxies +configured, needs no `callback_path`. + +## A failed callback throws `AuthenticationFailedException` + +`OpenIdLoginAuthenticator::onAuthenticationFailure()` now throws +`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException`, which is not a +Symfony `AuthenticationException` and so escapes the firewall. Your application renders +it — a 500 by default. + +Nothing to do unless you catch `AuthenticationException` around the callback, or +override `onAuthenticationFailure()`: + +```diff +- } catch (\Symfony\Component\Security\Core\Exception\AuthenticationException $e) { ++ } catch (\ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface $e) { +``` + +`getPrevious()` is the underlying OpenID Connect exception, not the +`AuthenticationException`. + +To render something friendlier than a 500, listen for the exception and set a response. +Do not redirect to the login route — that reintroduces the loop this replaced. Render a +template rather than `getMessage()`, which carries the identity provider's error text. + +`CliLoginTokenAuthenticator` is unchanged. + +## Removed + +| Removed | Use instead | +| --- | --- | +| `ItkOpenIdConnectBundleException` | `OpenIdConnectBundleExceptionInterface` | +| `UserDoesNotExistException` | Symfony's `UserNotFoundException` | + +`UsernameDoesNotExistException` stays. `symfony/deprecation-contracts` is no longer +required by the bundle; require it yourself if your own code calls +`trigger_deprecation()`. + +## Also worth knowing + +`client_secret_expires_at` is still optional, and the 5.1 deprecation for leaving it +unset is gone. New in 6.0: `callback_path`, `getSupportedProviderKeys()`, +`createTargetPathRedirect()` and `?target_path=` — see the +[README](README.md) and [CHANGELOG](CHANGELOG.md). diff --git a/composer.json b/composer.json index bb4385b..f925474 100644 --- a/composer.json +++ b/composer.json @@ -22,7 +22,6 @@ "psr/log": "^3.0", "symfony/cache": "^6.4 || ^7.0 || ^8.0", "symfony/clock": "^6.4 || ^7.0 || ^8.0", - "symfony/deprecation-contracts": "^2.5 || ^3.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", diff --git a/docs/adr/002-fail-closed-on-authentication-failure.md b/docs/adr/002-fail-closed-on-authentication-failure.md new file mode 100644 index 0000000..240c56e --- /dev/null +++ b/docs/adr/002-fail-closed-on-authentication-failure.md @@ -0,0 +1,80 @@ +# 002: Fail closed when an OpenID Connect callback cannot be validated + +- **Created By:** Ture Gjørup +- **Date:** 2026-08-19 +- **Decision Maker:** Ture Gjørup +- **Stakeholders:** Bundle consumers; operators of those applications; bundle + maintainers +- **Status:** Accepted + +## Context + +On 2026-08-12 an expired Azure client secret took `sites.itkdev.dk` down as an +endless redirect rather than an error: nine rounds in twenty-five seconds, no error +page, nothing logged. + +`onAuthenticationFailure()` threw `AuthenticationException`. Symfony's security +`ExceptionListener` catches those and calls the firewall's entry point, which for +this authenticator redirects to the identity provider — so every failure re-entered +the flow that had just failed. A live SSO session upstream returned a fresh `code` +immediately, so nothing degraded and nothing stopped it. + +`AuthenticatorManager::executeAuthenticator()` catches only +`AuthenticationException` (verified in `symfony/security-http` 6.4 and 8.1). +Anything else propagates to `HttpKernel`. + +## Options Considered + +1. **Throw outside the security hierarchy (chosen).** The firewall does not catch + it, so the loop is impossible by construction and the invariant is one test + assertion. Costs a MAJOR: consumers catching `AuthenticationException` must + migrate, and users see a 500 instead of a redirect. +2. **Count attempts in the session.** No BC break, but it adds mutable state to a + failure path that can itself lose the session, N is arbitrary, and the user + still makes N pointless round trips first. Treats the symptom. +3. **Return an error `Response` from the handler.** Puts presentation in a bundle + with no templates, and swallows the failure so error reporters never see it. + +## Decision + +Adopt option 1 in 6.0.0. `OpenIdLoginAuthenticator::onAuthenticationFailure()` +throws `AuthenticationFailedException` — a `\RuntimeException` implementing the +ADR 001 marker. + +**The cause has to stay outside the hierarchy as well.** `ExceptionListener` walks +the whole `$previous` chain, so chaining the `AuthenticationException` it handed us +re-enters the entry point exactly as throwing one would. The bundle chains the +first cause below it instead — the library exception that says why the callback +failed — and nothing at all when the chain holds only security exceptions. A +knowing departure from ADR 001's "always pass `$previous`": the chain is kept as +far as it can be without restoring the loop, and the message carries the original +text either way. + +**Scope: the OIDC authenticator only.** `CliLoginTokenAuthenticator` is not an +entry point, so its failures redirect to normal login and cannot loop: `supports()` +requires a `loginToken` the redirect does not carry. For a consumed single-use +token that is friendlier than a 500. The loop needs an authenticator whose own +entry point re-triggers it. + +**Not logged here.** `AuthenticatorManager` logs the original exception, +`validateClaims()` logs the specific reason, and the application logs what escapes. + +## Consequences + +- The loop cannot recur. It is prevented by a type, not by configuration or a + counter. +- Consumers catching `AuthenticationException` around the callback must switch to + `OpenIdConnectBundleExceptionInterface`. See `UPGRADE-6.0.md`. +- `getPrevious()` is the underlying OpenID Connect exception, not the + `AuthenticationException` the firewall raised. +- A transient identity-provider failure is now an error rather than a silent retry. + Accepted: the bundle cannot tell transient from permanent, and retrying is what + caused the outage. +- Applications wanting better than a 500 add an exception listener. + +## References + +- [ADR 001](001-marker-interface-exception-hierarchy.md) — the marker contract this + concrete follows +- `AuthenticatorManager` and the security `ExceptionListener` in + `symfony/security-http` diff --git a/docs/adr/003-constrain-supports-to-callback-path.md b/docs/adr/003-constrain-supports-to-callback-path.md new file mode 100644 index 0000000..465313a --- /dev/null +++ b/docs/adr/003-constrain-supports-to-callback-path.md @@ -0,0 +1,98 @@ +# 003: Treat only the configured callback path as a callback + +- **Created By:** Ture Gjørup +- **Date:** 2026-08-20 +- **Decision Maker:** Ture Gjørup +- **Stakeholders:** Bundle consumers; operators of those applications; bundle + maintainers +- **Status:** Accepted + +## Context + +`OpenIdLoginAuthenticator::supports()` matched on `state` and `code` alone, so every +URL behind the firewall was a potential callback. Before 6.0 a stray or forged +callback degraded quietly: the failure was an `AuthenticationException`, so the +firewall answered with a redirect to the entry point. [ADR 002](002-fail-closed-on-authentication-failure.md) +made that failure escape as `AuthenticationFailedException`, which turned the same +request into a 500 — raisable on any URL by an unauthenticated caller, and noise for +error reporting. Issue #63. + +## Options Considered + +1. **Match the provider's configured callback path (chosen).** The path comes from + configuration the consumer already writes, and a request that is not a callback is + left to the firewall. +2. **Require the session to hold `oauth2provider`.** No new configuration, but it + reinstates the outage: a lost session would make the request merely + unauthenticated, so the firewall calls the entry point, the provider returns a + fresh `code`, and it arrives back with the session still broken — the loop of + 2026-08-12. It also conflates "is this a callback" with "did this browser start a + login", and touching the session in `supports()` starts one for anonymous traffic. +3. **Leave it and filter in error reporting.** Moves a bundle defect into every + consumer's monitoring configuration. + +## Decision + +Adopt option 1 in 6.0.0. `supports()` requires `state`, `code`, and a path matching +one of this authenticator's providers. + +- **Paths come from configuration, not from a provider instance.** Building a provider + pulls in discovery, an HTTP client and a cache pool; `supports()` runs on every + request. `OpenIdConfigurationProviderManager::getRedirectUriPaths()` derives and + memoizes them. +- **`callback_path` is the escape hatch** for a proxy that rewrites the path, where the + external `redirect_uri` path is not the one the application sees. +- **`redirect_route` is generated as `ABSOLUTE_PATH`**, so host and scheme + requirements on the route do not enter the comparison; a route whose path varies by + host is not supported. +- **The request path is `getBaseUrl().getPathInfo()`, not path info alone.** + `Request::preparePathInfo()` strips `getBaseUrlReal()`, so path info excludes a + subdirectory deployment's base path and any trusted `X-Forwarded-Prefix` — while a + `redirect_uri` contains them, being the URL the identity provider was given, and + `UrlGenerator` prepends the routing context's base URL, which includes the trusted + prefix. Comparing path info alone rejected every callback in either deployment. + Derived paths are therefore memoized per routing-context base URL, so a service that + sees both proxied and direct traffic is not frozen to whichever arrived first. +- **A provider must declare `redirect_uri`, `redirect_route` or `callback_path`.** + Enforced when the container compiles. A provider with none has no callback path, and + "matches every path" is the defect being removed. +- **`getSupportedProviderKeys()`** defaults to every provider, so existing + multi-authenticator firewalls behave as before, and can be overridden by an + authenticator bound to one provider. +- **Nothing is logged from `supports()`.** It runs pre-authentication on every + request; a log call there is an amplifier for anyone sending traffic. The firewall's + own handling is the record. + +## Consequences + +- A forged callback is handled by the firewall again, as it was before 6.0, without + giving up fail-closed behaviour for real callbacks. +- Consumers behind a rewriting proxy must set `callback_path`, and every provider must + declare a callback target. See `UPGRADE-6.0.md`. +- The callback path is now part of the bundle's contract with the identity provider: + changing `redirect_uri` without changing the registration at the provider fails in + the same way it always did, but changing it *only* at the provider now also stops + callbacks being recognised. + +## Returning to a page the firewall never saw + +Symfony saves the requested page when the entry point fires, which covers a user who +was denied something. A login link followed from a public page has no such record, so +`?target_path=` on the login route lets the link name where to go. + +It is stored under a bundle-private session key rather than in `TargetPathTrait`'s +slot: that slot is keyed by firewall, `LoginController` has no firewall name, and +writing there would put a value in the firewall's own record that the firewall never +saved. `createTargetPathRedirect()` prefers the firewall's record when both exist — +that is the page the user was actually stopped from reaching — and clears both. + +The parameter ends up in a `Location` header, so it is validated as a path within the +application and dropped otherwise: a single leading `/`, no backslashes, no scheme +separator, no control characters. A rejected value is logged at `warning`; correcting +one would be guessing at intent on a security boundary. + +## References + +- [ADR 002](002-fail-closed-on-authentication-failure.md) — the fail-closed decision + that made this worth fixing now +- Issue #63 diff --git a/docs/adr/README.md b/docs/adr/README.md index d766d1a..0b58831 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -12,3 +12,9 @@ context that drove them and the consequences we accept. See - **[001 — Adopt marker-interface exception hierarchy across library and bundle](001-marker-interface-exception-hierarchy.md)** — Draft — 2026-05-11 +- **[002 — Fail closed when an OpenID Connect callback cannot be + validated](002-fail-closed-on-authentication-failure.md)** — Accepted — + 2026-08-19 +- **[003 — Treat only the configured callback path as a + callback](003-constrain-supports-to-callback-path.md)** — Accepted — + 2026-08-20 diff --git a/infection.json5 b/infection.json5 index 6686ea2..0b231c3 100644 --- a/infection.json5 +++ b/infection.json5 @@ -24,6 +24,23 @@ // typed int property then coerces, so the mutant is behaviourally // identical and cannot be killed by a test. The cast stays because // PHPStan at max level requires it. + // Alias::__toString() returns the target id, so dropping the cast leaves an + // Alias object that every consumer of $loggerId coerces to the same string. + // The cast stays because PHPStan at max level requires it. + "CastString": { + "ignoreSourceCodeByRegex": [ + "\\$loggerId = \\(string\\) \\$container->getAlias\\(\\$loggerId\\);" + ] + }, + // `Bundle::build()` is empty in Symfony, so dropping the parent call changes + // nothing observable. It stays because a future Symfony may put something + // there. Scoped to the call itself: the `addCompilerPass()` below it is a + // real mutant and is killed by a test. + "MethodCallRemoval": { + "ignoreSourceCodeByRegex": [ + "parent::build\\(\\$container\\);" + ] + }, "CastInt": { "ignore": [ "ItkDev\\OpenIdConnectBundle\\Util\\ClientSecretExpiryChecker::getStatus" diff --git a/phpstan.neon b/phpstan.neon index dfebdc7..17a6df5 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -15,6 +15,19 @@ parameters: - phpstan/Rule reportIgnoresWithoutComments: true + # Analyse against the whole range composer.json declares (php ^8.3), not just the + # version this happens to run on. Without it PHPStan assumes the runtime PHP, so + # analysing on 8.3 says nothing about 8.5 and analysing on 8.5 would accept syntax + # that breaks 8.3 consumers. + # + # It does not cover the *dependency* axis, which is where this bundle's real + # findings have come from: Symfony 8.1 deprecations are invisible on PHP 8.3 + # because symfony/http-kernel 8.1 requires PHP >= 8.4.1, so composer cannot install + # it here. Widening that needs a second job, not a setting. + phpVersion: + min: 80300 + max: 80500 + ignoreErrors: # PHPUnit declares assertions as static methods on `Assert`, but the # pervasive idiom is `$this->assertX()`. phpstan-phpunit handles type diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index b40a76f..6cac73f 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -5,6 +5,7 @@ use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; +use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Util\ClientSecretExpiryChecker; use Psr\Log\LoggerInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -19,6 +20,11 @@ */ class LoginController extends AbstractController { + /** + * Query parameter naming where to go after a successful login. + */ + public const string TARGET_PATH_PARAMETER = 'target_path'; + public function __construct( private readonly OpenIdConfigurationProviderManager $providerManager, private readonly LoggerInterface $logger, @@ -52,6 +58,8 @@ public function login(Request $request, SessionInterface $session, string $provi $nonce = $provider->generateNonce(); $state = $provider->generateState(); + $this->rememberNamedTargetPath($request, $session); + // Save to session $session->set('oauth2provider', $providerKey); $session->set('oauth2state', $state); @@ -79,6 +87,62 @@ public function login(Request $request, SessionInterface $session, string $provi return new RedirectResponse($authUrl); } + /** + * Remember a return target named on the login link. + * + * For a login link followed from a public page: the firewall saves nothing, + * because nothing was denied, so there is no requested page for + * `createTargetPathRedirect()` to return to. A link may name one instead. + * + * Anything not plainly a path within this application is dropped rather than + * corrected. This value ends up in a `Location` header after a successful login, + * so a permissive reading turns the login route into an open redirect for anyone + * who can get a user to follow a link. + */ + private function rememberNamedTargetPath(Request $request, SessionInterface $session): void + { + $target = $request->query->get(self::TARGET_PATH_PARAMETER); + + if (null === $target) { + // A target from an abandoned login link would otherwise sit in the session + // and be spent by whatever login came next. + $session->remove(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY); + + return; + } + + if (!self::isLocalPath($target)) { + $this->logger->warning('OIDC login: ignoring an unusable target_path', [ + 'target_path' => $target, + ]); + + return; + } + + $session->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, $target); + } + + /** + * Whether a value is a path into this application and nothing else. + * + * Rejected, in order: anything not starting with a single `/` (absolute URLs, + * scheme-relative `//host`, bare words); a backslash anywhere, since browsers + * have historically read `/\host` as scheme-relative; a scheme separator + * anywhere; and control characters, which belong to header-splitting attempts. + */ + private static function isLocalPath(string $target): bool + { + if (!str_starts_with($target, '/') || str_starts_with($target, '//')) { + return false; + } + + if (str_contains($target, '\\') || str_contains($target, '://')) { + return false; + } + + return 1 !== preg_match('/[\x00-\x1F\x7F]/', $target); + } + /** * Report on the client secret's expiry without standing in the way. * diff --git a/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php b/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php new file mode 100644 index 0000000..d223fa7 --- /dev/null +++ b/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php @@ -0,0 +1,123 @@ +hasParameter(self::LOGGER_PARAMETER)) { + return; + } + + $loggerId = $container->getParameter(self::LOGGER_PARAMETER); + $container->getParameterBag()->remove(self::LOGGER_PARAMETER); + + if (!is_string($loggerId)) { + return; + } + + // References in existing method calls have already been rewritten to concrete + // ids by ResolveReferencesToAliasesPass, which runs in the optimization phase + // ahead of this one. Comparing an alias against them would never match, so a + // logger configured by alias id — `logger` among them — would silently leave + // the ordering unsettled. Emitting the alias would be worse still: private + // aliases are removed after this pass, leaving a dangling reference. + // That pass has already rejected circular aliases, so this terminates. + while ($container->hasAlias($loggerId)) { + $loggerId = (string) $container->getAlias($loggerId); + } + + foreach ($container->getDefinitions() as $definition) { + $this->giveTheConfiguredLoggerTheLastWord($definition, $loggerId); + } + } + + /** + * Definitions are recognised by the call the bundle put there, not by their + * class. Checking classes would mean autoloading every class in the container, + * which is fatal for a consumer whose container names a class from a package it + * has not installed. + */ + private function giveTheConfiguredLoggerTheLastWord(Definition $definition, string $loggerId): void + { + /** @var list $calls */ + $calls = $definition->getMethodCalls(); + + $kept = []; + $wanted = false; + + foreach ($calls as $call) { + if (self::METHOD === $call[0]) { + // Every existing call goes, the configured one being appended below. + // Keeping FrameworkBundle's would put it after ours again. + $wanted = $wanted || $this->references($call[1], $loggerId); + + continue; + } + + $kept[] = $call; + } + + // Without the bundle's own call this is some other LoggerAware service, or an + // authenticator whose consumer turned autoconfiguration off — which the README + // documents as opting out of the bundle's logging. Neither is ours to change, + // and returning here leaves the definition exactly as it was. + if (!$wanted) { + return; + } + + $kept[] = [self::METHOD, [new Reference($loggerId)]]; + + $definition->setMethodCalls($kept); + } + + /** + * @param mixed[] $arguments + */ + private function references(array $arguments, string $loggerId): bool + { + return ($arguments[0] ?? null) instanceof Reference && $loggerId === (string) $arguments[0]; + } +} diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index d5372a2..bd01dc1 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -122,8 +122,17 @@ public function getConfigTreeBuilder(): TreeBuilder // ClientSecretExpiryChecker. It never caught whitespace-only // values regardless: ScalarNode::isValueEmpty() is // `null === $value || '' === $value`. - ->info('Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. An expired secret breaks every login, so configuring this lets the bundle warn while there is still time to rotate. Will be required in 6.0.') - ->defaultNull() + ->info('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.') + ->validate() + // YAML reads an unquoted 2027-01-31 as the integer 1801353600, and the + // closure below only inspects strings, so without this the most natural + // way to write the value would pass, be discarded as untyped, and leave + // the provider unmonitored with nothing logged. Leaving the key out + // entirely is a decision and reports "unknown"; writing a value that + // cannot be one is a mistake, including an explicit null. + ->ifTrue(static fn (mixed $v): bool => !is_string($v)) + ->thenInvalid('client_secret_expires_at must be a string. YAML reads an unquoted date as a number, so quote it: "2027-01-31". From an environment variable, cast it as %%env(string:NAME)%%. Got %s.') + ->end() ->validate() // '' is exempt because it is the dummy fixture Symfony // substitutes for %env(string:...)% while compiling @@ -138,6 +147,10 @@ public function getConfigTreeBuilder(): TreeBuilder // a timestamp rather than false, the same "blank means now" // quirk DateTimeImmutable has, so whitespace would otherwise // sail through as a valid date. + // is_string() is unreachable-looking now that the closure above + // rejects non-strings, but it is what lets trim() and strtotime() + // take a mixed value under PHPStan, and it keeps this closure + // correct on its own terms rather than by ordering. ->ifTrue(static fn (mixed $v): bool => is_string($v) && '' !== $v && ('' === trim($v) || false === strtotime($v))) ->thenInvalid('client_secret_expires_at must be a date parseable by strtotime(), e.g. "2027-01-31". Got %s.') ->end() @@ -161,6 +174,22 @@ public function getConfigTreeBuilder(): TreeBuilder ->arrayNode('redirect_route_parameters') ->info('Redirect route parameters') ->end() + ->scalarNode('callback_path') + ->info('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.') + // As on client_secret_expires_at: a validated node that also + // disallows empty values refuses environment variables, and the + // closure is the half worth keeping. + ->validate() + ->ifTrue(static fn (mixed $v): bool => !is_string($v)) + ->thenInvalid('callback_path must be a string, e.g. "/auth/callback". Got %s.') + ->end() + ->validate() + // '' is the fixture Symfony substitutes for a string + // environment variable while compiling, so it has to pass. + ->ifTrue(static fn (mixed $v): bool => is_string($v) && '' !== $v && !str_starts_with($v, '/')) + ->thenInvalid('callback_path must start with "/", e.g. "/auth/callback". Got %s.') + ->end() + ->end() ->booleanNode('allow_http') ->info('Whether to allow http or not (default: false)') ->defaultValue(false) @@ -189,6 +218,14 @@ public function getConfigTreeBuilder(): TreeBuilder ->validate() ->ifTrue(static fn (array $v) => isset($v['redirect_uri'], $v['redirect_route'])) ->thenInvalid('Only one of redirect_uri or redirect_route must be set.') + ->end() + ->validate() + // Without one of these there is no path to recognise a callback on, + // and since 6.0 that means the provider can never complete a login: + // supports() matches the configured callback path, not any path + // carrying state and code. + ->ifTrue(static fn (array $v) => !isset($v['redirect_uri']) && !isset($v['redirect_route']) && !isset($v['callback_path'])) + ->thenInvalid('One of redirect_uri, redirect_route or callback_path must be set: it is how a callback is recognised.') ->end() ->end() ->end() diff --git a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php index e906f08..1450545 100644 --- a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php +++ b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php @@ -4,6 +4,7 @@ use ItkDev\OpenIdConnectBundle\Command\UserLoginCommand; use ItkDev\OpenIdConnectBundle\Controller\LoginController; +use ItkDev\OpenIdConnectBundle\DependencyInjection\Compiler\ConfiguredLoggerPass; use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; use ItkDev\OpenIdConnectBundle\Log\AuthenticationAuditLogger; use ItkDev\OpenIdConnectBundle\Security\CliLoginTokenAuthenticator; @@ -103,10 +104,14 @@ private function configureLogging(ContainerBuilder $container, array $options): // `OpenIdLoginAuthenticator` is abstract and subclassed by the consuming // application, so its subclasses are services this extension cannot name. - // Autoconfiguration reaches them, and runs after FrameworkBundle's own - // LoggerAwareInterface pass, so the configured logger wins. + // Autoconfiguration reaches them. $container->registerForAutoconfiguration(OpenIdLoginAuthenticator::class) ->addMethodCall('setLogger', [$logger]); + + // Whether this call or FrameworkBundle's is the one that takes effect depends + // on bundle registration order: see ConfiguredLoggerPass, which reads this + // parameter and settles it. + $container->setParameter(ConfiguredLoggerPass::LOGGER_PARAMETER, $options['logger']); } /** @@ -140,7 +145,7 @@ private function configureAuditLogging(ContainerBuilder $container, array $optio } /** - * Wire the expiry checker, and nudge installations that have not set a date. + * Wire the expiry checker. * * @param array}> $providers * @param array{warning_days: int} $options @@ -151,19 +156,11 @@ private function configureSecretExpiry(ContainerBuilder $container, array $provi foreach ($providers as $providerKey => $provider) { $expiresAt = $provider['options']['client_secret_expires_at'] ?? null; + // The key is optional, so null is the ordinary "not monitored" case: + // ClientSecretExpiryChecker reports it as Unknown without logging, since + // an unset date is a choice rather than a fault. A value that is set but + // unusable is a different matter, and is reported at error. $expiryDates[$providerKey] = is_string($expiresAt) ? $expiresAt : null; - - if (null === $expiryDates[$providerKey]) { - // Symfony's setDeprecated() fires when a node *is* used, which is - // the inverse of what is needed: the point is to nudge the - // installations that have not set a date yet. - trigger_deprecation( - 'itk-dev/openid-connect-bundle', - '5.1', - 'Not configuring "client_secret_expires_at" for OIDC provider "%s" is deprecated. Without it the bundle cannot warn before the secret expires, and an expired secret breaks every login. It will be required in 6.0.', - $providerKey, - ); - } } $definition = $container->getDefinition(ClientSecretExpiryChecker::class); diff --git a/src/Exception/AuthenticationFailedException.php b/src/Exception/AuthenticationFailedException.php new file mode 100644 index 0000000..85f495f --- /dev/null +++ b/src/Exception/AuthenticationFailedException.php @@ -0,0 +1,17 @@ +extension || false === $this->extension) { - $this->extension = new ItkDevOpenIdConnectExtension(); + if ($this->extension instanceof ExtensionInterface) { + return $this->extension; } - return $this->extension; + // Reached when the property is null, or false as Symfony sets it for a bundle + // with no extension — this bundle always has one. + $extension = new ItkDevOpenIdConnectExtension(); + $this->extension = $extension; + + return $extension; + } + + #[\Override] + public function build(ContainerBuilder $container): void + { + parent::build($container); + + // Runs after the instanceof conditionals it has to correct, and does nothing + // unless the extension recorded a configured logger. + $container->addCompilerPass(new ConfiguredLoggerPass(), PassConfig::TYPE_BEFORE_REMOVING); } #[\Override] diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php index dabf67b..d3941ba 100644 --- a/src/Security/OpenIdConfigurationProviderManager.php +++ b/src/Security/OpenIdConfigurationProviderManager.php @@ -5,6 +5,7 @@ use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; +use Psr\Cache\CacheItemPoolInterface; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; use Symfony\Component\Routing\RouterInterface; @@ -13,9 +14,12 @@ class OpenIdConfigurationProviderManager /** @var array */ private array $providers = []; + /** @var array> */ + private array $redirectUriPaths = []; + /** * @param array{ - * default_providers_options: array, + * default_providers_options: array{cacheItemPool?: CacheItemPoolInterface}, * providers: array, + * callback_path?: string, * leeway?: int, * cache_duration?: int, * allow_http?: bool, @@ -50,6 +55,114 @@ public function getProviderKeys(): array return array_keys($this->config['providers']); } + /** + * The request path each provider's callback arrives on, keyed by provider. + * + * Derived from configuration rather than from a provider instance: + * `supports()` consults this on every request through the firewall, and + * building a provider pulls in discovery, HTTP client and cache configuration + * for no reason. Memoized for the same reason. + * + * @return array + */ + public function getRedirectUriPaths(): array + { + // Keyed by the routing context's base URL, not memoized flat: a generated + // route includes that base URL, and it differs between a proxied request + // carrying X-Forwarded-Prefix and a direct one. One frozen map could only + // ever match one of them. + $memoKey = $this->router->getContext()->getBaseUrl(); + + if (isset($this->redirectUriPaths[$memoKey])) { + return $this->redirectUriPaths[$memoKey]; + } + + $paths = []; + + foreach ($this->config['providers'] as $key => $options) { + $path = $this->derivePath($options); + + if (null !== $path) { + $paths[$key] = $path; + } + } + + return $this->redirectUriPaths[$memoKey] = $paths; + } + + /** + * Whether a request path is the callback path of a given provider. + * + * Takes the path as `$request->getBaseUrl().$request->getPathInfo()`, which is + * what lines up with every derivation. `getPathInfo()` alone does not: + * `Request::preparePathInfo()` strips `getBaseUrlReal()`, so it excludes both a + * subdirectory deployment's base path and any trusted `X-Forwarded-Prefix` — + * while a `redirect_uri`'s path contains the prefix as the identity provider sees + * it, and `UrlGenerator` prepends the routing context's base URL, which includes + * the trusted prefix. + */ + public function isCallbackPath(string $requestPath, string $providerKey): bool + { + $paths = $this->getRedirectUriPaths(); + + if (!isset($paths[$providerKey])) { + return false; + } + + // Case-sensitive: paths are, and an identity provider sends the browser to + // the redirect URI exactly as it was registered. + return $paths[$providerKey] === $this->normalizePath($requestPath); + } + + /** + * @param array{redirect_uri?: string, redirect_route?: string, redirect_route_parameters?: array, callback_path?: string} $options + */ + private function derivePath(array $options): ?string + { + // callback_path first: it exists precisely for deployments where the + // external redirect_uri path is not the path this application receives. + // '' passes configuration on purpose — it is the fixture Symfony substitutes + // for a string environment variable while compiling — so an environment + // variable that resolves to nothing arrives here. Normalizing it would make + // the site root the callback path and shadow redirect_uri. + if (isset($options['callback_path']) && '' !== $options['callback_path']) { + return $this->normalizePath($options['callback_path']); + } + + // Generated as a path, not a URL, so a reverse proxy's prefix handling is + // already accounted for by the router. + if (isset($options['redirect_route'])) { + return $this->normalizePath($this->router->generate( + $options['redirect_route'], + $options['redirect_route_parameters'] ?? [], + UrlGeneratorInterface::ABSOLUTE_PATH + )); + } + + if (isset($options['redirect_uri'])) { + // An external URL: its path is what the identity provider sends the + // browser to, which is the internal path only when nothing rewrites it. + $path = parse_url($options['redirect_uri'], \PHP_URL_PATH); + + // A redirect_uri with no path at all, or one that could not be parsed: + // the provider then answers at the application root. + return $this->normalizePath(is_string($path) ? $path : '/'); + } + + return null; + } + + /** + * Leading slash, no trailing slash, so that the comparison in `supports()` + * does not turn on how the value was written. + */ + private function normalizePath(string $path): string + { + $trimmed = rtrim('/'.ltrim($path, '/'), '/'); + + return '' === $trimmed ? '/' : $trimmed; + } + /** * Get a provider by key. * @@ -91,7 +204,6 @@ public function getProvider(string $key): OpenIdConfigurationProvider $providerOptions += $options['http_client_options']; } - // @phpstan-ignore argument.type (library 5.0 narrowed $options to a strict array shape; the incremental build above is verified by the manager's tests but PHPStan can't track its evolution to the final shape) $this->providers[$key] = new OpenIdConfigurationProvider($providerOptions); } diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index d4a6298..ee9cd81 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -5,18 +5,26 @@ use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnect\Exception\ValidationException; use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; +use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Http\Authenticator\AbstractAuthenticator; use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface; +use Symfony\Component\Security\Http\Util\TargetPathTrait; /** * Authenticator for OpenId Connect login. * + * A failed callback throws `AuthenticationFailedException`, which is not an + * `AuthenticationException` and so is not turned back into another redirect to the + * identity provider. Consuming applications see a 500 and can render whatever they + * like from it; what they no longer see is an unbreakable redirect loop. + * * The logger is injected through `setLogger()` rather than the constructor on * purpose: this class is extended by consuming applications, whose subclasses * call `parent::__construct($providerManager)`. Adding a constructor argument @@ -39,6 +47,17 @@ */ abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements AuthenticationEntryPointInterface, LoggerAwareInterface { + use TargetPathTrait; + + /** + * Where `LoginController` puts a target named on the login link itself. + * + * Not `TargetPathTrait`'s key, which is per firewall: the controller has no + * firewall name, and inventing one to write into Symfony's slot would put a value + * there that the firewall never saved. + */ + public const string TARGET_PATH_SESSION_KEY = '_itkdev_oidc.target_path'; + private LoggerInterface $logger; /** @@ -55,10 +74,96 @@ public function setLogger(LoggerInterface $logger): void $this->logger = $logger; } + /** + * Whether this request is a callback for one of this authenticator's providers. + * + * `state` and `code` alone used to be enough, which made every URL under the + * firewall a callback: anyone could turn any page into a failed login, and since + * the bundle fails closed that means a 500 raised by an unauthenticated caller. + * Requiring the configured callback path as well leaves a forged callback to the + * firewall's ordinary handling. + * + * Nothing here touches the session. This runs on every request through the + * firewall, so starting a session for anonymous traffic would be a real cost, and + * "is this a callback" must not depend on whether this browser began the login. + * The session's provider key is still what decides which provider validates it, + * in `validateClaims()`. + */ public function supports(Request $request): ?bool { - // Check if request has state and code - return $request->query->has('state') && $request->query->has('code'); + if (!$request->query->has('state') || !$request->query->has('code')) { + return false; + } + + // Base URL included: getPathInfo() has any subdirectory base path and trusted + // proxy prefix stripped out, while the configured paths contain them. See + // OpenIdConfigurationProviderManager::isCallbackPath(). + $path = $request->getBaseUrl().$request->getPathInfo(); + + foreach ($this->getSupportedProviderKeys() as $providerKey) { + if ($this->providerManager->isCallbackPath($path, $providerKey)) { + return true; + } + } + + return false; + } + + /** + * Redirect to the page the user originally asked for. + * + * Symfony saves that page when the entry point fires, which covers both shapes of + * consumer: one that redirects straight to the identity provider, and one that + * shows a login screen carrying a link to it. `$fallbackUrl` is for a user who + * reached the login link without being sent there — nothing was saved then. + * + * The saved path is cleared on use, so a later visit to the login link does not + * replay a stale target. + */ + protected function createTargetPathRedirect(Request $request, string $firewallName, string $fallbackUrl): RedirectResponse + { + $session = $request->getSession(); + + // The firewall's record first: it is the page the user was actually denied. + $targetPath = $this->getTargetPath($session, $firewallName); + + if (null !== $targetPath && '' !== $targetPath) { + $this->removeTargetPath($session, $firewallName); + $session->remove(self::TARGET_PATH_SESSION_KEY); + + return new RedirectResponse($targetPath); + } + + // Then a target named on the login link, for a user who was never denied + // anything — they followed a login link from a public page. + $named = $session->get(self::TARGET_PATH_SESSION_KEY); + $session->remove(self::TARGET_PATH_SESSION_KEY); + + if (is_string($named) && '' !== $named) { + return new RedirectResponse($named); + } + + return new RedirectResponse($fallbackUrl); + } + + /** + * Provider keys whose callbacks this authenticator answers. + * + * Every configured provider by default, which is what keeps several + * `OpenIdLoginAuthenticator` subclasses on one firewall working as they do + * today: each supports every callback path, Symfony asks them in the order + * `security.yaml` lists them, and the session's provider key decides which + * provider validates the callback. + * + * Override in a subclass bound to particular providers so that, with a distinct + * callback path per provider, each callback is answered by the authenticator that + * owns it. + * + * @return string[] + */ + protected function getSupportedProviderKeys(): array + { + return $this->providerManager->getProviderKeys(); } /** @@ -139,14 +244,52 @@ protected function validateClaims(Request $request): array public function onAuthenticationFailure(Request $request, AuthenticationException $exception): ?Response { - // Deliberately not logged here. `AuthenticatorManager` already logs the - // original exception at info before it swaps sensitive causes for a generic - // `BadCredentialsException`, and the specific reason was logged by - // `validateClaims()`. A record here would be the third for one failure. + // Not an AuthenticationException, and that is the entire point. The + // security component catches those and hands control back to this + // authenticator's own start(), which redirects to the identity provider + // again — so a callback that keeps failing keeps being retried. That is the + // loop that took sites.itkdev.dk down for the duration of an expired client + // secret. AuthenticatorManager::executeAuthenticator() catches only + // AuthenticationException, so this propagates to HttpKernel instead and the + // application renders its own error. // - // Preserve the cause so logs and error reporters can see what actually - // failed (timeout, signature mismatch, wrong nonce, etc.). Symfony's - // security component renders only the safe message key to the user. - throw new AuthenticationException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), $exception); + // Still not logged here: AuthenticatorManager has already logged the + // original exception, validateClaims() logged the specific reason, and the + // application's error handling logs whatever escapes. A record here would be + // the fourth for one failure. + throw new AuthenticationFailedException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), self::causeOutsideSecurity($exception)); + } + + /** + * The first cause carrying no `AuthenticationException` anywhere beneath it. + * + * Changing the thrown type is not enough on its own: the security + * `ExceptionListener` walks the whole `$previous` chain, so chaining the + * `AuthenticationException` it handed us would put one back within reach and it + * would redirect to the entry point regardless — the loop restored by the cause + * instead of by the type. The library exception underneath carries the reason + * worth keeping, and `validateClaims()` has already logged it with the full + * chain attached. + */ + private static function causeOutsideSecurity(\Throwable $exception): ?\Throwable + { + for ($cause = $exception->getPrevious(); null !== $cause; $cause = $cause->getPrevious()) { + if (!self::containsSecurityException($cause)) { + return $cause; + } + } + + return null; + } + + private static function containsSecurityException(\Throwable $exception): bool + { + for ($current = $exception; null !== $current; $current = $current->getPrevious()) { + if ($current instanceof AuthenticationException) { + return true; + } + } + + return false; } } diff --git a/src/Util/ClientSecretExpiryChecker.php b/src/Util/ClientSecretExpiryChecker.php index 95db216..0e565a3 100644 --- a/src/Util/ClientSecretExpiryChecker.php +++ b/src/Util/ClientSecretExpiryChecker.php @@ -85,9 +85,12 @@ public function getStatus(string $providerKey): ClientSecretExpiry /** * Report a date that cannot be used. * - * Neither an empty nor a malformed value can be rejected when the container - * compiles: this comes from the environment in every real deployment, and - * Symfony will not accept an environment variable on a node that is validated. + * A literal is rejected when the container compiles, but the value comes from + * the environment in every real deployment and is still an unresolved + * placeholder then, so what it resolves to can only be judged here. Validation + * closures and environment variables do coexist on that node; what Symfony + * refuses is a validated node that also disallows empty values, which is why + * `cannotBeEmpty()` is absent from it. * So it is reported rather than thrown — a mistyped date must not take an * application down — but reported loudly, because the effect is that nothing is * monitoring this secret, and silence would equal not having the feature. diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index fa1ed9b..10c6641 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -9,6 +9,7 @@ use ItkDev\OpenIdConnectBundle\Controller\LoginController; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; +use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; use ItkDev\OpenIdConnectBundle\Util\ClientSecretExpiryChecker; use PHPUnit\Framework\Attributes\DataProvider; @@ -16,7 +17,9 @@ use Psr\Log\LogLevel; use Symfony\Component\Clock\MockClock; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException; @@ -264,4 +267,98 @@ private function createController(OpenIdConfigurationProvider $provider, ?Client return new LoginController($mockProviderManager, $this->logger, $expiryChecker ?? $this->createExpiryChecker()); } + + private function loginWith(?string $target): Session + { + // A stub, not a mock: nothing here asserts on the provider itself. + $provider = $this->createStub(OpenIdConfigurationProvider::class); + $provider->method('generateNonce')->willReturn('1234'); + $provider->method('generateState')->willReturn('abcd'); + $provider->method('getAuthorizationUrl')->willReturn('https://provider.example.org/authorize'); + + $query = ['provider' => 'test']; + + if (null !== $target) { + $query[LoginController::TARGET_PATH_PARAMETER] = $target; + } + + $session = new Session(new MockArraySessionStorage()); + $this->createController($provider)->login(new Request(query: $query), $session, 'test'); + + return $session; + } + + public function testATargetPathOnTheLinkIsRemembered(): void + { + $session = $this->loginWith('/admin/reports?page=2'); + + $this->assertSame('/admin/reports?page=2', $session->get(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + $this->assertSame([], $this->logger->records); + } + + public function testWithoutATargetPathNothingIsRememberedOrLogged(): void + { + $session = $this->loginWith(null); + + $this->assertFalse($session->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + $this->assertSame([], $this->logger->records); + } + + /** + * The last login link wins. A target left behind by an abandoned link would + * otherwise be spent by whatever login came next, sending the user somewhere they + * did not ask for this time. + */ + public function testAPlainLoginLinkForgetsAnEarlierTarget(): void + { + $provider = $this->createStub(OpenIdConfigurationProvider::class); + $provider->method('generateNonce')->willReturn('1234'); + $provider->method('generateState')->willReturn('abcd'); + $provider->method('getAuthorizationUrl')->willReturn('https://provider.example.org/authorize'); + + $session = new Session(new MockArraySessionStorage()); + $session->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, '/admin/abandoned'); + + $this->createController($provider)->login(new Request(query: ['provider' => 'test']), $session, 'test'); + + $this->assertFalse($session->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + } + + /** + * This value reaches a `Location` header after a successful login, so anything + * that is not plainly a path inside this application would make the login route + * an open redirect for anyone who can get a user to follow a link. + * + * @return iterable + */ + public static function unusableTargetPathProvider(): iterable + { + yield 'absolute url' => ['https://evil.example.org/phish']; + yield 'scheme relative' => ['//evil.example.org/phish']; + yield 'backslash scheme relative' => ['/\evil.example.org/phish']; + yield 'backslash anywhere' => ['/admin\reports']; + yield 'a scheme further in' => ['/redirect?to=https://evil.example.org']; + yield 'no leading slash' => ['admin/reports']; + yield 'empty' => ['']; + yield 'a bare word' => ['dashboard']; + yield 'header split attempt' => ["/admin\r\nSet-Cookie: session=stolen"]; + yield 'null byte' => ["/admin\0/reports"]; + yield 'javascript' => ['javascript:alert(1)']; + } + + #[DataProvider('unusableTargetPathProvider')] + public function testAnUnusableTargetPathIsDroppedAndReported(string $target): void + { + $session = $this->loginWith($target); + + $this->assertFalse( + $session->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY), + 'A value that is not a local path must never reach a Location header' + ); + + $record = $this->logger->singleRecord(); + $this->assertSame(LogLevel::WARNING, $record['level']); + $this->assertStringContainsString('ignoring an unusable target_path', $record['message']); + $this->assertSame($target, $record['context']['target_path']); + } } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index a14fd40..031d49b 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -35,6 +35,8 @@ private function getMinimalConfig(): array 'metadata_url' => 'https://example.com/.well-known/openid-configuration', 'client_id' => 'my_id', 'client_secret' => 'my_secret', + 'client_secret_expires_at' => '2027-01-31', + 'redirect_uri' => 'https://app.example.org/callback_uri', ], ], ], @@ -70,8 +72,7 @@ public function testMinimalConfig(): void $this->assertNull($config['audit_options']['logger']); $this->assertSame(AuthenticationAuditLogger::IDENTIFIER_RAW, $config['audit_options']['identifier']); - // No expiry date yet, and a 30-day default warning window. - $this->assertNull($provider['client_secret_expires_at']); + $this->assertSame('2027-01-31', $provider['client_secret_expires_at']); $this->assertSame(30, $config['secret_expiry_options']['warning_days']); } @@ -105,27 +106,18 @@ public function testClientSecretExpiresAtRejectsUnparseableLiterals(string $date $this->processor->processConfiguration($this->configuration, [$input]); } - /** - * @return iterable - */ - public static function toleratedEmptyDateProvider(): iterable - { - // '' is the fixture Symfony substitutes for a string env var while - // compiling, so it has to pass here; the checker reports it at runtime. - yield 'empty string' => ['']; - // An explicit null is a deliberate "not configured", not a typo. - yield 'explicit null' => [null]; - } - - #[DataProvider('toleratedEmptyDateProvider')] - public function testClientSecretExpiresAtToleratesEmptyValues(?string $date): void + public function testClientSecretExpiresAtToleratesAnEmptyString(): void { + // '' is the fixture Symfony substitutes for a string env var while compiling, + // so it has to pass here; the checker reports it at runtime. An explicit null + // is no longer tolerated — see testANonStringExpiryDateIsRejected. It used to + // mean "not configured", which is not a thing a required option has. $input = $this->getMinimalConfig(); - $input['openid_providers']['provider1']['options']['client_secret_expires_at'] = $date; + $input['openid_providers']['provider1']['options']['client_secret_expires_at'] = ''; $config = $this->processor->processConfiguration($this->configuration, [$input]); - $this->assertSame($date, $config['openid_providers']['provider1']['options']['client_secret_expires_at']); + $this->assertSame('', $config['openid_providers']['provider1']['options']['client_secret_expires_at']); } public function testClientSecretExpiresAtAccepted(): void @@ -261,6 +253,8 @@ public function testFullConfig(): void public function testRedirectRouteConfig(): void { $input = $this->getMinimalConfig(); + // Mutually exclusive with redirect_uri, which the minimal config sets. + unset($input['openid_providers']['provider1']['options']['redirect_uri']); $input['openid_providers']['provider1']['options']['redirect_route'] = 'my_redirect_route'; $config = $this->processor->processConfiguration( @@ -356,6 +350,88 @@ public function testProviderKeysAreNotNormalized(): void $this->assertArrayNotHasKey('my_provider', $config['openid_providers']); } + /** + * The definition itself must be free of deprecations. + * + * Symfony reports a contradictory definition — a required node that also carries + * a default, say — by deprecating it rather than refusing it, and + * `trigger_deprecation()` raises that with `@`, which PHPUnit's own + * `failOnDeprecation` respects and therefore never sees. A handler installed here + * does see it. Otherwise the first report comes from a consuming application's + * console, which is where this one was found. + */ + public function testTheDefinitionEmitsNoDeprecations(): void + { + $deprecations = []; + // All four arguments are forwarded: the handler being wrapped is PHPUnit's, + // whose __invoke() requires file and line. + $previous = set_error_handler(static function (int $level, string $message, string $file = '', int $line = 0) use (&$deprecations, &$previous): bool { + if (\E_USER_DEPRECATED === $level) { + $deprecations[] = $message; + + return true; + } + + return null !== $previous && false !== ($previous)($level, $message, $file, $line); + }); + + try { + $this->processor->processConfiguration($this->configuration, [$this->getMinimalConfig()]); + } finally { + restore_error_handler(); + } + + $this->assertSame([], $deprecations); + } + + /** + * The value a reader would most likely write. + * + * `client_secret_expires_at: 2027-01-31` without quotes is the integer + * 1801353600 by the time configuration sees it. Accepting it would discard the + * date and leave the provider unmonitored with nothing logged anywhere, which is + * the exact outcome this option exists to prevent. + * + * @return iterable + */ + public static function nonStringDateProvider(): iterable + { + yield 'unquoted date, read as a timestamp' => [1801353600]; + yield 'digits' => [20270131]; + yield 'boolean' => [true]; + yield 'explicit null, which isRequired() accepts' => [null]; + } + + #[DataProvider('nonStringDateProvider')] + public function testANonStringExpiryDateIsRejected(mixed $configured): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['client_secret_expires_at'] = $configured; + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('client_secret_expires_at must be a string'); + + $this->processor->processConfiguration($this->configuration, [$input]); + } + + /** + * Optional on purpose. A required key can force a value, never a correct one, and + * it cannot be scoped to the environment where the real secret lives — Symfony + * compiles a container per environment, so a required node has to appear in all of + * them. What that produces is a date in a committed default, which reports `ok` + * forever while monitoring nothing. Unset is the honest state, and it is visible: + * the provider reports `unknown`. + */ + public function testTheExpiryDateIsOptional(): void + { + $input = $this->getMinimalConfig(); + unset($input['openid_providers']['provider1']['options']['client_secret_expires_at']); + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertArrayNotHasKey('client_secret_expires_at', $config['openid_providers']['provider1']['options']); + } + public function testMultipleProviders(): void { $input = $this->getMinimalConfig(); @@ -364,6 +440,8 @@ public function testMultipleProviders(): void 'metadata_url' => 'https://other-provider.example.org/.well-known/openid-configuration', 'client_id' => 'other_id', 'client_secret' => 'other_secret', + 'client_secret_expires_at' => '2028-06-30', + 'redirect_uri' => 'https://app.example.org/other_callback', ], ]; @@ -376,4 +454,76 @@ public function testMultipleProviders(): void $this->assertArrayHasKey('provider1', $config['openid_providers']); $this->assertArrayHasKey('provider2', $config['openid_providers']); } + + /** + * @return iterable + */ + public static function invalidCallbackPathProvider(): iterable + { + yield 'not a string' => [42, 'callback_path must be a string']; + yield 'null' => [null, 'callback_path must be a string']; + yield 'no leading slash' => ['auth/callback', 'callback_path must start with "/"']; + yield 'a full url' => ['https://app.example.org/auth/callback', 'callback_path must start with "/"']; + } + + #[DataProvider('invalidCallbackPathProvider')] + public function testAnInvalidCallbackPathIsRejected(mixed $configured, string $expectedMessage): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['callback_path'] = $configured; + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage($expectedMessage); + + $this->processor->processConfiguration($this->configuration, [$input]); + } + + /** + * @return iterable + */ + public static function validCallbackPathProvider(): iterable + { + yield 'a path' => ['/auth/callback']; + yield 'the root' => ['/']; + // As on client_secret_expires_at: '' is the fixture Symfony substitutes for a + // string environment variable while compiling, so it must pass here. + yield 'the environment variable fixture' => ['']; + } + + #[DataProvider('validCallbackPathProvider')] + public function testAValidCallbackPathIsAccepted(string $configured): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['callback_path'] = $configured; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame($configured, $config['openid_providers']['provider1']['options']['callback_path']); + } + + /** + * A provider that declares no callback target cannot recognise a callback, so it + * could never complete a login. + */ + public function testAProviderMustDeclareACallbackTarget(): void + { + $input = $this->getMinimalConfig(); + unset($input['openid_providers']['provider1']['options']['redirect_uri']); + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('One of redirect_uri, redirect_route or callback_path must be set'); + + $this->processor->processConfiguration($this->configuration, [$input]); + } + + public function testCallbackPathAloneSatisfiesTheRequirement(): void + { + $input = $this->getMinimalConfig(); + unset($input['openid_providers']['provider1']['options']['redirect_uri']); + $input['openid_providers']['provider1']['options']['callback_path'] = '/auth/callback'; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame('/auth/callback', $config['openid_providers']['provider1']['options']['callback_path']); + } } diff --git a/tests/DependencyInjection/ConfiguredLoggerPassTest.php b/tests/DependencyInjection/ConfiguredLoggerPassTest.php new file mode 100644 index 0000000..3e4db16 --- /dev/null +++ b/tests/DependencyInjection/ConfiguredLoggerPassTest.php @@ -0,0 +1,200 @@ +captureExceptionHandler(); + $this->kernel = $this->boot('itkdev_openid_connect_configured_logger.yml'); + } + + protected function tearDown(): void + { + $this->restoreExceptionHandlers(); + } + + private function boot(string $bundleConfig): ItkDevOpenIdConnectBundleTestingKernel + { + $kernel = new ItkDevOpenIdConnectBundleTestingKernel([ + __DIR__.'/../config/framework.yml', + __DIR__.'/../config/framework_routing.yml', + __DIR__.'/../config/security_consumer.yml', + __DIR__.'/../config/'.$bundleConfig, + ]); + $kernel->boot(); + + return $kernel; + } + + private function configuredLogger(): TestLogger + { + $logger = $this->kernel->getContainer()->get(TestLogger::class); + $this->assertInstanceOf(TestLogger::class, $logger); + + return $logger; + } + + public function testTheBuiltAuthenticatorHoldsTheConfiguredLogger(): void + { + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $logger = (new \ReflectionProperty(OpenIdLoginAuthenticator::class, 'logger'))->getValue($authenticator); + + $this->assertSame( + $this->configuredLogger(), + $logger, + 'The application logger overwrote the configured one, so logging_options.logger turns on bundle registration order.' + ); + } + + /** + * The same thing again through behaviour, because holding the right object and + * writing to it are not quite the same claim. + */ + public function testAFailedLoginIsWrittenToTheConfiguredLogger(): void + { + $request = Request::create('/callback_uri?state=does-not-match&code=some-code'); + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'the-real-state'); + $session->set('oauth2nonce', 'the-real-nonce'); + $request->setSession($session); + + $this->kernel->handle($request, catch: true); + + $this->assertContains( + 'OIDC login failed: invalid state', + array_column($this->configuredLogger()->records, 'message'), + ); + } + + /** + * An alias is a service id like any other as far as configuration goes, but by + * the time this pass runs the references it is compared against have already been + * rewritten to the concrete id — so matching on the alias would quietly fail and + * leave exactly the ordering dependency this pass exists to remove. + */ + public function testALoggerConfiguredByAliasIsResolved(): void + { + $this->kernel = $this->boot('itkdev_openid_connect_alias_logger.yml'); + + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $logger = (new \ReflectionProperty(OpenIdLoginAuthenticator::class, 'logger'))->getValue($authenticator); + + $this->assertSame($this->configuredLogger(), $logger); + } + + /** + * A service that opts out of autoconfiguration keeps its NullLogger: the pass + * must not decide to start logging on a consumer's behalf. + */ + public function testAServiceWithoutAutoconfigurationIsLeftAlone(): void + { + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class.'.not_autoconfigured'); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $logger = (new \ReflectionProperty(OpenIdLoginAuthenticator::class, 'logger'))->getValue($authenticator); + + $this->assertInstanceOf(LoggerInterface::class, $logger); + $this->assertNotSame($this->configuredLogger(), $logger); + } + + public function testTheParameterDoesNotLingerInTheContainer(): void + { + $this->assertFalse( + $this->kernel->getContainer()->hasParameter(ConfiguredLoggerPass::LOGGER_PARAMETER), + 'The pass consumes its parameter rather than leaving it in a consumer container' + ); + } + + /** + * @param array}> $calls + */ + private function process(array $calls, string|array|null $parameter = 'configured.logger'): Definition + { + $container = new ContainerBuilder(); + if (null !== $parameter) { + $container->setParameter(ConfiguredLoggerPass::LOGGER_PARAMETER, $parameter); + } + $definition = $container->register('some.service', \stdClass::class); + $definition->setMethodCalls($calls); + + (new ConfiguredLoggerPass())->process($container); + + return $definition; + } + + public function testAParameterThatIsNotAServiceIdIsIgnored(): void + { + $calls = [['setLogger', [new Reference('configured.logger')]]]; + + $this->assertEquals($calls, $this->process($calls, parameter: ['not', 'a', 'service id'])->getMethodCalls()); + } + + public function testNoParameterMeansNoConfiguredLogger(): void + { + $calls = [['setLogger', [new Reference('logger')]]]; + + $this->assertEquals($calls, $this->process($calls, parameter: null)->getMethodCalls()); + } + + public function testAnUnrelatedLoggerAwareServiceKeepsItsCall(): void + { + // Every LoggerAware service in the application carries this call. Only the + // ones the bundle also wrote to are ours to rewrite. + $calls = [['setLogger', [new Reference('logger')]]]; + + $this->assertEquals($calls, $this->process($calls)->getMethodCalls()); + } + + public function testTheConfiguredCallIsMovedLastAndTheOtherOneDropped(): void + { + $definition = $this->process([ + ['setLogger', [new Reference('configured.logger')]], + ['setDependency', []], + ['setLogger', [new Reference('logger')]], + ]); + + $this->assertEquals([ + ['setDependency', []], + ['setLogger', [new Reference('configured.logger')]], + ], $definition->getMethodCalls()); + } +} diff --git a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php index a0db5a0..30b40c3 100644 --- a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php +++ b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php @@ -36,6 +36,7 @@ private function getBaseConfig(?string $userProvider = null): array 'client_id' => 'test_id', 'client_secret' => 'test_secret', 'client_secret_expires_at' => '2027-01-31', + 'redirect_uri' => 'https://app.example.org/callback_uri', ], ], ], @@ -184,6 +185,35 @@ public function testAuditOptionsAreWired(): void $this->assertSame('%kernel.secret%', $arguments['$identifierSecret']); } + public function testAProviderWithoutAnExpiryDateIsWiredAsUnmonitored(): void + { + $extension = new ItkDevOpenIdConnectExtension(); + $container = new ContainerBuilder(); + + $config = $this->getBaseConfig(); + // Built without the date rather than unset from the base config, which is + // untyped and would need narrowing for no gain. + $config['openid_providers'] = [ + 'test_provider' => [ + 'options' => [ + 'metadata_url' => 'https://example.com/.well-known/openid-configuration', + 'client_id' => 'test_id', + 'client_secret' => 'test_secret', + 'redirect_uri' => 'https://app.example.org/callback_uri', + ], + ], + ]; + + $extension->load([$config], $container); + + // null, not absent: the checker reports every configured provider, and one + // without a date has to be reportable as `unknown` rather than missing. + $this->assertSame( + ['test_provider' => null], + $container->getDefinition(ClientSecretExpiryChecker::class)->getArgument('$expiryDates') + ); + } + public function testSecretExpiryIsWired(): void { $extension = new ItkDevOpenIdConnectExtension(); @@ -218,61 +248,6 @@ public function testExpiryDateIsStrippedBeforeReachingTheProviderManager(): void $this->assertSame('test_secret', $provider['client_secret']); } - public function testMissingExpiryDateTriggersADeprecation(): void - { - $extension = new ItkDevOpenIdConnectExtension(); - $container = new ContainerBuilder(); - - $config = $this->getBaseConfig(); - // Built without the date rather than unset from the base config, which is - // untyped and would need narrowing for no gain. - $config['openid_providers'] = [ - 'test_provider' => [ - 'options' => [ - 'metadata_url' => 'https://example.com/.well-known/openid-configuration', - 'client_id' => 'test_id', - 'client_secret' => 'test_secret', - ], - ], - ]; - - $this->expectUserDeprecationMessage('Since itk-dev/openid-connect-bundle 5.1: Not configuring "client_secret_expires_at" for OIDC provider "test_provider" is deprecated. Without it the bundle cannot warn before the secret expires, and an expired secret breaks every login. It will be required in 6.0.'); - - $extension->load([$config], $container); - - $expiryDates = $container->getDefinition(ClientSecretExpiryChecker::class)->getArgument('$expiryDates'); - $this->assertIsArray($expiryDates); - $this->assertNull($expiryDates['test_provider'], 'An unset date is recorded as unknown, not guessed at'); - } - - public function testConfiguredExpiryDateTriggersNoDeprecation(): void - { - $extension = new ItkDevOpenIdConnectExtension(); - $container = new ContainerBuilder(); - - $deprecations = []; - // All four arguments must be forwarded: the handler being wrapped is - // PHPUnit's, whose __invoke() requires file and line. Passing two worked - // only while nothing else raised an error during the call. - $previous = set_error_handler(static function (int $level, string $message, string $file = '', int $line = 0) use (&$deprecations, &$previous): bool { - if (\E_USER_DEPRECATED === $level) { - $deprecations[] = $message; - - return true; - } - - return null !== $previous && false !== ($previous)($level, $message, $file, $line); - }); - - try { - $extension->load([$this->getBaseConfig()], $container); - } finally { - restore_error_handler(); - } - - $this->assertSame([], $deprecations, 'An installation that has set the date must not be nagged'); - } - public function testLoadWiresProviderManagerConfig(): void { $extension = new ItkDevOpenIdConnectExtension(); diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index 9ee9a93..3e140fd 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -4,12 +4,11 @@ use ItkDev\OpenIdConnect\Exception\HttpException as LibraryHttpException; use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; +use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\CacheException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; -use ItkDev\OpenIdConnectBundle\Exception\ItkOpenIdConnectBundleException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException; -use ItkDev\OpenIdConnectBundle\Exception\UserDoesNotExistException; use ItkDev\OpenIdConnectBundle\Exception\UsernameDoesNotExistException; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -41,7 +40,7 @@ public static function concreteProvider(): iterable // Runtime conditions → \RuntimeException yield 'CacheException' => [CacheException::class, \RuntimeException::class]; yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class]; - yield 'UserDoesNotExistException' => [UserDoesNotExistException::class, \RuntimeException::class]; + yield 'AuthenticationFailedException' => [AuthenticationFailedException::class, \RuntimeException::class]; } /** @@ -114,20 +113,4 @@ public function testLibraryMarkerCatchesBothPackages(): void $this->assertSame([LibraryHttpException::class, CacheException::class], $caught); } - - public function testDeprecatedAbstractBaseImplementsBundleMarker(): void - { - // `ItkOpenIdConnectBundleException` is kept as a deprecated alias through 5.x. - // Concrete bundle exceptions no longer extend it, but it still implements the - // marker so any consumer-defined subclass remains catchable via the marker. - // PHPStan can statically prove the assertion holds today; the test exists so - // the day a refactor removes the implements, the failure is loud. - $deprecated = ItkOpenIdConnectBundleException::class; // @phpstan-ignore classConstant.deprecatedClass (the test asserts a property of this deprecated class on purpose) - // @phpstan-ignore method.alreadyNarrowedType (the assertion is the guard — PHPStan proves it today; the test fails the day the guard stops holding) - $this->assertTrue( - // @phpstan-ignore function.alreadyNarrowedType (same as above — the static proof IS the contract being asserted) - is_subclass_of($deprecated, OpenIdConnectBundleExceptionInterface::class), - 'Deprecated abstract base must continue to implement the bundle marker through 5.x.', - ); - } } diff --git a/tests/ItkDevOpenIdConnectBundleTestingKernel.php b/tests/ItkDevOpenIdConnectBundleTestingKernel.php index ff0acb0..38bb378 100644 --- a/tests/ItkDevOpenIdConnectBundleTestingKernel.php +++ b/tests/ItkDevOpenIdConnectBundleTestingKernel.php @@ -8,6 +8,8 @@ namespace ItkDev\OpenIdConnectBundle\Tests; use ItkDev\OpenIdConnectBundle\ItkDevOpenIdConnectBundle; +use ItkDev\OpenIdConnectBundle\Tests\Security\ConsumerAuthenticator; +use ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController; use ItkDev\OpenIdConnectBundle\Tests\Security\TestAuthenticator; use Symfony\Bundle\FrameworkBundle\FrameworkBundle; use Symfony\Bundle\SecurityBundle\SecurityBundle; @@ -30,19 +32,45 @@ public function __construct( } /** - * A cache directory per config set. + * A cache directory per config set, and under Infection per process. * - * Without this every kernel in the suite shares `var/cache/test`, so the first - * container compiled is the one every later test gets — silently, and with - * whatever configuration that first test happened to use. Any test that boots a - * different configuration is then asserting against the wrong container. + * Per config set, because otherwise every kernel in the suite shares + * `var/cache/test`: the first container compiled is the one every later test + * gets, silently, with whatever configuration that first test happened to use. + * + * Per process under Infection, because it substitutes a mutated file through an + * include interceptor rather than by writing to disk. Nothing Symfony tracks as a + * resource changes, so a mutant is served the cached container and every mutation + * of compile-time code survives by default. Each mutant runs in its own process, + * so the pid separates them. Plain runs stay on the shared directory: they have + * nothing to isolate, and a recompile per process is a cost with no return. */ #[\Override] public function getCacheDir(): string { - return parent::getCacheDir().'/'.substr(hash('xxh128', implode('|', $this->pathToConfigs)), 0, 12); + $key = substr(hash('xxh128', implode('|', $this->pathToConfigs)), 0, 12); + + if (false !== getenv('INFECTION')) { + $key .= '-'.getmypid(); + } + + return parent::getCacheDir().'/'.$key; } + /** + * This bundle is registered before FrameworkBundle deliberately. It is the + * unconventional order, and the one where autoconfigured method calls land in the + * losing order — so it is the order that holds ConfiguredLoggerPass to its job. + * + * The return is annotated with the three bundle classes rather than inherited as + * `iterable`: Symfony 8.1 deprecates + * `HttpKernel\Bundle\BundleInterface` in favour of + * `DependencyInjection\Kernel\BundleInterface`, and naming either one would break + * on the other end of the supported range. The concrete classes are covariant with + * both, and more precise than either. + * + * @return list + */ public function registerBundles(): iterable { return [ @@ -59,9 +87,25 @@ public function registerContainerConfiguration(LoaderInterface $loader): void { $loader->load(function (ContainerBuilder $builder) { $builder->register(TestAuthenticator::class, TestAuthenticator::class); + // Autowired and autoconfigured, the way a consumer registers its own + // authenticator: autoconfiguration is what delivers the configured logger + // to `setLogger()`, and without it this fixture gets a NullLogger. + $builder->register(ConsumerAuthenticator::class, ConsumerAuthenticator::class) + ->setAutowired(true) + ->setAutoconfigured(true) + ->setPublic(true); + // A consumer who turned autoconfiguration off. Nothing calls setLogger on + // this one, and nothing should start. + $builder->register(ConsumerAuthenticator::class.'.not_autoconfigured', ConsumerAuthenticator::class) + ->setAutowired(true) + ->setPublic(true); + $builder->register(ProtectedController::class, ProtectedController::class)->setPublic(true); // Available as a logger a config fixture can point at, so a test can // read what the bundle actually wrote through the container. $builder->register(TestLogger::class, TestLogger::class)->setPublic(true); + // Aliases are resolved out of method calls before this bundle's compiler + // pass runs, so a logger configured by alias needs its own coverage. + $builder->setAlias('test.logger_alias', TestLogger::class); }); foreach ($this->pathToConfigs as $path) { diff --git a/tests/RestoresExceptionHandlers.php b/tests/RestoresExceptionHandlers.php new file mode 100644 index 0000000..69300bb --- /dev/null +++ b/tests/RestoresExceptionHandlers.php @@ -0,0 +1,55 @@ +handlerBeforeTest = $this->currentExceptionHandler(); + } + + protected function restoreExceptionHandlers(): void + { + // Compares identity: a test that replaced the baseline handler with an equal + // but distinct instance would drain past it. Nothing does, and nothing in + // PHPUnit's lifecycle can. + // + // Pops one handler at a time rather than draining the stack, which would + // discard a global handler registered before the suite ran. The null check is + // the terminating condition for a test that removed the handler it inherited + // instead of adding on top of it: restore_exception_handler() on an empty + // stack is a no-op, so without it the loop would never finish. + while (null !== ($current = $this->currentExceptionHandler()) && $current !== $this->handlerBeforeTest) { + restore_exception_handler(); + } + } + + /** + * Read the current handler without changing the stack: the push is undone + * immediately, and `set_exception_handler()` returns what it displaced. + */ + private function currentExceptionHandler(): mixed + { + $handler = set_exception_handler(null); + restore_exception_handler(); + + return $handler; + } +} diff --git a/tests/Security/ConsumerAuthenticator.php b/tests/Security/ConsumerAuthenticator.php new file mode 100644 index 0000000..1477114 --- /dev/null +++ b/tests/Security/ConsumerAuthenticator.php @@ -0,0 +1,58 @@ +validateClaims($request); + } catch (OpenIdConnectExceptionInterface $exception) { + throw new CustomUserMessageAuthenticationException($exception->getMessage(), [], 0, $exception); + } + + return new SelfValidatingPassport( + new UserBadge( + $claims['email'], + fn (string $email) => new TestUser($email) + ) + ); + } + + public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response + { + // As the README tells consumers to write it. + return $this->createTargetPathRedirect($request, $firewallName, self::FALLBACK_PATH); + } + + public function start(Request $request, ?AuthenticationException $authException = null): Response + { + return new RedirectResponse(self::LOGIN_PATH); + } +} diff --git a/tests/Security/FailedCallbackDoesNotLoopTest.php b/tests/Security/FailedCallbackDoesNotLoopTest.php new file mode 100644 index 0000000..5a83edb --- /dev/null +++ b/tests/Security/FailedCallbackDoesNotLoopTest.php @@ -0,0 +1,222 @@ +captureExceptionHandler(); + $this->kernel = new ItkDevOpenIdConnectBundleTestingKernel([ + __DIR__.'/../config/framework.yml', + __DIR__.'/../config/framework_routing.yml', + __DIR__.'/../config/security_consumer.yml', + __DIR__.'/../config/itkdev_openid_connect.yml', + ]); + $this->kernel->boot(); + } + + protected function tearDown(): void + { + $this->restoreExceptionHandlers(); + } + + /** + * A callback whose state does not match the session: the shape of every + * failure the outage produced, an expired client secret included. + */ + private function failingCallback(): Request + { + $request = Request::create('/callback_uri?state=does-not-match&code=some-code'); + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'the-real-state'); + $session->set('oauth2nonce', 'the-real-nonce'); + $request->setSession($session); + + return $request; + } + + /** + * Guards against the whole test passing vacuously. An unroutable path or a + * firewall that does not match gives a 500 with no redirect too, and every + * assertion below would then hold for the wrong reason. `validateClaims()` + * publishes the provider it resolved, so the attribute is proof it ran. + */ + private function assertTheAuthenticatorRejectedTheCallback(Request $request): void + { + $this->assertSame( + 'test_provider_1', + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'The request never reached validateClaims(), so this test proves nothing.' + ); + } + + public function testAFailedCallbackIsNotAnsweredWithARedirect(): void + { + $request = $this->failingCallback(); + $response = $this->kernel->handle($request, catch: true); + + $this->assertTheAuthenticatorRejectedTheCallback($request); + $this->assertNull( + $response->headers->get('Location'), + 'A failed callback was answered with a redirect: the firewall re-entered its entry point and the loop is back.' + ); + $this->assertSame( + Response::HTTP_INTERNAL_SERVER_ERROR, + $response->getStatusCode(), + 'The failure should surface as an error the application renders.' + ); + } + + public function testTheExceptionAndItsWholeCauseChainStayOutsideTheSecurityHierarchy(): void + { + $request = $this->failingCallback(); + + try { + $this->kernel->handle($request, catch: false); + $this->fail('A failed callback should not be handled silently.'); + } catch (AuthenticationFailedException $exception) { + // Catching the concrete type narrows it statically, which the unit test + // in OpenIdLoginAuthenticatorTest deliberately avoids. That guard belongs + // there and this test does not repeat it: what is under test here is the + // chain, and catching the type is how we get hold of it. Do not "align" + // the two tests by moving the narrowing into that one. + $this->assertTheAuthenticatorRejectedTheCallback($request); + + for ($cause = $exception; null !== $cause; $cause = $cause->getPrevious()) { + $this->assertNotInstanceOf( + AuthenticationException::class, + $cause, + 'An AuthenticationException in the chain is enough for the ExceptionListener to redirect: it walks $previous.' + ); + } + + $this->assertStringContainsString('Invalid state', $exception->getMessage(), 'The cause is still reported'); + } + } + + /** + * The observable fix for issue #63. + * + * `state` and `code` on a path that is not a callback used to enter the flow and, + * since the bundle fails closed, surface as a 500 that any unauthenticated caller + * could raise on any URL. It is the firewall's business again: an anonymous + * request is sent to the entry point, exactly as it would be without the query + * string. + */ + public function testAStrayCallbackIsLeftToTheFirewall(): void + { + $request = Request::create('/protected?state=forged&code=forged'); + $request->setSession(new Session(new MockArraySessionStorage())); + + $response = $this->kernel->handle($request, catch: true); + + $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); + $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); + $this->assertNull( + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'validateClaims() ran, so the authenticator accepted a callback on a path that is not one' + ); + } + + /** + * Symfony's half of "return to the page you asked for": the entry point fires and + * the target path is saved. Pinned here so a framework upgrade cannot quietly + * drop it and leave createTargetPathRedirect() with nothing to read. + */ + public function testTheEntryPointSavesTheRequestedPage(): void + { + $request = Request::create('/protected'); + $session = new Session(new MockArraySessionStorage()); + $request->setSession($session); + + $response = $this->kernel->handle($request, catch: true); + + $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); + $this->assertSame('http://localhost/protected', $session->get('_security.main.target_path')); + } + + /** + * The deep link, end to end. + * + * A user follows a link to a page they cannot see yet, logs in through the + * identity provider, and lands on the page they asked for — not on a default. + * Both halves are needed and neither is enough: the firewall saves the target + * when the entry point fires, and the authenticator reads it back on success. + */ + public function testADeepLinkSurvivesTheLoginRoundTrip(): void + { + $deepLink = Request::create('/protected/report/7'); + $session = new Session(new MockArraySessionStorage()); + $deepLink->setSession($session); + + // Leg one: denied, sent to the login flow, target remembered. + $response = $this->kernel->handle($deepLink, catch: true); + $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); + + // Leg two: the same session, now arriving back from the identity provider. + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $callback = Request::create('/callback_uri?state=s&code=c'); + $callback->setSession($session); + + $success = $authenticator->onAuthenticationSuccess( + $callback, + new PreAuthenticatedToken(new TestUser('someone@example.com'), 'main'), + 'main' + ); + + $this->assertInstanceOf(RedirectResponse::class, $success); + $this->assertSame('http://localhost/protected/report/7', $success->getTargetUrl()); + } + + /** + * Nothing was requested, so there is nothing to return to: a user who went + * straight to the login link gets the application's default. + */ + public function testWithoutARequestedPageTheFallbackIsUsed(): void + { + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $callback = Request::create('/callback_uri?state=s&code=c'); + $callback->setSession(new Session(new MockArraySessionStorage())); + + $success = $authenticator->onAuthenticationSuccess( + $callback, + new PreAuthenticatedToken(new TestUser('someone@example.com'), 'main'), + 'main' + ); + + $this->assertInstanceOf(RedirectResponse::class, $success); + $this->assertSame(ConsumerAuthenticator::FALLBACK_PATH, $success->getTargetUrl()); + } +} diff --git a/tests/Security/OpenIdConfigurationProviderManagerTest.php b/tests/Security/OpenIdConfigurationProviderManagerTest.php index 1da56bb..7660c83 100644 --- a/tests/Security/OpenIdConfigurationProviderManagerTest.php +++ b/tests/Security/OpenIdConfigurationProviderManagerTest.php @@ -6,10 +6,13 @@ use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; +use Psr\Cache\CacheItemPoolInterface; use Symfony\Component\Cache\Adapter\ArrayAdapter; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; +use Symfony\Component\Routing\RequestContext; use Symfony\Component\Routing\RouterInterface; class OpenIdConfigurationProviderManagerTest extends TestCase @@ -35,12 +38,24 @@ private function getBaseProviderConfig(): array } /** - * Test helper: callers build provider arrays from {@see getBaseProviderConfig()} - * plus optional fields, so the parameter is intentionally typed loosely. The - * production manager constructor has the precise array shape. - * - * @param array> $providers - * @param array $defaultOptions + * @param array, + * callback_path?: string, + * leeway?: int, + * cache_duration?: int, + * allow_http?: bool, + * http_client_options?: array{ + * timeout?: float, + * proxy?: string, + * verify?: bool, + * }, + * }> $providers + * @param array{cacheItemPool?: CacheItemPoolInterface} $defaultOptions */ private function createManager(array $providers, array $defaultOptions = []): OpenIdConfigurationProviderManager { @@ -52,7 +67,6 @@ private function createManager(array $providers, array $defaultOptions = []): Op 'providers' => $providers, ]; - // @phpstan-ignore argument.type (test helper relaxes the strict provider shape declared by the production constructor — callers build configs ad-hoc from getBaseProviderConfig() plus optional fields) return new OpenIdConfigurationProviderManager($this->stubRouter, $config); } @@ -220,4 +234,164 @@ public function testGetProviderCachesInstance(): void $this->assertSame($provider1, $provider2); } + + /** + * @return iterable + */ + public static function pathDerivationProvider(): iterable + { + yield 'path of an absolute redirect_uri' => [['redirect_uri' => 'https://app.example.org/callback_uri'], '/callback_uri']; + yield 'trailing slash removed' => [['redirect_uri' => 'https://app.example.org/callback_uri/'], '/callback_uri']; + yield 'nested path' => [['redirect_uri' => 'https://app.example.org/auth/oidc/callback'], '/auth/oidc/callback']; + yield 'query and fragment ignored' => [['redirect_uri' => 'https://app.example.org/callback_uri?x=1#f'], '/callback_uri']; + // A redirect_uri naming only a host answers at the root. + yield 'no path at all' => [['redirect_uri' => 'https://app.example.org'], '/']; + yield 'bare root' => [['redirect_uri' => 'https://app.example.org/'], '/']; + // callback_path exists for proxies that rewrite the external path, so it has + // to win over the redirect_uri it contradicts. + yield 'callback_path overrides redirect_uri' => [ + ['redirect_uri' => 'https://app.example.org/prefix/auth/callback', 'callback_path' => '/auth/callback'], + '/auth/callback', + ]; + yield 'callback_path is normalized too' => [['callback_path' => '/auth/callback/'], '/auth/callback']; + } + + /** + * @param array{redirect_uri?: string, callback_path?: string} $options + */ + #[DataProvider('pathDerivationProvider')] + public function testRedirectUriPathsAreDerivedAndNormalized(array $options, string $expected): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + $options]); + + $this->assertSame(['provider1' => $expected], $manager->getRedirectUriPaths()); + } + + public function testARouteIsGeneratedAsAPathNotAUrl(): void + { + // ABSOLUTE_PATH, so that whatever a reverse proxy does to the host or scheme + // cannot affect the comparison, and the router's base path is included. + $router = $this->createMock(RouterInterface::class); + $router->expects($this->once()) + ->method('generate') + ->with('my_route', ['id' => '7'], UrlGeneratorInterface::ABSOLUTE_PATH) + ->willReturn('/generated/callback'); + + $config = [ + 'default_providers_options' => [], + 'providers' => ['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_route' => 'my_route', + 'redirect_route_parameters' => ['id' => '7'], + ]], + ]; + + $manager = new OpenIdConfigurationProviderManager($router, $config); + + $this->assertSame(['provider1' => '/generated/callback'], $manager->getRedirectUriPaths()); + // Memoized: supports() asks on every request through the firewall, and the + // once() above is what holds that. + $manager->getRedirectUriPaths(); + } + + public function testAProviderWithNoRedirectTargetIsAbsentRatherThanMatchingEverything(): void + { + $manager = $this->createManager([ + 'with_path' => $this->getBaseProviderConfig() + ['redirect_uri' => 'https://app.example.org/callback_uri'], + 'without_path' => $this->getBaseProviderConfig(), + ]); + + $this->assertSame(['with_path' => '/callback_uri'], $manager->getRedirectUriPaths()); + } + + public function testDerivingPathsDoesNotBuildProviders(): void + { + // Building a provider pulls in discovery, an HTTP client and a cache pool. + // Nothing in this config could support that, so a successful call proves + // supports() is not paying for it on every request. + $manager = $this->createManager(['provider1' => [ + 'metadata_url' => 'https://unreachable.invalid/.well-known/openid-configuration', + 'client_id' => 'id', + 'client_secret' => 'secret', + 'redirect_uri' => 'https://app.example.org/callback_uri', + ]]); + + $this->assertSame(['provider1' => '/callback_uri'], $manager->getRedirectUriPaths()); + } + + /** + * @return iterable + */ + public static function requestPathProvider(): iterable + { + yield 'exactly' => ['/callback_uri', true]; + yield 'trailing slash' => ['/callback_uri/', true]; + yield 'another path' => ['/protected', false]; + yield 'below it' => ['/callback_uri/extra', false]; + yield 'differing in case' => ['/Callback_Uri', false]; + } + + #[DataProvider('requestPathProvider')] + public function testIsCallbackPathNormalizesWhatItIsGiven(string $requestPath, bool $expected): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback_uri', + ]]); + + $this->assertSame($expected, $manager->isCallbackPath($requestPath, 'provider1')); + } + + public function testAnUnknownProviderIsNotACallbackPath(): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback_uri', + ]]); + + $this->assertFalse($manager->isCallbackPath('/callback_uri', 'never-heard-of-it')); + } + + /** + * An environment variable that resolves to nothing must not turn the site root + * into the callback path: configuration lets '' through deliberately, because it + * is the fixture Symfony substitutes while compiling. + */ + public function testAnEmptyCallbackPathFallsThroughToRedirectUri(): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback_uri', + 'callback_path' => '', + ]]); + + $this->assertSame(['provider1' => '/callback_uri'], $manager->getRedirectUriPaths()); + } + + /** + * Generated routes carry the routing context's base URL, which differs between a + * request arriving through a proxy that sends X-Forwarded-Prefix and a direct one. + * Memoizing one map for both would leave one of them unable to match. + */ + public function testRoutePathsAreMemoizedPerBaseUrl(): void + { + $context = new RequestContext(); + // A stub: this asserts on the paths, not on how the router was called. + $router = $this->createStub(RouterInterface::class); + $router->method('getContext')->willReturn($context); + $router->method('generate')->willReturnCallback( + static fn (string $name, array $parameters, int $type): string => $context->getBaseUrl().'/generated/callback' + ); + + $config = ['default_providers_options' => [], 'providers' => ['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_route' => 'my_route', + ]]]; + + $manager = new OpenIdConfigurationProviderManager($router, $config); + + $this->assertSame(['provider1' => '/generated/callback'], $manager->getRedirectUriPaths()); + + $context->setBaseUrl('/prefix'); + $this->assertSame(['provider1' => '/prefix/generated/callback'], $manager->getRedirectUriPaths()); + + // And back: still memoized per base URL rather than recomputed blindly. + $context->setBaseUrl(''); + $this->assertSame(['provider1' => '/generated/callback'], $manager->getRedirectUriPaths()); + } } diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 799ca1c..20e05ba 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -7,15 +7,21 @@ use ItkDev\OpenIdConnect\Exception\ValidationException; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; +use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; +use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use Psr\Log\LogLevel; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; +use Symfony\Component\Routing\RouterInterface; use Symfony\Component\Security\Core\Exception\AuthenticationException; class OpenIdLoginAuthenticatorTest extends TestCase @@ -35,36 +41,244 @@ protected function setUp(): void $this->authenticator->setLogger($this->logger); } - public function testSupports(): void + /** + * A real manager, not a stub: the path comparison lives there, and stubbing it + * would mean reimplementing normalization in the test — where a bug in the real + * one could not be seen. + * + * @param array $paths callback_path per provider + */ + private function managerWithPaths(array $paths): OpenIdConfigurationProviderManager { - $request = new Request(); + $providers = []; + + foreach ($paths as $key => $path) { + $providers[$key] = [ + 'metadata_url' => 'https://provider.example.org/.well-known/openid-configuration', + 'client_id' => 'id', + 'client_secret' => 'secret', + 'callback_path' => $path, + ]; + } + + $config = ['default_providers_options' => [], 'providers' => $providers]; + + return new OpenIdConfigurationProviderManager($this->createStub(RouterInterface::class), $config); + } + + /** + * @param array $paths + */ + private function authenticatorWithPaths(array $paths): TestAuthenticator + { + $authenticator = new TestAuthenticator($this->managerWithPaths($paths)); + $authenticator->setLogger($this->logger); + + return $authenticator; + } + + /** + * `state` and `code` are necessary but no longer sufficient: without the path + * check any URL under the firewall is a callback, so an unauthenticated caller + * can turn any page into a failed login — a 500, since the bundle fails closed. + * + * @return iterable + */ + public static function callbackPathProvider(): iterable + { + yield 'the configured path' => ['/callback_uri', true]; + yield 'trailing slash is the same path' => ['/callback_uri/', true]; + yield 'another provider on this authenticator' => ['/other_callback', true]; + yield 'a protected page' => ['/protected', false]; + yield 'the root' => ['/', false]; + yield 'below the callback path' => ['/callback_uri/extra', false]; + yield 'above the callback path' => ['/callback', false]; + yield 'differing in case' => ['/Callback_Uri', false]; + yield 'the path as a query parameter' => ['/protected/callback_uri', false]; + } + + #[DataProvider('callbackPathProvider')] + public function testSupportsOnlyTheConfiguredCallbackPaths(string $path, bool $expected): void + { + $authenticator = $this->authenticatorWithPaths([ + 'test_provider_1' => '/callback_uri', + 'test_provider_2' => '/other_callback', + ]); + + $request = Request::create($path.'?state=abcd&code=xyz'); + + $this->assertSame($expected, $authenticator->supports($request)); + } + + /** + * Deployments where the request path is not the whole story. + * + * `getPathInfo()` has both a subdirectory's base path and a trusted + * `X-Forwarded-Prefix` stripped out of it, while a configured `redirect_uri` + * contains them — it is the URL the identity provider was given. Comparing path + * info alone would reject every callback in either deployment. + * + * @return iterable + */ + public static function baseUrlProvider(): iterable + { + // configured path, base url, request path info, expected + yield 'subdirectory deployment' => ['/app/callback_uri', '/app', true]; + yield 'trusted proxy prefix' => ['/prefix/callback_uri', '/prefix', true]; + yield 'root deployment' => ['/callback_uri', '', true]; + // A proxy that rewrites without a prefix header: the internal path really is + // different, which is what callback_path exists to declare. + yield 'rewriting proxy, no header' => ['/prefix/callback_uri', '', false]; + } + + #[DataProvider('baseUrlProvider')] + public function testTheCallbackPathIncludesTheBaseUrl(string $configured, string $baseUrl, bool $expected): void + { + $authenticator = $this->authenticatorWithPaths(['test_provider_1' => $configured]); + + $request = new RequestWithBaseUrl($baseUrl, ['state' => 'abcd', 'code' => 'xyz']); + $request->server->set('REQUEST_URI', $baseUrl.'/callback_uri?state=abcd&code=xyz'); - $this->assertFalse($this->authenticator->supports($request)); + $this->assertSame($expected, $authenticator->supports($request)); + } + + /** + * @return iterable}> + */ + public static function incompleteCallbackProvider(): iterable + { + yield 'neither' => [[]]; + yield 'state only' => [['state' => 'abcd']]; + yield 'code only' => [['code' => 'xyz']]; + } + + #[DataProvider('incompleteCallbackProvider')] + public function testTheRightPathAloneIsNotACallback(array $query): void + { + $authenticator = $this->authenticatorWithPaths(['test_provider_1' => '/callback_uri']); + + $this->assertFalse($authenticator->supports(Request::create('/callback_uri?'.http_build_query($query)))); + } + + /** + * A subclass bound to one provider does not answer another provider's callback, + * which is what lets one authenticator per provider share a firewall. + */ + public function testASubclassCanNarrowTheProvidersItAnswersFor(): void + { + $authenticator = new SingleProviderAuthenticator($this->managerWithPaths([ + 'test_provider_1' => '/callback_uri', + 'test_provider_2' => '/other_callback', + ])); + + $this->assertTrue($authenticator->supports(Request::create('/callback_uri?state=a&code=b'))); + $this->assertFalse($authenticator->supports(Request::create('/other_callback?state=a&code=b'))); + } - $request->query->set('state', 'abcd'); - $this->assertFalse($this->authenticator->supports($request)); + /** + * A provider with no derivable path contributes no match rather than matching + * everything, which would be the bug this constraint removes. + */ + public function testAProviderWithoutAPathMatchesNothing(): void + { + // No redirect_uri, redirect_route or callback_path: nothing to match on, and + // matching everything is the defect this constraint removes. + $config = ['default_providers_options' => [], 'providers' => ['test_provider_1' => [ + 'metadata_url' => 'https://provider.example.org/.well-known/openid-configuration', + 'client_id' => 'id', + 'client_secret' => 'secret', + ]]]; + + $manager = new OpenIdConfigurationProviderManager($this->createStub(RouterInterface::class), $config); - $request->query->set('code', 'xyz'); - $this->assertTrue($this->authenticator->supports($request)); + $authenticator = new TestAuthenticator($manager); + + $this->assertFalse($authenticator->supports(Request::create('/callback_uri?state=a&code=b'))); } - public function testOnAuthenticationFailurePreservesCause(): void + /** + * The assertion that encodes "the loop cannot come back". + * + * Everything else here is detail; what matters is the type. Symfony's security + * ExceptionListener catches `AuthenticationException` and re-enters the entry + * point, which for this authenticator is another redirect to the identity + * provider. Throwing something outside that hierarchy is what stops a failing + * callback from being retried forever. + */ + public function testOnAuthenticationFailureThrowsOutsideTheSecurityHierarchy(): void { $cause = new AuthenticationException('Original cause message'); + // Caught as Throwable on purpose: catching the expected type first would + // narrow it statically and make the assertions below tautologies, which is + // precisely the mistake that would let the type quietly regress. try { $this->authenticator->onAuthenticationFailure(new Request(), $cause); - $this->fail('Expected AuthenticationException'); - } catch (AuthenticationException $thrown) { - $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained as previous'); + $this->fail('Expected AuthenticationFailedException'); + } catch (\Throwable $thrown) { + $this->assertNotInstanceOf( + AuthenticationException::class, + $thrown, + 'An AuthenticationException would be caught by the firewall and turned back into a redirect to the identity provider', + ); + $this->assertInstanceOf( + OpenIdConnectBundleExceptionInterface::class, + $thrown, + 'Consumers catch the bundle marker, per ADR 001', + ); + $this->assertInstanceOf(AuthenticationFailedException::class, $thrown); + + // Not chained, even though ADR 001 asks for a cause: the security + // ExceptionListener walks the whole $previous chain, so an + // AuthenticationException reachable through it is caught and turned back + // into a redirect exactly as if it had been thrown directly. The message + // carries the reason instead. + $this->assertNull($thrown->getPrevious(), 'An AuthenticationException must not be reachable through the chain'); $this->assertStringContainsString('Original cause message', $thrown->getMessage(), 'Cause message must be preserved for logs'); // Deliberately no record: the framework already logs the original - // exception, and validateClaims() logged the specific reason. + // exception, validateClaims() logged the specific reason, and the + // application logs whatever escapes. $this->assertSame([], $this->logger->records); } } + /** + * The chain is dropped only as far as it has to be. A library exception below + * the AuthenticationException is what says *why* the callback failed, and it + * is safe to keep because the listener does not act on it. + */ + #[DataProvider('causeChainProvider')] + public function testACauseOutsideTheSecurityHierarchyIsKept(\Throwable $cause, ?\Throwable $expected): void + { + try { + $this->authenticator->onAuthenticationFailure(new Request(), new AuthenticationException('Sanitised by the firewall', 0, $cause)); + $this->fail('Expected AuthenticationFailedException'); + } catch (\Throwable $thrown) { + $this->assertSame($expected, $thrown->getPrevious()); + } + } + + /** + * @return iterable + */ + public static function causeChainProvider(): iterable + { + $root = new ValidationException('Invalid state'); + + yield 'library cause is kept' => [$root, $root]; + // The firewall wraps more than once in places, so one skip is not enough. + yield 'reached past nested security exceptions' => [new AuthenticationException('inner', 0, $root), $root]; + // A library exception is not safe merely by being one: skipping only the + // leading security exceptions would keep this outer cause and leave an + // AuthenticationException reachable one level further down. + yield 'library cause hiding a security exception is skipped too' => [ + new ValidationException('outer', 0, new AuthenticationException('inner', 0, $root)), + $root, + ]; + yield 'nothing left to keep' => [new AuthenticationException('inner'), null]; + } + public function testUnknownProviderIsLoggedAndRethrown(): void { $cause = new InvalidProviderException('Invalid provider: test_provider_1'); @@ -271,8 +485,8 @@ public function testEveryFailurePathWorksWithoutALogger(): void // onAuthenticationFailure(). try { $authenticator->onAuthenticationFailure(new Request(), new AuthenticationException('boom')); - $this->fail('Expected AuthenticationException'); - } catch (AuthenticationException $thrown) { + $this->fail('Expected AuthenticationFailedException'); + } catch (AuthenticationFailedException $thrown) { $this->assertStringContainsString('boom', $thrown->getMessage()); } } @@ -301,4 +515,121 @@ private function setSessionOnRequest(Request $request, ?string $nonce = 'test_no $request->setSession($stubSession); } + + /** + * The property above is deliberately typed as the abstract class, so the fixture + * method exposing the protected helper needs a concrete local. + */ + private function fixtureAuthenticator(): TestAuthenticator + { + $authenticator = new TestAuthenticator($this->stubProviderManager); + $authenticator->setLogger($this->logger); + + return $authenticator; + } + + private function requestWithSession(?string $targetPath): Request + { + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); + + if (null !== $targetPath) { + $session->set('_security.main.target_path', $targetPath); + } + + $request->setSession($session); + + return $request; + } + + public function testTheRequestedPageIsReturnedToAndThenForgotten(): void + { + $request = $this->requestWithSession('/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/admin/reports', $response->getTargetUrl()); + // Cleared, so a later visit to the login link does not replay it. + $this->assertFalse($request->getSession()->has('_security.main.target_path')); + } + + /** + * @return iterable + */ + public static function noTargetPathProvider(): iterable + { + yield 'nothing saved' => [null]; + yield 'saved but empty' => ['']; + } + + #[DataProvider('noTargetPathProvider')] + public function testTheFallbackIsUsedWhenNoPageWasRequested(?string $targetPath): void + { + // A user who went to the login link directly, rather than being sent there. + $request = $this->requestWithSession($targetPath); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/dashboard', $response->getTargetUrl()); + } + + public function testTheTargetPathIsReadForTheRightFirewall(): void + { + $request = $this->requestWithSession('/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'other_firewall', '/dashboard'); + + $this->assertSame('/dashboard', $response->getTargetUrl()); + $this->assertTrue($request->getSession()->has('_security.main.target_path'), 'Another firewall\'s target path is left alone'); + } + + public function testATargetNamedOnTheLoginLinkIsUsedWhenNothingWasDenied(): void + { + // The case the firewall cannot cover: the user was never refused anything, so + // Symfony saved nothing. They followed a login link that named where to go. + $request = $this->requestWithSession(null); + $request->getSession()->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, '/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/admin/reports', $response->getTargetUrl()); + $this->assertFalse($request->getSession()->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY), 'Consumed, so it cannot replay'); + } + + public function testTheDeniedPageWinsOverATargetNamedOnTheLink(): void + { + // Both present: the firewall's record is what the user was actually stopped + // from reaching, so it is the more faithful answer. + $request = $this->requestWithSession('/admin/denied-page'); + $request->getSession()->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, '/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/admin/denied-page', $response->getTargetUrl()); + // Both cleared, or the unused one would resurface on a later login. + $this->assertFalse($request->getSession()->has('_security.main.target_path')); + $this->assertFalse($request->getSession()->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + } + + /** + * @return iterable + */ + public static function unusableNamedTargetProvider(): iterable + { + yield 'empty' => ['']; + // Nothing writes a non-string, but the session is shared with the application. + yield 'not a string' => [['/admin/reports']]; + } + + #[DataProvider('unusableNamedTargetProvider')] + public function testAnUnusableNamedTargetFallsBack(mixed $stored): void + { + $request = $this->requestWithSession(null); + $request->getSession()->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, $stored); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/dashboard', $response->getTargetUrl()); + $this->assertFalse($request->getSession()->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + } } diff --git a/tests/Security/ProtectedController.php b/tests/Security/ProtectedController.php new file mode 100644 index 0000000..3dd7c30 --- /dev/null +++ b/tests/Security/ProtectedController.php @@ -0,0 +1,17 @@ + $query + */ + public function __construct(private readonly string $overriddenBaseUrl, array $query = []) + { + parent::__construct($query); + } + + #[\Override] + public function getBaseUrl(): string + { + return $this->overriddenBaseUrl; + } + + #[\Override] + public function getPathInfo(): string + { + return '/callback_uri'; + } +} diff --git a/tests/Security/SingleProviderAuthenticator.php b/tests/Security/SingleProviderAuthenticator.php new file mode 100644 index 0000000..4bdc7da --- /dev/null +++ b/tests/Security/SingleProviderAuthenticator.php @@ -0,0 +1,16 @@ +createTargetPathRedirect($request, $firewallName, $fallbackUrl); + } } diff --git a/tests/Util/ClientSecretExpiryCheckerTest.php b/tests/Util/ClientSecretExpiryCheckerTest.php index 3b07576..15a5a2f 100644 --- a/tests/Util/ClientSecretExpiryCheckerTest.php +++ b/tests/Util/ClientSecretExpiryCheckerTest.php @@ -45,6 +45,10 @@ public function testUnknownWhenNoDateConfigured(): void $this->assertNull($status->daysRemaining); $this->assertFalse($status->isExpired()); $this->assertFalse($status->isExpiringSoon()); + // And nothing logged: the option is optional, so an unset date is a decision + // rather than a fault. A value that is set but unusable is the opposite, and + // is reported at error. The gap is visible as this status, not as a record. + $this->assertSame([], $this->logger->records); } public function testUnknownForAProviderWithNoEntryAtAll(): void diff --git a/tests/config/framework_routing.yml b/tests/config/framework_routing.yml new file mode 100644 index 0000000..a10fcac --- /dev/null +++ b/tests/config/framework_routing.yml @@ -0,0 +1,6 @@ +# framework.yml disables the router with `resource: ~`, which makes any request +# fail at boot. Load this after it when a test dispatches a real request. +framework: + router: + resource: "%kernel.project_dir%/tests/config/routes.yml" + utf8: true diff --git a/tests/config/itkdev_openid_connect.yml b/tests/config/itkdev_openid_connect.yml index 59d5054..91f18e8 100644 --- a/tests/config/itkdev_openid_connect.yml +++ b/tests/config/itkdev_openid_connect.yml @@ -10,6 +10,7 @@ itkdev_openid_connect: metadata_url: "https://provider.example.org/openid-configuration" client_id: "test_id" client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" test_provider_2: options: @@ -17,4 +18,5 @@ itkdev_openid_connect: client_id: "test_id" leeway: 5 client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" diff --git a/tests/config/itkdev_openid_connect_alias_logger.yml b/tests/config/itkdev_openid_connect_alias_logger.yml new file mode 100644 index 0000000..73769d2 --- /dev/null +++ b/tests/config/itkdev_openid_connect_alias_logger.yml @@ -0,0 +1,24 @@ +itkdev_openid_connect: + logging_options: + logger: "test.logger_alias" + cache_options: + cache_pool: "cache.array" + cli_login_options: + route: "route_test" + user_provider: "security.user.provider.concrete.test_users_1" + openid_providers: + test_provider_1: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" + redirect_uri: "https://app.example.org/callback_uri" + test_provider_2: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + leeway: 5 + client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" + redirect_uri: "https://app.example.org/callback_uri" diff --git a/tests/config/itkdev_openid_connect_configured_logger.yml b/tests/config/itkdev_openid_connect_configured_logger.yml new file mode 100644 index 0000000..5eb1710 --- /dev/null +++ b/tests/config/itkdev_openid_connect_configured_logger.yml @@ -0,0 +1,24 @@ +itkdev_openid_connect: + logging_options: + logger: ItkDev\OpenIdConnectBundle\Tests\TestLogger + cache_options: + cache_pool: "cache.array" + cli_login_options: + route: "route_test" + user_provider: "security.user.provider.concrete.test_users_1" + openid_providers: + test_provider_1: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" + redirect_uri: "https://app.example.org/callback_uri" + test_provider_2: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + leeway: 5 + client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" + redirect_uri: "https://app.example.org/callback_uri" diff --git a/tests/config/routes.yml b/tests/config/routes.yml new file mode 100644 index 0000000..17a49f6 --- /dev/null +++ b/tests/config/routes.yml @@ -0,0 +1,15 @@ +protected: + path: /protected + controller: ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController + +# The path of the test providers' redirect_uri. supports() only recognises a +# callback here, so a request has to be able to reach it. +callback: + path: /callback_uri + controller: ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController + +# A deeper protected page, to check a link into the application survives the login +# round trip rather than collapsing to a default. +protected_report: + path: /protected/report/{id} + controller: ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController diff --git a/tests/config/security_consumer.yml b/tests/config/security_consumer.yml new file mode 100644 index 0000000..a04d795 --- /dev/null +++ b/tests/config/security_consumer.yml @@ -0,0 +1,25 @@ +security: + providers: + test_users_1: + memory: + users: + admin: { password: "test", roles: ["ROLE_ADMIN"] } + test_users_2: + memory: + users: + admin: { password: "test", roles: ["ROLE_ADMIN"] } + firewalls: + dev: + pattern: ^/(_(profiler|wdt)|css|images|js)/ + security: false + main: + lazy: true + custom_authenticators: + - ItkDev\OpenIdConnectBundle\Tests\Security\ConsumerAuthenticator + - ItkDev\OpenIdConnectBundle\Security\CliLoginTokenAuthenticator + entry_point: ItkDev\OpenIdConnectBundle\Tests\Security\ConsumerAuthenticator + provider: test_users_1 + # Without this nothing demands authentication, so the entry point never fires and + # there is no target path to save — the behaviour these tests are about. + access_control: + - { path: "^/protected", roles: "ROLE_ADMIN" }