From 5ca4aa8549103826b0d73e6129f8e1e630c63bb6 Mon Sep 17 00:00:00 2001
From: Thijs Kinkhorst <thijs@kinkhorst.com>
Date: Mon, 17 Jan 2022 11:23:09 +0000
Subject: [PATCH] Clean up code in bindings now that we have specific
 exceptions

---
 modules/saml/lib/IdP/SAML2.php              |  7 +------
 modules/saml/www/sp/saml2-acs.php           | 15 ++++-----------
 modules/saml/www/sp/saml2-logout.php        | 12 +++---------
 www/saml2/idp/ArtifactResolutionService.php | 12 ++----------
 www/saml2/idp/SSOService.php                | 10 +++-------
 www/saml2/idp/SingleLogoutService.php       | 15 +++------------
 6 files changed, 16 insertions(+), 55 deletions(-)

diff --git a/modules/saml/lib/IdP/SAML2.php b/modules/saml/lib/IdP/SAML2.php
index d0cc00008..f141ff0cf 100644
--- a/modules/saml/lib/IdP/SAML2.php
+++ b/modules/saml/lib/IdP/SAML2.php
@@ -354,12 +354,7 @@ class SAML2
                 'SAML2.0 - IdP.SSOService: IdP initiated authentication: ' . var_export($spEntityId, true)
             );
         } else {
-            try {
-                $binding = Binding::getCurrentBinding();
-            } catch (Exception $e) {
-                header($_SERVER["SERVER_PROTOCOL"] . " 405 Method Not Allowed", true, 405);
-                exit;
-            }
+            $binding = Binding::getCurrentBinding();
             $request = $binding->receive();
 
             if (!($request instanceof AuthnRequest)) {
diff --git a/modules/saml/www/sp/saml2-acs.php b/modules/saml/www/sp/saml2-acs.php
index 23aad3391..7cc0ce42f 100644
--- a/modules/saml/www/sp/saml2-acs.php
+++ b/modules/saml/www/sp/saml2-acs.php
@@ -6,6 +6,7 @@
 
 use SAML2\Binding;
 use SAML2\Assertion;
+use SAML2\Exception\Protocol\UnsupportedBindingException;
 use SAML2\HTTPArtifact;
 use SAML2\Response;
 use SimpleSAML\Assert\Assert;
@@ -30,16 +31,8 @@ $source = Auth\Source::getById($sourceId, '\SimpleSAML\Module\saml\Auth\Source\S
 $spMetadata = $source->getMetadata();
 try {
     $b = Binding::getCurrentBinding();
-} catch (Exception $e) {
-    // TODO: look for a specific exception
-    // This is dirty. Instead of checking the message of the exception, \SAML2\Binding::getCurrentBinding() should throw
-    // a specific exception when the binding is unknown, and we should capture that here
-    if ($e->getMessage() === 'Unable to find the current binding.') {
-        throw new Error\Error('ACSPARAMS', $e, 400);
-    } else {
-        // do not ignore other exceptions!
-        throw $e;
-    }
+} catch (UnsupportedBindingException $e) {
+    throw new Error\Error('ACSPARAMS', $e, 400);
 }
 
 if ($b instanceof HTTPArtifact) {
@@ -48,7 +41,7 @@ if ($b instanceof HTTPArtifact) {
 
 $response = $b->receive();
 if (!($response instanceof Response)) {
-    throw new Error\BadRequest('Invalid message received to AssertionConsumerService endpoint.');
+    throw new Error\BadRequest('Invalid message received at AssertionConsumerService endpoint.');
 }
 
 $issuer = $response->getIssuer();
diff --git a/modules/saml/www/sp/saml2-logout.php b/modules/saml/www/sp/saml2-logout.php
index a91cf3d3f..29478862a 100644
--- a/modules/saml/www/sp/saml2-logout.php
+++ b/modules/saml/www/sp/saml2-logout.php
@@ -9,6 +9,7 @@
 use Exception;
 use SAML2\Binding;
 use SAML2\Constants;
+use SAML2\Exception\Protocol\UnsupportedBindingException;
 use SAML2\LogoutResponse;
 use SAML2\LogoutRequest;
 use SAML2\SOAP;
@@ -36,15 +37,8 @@ if ($source === null) {
 
 try {
     $binding = Binding::getCurrentBinding();
-} catch (Exception $e) {
-    // TODO: look for a specific exception
-    // This is dirty. Instead of checking the message of the exception, \SAML2\Binding::getCurrentBinding() should throw
-    // an specific exception when the binding is unknown, and we should capture that here
-    if ($e->getMessage() === 'Unable to find the current binding.') {
-        throw new Error\Error('SLOSERVICEPARAMS', $e, 400);
-    } else {
-        throw $e; // do not ignore other exceptions!
-    }
+} catch (UnsupportedBindingException $e) {
+    throw new Error\Error('SLOSERVICEPARAMS', $e, 400);
 }
 $message = $binding->receive();
 
diff --git a/www/saml2/idp/ArtifactResolutionService.php b/www/saml2/idp/ArtifactResolutionService.php
index 5e91f3fc3..30b3652ac 100644
--- a/www/saml2/idp/ArtifactResolutionService.php
+++ b/www/saml2/idp/ArtifactResolutionService.php
@@ -9,7 +9,7 @@
 
 require_once('../../_include.php');
 
-use Exception;
+use SAML2\Exception\Protocol\UnsupportedBindingException;
 use SAML2\ArtifactResolve;
 use SAML2\ArtifactResponse;
 use SAML2\DOMDocumentFactory;
@@ -44,16 +44,8 @@ if ($store === false) {
 $binding = new SOAP();
 try {
     $request = $binding->receive();
-} catch (Exception $e) {
-    // TODO: look for a specific exception
-    // This is dirty. Instead of checking the message of the exception, \SAML2\Binding::getCurrentBinding() should throw
-    // an specific exception when the binding is unknown, and we should capture that here. Also note that the exception
-    // message here is bogus!
-    if ($e->getMessage() === 'Invalid message received to AssertionConsumerService endpoint.') {
+} catch (UnsupportedBindingException $e) {
         throw new Error\Error('ARSPARAMS', $e, 400);
-    } else {
-        throw $e; // do not ignore other exceptions!
-    }
 }
 if (!($request instanceof ArtifactResolve)) {
     throw new Exception('Message received on ArtifactResolutionService wasn\'t a ArtifactResolve request.');
diff --git a/www/saml2/idp/SSOService.php b/www/saml2/idp/SSOService.php
index ef1686c1d..f139b2db3 100644
--- a/www/saml2/idp/SSOService.php
+++ b/www/saml2/idp/SSOService.php
@@ -10,7 +10,7 @@
 
 require_once('../../_include.php');
 
-use Exception;
+use SAML2\Exception\Protocol\UnsupportedBindingException;
 use SimpleSAML\Assert\Assert;
 use SimpleSAML\Configuration;
 use SimpleSAML\Error;
@@ -32,11 +32,7 @@ $idp = IdP::getById('saml2:' . $idpEntityId);
 
 try {
     Module\saml\IdP\SAML2::receiveAuthnRequest($idp);
-} catch (Exception $e) {
-    if ($e->getMessage() === "Unable to find the current binding.") {
-        throw new Error\Error('SSOPARAMS', $e, 400);
-    } else {
-        throw $e; // do not ignore other exceptions!
-    }
+} catch (UnsupportedBindingException $e) {
+    throw new Error\Error('SSOPARAMS', $e, 400);
 }
 Assert::true(false);
diff --git a/www/saml2/idp/SingleLogoutService.php b/www/saml2/idp/SingleLogoutService.php
index 648320fa8..2a617332f 100644
--- a/www/saml2/idp/SingleLogoutService.php
+++ b/www/saml2/idp/SingleLogoutService.php
@@ -9,7 +9,7 @@
 
 require_once('../../_include.php');
 
-use Exception;
+use SAML2\Exception\Protocol\UnsupportedBindingException;
 use SimpleSAML\Assert\Assert;
 use SimpleSAML\Configuration;
 use SimpleSAML\Error;
@@ -36,17 +36,8 @@ if (isset($_REQUEST['ReturnTo'])) {
 } else {
     try {
         Module\saml\IdP\SAML2::receiveLogoutMessage($idp);
-    } catch (Exception $e) {
-        // TODO: look for a specific exception
-        /*
-         * This is dirty. Instead of checking the message of the exception, \SAML2\Binding::getCurrentBinding() should
-         * throw an specific exception when the binding is unknown, and we should capture that here
-         */
-        if ($e->getMessage() === 'Unable to find the current binding.') {
-            throw new Error\Error('SLOSERVICEPARAMS', $e, 400);
-        } else {
-            throw $e; // do not ignore other exceptions!
-        }
+    } catch (UnsupportedBindingException $e) {
+        throw new Error\Error('SLOSERVICEPARAMS', $e, 400);
     }
 }
 Assert::true(false);
-- 
GitLab