From 6097f7c72afbc251b58b5c206d5d2d078fd7ac36 Mon Sep 17 00:00:00 2001
From: Thijs Kinkhorst <thijs@kinkhorst.com>
Date: Fri, 27 Aug 2021 10:23:53 +0000
Subject: [PATCH] We only support SAML 2.0, so clean up tracking which protocol
we support
This auth source no longer supports SAML 1.1 so the ACS code was keeping
track of protocol support with only SAML 2.0 as an option to be added.
Therefore, simplify and set this globally in the class. Keep the outside
interface for now the same where getSupportedProtocols returns as list
of the supported protocol.
Note: if you specify AssertionConsumerService in your config you are
responsible for it having actually bindings that we support. This has
always been the case. This PR makes no attempt to change it (the
specified URLs can even be outside of SSP).
---
modules/saml/docs/sp.md | 4 +-
modules/saml/lib/Auth/Source/SP.php | 11 +---
tests/modules/saml/lib/Auth/Source/SPTest.php | 50 +++++++++++++++++++
3 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/modules/saml/docs/sp.md b/modules/saml/docs/sp.md
index 9fd231bbe..efa40a0f6 100644
--- a/modules/saml/docs/sp.md
+++ b/modules/saml/docs/sp.md
@@ -113,7 +113,9 @@ Options
`AssertionConsumerService`
: List of Assertion Consumer Services in the generated metadata. Specified in the array of
arrays format as seen in the [Metadata endpoints](./simplesamlphp-metadata-endpoints)
- documentation.
+ documentation. Note that this list is taken at face value, so it's not useful to list
+ anything here that the SP auth source does not actually support (unless the URLs point
+ externally).
`AssertionConsumerServiceIndex`
: The Assertion Consumer Service Index to be used in the AuthnRequest in place of the Assertion
diff --git a/modules/saml/lib/Auth/Source/SP.php b/modules/saml/lib/Auth/Source/SP.php
index bddeac4d3..1a6f8db57 100644
--- a/modules/saml/lib/Auth/Source/SP.php
+++ b/modules/saml/lib/Auth/Source/SP.php
@@ -62,7 +62,7 @@ class SP extends \SimpleSAML\Auth\Source
*
* @var string[]
*/
- private array $protocols = [];
+ private array $protocols = [Constants::NS_SAMLP];
/**
@@ -351,18 +351,12 @@ class SP extends \SimpleSAML\Auth\Source
'Binding' => Constants::BINDING_HTTP_POST,
'Location' => Module::getModuleURL('saml/sp/saml2-acs.php/' . $this->getAuthId()),
];
- if (!in_array(Constants::NS_SAMLP, $this->protocols, true)) {
- $this->protocols[] = Constants::NS_SAMLP;
- }
break;
case Constants::BINDING_HTTP_ARTIFACT:
$acs = [
'Binding' => Constants::BINDING_HTTP_ARTIFACT,
'Location' => Module::getModuleURL('saml/sp/saml2-acs.php/' . $this->getAuthId()),
];
- if (!in_array(Constants::NS_SAMLP, $this->protocols, true)) {
- $this->protocols[] = Constants::NS_SAMLP;
- }
break;
case Constants::BINDING_HOK_SSO:
$acs = [
@@ -370,9 +364,6 @@ class SP extends \SimpleSAML\Auth\Source
'Location' => Module::getModuleURL('saml/sp/saml2-acs.php/' . $this->getAuthId()),
'hoksso:ProtocolBinding' => Constants::BINDING_HTTP_REDIRECT,
];
- if (!in_array(Constants::NS_SAMLP, $this->protocols, true)) {
- $this->protocols[] = Constants::NS_SAMLP;
- }
break;
default:
Logger::warning('Unknown acs.Binding value specified, ignoring: ' . $service);
diff --git a/tests/modules/saml/lib/Auth/Source/SPTest.php b/tests/modules/saml/lib/Auth/Source/SPTest.php
index 883fa0096..6b3804551 100644
--- a/tests/modules/saml/lib/Auth/Source/SPTest.php
+++ b/tests/modules/saml/lib/Auth/Source/SPTest.php
@@ -1198,4 +1198,54 @@ class SPTest extends ClearStateTestCase
$this->assertEquals('new_', $md['keys'][0]['prefix']);
$this->assertEquals('', $md['keys'][1]['prefix']);
}
+
+ /**
+ * We only support SAML 2.0 as a protocol with this auth source
+ */
+ public function testSupportedProtocolsReturnsSAML20Only(): void
+ {
+ $spId = 'myhosted-sp';
+ $info = ['AuthId' => $spId];
+ $config = [];
+ $as = new SpTester($info, $config);
+
+ $md = $as->getHostedMetadata();
+ $protocols = $as->getSupportedProtocols();
+ $this->assertIsArray($protocols);
+ $this->assertCount(1, $protocols);
+ $this->assertEquals('urn:oasis:names:tc:SAML:2.0:protocol', $protocols[0]);
+ }
+
+ /**
+ * We only support SAML 2.0 as a protocol with this auth source
+ */
+ public function testSAML11BindingsDoesNotInfluenceProtocolsSupported(): void
+ {
+ $spId = 'myhosted-sp';
+ $info = ['AuthId' => $spId];
+ $config = [
+ 'AssertionConsumerService' => [
+ [
+ 'index' => 1,
+ 'isDefault' => TRUE,
+ 'Location' => 'https://sp.example.org/ACS',
+ 'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST',
+ ],
+ [
+ 'index' => 17,
+ 'Location' => 'https://sp.example.org/ACS',
+ 'Binding' => 'urn:oasis:names:tc:SAML:1.0:profiles:browser-post',
+ ],
+ ],
+ ];
+ $as = new SpTester($info, $config);
+
+ $md = $as->getHostedMetadata();
+
+ $protocols = $as->getSupportedProtocols();
+ $this->assertIsArray($protocols);
+ $this->assertCount(1, $protocols);
+ $this->assertEquals('urn:oasis:names:tc:SAML:2.0:protocol', $protocols[0]);
+ }
+
}
--
GitLab