diff --git a/CHANGELOG.md b/CHANGELOG.md index cccd3f0..32515ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 in the session under `oauth2pkce_verifier`, and sends the challenge; the authenticator redeems the code with it. Turn it off per provider with `pkce: false` for an identity provider that rejects the parameters rather than ignoring them. -- `OpenIdConfigurationProviderManager::isPkceEnabled()`. +- `OpenIdConfigurationProviderManager::isPkceEnabled()` and `getScopes()`. +- Per-provider `scopes`, defaulting to `openid`, `email` and `profile` — the scopes the + bundle has always requested. Accepts a list or a space-separated string, so the value + can come from an environment variable. A list without `openid` is rejected at compile + time. +- `StatelessFirewallException`, naming the misconfiguration when the authenticator is + put on a firewall declared `stateless: true`. Previously Symfony's + `SessionNotFoundException` surfaced as an unexplained 500. - `ProviderErrorException`, thrown when the identity provider refuses the authorization request (RFC 6749 §4.1.2.1). It extends `AuthenticationFailedException`, so existing `catch` blocks keep matching, and carries `getError()`, `getErrorDescription()` and @@ -29,6 +36,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `OpenIdLoginAuthenticator` implements `InteractiveAuthenticatorInterface`, so a + completed login dispatches `security.interactive_login` and remember-me treats the + token as one a user asked for. +- `leeway` and `cache_duration` reject a negative value while the container compiles. + A negative leeway used to fail at the first login that needed it, and a negative + cache duration passed through to the cache unnoticed. - Requires `itk-dev/openid-connect` `^5.1`, for its PKCE support. That release also enforces `allowHttp` on every discovered endpoint, requires `exp` and `iat` on the ID token, and changes the JWKS cache key — see its changelog before upgrading. diff --git a/README.md b/README.md index 76e0b13..12927ff 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,10 @@ itkdev_openid_connect: # Optional: Send a PKCE challenge (RFC 7636, S256) with the authorization # request. Defaults to true. See "PKCE" below. pkce: true + # Optional: Scopes to request. Defaults to openid, email, profile. + # Must include openid. A space-separated string is accepted too, + # so the value can come from an environment variable. + scopes: ['openid', 'email', 'profile'] # Optional: Allow (non-secure) http requests (used for mocking a IdP). NOT RECOMMENDED FOR PRODUCTION. # Defaults to false allow_http: '%env(bool:ADMIN_OIDC_ALLOW_HTTP)%' @@ -887,6 +891,30 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator } ``` +### Scopes + +The authorization request asks for `openid`, `email` and `profile`. Set `scopes` per +provider to ask for something else: + +```yaml +openid_providers: + admin: + options: + scopes: ['openid', 'profile', 'groups'] +``` + +`openid` must be among them — OpenID Connect Core 1.0 §3.1.2.1 defines an +authentication request as one that asks for it, and without it the provider returns an +OAuth2 grant with no ID token, which is the only thing this bundle can validate. A list +missing it fails at compile time. + +A space-separated string is accepted and split, since an environment variable can only +carry a scalar: + +```yaml + scopes: '%env(ADMIN_OIDC_SCOPES)%' # ADMIN_OIDC_SCOPES=openid profile groups +``` + ### PKCE The bundle sends a PKCE challenge (RFC 7636, S256) with every authorization request. diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md index cf46e95..5168b1c 100644 --- a/UPGRADE-6.1.md +++ b/UPGRADE-6.1.md @@ -76,6 +76,16 @@ The verifier is kept in the session under `oauth2pkce_verifier`. If your applica clears or rewrites the session between the login redirect and the callback, it must preserve that key alongside `oauth2state` and `oauth2nonce`. +## A stateless firewall is now named as such + +Putting the OpenID Connect authenticator on a firewall declared `stateless: true` +throws `StatelessFirewallException`, naming the setting to remove. It used to surface +as Symfony's `SessionNotFoundException` and an unexplained 500. + +The flow spans two requests and the session is where the state, nonce and PKCE +verifier wait, so such a firewall could never complete a login. Nothing that worked +before stops working. + ## The library requires 5.1 `itk-dev/openid-connect` `^5.1` comes with this release. Three of its changes affect @@ -95,6 +105,11 @@ reused. Nothing to do unless you held the returned provider and relied on getting the same object back. +## Optional: scopes are configurable + +The authorization request still asks for `openid`, `email` and `profile`. Set `scopes` +per provider to change that; `openid` must remain among them. + See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning behind the error-callback handling, and [CHANGELOG.md](CHANGELOG.md) for the rest of the release. diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index ecd7a05..d7eec6f 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -80,7 +80,8 @@ public function login(Request $request, SessionInterface $session, string $provi 'state' => $state, 'nonce' => $nonce, 'response_type' => 'code', - 'scope' => 'openid email profile', + // Space-delimited, as RFC 6749 §3.3 defines the parameter. + 'scope' => implode(' ', $this->providerManager->getScopes($providerKey)), ]; if (null !== $pkceVerifier) { diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index d408470..f870627 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -158,10 +158,47 @@ public function getConfigTreeBuilder(): TreeBuilder ->integerNode('leeway') ->info('Leeway in seconds to account for clock skew between server and provider') ->defaultValue(10) + // A window, so zero means no window. A negative one is + // rejected here rather than at the first login it breaks. + ->min(0) ->end() ->integerNode('cache_duration') ->info('Cache duration in seconds for the OIDC discovery document and JWKS (default: 86400 — 24 hours)') ->defaultValue(86400) + // Zero fetches the discovery document every time, which is + // wasteful but coherent. A negative lifetime is not. + ->min(0) + ->end() + ->arrayNode('scopes') + ->info('Scopes requested from the identity provider (default: openid, email, profile)') + ->scalarPrototype()->end() + ->defaultValue(['openid', 'email', 'profile']) + ->requiresAtLeastOneElement() + // Accept a space-separated string so the list can come from + // an environment variable, which can only carry a scalar. + ->beforeNormalization() + ->ifString() + ->then(static function (string $scopes): array { + // NO_EMPTY drops the empty strings a + // leading or trailing space produces, + // so surrounding whitespace needs no + // separate trim. + $split = preg_split('/\s+/', $scopes, -1, PREG_SPLIT_NO_EMPTY); + + // preg_split only fails on a malformed + // pattern; this one is a literal. An + // empty list is caught below. + return false === $split ? [] : $split; + }) + ->end() + ->validate() + // OpenID Connect Core 1.0 §3.1.2.1: an authentication + // request is one that asks for `openid`. Without it the + // provider answers with a plain OAuth2 grant and no ID + // token, and every check this bundle makes needs one. + ->ifTrue(static fn (array $scopes): bool => !in_array('openid', $scopes, true)) + ->thenInvalid('scopes must include openid: without it the provider returns no ID token.') + ->end() ->end() ->booleanNode('pkce') // On by default: RFC 6749 §3.1 requires an authorization diff --git a/src/Exception/StatelessFirewallException.php b/src/Exception/StatelessFirewallException.php new file mode 100644 index 0000000..b21b420 --- /dev/null +++ b/src/Exception/StatelessFirewallException.php @@ -0,0 +1,19 @@ +config['providers']); } + /** + * The scopes this provider's authorization request asks for. + * + * Read from configuration, like the callback paths: the caller needs the answer + * before it has any use for a provider. + * + * An unconfigured key answers the same default the config tree applies, keeping + * one rule in two places from drifting apart. + * + * @return string[] + */ + public function getScopes(string $providerKey): array + { + return $this->config['providers'][$providerKey]['scopes'] ?? ['openid', 'email', 'profile']; + } + /** * Whether this provider's authorization request carries a PKCE challenge. * diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index d3411d6..67b65fe 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -7,14 +7,17 @@ use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use ItkDev\OpenIdConnectBundle\Exception\StatelessFirewallException; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException; 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\Authenticator\InteractiveAuthenticatorInterface; use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface; use Symfony\Component\Security\Http\Util\TargetPathTrait; @@ -47,7 +50,7 @@ * class with two states where only one is meaningful. Defaulting to a * `NullLogger` keeps a single code path for consumers who never get a logger. */ -abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements AuthenticationEntryPointInterface, LoggerAwareInterface +abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements AuthenticationEntryPointInterface, InteractiveAuthenticatorInterface, LoggerAwareInterface { use TargetPathTrait; @@ -85,6 +88,18 @@ public function setLogger(LoggerInterface $logger): void $this->logger = $logger; } + /** + * A person logged in here, at an identity provider, just now. + * + * Symfony dispatches `security.interactive_login` for an authenticator that says + * so, and remember-me treats the resulting token as one a user actually asked + * for. Both are true of every login this authenticator completes. + */ + public function isInteractive(): bool + { + return true; + } + /** * Whether this request is a callback for one of this authenticator's providers. * @@ -197,7 +212,11 @@ protected function getSupportedProviderKeys(): array */ protected function validateClaims(Request $request): array { - $session = $request->getSession(); + try { + $session = $request->getSession(); + } catch (SessionNotFoundException $exception) { + throw new StatelessFirewallException('The OpenID Connect authenticator needs a session to hold the state, nonce and PKCE verifier between the authorization request and the callback. Remove `stateless: true` from this firewall.', previous: $exception); + } // Every one-time value is spent here, before anything below can throw. A // callback is used up whether it succeeds, fails validation, or carries the diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 2f475d4..ef58dd5 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -174,6 +174,24 @@ public function testAStaleVerifierIsOverwrittenWhenPkceIsOff(): void $this->assertNull($session->get('oauth2pkce_verifier')); } + public function testConfiguredScopesReachTheAuthorizationRequest(): void + { + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider->method('generateNonce')->willReturn('1234'); + $mockProvider->method('generateState')->willReturn('abcd'); + $mockProvider + ->expects($this->once()) + ->method('getAuthorizationUrl') + ->with($this->callback( + static fn (array $options): bool => 'openid profile groups' === ($options['scope'] ?? null) + )) + ->willReturn('https://provider.example.org/authorize'); + + $controller = $this->createController($mockProvider, scopes: ['openid', 'profile', 'groups']); + + $controller->login(new Request(), new Session(new MockArraySessionStorage()), 'test'); + } + public function testUnknownProviderKeyMapsTo404(): void { $cause = new InvalidProviderException('Invalid provider: bogus'); @@ -339,7 +357,10 @@ public function testUnknownProviderIsRefusedBeforeTheExpiryCheck(): void $this->fail('Expected NotFoundHttpException'); } - private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null, bool $pkce = false): LoginController + /** + * @param string[] $scopes + */ + private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null, bool $pkce = false, array $scopes = ['openid', 'email', 'profile']): LoginController { $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); $mockProviderManager @@ -351,6 +372,10 @@ private function createController(OpenIdConfigurationProvider $provider, ?Client ->method('isPkceEnabled') ->with('test') ->willReturn($pkce); + $mockProviderManager + ->method('getScopes') + ->with('test') + ->willReturn($scopes); return new LoginController($mockProviderManager, $this->logger, $expiryChecker ?? $this->createExpiryChecker()); } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 0694d96..ff3e0cd 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -543,4 +543,124 @@ public function testPkceCanBeTurnedOff(): void $this->assertFalse($config['openid_providers']['provider1']['options']['pkce']); } + + public function testScopesDefaultToTheOpenIdConnectBasics(): void + { + $config = $this->processor->processConfiguration($this->configuration, [$this->getMinimalConfig()]); + + $this->assertSame(['openid', 'email', 'profile'], $config['openid_providers']['provider1']['options']['scopes']); + } + + public function testScopesCanBeConfigured(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = ['openid', 'profile', 'groups']; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame(['openid', 'profile', 'groups'], $config['openid_providers']['provider1']['options']['scopes']); + } + + /** + * @return iterable + */ + public static function scopeStringProvider(): iterable + { + yield 'single space' => ['openid profile groups', ['openid', 'profile', 'groups']]; + yield 'surrounding whitespace' => [' openid profile ', ['openid', 'profile']]; + yield 'runs of whitespace' => ["openid\t\tprofile\ngroups", ['openid', 'profile', 'groups']]; + yield 'one scope' => ['openid', ['openid']]; + } + + /** + * An environment variable can only carry a scalar, so the space-delimited form + * RFC 6749 §3.3 already uses on the wire is accepted here too. + * + * @param string[] $expected + */ + #[DataProvider('scopeStringProvider')] + public function testScopesAcceptASpaceSeparatedString(string $configured, array $expected): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = $configured; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame($expected, $config['openid_providers']['provider1']['options']['scopes']); + } + + /** + * @return iterable + */ + public static function scopesWithoutOpenIdProvider(): iterable + { + yield 'a list' => [['email', 'profile']]; + yield 'a string' => ['email profile']; + } + + #[DataProvider('scopesWithoutOpenIdProvider')] + public function testScopesMustIncludeOpenId(mixed $configured): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = $configured; + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('scopes must include openid: without it the provider returns no ID token.'); + $this->processor->processConfiguration($this->configuration, [$input]); + } + + public function testScopesCannotBeEmpty(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = []; + + $this->expectException(InvalidConfigurationException::class); + $this->processor->processConfiguration($this->configuration, [$input]); + } + + /** + * @return iterable + */ + public static function negativeDurationProvider(): iterable + { + yield 'leeway' => ['leeway', -1]; + yield 'cache_duration' => ['cache_duration', -1]; + } + + /** + * Rejected while the container compiles rather than at the first login that + * needs the value. + */ + #[DataProvider('negativeDurationProvider')] + public function testDurationsCannotBeNegative(string $option, int $value): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options'][$option] = $value; + + $this->expectException(InvalidConfigurationException::class); + $this->processor->processConfiguration($this->configuration, [$input]); + } + + /** + * @return iterable + */ + public static function zeroableDurationProvider(): iterable + { + yield 'leeway' => ['leeway']; + yield 'cache_duration' => ['cache_duration']; + } + + /** + * Zero is a coherent setting for both: no clock-skew window, and no caching. + */ + #[DataProvider('zeroableDurationProvider')] + public function testDurationsMayBeZero(string $option): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options'][$option] = 0; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame(0, $config['openid_providers']['provider1']['options'][$option]); + } } diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index d5db498..148a23b 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -9,6 +9,7 @@ use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use ItkDev\OpenIdConnectBundle\Exception\StatelessFirewallException; use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException; use ItkDev\OpenIdConnectBundle\Exception\UsernameDoesNotExistException; use PHPUnit\Framework\Attributes\DataProvider; @@ -38,6 +39,9 @@ public static function concreteProvider(): iterable yield 'InvalidProviderException' => [InvalidProviderException::class, \InvalidArgumentException::class]; yield 'UsernameDoesNotExistException' => [UsernameDoesNotExistException::class, \InvalidArgumentException::class]; + // Programmer error that should be fixed in code → \LogicException + yield 'StatelessFirewallException' => [StatelessFirewallException::class, \LogicException::class]; + // Runtime conditions → \RuntimeException yield 'CacheException' => [CacheException::class, \RuntimeException::class]; yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class]; diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 5fe8982..2c3ffdd 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -11,6 +11,7 @@ use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use ItkDev\OpenIdConnectBundle\Exception\StatelessFirewallException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; @@ -18,6 +19,7 @@ use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use Psr\Log\LogLevel; +use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\SessionInterface; @@ -821,6 +823,36 @@ public function testTheProviderErrorLeavesOnAuthenticationFailureUnwrapped(): vo $this->fail('Expected ProviderErrorException'); } + /** + * Symfony dispatches `security.interactive_login` for an authenticator that says + * so, and keys remember-me off it. + */ + public function testTheAuthenticatorIsInteractive(): void + { + $this->assertTrue($this->authenticator->isInteractive()); + } + + /** + * The flow spans two requests and the session is what ties them together, so a + * firewall with none can never complete a login. Named as the misconfiguration it + * is rather than surfacing Symfony's SessionNotFoundException as a 500. + */ + public function testAStatelessFirewallIsNamedAsTheProblem(): void + { + // A Request with no session set is what a stateless firewall hands over. + $request = new Request(query: ['state' => 'test_state', 'code' => 'test_code']); + + try { + $this->authenticator->authenticate($request); + } catch (StatelessFirewallException $thrown) { + $this->assertStringContainsString('stateless: true', $thrown->getMessage()); + $this->assertInstanceOf(SessionNotFoundException::class, $thrown->getPrevious()); + + return; + } + $this->fail('Expected StatelessFirewallException'); + } + public function testTheStoredVerifierIsSentWithTheTokenRequest(): void { $claims = new \stdClass();