diff --git a/.github/workflows/test-integration.yml b/.github/workflows/test-integration.yml index 1f4c12ce3..fc42e4855 100644 --- a/.github/workflows/test-integration.yml +++ b/.github/workflows/test-integration.yml @@ -82,6 +82,10 @@ jobs: if: always() run: | cd docker && docker compose exec -T --user www-data engine.dev.openconext.local bash -c ' + echo -e "\nContainer lint\n" && \ + bin/console lint:container --env=ci && \ + echo -e "\nYAML lint\n" && \ + bin/console lint:yaml config/ && \ echo -e "\nTwig lint\n" && \ bin/console lint:twig theme/ && \ cd theme && \ diff --git a/ci/qa/lint.sh b/ci/qa/lint.sh index 16eac7e72..5ab518a22 100755 --- a/ci/qa/lint.sh +++ b/ci/qa/lint.sh @@ -3,6 +3,12 @@ set -e cd $(dirname $0)/../../ +echo -e "\nContainer lint\n" +bin/console lint:container --env=ci + +echo -e "\nYAML lint\n" +bin/console lint:yaml config/ + echo -e "\nTwig lint\n" bin/console lint:twig theme/ diff --git a/config/services/bridge.yml b/config/services/bridge.yml index f1847dc5a..ff5b232c9 100644 --- a/config/services/bridge.yml +++ b/config/services/bridge.yml @@ -7,6 +7,8 @@ services: - '@engineblock.compat.application' - '@logger' - '@request_stack' + - '@OpenConext\EngineBlock\Service\FeedbackStateHelper' + - '@OpenConext\EngineBlock\Service\FeedbackInfoCollector' OpenConext\EngineBlockBridge\Logger\AuthenticationLoggerAdapter: arguments: diff --git a/config/services/ci/controllers.yml b/config/services/ci/controllers.yml index 2113eabfc..0c0b7b5cc 100644 --- a/config/services/ci/controllers.yml +++ b/config/services/ci/controllers.yml @@ -24,9 +24,8 @@ services: engineblock.functional_test.controller.feedback: class: OpenConext\EngineBlockFunctionalTestingBundle\Controllers\FeedbackController arguments: - - '@translator' - '@twig' - - '@engineblock.compat.logger' + - '@OpenConext\EngineBlock\Service\FeedbackStateHelper' engineblock.functional_test.controller.consent: class: OpenConext\EngineBlockFunctionalTestingBundle\Controllers\ConsentController diff --git a/config/services/controllers/authentication.yml b/config/services/controllers/authentication.yml index 80a86dfbb..e2397e9d9 100644 --- a/config/services/controllers/authentication.yml +++ b/config/services/controllers/authentication.yml @@ -32,7 +32,7 @@ services: arguments: - '@translator' - '@twig' - - '@engineblock.compat.logger' + - '@OpenConext\EngineBlock\Service\FeedbackStateHelper' OpenConext\EngineBlockBundle\Controller\MetadataController: arguments: diff --git a/config/services/services.yml b/config/services/services.yml index d7613ec3f..74643fcb8 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -80,6 +80,24 @@ services: arguments: - '@request_stack' + OpenConext\EngineBlock\Service\FeedbackStateHelper: + arguments: + - '@request_stack' + + OpenConext\EngineBlock\Service\FeedbackStateHelperInterface: + alias: OpenConext\EngineBlock\Service\FeedbackStateHelper + + OpenConext\EngineBlock\Service\FeedbackInfoCollector: + arguments: + - '@request_stack' + - '@OpenConext\EngineBlock\Request\RequestId' + - '@OpenConext\EngineBlock\Metadata\MetadataRepository\CachedDoctrineMetadataRepository' + - '@OpenConext\EngineBlockBundle\Localization\LocaleProvider' + - '@OpenConext\EngineBlock\Service\FeedbackStateHelper' + + OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface: + alias: OpenConext\EngineBlock\Service\FeedbackInfoCollector + OpenConext\EngineBlock\Stepup\StepupGatewayCallOutHelper: arguments: - '@OpenConext\EngineBlock\Stepup\StepupGatewayLoaMapping' @@ -330,6 +348,7 @@ services: - '@OpenConext\EngineBlockBundle\Configuration\ErrorFeedbackConfiguration' - '@engineblock.compat.repository.metadata' - '@OpenConext\EngineBlockBundle\Authentication\Service\SamlResponseHelper' + - '@OpenConext\EngineBlock\Service\FeedbackStateHelper' OpenConext\EngineBlockBundle\Twig\Extensions\Extension\GlobalSiteNotice: autoconfigure: true diff --git a/library/EngineBlock/ApplicationSingleton.php b/library/EngineBlock/ApplicationSingleton.php index 8eca5e7ff..bd8024bc8 100644 --- a/library/EngineBlock/ApplicationSingleton.php +++ b/library/EngineBlock/ApplicationSingleton.php @@ -17,10 +17,8 @@ */ use OpenConext\EngineBlock\Logger\Handler\FingersCrossed\ManualOrDecoratedActivationStrategy; -use OpenConext\EngineBlock\Metadata\MetadataRepository\EntityNotFoundException; use OpenConext\EngineBlock\Request\RequestId; use OpenConext\EngineBlockBundle\Bridge\DiContainerRuntime; -use OpenConext\EngineBlockBundle\Exception\Art; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException; @@ -238,7 +236,10 @@ public function reportError(Throwable $exception, $messageSuffix = '') // Store some valuable debug info in session so it can be displayed on feedback pages if($this->hasSession()) { // In CLI context, the session is not available - $this->getSession()->set('feedbackInfo', $this->collectFeedbackInfo($exception)); + $feedbackHelper = $this->getDiContainerRuntime()->feedbackStateHelper; + $feedbackHelper->storeFeedbackInfo( + $this->getDiContainerRuntime()->feedbackInfoCollector->collect($exception) + ); } // flush all messages in queue, something went wrong! @@ -247,51 +248,6 @@ public function reportError(Throwable $exception, $messageSuffix = '') return true; } - /** - * @param Exception $exception - * @return array - */ - public function collectFeedbackInfo(Throwable $exception) - { - $logRequestId = $this->getLogRequestId(); - if ($logRequestId === null) { - $logRequestId = 'N/A - application not yet bootstrapped'; - } else { - $logRequestId = $logRequestId->get(); - } - - $request = $this->getDiContainer()->getSymfonyRequest(); - - $feedbackInfo = array(); - $feedbackInfo['datetime'] = (new DateTime())->format(DateTimeInterface::ATOM); - $feedbackInfo['requestUrl'] = sprintf('%s%s', $request->getSchemeAndHttpHost(), $request->getPathInfo()); - $feedbackInfo['requestId'] = $logRequestId; - $feedbackInfo['ipAddress'] = $this->getClientIpAddress(); - $feedbackInfo['artCode'] = Art::forException($exception); - - // @todo reset this when login is succesful - // Find the current identity provider - $spEntityId = $_SESSION['originalServiceProvider'] ?? $_SESSION['currentServiceProvider'] ?? null; - if ($spEntityId !== null) { - $feedbackInfo['serviceProvider'] = $spEntityId; - $feedbackInfo['serviceProviderName'] = $this->getServiceProviderName($spEntityId); - } - - if (isset($_SESSION['proxyServiceProvider'])) { - $feedbackInfo['proxyServiceProvider'] = $_SESSION['proxyServiceProvider']; - } - - // @todo reset this when login is succesful - // Find the current identity provider - if (isset($_SESSION['currentIdentityProvider'])) { - $idpEntityId = $_SESSION['currentIdentityProvider']; - $feedbackInfo['identityProvider'] = $idpEntityId; - $feedbackInfo['identityProviderName'] = $this->getidentityProviderName($idpEntityId); - } - - return $feedbackInfo; - } - /** * Get the IP address for the HTTP client (optionally taking into account proxies). * @@ -441,26 +397,4 @@ public function getDiContainer() return $this->_diContainer; } - /** - * @param string $serviceProviderId - * @return string - */ - private function getServiceProviderName(string $serviceProviderId){ - try { - $serviceProvider = $this->getDiContainer()->getMetadataRepository()->fetchServiceProviderByEntityId($serviceProviderId); - return $serviceProvider->getDisplayName($this->getDiContainer()->getLocaleProvider()->getLocale()); - } catch (EntityNotFoundException $e) {} - - return ''; - } - - private function getIdentityProviderName(string $identityProviderId): string - { - try { - $identityProvider = $this->getDiContainer()->getMetadataRepository()->fetchIdentityProviderByEntityId($identityProviderId); - return $identityProvider->getDisplayName($this->getDiContainer()->getLocaleProvider()->getLocale()); - } catch (EntityNotFoundException $e) {} - - return ''; - } } diff --git a/library/EngineBlock/Corto/Module/Bindings.php b/library/EngineBlock/Corto/Module/Bindings.php index 82d131e46..82d150bfa 100644 --- a/library/EngineBlock/Corto/Module/Bindings.php +++ b/library/EngineBlock/Corto/Module/Bindings.php @@ -180,8 +180,11 @@ public function receiveRequest() 'Missing in message delivered to AssertionConsumerService.' ); } - // Remember sp for debugging - $_SESSION['currentServiceProvider'] = $ebRequest->getIssuer()->getValue(); + + EngineBlock_ApplicationSingleton::getInstance()->getDiContainerRuntime()->feedbackStateHelper->startNewFlow( + $ebRequest->getId(), + $ebRequest->getIssuer()->getValue() + ); // Verify that we know this SP and have metadata for it. $serviceProvider = $this->_verifyKnownSP($spEntityId->getValue()); @@ -324,7 +327,7 @@ public function receiveResponse($serviceEntityId, $expectedDestination) } // Remember idp for debugging - $_SESSION['currentIdentityProvider'] = $idpEntityId; + EngineBlock_ApplicationSingleton::getInstance()->getDiContainerRuntime()->feedbackStateHelper->setCurrentIdentityProvider($idpEntityId->getValue()); // Verify that we know this IdP and have metadata for it. $cortoIdpMetadata = $this->_verifyKnownIdP( diff --git a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php index 751d1ddd4..f9a1c9a11 100644 --- a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php @@ -19,6 +19,7 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\Factory\Factory\ServiceProviderFactory; use OpenConext\EngineBlock\Metadata\X509\KeyPairFactory; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface; use OpenConext\EngineBlock\Stepup\StepupGatewayCallOutHelper; use OpenConext\EngineBlockBundle\Authentication\AuthenticationState; @@ -31,33 +32,19 @@ class EngineBlock_Corto_Module_Service_AssertionConsumer implements EngineBlock_Corto_Module_Service_ServiceInterface { - /** - * @var EngineBlock_Corto_ProxyServer - */ - private $_server; + private EngineBlock_Corto_ProxyServer $_server; - /** - * @var EngineBlock_Corto_XmlToArray - */ - private $_xmlConverter; + private EngineBlock_Corto_XmlToArray $_xmlConverter; - /** - * @var Session - */ - private $_session; + private Session $_session; - /** - * @var ProcessingStateHelperInterface - */ - private $_processingStateHelper; - /** - * @var StepupGatewayCallOutHelper - */ - private $_stepupGatewayCallOutHelper; - /** - * @var ServiceProviderFactory - */ - private $_serviceProviderFactory; + private ProcessingStateHelperInterface $_processingStateHelper; + + private StepupGatewayCallOutHelper $_stepupGatewayCallOutHelper; + + private ServiceProviderFactory $_serviceProviderFactory; + + private FeedbackStateHelperInterface $_feedbackStateHelper; public function __construct( EngineBlock_Corto_ProxyServer $server, @@ -65,7 +52,8 @@ public function __construct( Session $session, ProcessingStateHelperInterface $processingStateHelper, StepupGatewayCallOutHelper $stepupGatewayCallOutHelper, - ServiceProviderFactory $serviceProviderFactory + ServiceProviderFactory $serviceProviderFactory, + FeedbackStateHelperInterface $feedbackStateHelper ) { $this->_server = $server; @@ -74,6 +62,7 @@ public function __construct( $this->_processingStateHelper = $processingStateHelper; $this->_stepupGatewayCallOutHelper = $stepupGatewayCallOutHelper; $this->_serviceProviderFactory = $serviceProviderFactory; + $this->_feedbackStateHelper = $feedbackStateHelper; } /** @@ -126,8 +115,7 @@ public function serve($serviceName, Request $httpRequest) if ($sp->getCoins()->isTrustedProxy()) { $proxySp = $sp; $sp = $this->_server->findOriginalServiceProvider($receivedRequest, $log); - $_SESSION['originalServiceProvider'] = $sp->entityId; - $_SESSION['proxyServiceProvider'] = $proxySp->entityId; + $this->_feedbackStateHelper->setProxyContext($sp->entityId, $proxySp->entityId); } // Verify the SP requester chain. diff --git a/library/EngineBlock/Corto/Module/Service/ProcessedAssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/ProcessedAssertionConsumer.php index 4837c80c0..1c111c368 100644 --- a/library/EngineBlock/Corto/Module/Service/ProcessedAssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/ProcessedAssertionConsumer.php @@ -16,6 +16,7 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface; use Symfony\Component\HttpFoundation\Request; @@ -31,12 +32,19 @@ class EngineBlock_Corto_Module_Service_ProcessedAssertionConsumer implements Eng */ private $_processingStateHelper; + /** + * @var FeedbackStateHelperInterface + */ + private $_feedbackStateHelper; + public function __construct( EngineBlock_Corto_ProxyServer $server, - ProcessingStateHelperInterface $processingStateHelper + ProcessingStateHelperInterface $processingStateHelper, + FeedbackStateHelperInterface $feedbackStateHelper ) { $this->_server = $server; $this->_processingStateHelper = $processingStateHelper; + $this->_feedbackStateHelper = $feedbackStateHelper; } /** @@ -84,6 +92,8 @@ public function serve($serviceName, Request $httpRequest) $sentResponse = $this->_server->createEnhancedResponse($receivedRequest, $response); $this->_server->sendResponseToRequestIssuer($receivedRequest, $sentResponse); + + $this->_feedbackStateHelper->clearFlowContext(); return; } } diff --git a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php index 3fa431c70..1d38294cd 100644 --- a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php +++ b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php @@ -186,8 +186,7 @@ public function serve($serviceName, Request $httpRequest) // When a trusted proxy is used, the originalServiceProvider is set to the entityId of the original issuing // SP. This prevents the display of the Proxy in the feedback information. if (isset($proxySp) && $proxySp->getCoins()->isTrustedProxy()) { - $_SESSION['originalServiceProvider'] = $sp->entityId; - $_SESSION['proxyServiceProvider'] = $proxySp->entityId; + EngineBlock_ApplicationSingleton::getInstance()->getDiContainerRuntime()->feedbackStateHelper->setProxyContext($sp->entityId, $proxySp->entityId); } throw new EngineBlock_Corto_Module_Service_SingleSignOn_NoIdpsException('No candidate IdPs found'); } diff --git a/library/EngineBlock/Corto/Module/Services.php b/library/EngineBlock/Corto/Module/Services.php index 7b15a3b6c..79324bae0 100644 --- a/library/EngineBlock/Corto/Module/Services.php +++ b/library/EngineBlock/Corto/Module/Services.php @@ -110,12 +110,14 @@ private function factoryService($className, EngineBlock_Corto_ProxyServer $serve $diContainer->getSession(), $diContainer->getProcessingStateHelper(), $diContainer->getStepupGatewayCallOutHelper(), - $diContainer->getServiceProviderFactory() + $diContainer->getServiceProviderFactory(), + $diContainerRuntime->feedbackStateHelper ); case EngineBlock_Corto_Module_Service_ProcessedAssertionConsumer::class : return new EngineBlock_Corto_Module_Service_ProcessedAssertionConsumer( $server, - $diContainer->getProcessingStateHelper() + $diContainer->getProcessingStateHelper(), + $diContainerRuntime->feedbackStateHelper ); case EngineBlock_Corto_Module_Service_StepupAssertionConsumer::class : return new EngineBlock_Corto_Module_Service_StepupAssertionConsumer( diff --git a/library/EngineBlock/SamlHelper.php b/library/EngineBlock/SamlHelper.php index e5a7483e8..9548f9e65 100644 --- a/library/EngineBlock/SamlHelper.php +++ b/library/EngineBlock/SamlHelper.php @@ -120,8 +120,7 @@ public static function findRequesterServiceProvider( // The filters will log any additional information that is available. $lastRequesterEntity = $repository->findServiceProviderByEntityId($lastRequesterEntityId, $logger); if (!$lastRequesterEntity) { - $_SESSION['originalServiceProvider'] = $lastRequesterEntityId; - $_SESSION['proxyServiceProvider'] = $serviceProvider->entityId; + EngineBlock_ApplicationSingleton::getInstance()->getDiContainerRuntime()->feedbackStateHelper->setProxyContext($lastRequesterEntityId, $serviceProvider->entityId); throw new EngineBlock_Exception_UnknownServiceProvider( 'Invalid RequesterID specified', $lastRequesterEntityId, diff --git a/src/OpenConext/EngineBlock/Service/FeedbackInfoCollector.php b/src/OpenConext/EngineBlock/Service/FeedbackInfoCollector.php new file mode 100644 index 000000000..0e9d2a6cf --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/FeedbackInfoCollector.php @@ -0,0 +1,92 @@ +requestStack->getCurrentRequest(); + + $feedbackInfo = [ + 'datetime' => (new DateTime())->format(DateTimeInterface::ATOM), + 'requestUrl' => $request !== null + ? sprintf('%s%s', $request->getSchemeAndHttpHost(), $request->getPathInfo()) + : 'N/A', + 'requestId' => $this->requestId->get(), + 'ipAddress' => $request?->getClientIp() ?? 'N/A', + 'artCode' => Art::forException($exception), + ]; + + $bucket = $this->feedbackStateHelper->getActiveFlowContext(); + + $spEntityId = $bucket['originalServiceProvider'] ?? $bucket['serviceProvider'] ?? null; + if ($spEntityId !== null) { + $feedbackInfo['serviceProvider'] = $spEntityId; + $feedbackInfo['serviceProviderName'] = $this->getEntityDisplayName('sp', $spEntityId); + } + + if (isset($bucket['proxyServiceProvider'])) { + $feedbackInfo['proxyServiceProvider'] = $bucket['proxyServiceProvider']; + } + + if (isset($bucket['identityProvider'])) { + $idpEntityId = $bucket['identityProvider']; + $feedbackInfo['identityProvider'] = $idpEntityId; + $feedbackInfo['identityProviderName'] = $this->getEntityDisplayName('idp', $idpEntityId); + } + + return $feedbackInfo; + } + + private function getEntityDisplayName(string $type, string $entityId): string + { + $locale = $this->localeProvider->getLocale(); + try { + $entity = $type === 'sp' + ? $this->metadataRepository->fetchServiceProviderByEntityId($entityId) + : $this->metadataRepository->fetchIdentityProviderByEntityId($entityId); + return $entity->getDisplayName($locale); + } catch (EntityNotFoundException) { + return ''; + } + } +} diff --git a/src/OpenConext/EngineBlock/Service/FeedbackInfoCollectorInterface.php b/src/OpenConext/EngineBlock/Service/FeedbackInfoCollectorInterface.php new file mode 100644 index 000000000..e67b169bc --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/FeedbackInfoCollectorInterface.php @@ -0,0 +1,30 @@ +requestStack->getSession(); + } + + public function storeFeedbackInfo(array $feedback): void + { + $requestKey = $this->session()->get('currentSamlRequestId') ?? self::EARLY_FEEDBACK_KEY; + $all = $this->session()->get('feedbackInfo') ?: []; + $all[$requestKey] = $feedback; + $this->session()->set('feedbackInfo', $all); + $this->session()->set('currentFeedbackKey', $requestKey); + } + + public function getFeedbackInfo(): array + { + $feedbackKey = $this->session()->get('currentFeedbackKey'); + if ($feedbackKey === null) { + return []; + } + return ($this->session()->get('feedbackInfo') ?: [])[$feedbackKey] ?? []; + } + + public function getActiveFlowContext(): array + { + $currentRequestId = $this->session()->get('currentSamlRequestId'); + if ($currentRequestId === null) { + return []; + } + return ($this->session()->get('feedbackInfo') ?: [])[$currentRequestId] ?? []; + } + + public function mergeFeedbackInfo(array $defaults): void + { + $feedbackKey = $this->session()->get('currentFeedbackKey') ?? self::EARLY_FEEDBACK_KEY; + $all = $this->session()->get('feedbackInfo') ?: []; + $all[$feedbackKey] = array_merge($defaults, $all[$feedbackKey] ?? []); + $this->session()->set('feedbackInfo', $all); + } + + public function clearFeedbackInfo(): void + { + $currentRequestId = $this->session()->get('currentSamlRequestId'); + if ($currentRequestId !== null) { + $all = $this->session()->get('feedbackInfo') ?: []; + unset($all[$currentRequestId]); + if (empty($all)) { + $this->session()->remove('feedbackInfo'); + } else { + $this->session()->set('feedbackInfo', $all); + } + } + $this->session()->remove('currentSamlRequestId'); + // currentFeedbackKey is intentionally preserved so error pages remain accessible after a subsequent successful login. + } + + public function startNewFlow(string $samlRequestId, string $serviceProviderId): void + { + $this->session()->set('currentSamlRequestId', $samlRequestId); + $all = $this->session()->get('feedbackInfo') ?: []; + $all[$samlRequestId] = ['serviceProvider' => $serviceProviderId]; + $this->session()->set('feedbackInfo', $all); + } + + public function setCurrentIdentityProvider(string $idpEntityId): void + { + $currentRequestId = $this->session()->get('currentSamlRequestId'); + if ($currentRequestId === null) { + return; + } + $all = $this->session()->get('feedbackInfo') ?: []; + $all[$currentRequestId]['identityProvider'] = $idpEntityId; + $this->session()->set('feedbackInfo', $all); + } + + public function setProxyContext(string $originalSpId, string $proxySpId): void + { + $currentRequestId = $this->session()->get('currentSamlRequestId'); + if ($currentRequestId === null) { + return; + } + $all = $this->session()->get('feedbackInfo') ?: []; + $all[$currentRequestId]['originalServiceProvider'] = $originalSpId; + $all[$currentRequestId]['proxyServiceProvider'] = $proxySpId; + $this->session()->set('feedbackInfo', $all); + } + + public function clearFlowContext(): void + { + $this->clearFeedbackInfo(); + } +} diff --git a/src/OpenConext/EngineBlock/Service/FeedbackStateHelperInterface.php b/src/OpenConext/EngineBlock/Service/FeedbackStateHelperInterface.php new file mode 100644 index 000000000..940714158 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/FeedbackStateHelperInterface.php @@ -0,0 +1,52 @@ +engineBlockApplicationSingleton = $engineBlockApplicationSingleton; $this->logger = $logger; $this->requestStack = $requestStack; + $this->feedbackStateHelper = $feedbackStateHelper; + $this->feedbackInfoCollector = $feedbackInfoCollector; } public function reportError(Exception $exception, string $messageSuffix): void @@ -94,18 +93,18 @@ private function storeSessionFeedback(Exception $exception): void return; } - $session = $this->requestStack->getSession(); - $feedback = $session->get('feedbackInfo') ?: []; + $session = $this->requestStack->getSession(); + + if ($exception instanceof EngineBlock_Corto_Exception_PEPNoAccess) { + $session->set('error_authorization_policy_decision', $exception->getPolicyDecision()); + } + + $feedback = $this->feedbackInfoCollector->collect($exception); if ($exception instanceof EngineBlock_Corto_Exception_HasFeedbackInfoInterface) { $feedback = array_merge($feedback, $exception->getFeedbackInfo()); - } elseif ($exception instanceof EngineBlock_Corto_Exception_PEPNoAccess) { - $session->set('error_authorization_policy_decision', $exception->getPolicyDecision()); } - $session->set('feedbackInfo', array_merge( - $feedback, - $this->engineBlockApplicationSingleton->collectFeedbackInfo($exception) - )); + $this->feedbackStateHelper->storeFeedbackInfo($feedback); } } diff --git a/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php b/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php index b139c7c3b..47a0f71a4 100644 --- a/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php +++ b/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php @@ -18,6 +18,8 @@ namespace OpenConext\EngineBlockBundle\Bridge; +use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use Twig\Environment; @@ -33,6 +35,8 @@ public function __construct( public Environment $twig, public WayfRenderer $wayfRenderer, + public FeedbackStateHelperInterface $feedbackStateHelper, + public FeedbackInfoCollectorInterface $feedbackInfoCollector, private array $preferredIdpEntityIds = [], ) { } diff --git a/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php b/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php index 05824f300..9c094ea2e 100644 --- a/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php +++ b/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php @@ -19,6 +19,8 @@ namespace OpenConext\EngineBlockBundle\Bridge; use EngineBlock_ApplicationSingleton; +use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpKernel\KernelEvents; @@ -31,9 +33,17 @@ class EngineBlockBootstrapper implements EventSubscriberInterface public function __construct( Environment $twig, WayfRenderer $wayfRenderer, + FeedbackStateHelperInterface $feedbackStateHelper, + FeedbackInfoCollectorInterface $feedbackInfoCollector, array $preferredIdpEntityIds = [], ) { - $this->diContainerRuntime = new DiContainerRuntime($twig, $wayfRenderer, $preferredIdpEntityIds); + $this->diContainerRuntime = new DiContainerRuntime( + $twig, + $wayfRenderer, + $feedbackStateHelper, + $feedbackInfoCollector, + $preferredIdpEntityIds, + ); } public function onKernelRequest(): void diff --git a/src/OpenConext/EngineBlockBundle/Controller/FeedbackController.php b/src/OpenConext/EngineBlockBundle/Controller/FeedbackController.php index e20438f45..998f9cf6a 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/FeedbackController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/FeedbackController.php @@ -18,12 +18,10 @@ namespace OpenConext\EngineBlockBundle\Controller; -use EngineBlock_Corto_ProxyServer; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlockBundle\Pdp\PolicyDecision; -use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; -use Symfony\Component\HttpFoundation\Session\SessionInterface; use Symfony\Component\Routing\Attribute\Route; use Symfony\Contracts\Translation\TranslatorInterface; use Twig\Environment; @@ -35,33 +33,20 @@ */ class FeedbackController { - /** - * @var \Symfony\Contracts\Translation\TranslatorInterface - */ - private $translator; + private TranslatorInterface $translator; - /** - * @var Environment - */ - private $twig; + private Environment $twig; - /** - * @var LoggerInterface - */ - private $logger; + private FeedbackStateHelperInterface $feedbackStateHelper; public function __construct( TranslatorInterface $translator, Environment $twig, - LoggerInterface $logger + FeedbackStateHelperInterface $feedbackStateHelper ) { $this->translator = $translator; $this->twig = $twig; - $this->logger = $logger; - - // we have to start the old session in order to be able to retrieve the feedback info - $server = new EngineBlock_Corto_ProxyServer($twig); - $server->startSession(); + $this->feedbackStateHelper = $feedbackStateHelper; } #[Route( @@ -173,7 +158,7 @@ public function unknownServiceProviderAction(Request $request) // Add feedback info from url $customFeedbackInfo = ['EntityID' => $entityId]; - $this->setFeedbackInformationOnSession($request->getSession(), $customFeedbackInfo); + $this->setFeedbackInformationOnSession($customFeedbackInfo); $body = $this->twig->render( '@theme/Authentication/View/Feedback/unknown-service-provider.html.twig', @@ -194,7 +179,7 @@ public function unknownIdentityProviderAction(Request $request) 'Destination' => $request->query->get('destination'), ]; - $this->setFeedbackInformationOnSession($request->getSession(), $customFeedbackInfo); + $this->setFeedbackInformationOnSession($customFeedbackInfo); $body = $this->twig->render('@theme/Authentication/View/Feedback/unknown-identity-provider.html.twig'); @@ -250,12 +235,12 @@ public function invalidMfAuthnContextClassRefAction() #[Route(path: '/authentication/feedback/invalid-attribute-value', name: 'authentication_feedback_invalid_attribute_value', methods: ['GET'])] - public function invalidAttributeValueAction(Request $request) + public function invalidAttributeValueAction() { - $feedbackInfo = $request->getSession()->get('feedbackInfo'); + $feedbackInfo = $this->feedbackStateHelper->getFeedbackInfo(); - $attributeName = $feedbackInfo['attributeName']; - $attributeValue = $feedbackInfo['attributeValue']; + $attributeName = $feedbackInfo['attributeName'] ?? ''; + $attributeValue = $feedbackInfo['attributeValue'] ?? ''; return new Response( $this->twig->render( @@ -421,7 +406,7 @@ public function unknownPreselectedIdpAction(Request $request) $customFeedbackInfo = [ 'Idp Hash' => $request->query->get('idp-hash'), ]; - $this->setFeedbackInformationOnSession($request->getSession(), $customFeedbackInfo); + $this->setFeedbackInformationOnSession($customFeedbackInfo); return new Response( $this->twig->render('@theme/Authentication/View/Feedback/unknown-preselected-idp.html.twig'), @@ -436,7 +421,7 @@ public function unknownKeyIdAction(Request $request): Response $customFeedbackInfo = [ 'Key ID' => $request->query->get('keyid'), ]; - $this->setFeedbackInformationOnSession($request->getSession(), $customFeedbackInfo); + $this->setFeedbackInformationOnSession($customFeedbackInfo); return new Response( $this->twig->render('@theme/Authentication/View/Feedback/unknown-keyid.html.twig'), @@ -539,13 +524,8 @@ public function stepupCalloutUnknownAction(Request $request) ); } - /** - * @param SessionInterface $session - * @param array $customFeedbackInfo - */ - private function setFeedbackInformationOnSession(SessionInterface $session, array $customFeedbackInfo) + private function setFeedbackInformationOnSession(array $customFeedbackInfo): void { - $feedbackInfo = $session->get('feedbackInfo', []); - $session->set('feedbackInfo', array_merge($customFeedbackInfo, $feedbackInfo)); + $this->feedbackStateHelper->mergeFeedbackInfo($customFeedbackInfo); } } diff --git a/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Feedback.php b/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Feedback.php index 988ab2186..a0ff374c8 100644 --- a/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Feedback.php +++ b/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Feedback.php @@ -21,6 +21,7 @@ use EngineBlock_ApplicationSingleton; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\MetadataRepository\MetadataRepositoryInterface; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlockBundle\Authentication\Service\SamlResponseHelper; use OpenConext\EngineBlockBundle\Configuration\ErrorFeedbackConfigurationInterface; use OpenConext\EngineBlockBundle\Value\FeedbackInformation; @@ -50,16 +51,23 @@ class Feedback */ private $samlResponseHelper; + /** + * @var FeedbackStateHelperInterface + */ + private $feedbackStateHelper; + public function __construct( EngineBlock_ApplicationSingleton $application, ErrorFeedbackConfigurationInterface $errorFeedbackConfiguration, MetadataRepositoryInterface $metadataRepository, - SamlResponseHelper $samlResponseHelper + SamlResponseHelper $samlResponseHelper, + FeedbackStateHelperInterface $feedbackStateHelper ) { $this->application = $application; $this->errorFeedbackConfiguration = $errorFeedbackConfiguration; $this->metadataRepository = $metadataRepository; $this->samlResponseHelper = $samlResponseHelper; + $this->feedbackStateHelper = $feedbackStateHelper; } #[AsTwigFunction(name: 'flushLog')] @@ -171,8 +179,7 @@ public function getAcu(): string #[AsTwigFunction(name: 'getSamlFailedResponse')] public function getSamlFailedResponse(): string { - $session = $this->application->getSession(); - $feedbackInfo = $session->get('feedbackInfo'); + $feedbackInfo = $this->feedbackStateHelper->getFeedbackInfo(); // If AuthnFailedResponse is not set, we are unable to render a createAuthnFailedResponse $sspResponse = $feedbackInfo['AuthnFailedResponse'] ?? null; $value = ''; @@ -196,8 +203,7 @@ public function getSamlFailedResponse(): string */ private function retrieveFeedbackInfo() { - $session = $this->application->getSession(); - $feedbackInfo = $session->get('feedbackInfo'); + $feedbackInfo = $this->feedbackStateHelper->getFeedbackInfo(); $feedbackInfoMap = new FeedbackInformationMap(); // Remove the empty valued feedback info entries. diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/FeedbackController.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/FeedbackController.php index 482717901..d37273856 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/FeedbackController.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/FeedbackController.php @@ -18,12 +18,10 @@ namespace OpenConext\EngineBlockFunctionalTestingBundle\Controllers; -use EngineBlock_Corto_ProxyServer; -use Psr\Log\LoggerInterface; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; -use Symfony\Contracts\Translation\TranslatorInterface; use Twig\Environment; /** @@ -32,33 +30,17 @@ */ class FeedbackController extends AbstractController { - /** - * @var TranslatorInterface - */ - private $translator; - /** - * @var Environment - */ - private $twig; + private Environment $twig; - /** - * @var LoggerInterface - */ - private $logger; + private FeedbackStateHelperInterface $feedbackStateHelper; public function __construct( - TranslatorInterface $translator, Environment $twig, - LoggerInterface $logger + FeedbackStateHelperInterface $feedbackStateHelper ) { - $this->translator = $translator; $this->twig = $twig; - $this->logger = $logger; - - // we have to start the old session in order to be able to retrieve the feedback info - $server = new EngineBlock_Corto_ProxyServer($twig); - $server->startSession(); + $this->feedbackStateHelper = $feedbackStateHelper; } /** @@ -74,14 +56,12 @@ public function feedbackAction(Request $request) $feedbackInfo = $this->getFeedbackInfo($request); $parameters = $this->getTemplateParameters($request); - $session = $request->getSession(); - $template = sprintf( '@theme/Authentication/View/Feedback/%s.html.twig', $key ); - $session->set('feedbackInfo', $feedbackInfo); + $this->feedbackStateHelper->storeFeedbackInfo($feedbackInfo); return new Response($this->twig->render($template, $parameters), 200); } diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/ClearErrorMessages.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/ClearErrorMessages.feature index 9fd2a9633..dd6f21a4c 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/ClearErrorMessages.feature +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/ClearErrorMessages.feature @@ -214,7 +214,7 @@ Feature: And I pass through the IdP And I give my consent Then I should see "Attribute value not allowed" - And I should see "Your organisation sends a value for attribute schacHomeOrganization (\"out-of-scope\") which is not allowed for this organisation. Therefore you cannot log in." + And I should see "Dummy Idp sends a value for attribute schacHomeOrganization (\"out-of-scope\") which is not allowed for this organisation. Therefore you cannot log in." And I should see "UR ID:" And I should see "IP:" And I should see "EC:" @@ -243,7 +243,7 @@ Feature: And I pass through the IdP And I give my consent Then I should see "Attribute value not allowed" - And I should see "Your organisation sends a value for attribute eduPersonPrincipalName (\"out-of-scope\") which is not allowed for this organisation. Therefore you cannot log in." + And I should see "Dummy Idp sends a value for attribute eduPersonPrincipalName (\"out-of-scope\") which is not allowed for this organisation. Therefore you cannot log in." And I should see "UR ID:" And I should see "IP:" And I should see "EC:" @@ -273,7 +273,7 @@ Feature: And I pass through the IdP And I give my consent Then I should see "Attribute value not allowed" - And I should see "Your organisation sends a value for attribute subject-id (\"out-of-scope\") which is not allowed for this organisation. Therefore you cannot log in." + And I should see "Dummy Idp sends a value for attribute subject-id (\"out-of-scope\") which is not allowed for this organisation. Therefore you cannot log in." And I should see "UR ID:" And I should see "IP:" And I should see "EC:" @@ -289,7 +289,7 @@ Feature: And I pass through the IdP And I give my consent Then I should see "Attribute value not allowed" - And I should see "Your organisation sends a value for attribute subject-id (\"not exactly one value\") which is not allowed for this organisation. Therefore you cannot log in." + And I should see "Dummy Idp sends a value for attribute subject-id (\"not exactly one value\") which is not allowed for this organisation. Therefore you cannot log in." Scenario: I log in at my Identity Provider, and have a subject-id attribute without a scope. Given the IdP "Dummy Idp" sends attribute "urn:mace:terena.org:attribute-def:schacHomeOrganization" with value "test" @@ -300,7 +300,7 @@ Feature: And I pass through the IdP And I give my consent Then I should see "Attribute value not allowed" - And I should see "Your organisation sends a value for attribute subject-id (\"missing @ in value\") which is not allowed for this organisation. Therefore you cannot log in." + And I should see "Dummy Idp sends a value for attribute subject-id (\"missing @ in value\") which is not allowed for this organisation. Therefore you cannot log in." Scenario: I log in at my Identity Provider, that has the 'block_user_on_violation' feature activated, and has a valid subject-id attribute. Given feature "eb.block_user_on_violation" is enabled @@ -419,6 +419,49 @@ Feature: Then I should see "Unknown key id" And I should see "Key ID: does-not-exist" + Scenario: feedbackInfo from a previous failed authentication should not bleed into a subsequent unrelated error + # First auth: causes an error that writes IdP info into feedbackInfo in the session. + Given the IdP is configured to always return Responses with StatusCode Responder/RequestDenied + When I log in at "Dummy SP" + And I pass through EngineBlock + And I pass through the IdP + Then I should see "Identity Provider error" + And I should see "IdP:" + # Second auth: causes an error that has no IdP context. + # The stale IdP info from the previous error must NOT appear on this error page. + When I log in at "Unconnected SP" + Then I should see "No organisations found" + And I should not see "IdP:" + + Scenario: Global session context from a completed login should not appear in a subsequent unrelated error + When I log in at "Dummy SP" + And I pass through EngineBlock + And I pass through the IdP + And I give my consent + And I pass through EngineBlock + # Auth is now complete. An early error (before any SAML request is parsed) must not show + # SP/IdP context that leaked from the completed auth into the session. + When I post data "{}" to Engineblock URL "/authentication/idp/single-sign-on" + Then I should see "The parameter \"SAMLRequest\" is missing on the SAML SSO request" + And I should not see "IdP:" + And I should not see "SP:" + + Scenario: A previous failed auth's feedback context survives a subsequent successful login + # Flow A: an error that has SP context but no IdP context. + When I log in at "Unconnected SP" + Then I should see "No organisations found" + And I should see "SP:" + # Flow B: a completely different, successful auth. + When I log in at "Dummy SP" + And I pass through EngineBlock + And I pass through the IdP + And I give my consent + And I pass through EngineBlock + # Navigate back to flow A's error page (as if via back button or bookmarked URL). + # Flow A's SP context must still be readable from the session. + When I go to Engineblock URL "/authentication/feedback/no-idps" + Then I should see "SP:" + # Scenario: I try an unsolicited login (at EB) but mess up by not specifying a location # Scenario: I try an unsolicited login (at EB) but mess up by not specifying a binding # Scenario: I try an unsolicited login (at EB) but mess up by not specifying an invalid index diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FeedbackFooters.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FeedbackFooters.feature index a4f96ffb7..b136bc974 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FeedbackFooters.feature +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FeedbackFooters.feature @@ -44,6 +44,11 @@ Feature: And I should see "support@openconext.org" + + Scenario: The functional-testing feedback page renders correctly with feedback-info + When I go to Engineblock URL "/functional-testing/feedback?template=session-lost&feedback-info=%7B%22requestId%22%3A%22test-abc%22%2C%22ipAddress%22%3A%221.2.3.4%22%2C%22artCode%22%3A%2231914%22%7D" + Then I should see "your session was lost" + Scenario: When a IdP specific error page is shown and a translation is not configured the support emailaddress of the IdP should be hidden Given The clock on the IdP "Dummy Idp" is ahead And I have configured the following translations: diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/PolicyEnforcement.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/PolicyEnforcement.feature index 292a0e998..abb0267b8 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/PolicyEnforcement.feature +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/PolicyEnforcement.feature @@ -19,7 +19,7 @@ Feature: And I pass through EngineBlock And I pass through the IdP And I should see "Error - Access denied" - And I should see "Message from your organisation:" + And I should see "Message from Dummy IdP:" And I should see "Students of MyIdP do not have access to this resource" And the response should contain "idp-logo.jpg" @@ -30,7 +30,7 @@ Feature: And I pass through EngineBlock And I pass through the IdP And I should see "Error - Access denied" - And I should see "Message from your organisation:" + And I should see "Message from Dummy IdP:" And I should see "Students of MyIdP do not have access to this resource" And the response should contain "idp-logo.jpg" @@ -41,7 +41,7 @@ Feature: And I pass through EngineBlock And I pass through the IdP And I should see "Error - Access denied" - And I should see "Message from your organisation:" + And I should see "Message from Dummy IdP:" Scenario: Access is denied because of an Indeterminate policy Given SP "Dummy SP" requires a policy enforcement decision @@ -50,7 +50,7 @@ Feature: And I pass through EngineBlock And I pass through the IdP And I should see "Error - Access denied" - And I should see "Message from your organisation:" + And I should see "Message from Dummy IdP:" Scenario: Access is permitted because of a Permit policy Given SP "Dummy SP" requires a policy enforcement decision @@ -78,7 +78,7 @@ Feature: And I pass through EngineBlock And I pass through the IdP And I should see "Error - Access denied" - And I should see "Message from your organisation:" + And I should see "Message from Dummy IdP:" Scenario: Error page shows the end-SP name, not the trusted proxy name Given SP "Stepup SelfService" requires a policy enforcement decision @@ -103,7 +103,7 @@ Feature: And I pass through EngineBlock And I pass through the IdP And I should see "Error - Access denied" - And I should see "Message from your organisation:" + And I should see "Message from Dummy IdP:" Scenario: Access is permitted because of a Permit policy, End-SP behind Trusted Proxy Given SP "Stepup SelfService" requires a policy enforcement decision diff --git a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php index 438fb8a14..b71041e8c 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php @@ -20,6 +20,8 @@ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; +use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlockBundle\Bridge\DiContainerRuntime; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use PHPUnit\Framework\TestCase; @@ -60,7 +62,12 @@ public function setUp(): void Phake::when($this->proxyServer)->getConfig('WantsAuthnRequestsSigned')->thenReturn(false); $engineBlock = \EngineBlock_ApplicationSingleton::getInstance(); - $engineBlock->setDiContainerRuntime(new DiContainerRuntime(Phake::mock(Twig\Environment::class), Phake::mock(WayfRenderer::class))); + $engineBlock->setDiContainerRuntime(new DiContainerRuntime( + $this->createStub(Twig\Environment::class), + $this->createStub(WayfRenderer::class), + $this->createStub(FeedbackStateHelperInterface::class), + $this->createStub(FeedbackInfoCollectorInterface::class), + )); $this->bindings = new EngineBlock_Corto_Module_Bindings($this->proxyServer); } diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/AssertionConsumerTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/AssertionConsumerTest.php index 97c94f6e1..228424a3f 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/AssertionConsumerTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/AssertionConsumerTest.php @@ -21,8 +21,8 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\Factory\Factory\ServiceProviderFactory; use OpenConext\EngineBlock\Metadata\MetadataRepository\MetadataRepositoryInterface; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface; -use OpenConext\EngineBlock\Stepup\StepupGatewayCallOutHelper; use PHPUnit\Framework\TestCase; use SAML2\Assertion; use SAML2\Constants; @@ -51,9 +51,15 @@ class EngineBlock_Test_Corto_Module_Service_AssertionConsumerTest extends TestCa /** @var EngineBlock_Saml2_AuthnRequestAnnotationDecorator */ private $requestMock; + /** @var Session */ + private Session $session; + + private FeedbackStateHelperInterface $feedbackStateHelperMock; + public function setUp(): void { - $_SERVER['HTTP_HOST'] = 'engine.example.com'; + $this->session = new Session(new MockArraySessionStorage()); + $this->feedbackStateHelperMock = Phake::mock(FeedbackStateHelperInterface::class); $this->bindingsModuleMock = Phake::mock('EngineBlock_Corto_Module_Bindings'); $this->proxyServerMock = $this->mockProxyServer(); @@ -101,15 +107,9 @@ public function testTrustedProxySetsCurrentAndProxySessionVariables(): void $this->runServe(); - $this->assertSame( + Phake::verify($this->feedbackStateHelperMock)->setProxyContext( self::REAL_SP_ENTITY_ID, - $_SESSION['originalServiceProvider'], - 'originalServiceProvider must be set to the real SP entity ID behind the trusted proxy' - ); - $this->assertSame( - self::PROXY_SP_ENTITY_ID, - $_SESSION['proxyServiceProvider'], - 'proxyServiceProvider must be set to the trusted proxy SP entity ID' + self::PROXY_SP_ENTITY_ID ); } @@ -127,15 +127,8 @@ public function testNonTrustedSpDoesNotSetProxySessionVariables(): void $this->runServe(); - $this->assertArrayNotHasKey( - 'originalServiceProvider', - $_SESSION, - 'originalServiceProvider must NOT be written when the SP is not a trusted proxy' - ); - $this->assertArrayNotHasKey( - 'proxyServiceProvider', - $_SESSION, - 'proxyServiceProvider must NOT be written when the SP is not a trusted proxy' + Phake::verify($this->feedbackStateHelperMock, Phake::never())->setProxyContext( + Phake::anyParameters() ); } @@ -148,12 +141,13 @@ private function runServe(): void $service = new EngineBlock_Corto_Module_Service_AssertionConsumer( $this->proxyServerMock, Phake::mock('EngineBlock_Corto_XmlToArray'), - new Session(new MockArraySessionStorage()), + $this->session, Phake::mock(ProcessingStateHelperInterface::class), // StepupGatewayCallOutHelper is declared final and cannot be Phake-mocked; // use the real instance from the DI container (never called in this code path). EngineBlock_ApplicationSingleton::getInstance()->getDiContainer()->getStepupGatewayCallOutHelper(), - Phake::mock(ServiceProviderFactory::class) + Phake::mock(ServiceProviderFactory::class), + $this->feedbackStateHelperMock ); try { @@ -276,4 +270,4 @@ private function buildMockedRepository( return $repo; } -} \ No newline at end of file +} diff --git a/tests/unit/OpenConext/EngineBlock/Service/FeedbackInfoCollectorTest.php b/tests/unit/OpenConext/EngineBlock/Service/FeedbackInfoCollectorTest.php new file mode 100644 index 000000000..ccd7a72f0 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Service/FeedbackInfoCollectorTest.php @@ -0,0 +1,168 @@ +push($request); + + $requestId = new RequestId(new class implements RequestIdGenerator { + public function generateRequestId(): string + { + return 'test-request-id'; + } + }); + + $this->metadataRepository = m::mock(MetadataRepositoryInterface::class); + $this->metadataRepository->shouldReceive('fetchServiceProviderByEntityId') + ->andThrow(new EntityNotFoundException(''))->byDefault(); + $this->metadataRepository->shouldReceive('fetchIdentityProviderByEntityId') + ->andThrow(new EntityNotFoundException(''))->byDefault(); + + $localeProvider = new LocaleProvider( + new LanguageSupportProvider(['en', 'nl'], ['en', 'nl']), + 'en', + ); + + $this->feedbackStateHelper = m::mock(FeedbackStateHelperInterface::class); + $this->feedbackStateHelper->shouldReceive('getActiveFlowContext')->andReturn([])->byDefault(); + + $this->collector = new FeedbackInfoCollector( + $requestStack, + $requestId, + $this->metadataRepository, + $localeProvider, + $this->feedbackStateHelper, + ); + } + + #[Test] + public function collect_includes_standard_request_fields(): void + { + $this->metadataRepository->shouldIgnoreMissing(); + + $info = $this->collector->collect(new Exception('oops')); + + self::assertArrayHasKey('datetime', $info); + self::assertArrayHasKey('requestUrl', $info); + self::assertArrayHasKey('requestId', $info); + self::assertArrayHasKey('ipAddress', $info); + self::assertArrayHasKey('artCode', $info); + self::assertSame('test-request-id', $info['requestId']); + } + + #[Test] + public function collect_includes_sp_and_idp_names_from_session(): void + { + $this->feedbackStateHelper->shouldReceive('getActiveFlowContext')->andReturn([ + 'serviceProvider' => 'https://sp.example.com', + 'identityProvider' => 'https://idp.example.com', + ]); + + $sp = m::mock(ServiceProvider::class); + $sp->shouldReceive('getDisplayName')->andReturn('My SP'); + + $idp = m::mock(IdentityProvider::class); + $idp->shouldReceive('getDisplayName')->andReturn('My IdP'); + + $this->metadataRepository + ->shouldReceive('fetchServiceProviderByEntityId') + ->with('https://sp.example.com') + ->andReturn($sp); + + $this->metadataRepository + ->shouldReceive('fetchIdentityProviderByEntityId') + ->with('https://idp.example.com') + ->andReturn($idp); + + $info = $this->collector->collect(new Exception('oops')); + + self::assertSame('https://sp.example.com', $info['serviceProvider']); + self::assertSame('My SP', $info['serviceProviderName']); + self::assertSame('https://idp.example.com', $info['identityProvider']); + self::assertSame('My IdP', $info['identityProviderName']); + } + + #[Test] + public function collect_uses_original_sp_when_proxy_context_is_set(): void + { + $this->feedbackStateHelper->shouldReceive('getActiveFlowContext')->andReturn([ + 'serviceProvider' => 'https://proxy.example.com', + 'originalServiceProvider' => 'https://realsp.example.com', + 'proxyServiceProvider' => 'https://proxy.example.com', + ]); + + $sp = m::mock(ServiceProvider::class); + $sp->shouldReceive('getDisplayName')->andReturn('Real SP'); + + $this->metadataRepository + ->shouldReceive('fetchServiceProviderByEntityId') + ->with('https://realsp.example.com') + ->andReturn($sp); + + $info = $this->collector->collect(new Exception('oops')); + + self::assertSame('https://realsp.example.com', $info['serviceProvider']); + self::assertSame('Real SP', $info['serviceProviderName']); + self::assertSame('https://proxy.example.com', $info['proxyServiceProvider']); + } + + #[Test] + public function collect_returns_empty_name_when_entity_not_found(): void + { + $this->feedbackStateHelper->shouldReceive('getActiveFlowContext')->andReturn([ + 'serviceProvider' => 'https://unknown.example.com', + ]); + + $this->metadataRepository + ->shouldReceive('fetchServiceProviderByEntityId') + ->andThrow(new EntityNotFoundException('not found')); + + $info = $this->collector->collect(new Exception('oops')); + + self::assertSame('https://unknown.example.com', $info['serviceProvider']); + self::assertSame('', $info['serviceProviderName']); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Service/FeedbackStateHelperTest.php b/tests/unit/OpenConext/EngineBlock/Service/FeedbackStateHelperTest.php new file mode 100644 index 000000000..7d5ccb899 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Service/FeedbackStateHelperTest.php @@ -0,0 +1,219 @@ +session = new Session(new MockArraySessionStorage()); + + $request = new Request(); + $request->setSession($this->session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $this->helper = new FeedbackStateHelper($requestStack); + } + + #[Test] + public function it_removes_only_the_current_flows_feedback_entry(): void + { + $this->session->set('feedbackInfo', ['req-1' => ['serviceProvider' => 'https://sp.example.com']]); + $this->session->set('currentFeedbackKey', 'req-1'); + $this->session->set('currentSamlRequestId', 'req-1'); + + $this->helper->clearFeedbackInfo(); + + // The one entry was removed so feedbackInfo is now empty/gone + self::assertNull($this->session->get('feedbackInfo')); + // currentSamlRequestId is removed (the flow is done) + self::assertNull($this->session->get('currentSamlRequestId')); + // currentFeedbackKey is preserved so the error page URL still resolves + self::assertSame('req-1', $this->session->get('currentFeedbackKey')); + } + + #[Test] + public function it_preserves_other_flows_feedback_info_when_clearing_current_flow(): void + { + $this->session->set('feedbackInfo', [ + 'req-1' => ['serviceProvider' => 'https://failed-sp.example.com'], + 'req-2' => ['serviceProvider' => 'https://current-sp.example.com'], + ]); + $this->session->set('currentFeedbackKey', 'req-1'); + $this->session->set('currentSamlRequestId', 'req-2'); + + $this->helper->clearFeedbackInfo(); + + $remaining = $this->session->get('feedbackInfo'); + self::assertArrayHasKey('req-1', $remaining); + self::assertArrayNotHasKey('req-2', $remaining); + self::assertSame('req-1', $this->session->get('currentFeedbackKey')); + self::assertNull($this->session->get('currentSamlRequestId')); + } + + #[Test] + public function it_is_a_no_op_when_no_feedback_info_is_in_the_session(): void + { + // Should not throw when keys are absent + $this->helper->clearFeedbackInfo(); + + self::assertNull($this->session->get('feedbackInfo')); + self::assertNull($this->session->get('currentFeedbackKey')); + self::assertNull($this->session->get('currentSamlRequestId')); + } + + #[Test] + public function it_stores_feedback_info_keyed_by_saml_request_id(): void + { + $this->session->set('currentSamlRequestId', 'req-xyz'); + + $this->helper->storeFeedbackInfo(['serviceProvider' => 'https://sp.example.com']); + + $all = $this->session->get('feedbackInfo'); + self::assertArrayHasKey('req-xyz', $all); + self::assertSame('https://sp.example.com', $all['req-xyz']['serviceProvider']); + self::assertSame('req-xyz', $this->session->get('currentFeedbackKey')); + } + + #[Test] + public function it_stores_feedback_info_under_a_fallback_key_when_no_saml_request_id(): void + { + $feedback = ['serviceProvider' => 'https://sp.example.com']; + $this->helper->storeFeedbackInfo($feedback); + + self::assertSame($feedback, $this->helper->getFeedbackInfo()); + } + + #[Test] + public function it_returns_the_current_keyed_feedback_info(): void + { + $this->session->set('feedbackInfo', [ + 'req-1' => ['serviceProvider' => 'https://sp.example.com'], + 'req-2' => ['serviceProvider' => 'https://other.example.com'], + ]); + $this->session->set('currentFeedbackKey', 'req-1'); + + $result = $this->helper->getFeedbackInfo(); + + self::assertSame(['serviceProvider' => 'https://sp.example.com'], $result); + } + + #[Test] + public function it_returns_empty_array_when_no_current_feedback_key(): void + { + $result = $this->helper->getFeedbackInfo(); + self::assertSame([], $result); + } + + #[Test] + public function it_returns_active_flow_context_by_saml_request_id(): void + { + $this->session->set('currentSamlRequestId', 'req-1'); + $this->session->set('feedbackInfo', [ + 'req-1' => ['serviceProvider' => 'https://sp.example.com', 'identityProvider' => 'https://idp.example.com'], + 'req-2' => ['serviceProvider' => 'https://other.example.com'], + ]); + + $result = $this->helper->getActiveFlowContext(); + + self::assertSame('https://sp.example.com', $result['serviceProvider']); + self::assertSame('https://idp.example.com', $result['identityProvider']); + } + + #[Test] + public function it_returns_empty_active_flow_context_when_no_saml_request_id(): void + { + self::assertSame([], $this->helper->getActiveFlowContext()); + } + + #[Test] + public function it_starts_a_new_flow_setting_request_id_and_initialising_the_feedback_bucket(): void + { + $this->helper->startNewFlow('req-new', 'https://sp.example.com'); + + self::assertSame('req-new', $this->session->get('currentSamlRequestId')); + $bucket = ($this->session->get('feedbackInfo') ?: [])['req-new'] ?? null; + self::assertSame('https://sp.example.com', $bucket['serviceProvider']); + // No loose currentServiceProvider key is written + self::assertNull($this->session->get('currentServiceProvider')); + } + + #[Test] + public function it_sets_the_current_identity_provider(): void + { + $this->session->set('currentSamlRequestId', 'req-1'); + $this->session->set('feedbackInfo', ['req-1' => ['serviceProvider' => 'https://sp.example.com']]); + + $this->helper->setCurrentIdentityProvider('https://idp.example.com'); + + $bucket = ($this->session->get('feedbackInfo') ?: [])['req-1'] ?? []; + self::assertSame('https://idp.example.com', $bucket['identityProvider']); + // No loose currentIdentityProvider key is written + self::assertNull($this->session->get('currentIdentityProvider')); + } + + #[Test] + public function it_sets_the_proxy_context_for_trusted_proxy_flows(): void + { + $this->session->set('currentSamlRequestId', 'req-1'); + $this->session->set('feedbackInfo', ['req-1' => ['serviceProvider' => 'https://sp.example.com']]); + + $this->helper->setProxyContext('https://realsp.example.com', 'https://proxy.example.com'); + + $bucket = ($this->session->get('feedbackInfo') ?: [])['req-1'] ?? []; + self::assertSame('https://realsp.example.com', $bucket['originalServiceProvider']); + self::assertSame('https://proxy.example.com', $bucket['proxyServiceProvider']); + // No loose session keys written + self::assertNull($this->session->get('originalServiceProvider')); + self::assertNull($this->session->get('proxyServiceProvider')); + } + + #[Test] + public function it_clears_flow_context_including_feedback_and_session_vars(): void + { + $this->session->set('feedbackInfo', ['req-1' => [ + 'serviceProvider' => 'https://sp.example.com', + 'identityProvider' => 'https://idp.example.com', + 'originalServiceProvider' => 'https://orig-sp.example.com', + 'proxyServiceProvider' => 'https://proxy-sp.example.com', + ]]); + $this->session->set('currentFeedbackKey', 'req-1'); + $this->session->set('currentSamlRequestId', 'req-1'); + + $this->helper->clearFlowContext(); + + // feedbackInfo entry for the completed flow is removed (was the only entry) + self::assertNull($this->session->get('feedbackInfo')); + // currentFeedbackKey is preserved so error page URLs keep working + self::assertSame('req-1', $this->session->get('currentFeedbackKey')); + self::assertNull($this->session->get('currentSamlRequestId')); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Service/ProcessingStateHelperTest.php b/tests/unit/OpenConext/EngineBlock/Service/ProcessingStateHelperTest.php new file mode 100644 index 000000000..3382fcfce --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Service/ProcessingStateHelperTest.php @@ -0,0 +1,86 @@ +session = new Session(new MockArraySessionStorage()); + + $request = new Request(); + $request->setSession($this->session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $this->helper = new ProcessingStateHelper($requestStack); + } + + #[Test] + public function it_stores_and_retrieves_a_processing_step(): void + { + $role = m::mock(AbstractRole::class); + $response = m::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class); + + $step = $this->helper->addStep('req-1', ProcessingStateHelperInterface::STEP_CONSENT, $role, $response); + + self::assertInstanceOf(ProcessingStateStep::class, $step); + self::assertSame($step, $this->helper->getStepByRequestId('req-1', ProcessingStateHelperInterface::STEP_CONSENT)); + } + + #[Test] + public function it_throws_when_step_not_found(): void + { + $this->expectException(EngineBlock_Corto_Module_Services_SessionLostException::class); + + $this->helper->getStepByRequestId('nonexistent', ProcessingStateHelperInterface::STEP_CONSENT); + } + + #[Test] + public function it_clears_a_step_by_request_id(): void + { + $role = m::mock(AbstractRole::class); + $response = m::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class); + + $this->helper->addStep('req-1', ProcessingStateHelperInterface::STEP_CONSENT, $role, $response); + $this->helper->clearStepByRequestId('req-1'); + + $this->expectException(EngineBlock_Corto_Module_Services_SessionLostException::class); + $this->helper->getStepByRequestId('req-1', ProcessingStateHelperInterface::STEP_CONSENT); + } +} diff --git a/tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php b/tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php new file mode 100644 index 000000000..723ef6e4b --- /dev/null +++ b/tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php @@ -0,0 +1,146 @@ +session = new Session(new MockArraySessionStorage()); + + $request = new Request(); + $request->setSession($this->session); + + $this->requestStack = new RequestStack(); + $this->requestStack->push($request); + + $this->applicationSingleton = m::mock(EngineBlock_ApplicationSingleton::class); + $this->applicationSingleton->shouldReceive('flushLog')->byDefault(); + + $this->logger = m::mock(LoggerInterface::class); + $this->logger->shouldIgnoreMissing(); + + $this->feedbackStateHelper = m::mock(FeedbackStateHelperInterface::class); + $this->feedbackStateHelper->shouldReceive('storeFeedbackInfo')->byDefault(); + + $this->feedbackInfoCollector = m::mock(FeedbackInfoCollectorInterface::class); + $this->feedbackInfoCollector->shouldReceive('collect')->andReturn([])->byDefault(); + + $this->reporter = new ErrorReporter( + $this->applicationSingleton, + $this->logger, + $this->requestStack, + $this->feedbackStateHelper, + $this->feedbackInfoCollector, + ); + } + + #[Test] + public function it_collects_and_stores_feedback_via_the_dedicated_services(): void + { + $exception = new Exception('test'); + $feedback = ['artCode' => 'art:0:0:0:0']; + + $this->feedbackInfoCollector->shouldReceive('collect')->with($exception)->once()->andReturn($feedback); + $this->feedbackStateHelper->shouldReceive('storeFeedbackInfo')->with($feedback)->once(); + + $this->reporter->reportError($exception, ''); + } + + #[Test] + public function it_skips_feedback_storage_when_there_is_no_active_request(): void + { + $reporter = new ErrorReporter( + $this->applicationSingleton, + $this->logger, + new RequestStack(), + $this->feedbackStateHelper, + $this->feedbackInfoCollector, + ); + + $this->feedbackInfoCollector->shouldNotReceive('collect'); + $this->feedbackStateHelper->shouldNotReceive('storeFeedbackInfo'); + + $reporter->reportError(new Exception('test'), ''); + } + + #[Test] + public function it_merges_exception_feedback_info_when_exception_implements_has_feedback_info_interface(): void + { + $exception = new class('test') extends Exception implements EngineBlock_Corto_Exception_HasFeedbackInfoInterface { + public function getFeedbackInfo(): array + { + return ['customKey' => 'customValue', 'artCode' => 'overridden-art-code']; + } + }; + + $this->feedbackInfoCollector + ->shouldReceive('collect') + ->andReturn(['artCode' => 'original-art-code', 'datetime' => '2026-01-01']); + + $this->feedbackStateHelper + ->shouldReceive('storeFeedbackInfo') + ->once() + ->with([ + 'artCode' => 'overridden-art-code', + 'datetime' => '2026-01-01', + 'customKey' => 'customValue', + ]); + + $this->reporter->reportError($exception, ''); + } + + #[Test] + public function it_stores_the_pep_policy_decision_in_the_session(): void + { + $decision = new stdClass(); + $exception = EngineBlock_Corto_Exception_PEPNoAccess::basedOn($decision); + + $this->reporter->reportError($exception, ''); + + self::assertSame($decision, $this->session->get('error_authorization_policy_decision')); + } +} diff --git a/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php b/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php index 073562534..b90701dde 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php @@ -18,6 +18,8 @@ namespace Tests\OpenConext\EngineBlockBundle; +use OpenConext\EngineBlock\Service\FeedbackInfoCollectorInterface; +use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface; use OpenConext\EngineBlockBundle\Bridge\DiContainerRuntime; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use PHPUnit\Framework\TestCase; @@ -30,6 +32,8 @@ public function testGetPreferredIdpEntityIdsReturnsEmptyArrayByDefault(): void $runtime = new DiContainerRuntime( $this->createStub(Environment::class), $this->createStub(WayfRenderer::class), + $this->createStub(FeedbackStateHelperInterface::class), + $this->createStub(FeedbackInfoCollectorInterface::class), ); $this->assertSame([], $runtime->getPreferredIdpEntityIds()); @@ -42,6 +46,8 @@ public function testGetPreferredIdpEntityIdsReturnsConfiguredList(): void $runtime = new DiContainerRuntime( $this->createStub(Environment::class), $this->createStub(WayfRenderer::class), + $this->createStub(FeedbackStateHelperInterface::class), + $this->createStub(FeedbackInfoCollectorInterface::class), $entityIds, );