diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md index 43ec6a76..78838580 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -204,6 +204,16 @@ language tags). Note: use the canonical SimpleSAMLphp locale codes in (`pt-br`, `zh-tw`), which SimpleSAMLphp itself warns against, are not fully supported by `ui_locales` on the authorization endpoint, because the language cookie is validated against the raw configured codes. +- Support for the `login_hint` parameter on the authorization endpoint +(previously ignored). The parameter carries a hint about the login identifier +the End-User might use to log in. When present, it is propagated to the +SimpleSAMLphp login page as the pre-filled username (using the standard +`core:username` state key consumed by the core UserPass login form). Per +specification this is best-effort: authentication sources that do not use the +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`. - 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/Factories/RequestRulesManagerFactory.php b/src/Factories/RequestRulesManagerFactory.php index 699488fc..86b00b49 100644 --- a/src/Factories/RequestRulesManagerFactory.php +++ b/src/Factories/RequestRulesManagerFactory.php @@ -25,6 +25,7 @@ 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; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\PostLogoutRedirectUriRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\PromptRule; @@ -171,6 +172,7 @@ private function getDefaultRules(): array ), new PostLogoutRedirectUriRule($this->requestParamsResolver, $this->helpers, $this->clientRepository), new UiLocalesRule($this->requestParamsResolver, $this->helpers), + new LoginHintRule($this->requestParamsResolver, $this->helpers), new AcrValuesRule($this->requestParamsResolver, $this->helpers), new ScopeOfflineAccessRule($this->requestParamsResolver, $this->helpers), new ClientAuthenticationRule( diff --git a/src/Server/Grants/AuthCodeGrant.php b/src/Server/Grants/AuthCodeGrant.php index e083ea8c..604aac6f 100644 --- a/src/Server/Grants/AuthCodeGrant.php +++ b/src/Server/Grants/AuthCodeGrant.php @@ -55,6 +55,7 @@ use SimpleSAML\Module\oidc\Server\RequestRules\Rules\CodeChallengeRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\CodeVerifierRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\IssuerStateRule; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\LoginHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\MaxAgeRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\PromptRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\RequestedClaimsRule; @@ -857,6 +858,9 @@ public function validateAuthorizationRequestWithRequestRules( ClientIdRule::class, ResponseTypeRule::class, RequestObjectRule::class, + // 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, PromptRule::class, MaxAgeRule::class, ScopeRule::class, @@ -990,6 +994,12 @@ public function validateAuthorizationRequestWithRequestRules( $this->loggerService->debug('AuthCodeGrant: UI locales: ', ['uiLocales' => $uiLocales]); $authorizationRequest->setUiLocales($uiLocales); + $loginHint = $resultBag->getOrFail(LoginHintRule::class)->getValue(); + // Only log presence, not the value: login_hint is commonly a PII. + $loginHintPresent = $loginHint !== null; + $this->loggerService->debug('AuthCodeGrant: Login hint present: ', ['loginHintPresent' => $loginHintPresent]); + $authorizationRequest->setLoginHint($loginHint); + $authorizationRequest->setIsVciRequest($isVciAuthorizationCodeRequest); $flowType = $isVciAuthorizationCodeRequest ? diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index df837dc2..14312917 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\LoginHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\MaxAgeRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\PromptRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\RequestedClaimsRule; @@ -126,6 +127,9 @@ public function validateAuthorizationRequestWithRequestRules( $rulesToExecute = [ ScopeRule::class, RequestObjectRule::class, + // 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, PromptRule::class, MaxAgeRule::class, RequiredOpenIdScopeRule::class, @@ -194,6 +198,9 @@ public function validateAuthorizationRequestWithRequestRules( $uiLocales = $resultBag->getOrFail(UiLocalesRule::class)->getValue(); $authorizationRequest->setUiLocales($uiLocales); + $loginHint = $resultBag->getOrFail(LoginHintRule::class)->getValue(); + $authorizationRequest->setLoginHint($loginHint); + $responseMode = $resultBag->getOrFail(ResponseModeRule::class)->getValue(); $authorizationRequest->setResponseMode($responseMode); diff --git a/src/Server/RequestRules/Rules/LoginHintRule.php b/src/Server/RequestRules/Rules/LoginHintRule.php new file mode 100644 index 00000000..f984fb60 --- /dev/null +++ b/src/Server/RequestRules/Rules/LoginHintRule.php @@ -0,0 +1,41 @@ + + */ +class LoginHintRule extends AbstractRule +{ + /** + * @inheritDoc + * + * @param ResponseModeInterface $responseMode + * @param HttpMethodsEnum[] $allowedServerRequestMethods + */ + public function checkRule( + ServerRequestInterface $request, + ResultBagInterface $currentResultBag, + LoggerService $loggerService, + array $data = [], + ResponseModeInterface $responseMode = new QueryResponseMode(), + array $allowedServerRequestMethods = [HttpMethodsEnum::GET], + ): ?Result { + return new Result($this->getKey(), $this->requestParamsResolver->getAsStringBasedOnAllowedMethods( + ParamsEnum::LoginHint->value, + $request, + $allowedServerRequestMethods, + )); + } +} diff --git a/src/Server/RequestRules/Rules/MaxAgeRule.php b/src/Server/RequestRules/Rules/MaxAgeRule.php index 89caab14..4eebce56 100644 --- a/src/Server/RequestRules/Rules/MaxAgeRule.php +++ b/src/Server/RequestRules/Rules/MaxAgeRule.php @@ -117,6 +117,11 @@ public function checkRule( $this->sspBridge->utils()->http()->getSelfURLNoQuery(), $requestParams, ); + // Propagate login_hint (resolved earlier by LoginHintRule) as the pre-filled username. + $loginHint = $currentResultBag->getOrFail(LoginHintRule::class)->getValue(); + if ($loginHint !== null) { + $loginParams[AuthenticationService::LOGIN_PARAM_USERNAME] = $loginHint; + } $this->authenticationService->authenticateForClient($client, $loginParams); } diff --git a/src/Server/RequestRules/Rules/PromptRule.php b/src/Server/RequestRules/Rules/PromptRule.php index 276d94c0..6c88db03 100644 --- a/src/Server/RequestRules/Rules/PromptRule.php +++ b/src/Server/RequestRules/Rules/PromptRule.php @@ -105,6 +105,11 @@ public function checkRule( $this->sspBridge->utils()->http()->getSelfURLNoQuery(), $requestParams, ); + // Propagate login_hint (resolved earlier by LoginHintRule) as the pre-filled username. + $loginHint = $currentResultBag->getOrFail(LoginHintRule::class)->getValue(); + if ($loginHint !== null) { + $loginParams[AuthenticationService::LOGIN_PARAM_USERNAME] = $loginHint; + } $this->authenticationService->authenticateForClient($client, $loginParams); } diff --git a/src/Server/RequestTypes/AuthorizationRequest.php b/src/Server/RequestTypes/AuthorizationRequest.php index 6c2cb018..dc3747d6 100644 --- a/src/Server/RequestTypes/AuthorizationRequest.php +++ b/src/Server/RequestTypes/AuthorizationRequest.php @@ -42,6 +42,11 @@ class AuthorizationRequest extends OAuth2AuthorizationRequest */ protected ?string $uiLocales = null; + /** + * Hint about the login identifier the End-User might use to log in, as requested using the login_hint parameter. + */ + protected ?string $loginHint = null; + /** * ACR used during authn. */ @@ -241,6 +246,16 @@ public function setUiLocales(?string $uiLocales): void $this->uiLocales = $uiLocales; } + public function getLoginHint(): ?string + { + return $this->loginHint; + } + + public function setLoginHint(?string $loginHint): void + { + $this->loginHint = $loginHint; + } + public function getAcr(): ?string { return $this->acr; diff --git a/src/Services/AuthenticationService.php b/src/Services/AuthenticationService.php index dcad70d0..56c248f4 100644 --- a/src/Services/AuthenticationService.php +++ b/src/Services/AuthenticationService.php @@ -46,6 +46,11 @@ class AuthenticationService { + /** + * Login parameter (state array key) used to pre-fill the username on the SimpleSAMLphp core UserPass login form. + */ + public const string LOGIN_PARAM_USERNAME = 'core:username'; + /** * ID of auth source used during authn. */ @@ -100,7 +105,7 @@ public function processRequest( $this->authSourceId = $authSimple->getAuthSource()->getAuthId(); if (! $authSimple->isAuthenticated()) { - $this->authenticate($authSimple); + $this->authenticate($authSimple, $this->resolveLoginParams($authorizationRequest)); } elseif ($this->sessionService->getIsAuthnPerformedInPreviousRequest()) { $this->sessionService->setIsAuthnPerformedInPreviousRequest(false); @@ -272,6 +277,29 @@ public function getSessionId(): ?string return $this->sessionService->getCurrentSession()->getSessionId(); } + /** + * Resolve additional login parameters to pass to the authentication source, based on the authorization request. + * + * Currently this propagates the login_hint authorization request parameter to the SimpleSAMLphp login page as + * the pre-filled username (the standard 'core:username' state key consumed by the core UserPass login form). + * Per specification this is a hint about the identifier the End-User might use to log in, so it is best-effort: + * auth sources that do not use the core login form simply ignore it, and an incorrect value is corrected by the + * user (or the login simply fails with invalid credentials). + * + * @return array + */ + protected function resolveLoginParams(OAuth2AuthorizationRequestInterface $authorizationRequest): array + { + if ( + !$authorizationRequest instanceof AuthorizationRequest || + ($loginHint = $authorizationRequest->getLoginHint()) === null + ) { + return []; + } + + return [self::LOGIN_PARAM_USERNAME => $loginHint]; + } + /** * @throws Error\BadRequest * @throws Error\NotFound diff --git a/tests/unit/src/Server/RequestRules/Rules/LoginHintRuleTest.php b/tests/unit/src/Server/RequestRules/Rules/LoginHintRuleTest.php new file mode 100644 index 00000000..62e7bd7f --- /dev/null +++ b/tests/unit/src/Server/RequestRules/Rules/LoginHintRuleTest.php @@ -0,0 +1,95 @@ +requestStub = $this->createStub(ServerRequestInterface::class); + $this->requestStub->method('getMethod')->willReturn('GET'); + + $this->resultBagStub = $this->createStub(ResultBagInterface::class); + $this->loggerServiceStub = $this->createStub(LoggerService::class); + $this->requestParamsResolverStub = $this->createStub(RequestParamsResolver::class); + $this->helpers = new Helpers(); + $this->responseModeStub = $this->createStub(ResponseModeInterface::class); + } + + protected function sut( + ?RequestParamsResolver $requestParamsResolver = null, + ?Helpers $helpers = null, + ): LoginHintRule { + $requestParamsResolver ??= $this->requestParamsResolverStub; + $helpers ??= $this->helpers; + + return new LoginHintRule( + $requestParamsResolver, + $helpers, + ); + } + + /** + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + */ + public function testCheckRuleReturnsResultWhenParamSet() + { + $this->requestParamsResolverStub->method('getAsStringBasedOnAllowedMethods')->willReturn('user@example.org'); + + $result = $this->sut()->checkRule( + $this->requestStub, + $this->resultBagStub, + $this->loggerServiceStub, + [], + $this->responseModeStub, + ) ?? + new Result(LoginHintRule::class); + + $this->assertEquals('user@example.org', $result->getValue()); + } + + /** + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + */ + public function testCheckRuleReturnsNullWhenParamNotSet() + { + $this->requestStub->method('getQueryParams')->willReturn([]); + + $result = $this->sut()->checkRule( + $this->requestStub, + $this->resultBagStub, + $this->loggerServiceStub, + [], + $this->responseModeStub, + ) ?? + new Result(LoginHintRule::class); + + $this->assertNull($result->getValue()); + } +} diff --git a/tests/unit/src/Server/RequestRules/Rules/MaxAgeRuleTest.php b/tests/unit/src/Server/RequestRules/Rules/MaxAgeRuleTest.php index 920d3528..1927a6fa 100644 --- a/tests/unit/src/Server/RequestRules/Rules/MaxAgeRuleTest.php +++ b/tests/unit/src/Server/RequestRules/Rules/MaxAgeRuleTest.php @@ -10,17 +10,22 @@ use Psr\Http\Message\ServerRequestInterface; use SimpleSAML\Auth\Simple; use SimpleSAML\Module\oidc\Bridges\SspBridge; +use SimpleSAML\Module\oidc\Bridges\SspBridge\Utils as SspBridgeUtils; use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\Factories\AuthSimpleFactory; use SimpleSAML\Module\oidc\Helpers; 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\LoginHintRule; use SimpleSAML\Module\oidc\Server\RequestRules\Rules\MaxAgeRule; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\StateRule; use SimpleSAML\Module\oidc\Server\ResponseModes\ResponseModeInterface; use SimpleSAML\Module\oidc\Services\AuthenticationService; use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; +use SimpleSAML\Utils\HTTP as SspHttp; #[CoversClass(MaxAgeRule::class)] class MaxAgeRuleTest extends TestCase @@ -111,4 +116,34 @@ public function testDefaultMaxAgeNotExpiredReturnsAuthInstant(): void $this->assertNotNull($this->checkRule()); } + + public function testExpiredMaxAgeReAuthenticatesAndPropagatesLoginHint(): void + { + $this->resultBag->add(new Result(ClientRedirectUriRule::class, 'https://rp.example.org/cb')); + $this->resultBag->add(new Result(StateRule::class, 'state123')); + $this->resultBag->add(new Result(LoginHintRule::class, 'user@example.org')); + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') + ->willReturn(['max_age' => 0, 'login_hint' => 'user@example.org']); + $this->clientMock->method('getRequireAuthTime')->willReturn(false); + $this->authSimpleMock->method('isAuthenticated')->willReturn(true); + // Authenticated well before the (zero) max_age window, so re-authentication is enforced. + $this->authSimpleMock->method('getAuthData')->willReturn(time() - 3600); + + $httpMock = $this->createMock(SspHttp::class); + $httpMock->method('getSelfURLNoQuery')->willReturn('https://op.example.org/authorize'); + $httpMock->method('addURLParameters')->willReturn('https://op.example.org/authorize?max_age=0'); + $utilsMock = $this->createMock(SspBridgeUtils::class); + $utilsMock->method('http')->willReturn($httpMock); + $this->sspBridgeMock->method('utils')->willReturn($utilsMock); + + $this->authenticationServiceMock->expects($this->once()) + ->method('authenticateForClient') + ->with( + $this->clientMock, + $this->callback(fn(array $loginParams): bool => + ($loginParams['core:username'] ?? null) === 'user@example.org'), + ); + + $this->checkRule(); + } } diff --git a/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php b/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php new file mode 100644 index 00000000..60a98b8d --- /dev/null +++ b/tests/unit/src/Server/RequestRules/Rules/PromptRuleTest.php @@ -0,0 +1,119 @@ +requestParamsResolverMock = $this->createMock(RequestParamsResolver::class); + $this->authSimpleFactoryMock = $this->createMock(AuthSimpleFactory::class); + $this->authenticationServiceMock = $this->createMock(AuthenticationService::class); + $this->sspBridgeMock = $this->createMock(SspBridge::class); + $this->authSimpleMock = $this->createMock(Simple::class); + $this->clientMock = $this->createMock(ClientEntityInterface::class); + $this->loggerServiceMock = $this->createMock(LoggerService::class); + $this->requestMock = $this->createMock(ServerRequestInterface::class); + $this->responseModeMock = $this->createMock(ResponseModeInterface::class); + + $this->authSimpleFactoryMock->method('build')->willReturn($this->authSimpleMock); + + $this->resultBag = new ResultBag(); + $this->resultBag->add(new Result(ClientRule::class, $this->clientMock)); + $this->resultBag->add(new Result(ClientRedirectUriRule::class, 'https://rp.example.org/cb')); + $this->resultBag->add(new Result(StateRule::class, 'state123')); + } + + protected function sut(): PromptRule + { + return new PromptRule( + $this->requestParamsResolverMock, + new Helpers(), + $this->authSimpleFactoryMock, + $this->authenticationServiceMock, + $this->sspBridgeMock, + ); + } + + protected function checkRule(): ?Result + { + return $this->sut()->checkRule( + $this->requestMock, + $this->resultBag, + $this->loggerServiceMock, + [], + $this->responseModeMock, + ); + } + + public function testReturnsNullWhenNoPromptParam(): void + { + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods')->willReturn([]); + $this->authenticationServiceMock->expects($this->never())->method('authenticateForClient'); + + $this->assertNull($this->checkRule()); + } + + public function testPromptLoginReAuthenticatesAndPropagatesLoginHint(): void + { + $this->resultBag->add(new Result(LoginHintRule::class, 'user@example.org')); + $this->requestParamsResolverMock->method('getAllBasedOnAllowedMethods') + ->willReturn(['prompt' => 'login', 'login_hint' => 'user@example.org']); + $this->authSimpleMock->method('isAuthenticated')->willReturn(true); + + $httpMock = $this->createMock(SspHttp::class); + $httpMock->method('getSelfURLNoQuery')->willReturn('https://op.example.org/authorize'); + $httpMock->method('addURLParameters') + ->willReturn('https://op.example.org/authorize?login_hint=user@example.org'); + $utilsMock = $this->createMock(SspBridgeUtils::class); + $utilsMock->method('http')->willReturn($httpMock); + $this->sspBridgeMock->method('utils')->willReturn($utilsMock); + + $this->authenticationServiceMock->expects($this->once()) + ->method('authenticateForClient') + ->with( + $this->clientMock, + $this->callback(fn(array $loginParams): bool => + ($loginParams['core:username'] ?? null) === 'user@example.org'), + ); + + $this->assertNull($this->checkRule()); + } +} diff --git a/tests/unit/src/Services/AuthenticationServiceTest.php b/tests/unit/src/Services/AuthenticationServiceTest.php index a6538d9c..c8bceac2 100644 --- a/tests/unit/src/Services/AuthenticationServiceTest.php +++ b/tests/unit/src/Services/AuthenticationServiceTest.php @@ -433,6 +433,54 @@ public function testItProcessesRequest(bool $isAuthnPer): void ); } + /** + * When the user is not yet authenticated and a login_hint is present in the authorization request, it must be + * propagated to the authentication source as the pre-filled username (the 'core:username' login parameter). + * + * @throws \Throwable + */ + public function testItPropagatesLoginHintToAuthentication(): void + { + $authenticationServiceMock = $this->getMockBuilder(AuthenticationService::class) + ->enableOriginalConstructor() + ->setConstructorArgs([ + $this->userRepositoryMock, + $this->authSimpleFactoryMock, + $this->clientRepositoryMock, + $this->opMetadataService, + $this->sessionServiceMock, + $this->claimTranslatorExtractorMock, + $this->moduleConfigMock, + $this->processingChainFactoryMock, + $this->stateServiceMock, + $this->requestParamsResolverMock, + $this->userEntityFactoryMock, + $this->routesMock, + new UserIdentifierResolver(), + ]) + ->onlyMethods(['runAuthProcs', 'prepareStateArray', 'authenticate']) + ->getMock(); + + $this->authSimpleMock->method('getAuthSource')->willReturn($this->authSourceMock); + $this->authSourceMock->method('getAuthId')->willReturn(self::AUTH_SOURCE); + $this->authSimpleMock->expects($this->once())->method('isAuthenticated')->willReturn(false); + $this->authorizationRequestMock->method('getClient')->willReturn($this->clientEntityMock); + $this->authorizationRequestMock->method('getLoginHint')->willReturn('user@example.org'); + $authenticationServiceMock->method('prepareStateArray')->willReturn(self::STATE); + + $authenticationServiceMock->expects($this->once()) + ->method('authenticate') + ->with($this->authSimpleMock, ['core:username' => 'user@example.org']); + + $this->assertSame( + $authenticationServiceMock->processRequest( + $this->serverRequestMock, + $this->authorizationRequestMock, + ), + self::STATE, + ); + } + /** * @throws NoState */