Remove quadruple wrong binding log (#1978)

Prior to this change, when an SP used an unsupported ProtocolBinding in its AuthnRequest, EB would output 4x the same log message.
This was the case because the log was very deep in the call tree AND the ACS lookup was performed twice in createEnhancedResponse().

This change first removes the duplicate ACS lookup, reducing the log message to 2x. But even then, logging at a very low level still caused duplicate messages. So the check was moved to the entry point (SingleSignOn::serve()) where the SP request is first received.

Resolves #1807

Author: johanib

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

@@ -16,15 +16,13 @@
  * limitations under the License.
  */
 
-use OpenConext\EngineBlock\Logger\Message\AdditionalInfo;
-use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider;
 use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
 use OpenConext\EngineBlock\Metadata\Factory\Factory\ServiceProviderFactory;
 use OpenConext\EngineBlock\Metadata\Discovery;
 use OpenConext\EngineBlock\Metadata\X509\KeyPairFactory;
-use OpenConext\EngineBlockBundle\Exception\InvalidArgumentException as EngineBlockBundleInvalidArgumentException;
 use OpenConext\EngineBlockBundle\Service\DiscoverySelectionService;
 use SAML2\AuthnRequest;
+use SAML2\Constants;
 use SAML2\Response;
 use SAML2\XML\saml\Issuer;
 use Symfony\Component\HttpFoundation\Request;
@@ -104,6 +102,14 @@ public function serve($serviceName, Request $httpRequest)
         // Exposing entityId to be used when tracking the start of an authentication procedure
         $application->authenticationStateSpEntityId = $sp->entityId;
 
+        // EB will fall back to HTTP-POST
+        $protocolBinding = $request->getProtocolBinding();
+        if (!empty($protocolBinding) && $protocolBinding !== Constants::BINDING_HTTP_POST) {
+            $log->notice(
+                "ProtocolBinding '{$protocolBinding}' requested by Issuer '{$sp->entityId}' is not supported, ignoring..."
+            );
+        }
+
         // Flush log if an SP in the requester chain has additional logging enabled
         $log->info("Determining whether service provider in chain requires additional logging");
 

library/EngineBlock/Corto/ProxyServer.php

@@ -830,8 +830,7 @@ public function createEnhancedResponse(
         $subjectConfirmation->setSubjectConfirmationData($subjectConfirmationData);
 
         // Confirm where we are sending it.
-        $acs = $this->getRequestAssertionConsumer($request);
-        $subjectConfirmationData->setRecipient($acs->location);
+        $subjectConfirmationData->setRecipient($newResponse->getDestination());
 
         // Confirm that this is in response to their AuthnRequest (unless, you know, it isn't).
         if (!$request->isUnsolicited()) {
@@ -964,7 +963,7 @@ public function getDefaultAssertionConsumer(ServiceProvider $serviceProvider)
      *
      * @param EngineBlock_Saml2_AuthnRequestAnnotationDecorator $request
      * @param ServiceProvider $serviceProvider
-     * @return null|\OpenConext\EngineBlock\Metadata\IndexedService
+     * @return null|\OpenConext\EngineBlock\Metadata\IndexedService|false
      */
     public function getCustomAssertionConsumer(
         EngineBlock_Saml2_AuthnRequestAnnotationDecorator $request,
@@ -983,9 +982,6 @@ public function getCustomAssertionConsumer(
         }
 
         if ($protocolBinding !== Constants::BINDING_HTTP_POST) {
-            $this->_server->getLogger()->notice(
-                "ProtocolBinding '{$protocolBinding}' requested by Issuer '{$serviceProvider->entityId}' is not supported, ignoring..."
-            );
             return false;
         }
 

src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Bindings.feature

@@ -118,3 +118,12 @@ Feature:
       And I pass through the IdP
      Then the url should match "authentication/feedback/unsolicited-response"
       And I should see "Error - Sign-in could not be completed"
+
+  Scenario: EngineBlock falls back to HTTP-POST when an unsupported ProtocolBinding is requested
+    Given the SP requests ProtocolBinding "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect"
+     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
+     Then the url should match "functional-testing/Dummy%20SP/acs"

src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/MockSpContext.php

@@ -332,6 +332,18 @@ public function theSpUsesTheHttpRedirectBinding()
         $this->mockSpRegistry->save();
     }
 
+    /**
+     * @Given /^the SP requests ProtocolBinding "([^"]*)"$/
+     */
+    public function theSpRequestsProtocolBinding($binding)
+    {
+        $sp = $this->mockSpRegistry->getOnly();
+
+        $sp->setRequestedProtocolBinding($binding);
+
+        $this->mockSpRegistry->save();
+    }
+
     /**
      * @Given /^no registered SPs/
      */

src/OpenConext/EngineBlockFunctionalTestingBundle/Mock/MockServiceProvider.php

@@ -133,6 +133,11 @@ public function setAuthnRequestProxyCountTo($proxyCount)
         $this->descriptor->getExtensions()['SAMLRequest']->setProxyCount($proxyCount);
     }
 
+    public function setRequestedProtocolBinding(string $binding)
+    {
+        $this->descriptor->getExtensions()['SAMLRequest']->setProtocolBinding($binding);
+    }
+
     public function setAuthnRequestToPassive()
     {
         $this->descriptor->getExtensions()['SAMLRequest']->setIsPassive(true);