From 4b3ab7375c120b7e05348195be09ab9c29557fbc Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Mon, 18 May 2026 15:28:39 +0200 Subject: [PATCH] Fix Successful login being logged before login was successful Prior to this change, the logs logged `login granted` during the assertionConsumerService phase, before consent was given. This happened because the Success log was implemented as a output filter. This is no problem on its own, but, when the `run_all_manipulations_prior_to_consent` feature is enabled, the output filters are performed before consent is handled. This caused the Log line to be logged before the consent was handled. This change removes the LogLogin output filter and moves the code to a separate log fuction, to be called from the correct step in the flow. Fixes #1742 --- config/packages/ci/parameters.yml | 2 + config/services/bridge.yml | 5 + .../Corto/Filter/Command/LogLogin.php | 80 --------- library/EngineBlock/Corto/Filter/Input.php | 11 +- library/EngineBlock/Corto/Filter/Output.php | 46 ++++-- .../Corto/Module/Service/SingleSignOn.php | 2 +- .../Saml2/AuthnRequestSessionRepository.php | 4 +- .../Request/CorrelationIdRepository.php | 2 +- .../EngineBlockBridge/Logger/LoginLogger.php | 87 ++++++++++ .../Bridge/DiContainerRuntime.php | 2 + .../Bridge/EngineBlockBootstrapper.php | 3 + ...nWithAllManipulationsBeforeConsent.feature | 14 ++ .../Features/Context/LoggingContext.php | 67 +++++++- .../Test/Corto/Filter/InputFilterTest.php | 66 ++++++++ .../Test/Corto/Filter/OutputFilterTest.php | 49 ++++++ .../Test/Corto/Module/BindingsTest.php | 2 + .../EngineBlock/Xml/MetadataRendererTest.php | 2 +- .../Logger/LoginLoggerTest.php | 152 ++++++++++++++++++ .../DiContainerRuntimeTest.php | 2 + 19 files changed, 496 insertions(+), 102 deletions(-) delete mode 100644 library/EngineBlock/Corto/Filter/Command/LogLogin.php create mode 100644 src/OpenConext/EngineBlockBridge/Logger/LoginLogger.php create mode 100644 tests/library/EngineBlock/Test/Corto/Filter/InputFilterTest.php create mode 100644 tests/library/EngineBlock/Test/Corto/Filter/OutputFilterTest.php create mode 100644 tests/unit/OpenConext/EngineBlockBridge/Logger/LoginLoggerTest.php diff --git a/config/packages/ci/parameters.yml b/config/packages/ci/parameters.yml index 86a61fcf8f..b9ec835381 100644 --- a/config/packages/ci/parameters.yml +++ b/config/packages/ci/parameters.yml @@ -2,6 +2,8 @@ parameters: api.users.nameidlookup.username: nameid api.users.nameidlookup.password: secret feature_api_users_nameid_lookup: true + auth.log.attributes: + uid: 'urn:mace:dir:attribute-def:uid' encryption_keys: default: publicFile: /config/engine/engineblock.crt diff --git a/config/services/bridge.yml b/config/services/bridge.yml index ff5b232c95..088052135b 100644 --- a/config/services/bridge.yml +++ b/config/services/bridge.yml @@ -14,6 +14,11 @@ services: arguments: - '@OpenConext\EngineBlock\Logger\AuthenticationLogger' + OpenConext\EngineBlockBridge\Logger\LoginLogger: + arguments: + - '@OpenConext\EngineBlockBridge\Logger\AuthenticationLoggerAdapter' + - '%auth.log.attributes%' + OpenConext\EngineBlockBridge\Authentication\Repository\UserDirectoryAdapter: arguments: - '@OpenConext\EngineBlockBundle\Authentication\Service\UserService' diff --git a/library/EngineBlock/Corto/Filter/Command/LogLogin.php b/library/EngineBlock/Corto/Filter/Command/LogLogin.php deleted file mode 100644 index 0a34ed6bc6..0000000000 --- a/library/EngineBlock/Corto/Filter/Command/LogLogin.php +++ /dev/null @@ -1,80 +0,0 @@ -authenticationLogger = $authenticationLogger; - $this->configuredLogAttributes = $configuredLogAttributes; - } - - public function execute() - { - if (!$this->_collabPersonId) { - throw new EngineBlock_Corto_Filter_Command_Exception_PreconditionFailed( - 'Missing collabPersonId' - ); - } - - // Get the Requester chain, which starts at the oldest (farthest away from us SP) and ends with our next hop. - $requesterChain = EngineBlock_SamlHelper::getSpRequesterChain( - $this->_serviceProvider, - $this->_request, - $this->_server->getRepository() - ); - - // Remove the SP that is our next hop - array_pop($requesterChain); - - $logAttributes = []; - if (!empty($this->configuredLogAttributes)) { - foreach ($this->configuredLogAttributes as $attributeLabel => $responseAttributeKey) { - if (array_key_exists((string) $responseAttributeKey, $this->_responseAttributes)) { - $attributeValues = implode(',', $this->_responseAttributes[$responseAttributeKey]); - $logAttributes[$attributeLabel] = $attributeValues; - } - } - } - - $this->authenticationLogger->logLogin( - $this->_serviceProvider, - $this->_identityProvider, - $this->_collabPersonId, - $this->_request->getKeyId(), - $requesterChain, - $this->_response->getNameIdValue(), - $this->_response->getAssertion()->getAuthnContextClassRef(), - $this->_request->getDestination(), - $this->_request->getIDPList(), - $logAttributes - ); - } -} diff --git a/library/EngineBlock/Corto/Filter/Input.php b/library/EngineBlock/Corto/Filter/Input.php index 7c2d098219..a399f3f92b 100644 --- a/library/EngineBlock/Corto/Filter/Input.php +++ b/library/EngineBlock/Corto/Filter/Input.php @@ -16,6 +16,8 @@ * limitations under the License. */ +use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; + /** * Called by Corto before consent, right after it receives an Assertion with attributes from an Identity Provider. */ @@ -33,7 +35,7 @@ class EngineBlock_Corto_Filter_Input extends EngineBlock_Corto_Filter_Abstract public function getCommands() { $diContainer = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer(); - $featureConfiguration = $diContainer->getFeatureConfiguration(); + $featureConfiguration = $this->resolveFeatureConfiguration(); $logger = EngineBlock_ApplicationSingleton::getLog(); $blockUsersOnViolation = $featureConfiguration->isEnabled('eb.block_user_on_violation'); @@ -114,4 +116,11 @@ public function getCommands() return array_merge($commands, $outputFilter->getCommands()); } + + protected function resolveFeatureConfiguration(): FeatureConfigurationInterface + { + return EngineBlock_ApplicationSingleton::getInstance() + ->getDiContainer() + ->getFeatureConfiguration(); + } } diff --git a/library/EngineBlock/Corto/Filter/Output.php b/library/EngineBlock/Corto/Filter/Output.php index 88cb02970d..462647b045 100644 --- a/library/EngineBlock/Corto/Filter/Output.php +++ b/library/EngineBlock/Corto/Filter/Output.php @@ -18,10 +18,11 @@ use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; +use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; /** - * Commands are run before consent if the feature run_all_manipulations_prior_to_consent is turned on - * and after consent if the feature is turned off + * Output filter commands run after consent (when called via filter()) or before + * consent (when the Input filter merges them via getCommands()). */ class EngineBlock_Corto_Filter_Output extends EngineBlock_Corto_Filter_Abstract { @@ -33,20 +34,33 @@ public function filter( ServiceProvider $serviceProvider, IdentityProvider $identityProvider ) { - $featureConfiguration = EngineBlock_ApplicationSingleton::getInstance() - ->getDiContainer() - ->getFeatureConfiguration(); + if (!$this->resolveFeatureConfiguration()->isEnabled('eb.run_all_manipulations_prior_to_consent')) { + parent::filter( + $response, + $responseAttributes, + $request, + $serviceProvider, + $identityProvider + ); + } - if ($featureConfiguration->isEnabled('eb.run_all_manipulations_prior_to_consent')) { - return; + $sessionKey = $serviceProvider->entityId . '>' . $request->getId(); + $collabPersonId = $_SESSION[$sessionKey]['collabPersonId'] + ?? $response->getCollabPersonId(); + + if (!$collabPersonId) { + throw new EngineBlock_Corto_Filter_Command_Exception_PreconditionFailed('Missing collabPersonId'); } - parent::filter( + $diContainerRuntime = EngineBlock_ApplicationSingleton::getInstance()->getDiContainerRuntime(); + $diContainerRuntime->loginLogger->logLogin( $response, - $responseAttributes, $request, $serviceProvider, - $identityProvider + $identityProvider, + $this->_server->getRepository(), + $collabPersonId, + $responseAttributes, ); } @@ -54,7 +68,7 @@ public function filter( * These commands will be evaluated in order. * * A command can throw an exception and halt SSO, - * it can manipulate the response or it's attributes or it can communicate with external systems. + * it can manipulate the response or its attributes or it can communicate with external systems. * One thing it can't do is communicate with the user. * * @return array @@ -88,9 +102,13 @@ public function getCommands() // Convert all attributes to their OID format (if known) and add these. new EngineBlock_Corto_Filter_Command_DenormalizeAttributes(), - - // Log the login - new EngineBlock_Corto_Filter_Command_LogLogin($diContainer->getAuthenticationLogger(), $diContainer->getAuthLogAttributes()), ); } + + protected function resolveFeatureConfiguration(): FeatureConfigurationInterface + { + return EngineBlock_ApplicationSingleton::getInstance() + ->getDiContainer() + ->getFeatureConfiguration(); + } } diff --git a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php index a3f74b76f4..5a9b102505 100644 --- a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php +++ b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php @@ -378,7 +378,7 @@ protected function _createUnsolicitedRequest() $request->setKeyId($keyid); } if ($destination) { - // Set for logging purposes (LogLogin Command) note that only the REQUEST_URI (no hostname + protocol) + // Set for logging purposes, note that only the REQUEST_URI (no hostname + protocol) $sspRequest->setDestination($destination); } diff --git a/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php b/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php index 98728df6d6..2b66d92075 100644 --- a/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php +++ b/library/EngineBlock/Saml2/AuthnRequestSessionRepository.php @@ -30,8 +30,8 @@ */ class EngineBlock_Saml2_AuthnRequestSessionRepository { - private const SESSION_KEY_REQUESTS = 'SAMLRequest'; - private const SESSION_KEY_LINKS = 'SAMLRequestLinks'; + private const string SESSION_KEY_REQUESTS = 'SAMLRequest'; + private const string SESSION_KEY_LINKS = 'SAMLRequestLinks'; /** * @var RequestStack diff --git a/src/OpenConext/EngineBlock/Request/CorrelationIdRepository.php b/src/OpenConext/EngineBlock/Request/CorrelationIdRepository.php index a0148459e7..26b16403ef 100644 --- a/src/OpenConext/EngineBlock/Request/CorrelationIdRepository.php +++ b/src/OpenConext/EngineBlock/Request/CorrelationIdRepository.php @@ -23,7 +23,7 @@ final class CorrelationIdRepository { - private const SESSION_KEY = 'CorrelationIds'; + private const string SESSION_KEY = 'CorrelationIds'; public function __construct(private readonly RequestStack $requestStack) { diff --git a/src/OpenConext/EngineBlockBridge/Logger/LoginLogger.php b/src/OpenConext/EngineBlockBridge/Logger/LoginLogger.php new file mode 100644 index 0000000000..d48603ea31 --- /dev/null +++ b/src/OpenConext/EngineBlockBridge/Logger/LoginLogger.php @@ -0,0 +1,87 @@ +authenticationLogger = $authenticationLogger; + $this->configuredLogAttributes = $configuredLogAttributes; + } + + /** + * Log a successful login. + * + * @param string $collabPersonId Resolved collabPersonId (from response or session) + * @param array $responseAttributes Final response attributes after all filter commands + */ + public function logLogin( + EngineBlock_Saml2_ResponseAnnotationDecorator $response, + EngineBlock_Saml2_AuthnRequestAnnotationDecorator $request, + ServiceProvider $serviceProvider, + IdentityProvider $identityProvider, + MetadataRepositoryInterface $repository, + string $collabPersonId, + array $responseAttributes, + ): void { + // Get the Requester chain, which starts at the oldest (farthest away from us SP) and ends with our next hop. + $requesterChain = EngineBlock_SamlHelper::getSpRequesterChain( + $serviceProvider, + $request, + $repository + ); + + // Remove the SP that is our next hop + array_pop($requesterChain); + + $logAttributes = []; + if (!empty($this->configuredLogAttributes)) { + foreach ($this->configuredLogAttributes as $attributeLabel => $responseAttributeKey) { + if (array_key_exists((string) $responseAttributeKey, $responseAttributes)) { + $attributeValues = implode(',', $responseAttributes[$responseAttributeKey]); + $logAttributes[$attributeLabel] = $attributeValues; + } + } + } + + $this->authenticationLogger->logLogin( + $serviceProvider, + $identityProvider, + $collabPersonId, + $request->getKeyId(), + $requesterChain, + $response->getNameIdValue(), + $response->getAssertion()->getAuthnContextClassRef(), + $request->getDestination(), + $request->getIDPList(), + $logAttributes + ); + } +} diff --git a/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php b/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php index a53829f036..599e517aaf 100644 --- a/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php +++ b/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php @@ -22,6 +22,7 @@ use OpenConext\EngineBlock\Request\CurrentCorrelationId; use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; +use OpenConext\EngineBlockBridge\Logger\LoginLogger; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use Twig\Environment; @@ -41,6 +42,7 @@ public function __construct( public CurrentCorrelationId $currentCorrelationId, public FeedbackStateHelperInterface $feedbackStateHelper, public FeedbackInfoCollectorInterface $feedbackInfoCollector, + public LoginLogger $loginLogger, private array $preferredIdpEntityIds = [], ) { } diff --git a/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php b/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php index 6bb5f4a761..f4be71278f 100644 --- a/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php +++ b/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php @@ -23,6 +23,7 @@ use OpenConext\EngineBlock\Request\CurrentCorrelationId; use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; +use OpenConext\EngineBlockBridge\Logger\LoginLogger; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpKernel\KernelEvents; @@ -39,6 +40,7 @@ public function __construct( CurrentCorrelationId $currentCorrelationId, FeedbackStateHelperInterface $feedbackStateHelper, FeedbackInfoCollectorInterface $feedbackInfoCollector, + LoginLogger $loginLogger, array $preferredIdpEntityIds = [], ) { $this->diContainerRuntime = new DiContainerRuntime( @@ -48,6 +50,7 @@ public function __construct( $currentCorrelationId, $feedbackStateHelper, $feedbackInfoCollector, + $loginLogger, $preferredIdpEntityIds, ); } diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/AttributeManipulationWithAllManipulationsBeforeConsent.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/AttributeManipulationWithAllManipulationsBeforeConsent.feature index ef73462669..886aee98f6 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/AttributeManipulationWithAllManipulationsBeforeConsent.feature +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/AttributeManipulationWithAllManipulationsBeforeConsent.feature @@ -156,5 +156,19 @@ Feature: And the response should not match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" and text()="NOOT"]' And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"]' + Scenario: The login audit log records the post-manipulation attribute value + Given SP "SP-with-Attribute-Manipulations" has the following Attribute Manipulation: + """ + $attributes['urn:mace:dir:attribute-def:uid'] = array("manipulated-uid-value"); + """ + When I log in at "SP-with-Attribute-Manipulations" + And I select "Dummy-IdP" 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/SP-with-Attribute-Manipulations/acs" + And the login grant log should contain response attribute "uid" with value "manipulated-uid-value" + # # Scenario: Sp and IdP attribute manipulations diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/LoggingContext.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/LoggingContext.php index c723eb2876..92343716f9 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/LoggingContext.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/LoggingContext.php @@ -89,6 +89,34 @@ public function theFollowingLogMessagesShouldHaveACorrelationId(TableNode $table } } + /** + * @Then the log message :message should appear after log message :anchor + */ + public function theLogMessageShouldAppearAfterLogMessage(string $message, string $anchor): void + { + $records = $this->readRecords(); + $messages = array_column($records, 'message'); + + $anchorPos = array_search($anchor, $messages, true); + $messagePos = array_search($message, $messages, true); + + if ($anchorPos === false) { + throw new RuntimeException(sprintf('Anchor log message "%s" not found in log.', $anchor)); + } + if ($messagePos === false) { + throw new RuntimeException(sprintf('Log message "%s" not found in log.', $message)); + } + if ($messagePos <= $anchorPos) { + throw new RuntimeException(sprintf( + 'Expected log message "%s" (pos %d) to appear after "%s" (pos %d), but it appeared before or at the same position.', + $message, + $messagePos, + $anchor, + $anchorPos, + )); + } + } + /** * @Then the log should contain multiple distinct request_ids */ @@ -133,8 +161,43 @@ public function iDumpTheLogRecords(): void } /** - * Reads all records from the log file, decodes each JSON line, and returns only - * records not belonging to the event channel (Symfony kernel internals). + * @Then the login grant log should contain response attribute :label with value :value + */ + public function theLoginGrantLogShouldContainResponseAttribute(string $label, string $value): void + { + $records = $this->readRecords(); + + $loginGranted = array_filter( + $records, + static fn(array $r) => ($r['message'] ?? '') === 'login granted', + ); + + if (empty($loginGranted)) { + throw new RuntimeException('No "login granted" log record found.'); + } + + $record = reset($loginGranted); + $responseAttributes = $record['context']['response_attributes'] ?? []; + + if (!array_key_exists($label, $responseAttributes)) { + throw new RuntimeException(sprintf( + 'Login grant log has no response_attribute "%s". Available: %s', + $label, + implode(', ', array_keys($responseAttributes)), + )); + } + + if ($responseAttributes[$label] !== $value) { + throw new RuntimeException(sprintf( + 'Login grant log response_attribute "%s" expected "%s" but got "%s".', + $label, + $value, + $responseAttributes[$label], + )); + } + } + + /** * * @return array> */ diff --git a/tests/library/EngineBlock/Test/Corto/Filter/InputFilterTest.php b/tests/library/EngineBlock/Test/Corto/Filter/InputFilterTest.php new file mode 100644 index 0000000000..b1581832ad --- /dev/null +++ b/tests/library/EngineBlock/Test/Corto/Filter/InputFilterTest.php @@ -0,0 +1,66 @@ + $featureEnabled, + 'eb.block_user_on_violation' => true, + ]); + + return new class ($server, $featureConfig) extends EngineBlock_Corto_Filter_Input { + private FeatureConfigurationInterface $featureConfig; + + public function __construct(EngineBlock_Corto_ProxyServer $server, FeatureConfigurationInterface $featureConfig) + { + parent::__construct($server); + $this->featureConfig = $featureConfig; + } + + protected function resolveFeatureConfiguration(): FeatureConfigurationInterface + { + return $this->featureConfig; + } + }; + } + + public function testGetCommandsFeatureEnabledReturnsMoreCommandsThanFeatureDisabled(): void + { + $featureOff = $this->buildFilter(false)->getCommands(); + $featureOn = $this->buildFilter(true)->getCommands(); + + self::assertGreaterThan( + count($featureOff), + count($featureOn), + 'Feature-enabled must add output commands to the input filter' + ); + } +} diff --git a/tests/library/EngineBlock/Test/Corto/Filter/OutputFilterTest.php b/tests/library/EngineBlock/Test/Corto/Filter/OutputFilterTest.php new file mode 100644 index 0000000000..19d92ec4fe --- /dev/null +++ b/tests/library/EngineBlock/Test/Corto/Filter/OutputFilterTest.php @@ -0,0 +1,49 @@ +getCommands(); + + self::assertNotEmpty($commands, 'Output filter must have at least one command'); + } + + public function testGetCommandsContainsOnlyFilterCommands(): void + { + $server = Phake::mock(EngineBlock_Corto_ProxyServer::class); + $filter = new EngineBlock_Corto_Filter_Output($server); + $commands = $filter->getCommands(); + + foreach ($commands as $command) { + self::assertInstanceOf( + EngineBlock_Corto_Filter_Command_Abstract::class, + $command, + 'All commands must extend the abstract filter command' + ); + } + } +} diff --git a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php index e0e583a64d..5b8ef858cf 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php @@ -24,6 +24,7 @@ use OpenConext\EngineBlock\Request\CurrentCorrelationId; use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; +use OpenConext\EngineBlockBridge\Logger\LoginLogger; use OpenConext\EngineBlockBundle\Bridge\DiContainerRuntime; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use PHPUnit\Framework\TestCase; @@ -71,6 +72,7 @@ public function setUp(): void new CurrentCorrelationId(), $this->createStub(FeedbackStateHelperInterface::class), $this->createStub(FeedbackInfoCollectorInterface::class), + $this->createStub(LoginLogger::class), )); $this->bindings = new EngineBlock_Corto_Module_Bindings($this->proxyServer); diff --git a/tests/unit/OpenConext/EngineBlock/Xml/MetadataRendererTest.php b/tests/unit/OpenConext/EngineBlock/Xml/MetadataRendererTest.php index 24a37b8195..c2055403e6 100644 --- a/tests/unit/OpenConext/EngineBlock/Xml/MetadataRendererTest.php +++ b/tests/unit/OpenConext/EngineBlock/Xml/MetadataRendererTest.php @@ -18,7 +18,6 @@ namespace OpenConext\EngineBlock\Xml; use DOMDocument; -use OpenConext\EngineBlock\Saml2\IdGenerator as SamlIdGenerator; use Exception; use InvalidArgumentException; use Mockery as m; @@ -40,6 +39,7 @@ use OpenConext\EngineBlock\Metadata\X509\X509Certificate; use OpenConext\EngineBlock\Metadata\X509\X509KeyPair; use OpenConext\EngineBlock\Metadata\X509\X509PrivateKey; +use OpenConext\EngineBlock\Saml2\IdGenerator as SamlIdGenerator; use OpenConext\EngineBlock\Service\TimeProvider\TimeProvider; use OpenConext\EngineBlockBundle\Localization\LanguageSupportProvider; use OpenConext\EngineBlockBundle\Twig\Extensions\Extension\Spaceless; diff --git a/tests/unit/OpenConext/EngineBlockBridge/Logger/LoginLoggerTest.php b/tests/unit/OpenConext/EngineBlockBridge/Logger/LoginLoggerTest.php new file mode 100644 index 0000000000..683253066a --- /dev/null +++ b/tests/unit/OpenConext/EngineBlockBridge/Logger/LoginLoggerTest.php @@ -0,0 +1,152 @@ +shouldReceive('logLogin')->once()->withArgs(function ( + ServiceProvider $sp, + IdentityProvider $idp, + string $collabPersonId, + ?string $keyId, + array $requesterChain, + string $nameId, + ?string $authnContext, + ?string $destination, + ?array $idpList, + array $logAttributes + ) { + return $collabPersonId === 'urn:collab:person:example.com:admin' + && $nameId === 'some-name-id' + && $logAttributes === ['email' => 'admin@example.com']; + }); + + $configuredLogAttributes = ['email' => 'urn:mace:dir:attribute-def:mail']; + + $logger = new LoginLogger($authLogger, $configuredLogAttributes); + + $response = $this->createResponse('some-name-id'); + $request = $this->createRequest(); + $sp = new ServiceProvider('https://sp.example.com'); + $idp = new IdentityProvider('https://idp.example.com'); + $repository = Mockery::mock(MetadataRepositoryInterface::class); + $repository->shouldReceive('fetchServiceProviderByEntityId')->andReturn($sp); + + $responseAttributes = [ + 'urn:mace:dir:attribute-def:mail' => ['admin@example.com'], + ]; + + $logger->logLogin( + $response, + $request, + $sp, + $idp, + $repository, + 'urn:collab:person:example.com:admin', + $responseAttributes, + ); + } + + public function testLogLoginFiltersOnlyConfiguredAttributes(): void + { + $authLogger = Mockery::mock(AuthenticationLoggerAdapter::class); + $authLogger->shouldReceive('logLogin')->once()->withArgs(function ( + ServiceProvider $sp, + IdentityProvider $idp, + string $collabPersonId, + ?string $keyId, + array $requesterChain, + string $nameId, + ?string $authnContext, + ?string $destination, + ?array $idpList, + array $logAttributes + ) { + // Only 'uid' should be in logAttributes (mail is not in responseAttributes) + return $logAttributes === ['uid' => 'admin']; + }); + + $configuredLogAttributes = [ + 'uid' => 'urn:mace:dir:attribute-def:uid', + 'email' => 'urn:mace:dir:attribute-def:mail', + ]; + + $logger = new LoginLogger($authLogger, $configuredLogAttributes); + + $response = $this->createResponse('name-id'); + $request = $this->createRequest(); + $sp = new ServiceProvider('https://sp.example.com'); + $idp = new IdentityProvider('https://idp.example.com'); + $repository = Mockery::mock(MetadataRepositoryInterface::class); + $repository->shouldReceive('fetchServiceProviderByEntityId')->andReturn($sp); + + $responseAttributes = [ + 'urn:mace:dir:attribute-def:uid' => ['admin'], + ]; + + $logger->logLogin( + $response, + $request, + $sp, + $idp, + $repository, + 'urn:collab:person:example.com:admin', + $responseAttributes, + ); + } + + private function createResponse(string $nameId = ''): EngineBlock_Saml2_ResponseAnnotationDecorator + { + $assertion = new Assertion(); + $assertion->setAuthnContextClassRef('urn:oasis:names:tc:SAML:2.0:ac:classes:Password'); + if ($nameId !== '') { + $nameIdObj = new NameID(); + $nameIdObj->setValue($nameId); + $assertion->setNameId($nameIdObj); + } + + $samlResponse = new SAMLResponse(); + $samlResponse->setAssertions([$assertion]); + + return new EngineBlock_Saml2_ResponseAnnotationDecorator($samlResponse); + } + + private function createRequest(): EngineBlock_Saml2_AuthnRequestAnnotationDecorator + { + return new EngineBlock_Saml2_AuthnRequestAnnotationDecorator(new AuthnRequest()); + } +} diff --git a/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php b/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php index 51207a08fa..13d9f5f314 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php @@ -22,6 +22,7 @@ use OpenConext\EngineBlock\Request\CurrentCorrelationId; use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; +use OpenConext\EngineBlockBridge\Logger\LoginLogger; use OpenConext\EngineBlockBundle\Bridge\DiContainerRuntime; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use PHPUnit\Framework\TestCase; @@ -57,6 +58,7 @@ private function runtimeFactory(array $entityIds): DiContainerRuntime new CurrentCorrelationId(), $this->createStub(FeedbackStateHelperInterface::class), $this->createStub(FeedbackInfoCollectorInterface::class), + $this->createStub(LoginLogger::class), $entityIds, ); }