diff --git a/config/services/logging.yml b/config/services/logging.yml index 9229a34f98..d3b11aa71d 100644 --- a/config/services/logging.yml +++ b/config/services/logging.yml @@ -30,6 +30,12 @@ services: tags: - { name: monolog.processor } + OpenConext\EngineBlock\Logger\Processor\CorrelationIdProcessor: + arguments: + - '@OpenConext\EngineBlock\Request\CorrelationId' + tags: + - { name: monolog.processor } + OpenConext\EngineBlock\Logger\Processor\SessionIdProcessor: tags: - { name: monolog.processor } diff --git a/config/services/services.yml b/config/services/services.yml index 2af8da7cb9..e94489bc09 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -18,6 +18,7 @@ services: class: OpenConext\EngineBlockBundle\HealthCheck\DoctrineConnectionHealthCheck arguments: - '%monitor_database_health_check_query%' + - '@logger' calls: - [ setEntityManager, ['@?doctrine.orm.entity_manager']] tags: @@ -56,6 +57,15 @@ services: - '@OpenConext\EngineBlock\Request\UniqidGenerator' public: true + OpenConext\EngineBlock\Request\CorrelationId: + public: true + + OpenConext\EngineBlock\Request\CorrelationIdRepository: + public: true + arguments: + - '@OpenConext\EngineBlock\Request\CorrelationId' + - '@request_stack' + OpenConext\EngineBlockBundle\Security\Http\EntryPoint\JsonBasicAuthenticationEntryPoint: arguments: - 'engine-api.%domain%' @@ -377,3 +387,9 @@ services: class: OpenConext\EngineBlockBundle\Sbs\SbsAttributeMerger arguments: - "%sram.allowed_attributes%" + + EngineBlock_Saml2_AuthnRequestSessionRepository: + class: EngineBlock_Saml2_AuthnRequestSessionRepository + public: true + arguments: + - '@request_stack' diff --git a/library/EngineBlock/Application/DiContainer.php b/library/EngineBlock/Application/DiContainer.php index 03d4a6f3e6..8bb9a567fd 100644 --- a/library/EngineBlock/Application/DiContainer.php +++ b/library/EngineBlock/Application/DiContainer.php @@ -613,4 +613,20 @@ public function getNameIdSubstituteResolver() { return new EngineBlock_Arp_NameIdSubstituteResolver($this->container->get('engineblock.compat.logger')); } + + /** + * @return \OpenConext\EngineBlock\Request\CorrelationIdRepository + */ + public function getCorrelationIdRepository(): \OpenConext\EngineBlock\Request\CorrelationIdRepository + { + return $this->container->get(\OpenConext\EngineBlock\Request\CorrelationIdRepository::class); + } + + /** + * @return EngineBlock_Saml2_AuthnRequestSessionRepository + */ + public function getAuthnRequestSessionRepository(): EngineBlock_Saml2_AuthnRequestSessionRepository + { + return $this->container->get(EngineBlock_Saml2_AuthnRequestSessionRepository::class); + } } diff --git a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php index 751d1ddd4f..b8c7661a2c 100644 --- a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php @@ -89,6 +89,9 @@ public function serve($serviceName, Request $httpRequest) $receivedRequest = $this->_server->getReceivedRequestFromResponse($receivedResponse); $application = EngineBlock_ApplicationSingleton::getInstance(); + + $correlationIdRepository = $application->getDiContainer()->getCorrelationIdRepository(); + $correlationIdRepository->resolve($receivedResponse->getInResponseTo()); $log = $application->getLogInstance(); if(!$receivedRequest instanceof EngineBlock_Saml2_AuthnRequestAnnotationDecorator){ diff --git a/library/EngineBlock/Corto/Module/Service/ContinueToIdp.php b/library/EngineBlock/Corto/Module/Service/ContinueToIdp.php index ee3a3a0f3b..53395080fe 100644 --- a/library/EngineBlock/Corto/Module/Service/ContinueToIdp.php +++ b/library/EngineBlock/Corto/Module/Service/ContinueToIdp.php @@ -85,7 +85,9 @@ public function serve($serviceName, Request $httpRequest) ); } - $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($this->_server->getLogger()); + $authnRequestRepository = EngineBlock_ApplicationSingleton::getInstance() + ->getDiContainer() + ->getAuthnRequestSessionRepository(); $request = $authnRequestRepository->findRequestById($id); if (!$request) { @@ -94,6 +96,15 @@ public function serve($serviceName, Request $httpRequest) ); } + // Resolve here so log entries emitted during this leg (e.g. the debug + // block below) carry the correlation ID. ProxyServer::sendAuthenticationRequest + // will also call resolve() when it links the IdP request ID — that is + // idempotent and sets the same value. + $correlationIdRepository = EngineBlock_ApplicationSingleton::getInstance() + ->getDiContainer() + ->getCorrelationIdRepository(); + $correlationIdRepository->resolve($id); + // Flush log if SP or IdP has additional logging enabled if ($request->isDebugRequest()) { $sp = $this->getEngineSpRole($this->_server); diff --git a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php index dd882adcc2..48b083e627 100644 --- a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php @@ -85,6 +85,12 @@ public function serve($serviceName, Request $httpRequest) $response = $processStep->getResponse(); $request = $this->_server->getReceivedRequestFromResponse($response); + + $correlationIdRepository = EngineBlock_ApplicationSingleton::getInstance() + ->getDiContainer() + ->getCorrelationIdRepository(); + $correlationIdRepository->resolve($request->getId()); + $serviceProvider = $this->_server->getRepository()->fetchServiceProviderByEntityId($request->getIssuer()->getValue()); $destinationMetadata = EngineBlock_SamlHelper::getDestinationSpMetadata( diff --git a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php index cf8cc3d3ab..29b4923da8 100644 --- a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php @@ -100,6 +100,11 @@ public function serve($serviceName, Request $httpRequest) $receivedRequest = $this->_server->getReceivedRequestFromResponse($response); + $correlationIdRepository = EngineBlock_ApplicationSingleton::getInstance() + ->getDiContainer() + ->getCorrelationIdRepository(); + $correlationIdRepository->resolve($receivedRequest->getId()); + // update previous response with current response $this->_processingStateHelper->updateStepResponseByRequestId( $receivedRequest->getId(), diff --git a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php index a9629945ed..dd97ac045e 100644 --- a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php +++ b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php @@ -194,11 +194,11 @@ public function serve($serviceName, Request $httpRequest) } // Multiple IdPs found... - + $container = $application->getDiContainer(); // Auto-select IdP when 'feature_enable_sso_notification' is enabled and send AuthenticationRequest on success - if ($application->getDiContainer()->getFeatureConfiguration()->isEnabled("eb.enable_sso_notification")) { - $idpEntityId = $application->getDiContainer()->getSsoNotificationService()-> - handleSsoNotification($application->getDiContainer()->getSymfonyRequest()->cookies, $this->_server); + if ($container->getFeatureConfiguration()->isEnabled("eb.enable_sso_notification")) { + $idpEntityId = $container->getSsoNotificationService() + ->handleSsoNotification($container->getSymfonyRequest()->cookies, $this->_server); if (!empty($idpEntityId)) { try { @@ -214,8 +214,8 @@ public function serve($serviceName, Request $httpRequest) } // Auto-select IdP when 'wayf.rememberChoice' feature is enabled and is allowed for the current request - if (($application->getDiContainer()->getRememberChoice() === true) && !($request->getForceAuthn() || $request->isDebugRequest())) { - $cookies = $application->getDiContainer()->getSymfonyRequest()->cookies->all(); + if (($container->getRememberChoice() === true) && !($request->getForceAuthn() || $request->isDebugRequest())) { + $cookies = $container->getSymfonyRequest()->cookies->all(); if (array_key_exists('rememberchoice', $cookies)) { $remembered = json_decode($cookies['rememberchoice']); if (array_search($remembered, $candidateIDPs) !== false) { @@ -234,9 +234,13 @@ public function serve($serviceName, Request $httpRequest) return; } - $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($log); + $authnRequestRepository = $container->getAuthnRequestSessionRepository(); $authnRequestRepository->store($request); + $correlationIdRepository = $container->getCorrelationIdRepository(); + $correlationIdRepository->mint($request->getId()); + $correlationIdRepository->resolve($request->getId()); + // Show WAYF $log->info("Multiple candidate IdPs: redirecting to WAYF"); $this->_showWayf($request, $candidateIDPs); diff --git a/library/EngineBlock/Corto/ProxyServer.php b/library/EngineBlock/Corto/ProxyServer.php index 8ec67673a7..1887d39ca1 100644 --- a/library/EngineBlock/Corto/ProxyServer.php +++ b/library/EngineBlock/Corto/ProxyServer.php @@ -461,16 +461,19 @@ public function sendAuthenticationRequest( } } - $authenticationState = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer() - ->getAuthenticationStateHelper() - ->getAuthenticationState(); + $diContainer = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer(); + $authenticationState = $diContainer->getAuthenticationStateHelper()->getAuthenticationState(); $authenticationState->startAuthenticationOnBehalfOf($ebRequest->getId(), $serviceProvider); // Store the original Request - $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($this->_logger); + $authnRequestRepository = $diContainer->getAuthnRequestSessionRepository(); $authnRequestRepository->store($spRequest); $authnRequestRepository->link($ebRequest, $spRequest); + $correlationIdRepository = $diContainer->getCorrelationIdRepository(); + $correlationIdRepository->mint($spRequest->getId()); + $correlationIdRepository->link($ebRequest->getId(), $spRequest->getId()); + $correlationIdRepository->resolve($spRequest->getId()); $this->getBindingsModule()->send($ebRequest, $identityProvider); } @@ -552,10 +555,15 @@ public function sendStepupAuthenticationRequest( // Link with the original Request - $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($this->_logger); + $authnRequestRepository = $container->getAuthnRequestSessionRepository(); $authnRequestRepository->store($spRequest); $authnRequestRepository->link($ebRequest, $spRequest); + $correlationIdRepository = $container->getCorrelationIdRepository(); + $correlationIdRepository->mint($spRequest->getId()); + $correlationIdRepository->link($ebRequest->getId(), $spRequest->getId()); + $correlationIdRepository->resolve($spRequest->getId()); + $this->getBindingsModule()->send($ebRequest, $identityProvider, true); } @@ -1101,7 +1109,9 @@ public function getReceivedRequestFromResponse(EngineBlock_Saml2_ResponseAnnotat public function findRequestFromRequestId(string $requestId): ?EngineBlock_Saml2_AuthnRequestAnnotationDecorator { - $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($this->getLogger()); + $authnRequestRepository = EngineBlock_ApplicationSingleton::getInstance() + ->getDiContainer() + ->getAuthnRequestSessionRepository(); $spRequestId = $authnRequestRepository->findLinkedRequestId($requestId); if ($spRequestId === null) { diff --git a/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php b/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php index be612b395c..b136d0babf 100644 --- a/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php +++ b/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php @@ -16,81 +16,77 @@ * limitations under the License. */ +use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException; +use Symfony\Component\HttpFoundation\RequestStack; + /** * Session storage for Authentication Requests. Store AuthnRequests and link requests together. */ class EngineBlock_Saml2_AuthnRequestSessionRepository { - /** - * @var Psr\Log\LoggerInterface - */ - private $sessionLog; - - /** - * @var - */ - private $requestStorage; + private const SESSION_KEY_REQUESTS = 'SAMLRequest'; + private const SESSION_KEY_LINKS = 'SAMLRequestLinks'; /** - * @var array + * @var RequestStack */ - private $linkStorage; + private $requestStack; - /** - * @param Psr\Log\LoggerInterface $sessionLog - */ - public function __construct(Psr\Log\LoggerInterface $sessionLog) + public function __construct(RequestStack $requestStack) { - if (!isset($_SESSION['SAMLRequest'])) { - $_SESSION['SAMLRequest'] = array(); - } - $this->requestStorage = &$_SESSION['SAMLRequest']; - - if (!isset($_SESSION['SAMLRequestLinks'])) { - $_SESSION['SAMLRequestLinks'] = array(); - } - $this->linkStorage = &$_SESSION['SAMLRequestLinks']; - - $this->sessionLog = $sessionLog; + $this->requestStack = $requestStack; } /** * @param string $requestId - * @return EngineBlock_Saml2_AuthnRequestAnnotationDecorator + * @return EngineBlock_Saml2_AuthnRequestAnnotationDecorator|null */ public function findRequestById($requestId) { - if (!isset($this->requestStorage[$requestId])) { + try { + $session = $this->requestStack->getSession(); + } catch (SessionNotFoundException $e) { return null; } - return $this->requestStorage[$requestId]; + return $session->get(self::SESSION_KEY_REQUESTS, [])[$requestId] ?? null; } /** - * @param $requestId + * @param string|null $requestId * @return string|null */ public function findLinkedRequestId($requestId) { - // Check the session for a AuthnRequest with the given ID - // Expect to get back an AuthnRequest issued by EngineBlock and destined for the IdP - if (!$requestId || !isset($this->linkStorage[$requestId])) { + if (!$requestId) { return null; } - return $this->linkStorage[$requestId]; + try { + $session = $this->requestStack->getSession(); + } catch (SessionNotFoundException $e) { + return null; + } + + return $session->get(self::SESSION_KEY_LINKS, [])[$requestId] ?? null; } /** * @param EngineBlock_Saml2_AuthnRequestAnnotationDecorator $spRequest * @return $this */ - public function store( - EngineBlock_Saml2_AuthnRequestAnnotationDecorator $spRequest - ) { - // Store the original Request - $this->requestStorage[$spRequest->getId()] = $spRequest; + public function store(EngineBlock_Saml2_AuthnRequestAnnotationDecorator $spRequest) + { + try { + $session = $this->requestStack->getSession(); + } catch (SessionNotFoundException $e) { + return $this; + } + + $requests = $session->get(self::SESSION_KEY_REQUESTS, []); + $requests[$spRequest->getId()] = $spRequest; + $session->set(self::SESSION_KEY_REQUESTS, $requests); + return $this; } @@ -103,8 +99,17 @@ public function link( EngineBlock_Saml2_AuthnRequestAnnotationDecorator $fromRequest, EngineBlock_Saml2_AuthnRequestAnnotationDecorator $toRequest ) { - // Store the mapping from the new request ID to the original request ID - $this->linkStorage[$fromRequest->getId()] = $toRequest->getId(); + try { + $session = $this->requestStack->getSession(); + } catch (SessionNotFoundException $e) { + return $this; + } + + $links = $session->get(self::SESSION_KEY_LINKS, []); + $links[$fromRequest->getId()] = $toRequest->getId(); + $session->set(self::SESSION_KEY_LINKS, $links); + return $this; } + } diff --git a/src/OpenConext/EngineBlock/Logger/Processor/CorrelationIdProcessor.php b/src/OpenConext/EngineBlock/Logger/Processor/CorrelationIdProcessor.php new file mode 100644 index 0000000000..cfec648dfb --- /dev/null +++ b/src/OpenConext/EngineBlock/Logger/Processor/CorrelationIdProcessor.php @@ -0,0 +1,37 @@ +extra['correlation_id'] = $this->correlationId->get(); + + return $record; + } +} diff --git a/src/OpenConext/EngineBlock/Request/CorrelationId.php b/src/OpenConext/EngineBlock/Request/CorrelationId.php new file mode 100644 index 0000000000..7b8c2f9db3 --- /dev/null +++ b/src/OpenConext/EngineBlock/Request/CorrelationId.php @@ -0,0 +1,34 @@ +correlationId; + } + + public function set(string $correlationId): void + { + $this->correlationId = $correlationId; + } +} diff --git a/src/OpenConext/EngineBlock/Request/CorrelationIdRepository.php b/src/OpenConext/EngineBlock/Request/CorrelationIdRepository.php new file mode 100644 index 0000000000..510241d513 --- /dev/null +++ b/src/OpenConext/EngineBlock/Request/CorrelationIdRepository.php @@ -0,0 +1,115 @@ +requestStack->getSession(); + } catch (SessionNotFoundException) { + return; + } + + $ids = $session->get(self::SESSION_KEY, []); + + if (!isset($ids[$requestId])) { + $ids[$requestId] = bin2hex(random_bytes(16)); + $session->set(self::SESSION_KEY, $ids); + } + } + + /** + * Copies the correlation ID from $sourceRequestId to $targetRequestId. + * Called when EngineBlock creates its own AuthnRequest to send to the IdP, + * so that the IdP leg can be traced back to the original SP flow. + */ + public function link(string $targetRequestId, string $sourceRequestId): void + { + try { + $session = $this->requestStack->getSession(); + } catch (SessionNotFoundException) { + return; + } + + $ids = $session->get(self::SESSION_KEY, []); + + if (!array_key_exists($sourceRequestId, $ids)) { + return; + } + + $ids[$targetRequestId] = $ids[$sourceRequestId]; + $session->set(self::SESSION_KEY, $ids); + } + + /** + * Looks up the correlation ID for $requestId and pushes it into the + * CorrelationId DI service so all subsequent log entries carry it. + * No-op when $requestId is null or not found. + */ + public function resolve(?string $requestId): void + { + if ($requestId === null) { + return; + } + + try { + $session = $this->requestStack->getSession(); + } catch (SessionNotFoundException) { + return; + } + + $cid = $session->get(self::SESSION_KEY, [])[$requestId] ?? null; + + if ($cid !== null) { + $this->correlationId->set($cid); + } + } +} diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/CorrelationId.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/CorrelationId.feature new file mode 100644 index 0000000000..874c1d2953 --- /dev/null +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/CorrelationId.feature @@ -0,0 +1,67 @@ +Feature: + In order to trace a complete authentication flow across log entries + As a SURF operator + I need a single correlation_id to appear in every log record belonging to the same SAML flow + + Background: + Given an EngineBlock instance on "dev.openconext.local" + And no registered SPs + And no registered Idps + And a Service Provider named "CorrId-SP" + + # ── WAYF path ────────────────────────────────────────────────────────────── + # Two IdPs are registered, so the WAYF is shown after the initial SSO request. + # The correlation ID is minted in SingleSignOn.serve(), propagated to + # ContinueToIdp (user picks an IdP), then forwarded to the IdP request via + # link(), and finally picked up in AssertionConsumer and ProvideConsent/ + # ProcessConsent. A complete round-trip through all four HTTP legs must + # succeed without error. + Scenario: A user authenticating via the WAYF completes the full four-leg flow + Given an Identity Provider named "CorrId-IdP-A" + And an Identity Provider named "CorrId-IdP-B" + When I log in at "CorrId-SP" + And I select "CorrId-IdP-A" on the WAYF + And I pass through EngineBlock + And I pass through the IdP + And I give my consent + And I pass through EngineBlock + Then the url should match "functional-testing/CorrId-SP/acs" + + # ── Direct path (no WAYF) ─────────────────────────────────────────────────── + # When only one IdP is available the WAYF is skipped; the correlation ID is + # minted inside ProxyServer.sendAuthenticationRequest() and linked to the IdP + # request. AssertionConsumer and consent legs must resolve it from the IdP + # request ID stored in InResponseTo. + Scenario: A user authenticating without the WAYF completes the full flow + Given an Identity Provider named "CorrId-IdP-Only" + When I log in at "CorrId-SP" + And I pass through EngineBlock + And I pass through the IdP + And I give my consent + And I pass through EngineBlock + Then the url should match "functional-testing/CorrId-SP/acs" + + # ── Concurrent flows ──────────────────────────────────────────────────────── + # Two simultaneous authentications in separate browser tabs share the same PHP + # session. Each flow must mint its own correlation ID and the two IDs must + # not bleed into each other. Both flows must complete successfully and land + # on the correct SP ACS URL. + # Requires the @functional tag to use the Chrome driver (browser tabs need JS). + @functional + Scenario: Two concurrent authentication flows each complete independently + Given an Identity Provider named "CorrId-IdP-A" + And an Identity Provider named "CorrId-IdP-B" + When I open 2 browser tabs identified by "Tab-A, Tab-B" + And I switch to "Tab-A" + And I log in at "CorrId-SP" + And I select "CorrId-IdP-A" on the WAYF + And I switch to "Tab-B" + And I log in at "CorrId-SP" + And I select "CorrId-IdP-B" on the WAYF + And I pass through the IdP + And I give my consent + Then the url should match "functional-testing/CorrId-SP/acs" + And I switch to "Tab-A" + And I pass through the IdP + And I give my consent + Then the url should match "functional-testing/CorrId-SP/acs" diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php index 3b289e97d4..b4d1d221d1 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php @@ -80,16 +80,21 @@ class EngineBlock_Test_Corto_Module_Service_ProcessConsentTest extends TestCase */ private $sessionMock; + /** + * @var EngineBlock_Saml2_AuthnRequestAnnotationDecorator + */ + private $spRequest; + public function setUp(): void { $diContainer = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer(); + $this->sspResponseMock = $this->mockSspResponse(); // sets $this->spRequest first $this->proxyServerMock = $this->mockProxyServer(); $this->xmlConverterMock = $this->mockXmlConverter($diContainer->getXmlConverter()); $this->consentFactoryMock = $diContainer->getConsentFactory(); $this->authnStateHelperMock = $this->mockAuthnStateHelper(); - $this->sspResponseMock = $this->mockSspResponse(); $this->processingStateHelperMock = $this->mockProcessingStateHelper(); $this->httpRequestMock = $this->mockHttpRequest(); } @@ -184,6 +189,11 @@ private function mockProxyServer() )) ->setBindingsModule($this->mockBindingsModule()); + // Mock findRequestFromRequestId so no session lookup is needed + Phake::when($proxyServerMock) + ->findRequestFromRequestId('EBREQUEST') + ->thenReturn($this->spRequest); + return $proxyServerMock; } @@ -255,17 +265,7 @@ private function mockSspResponse() $issuer = new Issuer(); $issuer->setValue('https://sp.example.edu'); $spRequest->setIssuer($issuer); - $spRequest = new EngineBlock_Saml2_AuthnRequestAnnotationDecorator($spRequest); - - $ebRequest = new AuthnRequest(); - $ebRequest->setId('EBREQUEST'); - $ebRequest = new EngineBlock_Saml2_AuthnRequestAnnotationDecorator($ebRequest); - - $dummySessionLog = new Psr\Log\NullLogger(); - $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($dummySessionLog); - $authnRequestRepository->store($spRequest); - $authnRequestRepository->store($ebRequest); - $authnRequestRepository->link($ebRequest, $spRequest); + $this->spRequest = new EngineBlock_Saml2_AuthnRequestAnnotationDecorator($spRequest); $sspResponse = new Response(); $sspResponse->setInResponseTo('EBREQUEST'); diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php index cd494032ca..392c678367 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php @@ -184,9 +184,14 @@ private function mockProxyServer() array(new ServiceProvider('testSp')) )); - $bindingsModuleMock = $this->mockBindingsModule(); + [$bindingsModuleMock, $spRequest] = $this->mockBindingsModule(); $proxyServerMock->setBindingsModule($bindingsModuleMock); + // Mock findRequestFromRequestId so no session lookup is needed + Phake::when($proxyServerMock) + ->findRequestFromRequestId('EBREQUEST') + ->thenReturn($spRequest); + Phake::when($proxyServerMock) ->renderTemplate(Phake::anyParameters()) ->thenReturn(null); @@ -199,9 +204,9 @@ private function mockProxyServer() } /** - * @return EngineBlock_Corto_Module_Bindings + * @return array{0: EngineBlock_Corto_Module_Bindings, 1: EngineBlock_Saml2_AuthnRequestAnnotationDecorator} */ - private function mockBindingsModule() + private function mockBindingsModule(): array { $spRequest = new AuthnRequest(); $spRequest->setId('SPREQUEST'); @@ -210,16 +215,6 @@ private function mockBindingsModule() $spRequest->setIssuer($issuer); $spRequest = new EngineBlock_Saml2_AuthnRequestAnnotationDecorator($spRequest); - $ebRequest = new AuthnRequest(); - $ebRequest->setId('EBREQUEST'); - $ebRequest = new EngineBlock_Saml2_AuthnRequestAnnotationDecorator($ebRequest); - - $dummyLog = new Psr\Log\NullLogger(); - $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($dummyLog); - $authnRequestRepository->store($spRequest); - $authnRequestRepository->store($ebRequest); - $authnRequestRepository->link($ebRequest, $spRequest); - $assertion = new Assertion(); $assertion->setAttributes(array( 'urn:org:openconext:corto:internal:sp-entity-id' => array( @@ -246,7 +241,7 @@ private function mockBindingsModule() ->receiveResponse(Phake::anyParameters()) ->thenReturn($responseFixture); - return $bindingsModuleMock; + return [$bindingsModuleMock, $spRequest]; } /** diff --git a/tests/library/EngineBlock/Test/Saml2/AuthnRequestSessionRepositoryTest.php b/tests/library/EngineBlock/Test/Saml2/AuthnRequestSessionRepositoryTest.php new file mode 100644 index 0000000000..c80d4f2969 --- /dev/null +++ b/tests/library/EngineBlock/Test/Saml2/AuthnRequestSessionRepositoryTest.php @@ -0,0 +1,143 @@ +session = new Session(new MockArraySessionStorage()); + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession')->willReturn($this->session); + $this->repo = new EngineBlock_Saml2_AuthnRequestSessionRepository($requestStack); + } + + private function makeRequest(string $id): EngineBlock_Saml2_AuthnRequestAnnotationDecorator + { + $authnRequest = new AuthnRequest(); + $authnRequest->setId($id); + return new EngineBlock_Saml2_AuthnRequestAnnotationDecorator($authnRequest); + } + + public function test_store_saves_request(): void + { + $request = $this->makeRequest('_sp-request-A'); + + $this->repo->store($request); + + $this->assertSame($request, $this->repo->findRequestById('_sp-request-A')); + } + + public function test_find_request_by_id_returns_null_for_unknown_id(): void + { + $this->assertNull($this->repo->findRequestById('_unknown')); + } + + public function test_link_stores_request_mapping(): void + { + $spRequest = $this->makeRequest('_sp-request-A'); + $idpRequest = $this->makeRequest('_idp-request-B'); + + $this->repo->store($spRequest); + $this->repo->link($idpRequest, $spRequest); + + $this->assertSame('_sp-request-A', $this->repo->findLinkedRequestId('_idp-request-B')); + } + + public function test_find_linked_request_id_returns_null_for_unknown_id(): void + { + $this->assertNull($this->repo->findLinkedRequestId('_unknown')); + } + + public function test_find_linked_request_id_returns_null_for_null_input(): void + { + $this->assertNull($this->repo->findLinkedRequestId(null)); + } + + public function test_store_and_find_multiple_requests(): void + { + $req1 = $this->makeRequest('_req-1'); + $req2 = $this->makeRequest('_req-2'); + + $this->repo->store($req1); + $this->repo->store($req2); + + $this->assertSame($req1, $this->repo->findRequestById('_req-1')); + $this->assertSame($req2, $this->repo->findRequestById('_req-2')); + } + + public function test_store_is_noop_when_no_session_available(): void + { + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession') + ->willThrowException(new SessionNotFoundException()); + + $repo = new EngineBlock_Saml2_AuthnRequestSessionRepository($requestStack); + $request = $this->makeRequest('_req-A'); + + $repo->store($request); // must not throw + + $this->assertNull($repo->findRequestById('_req-A')); + } + + public function test_link_is_noop_when_no_session_available(): void + { + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession') + ->willThrowException(new SessionNotFoundException()); + + $repo = new EngineBlock_Saml2_AuthnRequestSessionRepository($requestStack); + $spRequest = $this->makeRequest('_sp-A'); + $idpRequest = $this->makeRequest('_idp-B'); + + $repo->link($idpRequest, $spRequest); // must not throw + + $this->assertNull($repo->findLinkedRequestId('_idp-B')); + } + + public function test_find_request_is_noop_when_no_session_available(): void + { + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession') + ->willThrowException(new SessionNotFoundException()); + + $repo = new EngineBlock_Saml2_AuthnRequestSessionRepository($requestStack); + + $this->assertNull($repo->findRequestById('_req-A')); + } + + public function test_find_linked_is_noop_when_no_session_available(): void + { + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession') + ->willThrowException(new SessionNotFoundException()); + + $repo = new EngineBlock_Saml2_AuthnRequestSessionRepository($requestStack); + + $this->assertNull($repo->findLinkedRequestId('_req-A')); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Logger/Processor/CorrelationIdProcessorTest.php b/tests/unit/OpenConext/EngineBlock/Logger/Processor/CorrelationIdProcessorTest.php new file mode 100644 index 0000000000..b7c6b6c4dc --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Logger/Processor/CorrelationIdProcessorTest.php @@ -0,0 +1,75 @@ +set('test-correlation-id'); + + $processor = new CorrelationIdProcessor($correlationId); + $record = new LogRecord( + datetime: new DateTimeImmutable(), + channel: 'test', + level: Level::Debug, + message: 'test message', + context: [], + extra: [], + ); + + $processedRecord = ($processor)($record); + + $this->assertEquals('test-correlation-id', $processedRecord->extra['correlation_id']); + } + + #[Group('EngineBlock')] + #[Group('Logger')] + #[Test] + public function correlation_id_is_null_when_not_set() + { + $correlationId = new CorrelationId(); + + $processor = new CorrelationIdProcessor($correlationId); + $record = new LogRecord( + datetime: new DateTimeImmutable(), + channel: 'test', + level: Level::Debug, + message: 'test message', + context: [], + extra: [], + ); + + $processedRecord = ($processor)($record); + + $this->assertNull($processedRecord->extra['correlation_id']); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Request/CorrelationIdFlowTest.php b/tests/unit/OpenConext/EngineBlock/Request/CorrelationIdFlowTest.php new file mode 100644 index 0000000000..9bc9543ed8 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Request/CorrelationIdFlowTest.php @@ -0,0 +1,153 @@ +session = new Session(new MockArraySessionStorage()); + $this->requestStack = $this->createMock(RequestStack::class); + $this->requestStack->method('getSession')->willReturn($this->session); + + $this->correlationId = new CorrelationId(); + $this->repo = new CorrelationIdRepository($this->correlationId, $this->requestStack); + } + + private function newRepo(CorrelationId $cid): CorrelationIdRepository + { + // All legs share the same session (same browser session across requests) + return new CorrelationIdRepository($cid, $this->requestStack); + } + + // ── WAYF path ──────────────────────────────────────────────────────────── + + public function test_wayf_flow_all_four_legs_share_the_same_correlation_id(): void + { + $spRequestId = '_sp-request-A'; + $idpRequestId = '_idp-request-B'; + + // Leg 1 — SSO: mint the correlation ID. + $this->repo->mint($spRequestId); + $this->repo->resolve($spRequestId); + $mintedCx = $this->correlationId->get(); + $this->assertNotNull($mintedCx, 'SSO must mint a correlation ID'); + + // Leg 2 — ContinueToIdp: resolves SP request ID A. + $freshCid = new CorrelationId(); + $this->newRepo($freshCid)->resolve($spRequestId); + $this->assertSame($mintedCx, $freshCid->get(), 'ContinueToIdp must see the same correlation ID'); + + // ProxyServer::sendAuthenticationRequest links the new IdP request ID to + // the SP request ID (happens after ContinueToIdp, before IdP response). + $this->repo->link($idpRequestId, $spRequestId); + + // Leg 3 — ACS: IdP response InResponseTo=B, resolves via B. + $freshCid2 = new CorrelationId(); + $this->newRepo($freshCid2)->resolve($idpRequestId); + $this->assertSame($mintedCx, $freshCid2->get(), 'ACS must see the same correlation ID'); + + // Leg 4 — Consent: resolves SP request ID A again. + $freshCid3 = new CorrelationId(); + $this->newRepo($freshCid3)->resolve($spRequestId); + $this->assertSame($mintedCx, $freshCid3->get(), 'Consent must see the same correlation ID'); + } + + // ── Direct path (no WAYF) ───────────────────────────────────────────────── + + public function test_direct_flow_acs_and_consent_share_the_correlation_id_minted_at_sso(): void + { + $spRequestId = '_sp-direct-A'; + $idpRequestId = '_idp-direct-B'; + + $this->repo->mint($spRequestId); + $this->repo->link($idpRequestId, $spRequestId); + $this->repo->resolve($spRequestId); + $mintedCx = $this->correlationId->get(); + $this->assertNotNull($mintedCx); + + $ids = $this->session->get('CorrelationIds'); + $this->assertSame($mintedCx, $ids[$idpRequestId], 'ACS resolves via IdP request ID'); + $this->assertSame($mintedCx, $ids[$spRequestId], 'Consent resolves via SP request ID'); + } + + // ── Concurrent flows ────────────────────────────────────────────────────── + + public function test_two_concurrent_flows_have_independent_correlation_ids(): void + { + $this->repo->mint('_sp-A1'); + $this->repo->link('_idp-B1', '_sp-A1'); + + $this->repo->mint('_sp-A2'); + $this->repo->link('_idp-B2', '_sp-A2'); + + $ids = $this->session->get('CorrelationIds'); + $cx1 = $ids['_sp-A1']; + $cx2 = $ids['_sp-A2']; + + $this->assertNotNull($cx1); + $this->assertNotNull($cx2); + $this->assertNotSame($cx1, $cx2, 'Concurrent flows must have different correlation IDs'); + $this->assertSame($cx1, $ids['_idp-B1']); + $this->assertSame($cx2, $ids['_idp-B2']); + } + + // ── Back-button replay guard ─────────────────────────────────────────────── + + public function test_replaying_an_sso_request_does_not_change_the_correlation_id(): void + { + $spRequestId = '_sp-replay-A'; + + $this->repo->mint($spRequestId); + $cx = $this->session->get('CorrelationIds')[$spRequestId]; + + $this->repo->mint($spRequestId); + + $this->assertSame($cx, $this->session->get('CorrelationIds')[$spRequestId], 'Back-button replay must not change the correlation ID'); + } + + // ── Null safety ─────────────────────────────────────────────────────────── + + public function test_unknown_request_id_does_not_set_correlation_id(): void + { + $this->repo->resolve('_unknown-id'); + $this->assertNull($this->correlationId->get(), 'Correlation ID must remain null for unknown request IDs'); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Request/CorrelationIdRepositoryTest.php b/tests/unit/OpenConext/EngineBlock/Request/CorrelationIdRepositoryTest.php new file mode 100644 index 0000000000..2428de0288 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Request/CorrelationIdRepositoryTest.php @@ -0,0 +1,154 @@ +session = new Session(new MockArraySessionStorage()); + $this->correlationId = new CorrelationId(); + + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession')->willReturn($this->session); + + $this->repo = new CorrelationIdRepository($this->correlationId, $requestStack); + } + + public function test_mint_generates_a_correlation_id_for_a_new_request_id(): void + { + $this->repo->mint('_req-A'); + + $ids = $this->session->get('CorrelationIds'); + $this->assertArrayHasKey('_req-A', $ids); + $this->assertNotEmpty($ids['_req-A']); + } + + public function test_mint_does_not_overwrite_an_existing_correlation_id(): void + { + $this->session->set('CorrelationIds', ['_req-A' => 'cx-existing']); + + $this->repo->mint('_req-A'); + + $this->assertSame('cx-existing', $this->session->get('CorrelationIds')['_req-A']); + } + + public function test_link_copies_the_correlation_id_to_the_new_request_id(): void + { + $this->session->set('CorrelationIds', ['_sp-A' => 'cx-123']); + + $this->repo->link('_idp-B', '_sp-A'); + + $this->assertSame('cx-123', $this->session->get('CorrelationIds')['_idp-B']); + } + + public function test_link_with_unknown_source_is_a_noop(): void + { + $this->repo->link('_idp-B', '_unknown'); + + $this->assertArrayNotHasKey('_idp-B', $this->session->get('CorrelationIds', [])); + } + + public function test_resolve_pushes_correlation_id_into_the_service(): void + { + $this->session->set('CorrelationIds', ['_req-A' => 'cx-expected']); + + $this->repo->resolve('_req-A'); + + $this->assertSame('cx-expected', $this->correlationId->get()); + } + + public function test_resolve_with_unknown_id_does_not_overwrite_existing_value(): void + { + $this->correlationId->set('cx-existing'); + + $this->repo->resolve('_unknown'); + + $this->assertSame('cx-existing', $this->correlationId->get()); + } + + public function test_resolve_with_null_does_not_overwrite_existing_value(): void + { + $this->correlationId->set('cx-existing'); + + $this->repo->resolve(null); + + $this->assertSame('cx-existing', $this->correlationId->get()); + } + + public function test_mint_then_link_then_resolve_via_linked_id_returns_same_value(): void + { + $this->repo->mint('_sp-A'); + $mintedCx = $this->session->get('CorrelationIds')['_sp-A']; + + $this->repo->link('_idp-B', '_sp-A'); + $this->repo->resolve('_idp-B'); + + $this->assertSame($mintedCx, $this->correlationId->get()); + } + + // ── SessionNotFoundException safety ─────────────────────────────────────── + + public function test_mint_is_noop_when_no_session_available(): void + { + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession') + ->willThrowException(new \Symfony\Component\HttpFoundation\Exception\SessionNotFoundException()); + + $repo = new CorrelationIdRepository($this->correlationId, $requestStack); + $repo->mint('_req-A'); + + $this->assertNull($this->correlationId->get()); + } + + public function test_link_is_noop_when_no_session_available(): void + { + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession') + ->willThrowException(new \Symfony\Component\HttpFoundation\Exception\SessionNotFoundException()); + + $repo = new CorrelationIdRepository($this->correlationId, $requestStack); + $repo->link('_idp-B', '_sp-A'); + + $this->assertNull($this->session->get('CorrelationIds')); + } + + public function test_resolve_is_noop_when_no_session_available(): void + { + $this->correlationId->set('cx-existing'); + + $requestStack = $this->createMock(RequestStack::class); + $requestStack->method('getSession') + ->willThrowException(new \Symfony\Component\HttpFoundation\Exception\SessionNotFoundException()); + + $repo = new CorrelationIdRepository($this->correlationId, $requestStack); + $repo->resolve('_req-A'); + + $this->assertSame('cx-existing', $this->correlationId->get()); + } +} diff --git a/tests/unit/OpenConext/EngineBlockBundle/EventListener/RedirectToFeedbackPageExceptionListenerTest.php b/tests/unit/OpenConext/EngineBlockBundle/EventListener/RedirectToFeedbackPageExceptionListenerTest.php index a7ab5b4629..c1544220d6 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/EventListener/RedirectToFeedbackPageExceptionListenerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/EventListener/RedirectToFeedbackPageExceptionListenerTest.php @@ -49,8 +49,8 @@ use Mockery; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Exception\InvalidBindingException; -use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Exception\MissingParameterException; +use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlockBridge\ErrorReporter; use OpenConext\EngineBlockBundle\EventListener\RedirectToFeedbackPageExceptionListener; use OpenConext\EngineBlockBundle\Exception\AuthenticationSessionLimitExceededException;