From 8cf955dab898e2ae4bc53cfa8ade65257b9ca731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Fri, 6 Nov 2026 15:23:31 +0100 Subject: [PATCH] Give the authentication source Source and Destination before login An authentication source that reports failed logins, for example in F-ticks, reads $state['Source']['entityid'] and $state['Destination']['entityid']. casserver set them only after login, for the authproc filters, so a failed CAS login named neither entity. handleAuthenticate() now passes both to Auth\Simple::login(), with the values the filters see after a successful login: Source comes from resolveIdpEntityId(), now shared with runAuthProcs(), and Destination is the requesting service, left out when the request names none. This covers interactive, renewed and passive logins. A session keeps neither key, so login()'s check that a resumed state belongs to the requested service does not bind a session to its first service. The tests pin both the equal values and that. Assisted-by: Claude Code (claude-opus-5-5, implementation & unit tests) Assisted-by: Google Antigravity (gemini-3.8-flash-high, review) --- config/module_casserver.php.dist | 2 + docs/ChangeLog.md | 6 + src/Cas/AttributeExtractor.php | 5 +- src/Controller/LoginController.php | 15 ++ tests/src/Controller/LoginControllerTest.php | 238 +++++++++++++++++++ 5 files changed, 264 insertions(+), 2 deletions(-) diff --git a/config/module_casserver.php.dist b/config/module_casserver.php.dist index b4dd55a..97806aa 100644 --- a/config/module_casserver.php.dist +++ b/config/module_casserver.php.dist @@ -102,6 +102,8 @@ $config = [ * as $state['Source']['entityid']. Filters that key off the authenticating entity (federation * logging such as F-ticks, per-entity authorization, audit trails) need it. The requesting CAS * service URL is made available to the same filters as $state['Destination']['entityid']. + * The authentication source gets both in the login state before the user logs in, so that it can + * report a failed login against them. * * NOTE: this is unrelated to the ?entityId= query parameter of the login endpoint, which selects * which upstream SAML IdP the authsource should authenticate against. diff --git a/docs/ChangeLog.md b/docs/ChangeLog.md index 7bd087b..f7447d6 100644 --- a/docs/ChangeLog.md +++ b/docs/ChangeLog.md @@ -1,5 +1,11 @@ # Changelog +## [Unreleased] + +### Changed + +- Give the authentication source Source and Destination before login, so that it can report a failed login against them (Marko Ivančić) + ## [7.0.5] - 2026-08-11 ### Changed diff --git a/src/Cas/AttributeExtractor.php b/src/Cas/AttributeExtractor.php index 7626661..601cc98 100644 --- a/src/Cas/AttributeExtractor.php +++ b/src/Cas/AttributeExtractor.php @@ -140,7 +140,8 @@ protected function runAuthProcs(array &$state): void /** - * Resolve the entity ID that casserver presents to the authproc filters as the authenticating IdP. + * Resolve the entity ID that casserver presents as the authenticating IdP, to the authentication + * source before login and to the authproc filters after it. * * casserver has no entity ID of its own, so the value is resolved in the following order: * whatever the state already carries, then the 'idp_entity_id' configuration option, and finally @@ -152,7 +153,7 @@ protected function runAuthProcs(array &$state): void * @return string * @throws \Exception */ - private function resolveIdpEntityId(mixed $stateEntityId): string + public function resolveIdpEntityId(mixed $stateEntityId = null): string { if (\is_string($stateEntityId) && $stateEntityId !== '') { return $stateEntityId; diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 0d13299..b88b51d 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -193,6 +193,7 @@ public function login( forceAuthn: $forceAuthn, returnToUrl: $returnToUrl, entityId: $entityId, + serviceUrl: $serviceUrl, ); } @@ -501,6 +502,7 @@ private function instantiateClassDependencies(): void * @param bool $forceAuthn * @param string $returnToUrl * @param string|null $entityId + * @param string|null $serviceUrl * * @return \SimpleSAML\HTTP\RunnableResponse */ @@ -508,12 +510,14 @@ private function handleInteractiveAuthenticate( bool $forceAuthn, string $returnToUrl, ?string $entityId, + ?string $serviceUrl, ): RunnableResponse { return $this->handleAuthenticate( forceAuthn: $forceAuthn, gateway: false, returnToUrl: $returnToUrl, entityId: $entityId, + serviceUrl: $serviceUrl, ); } @@ -548,6 +552,7 @@ private function handleUnauthenticatedGateway( gateway: true, returnToUrl: $returnToUrl, entityId: $entityId, + serviceUrl: $serviceUrl, ); } @@ -559,6 +564,7 @@ private function handleUnauthenticatedGateway( * @param bool $gateway Whether authentication should be passive/non-interactive * @param string $returnToUrl URL to return to after authentication * @param string|null $entityId Optional specific IdP entity ID to use + * @param string|null $serviceUrl The requesting CAS service, if the request names one * * @return \SimpleSAML\HTTP\RunnableResponse Response containing the login redirect */ @@ -567,13 +573,22 @@ private function handleAuthenticate( bool $gateway, string $returnToUrl, ?string $entityId, + ?string $serviceUrl, ): RunnableResponse { $params = [ 'ForceAuthn' => $forceAuthn, 'isPassive' => $gateway, 'ReturnTo' => $returnToUrl, + // The same entities the authproc filters see after a successful login, so that an authentication + // source can report a failed login against them. They end with the login state: a session does not + // keep them, so they do not bind it to this service. + 'Source' => ['entityid' => $this->attributeExtractor->resolveIdpEntityId()], ]; + if (!empty($serviceUrl)) { + $params['Destination'] = ['entityid' => $serviceUrl]; + } + if (isset($entityId)) { $params['saml:idp'] = $entityId; } diff --git a/tests/src/Controller/LoginControllerTest.php b/tests/src/Controller/LoginControllerTest.php index e9eedc1..8cf05b3 100644 --- a/tests/src/Controller/LoginControllerTest.php +++ b/tests/src/Controller/LoginControllerTest.php @@ -9,6 +9,7 @@ use PHPUnit\Framework\TestCase; use SimpleSAML\Auth\ProcessingChain; use SimpleSAML\Auth\Simple; +use SimpleSAML\Auth\State; use SimpleSAML\Configuration; use SimpleSAML\HTTP\RunnableResponse; use SimpleSAML\Module; @@ -160,6 +161,8 @@ public static function loginOnAuthenticateParameters(): array 'isPassive' => false, // phpcs:ignore Generic.Files.LineLength.TooLong 'ReturnTo' => 'http://localhost/?service=https%3A%2F%2Fexample.com%2Fssp%2Fmodule.php%2Fcas%2Flinkback.php', + 'Source' => ['entityid' => 'http://localhost/'], + 'Destination' => ['entityid' => 'https://example.com/ssp/module.php/cas/linkback.php'], ], [], ], @@ -174,6 +177,8 @@ public static function loginOnAuthenticateParameters(): array 'isPassive' => false, // phpcs:ignore Generic.Files.LineLength.TooLong 'ReturnTo' => 'http://localhost/?service=https%3A%2F%2Fexample.com%2Fssp%2Fmodule.php%2Fcas%2Flinkback.php&entityId=http%3A%2F%2Flocalhost%2FentityId%2Fsso', + 'Source' => ['entityid' => 'http://localhost/'], + 'Destination' => ['entityid' => 'https://example.com/ssp/module.php/cas/linkback.php'], ], [], ], @@ -187,6 +192,8 @@ public static function loginOnAuthenticateParameters(): array 'isPassive' => false, // phpcs:ignore Generic.Files.LineLength.TooLong 'ReturnTo' => 'http://localhost/?service=https%3A%2F%2Fexample.com%2Fssp%2Fmodule.php%2Fcas%2Flinkback.php&scope=desktop', + 'Source' => ['entityid' => 'http://localhost/'], + 'Destination' => ['entityid' => 'https://example.com/ssp/module.php/cas/linkback.php'], 'saml:IDPList' => [ 'http://localhost/entityId/sso/scope/A', 'http://localhost/entityId/sso/scope/B', @@ -210,6 +217,8 @@ public static function loginOnAuthenticateParameters(): array 'isPassive' => false, // phpcs:ignore Generic.Files.LineLength.TooLong 'ReturnTo' => 'http://localhost/?service=https%3A%2F%2Fexample.com%2Fssp%2Fmodule.php%2Fcas%2Flinkback.php&scope=desktop', + 'Source' => ['entityid' => 'http://localhost/'], + 'Destination' => ['entityid' => 'https://example.com/ssp/module.php/cas/linkback.php'], 'saml:idp' => 'http://localhost/entityId/sso/scope/A', ], [ @@ -255,6 +264,231 @@ public function testAuthSourceLogin(array $requestParameters, array $loginParame } + public function testLoginParamsTakeSourceFromTheConfiguration(): void + { + $idpEntityId = 'https://login.example.org/idp/metadata.php'; + $moduleConfig = $this->moduleConfig; + $moduleConfig['idp_entity_id'] = $idpEntityId; + + $loginParams = $this->loginParamsFor( + $moduleConfig, + ['service' => 'https://example.com/ssp/module.php/cas/linkback.php'], + ); + + $this->assertEquals(['entityid' => $idpEntityId], $loginParams['Source'] ?? null); + } + + + /** + * Older CAS clients name the service with TARGET. + */ + public function testLoginParamsTakeDestinationFromTarget(): void + { + $serviceUrl = 'https://example.com/ssp/module.php/cas/linkback.php'; + + $loginParams = $this->loginParamsFor($this->moduleConfig, ['TARGET' => $serviceUrl]); + + $this->assertEquals(['entityid' => $serviceUrl], $loginParams['Destination'] ?? null); + } + + + /** + * A login to casserver itself has no service to name, and such a login does not run authproc filters either. + */ + public function testLoginParamsHaveNoDestinationWithoutAService(): void + { + $loginParams = $this->loginParamsFor($this->moduleConfig, []); + + $this->assertEquals(['entityid' => 'http://localhost/'], $loginParams['Source'] ?? null); + $this->assertArrayNotHasKey('Destination', $loginParams); + } + + + /** + * Renew makes a logged-in user log in again, and that login can fail like any other. + */ + public function testLoginParamsOfARenewedLoginNameTheEntities(): void + { + $serviceUrl = 'https://example.com/ssp/module.php/cas/linkback.php'; + + $loginParams = $this->loginParamsFor( + $this->moduleConfig, + ['service' => $serviceUrl, 'renew' => true, 'renewId' => 'not-the-renew-id-of-this-session'], + authenticated: true, + ); + + $this->assertTrue($loginParams['ForceAuthn'] ?? null); + $this->assertEquals(['entityid' => 'http://localhost/'], $loginParams['Source'] ?? null); + $this->assertEquals(['entityid' => $serviceUrl], $loginParams['Destination'] ?? null); + } + + + public static function entitiesProvider(): array + { + $serviceUrl = 'https://example.com/ssp/module.php/cas/linkback.php'; + + return [ + 'Configured' => [ + 'https://login.example.org/idp/metadata.php', + 'https://login.example.org/idp/metadata.php', + $serviceUrl, + ], + 'Base URL when not configured' => [null, 'http://localhost/', $serviceUrl], + // login() checks the service against the legal URLs without its session ID, and keeps it as it came + 'Service with a session ID' => [null, 'http://localhost/', $serviceUrl . ';jsessionid=abc123'], + ]; + } + + + /** + * An authentication source reporting a failed login has to name the same entities as an authproc + * filter reporting a successful one. + * + * @throws \Exception + */ + #[DataProvider('entitiesProvider')] + public function testLoginParamsNameTheEntitiesTheAuthprocFiltersSee( + ?string $configured, + string $expected, + string $serviceUrl, + ): void { + $moduleConfig = $this->moduleConfig; + if ($configured !== null) { + $moduleConfig['idp_entity_id'] = $configured; + } + $moduleConfig['authproc'] = [ + [ + 'class' => 'core:PHP', + 'code' => '$attributes["observedIdpEntityId"] = [$state["Source"]["entityid"] ?? "MISSING"];' + . '$attributes["observedSpEntityId"] = [$state["Destination"]["entityid"] ?? "MISSING"];', + ], + ]; + + $loginParams = $this->loginParamsFor($moduleConfig, ['service' => $serviceUrl]); + $ticket = $this->serviceTicketAfterLogin($moduleConfig, $serviceUrl, $loginParams); + + $this->assertEquals(['entityid' => $expected], $loginParams['Source'] ?? null); + $this->assertEquals(['entityid' => $serviceUrl], $loginParams['Destination'] ?? null); + $this->assertEquals([$expected], $ticket['attributes']['observedIdpEntityId'] ?? null); + $this->assertEquals([$serviceUrl], $ticket['attributes']['observedSpEntityId'] ?? null); + } + + + /** + * A session opened by a login for one service has to stay usable for another. login() refuses a + * service other than the one in the state, so Destination must not reach the session. + * + * @throws \Exception + */ + public function testLoginParamsDoNotBindTheSessionToTheService(): void + { + $otherServiceUrl = 'https://other.example.com/cas/linkback.php'; + $moduleConfig = $this->moduleConfig; + $moduleConfig['legal_service_urls'][] = $otherServiceUrl; + + $loginParams = $this->loginParamsFor( + $moduleConfig, + ['service' => 'https://example.com/ssp/module.php/cas/linkback.php'], + ); + $ticket = $this->serviceTicketAfterLogin($moduleConfig, $otherServiceUrl, $loginParams); + + $this->assertEquals($otherServiceUrl, $ticket['service'] ?? null); + } + + + /** + * What the authentication source is asked to log in with. + * + * @param array $moduleConfig + * @param array $queryParameters + * @return array + */ + private function loginParamsFor(array $moduleConfig, array $queryParameters, bool $authenticated = false): array + { + $controllerMock = $this->getMockBuilder(LoginController::class) + ->setConstructorArgs([ + $this->sspConfig, + Configuration::loadFromArray($moduleConfig), + $this->authSimpleMock, + $this->httpUtils, + ]) + ->onlyMethods(['getSession']) + ->getMock(); + $controllerMock->expects($this->once())->method('getSession')->willReturn($this->sessionMock); + $this->sessionMock->expects($this->once())->method('getSessionId')->willReturn(session_create_id()); + $this->authSimpleMock->expects($this->atMost(1))->method('isAuthenticated')->willReturn($authenticated); + + $loginRequest = Request::create( + uri: Module::getModuleURL('casserver/login'), + parameters: $queryParameters, + ); + $response = $this->callLogin($controllerMock, $loginRequest, $queryParameters); + + $this->assertInstanceOf(RunnableResponse::class, $response); + $this->assertEquals('login', ((array)$response->getCallable())[1] ?? ''); + + return $response->getArguments()[0] ?? []; + } + + + /** + * The service ticket issued once the user is back from a login which those parameters started. + * The session holds what SSP keeps of the completed login state, not the state itself. + * + * @param array $moduleConfig + * @param array $loginParams + * @return array + * @throws \Exception + */ + private function serviceTicketAfterLogin(array $moduleConfig, string $serviceUrl, array $loginParams): array + { + $authData = State::getPersistentAuthData(array_merge($loginParams, [ + 'Attributes' => ['eduPersonPrincipalName' => ['testuser@example.com']], + 'Expire' => 9999999999, + ])); + + $authSimpleMock = $this->getMockBuilder(Simple::class) + ->disableOriginalConstructor() + ->onlyMethods(['getAuthData', 'isAuthenticated', 'getAuthDataArray']) + ->getMock(); + $authSimpleMock->expects($this->atLeastOnce())->method('isAuthenticated')->willReturn(true); + $authSimpleMock->expects($this->once())->method('getAuthData')->with('Expire')->willReturn(9999999999); + $authSimpleMock->expects($this->once())->method('getAuthDataArray')->willReturn($authData); + + $sessionMock = $this->getMockBuilder(Session::class) + ->disableOriginalConstructor() + ->onlyMethods(['getSessionId']) + ->getMock(); + $sessionMock->expects($this->exactly(2))->method('getSessionId')->willReturn(session_create_id()); + + $controllerMock = $this->getMockBuilder(LoginController::class) + ->setConstructorArgs([ + $this->sspConfig, + Configuration::loadFromArray($moduleConfig), + $authSimpleMock, + $this->httpUtils, + ]) + ->onlyMethods(['getSession']) + ->getMock(); + $controllerMock->expects($this->once())->method('getSession')->willReturn($sessionMock); + + $queryParameters = ['service' => $serviceUrl]; + $loginRequest = Request::create( + uri: Module::getModuleURL('casserver/login'), + parameters: $queryParameters, + ); + $response = $this->callLogin($controllerMock, $loginRequest, $queryParameters); + + $this->assertInstanceOf(RunnableResponse::class, $response); + $ticketId = array_values($response->getArguments()[1])[0] ?? null; + $this->assertIsString($ticketId); + $ticket = $controllerMock->getTicketStore()->getTicket($ticketId); + $this->assertIsArray($ticket); + + return $ticket; + } + + /** * Check authenticated with debugMode false */ @@ -625,6 +859,10 @@ public function testGatewayPassiveEnabledPerformsPassiveAttempt(): void $this->assertTrue($actualLoginParams['isPassive']); $this->assertArrayHasKey('ReturnTo', $actualLoginParams); $this->assertIsString($actualLoginParams['ReturnTo']); + + // A passive attempt names the same entities as an interactive login + $this->assertEquals(['entityid' => 'http://localhost/'], $actualLoginParams['Source'] ?? null); + $this->assertEquals(['entityid' => $params['service']], $actualLoginParams['Destination'] ?? null); }