From 7f3844aafbbe0755af69f41ec439ce1adf8872ef Mon Sep 17 00:00:00 2001 From: Ioannis Igoumenos Date: Thu, 8 Oct 2026 08:05:49 +0000 Subject: [PATCH 1/3] test: add confidential client flow integration tests --- src/Server/Grants/RefreshTokenGrant.php | 2 +- tests/bootstrap.php | 17 +- .../src/Flows/ConfidentialClientFlowsTest.php | 288 ++++++++++++++++++ 3 files changed, 305 insertions(+), 2 deletions(-) create mode 100644 tests/integration/src/Flows/ConfidentialClientFlowsTest.php diff --git a/src/Server/Grants/RefreshTokenGrant.php b/src/Server/Grants/RefreshTokenGrant.php index ec7b58d1a..5bcfd3f0d 100644 --- a/src/Server/Grants/RefreshTokenGrant.php +++ b/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.'); diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 9c16119c8..3a4c6698b 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -15,7 +15,11 @@ */ $projectRoot = dirname(__DIR__); -require_once $projectRoot . '/vendor/autoload.php'; +if (file_exists($projectRoot . '/vendor/autoload.php')) { + require_once $projectRoot . '/vendor/autoload.php'; +} else { + require_once dirname(__DIR__, 3) . '/vendor/autoload.php'; +} // Symlink module into ssp vendor lib so that templates and urls can resolve correctly $linkPath = $projectRoot . '/vendor/simplesamlphp/simplesamlphp/modules/oidc'; @@ -25,3 +29,14 @@ } else { echo "'$linkPath' linked to '$projectRoot'\n"; } + +$certDir = $projectRoot . '/vendor/simplesamlphp/simplesamlphp/cert'; +$mainCertDir = dirname(__DIR__, 3) . '/cert'; +if (file_exists($mainCertDir) && file_exists($certDir)) { + foreach (glob($mainCertDir . '/*') as $certFile) { + $dest = $certDir . '/' . basename($certFile); + if (!file_exists($dest)) { + copy($certFile, $dest); + } + } +} diff --git a/tests/integration/src/Flows/ConfidentialClientFlowsTest.php b/tests/integration/src/Flows/ConfidentialClientFlowsTest.php new file mode 100644 index 000000000..87143388c --- /dev/null +++ b/tests/integration/src/Flows/ConfidentialClientFlowsTest.php @@ -0,0 +1,288 @@ +toArray(); + $config['database.dsn'] = 'sqlite::memory:'; + $config['database.username'] = null; + $config['database.password'] = null; + $config['database.prefix'] = 'phpunit_'; + $config['database.persistent'] = true; + $config['database.secondaries'] = []; + + Configuration::loadFromArray($config, '', 'simplesaml'); + (new DatabaseMigration())->migrate(); + } + + protected function setUp(): void + { + $this->originalServer = $_SERVER; + $_SERVER['HTTP_HOST'] = 'localhost'; + $_SERVER['SERVER_NAME'] = 'localhost'; + $_SERVER['SERVER_PORT'] = '80'; + $_SERVER['REQUEST_URI'] = '/module.php/oidc/authorization'; + $_SERVER['REQUEST_METHOD'] = 'GET'; + + Configuration::setConfigDir(__DIR__ . '/../../../config'); + $this->container = new Container(); + + $this->clientRepo = $this->container->get(ClientRepository::class); + $this->userRepo = $this->container->get(UserRepository::class); + $this->clientFactory = $this->container->get(ClientEntityFactory::class); + $this->authServer = $this->container->get(AuthorizationServer::class); + + // Set up test user + $this->user = new UserEntity( + $this->userId, + new DateTimeImmutable(), + new DateTimeImmutable(), + ['sub' => $this->userId, 'eduPersonPrincipalName' => [$this->userId]] + ); + $existingUser = $this->userRepo->getUserEntityByIdentifier($this->userId); + if ($existingUser) { + $this->userRepo->delete($existingUser); + } + $this->userRepo->add($this->user); + + // Set up confidential client + $existingClient = $this->clientRepo->findById($this->clientId); + if ($existingClient) { + $this->clientRepo->delete($existingClient); + } + + $this->client = $this->clientFactory->fromData( + id: $this->clientId, + secret: $this->clientSecret, + name: 'Confidential Flow Test Client', + description: 'Testing confidential client flows and implicit grant restriction', + redirectUri: [$this->redirectUri], + scopes: ['openid', 'profile', 'email'], + isEnabled: true, + isConfidential: true, // CONFIDENTIAL CLIENT + ); + $this->clientRepo->add($this->client); + } + + protected function tearDown(): void + { + $_SERVER = $this->originalServer; + + if (isset($this->client)) { + $existingClient = $this->clientRepo->findById($this->clientId); + if ($existingClient) { + $this->clientRepo->delete($existingClient); + } + } + if (isset($this->user)) { + $existingUser = $this->userRepo->getUserEntityByIdentifier($this->userId); + if ($existingUser) { + $this->userRepo->delete($existingUser); + } + } + } + + /** + * Test that confidential clients MUST NOT be permitted to use the Implicit Flow + * (response_type=id_token token or id_token). + * + * Per RFC 9700 Section 2.1.2, RFC 6749 Section 10.3, and RFC 7591 Section 2: + * Confidential clients must authenticate to receive tokens, and implicit grant + * must not issue tokens to confidential clients without client credentials. + * + * THIS TEST IS EXPECTED TO FAIL ON THE CURRENT IMPLEMENTATION because the OP + * allows confidential clients to obtain tokens via implicit flow without authentication. + */ + public function testConfidentialClientCannotUseImplicitFlow(): void + { + $implicitRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'id_token token', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'xyz123', + 'nonce' => 'nonce456', + ]); + + // A compliant OP must reject implicit flow requests for confidential clients + // (either during request validation or request completion by throwing an OAuth/OIDC exception) + $this->expectException(\Throwable::class); + $this->expectExceptionMessageMatches('/(unauthorized_client|invalid_grant|unsupported_response_type|not allowed|confidential)/i'); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($implicitRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $response = $this->authServer->completeAuthorizationRequest( + $validatedAuthReq, + new Response() + ); + + // If no exception was thrown, check whether tokens were issued in the fragment (violating guidelines) + $location = $response->getHeaderLine('Location'); + $this->assertStringNotContainsString('access_token=', $location, 'Confidential client received access_token via implicit flow!'); + $this->assertStringNotContainsString('id_token=', $location, 'Confidential client received id_token via implicit flow!'); + } + + /** + * Test that confidential clients using Authorization Code Flow with PKCE + * MUST authenticate with client credentials at the token endpoint. + */ + public function testConfidentialClientWithPkceRequiresSecretAtTokenEndpoint(): void + { + $codeVerifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + $codeChallenge = rtrim(strtr(base64_encode(hash('sha256', $codeVerifier, true)), '+/', '-_'), '='); + + // Step 1: Authorization Request (receives auth code) + $authCodeRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'code', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'state-pkce', + 'nonce' => 'nonce-pkce', + 'code_challenge' => $codeChallenge, + 'code_challenge_method' => 'S256', + ]); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($authCodeRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $authResponse = $this->authServer->completeAuthorizationRequest( + $validatedAuthReq, + new Response() + ); + + $location = $authResponse->getHeaderLine('Location'); + parse_str((string) parse_url($location, PHP_URL_QUERY), $queryParams); + $authCode = $queryParams['code'] ?? null; + $this->assertNotEmpty($authCode, 'Authorization code was not issued'); + + // Step 2: Token request WITHOUT client secret must be rejected + $tokenRequestNoSecret = (new ServerRequest('POST', '/module.php/oidc/token')) + ->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => $this->clientId, + 'code' => $authCode, + 'redirect_uri' => $this->redirectUri, + 'code_verifier' => $codeVerifier, + ]); + + $this->expectException(OAuthServerException::class); + $this->authServer->respondToAccessTokenRequest( + $tokenRequestNoSecret, + new Response() + ); + } + + /** + * Test that confidential clients using standard Authorization Code Flow (no PKCE) + * cannot exchange without secret, but succeed when valid secret is provided. + */ + public function testConfidentialClientWithoutPkceRequiresSecretAndSucceedsWithSecret(): void + { + // Step 1: Authorization Request (no PKCE) + $authCodeRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'code', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'state-no-pkce', + 'nonce' => 'nonce-no-pkce', + ]); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($authCodeRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $authResponse = $this->authServer->completeAuthorizationRequest( + $validatedAuthReq, + new Response() + ); + + $location = $authResponse->getHeaderLine('Location'); + parse_str((string) parse_url($location, PHP_URL_QUERY), $queryParams); + $authCode = $queryParams['code'] ?? null; + $this->assertNotEmpty($authCode, 'Authorization code was not issued'); + + // Step 2a: Token request WITHOUT client secret must fail + $tokenRequestNoSecret = (new ServerRequest('POST', '/module.php/oidc/token')) + ->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => $this->clientId, + 'code' => $authCode, + 'redirect_uri' => $this->redirectUri, + ]); + + try { + $this->authServer->respondToAccessTokenRequest( + $tokenRequestNoSecret, + new Response() + ); + $this->fail('Token endpoint unexpectedly issued tokens to confidential client without client credentials'); + } catch (OAuthServerException $e) { + $this->assertSame('access_denied', $e->getErrorType()); + } + + // Step 2b: Token request WITH valid client secret succeeds + $tokenRequestWithSecret = (new ServerRequest('POST', '/module.php/oidc/token')) + ->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => $this->clientId, + 'client_secret' => $this->clientSecret, + 'code' => $authCode, + 'redirect_uri' => $this->redirectUri, + ]); + + $tokenResponse = $this->authServer->respondToAccessTokenRequest( + $tokenRequestWithSecret, + new Response() + ); + + $this->assertSame(200, $tokenResponse->getStatusCode()); + $body = json_decode((string) $tokenResponse->getBody(), true); + $this->assertArrayHasKey('access_token', $body); + $this->assertArrayHasKey('id_token', $body); + } +} From fe734dbd2df430c032810fd9d61f4b6bd80c21b8 Mon Sep 17 00:00:00 2001 From: Ioannis Igoumenos Date: Thu, 8 Oct 2026 08:20:22 +0000 Subject: [PATCH 2/3] Disallow confidential clients from using implicit grant flow --- src/Server/Exceptions/OidcServerException.php | 27 ++++++++++++++++ src/Server/Grants/ImplicitGrant.php | 14 ++++++++ .../src/Server/Grants/ImplicitGrantTest.php | 32 +++++++++++++++++++ 3 files changed, 73 insertions(+) diff --git a/src/Server/Exceptions/OidcServerException.php b/src/Server/Exceptions/OidcServerException.php index 5a9be60db..218dde70e 100644 --- a/src/Server/Exceptions/OidcServerException.php +++ b/src/Server/Exceptions/OidcServerException.php @@ -106,6 +106,33 @@ public static function unsupportedResponseType( return $e; } + /** + * Unauthorized client error. + * + * @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1 + * @see https://datatracker.ietf.org/doc/html/rfc6749#section-10.3 + * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + * @see https://datatracker.ietf.org/doc/html/rfc7591#section-2 + * + * @param string|null $hint + * @param string|null $redirectUri + * @param string|null $state + * @param bool $useFragment + * @return self + */ + public static function unauthorizedClient( + ?string $hint = null, + ?string $redirectUri = null, + ?string $state = null, + bool $useFragment = false, + ): self { + $errorMessage = 'The client is not authorized to request an authorization code or access token using this method (unauthorized_client).'; + $e = new self($errorMessage, 4, 'unauthorized_client', 400, $hint, $redirectUri, null, $state); + $e->useFragmentInHttpResponses($useFragment); + + return $e; + } + /** * Invalid scope error. * diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 4e2026bcc..4bfdff31d 100644 --- a/src/Server/Grants/ImplicitGrant.php +++ b/src/Server/Grants/ImplicitGrant.php @@ -144,6 +144,20 @@ public function validateAuthorizationRequestWithRequestRules( /** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */ $client = $resultBag->getOrFail(ClientIdRule::class)->getValue(); + // Confidential clients MUST authenticate and are not permitted to use the implicit flow: + // https://datatracker.ietf.org/doc/html/rfc6749#section-10.3 + // https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + // https://datatracker.ietf.org/doc/html/rfc7591#section-2 + // https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication + if ($client->isConfidential()) { + throw OidcServerException::unauthorizedClient( + 'Confidential clients are not allowed to use the implicit grant.', + $redirectUri, + $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); diff --git a/tests/unit/src/Server/Grants/ImplicitGrantTest.php b/tests/unit/src/Server/Grants/ImplicitGrantTest.php index d28dabda8..75e7c65f8 100644 --- a/tests/unit/src/Server/Grants/ImplicitGrantTest.php +++ b/tests/unit/src/Server/Grants/ImplicitGrantTest.php @@ -171,4 +171,36 @@ public function testCanValidateAuthorizationRequestWithRequestRules(): void { $this->markTestIncomplete('RequestRulesManager needs to be refactored so it can be strongly typed.'); } + + /** + * @see https://datatracker.ietf.org/doc/html/rfc6749#section-10.3 + * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + */ + public function testValidateAuthorizationRequestThrowsForConfidentialClient(): void + { + $redirectUriResult = new \SimpleSAML\Module\oidc\Server\RequestRules\Result( + \SimpleSAML\Module\oidc\Server\RequestRules\Rules\RedirectUriRule::class, + 'https://example.com/callback' + ); + $stateResult = new \SimpleSAML\Module\oidc\Server\RequestRules\Result( + \SimpleSAML\Module\oidc\Server\RequestRules\Rules\StateRule::class, + 'test-state' + ); + + $this->clientEntityMock->method('isConfidential')->willReturn(true); + $clientResult = new \SimpleSAML\Module\oidc\Server\RequestRules\Result( + \SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientIdRule::class, + $this->clientEntityMock + ); + + $resultBag = new \SimpleSAML\Module\oidc\Server\RequestRules\ResultBag(); + $resultBag->add($redirectUriResult); + $resultBag->add($stateResult); + $resultBag->add($clientResult); + + $this->expectException(\SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException::class); + $this->expectExceptionMessageMatches('/unauthorized_client/'); + + $this->sut()->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $resultBag); + } } From 981ea181710d1596c0686501185795343d131c82 Mon Sep 17 00:00:00 2001 From: Ioannis Igoumenos Date: Wed, 21 Oct 2026 07:19:22 +0000 Subject: [PATCH 3/3] add opt-in guard against implicit flow for confidential clients --- config/module_oidc.php.dist | 9 +++ src/Factories/Grant/ImplicitGrantFactory.php | 1 + src/ModuleConfig.php | 16 +++++ src/Server/Exceptions/OidcServerException.php | 6 +- src/Server/Grants/ImplicitGrant.php | 12 ++-- .../src/Flows/ConfidentialClientFlowsTest.php | 70 +++++++++++++++---- .../src/Server/Grants/ImplicitGrantTest.php | 38 ++++++++-- 7 files changed, 126 insertions(+), 26 deletions(-) diff --git a/config/module_oidc.php.dist b/config/module_oidc.php.dist index a83f02fe9..836104a7d 100644 --- a/config/module_oidc.php.dist +++ b/config/module_oidc.php.dist @@ -256,6 +256,15 @@ $config = [ // claims for which "SAML attribute to OIDC claim translation" has been defined above. ModuleConfig::OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED => false, + // Optional security hardening (disabled by default). If enabled, confidential clients will be rejected with + // 'unauthorized_client' when they try to use the implicit flow (response_type 'id_token' or 'id_token token'), + // so they can only get tokens using the authorization code flow, where they authenticate at the token endpoint. + // The specifications do not forbid confidential clients from using the implicit flow, but OAuth 2.0 Security + // BCP advises against it (https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2), and OIDC Core notes + // that confidential clients would normally use the authorization code flow + // (https://openid.net/specs/openid-connect-core-1_0.html#RPMTI). + 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. diff --git a/src/Factories/Grant/ImplicitGrantFactory.php b/src/Factories/Grant/ImplicitGrantFactory.php index a22791d84..d5c581761 100644 --- a/src/Factories/Grant/ImplicitGrantFactory.php +++ b/src/Factories/Grant/ImplicitGrantFactory.php @@ -45,6 +45,7 @@ public function build(): ImplicitGrant $this->requestParamsResolver, '#', $this->accessTokenEntityFactory, + $this->moduleConfig->getDisallowImplicitForConfidentialClients(), ); } } diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index 2f42d0852..296cabcf3 100644 --- a/src/ModuleConfig.php +++ b/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,20 @@ public function getProtocolDiscoveryShowClaimsSupported(): bool ); } + /** + * Opt-in hardening: reject implicit flow requests from confidential clients. + * + * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + * @see https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + */ + public function getDisallowImplicitForConfidentialClients(): bool + { + return $this->config()->getOptionalBoolean( + self::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS, + false, + ); + } + /***************************************************************************************************************** * OpenID Federation related config. diff --git a/src/Server/Exceptions/OidcServerException.php b/src/Server/Exceptions/OidcServerException.php index 218dde70e..1ac5bf415 100644 --- a/src/Server/Exceptions/OidcServerException.php +++ b/src/Server/Exceptions/OidcServerException.php @@ -110,9 +110,6 @@ public static function unsupportedResponseType( * Unauthorized client error. * * @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1 - * @see https://datatracker.ietf.org/doc/html/rfc6749#section-10.3 - * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 - * @see https://datatracker.ietf.org/doc/html/rfc7591#section-2 * * @param string|null $hint * @param string|null $redirectUri @@ -126,7 +123,8 @@ public static function unauthorizedClient( ?string $state = null, bool $useFragment = false, ): self { - $errorMessage = 'The client is not authorized to request an authorization code or access token using this method (unauthorized_client).'; + $errorMessage = 'The client is not authorized to request an authorization code or access token ' . + 'using this method (unauthorized_client).'; $e = new self($errorMessage, 4, 'unauthorized_client', 400, $hint, $redirectUri, null, $state); $e->useFragmentInHttpResponses($useFragment); diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 4bfdff31d..89a3d20a3 100644 --- a/src/Server/Grants/ImplicitGrant.php +++ b/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,12 +145,13 @@ public function validateAuthorizationRequestWithRequestRules( /** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */ $client = $resultBag->getOrFail(ClientIdRule::class)->getValue(); - // Confidential clients MUST authenticate and are not permitted to use the implicit flow: - // https://datatracker.ietf.org/doc/html/rfc6749#section-10.3 + // Opt-in hardening (ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS, default false). + // The specs do not forbid confidential clients from using the implicit flow, but the implicit flow never + // authenticates the client, and OAuth 2.0 Security BCP advises against it: // https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 - // https://datatracker.ietf.org/doc/html/rfc7591#section-2 - // https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication - if ($client->isConfidential()) { + // OIDC Core notes that confidential clients would normally use the authorization code flow: + // https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + if ($this->disallowImplicitForConfidentialClients && $client->isConfidential()) { throw OidcServerException::unauthorizedClient( 'Confidential clients are not allowed to use the implicit grant.', $redirectUri, diff --git a/tests/integration/src/Flows/ConfidentialClientFlowsTest.php b/tests/integration/src/Flows/ConfidentialClientFlowsTest.php index 87143388c..b080a45d2 100644 --- a/tests/integration/src/Flows/ConfidentialClientFlowsTest.php +++ b/tests/integration/src/Flows/ConfidentialClientFlowsTest.php @@ -13,6 +13,7 @@ use SimpleSAML\Module\oidc\Entities\ClientEntity; use SimpleSAML\Module\oidc\Entities\UserEntity; use SimpleSAML\Module\oidc\Factories\Entities\ClientEntityFactory; +use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Repositories\ClientRepository; use SimpleSAML\Module\oidc\Repositories\UserRepository; use SimpleSAML\Module\oidc\Server\AuthorizationServer; @@ -36,6 +37,7 @@ class ConfidentialClientFlowsTest extends TestCase protected ClientEntity $client; protected UserEntity $user; protected array $originalServer; + protected Configuration $originalModuleConfig; public static function setUpBeforeClass(): void { @@ -62,12 +64,9 @@ protected function setUp(): void $_SERVER['REQUEST_METHOD'] = 'GET'; Configuration::setConfigDir(__DIR__ . '/../../../config'); - $this->container = new Container(); - - $this->clientRepo = $this->container->get(ClientRepository::class); - $this->userRepo = $this->container->get(UserRepository::class); - $this->clientFactory = $this->container->get(ClientEntityFactory::class); - $this->authServer = $this->container->get(AuthorizationServer::class); + $this->originalModuleConfig = Configuration::getConfig(ModuleConfig::DEFAULT_FILE_NAME); + // Enable the opt-in hardening for these tests. + $this->buildContainer(disallowImplicitForConfidentialClients: true); // Set up test user $this->user = new UserEntity( @@ -101,6 +100,23 @@ protected function setUp(): void $this->clientRepo->add($this->client); } + protected function buildContainer(bool $disallowImplicitForConfidentialClients): void + { + Configuration::setPreLoadedConfig( + Configuration::loadFromArray(array_merge( + $this->originalModuleConfig->toArray(), + [ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS => $disallowImplicitForConfidentialClients], + )), + ModuleConfig::DEFAULT_FILE_NAME, + ); + + $this->container = new Container(); + $this->clientRepo = $this->container->get(ClientRepository::class); + $this->userRepo = $this->container->get(UserRepository::class); + $this->clientFactory = $this->container->get(ClientEntityFactory::class); + $this->authServer = $this->container->get(AuthorizationServer::class); + } + protected function tearDown(): void { $_SERVER = $this->originalServer; @@ -117,18 +133,19 @@ protected function tearDown(): void $this->userRepo->delete($existingUser); } } + + Configuration::setPreLoadedConfig($this->originalModuleConfig, ModuleConfig::DEFAULT_FILE_NAME); } /** * Test that confidential clients MUST NOT be permitted to use the Implicit Flow * (response_type=id_token token or id_token). * - * Per RFC 9700 Section 2.1.2, RFC 6749 Section 10.3, and RFC 7591 Section 2: - * Confidential clients must authenticate to receive tokens, and implicit grant - * must not issue tokens to confidential clients without client credentials. - * - * THIS TEST IS EXPECTED TO FAIL ON THE CURRENT IMPLEMENTATION because the OP - * allows confidential clients to obtain tokens via implicit flow without authentication. + * Requires opt-in ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS (enabled in setUp). + * This is hardening, not a spec requirement. The implicit flow never authenticates the client and + * OAuth 2.0 Security BCP advises against it: https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + * OIDC Core notes confidential clients would normally use the code flow: + * https://openid.net/specs/openid-connect-core-1_0.html#RPMTI */ public function testConfidentialClientCannotUseImplicitFlow(): void { @@ -162,6 +179,35 @@ public function testConfidentialClientCannotUseImplicitFlow(): void $this->assertStringNotContainsString('id_token=', $location, 'Confidential client received id_token via implicit flow!'); } + /** + * With the opt-in option disabled (default), behaviour is unchanged: the implicit flow works for + * confidential clients, as the specs do not forbid it. + */ + public function testConfidentialClientCanUseImplicitFlowWhenOptionDisabled(): void + { + $this->buildContainer(disallowImplicitForConfidentialClients: false); + + $implicitRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'id_token token', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'xyz123', + 'nonce' => 'nonce456', + ]); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($implicitRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $response = $this->authServer->completeAuthorizationRequest($validatedAuthReq, new Response()); + + $location = $response->getHeaderLine('Location'); + $this->assertStringContainsString('access_token=', $location); + $this->assertStringContainsString('id_token=', $location); + } + /** * Test that confidential clients using Authorization Code Flow with PKCE * MUST authenticate with client credentials at the token endpoint. diff --git a/tests/unit/src/Server/Grants/ImplicitGrantTest.php b/tests/unit/src/Server/Grants/ImplicitGrantTest.php index 75e7c65f8..4f46416ae 100644 --- a/tests/unit/src/Server/Grants/ImplicitGrantTest.php +++ b/tests/unit/src/Server/Grants/ImplicitGrantTest.php @@ -69,6 +69,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 +89,7 @@ protected function sut( $requestParamsResolver, $queryDelimiter, $accessTokenEntityFactory, + $disallowImplicitForConfidentialClients, ); $implicitGrant->setScopeRepository($scopeRepository); @@ -172,11 +174,7 @@ public function testCanValidateAuthorizationRequestWithRequestRules(): void $this->markTestIncomplete('RequestRulesManager needs to be refactored so it can be strongly typed.'); } - /** - * @see https://datatracker.ietf.org/doc/html/rfc6749#section-10.3 - * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 - */ - public function testValidateAuthorizationRequestThrowsForConfidentialClient(): void + protected function confidentialClientResultBag(): \SimpleSAML\Module\oidc\Server\RequestRules\ResultBag { $redirectUriResult = new \SimpleSAML\Module\oidc\Server\RequestRules\Result( \SimpleSAML\Module\oidc\Server\RequestRules\Rules\RedirectUriRule::class, @@ -198,9 +196,39 @@ public function testValidateAuthorizationRequestThrowsForConfidentialClient(): v $resultBag->add($stateResult); $resultBag->add($clientResult); + return $resultBag; + } + + /** + * Opt-in hardening, see ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS. + * + * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + * @see https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + */ + public function testValidateAuthorizationRequestThrowsForConfidentialClientWhenDisallowed(): void + { + $resultBag = $this->confidentialClientResultBag(); + + $this->requestRulesManagerMock->expects($this->never())->method('check'); + $this->expectException(\SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException::class); $this->expectExceptionMessageMatches('/unauthorized_client/'); + $this->sut(disallowImplicitForConfidentialClients: true) + ->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $resultBag); + } + + public function testValidateAuthorizationRequestDoesNotRejectConfidentialClientByDefault(): void + { + $resultBag = $this->confidentialClientResultBag(); + + // Guard is off by default, so processing continues to the request rules. + $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, $resultBag); } }