Add an option refusing the implicit flow for confidential clients

config/module_oidc.php.dist

@@ -256,6 +256,17 @@ $config = [
     // claims for which "SAML attribute to OIDC claim translation" has been defined above.
     ModuleConfig::OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED => false,
 
+    // Optional hardening, disabled by default. If enabled, a confidential client requesting the implicit flow
+    // (response_type 'id_token' or 'id_token token') is refused with the 'unauthorized_client' error, so it can only
+    // get tokens through the authorization code flow, where it authenticates at the token endpoint. Public clients
+    // are not affected, and a confidential client which uses the implicit flow today stops working once this is on.
+    // This goes beyond what the specifications require, as they do not forbid the flow for confidential clients.
+    // OIDC Core says the authorization code flow may be appropriate for them
+    // (https://openid.net/specs/openid-connect-core-1_0.html#RPMTI), and the OAuth 2.0 Security BCP advises clients
+    // against response types which issue access tokens in the authorization response, such as 'id_token token'
+    // (https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2).
+    ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS => false,
+
     // Settings regarding Authentication Processing Filters.
     // Note: OIDC authN state array will not contain all the keys which are available during SAML authN,
     // like Service Provider metadata, etc.

src/Factories/Grant/ImplicitGrantFactory.php

@@ -45,6 +45,7 @@ public function build(): ImplicitGrant
             $this->requestParamsResolver,
             '#',
             $this->accessTokenEntityFactory,
+            $this->moduleConfig->getDisallowImplicitForConfidentialClients(),
         );
     }
 }

src/ModuleConfig.php

@@ -96,6 +96,8 @@ class ModuleConfig
     final public const string OPTION_PROTOCOL_CLIENT_ENTITY_CACHE_DURATION = 'protocol_client_entity_cache_duration';
     final public const string OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED =
     'protocol_discover_show_claims_supported';
+    final public const string OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS =
+    'disallow_implicit_for_confidential_clients';
 
     final public const string OPTION_PKI_NEW_PRIVATE_KEY_PASSPHRASE = 'new_private_key_passphrase';
     final public const string OPTION_PKI_NEW_PRIVATE_KEY_FILENAME = 'new_privatekey';
@@ -527,6 +529,18 @@ public function getProtocolDiscoveryShowClaimsSupported(): bool
         );
     }
 
+    /**
+     * Opt-in hardening: refuse implicit flow requests from confidential clients. See the comment on this option in
+     * config/module_oidc.php.dist.
+     */
+    public function getDisallowImplicitForConfidentialClients(): bool
+    {
+        return $this->config()->getOptionalBoolean(
+            self::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS,
+            false,
+        );
+    }
+
 
     /*****************************************************************************************************************
      * OpenID Federation related config.

src/Server/Exceptions/OidcServerException.php

@@ -179,6 +179,31 @@ public static function accessDenied(
         return $e;
     }
 
+    /**
+     * The client is not authorized to use this response type (RFC 6749 section 4.2.2.1).
+     *
+     * @param string|null $hint
+     * @param string|null $redirectUri
+     * @param \Throwable|null $previous
+     * @param string|null $state
+     * @param bool $useFragment Use URI fragment to return error parameters
+     * @return self
+     */
+    public static function unauthorizedClient(
+        ?string $hint = null,
+        ?string $redirectUri = null,
+        ?Throwable $previous = null,
+        ?string $state = null,
+        bool $useFragment = false,
+    ): OidcServerException {
+        $errorMessage = 'The client is not authorized to request a token using this method.';
+
+        $e = new self($errorMessage, 10, 'unauthorized_client', 400, $hint, $redirectUri, $previous, $state);
+        $e->useFragmentInHttpResponses($useFragment);
+
+        return $e;
+    }
+
     /**
      * Prompt none requires that user should be authenticated.
      *

src/Server/Grants/ImplicitGrant.php

@@ -64,6 +64,7 @@ public function __construct(
         protected RequestParamsResolver $requestParamsResolver,
         protected string $queryDelimiter,
         AccessTokenEntityFactory $accessTokenEntityFactory,
+        protected bool $disallowImplicitForConfidentialClients = false,
     ) {
         parent::__construct($accessTokenTTL, $queryDelimiter);
 
@@ -144,6 +145,18 @@ public function validateAuthorizationRequestWithRequestRules(
         /** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */
         $client = $resultBag->getOrFail(ClientIdRule::class)->getValue();
 
+        // Opt-in hardening, see ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS. The implicit flow
+        // never authenticates the client, which a confidential client could do at the token endpoint.
+        if ($this->disallowImplicitForConfidentialClients && $client->isConfidential()) {
+            throw OidcServerException::unauthorizedClient(
+                'Confidential clients are not allowed to use the implicit grant.',
+                $redirectUri,
+                null,
+                $state,
+                $this->shouldUseFragment(),
+            );
+        }
+
         // Some rules need certain things available in order to work properly...
         $this->requestRulesManager->setData('default_scope', $this->defaultScope);
         $this->requestRulesManager->setData('scope_delimiter_string', self::SCOPE_DELIMITER_STRING);

