From b20fa79506439906323663ac15c9ddb07bbb063b Mon Sep 17 00:00:00 2001
From: Jaime Perez Crespo <jaime.perez@uninett.no>
Date: Thu, 19 Mar 2015 16:02:10 +0100
Subject: [PATCH] Fail more gracefully when the different endpoints are
accessed directly. Instead of displaying an "Unable to find the current
binding" error message that creates confusion, tell the user it's his fault.
---
dictionaries/errors.definition.json | 18 +++++++++++++++---
modules/saml/www/sp/saml2-acs.php | 13 ++++++++++++-
modules/saml/www/sp/saml2-logout.php | 12 +++++++++++-
www/saml2/idp/ArtifactResolutionService.php | 13 ++++++++++++-
www/saml2/idp/SSOService.php | 10 +++++++++-
www/saml2/idp/SingleLogoutService.php | 12 +++++++++++-
6 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/dictionaries/errors.definition.json b/dictionaries/errors.definition.json
index fdea1a8a4..e79e55eda 100644
--- a/dictionaries/errors.definition.json
+++ b/dictionaries/errors.definition.json
@@ -96,14 +96,26 @@
"en": "No SAML message provided"
},
"descr_SLOSERVICEPARAMS": {
- "en": "You accessed the SingleLogoutService interface, but did not provide a SAML LogoutRequest or LogoutResponse."
+ "en": "You accessed the SingleLogoutService interface, but did not provide a SAML LogoutRequest or LogoutResponse. Please note that this endpoint is not intended to be accessed directly."
},
"title_ACSPARAMS": {
"en": "No SAML response provided"
},
"descr_ACSPARAMS": {
- "en": "You accessed the Assertion Consumer Service interface, but did not provide a SAML Authentication Response."
- },
+ "en": "You accessed the Assertion Consumer Service interface, but did not provide a SAML Authentication Response. Please note that this endpoint is not intended to be accessed directly."
+ },
+ "title_SSOPARAMS": {
+ "en": "No SAML request provided"
+ },
+ "descr_SSOPARAMS": {
+ "en": "You accessed the Single Sign On Service interface, but did not provide a SAML Authentication Request. Please note that this endpoint is not intended to be accessed directly."
+ },
+ "title_ARSPARAMS": {
+ "en": "No SAML message provided"
+ },
+ "descr_ARSPARAMS": {
+ "en": "You accessed the Artifact Resolution Service interface, but did not provide a SAML ArtifactResolve message. Please note that this endpoint is not intended to be accessed directly."
+ },
"title_CASERROR": {
"en": "CAS Error"
},
diff --git a/modules/saml/www/sp/saml2-acs.php b/modules/saml/www/sp/saml2-acs.php
index d22286278..21bdce256 100644
--- a/modules/saml/www/sp/saml2-acs.php
+++ b/modules/saml/www/sp/saml2-acs.php
@@ -12,7 +12,18 @@ $sourceId = substr($_SERVER['PATH_INFO'], 1);
$source = SimpleSAML_Auth_Source::getById($sourceId, 'sspmod_saml_Auth_Source_SP');
$spMetadata = $source->getMetadata();
-$b = SAML2_Binding::getCurrentBinding();
+try {
+ $b = SAML2_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 SimpleSAML_Error_Error('ACSPARAMS', $e, 400);
+ } else {
+ throw $e; // do not ignore other exceptions!
+ }
+}
+
if ($b instanceof SAML2_HTTPArtifact) {
$b->setSPMetadata($spMetadata);
}
diff --git a/modules/saml/www/sp/saml2-logout.php b/modules/saml/www/sp/saml2-logout.php
index d3898e779..1c5d87546 100644
--- a/modules/saml/www/sp/saml2-logout.php
+++ b/modules/saml/www/sp/saml2-logout.php
@@ -20,7 +20,17 @@ if (!($source instanceof sspmod_saml_Auth_Source_SP)) {
throw new SimpleSAML_Error_Exception('Source type changed?');
}
-$binding = SAML2_Binding::getCurrentBinding();
+try {
+ $binding = SAML2_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 SimpleSAML_Error_Error('SLOSERVICEPARAMS', $e, 400);
+ } else {
+ throw $e; // do not ignore other exceptions!
+ }
+}
$message = $binding->receive();
$idpEntityId = $message->getIssuer();
diff --git a/www/saml2/idp/ArtifactResolutionService.php b/www/saml2/idp/ArtifactResolutionService.php
index a6ea5984e..66355489e 100644
--- a/www/saml2/idp/ArtifactResolutionService.php
+++ b/www/saml2/idp/ArtifactResolutionService.php
@@ -29,7 +29,18 @@ if ($store === FALSE) {
}
$binding = new SAML2_SOAP();
-$request = $binding->receive();
+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.') {
+ throw new SimpleSAML_Error_Error('ARSPARAMS', $e, 400);
+ } else {
+ throw $e; // do not ignore other exceptions!
+ }
+}
if (!($request instanceof SAML2_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 40f2c1582..358dad6da 100644
--- a/www/saml2/idp/SSOService.php
+++ b/www/saml2/idp/SSOService.php
@@ -15,5 +15,13 @@ SimpleSAML_Logger::info('SAML2.0 - IdP.SSOService: Accessing SAML 2.0 IdP endpoi
$metadata = SimpleSAML_Metadata_MetaDataStorageHandler::getMetadataHandler();
$idpEntityId = $metadata->getMetaDataCurrentEntityID('saml20-idp-hosted');
$idp = SimpleSAML_IdP::getById('saml2:' . $idpEntityId);
-sspmod_saml_IdP_SAML2::receiveAuthnRequest($idp);
+try {
+ sspmod_saml_IdP_SAML2::receiveAuthnRequest($idp);
+} catch (Exception $e) {
+ if ($e->getMessage() === "Unable to find the current binding.") {
+ throw new SimpleSAML_Error_Error('SSOPARAMS', $e, 400);
+ } else {
+ throw $e; // do not ignore other exceptions!
+ }
+}
assert('FALSE');
diff --git a/www/saml2/idp/SingleLogoutService.php b/www/saml2/idp/SingleLogoutService.php
index de7510255..032027abf 100644
--- a/www/saml2/idp/SingleLogoutService.php
+++ b/www/saml2/idp/SingleLogoutService.php
@@ -19,6 +19,16 @@ $idp = SimpleSAML_IdP::getById('saml2:' . $idpEntityId);
if (isset($_REQUEST['ReturnTo'])) {
$idp->doLogoutRedirect(SimpleSAML_Utilities::checkURLAllowed((string)$_REQUEST['ReturnTo']));
} else {
- sspmod_saml_IdP_SAML2::receiveLogoutMessage($idp);
+ try {
+ sspmod_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 SimpleSAML_Error_Error('SLOSERVICEPARAMS', $e, 400);
+ } else {
+ throw $e; // do not ignore other exceptions!
+ }
+ }
}
assert('FALSE');
--
GitLab