From 7f3844aafbbe0755af69f41ec439ce1adf8872ef Mon Sep 17 00:00:00 2001 From: Ioannis Igoumenos Date: Thu, 8 Oct 2026 08:05:49 +0000 Subject: [PATCH 1/4] 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/4] 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/4] 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); } } From 54679256587e195cca3125de604c4c1cae68df28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Wed, 4 Nov 2026 12:12:12 +0100 Subject: [PATCH 4/4] Tighten the opt-in refusal of the implicit flow for confidential clients Give OidcServerException::unauthorizedClient() the message, code and parameter order it has on wip-version-7, which also follow its sibling methods here. Say in the comments what the specifications say. RFC 9700 section 2.1.2 is about access tokens in the authorization response, so it covers 'id_token token' but not 'id_token' alone, and OpenID Connect Core section 15.4 only says the authorization code flow may be appropriate for confidential clients. The option goes beyond both, and enabling it stops any confidential client which uses the implicit flow today. Replace the integration test, which CI could not run without a signing key, with unit tests pinning the whole error response, public clients, the default, and the option reaching the grant through the factory. Its cases passed once against this code, with a temporary key. tests/bootstrap.php goes back to what it was, as its additions served one local layout. Assisted-by: Claude Code (claude-opus-5-5, rework & unit tests) --- config/module_oidc.php.dist | 16 +- src/ModuleConfig.php | 6 +- src/Server/Exceptions/OidcServerException.php | 50 +-- src/Server/Grants/ImplicitGrant.php | 9 +- tests/bootstrap.php | 17 +- .../src/Flows/ConfidentialClientFlowsTest.php | 334 ------------------ .../Grant/ImplicitGrantFactoryTest.php | 93 +++++ tests/unit/src/ModuleConfigTest.php | 10 + .../src/Server/Grants/ImplicitGrantTest.php | 90 +++-- 9 files changed, 195 insertions(+), 430 deletions(-) delete mode 100644 tests/integration/src/Flows/ConfidentialClientFlowsTest.php create mode 100644 tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php diff --git a/config/module_oidc.php.dist b/config/module_oidc.php.dist index 836104a7d..a3a487b60 100644 --- a/config/module_oidc.php.dist +++ b/config/module_oidc.php.dist @@ -256,13 +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). + // 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. diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index 296cabcf3..48d36a06a 100644 --- a/src/ModuleConfig.php +++ b/src/ModuleConfig.php @@ -530,10 +530,8 @@ 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 + * 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 { diff --git a/src/Server/Exceptions/OidcServerException.php b/src/Server/Exceptions/OidcServerException.php index 1ac5bf415..e8f9bc220 100644 --- a/src/Server/Exceptions/OidcServerException.php +++ b/src/Server/Exceptions/OidcServerException.php @@ -106,31 +106,6 @@ public static function unsupportedResponseType( return $e; } - /** - * Unauthorized client error. - * - * @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1 - * - * @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. * @@ -204,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. * diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 89a3d20a3..753dd056e 100644 --- a/src/Server/Grants/ImplicitGrant.php +++ b/src/Server/Grants/ImplicitGrant.php @@ -145,16 +145,13 @@ public function validateAuthorizationRequestWithRequestRules( /** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */ $client = $resultBag->getOrFail(ClientIdRule::class)->getValue(); - // 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 - // OIDC Core notes that confidential clients would normally use the authorization code flow: - // https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + // 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(), ); diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 3a4c6698b..9c16119c8 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -15,11 +15,7 @@ */ $projectRoot = dirname(__DIR__); -if (file_exists($projectRoot . '/vendor/autoload.php')) { - require_once $projectRoot . '/vendor/autoload.php'; -} else { - require_once dirname(__DIR__, 3) . '/vendor/autoload.php'; -} +require_once $projectRoot . '/vendor/autoload.php'; // Symlink module into ssp vendor lib so that templates and urls can resolve correctly $linkPath = $projectRoot . '/vendor/simplesamlphp/simplesamlphp/modules/oidc'; @@ -29,14 +25,3 @@ } 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 deleted file mode 100644 index b080a45d2..000000000 --- a/tests/integration/src/Flows/ConfidentialClientFlowsTest.php +++ /dev/null @@ -1,334 +0,0 @@ -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->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( - $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 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; - - 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); - } - } - - 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). - * - * 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 - { - $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!'); - } - - /** - * 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. - */ - 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); - } -} diff --git a/tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php b/tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php new file mode 100644 index 000000000..579962c40 --- /dev/null +++ b/tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php @@ -0,0 +1,93 @@ +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(); + } +} diff --git a/tests/unit/src/ModuleConfigTest.php b/tests/unit/src/ModuleConfigTest.php index 94b84f8d4..429df6762 100644 --- a/tests/unit/src/ModuleConfigTest.php +++ b/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()); diff --git a/tests/unit/src/Server/Grants/ImplicitGrantTest.php b/tests/unit/src/Server/Grants/ImplicitGrantTest.php index 4f46416ae..6377ad98d 100644 --- a/tests/unit/src/Server/Grants/ImplicitGrantTest.php +++ b/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; @@ -174,61 +182,67 @@ public function testCanValidateAuthorizationRequestWithRequestRules(): void $this->markTestIncomplete('RequestRulesManager needs to be refactored so it can be strongly typed.'); } - protected function confidentialClientResultBag(): \SimpleSAML\Module\oidc\Server\RequestRules\ResultBag + protected function resultBagFor(bool $isConfidential): ResultBag { - $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 - ); + $this->clientEntityMock->method('isConfidential')->willReturn($isConfidential); - $resultBag = new \SimpleSAML\Module\oidc\Server\RequestRules\ResultBag(); - $resultBag->add($redirectUriResult); - $resultBag->add($stateResult); - $resultBag->add($clientResult); + $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; } - /** - * 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 + public function testValidateAuthorizationRequestRefusesConfidentialClientWhenDisallowed(): void { - $resultBag = $this->confidentialClientResultBag(); - $this->requestRulesManagerMock->expects($this->never())->method('check'); - $this->expectException(\SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException::class); - $this->expectExceptionMessageMatches('/unauthorized_client/'); + $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, $resultBag); + ->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(false)); } - public function testValidateAuthorizationRequestDoesNotRejectConfidentialClientByDefault(): void + public function testValidateAuthorizationRequestContinuesForConfidentialClientByDefault(): 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')); + ->willThrowException(new RuntimeException('rules executed')); - $this->expectException(\RuntimeException::class); + $this->expectException(RuntimeException::class); $this->expectExceptionMessage('rules executed'); - $this->sut()->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $resultBag); + $this->sut()->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(true)); } }