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 78838580..6de5ee30 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -214,6 +214,36 @@ 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 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 604aac6f..e731975d 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, @@ -1000,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 14312917..2d52c45a 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, @@ -201,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 389ec235..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 { @@ -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, ); } @@ -77,19 +85,41 @@ 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( - '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,18 +127,39 @@ 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, ); } + // 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 6c88db03..5b2ed80a 100644 --- a/src/Server/RequestRules/Rules/PromptRule.php +++ b/src/Server/RequestRules/Rules/PromptRule.php @@ -84,18 +84,48 @@ 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, + ); + } + + // `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 && + !$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 (in_array('login', $prompt, true) && $authSimple->isAuthenticated()) { 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 56c248f4..1c922c0e 100644 --- a/src/Services/AuthenticationService.php +++ b/src/Services/AuthenticationService.php @@ -330,6 +330,37 @@ public function authenticateForClient( $this->authenticate($this->authSimpleFactory->build($clientEntity), $loginParams); } + /** + * 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). + * + * 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. + * + * 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 subjectMatchesAttributes(string $subject, array $attributes): bool + { + $userId = $this->userIdentifierResolver->resolve($this->userIdAttrs, $attributes); + if ($userId === 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 hash_equals($canonicalSubject, $subject); + } + /** * Store Relying on Party Association to the current session. * @throws \Exception 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 60a98b8d..8955dd3e 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\IdTokenHint; use SimpleSAML\Utils\HTTP as SspHttp; #[CoversClass(PromptRule::class)] @@ -116,4 +119,59 @@ 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 testPromptNoneWithMatchingIdTokenHintProceeds(): void + { + $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->authSimpleMock->method('getAttributes')->willReturn(['uid' => ['subject-123']]); + $this->authenticationServiceMock->method('subjectMatchesAttributes')->willReturn(true); + + $this->assertNull($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('subjectMatchesAttributes'); + + $this->assertNull($this->checkRule()); + } + + public function testPromptNoneWithMismatchedIdTokenHintThrowsLoginRequired(): void + { + $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->authSimpleMock->method('getAttributes')->willReturn(['uid' => ['other-user']]); + $this->authenticationServiceMock->method('subjectMatchesAttributes')->willReturn(false); + + $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..a05a7dbe 100644 --- a/tests/unit/src/Services/AuthenticationServiceTest.php +++ b/tests/unit/src/Services/AuthenticationServiceTest.php @@ -481,6 +481,61 @@ public function testItPropagatesLoginHintToAuthentication(): void ); } + /** + * 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 testSubjectMatchesAttributesMatchesUserIdentifier(): void + { + $this->claimTranslatorExtractorMock->method('extract') + ->with(['openid'], self::USER_ENTITY_ATTRIBUTES) + ->willReturn([]); + + $this->assertTrue( + $this->mock()->subjectMatchesAttributes(self::USERNAME, self::USER_ENTITY_ATTRIBUTES), + ); + } + + /** + * 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 testSubjectMatchesAttributesMatchesMappedSubClaim(): void + { + $this->claimTranslatorExtractorMock->method('extract') + ->with(['openid'], self::USER_ENTITY_ATTRIBUTES) + ->willReturn(['sub' => 'mapped-subject']); + + $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 no user identifier can be resolved from the attributes, no subject can match. + */ + public function testSubjectMatchesAttributesReturnsFalseWhenNoIdentifier(): void + { + $this->assertFalse( + $this->mock()->subjectMatchesAttributes('anything', ['someOtherAttribute' => ['value']]), + ); + } + /** * @throws NoState */ 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')