feat: configurable scopes, config bounds, and two clearer failures

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.

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.

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.

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) {

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

src/Exception/StatelessFirewallException.php

@@ -0,0 +1,19 @@
+<?php
+
+namespace ItkDev\OpenIdConnectBundle\Exception;
+
+/**
+ * The OpenID Connect authenticator was used on a firewall with no session.
+ *
+ * The authorization code flow spans two requests: one that sends the browser to the
+ * identity provider, and one that receives the callback. The state, the nonce and the
+ * PKCE verifier are what tie them together, and a session is where they wait. A
+ * firewall declared `stateless: true` has nowhere to keep them, so the callback can
+ * never be validated.
+ *
+ * A `\LogicException`, because it describes a firewall that cannot work rather than a
+ * login that went wrong: the fix is in `security.yaml`, not in a retry.
+ */
+class StatelessFirewallException extends \LogicException implements OpenIdConnectBundleExceptionInterface
+{
+}

src/Security/OpenIdConfigurationProviderManager.php

@@ -51,6 +51,7 @@ class OpenIdConfigurationProviderManager
      *         leeway?: int,
      *         cache_duration?: int,
      *         pkce?: bool,
+     *         scopes?: string[],
      *         allow_http?: bool,
      *         http_client_options?: array{
      *             timeout?: float,
@@ -76,6 +77,22 @@ public function getProviderKeys(): array
         return array_keys($this->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.
      *

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

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());
     }