Fix feedbackInfo session bleed-through between auth flows (#1795)

feedbackInfo was stored as a flat dict in $_SESSION['feedbackInfo'] and
merged with existing data on each write. This meant info collected
during one auth flow (e.g. which IdP was involved) could appear on error
pages in a completely separate flow.

Additionally, currentServiceProvider and currentIdentityProvider were
never cleared from the session after a successful login, so they would
show up on error pages for unrelated early errors.

The fix:
- feedbackInfo is now keyed per SAML request ID so each auth flow has
  its own isolated bucket with no merging across flows
- clearFlowContext() is called by ProcessedAssertionConsumer after a
  successful auth, clearing all flow context from the session
- All session access moved from raw $_SESSION to the Symfony session
- Session ops for feedbackInfo and flow context are centralised in a new
  FeedbackStateHelper class, split out from ProcessingStateHelper
- FeedbackStateHelper is wired through DiContainerRuntime instead of the
  legacy DiContainer

Author: johanib

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:

config/services/ci/controllers.yml

@@ -27,6 +27,7 @@ services:
             - '@translator'
             - '@twig'
             - '@engineblock.compat.logger'
+            - '@OpenConext\EngineBlock\Service\FeedbackStateHelper'
 
     engineblock.functional_test.controller.consent:
         class: OpenConext\EngineBlockFunctionalTestingBundle\Controllers\ConsentController

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:

config/services/services.yml

@@ -69,6 +69,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'
@@ -319,6 +337,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

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 '';
-    }
 }

library/EngineBlock/Corto/Module/Bindings.php

@@ -180,8 +180,11 @@ public function receiveRequest()
                 'Missing <saml:Issuer> 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(

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,41 +32,28 @@
 
 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,
         EngineBlock_Corto_XmlToArray $xmlConverter,
         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.

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;
     }
 }

library/EngineBlock/Corto/Module/Service/SingleSignOn.php

@@ -185,8 +185,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');
         }

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(