From 08f9dd3642635ab22a493dbd457e9540156878a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Sun, 12 Jul 2026 16:27:43 +0200 Subject: [PATCH 1/2] Initial id_token_hint implementation --- docs/6-oidc-upgrade.md | 10 ++++ src/Server/Grants/AuthCodeGrant.php | 4 ++ src/Server/Grants/ImplicitGrant.php | 4 ++ .../RequestRules/Rules/IdTokenHintRule.php | 22 +++++-- src/Server/RequestRules/Rules/PromptRule.php | 53 ++++++++++++---- src/Services/AuthenticationService.php | 26 ++++++++ .../RequestRules/Rules/PromptRuleTest.php | 60 +++++++++++++++++++ .../Services/AuthenticationServiceTest.php | 42 +++++++++++++ 8 files changed, 203 insertions(+), 18 deletions(-) diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md index 78838580..5c6304e3 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -214,6 +214,16 @@ core login form simply ignore it, and an incorrect value can be corrected by the user (or the login fails with invalid credentials). No error is raised if the parameter is present but unused. This also applies to forced re-authentication triggered by `prompt=login` or an expired `max_age`. +- Support for the `id_token_hint` parameter on the authorization endpoint +(previously ignored; it was already supported on the end session endpoint). The +parameter carries an ID Token previously issued by this OP as a hint about the +End-User's session. When present, it is validated (issuer and signature). Its +main effect is with `prompt=none`: the request is only satisfied when the +currently authenticated End-User matches the subject (`sub`) in the +`id_token_hint`; otherwise a `login_required` error is returned rather than +silently issuing a token for a different user. A malformed, wrongly-issued or +improperly-signed `id_token_hint` results in an `invalid_request` error +redirected back to the client. - Logging has been improved for authentication flows. It should now be easier to find information about what went wrong by looking at the relevant log entries. diff --git a/src/Server/Grants/AuthCodeGrant.php b/src/Server/Grants/AuthCodeGrant.php index 604aac6f..616c1b42 100644 --- a/src/Server/Grants/AuthCodeGrant.php +++ b/src/Server/Grants/AuthCodeGrant.php @@ -54,6 +54,7 @@ use SimpleSAML\Module\oidc\Server\RequestRules\Rules\CodeChallengeMethodRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\CodeChallengeRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\CodeVerifierRule; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\IdTokenHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\IssuerStateRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\LoginHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\MaxAgeRule; @@ -861,6 +862,9 @@ public function validateAuthorizationRequestWithRequestRules( // LoginHintRule must run before PromptRule and MaxAgeRule, which consume its result when they // trigger re-authentication (prompt=login / expired max_age) to pre-fill the username. LoginHintRule::class, + // IdTokenHintRule must run before PromptRule, which consumes its result to enforce that a prompt=none + // request is only satisfied for the End-User identified by the id_token_hint. + IdTokenHintRule::class, PromptRule::class, MaxAgeRule::class, ScopeRule::class, diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 14312917..67efe4d6 100644 --- a/src/Server/Grants/ImplicitGrant.php +++ b/src/Server/Grants/ImplicitGrant.php @@ -25,6 +25,7 @@ use SimpleSAML\Module\oidc\Server\RequestRules\Rules\AddClaimsToIdTokenRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientRedirectUriRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientRule; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\IdTokenHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\LoginHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\MaxAgeRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\PromptRule; @@ -130,6 +131,9 @@ public function validateAuthorizationRequestWithRequestRules( // LoginHintRule must run before PromptRule and MaxAgeRule, which consume its result when they // trigger re-authentication (prompt=login / expired max_age) to pre-fill the username. LoginHintRule::class, + // IdTokenHintRule must run before PromptRule, which consumes its result to enforce that a prompt=none + // request is only satisfied for the End-User identified by the id_token_hint. + IdTokenHintRule::class, PromptRule::class, MaxAgeRule::class, RequiredOpenIdScopeRule::class, diff --git a/src/Server/RequestRules/Rules/IdTokenHintRule.php b/src/Server/RequestRules/Rules/IdTokenHintRule.php index 389ec235..d2725a50 100644 --- a/src/Server/RequestRules/Rules/IdTokenHintRule.php +++ b/src/Server/RequestRules/Rules/IdTokenHintRule.php @@ -52,6 +52,13 @@ public function checkRule( ): ?Result { $state = $currentResultBag->getOrFail(StateRule::class)->getValue(); + // When this rule runs in the authorization flow, the redirect URI has already been validated and is + // available in the result bag, so validation errors can be redirected back to the client (as required at + // the authorization endpoint). In the logout (end session) flow there is no ClientRedirectUriRule, so this + // resolves to null and the error is returned directly, preserving the previous behavior. + $redirectUriValue = $currentResultBag->get(ClientRedirectUriRule::class)?->getValue(); + $redirectUri = is_string($redirectUriValue) ? $redirectUriValue : null; + $idTokenHintParam = $this->requestParamsResolver->getAsStringBasedOnAllowedMethods( ParamsEnum::IdTokenHint->value, $request, @@ -63,13 +70,14 @@ public function checkRule( } if (empty($idTokenHintParam)) { - $loggerService->notice('End session request rejected: `id_token_hint` was provided but empty.'); + $loggerService->notice('Request rejected: `id_token_hint` was provided but empty.'); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, 'Received empty id_token_hint', null, - null, + $redirectUri, $state, + $responseMode, ); } @@ -81,15 +89,16 @@ public function checkRule( if ($idTokenHint->getIssuer() !== $this->moduleConfig->getIssuer()) { $loggerService->notice( - 'End session request rejected: `id_token_hint` was not issued by this OP.', + 'Request rejected: `id_token_hint` was not issued by this OP.', ['issuer' => $idTokenHint->getIssuer(), 'expected_issuer' => $this->moduleConfig->getIssuer()], ); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, 'Invalid ID Token Hint Issuer', null, - null, + $redirectUri, $state, + $responseMode, ); } @@ -97,15 +106,16 @@ public function checkRule( $idTokenHint->verifyWithKeySet($jwks); } catch (\Throwable $exception) { $loggerService->notice( - 'End session request rejected: `id_token_hint` signature verification failed.', + 'Request rejected: `id_token_hint` signature verification failed.', ['exception' => $exception->getMessage()], ); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, $exception->getMessage(), null, - null, + $redirectUri, $state, + $responseMode, ); } diff --git a/src/Server/RequestRules/Rules/PromptRule.php b/src/Server/RequestRules/Rules/PromptRule.php index 6c88db03..698b4507 100644 --- a/src/Server/RequestRules/Rules/PromptRule.php +++ b/src/Server/RequestRules/Rules/PromptRule.php @@ -84,18 +84,47 @@ public function checkRule( $redirectUri = $currentResultBag->getOrFail(ClientRedirectUriRule::class)->getValue(); $state = $currentResultBag->getOrFail(StateRule::class)->getValue(); - if (in_array('none', $prompt, true) && !$authSimple->isAuthenticated()) { - $loggerService->notice( - 'Authorization request rejected: `prompt=none` was requested but the user is not authenticated.', - ['client_id' => $client->getIdentifier()], - ); - throw OidcServerException::loginRequired( - null, - $redirectUri, - null, - $state, - $responseMode, - ); + if (in_array('none', $prompt, true)) { + if (!$authSimple->isAuthenticated()) { + $loggerService->notice( + 'Authorization request rejected: `prompt=none` was requested but the user is not authenticated.', + ['client_id' => $client->getIdentifier()], + ); + throw OidcServerException::loginRequired( + null, + $redirectUri, + null, + $state, + $responseMode, + ); + } + + // If an id_token_hint is present, the RP is asking to silently continue the session of a specific + // End-User. Since prompt=none forbids any interaction, we can not switch users: the currently + // authenticated subject must match the subject in the hint. Otherwise, per OIDC Core, we return + // login_required rather than silently issuing a token for a different user. + $idTokenHint = $currentResultBag->getOrFail(IdTokenHintRule::class)->getValue(); + if ($idTokenHint !== null) { + $hintSubject = $idTokenHint->getSubject(); + $currentSubject = $this->authenticationService->resolveSubjectFromAttributes( + $authSimple->getAttributes(), + ); + + if ($currentSubject === null || !hash_equals($hintSubject, $currentSubject)) { + $loggerService->notice( + 'Authorization request rejected: `prompt=none` was requested with an `id_token_hint` for a ' . + 'different End-User than the one currently authenticated.', + ['client_id' => $client->getIdentifier()], + ); + throw OidcServerException::loginRequired( + null, + $redirectUri, + null, + $state, + $responseMode, + ); + } + } } if (in_array('login', $prompt, true) && $authSimple->isAuthenticated()) { diff --git a/src/Services/AuthenticationService.php b/src/Services/AuthenticationService.php index 56c248f4..8f213681 100644 --- a/src/Services/AuthenticationService.php +++ b/src/Services/AuthenticationService.php @@ -330,6 +330,32 @@ public function authenticateForClient( $this->authenticate($this->authSimpleFactory->build($clientEntity), $loginParams); } + /** + * Resolve the OIDC subject identifier (`sub`) from a set of released user attributes, using the same logic + * that produces the `sub` claim during token issuance: the configured user identifier attributes select the + * user identifier, and any `sub` claim mapping (openid scope) takes precedence over it. + * + * This is used to compare the currently authenticated user against an `id_token_hint` subject at the + * authorization endpoint (see PromptRule), so that a `prompt=none` request is not silently satisfied for a + * different user than the one the hint identifies. + * + * @param array $attributes Released attributes (each value an array of values). + * @return string|null The resolved subject, or null when no user identifier can be resolved. + */ + public function resolveSubjectFromAttributes(array $attributes): ?string + { + $userId = $this->userIdentifierResolver->resolve($this->userIdAttrs, $attributes); + + if ($userId === null) { + return null; + } + + // We need to make sure that we use 'sub' as user identifier, if configured. + $claims = $this->claimTranslatorExtractor->extract(['openid'], $attributes); + + return (string)($claims['sub'] ?? $userId); + } + /** * Store Relying on Party Association to the current session. * @throws \Exception diff --git a/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php b/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php index 60a98b8d..7df5995b 100644 --- a/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php +++ b/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php @@ -14,10 +14,12 @@ use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\Factories\AuthSimpleFactory; use SimpleSAML\Module\oidc\Helpers; +use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException; use SimpleSAML\Module\oidc\Server\RequestRules\Result; use SimpleSAML\Module\oidc\Server\RequestRules\ResultBag; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientRedirectUriRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientRule; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\IdTokenHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\LoginHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\PromptRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\StateRule; @@ -25,6 +27,7 @@ use SimpleSAML\Module\oidc\Services\AuthenticationService; use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; +use SimpleSAML\OpenID\Core\IdToken; use SimpleSAML\Utils\HTTP as SspHttp; #[CoversClass(PromptRule::class)] @@ -116,4 +119,61 @@ public function testPromptLoginReAuthenticatesAndPropagatesLoginHint(): void $this->assertNull($this->checkRule()); } + + public function testPromptNoneThrowsLoginRequiredWhenNotAuthenticated(): void + { + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') + ->willReturn(['prompt' => 'none']); + $this->authSimpleMock->method('isAuthenticated')->willReturn(false); + + $this->expectException(OidcServerException::class); + $this->expectExceptionMessage('End-User is not already authenticated.'); + $this->checkRule(); + } + + public function testPromptNoneWithoutIdTokenHintProceeds(): void + { + $this->resultBag->add(new Result(IdTokenHintRule::class, null)); + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') + ->willReturn(['prompt' => 'none']); + $this->authSimpleMock->method('isAuthenticated')->willReturn(true); + + $this->authenticationServiceMock->expects($this->never())->method('resolveSubjectFromAttributes'); + + $this->assertNull($this->checkRule()); + } + + public function testPromptNoneWithMatchingIdTokenHintSubjectProceeds(): void + { + $idTokenHintMock = $this->createMock(IdToken::class); + $idTokenHintMock->method('getSubject')->willReturn('subject-123'); + $this->resultBag->add(new Result(IdTokenHintRule::class, $idTokenHintMock)); + + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') + ->willReturn(['prompt' => 'none']); + $this->authSimpleMock->method('isAuthenticated')->willReturn(true); + $this->authSimpleMock->method('getAttributes')->willReturn(['uid' => ['subject-123']]); + $this->authenticationServiceMock->method('resolveSubjectFromAttributes') + ->willReturn('subject-123'); + + $this->assertNull($this->checkRule()); + } + + public function testPromptNoneWithMismatchedIdTokenHintSubjectThrowsLoginRequired(): void + { + $idTokenHintMock = $this->createMock(IdToken::class); + $idTokenHintMock->method('getSubject')->willReturn('subject-123'); + $this->resultBag->add(new Result(IdTokenHintRule::class, $idTokenHintMock)); + + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') + ->willReturn(['prompt' => 'none']); + $this->authSimpleMock->method('isAuthenticated')->willReturn(true); + $this->authSimpleMock->method('getAttributes')->willReturn(['uid' => ['other-user']]); + $this->authenticationServiceMock->method('resolveSubjectFromAttributes') + ->willReturn('other-subject'); + + $this->expectException(OidcServerException::class); + $this->expectExceptionMessage('End-User is not already authenticated.'); + $this->checkRule(); + } } diff --git a/tests/unit/src/Services/AuthenticationServiceTest.php b/tests/unit/src/Services/AuthenticationServiceTest.php index c8bceac2..749128b9 100644 --- a/tests/unit/src/Services/AuthenticationServiceTest.php +++ b/tests/unit/src/Services/AuthenticationServiceTest.php @@ -481,6 +481,48 @@ public function testItPropagatesLoginHintToAuthentication(): void ); } + /** + * When no `sub` claim mapping applies, the resolved subject is the user identifier selected from the + * configured user identifier attributes. + */ + public function testResolveSubjectFromAttributesFallsBackToUserIdentifier(): void + { + $this->claimTranslatorExtractorMock->method('extract') + ->with(['openid'], self::USER_ENTITY_ATTRIBUTES) + ->willReturn([]); + + $this->assertSame( + self::USERNAME, + $this->mock()->resolveSubjectFromAttributes(self::USER_ENTITY_ATTRIBUTES), + ); + } + + /** + * When a `sub` claim is produced (e.g. via a configured `sub` mapping), it takes precedence over the raw + * user identifier. + */ + public function testResolveSubjectFromAttributesUsesSubClaimWhenPresent(): void + { + $this->claimTranslatorExtractorMock->method('extract') + ->with(['openid'], self::USER_ENTITY_ATTRIBUTES) + ->willReturn(['sub' => 'mapped-subject']); + + $this->assertSame( + 'mapped-subject', + $this->mock()->resolveSubjectFromAttributes(self::USER_ENTITY_ATTRIBUTES), + ); + } + + /** + * When none of the configured user identifier attributes are present, no subject can be resolved. + */ + public function testResolveSubjectFromAttributesReturnsNullWhenNoIdentifier(): void + { + $this->assertNull( + $this->mock()->resolveSubjectFromAttributes(['someOtherAttribute' => ['value']]), + ); + } + /** * @throws NoState */ From 6783f335dda0c074622c1d75adb38ce44e40df08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Sat, 25 Jul 2026 12:42:04 +0200 Subject: [PATCH 2/2] Switch to deficated ID Token Hint abstraction --- composer.json | 2 +- docs/6-oidc-upgrade.md | 30 ++++- src/Controllers/AuthorizationController.php | 71 ++++++++++++ src/Server/Grants/AuthCodeGrant.php | 9 ++ src/Server/Grants/ImplicitGrant.php | 5 + .../RequestRules/Rules/IdTokenHintRule.php | 45 +++++++- src/Server/RequestRules/Rules/PromptRule.php | 45 ++++---- .../RequestTypes/AuthorizationRequest.php | 19 ++++ src/Services/AuthenticationService.php | 29 +++-- src/Services/IdTokenBuilder.php | 18 ++- .../AuthorizationControllerTest.php | 62 +++++++++++ .../Rules/IdTokenHintRuleTest.php | 59 ++++++++-- .../RequestRules/Rules/PromptRuleTest.php | 30 +++-- .../Services/AuthenticationServiceTest.php | 45 +++++--- .../unit/src/Services/IdTokenBuilderTest.php | 103 +++++++++++++++++- 15 files changed, 486 insertions(+), 86 deletions(-) diff --git a/composer.json b/composer.json index e4500362..3ba1d343 100644 --- a/composer.json +++ b/composer.json @@ -32,7 +32,7 @@ "psr/log": "^3", "psr/simple-cache": "^3", "simplesamlphp/composer-module-installer": "^1.3", - "simplesamlphp/openid": "~0.3.8", + "simplesamlphp/openid": "~0.3.12", "spomky-labs/base64url": "^2.0", "symfony/cache": "^7.4", "symfony/expression-language": "^7.4", diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md index 5c6304e3..6de5ee30 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -217,13 +217,33 @@ re-authentication triggered by `prompt=login` or an expired `max_age`. - Support for the `id_token_hint` parameter on the authorization endpoint (previously ignored; it was already supported on the end session endpoint). The parameter carries an ID Token previously issued by this OP as a hint about the -End-User's session. When present, it is validated (issuer and signature). Its -main effect is with `prompt=none`: the request is only satisfied when the -currently authenticated End-User matches the subject (`sub`) in the -`id_token_hint`; otherwise a `login_required` error is returned rather than -silently issuing a token for a different user. A malformed, wrongly-issued or +End-User's session with the requesting client. When present, it is validated +(issuer, signature, and — since the hint represents a session with the +requesting client — that the client is an audience of the hint) and, after +authentication, the subject (`sub`) that would be issued for the authenticated +End-User is compared to the subject in the `id_token_hint`. If +they differ, a `login_required` error is returned rather than issuing a +token/code for a different End-User than the one the hint identifies. This +applies to all prompt modes: with `prompt=none` it prevents a silent response +for a mismatched cookie session, and with interactive authentication it rejects +the request when a different End-User authenticated than requested (the client +can then retry, e.g. with `prompt=login`). An otherwise-valid `id_token_hint` +whose ID Token has expired is accepted (as recommended by the specification, +since a hint is commonly sent after it has expired); its signature, issuer and +`nbf`/`iat` timestamps are still validated. A malformed, wrongly-issued or improperly-signed `id_token_hint` results in an `invalid_request` error redirected back to the client. +- The ID Token subject (`sub`) is now resolved consistently for a given +End-User, independently of the flow, the granted scopes and the client's +`add_claims_to_id_token` setting. Previously, when a `sub` claim mapping was +configured, the mapped value was only applied if the user's claims were +released in the ID Token, so the same End-User could receive different `sub` +values depending on the flow and client. Deployments that did not customize the +`sub` claim mapping are unaffected (the user identifier attributes are used for +`sub` by default, which already produced the same value in both cases). If you +did map `sub` to a different attribute, clients relying on the previously +inconsistent value may see a changed `sub` in flows where the claims were not +released. - Logging has been improved for authentication flows. It should now be easier to find information about what went wrong by looking at the relevant log entries. diff --git a/src/Controllers/AuthorizationController.php b/src/Controllers/AuthorizationController.php index 0b2a77ac..01d7048c 100644 --- a/src/Controllers/AuthorizationController.php +++ b/src/Controllers/AuthorizationController.php @@ -77,6 +77,13 @@ public function __invoke(ServerRequestInterface $request): ResponseInterface $state ??= $this->authenticationService->manageState($queryParameters); $authorizationRequest = $this->authenticationService->getAuthorizationRequestFromState($state); + // Validate any id_token_hint against the authenticated End-User before the user is resolved and (as a side + // effect) associated with the client, so that a mismatched request leaves no relying party association + // behind (which could otherwise later receive a back-channel logout for that End-User). + if ($authorizationRequest instanceof AuthorizationRequest) { + $this->validateIdTokenHint($authorizationRequest, $state); + } + $user = $this->authenticationService->getAuthenticateUser($state); $authorizationRequest->setUser($user); @@ -180,6 +187,70 @@ protected function validatePostAuthnAuthorizationRequest(AuthorizationRequest $a $this->validateAcr($authorizationRequest); } + /** + * Validate the `id_token_hint` authorization request parameter (if any) against the authenticated End-User. + * + * The hint is an ID Token previously issued by this OP; its issuer and signature were already validated early + * (IdTokenHintRule) and its subject carried on the authorization request. Here, using the released post-authproc + * attributes (the same ones from which the issued subject is derived), we verify that the authenticated End-User + * matches the subject in the hint. Per OpenID Connect Core, the request must not be satisfied for a different + * End-User than the one the hint identifies; if they differ we return `login_required` (the specification's + * suggested error) rather than issuing a token/code for the wrong user. This applies to all prompt modes: with + * `prompt=none` it prevents a silent response for a mismatched cookie session, and with interactive + * authentication it rejects the request when a different End-User authenticated than the hint asked for (the + * client can then retry, e.g. with `prompt=login`). + * + * This runs before the End-User is resolved and associated with the client, so a mismatch does not leave a + * relying party association behind. + * + * @param array|null $state + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + */ + protected function validateIdTokenHint(AuthorizationRequest $authorizationRequest, ?array $state): void + { + $hintSubject = $authorizationRequest->getIdTokenHintSubject(); + if ($hintSubject === null) { + return; + } + + // No released attributes means no End-User to match the hint against, so the request can not be satisfied + // for the End-User the hint identifies (subjectMatchesAttributes() returns false for an empty set). + $attributes = (isset($state['Attributes']) && is_array($state['Attributes'])) ? $state['Attributes'] : []; + + if ($this->authenticationService->subjectMatchesAttributes($hintSubject, $attributes)) { + return; + } + + $this->loggerService->notice( + 'Authorization request rejected: the authenticated End-User does not match the `id_token_hint` subject.', + ['client_id' => $authorizationRequest->getClient()->getIdentifier()], + ); + + throw OidcServerException::loginRequired( + 'Authenticated End-User does not match the id_token_hint subject.', + $this->resolveRedirectUri($authorizationRequest), + null, + $authorizationRequest->getState(), + $authorizationRequest->getResponseMode(), + ); + } + + /** + * Resolve the redirect URI to use for redirected error responses: the one validated for this request, or the + * client's first registered redirect URI as a fallback. + */ + protected function resolveRedirectUri(AuthorizationRequest $authorizationRequest): ?string + { + $redirectUri = $authorizationRequest->getRedirectUri(); + if ($redirectUri !== null) { + return $redirectUri; + } + + $clientRedirectUri = $authorizationRequest->getClient()->getRedirectUri(); + + return is_array($clientRedirectUri) ? ($clientRedirectUri[0] ?? null) : $clientRedirectUri; + } + /** * @throws \Exception */ diff --git a/src/Server/Grants/AuthCodeGrant.php b/src/Server/Grants/AuthCodeGrant.php index 616c1b42..e731975d 100644 --- a/src/Server/Grants/AuthCodeGrant.php +++ b/src/Server/Grants/AuthCodeGrant.php @@ -1004,6 +1004,15 @@ public function validateAuthorizationRequestWithRequestRules( $this->loggerService->debug('AuthCodeGrant: Login hint present: ', ['loginHintPresent' => $loginHintPresent]); $authorizationRequest->setLoginHint($loginHint); + // Carry the id_token_hint subject (if the hint was provided and validated by IdTokenHintRule) so that, + // after authentication, the authenticated End-User can be verified against the one the hint identifies. + $idTokenHint = $resultBag->getOrFail(IdTokenHintRule::class)->getValue(); + $this->loggerService->debug( + 'AuthCodeGrant: ID Token hint present: ', + ['idTokenHintPresent' => $idTokenHint !== null], + ); + $authorizationRequest->setIdTokenHintSubject($idTokenHint?->getSubject()); + $authorizationRequest->setIsVciRequest($isVciAuthorizationCodeRequest); $flowType = $isVciAuthorizationCodeRequest ? diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 67efe4d6..2d52c45a 100644 --- a/src/Server/Grants/ImplicitGrant.php +++ b/src/Server/Grants/ImplicitGrant.php @@ -205,6 +205,11 @@ public function validateAuthorizationRequestWithRequestRules( $loginHint = $resultBag->getOrFail(LoginHintRule::class)->getValue(); $authorizationRequest->setLoginHint($loginHint); + // Carry the id_token_hint subject (if the hint was provided and validated by IdTokenHintRule) so that, + // after authentication, the authenticated End-User can be verified against the one the hint identifies. + $idTokenHint = $resultBag->getOrFail(IdTokenHintRule::class)->getValue(); + $authorizationRequest->setIdTokenHintSubject($idTokenHint?->getSubject()); + $responseMode = $resultBag->getOrFail(ResponseModeRule::class)->getValue(); $authorizationRequest->setResponseMode($responseMode); diff --git a/src/Server/RequestRules/Rules/IdTokenHintRule.php b/src/Server/RequestRules/Rules/IdTokenHintRule.php index d2725a50..f2181977 100644 --- a/src/Server/RequestRules/Rules/IdTokenHintRule.php +++ b/src/Server/RequestRules/Rules/IdTokenHintRule.php @@ -20,7 +20,7 @@ use SimpleSAML\OpenID\Jwks; /** - * @extends AbstractRule<\SimpleSAML\OpenID\Core\IdToken|null> + * @extends AbstractRule<\SimpleSAML\OpenID\Core\IdTokenHint|null> */ class IdTokenHintRule extends AbstractRule { @@ -85,7 +85,28 @@ public function checkRule( ...$this->moduleConfig->getProtocolSignatureKeyPairBag()->getAllPublicKeys(), )->jsonSerialize(); - $idTokenHint = $this->core->idTokenFactory()->fromToken($idTokenHintParam); + // Parsing constructs and validates the ID Token Hint (structure and required claims), throwing on any + // problem. We translate those failures into a protocol-level invalid_request error (which is redirected back + // to the client in the authorization flow) instead of letting a raw exception surface as an HTTP 500. The + // dedicated IdTokenHint abstraction deliberately does not validate the `exp` claim, so an otherwise-valid but + // expired hint is accepted (as recommended by OpenID Connect Core, since a hint is commonly sent after it has + // expired); the `nbf` and `iat` timestamps are still validated. + try { + $idTokenHint = $this->core->idTokenHintFactory()->fromToken($idTokenHintParam); + } catch (\Throwable $exception) { + $loggerService->notice( + 'Request rejected: `id_token_hint` could not be parsed or validated.', + ['exception' => $exception->getMessage()], + ); + throw OidcServerException::invalidRequest( + ParamsEnum::IdTokenHint->value, + $exception->getMessage(), + null, + $redirectUri, + $state, + $responseMode, + ); + } if ($idTokenHint->getIssuer() !== $this->moduleConfig->getIssuer()) { $loggerService->notice( @@ -119,6 +140,26 @@ public function checkRule( ); } + // In the authorization flow the requesting client is known (ClientRule). An id_token_hint represents the + // End-User's session with the requesting client, so require that client to be an audience of the hint. This + // binds the hint to the requesting client and rejects a token that was issued to a different client. In the + // logout (end session) flow there is no ClientRule in the result bag, so this is skipped, preserving that + // flow's behavior. + $client = $currentResultBag->get(ClientRule::class)?->getValue(); + if ($client !== null && !in_array($client->getIdentifier(), $idTokenHint->getAudience(), true)) { + $loggerService->notice( + 'Request rejected: `id_token_hint` was not issued to the requesting client.', + ['client_id' => $client->getIdentifier()], + ); + throw OidcServerException::invalidRequest( + ParamsEnum::IdTokenHint->value, + 'ID Token Hint audience does not include the requesting client', + null, + $redirectUri, + $state, + $responseMode, + ); + } return new Result($this->getKey(), $idTokenHint); } diff --git a/src/Server/RequestRules/Rules/PromptRule.php b/src/Server/RequestRules/Rules/PromptRule.php index 698b4507..5b2ed80a 100644 --- a/src/Server/RequestRules/Rules/PromptRule.php +++ b/src/Server/RequestRules/Rules/PromptRule.php @@ -99,31 +99,32 @@ public function checkRule( ); } - // If an id_token_hint is present, the RP is asking to silently continue the session of a specific - // End-User. Since prompt=none forbids any interaction, we can not switch users: the currently - // authenticated subject must match the subject in the hint. Otherwise, per OIDC Core, we return - // login_required rather than silently issuing a token for a different user. + // `prompt=none` forbids any interaction. If an id_token_hint is present, verify here (before the normal, + // potentially interactive, authentication processing runs) that the existing session belongs to the + // End-User the hint identifies, so a mismatch is rejected passively rather than after authproc filters + // may have shown UI. The authoritative post-authentication check still runs for all prompt modes in the + // controller; for `prompt=none` the End-User is the existing (cookie) session, so its released + // attributes are used here. $idTokenHint = $currentResultBag->getOrFail(IdTokenHintRule::class)->getValue(); - if ($idTokenHint !== null) { - $hintSubject = $idTokenHint->getSubject(); - $currentSubject = $this->authenticationService->resolveSubjectFromAttributes( + if ( + $idTokenHint !== null && + !$this->authenticationService->subjectMatchesAttributes( + $idTokenHint->getSubject(), $authSimple->getAttributes(), + ) + ) { + $loggerService->notice( + 'Authorization request rejected: `prompt=none` was requested with an `id_token_hint` for a ' . + 'different End-User than the one currently authenticated.', + ['client_id' => $client->getIdentifier()], + ); + throw OidcServerException::loginRequired( + null, + $redirectUri, + null, + $state, + $responseMode, ); - - if ($currentSubject === null || !hash_equals($hintSubject, $currentSubject)) { - $loggerService->notice( - 'Authorization request rejected: `prompt=none` was requested with an `id_token_hint` for a ' . - 'different End-User than the one currently authenticated.', - ['client_id' => $client->getIdentifier()], - ); - throw OidcServerException::loginRequired( - null, - $redirectUri, - null, - $state, - $responseMode, - ); - } } } diff --git a/src/Server/RequestTypes/AuthorizationRequest.php b/src/Server/RequestTypes/AuthorizationRequest.php index dc3747d6..034c99af 100644 --- a/src/Server/RequestTypes/AuthorizationRequest.php +++ b/src/Server/RequestTypes/AuthorizationRequest.php @@ -47,6 +47,15 @@ class AuthorizationRequest extends OAuth2AuthorizationRequest */ protected ?string $loginHint = null; + /** + * Subject (`sub`) claim of the ID Token supplied as the `id_token_hint` parameter, if any. Only the subject is + * carried here (not the whole token): the token itself is validated early (issuer, signature) by IdTokenHintRule, + * and this value must survive serialization into the SimpleSAMLphp authentication state across the login + * redirect. It is used after authentication to verify the authenticated End-User matches the one the hint + * identifies (see AuthorizationController). + */ + protected ?string $idTokenHintSubject = null; + /** * ACR used during authn. */ @@ -256,6 +265,16 @@ public function setLoginHint(?string $loginHint): void $this->loginHint = $loginHint; } + public function getIdTokenHintSubject(): ?string + { + return $this->idTokenHintSubject; + } + + public function setIdTokenHintSubject(?string $idTokenHintSubject): void + { + $this->idTokenHintSubject = $idTokenHintSubject; + } + public function getAcr(): ?string { return $this->acr; diff --git a/src/Services/AuthenticationService.php b/src/Services/AuthenticationService.php index 8f213681..1c922c0e 100644 --- a/src/Services/AuthenticationService.php +++ b/src/Services/AuthenticationService.php @@ -331,29 +331,34 @@ public function authenticateForClient( } /** - * Resolve the OIDC subject identifier (`sub`) from a set of released user attributes, using the same logic - * that produces the `sub` claim during token issuance: the configured user identifier attributes select the - * user identifier, and any `sub` claim mapping (openid scope) takes precedence over it. + * Determine whether the given subject identifier corresponds to the End-User described by the released + * (post-authproc) attributes. This is used to verify an `id_token_hint` subject against the authenticated + * End-User at the authorization endpoint (see AuthorizationController). * - * This is used to compare the currently authenticated user against an `id_token_hint` subject at the - * authorization endpoint (see PromptRule), so that a `prompt=none` request is not silently satisfied for a - * different user than the one the hint identifies. + * A single canonical subject is derived using the same default logic that produces the `sub` claim during token + * issuance (see IdTokenBuilder and addRelyingPartyAssociation()): the resolved user identifier, unless a `sub` + * claim mapping (openid scope) produces a value, which then takes precedence. Deriving one value (rather than + * accepting several candidate forms) is important for security: distinct subject namespaces could otherwise + * collide across users (one user's mapped `sub` equalling another user's raw identifier), which would let an + * `id_token_hint` match the wrong End-User. * - * @param array $attributes Released attributes (each value an array of values). - * @return string|null The resolved subject, or null when no user identifier can be resolved. + * This mirrors the canonical subject resolution in IdTokenBuilder, which issues the same value for a given user + * regardless of the flow, client claim-release settings or granted scopes, so the comparison is exact. + * + * @param array $attributes Released (post-authproc) attributes. */ - public function resolveSubjectFromAttributes(array $attributes): ?string + public function subjectMatchesAttributes(string $subject, array $attributes): bool { $userId = $this->userIdentifierResolver->resolve($this->userIdAttrs, $attributes); - if ($userId === null) { - return null; + return false; } // We need to make sure that we use 'sub' as user identifier, if configured. $claims = $this->claimTranslatorExtractor->extract(['openid'], $attributes); + $canonicalSubject = (isset($claims['sub']) && is_scalar($claims['sub'])) ? (string)$claims['sub'] : $userId; - return (string)($claims['sub'] ?? $userId); + return hash_equals($canonicalSubject, $subject); } /** diff --git a/src/Services/IdTokenBuilder.php b/src/Services/IdTokenBuilder.php index 2eb7ec8b..4153aa6f 100644 --- a/src/Services/IdTokenBuilder.php +++ b/src/Services/IdTokenBuilder.php @@ -64,6 +64,17 @@ public function buildFor( $currentTimestamp = $this->core->helpers()->dateTime()->getUtc()->getTimestamp(); + // Resolve the canonical subject identifier for this user. The subject is the user identifier, unless a `sub` + // claim mapping (openid scope) produces a value, which then takes precedence. This is resolved up front, and + // independently of the claim-release settings below, so that the issued `sub` is stable for a given user + // across flows and clients (it must not vary with `add_claims_to_id_token` or the granted scopes): the `sub` + // is the End-User's identity towards the client and is relied upon elsewhere, e.g. when matching an + // `id_token_hint` (see AuthenticationService::subjectMatchesAttributes()) and in logout token association. + $openIdClaims = $this->claimExtractor->extract(['openid'], $userEntity->getClaims()); + $subject = (isset($openIdClaims['sub']) && is_scalar($openIdClaims['sub'])) ? + (string)$openIdClaims['sub'] : + $userEntity->getIdentifier(); + $payload = array_filter([ ClaimsEnum::Iss->value => $this->moduleConfig->getIssuer(), ClaimsEnum::Iat->value => $currentTimestamp, @@ -71,9 +82,6 @@ public function buildFor( ClaimsEnum::Aud->value => $client->getIdentifier(), ClaimsEnum::Nbf->value => $currentTimestamp, ClaimsEnum::Exp->value => $accessToken->getExpiryDateTime()->getTimestamp(), - ClaimsEnum::Sub->value => $this->core->helpers()->type()->ensureNonEmptyString( - $userEntity->getIdentifier(), - ), ClaimsEnum::Nonce->value => $nonce, ClaimsEnum::AuthTime->value => $authTime, ClaimsEnum::ATHash->value => $addAccessTokenHash ? @@ -86,6 +94,10 @@ public function buildFor( ClaimsEnum::Sid->value => $sessionId, ]); + // The subject is set after the optional claims above have been filtered, since it is REQUIRED and must be + // kept even for a value that array_filter() would consider falsy (for example the valid subject "0"). + $payload[ClaimsEnum::Sub->value] = $this->core->helpers()->type()->ensureNonEmptyString($subject); + // Reduce the number of claims by provided scope. $claims = $this->claimExtractor->extract( $accessToken->getScopes(), diff --git a/tests/unit/src/Controllers/AuthorizationControllerTest.php b/tests/unit/src/Controllers/AuthorizationControllerTest.php index 3f352564..e4fc868d 100644 --- a/tests/unit/src/Controllers/AuthorizationControllerTest.php +++ b/tests/unit/src/Controllers/AuthorizationControllerTest.php @@ -594,4 +594,66 @@ public function testDoesNotOverrideExistingLanguageCookieWithUiLocales(): void ($this->mock())($this->serverRequestStub); } + + /** + * When an id_token_hint is present and the authenticated End-User's subject matches it, the request proceeds. + * + * @throws \Throwable + */ + public function testValidateIdTokenHintPassesOnSubjectMatch(): void + { + $this->authorizationRequestMock->method('getRequestedAcrValues')->willReturn(null); + $this->authorizationRequestMock->method('getIdTokenHintSubject')->willReturn('subject-a'); + + $this->authorizationServerStub + ->method('validateAuthorizationRequest') + ->willReturn($this->authorizationRequestMock); + $this->authorizationServerStub + ->method('completeAuthorizationRequest') + ->willReturn($this->responseStub); + + $this->serverRequestStub->method('getQueryParams')->willReturn([ProcessingChain::AUTHPARAM => '123']); + + $this->authenticationServiceStub->method('manageState')->willReturn($this->state); + $this->authenticationServiceStub->method('getAuthenticateUser')->willReturn($this->userEntityStub); + $this->authenticationServiceStub + ->method('getAuthorizationRequestFromState') + ->willReturn($this->authorizationRequestMock); + $this->authenticationServiceStub->method('subjectMatchesAttributes')->willReturn(true); + + $this->assertInstanceOf(ResponseInterface::class, ($this->mock())($this->serverRequestStub)); + } + + /** + * When an id_token_hint is present but the authenticated End-User's subject differs from it, the request is + * rejected with login_required rather than issued for a different user. + * + * @throws \Throwable + */ + public function testValidateIdTokenHintThrowsLoginRequiredOnSubjectMismatch(): void + { + $clientStub = $this->createStub(\SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface::class); + $clientStub->method('getIdentifier')->willReturn('clientid'); + + $this->authorizationRequestMock->method('getIdTokenHintSubject')->willReturn('subject-a'); + $this->authorizationRequestMock->method('getClient')->willReturn($clientStub); + $this->authorizationRequestMock->method('getRedirectUri')->willReturn('https://rp.example.org/cb'); + + $this->authorizationServerStub + ->method('validateAuthorizationRequest') + ->willReturn($this->authorizationRequestMock); + + $this->serverRequestStub->method('getQueryParams')->willReturn([ProcessingChain::AUTHPARAM => '123']); + + $this->authenticationServiceStub->method('manageState')->willReturn($this->state); + $this->authenticationServiceStub + ->method('getAuthorizationRequestFromState') + ->willReturn($this->authorizationRequestMock); + $this->authenticationServiceStub->method('subjectMatchesAttributes')->willReturn(false); + + $this->expectException(OidcServerException::class); + $this->expectExceptionMessage('End-User is not already authenticated.'); + + ($this->mock())($this->serverRequestStub); + } } diff --git a/tests/unit/src/Server/RequestRules/Rules/IdTokenHintRuleTest.php b/tests/unit/src/Server/RequestRules/Rules/IdTokenHintRuleTest.php index 65de0270..0bf2d9ed 100644 --- a/tests/unit/src/Server/RequestRules/Rules/IdTokenHintRuleTest.php +++ b/tests/unit/src/Server/RequestRules/Rules/IdTokenHintRuleTest.php @@ -9,17 +9,20 @@ use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ServerRequestInterface; +use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\Helpers; use SimpleSAML\Module\oidc\ModuleConfig; +use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException; use SimpleSAML\Module\oidc\Server\RequestRules\Interfaces\ResultBagInterface; use SimpleSAML\Module\oidc\Server\RequestRules\Result; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\IdTokenHintRule; use SimpleSAML\Module\oidc\Server\ResponseModes\ResponseModeInterface; use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; use SimpleSAML\OpenID\Core; -use SimpleSAML\OpenID\Core\Factories\IdTokenFactory; -use SimpleSAML\OpenID\Core\IdToken; +use SimpleSAML\OpenID\Core\Factories\IdTokenHintFactory; +use SimpleSAML\OpenID\Core\IdTokenHint; use SimpleSAML\OpenID\Jwks; use Throwable; @@ -69,9 +72,9 @@ protected function setUp(): void $this->jwksMock = $this->createMock(Jwks::class); $this->coreMock = $this->createMock(Core::class); - $this->idTokenFactoryMock = $this->createMock(IdTokenFactory::class); - $this->idTokenMock = $this->createMock(IdToken::class); - $this->coreMock->method('idTokenFactory')->willReturn($this->idTokenFactoryMock); + $this->idTokenFactoryMock = $this->createMock(IdTokenHintFactory::class); + $this->idTokenMock = $this->createMock(IdTokenHint::class); + $this->coreMock->method('idTokenHintFactory')->willReturn($this->idTokenFactoryMock); $this->responseModeStub = $this->createStub(ResponseModeInterface::class); } @@ -121,12 +124,19 @@ public function testCheckRuleIsNullWhenParamNotSet(): void } /** + * A hint that can not be parsed/validated (malformed JWS, missing/invalid required claims, or an expired + * token) must be translated into a protocol-level invalid_request error, not surface as a raw exception. + * * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException */ - public function testCheckRuleThrowsForMalformedIdToken(): void + public function testCheckRuleThrowsInvalidRequestForUnparsableIdToken(): void { $this->requestParamsResolverStub->method('getAsStringBasedOnAllowedMethods')->willReturn('malformed'); - $this->expectException(Throwable::class); + $this->idTokenFactoryMock->method('fromToken') + ->with('malformed') + ->willThrowException(new \Exception('parse-failure')); + + $this->expectException(OidcServerException::class); $this->sut()->checkRule( $this->requestStub, $this->resultBagStub, @@ -204,6 +214,39 @@ public function testCheckRulePassesForValidIdToken(): void ) ?? new Result(IdTokenHintRule::class); - $this->assertInstanceOf(IdToken::class, $result->getValue()); + $this->assertInstanceOf(IdTokenHint::class, $result->getValue()); + } + + /** + * In the authorization flow (ClientRule present), a hint whose audience does not include the requesting client + * is rejected, binding the hint to the requesting client. + * + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + */ + public function testCheckRuleThrowsWhenClientNotInAudience(): void + { + $clientStub = $this->createStub(ClientEntityInterface::class); + $clientStub->method('getIdentifier')->willReturn('client-b'); + + $resultBagStub = $this->createStub(ResultBagInterface::class); + $resultBagStub->method('get')->willReturnCallback( + fn(string $key): ?Result => $key === ClientRule::class ? + new Result(ClientRule::class, $clientStub) : + null, + ); + + $this->requestParamsResolverStub->method('getAsStringBasedOnAllowedMethods')->willReturn('id-token'); + $this->idTokenMock->method('getIssuer')->willReturn(self::$issuer); + $this->idTokenMock->method('getAudience')->willReturn(['client-a']); + $this->idTokenFactoryMock->method('fromToken')->willReturn($this->idTokenMock); + + $this->expectException(OidcServerException::class); + $this->sut()->checkRule( + $this->requestStub, + $resultBagStub, + $this->loggerServiceStub, + [], + $this->responseModeStub, + ); } } diff --git a/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php b/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php index 7df5995b..8955dd3e 100644 --- a/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php +++ b/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php @@ -27,7 +27,7 @@ use SimpleSAML\Module\oidc\Services\AuthenticationService; use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; -use SimpleSAML\OpenID\Core\IdToken; +use SimpleSAML\OpenID\Core\IdTokenHint; use SimpleSAML\Utils\HTTP as SspHttp; #[CoversClass(PromptRule::class)] @@ -131,37 +131,36 @@ public function testPromptNoneThrowsLoginRequiredWhenNotAuthenticated(): void $this->checkRule(); } - public function testPromptNoneWithoutIdTokenHintProceeds(): void + public function testPromptNoneWithMatchingIdTokenHintProceeds(): void { - $this->resultBag->add(new Result(IdTokenHintRule::class, null)); + $idTokenHintMock = $this->createMock(IdTokenHint::class); + $idTokenHintMock->method('getSubject')->willReturn('subject-123'); + $this->resultBag->add(new Result(IdTokenHintRule::class, $idTokenHintMock)); + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') ->willReturn(['prompt' => 'none']); $this->authSimpleMock->method('isAuthenticated')->willReturn(true); - - $this->authenticationServiceMock->expects($this->never())->method('resolveSubjectFromAttributes'); + $this->authSimpleMock->method('getAttributes')->willReturn(['uid' => ['subject-123']]); + $this->authenticationServiceMock->method('subjectMatchesAttributes')->willReturn(true); $this->assertNull($this->checkRule()); } - public function testPromptNoneWithMatchingIdTokenHintSubjectProceeds(): void + public function testPromptNoneWithoutIdTokenHintProceeds(): void { - $idTokenHintMock = $this->createMock(IdToken::class); - $idTokenHintMock->method('getSubject')->willReturn('subject-123'); - $this->resultBag->add(new Result(IdTokenHintRule::class, $idTokenHintMock)); + $this->resultBag->add(new Result(IdTokenHintRule::class, null)); $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') ->willReturn(['prompt' => 'none']); $this->authSimpleMock->method('isAuthenticated')->willReturn(true); - $this->authSimpleMock->method('getAttributes')->willReturn(['uid' => ['subject-123']]); - $this->authenticationServiceMock->method('resolveSubjectFromAttributes') - ->willReturn('subject-123'); + $this->authenticationServiceMock->expects($this->never())->method('subjectMatchesAttributes'); $this->assertNull($this->checkRule()); } - public function testPromptNoneWithMismatchedIdTokenHintSubjectThrowsLoginRequired(): void + public function testPromptNoneWithMismatchedIdTokenHintThrowsLoginRequired(): void { - $idTokenHintMock = $this->createMock(IdToken::class); + $idTokenHintMock = $this->createMock(IdTokenHint::class); $idTokenHintMock->method('getSubject')->willReturn('subject-123'); $this->resultBag->add(new Result(IdTokenHintRule::class, $idTokenHintMock)); @@ -169,8 +168,7 @@ public function testPromptNoneWithMismatchedIdTokenHintSubjectThrowsLoginRequire ->willReturn(['prompt' => 'none']); $this->authSimpleMock->method('isAuthenticated')->willReturn(true); $this->authSimpleMock->method('getAttributes')->willReturn(['uid' => ['other-user']]); - $this->authenticationServiceMock->method('resolveSubjectFromAttributes') - ->willReturn('other-subject'); + $this->authenticationServiceMock->method('subjectMatchesAttributes')->willReturn(false); $this->expectException(OidcServerException::class); $this->expectExceptionMessage('End-User is not already authenticated.'); diff --git a/tests/unit/src/Services/AuthenticationServiceTest.php b/tests/unit/src/Services/AuthenticationServiceTest.php index 749128b9..a05a7dbe 100644 --- a/tests/unit/src/Services/AuthenticationServiceTest.php +++ b/tests/unit/src/Services/AuthenticationServiceTest.php @@ -482,44 +482,57 @@ public function testItPropagatesLoginHintToAuthentication(): void } /** - * When no `sub` claim mapping applies, the resolved subject is the user identifier selected from the - * configured user identifier attributes. + * The subject matches when it equals the resolved user identifier (the default `sub` produced by + * IdTokenBuilder when no `sub` mapping is in effect). */ - public function testResolveSubjectFromAttributesFallsBackToUserIdentifier(): void + public function testSubjectMatchesAttributesMatchesUserIdentifier(): void { $this->claimTranslatorExtractorMock->method('extract') ->with(['openid'], self::USER_ENTITY_ATTRIBUTES) ->willReturn([]); - $this->assertSame( - self::USERNAME, - $this->mock()->resolveSubjectFromAttributes(self::USER_ENTITY_ATTRIBUTES), + $this->assertTrue( + $this->mock()->subjectMatchesAttributes(self::USERNAME, self::USER_ENTITY_ATTRIBUTES), ); } /** - * When a `sub` claim is produced (e.g. via a configured `sub` mapping), it takes precedence over the raw - * user identifier. + * The subject also matches a mapped `sub` claim, so a hint issued when the `sub` mapping was applied is + * accepted for the same user. */ - public function testResolveSubjectFromAttributesUsesSubClaimWhenPresent(): void + public function testSubjectMatchesAttributesMatchesMappedSubClaim(): void { $this->claimTranslatorExtractorMock->method('extract') ->with(['openid'], self::USER_ENTITY_ATTRIBUTES) ->willReturn(['sub' => 'mapped-subject']); - $this->assertSame( - 'mapped-subject', - $this->mock()->resolveSubjectFromAttributes(self::USER_ENTITY_ATTRIBUTES), + $this->assertTrue( + $this->mock()->subjectMatchesAttributes('mapped-subject', self::USER_ENTITY_ATTRIBUTES), + ); + } + + /** + * A subject that matches neither the user identifier nor a mapped `sub` claim identifies a different + * End-User and must not match. + */ + public function testSubjectMatchesAttributesRejectsDifferentSubject(): void + { + $this->claimTranslatorExtractorMock->method('extract') + ->with(['openid'], self::USER_ENTITY_ATTRIBUTES) + ->willReturn([]); + + $this->assertFalse( + $this->mock()->subjectMatchesAttributes('someone-else', self::USER_ENTITY_ATTRIBUTES), ); } /** - * When none of the configured user identifier attributes are present, no subject can be resolved. + * When no user identifier can be resolved from the attributes, no subject can match. */ - public function testResolveSubjectFromAttributesReturnsNullWhenNoIdentifier(): void + public function testSubjectMatchesAttributesReturnsFalseWhenNoIdentifier(): void { - $this->assertNull( - $this->mock()->resolveSubjectFromAttributes(['someOtherAttribute' => ['value']]), + $this->assertFalse( + $this->mock()->subjectMatchesAttributes('anything', ['someOtherAttribute' => ['value']]), ); } diff --git a/tests/unit/src/Services/IdTokenBuilderTest.php b/tests/unit/src/Services/IdTokenBuilderTest.php index a36c364b..774a1fea 100644 --- a/tests/unit/src/Services/IdTokenBuilderTest.php +++ b/tests/unit/src/Services/IdTokenBuilderTest.php @@ -108,7 +108,9 @@ public function testCanBuild(): void $this->arrayHasKey(ClaimsEnum::Iss->value), ); - $this->claimTranslatorExtractorMock->expects($this->once()) + // extract() is called twice: once with the openid scope to resolve the canonical `sub`, and once with the + // access token scopes to gather the claims to release. + $this->claimTranslatorExtractorMock->expects($this->exactly(2)) ->method('extract') ->willReturn(['foo' => 'bar']); @@ -131,6 +133,105 @@ public function testCanBuild(): void ); } + /** + * The issued `sub` must be the canonical subject (the mapped `sub` claim when one is configured), regardless of + * whether the client releases the user's claims in the ID Token. A stable `sub` is relied upon elsewhere, e.g. + * when matching an `id_token_hint`. + */ + public function testSubIsCanonicalRegardlessOfClaimRelease(): void + { + $this->userEntityMock->method('getIdentifier')->willReturn('raw-identifier'); + $this->userEntityMock->method('getClaims')->willReturn(['uid' => ['raw-identifier']]); + + // The `sub` payload value passes through the type helper, so make it return the value it is given. + $typeHelperMock = $this->createMock(\SimpleSAML\OpenID\Helpers\Type::class); + $typeHelperMock->method('ensureNonEmptyString')->willReturnArgument(0); + $dateTimeHelperMock = $this->createMock(\SimpleSAML\OpenID\Helpers\DateTime::class); + $dateTimeHelperMock->method('getUtc')->willReturn(new DateTimeImmutable()); + $randomHelperMock = $this->createMock(\SimpleSAML\OpenID\Helpers\Random::class); + $randomHelperMock->method('string')->willReturn('random-jti'); + $openIdHelpersMock = $this->createMock(\SimpleSAML\OpenID\Helpers::class); + $openIdHelpersMock->method('type')->willReturn($typeHelperMock); + $openIdHelpersMock->method('dateTime')->willReturn($dateTimeHelperMock); + $openIdHelpersMock->method('random')->willReturn($randomHelperMock); + $this->coreMock->method('helpers')->willReturn($openIdHelpersMock); + + $this->claimTranslatorExtractorMock->method('extract') + ->willReturnCallback( + fn(array $scopes, array $claims): array => $scopes === ['openid'] ? + ['sub' => 'mapped-subject'] : + [], + ); + $this->claimTranslatorExtractorMock->method('extractAdditionalIdTokenClaims')->willReturn([]); + + $this->idTokenFactoryMock->expects($this->once())->method('fromData') + ->with( + $this->anything(), + $this->anything(), + $this->callback( + fn(array $payload): bool => ($payload[ClaimsEnum::Sub->value] ?? null) === 'mapped-subject', + ), + $this->anything(), + ); + + // add_claims_to_id_token = false: the user's claims are not released, but `sub` must still be canonical. + $this->sut()->buildFor( + $this->userEntityMock, + $this->accessTokenEntityMock, + false, + false, + null, + null, + null, + null, + ); + } + + /** + * The subject is REQUIRED, so it must be kept even when its value would be considered falsy (e.g. "0"). + */ + public function testSubIsKeptForFalsyValue(): void + { + $this->userEntityMock->method('getIdentifier')->willReturn('0'); + $this->userEntityMock->method('getClaims')->willReturn(['uid' => ['0']]); + + $typeHelperMock = $this->createMock(\SimpleSAML\OpenID\Helpers\Type::class); + $typeHelperMock->method('ensureNonEmptyString')->willReturnArgument(0); + $dateTimeHelperMock = $this->createMock(\SimpleSAML\OpenID\Helpers\DateTime::class); + $dateTimeHelperMock->method('getUtc')->willReturn(new DateTimeImmutable()); + $randomHelperMock = $this->createMock(\SimpleSAML\OpenID\Helpers\Random::class); + $randomHelperMock->method('string')->willReturn('random-jti'); + $openIdHelpersMock = $this->createMock(\SimpleSAML\OpenID\Helpers::class); + $openIdHelpersMock->method('type')->willReturn($typeHelperMock); + $openIdHelpersMock->method('dateTime')->willReturn($dateTimeHelperMock); + $openIdHelpersMock->method('random')->willReturn($randomHelperMock); + $this->coreMock->method('helpers')->willReturn($openIdHelpersMock); + + $this->claimTranslatorExtractorMock->method('extract')->willReturn([]); + $this->claimTranslatorExtractorMock->method('extractAdditionalIdTokenClaims')->willReturn([]); + + $this->idTokenFactoryMock->expects($this->once())->method('fromData') + ->with( + $this->anything(), + $this->anything(), + $this->callback( + fn(array $payload): bool => ($payload[ClaimsEnum::Sub->value] ?? null) === '0', + ), + $this->anything(), + ); + + $this->sut()->buildFor( + $this->userEntityMock, + $this->accessTokenEntityMock, + false, + false, + null, + null, + null, + null, + ); + } + public function testWillNegotiateIdTokenSignatureAlgorithm(): void { $this->clientEntityMock->method('getIdTokenSignedResponseAlg')