src/Server/Grants/RefreshTokenGrant.php

@@ -103,7 +103,7 @@ protected function validateOldRefreshToken(ServerRequestInterface $request, $cli
 
     protected function issueRefreshToken(
         OAuth2AccessTokenEntityInterface $accessToken,
-        string $authCodeId = null,
+        ?string $authCodeId = null,
     ): ?RefreshTokenEntityInterface {
         if (! is_a($accessToken, AccessTokenEntityInterface::class)) {
             throw OidcServerException::serverError('Unexpected access token entity type.');

tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php

@@ -0,0 +1,93 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SimpleSAML\Test\Module\oidc\unit\Factories\Grant;
+
+use DateInterval;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\ServerRequestInterface;
+use RuntimeException;
+use SimpleSAML\Module\oidc\Entities\ClientEntity;
+use SimpleSAML\Module\oidc\Factories\Entities\AccessTokenEntityFactory;
+use SimpleSAML\Module\oidc\Factories\Grant\ImplicitGrantFactory;
+use SimpleSAML\Module\oidc\ModuleConfig;
+use SimpleSAML\Module\oidc\Repositories\AccessTokenRepository;
+use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException;
+use SimpleSAML\Module\oidc\Server\RequestRules\RequestRulesManager;
+use SimpleSAML\Module\oidc\Server\RequestRules\Result;
+use SimpleSAML\Module\oidc\Server\RequestRules\ResultBag;
+use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientIdRule;
+use SimpleSAML\Module\oidc\Server\RequestRules\Rules\RedirectUriRule;
+use SimpleSAML\Module\oidc\Server\RequestRules\Rules\StateRule;
+use SimpleSAML\Module\oidc\Services\IdTokenBuilder;
+use SimpleSAML\Module\oidc\Utils\RequestParamsResolver;
+
+#[CoversClass(ImplicitGrantFactory::class)]
+class ImplicitGrantFactoryTest extends TestCase
+{
+    protected MockObject $moduleConfigMock;
+
+    protected MockObject $requestRulesManagerMock;
+
+
+    protected function setUp(): void
+    {
+        $this->moduleConfigMock = $this->createMock(ModuleConfig::class);
+        $this->moduleConfigMock->method('getAccessTokenDuration')->willReturn(new DateInterval('PT1H'));
+        $this->requestRulesManagerMock = $this->createMock(RequestRulesManager::class);
+    }
+
+
+    protected function buildAndValidateForConfidentialClient(): void
+    {
+        $clientEntityMock = $this->createMock(ClientEntity::class);
+        $clientEntityMock->method('isConfidential')->willReturn(true);
+
+        $resultBag = new ResultBag();
+        $resultBag->add(new Result(RedirectUriRule::class, 'https://example.com/callback'));
+        $resultBag->add(new Result(StateRule::class, 'test-state'));
+        $resultBag->add(new Result(ClientIdRule::class, $clientEntityMock));
+
+        $implicitGrant = (new ImplicitGrantFactory(
+            $this->moduleConfigMock,
+            $this->createMock(IdTokenBuilder::class),
+            $this->requestRulesManagerMock,
+            $this->createMock(AccessTokenRepository::class),
+            $this->createMock(RequestParamsResolver::class),
+            $this->createMock(AccessTokenEntityFactory::class),
+        ))->build();
+
+        $implicitGrant->validateAuthorizationRequestWithRequestRules(
+            $this->createMock(ServerRequestInterface::class),
+            $resultBag,
+        );
+    }
+
+
+    public function testBuiltGrantRefusesConfidentialClientWhenOptionIsOn(): void
+    {
+        $this->moduleConfigMock->method('getDisallowImplicitForConfidentialClients')->willReturn(true);
+        $this->requestRulesManagerMock->expects($this->never())->method('check');
+
+        $this->expectException(OidcServerException::class);
+
+        $this->buildAndValidateForConfidentialClient();
+    }
+
+
+    public function testBuiltGrantLetsConfidentialClientThroughWhenOptionIsOff(): void
+    {
+        $this->moduleConfigMock->method('getDisallowImplicitForConfidentialClients')->willReturn(false);
+        // The rule check is where the request goes next; failing there stops the test before the rest of the flow.
+        $this->requestRulesManagerMock->expects($this->once())->method('check')
+            ->willThrowException(new RuntimeException('rules executed'));
+
+        $this->expectException(RuntimeException::class);
+        $this->expectExceptionMessage('rules executed');
+
+        $this->buildAndValidateForConfidentialClient();
+    }
+}

tests/unit/src/ModuleConfigTest.php

@@ -400,6 +400,16 @@ public function testCanGetProtocolDiscoveryShowClaimsSupported(): void
         );
     }
 
+    public function testCanGetDisallowImplicitForConfidentialClients(): void
+    {
+        $this->assertFalse($this->sut()->getDisallowImplicitForConfidentialClients());
+        $this->assertTrue(
+            $this->sut(
+                overrides: [ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS => true],
+            )->getDisallowImplicitForConfidentialClients(),
+        );
+    }
+
     public function testCanGetProtocolNewCertPath(): void
     {
         $this->assertNull($this->sut()->getProtocolNewCertPath());

tests/unit/src/Server/Grants/ImplicitGrantTest.php

@@ -10,15 +10,23 @@
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
+use RuntimeException;
 use SimpleSAML\Module\oidc\Entities\ClientEntity;
 use SimpleSAML\Module\oidc\Entities\UserEntity;
 use SimpleSAML\Module\oidc\Factories\Entities\AccessTokenEntityFactory;
 use SimpleSAML\Module\oidc\Repositories\AccessTokenRepository;
 use SimpleSAML\Module\oidc\Repositories\Interfaces\AccessTokenRepositoryInterface;
+use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException;
 use SimpleSAML\Module\oidc\Server\Grants\ImplicitGrant;
 use SimpleSAML\Module\oidc\Server\RequestRules\Interfaces\ResultBagInterface;
 use SimpleSAML\Module\oidc\Server\RequestRules\RequestRulesManager;
+use SimpleSAML\Module\oidc\Server\RequestRules\Result;
+use SimpleSAML\Module\oidc\Server\RequestRules\ResultBag;
+use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientIdRule;
+use SimpleSAML\Module\oidc\Server\RequestRules\Rules\RedirectUriRule;
+use SimpleSAML\Module\oidc\Server\RequestRules\Rules\StateRule;
 use SimpleSAML\Module\oidc\Server\RequestTypes\AuthorizationRequest;
 use SimpleSAML\Module\oidc\Services\IdTokenBuilder;
 use SimpleSAML\Module\oidc\Utils\RequestParamsResolver;
@@ -69,6 +77,7 @@ protected function sut(
         ?string $queryDelimiter = null,
         ?AccessTokenEntityFactory $accessTokenEntityFactory = null,
         ?ScopeRepositoryInterface $scopeRepository = null,
+        bool $disallowImplicitForConfidentialClients = false,
     ): ImplicitGrant {
         $idTokenBuilder ??= $this->idTokenBuilderMock;
         $accessTokenTtl ??= $this->accessTokenTtl1h;
@@ -88,6 +97,7 @@ protected function sut(
             $requestParamsResolver,
             $queryDelimiter,
             $accessTokenEntityFactory,
+            $disallowImplicitForConfidentialClients,
         );
 
         $implicitGrant->setScopeRepository($scopeRepository);
@@ -171,4 +181,68 @@ public function testCanValidateAuthorizationRequestWithRequestRules(): void
     {
         $this->markTestIncomplete('RequestRulesManager needs to be refactored so it can be strongly typed.');
     }
+
+    protected function resultBagFor(bool $isConfidential): ResultBag
+    {
+        $this->clientEntityMock->method('isConfidential')->willReturn($isConfidential);
+
+        $resultBag = new ResultBag();
+        $resultBag->add(new Result(RedirectUriRule::class, 'https://example.com/callback'));
+        $resultBag->add(new Result(StateRule::class, 'test-state'));
+        $resultBag->add(new Result(ClientIdRule::class, $this->clientEntityMock));
+
+        return $resultBag;
+    }
+
+    public function testValidateAuthorizationRequestRefusesConfidentialClientWhenDisallowed(): void
+    {
+        $this->requestRulesManagerMock->expects($this->never())->method('check');
+
+        $responseMock = $this->createMock(ResponseInterface::class);
+        $responseMock->expects($this->once())->method('withStatus')->with(302)->willReturnSelf();
+        $responseMock->expects($this->once())->method('withHeader')
+            ->with(
+                'Location',
+                'https://example.com/callback#' . http_build_query([
+                    'error' => 'unauthorized_client',
+                    'error_description' => 'The client is not authorized to request a token using this method. ' .
+                        '(Confidential clients are not allowed to use the implicit grant.)',
+                    'state' => 'test-state',
+                ]),
+            )
+            ->willReturnSelf();
+
+        try {
+            $this->sut(disallowImplicitForConfidentialClients: true)
+                ->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(true));
+            $this->fail('The request should have been refused.');
+        } catch (OidcServerException $exception) {
+            $this->assertSame('unauthorized_client', $exception->getErrorType());
+            $exception->generateHttpResponse($responseMock);
+        }
+    }
+
+    public function testValidateAuthorizationRequestContinuesForPublicClientWhenDisallowed(): void
+    {
+        // The rule check is where the request goes next; failing there stops the test before the rest of the flow.
+        $this->requestRulesManagerMock->expects($this->once())->method('check')
+            ->willThrowException(new RuntimeException('rules executed'));
+
+        $this->expectException(RuntimeException::class);
+        $this->expectExceptionMessage('rules executed');
+
+        $this->sut(disallowImplicitForConfidentialClients: true)
+            ->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(false));
+    }
+
+    public function testValidateAuthorizationRequestContinuesForConfidentialClientByDefault(): void
+    {
+        $this->requestRulesManagerMock->expects($this->once())->method('check')
+            ->willThrowException(new RuntimeException('rules executed'));
+
+        $this->expectException(RuntimeException::class);
+        $this->expectExceptionMessage('rules executed');
+
+        $this->sut()->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(true));
+    }
 }