From a20debe73c123fee05e6de201028afab1d1eaaba Mon Sep 17 00:00:00 2001
From: Thijs Kinkhorst <thijs@kinkhorst.com>
Date: Wed, 24 Aug 2022 19:25:35 +0000
Subject: [PATCH] Repair broken (but passing) tests and add more coverage
The protectindexpage test was broken and never changed the
protectindexpage setting, but the logic was also broken so
it always passed and never got past the protected index
page guard in the controller so not a lot of code in the
controller was run.
Also add more tests for the metadata controllers.
---
modules/saml/src/Controller/Metadata.php | 17 +-
modules/saml/src/IdP/SAML2.php | 8 +-
src/SimpleSAML/Auth/ProcessingChain.php | 3 +-
.../saml/src/Controller/MetadataTest.php | 209 ++++++++++++++++--
.../src/Controller/ServiceProviderTest.php | 94 ++++++--
5 files changed, 291 insertions(+), 40 deletions(-)
diff --git a/modules/saml/src/Controller/Metadata.php b/modules/saml/src/Controller/Metadata.php
index ceae302e0..1498c82a8 100644
--- a/modules/saml/src/Controller/Metadata.php
+++ b/modules/saml/src/Controller/Metadata.php
@@ -9,6 +9,7 @@ use SimpleSAML\Configuration;
use SimpleSAML\Error;
use SimpleSAML\HTTP\RunnableResponse;
use SimpleSAML\Metadata as SSPMetadata;
+use SimpleSAML\Metadata\MetaDataStorageHandler;
use SimpleSAML\Module;
use SimpleSAML\Module\saml\IdP\SAML2 as SAML2_IdP;
use SimpleSAML\Utils;
@@ -34,6 +35,7 @@ class Metadata
/** @var \SimpleSAML\Utils\Auth */
protected Utils\Auth $authUtils;
+ protected MetadataStorageHandler $mdHandler;
/**
* Controller constructor.
@@ -47,9 +49,9 @@ class Metadata
) {
$this->config = $config;
$this->authUtils = new Utils\Auth();
+ $this->mdHandler = MetaDataStorageHandler::getMetadataHandler();
}
-
/**
* Inject the \SimpleSAML\Utils\Auth dependency.
*
@@ -60,6 +62,13 @@ class Metadata
$this->authUtils = $authUtils;
}
+ /**
+ * Inject the \SimpleSAML\Metadata\MetadataStorageHandler dependency.
+ */
+ public function setMetadataStorageHandler(MetadataStorageHandler $mdHandler): void
+ {
+ $this->mdHandler = $mdHandler;
+ }
/**
* This endpoint will offer the SAML 2.0 IdP metadata.
@@ -78,15 +87,13 @@ class Metadata
return new RunnableResponse([$this->authUtils, 'requireAdmin']);
}
- $metadata = SSPMetadata\MetaDataStorageHandler::getMetadataHandler();
-
try {
if ($request->query->has('idpentityid')) {
$idpentityid = $request->query->get('idpentityid');
} else {
- $idpentityid = $metadata->getMetaDataCurrentEntityID('saml20-idp-hosted');
+ $idpentityid = $this->mdHandler->getMetaDataCurrentEntityID('saml20-idp-hosted');
}
- $metaArray = SAML2_IdP::getHostedMetadata($idpentityid);
+ $metaArray = SAML2_IdP::getHostedMetadata($idpentityid, $this->mdHandler);
$metaBuilder = new SSPMetadata\SAMLBuilder($idpentityid);
$metaBuilder->addMetadataIdP20($metaArray);
diff --git a/modules/saml/src/IdP/SAML2.php b/modules/saml/src/IdP/SAML2.php
index 3fb76aa8d..54ff1a12e 100644
--- a/modules/saml/src/IdP/SAML2.php
+++ b/modules/saml/src/IdP/SAML2.php
@@ -747,15 +747,19 @@ class SAML2
* Retrieve the metadata of a hosted SAML 2 IdP.
*
* @param string $entityid The entity ID of the hosted SAML 2 IdP whose metadata we want.
+ * @param MetaDataStorageHandler $handler Optionally the metadata storage to use,
+ * if omitted the configured handler will be used.
*
* @return array
* @throws \SimpleSAML\Error\CriticalConfigurationError
* @throws \SimpleSAML\Error\Exception
* @throws \SimpleSAML\Error\MetadataNotFound
*/
- public static function getHostedMetadata(string $entityid): array
+ public static function getHostedMetadata(string $entityid, MetaDataStorageHandler $handler = null): array
{
- $handler = MetaDataStorageHandler::getMetadataHandler();
+ if ($handler === null) {
+ $handler = MetaDataStorageHandler::getMetadataHandler();
+ }
$config = $handler->getMetaDataConfig($entityid, 'saml20-idp-hosted');
// configure endpoints
diff --git a/src/SimpleSAML/Auth/ProcessingChain.php b/src/SimpleSAML/Auth/ProcessingChain.php
index 262e95744..c6a829cda 100644
--- a/src/SimpleSAML/Auth/ProcessingChain.php
+++ b/src/SimpleSAML/Auth/ProcessingChain.php
@@ -126,7 +126,8 @@ class ProcessingChain
if (!is_array($filter)) {
throw new Exception(
- "Invalid authentication processing filter configuration: One of the filters wasn't a string or an array."
+ "Invalid authentication processing filter configuration: " .
+ "One of the filters wasn't a string or an array."
);
}
diff --git a/tests/modules/saml/src/Controller/MetadataTest.php b/tests/modules/saml/src/Controller/MetadataTest.php
index f507c9e55..f106822b5 100644
--- a/tests/modules/saml/src/Controller/MetadataTest.php
+++ b/tests/modules/saml/src/Controller/MetadataTest.php
@@ -6,7 +6,9 @@ namespace SimpleSAML\Test\Module\saml\Controller;
use PHPUnit\Framework\TestCase;
use SimpleSAML\Configuration;
+use SimpleSAML\Error;
use SimpleSAML\HTTP\RunnableResponse;
+use SimpleSAML\Metadata\MetaDataStorageHandler;
use SimpleSAML\Module\saml\Controller;
use SimpleSAML\Session;
use SimpleSAML\Utils;
@@ -30,7 +32,7 @@ class MetadataTest extends TestCase
/** @var \SimpleSAML\Utils\Auth */
protected Utils\Auth $authUtils;
-
+ protected MetaDataStorageHandler $mdh;
/**
* Set up for each test.
*/
@@ -38,13 +40,69 @@ class MetadataTest extends TestCase
{
parent::setUp();
+ $this->mdh = new class () extends MetaDataStorageHandler {
+ /** @var string */
+ private const XMLSEC = '../vendor/simplesamlphp/xml-security/tests/resources';
+
+ /** @var string */
+ public const CERT_KEY = self::XMLSEC . '/certificates/rsa-pem/selfsigned.simplesamlphp.org.key';
+
+ /** @var string */
+ public const CERT_PUBLIC = self::XMLSEC . '/certificates/rsa-pem/selfsigned.simplesamlphp.org.crt';
+
+ private array $idps;
+
+ public function __construct()
+ {
+ $this->idps = [
+ 'urn:example:simplesaml:idp' => [
+ 'name' => 'SimpleSAMLphp Hosted IDP',
+ 'descr' => 'The local IDP',
+ 'OrganizationDisplayName' => ['en' => 'My IDP', 'nl' => 'Mijn IDP'],
+ 'certificate' => self::CERT_PUBLIC,
+ 'privatekey' => self::CERT_KEY,
+
+ ],
+ 'urn:example:simplesaml:another:idp' => [
+ 'name' => 'SimpleSAMLphp Hosted Another IDP',
+ 'descr' => 'Different IDP',
+ 'OrganizationDisplayName' => ['en' => 'Other IDP', 'nl' => 'Andere IDP'],
+ 'certificate' => self::CERT_PUBLIC,
+ 'privatekey' => self::CERT_KEY,
+
+ ],
+ ];
+ }
+
+ public function getMetaData(?string $entityId, string $set): array
+ {
+ if (isset($this->idps[$entityId]) && $set === 'saml20-idp-hosted') {
+ return $this->idps[$entityId];
+ }
+
+ throw new Error\MetadataNotFound($entityId ?? '');
+ }
+
+ public function getList(string $set = 'saml20-idp-remote', bool $showExpired = false): array
+ {
+ if ($set === 'saml20-idp-hosted') {
+ return $idps;
+ }
+ return [];
+ }
+
+ public function getMetaDataCurrentEntityID(string $set, string $type = 'entityid'): string
+ {
+ return 'urn:example:simplesaml:another:idp';
+ }
+ };
$this->session = Session::getSessionFromRequest();
$this->config = Configuration::loadFromArray(
[
'module.enable' => ['saml' => true],
'enable.saml20-idp' => true,
- 'admin.protectmetadata' => true,
+ 'admin.protectmetadata' => false,
],
'[ARRAY]',
'simplesaml'
@@ -70,6 +128,8 @@ class MetadataTest extends TestCase
// stub
}
};
+
+ $_SERVER['REQUEST_URI'] = '/dummy';
}
@@ -78,39 +138,43 @@ class MetadataTest extends TestCase
* and admin.protectmetadata set to true or false is handled properly
*
* @dataProvider provideMetadataAccess
- * @param bool $protected
- * @param bool $authenticated
- * @return void
*/
public function testMetadataAccess(bool $authenticated, bool $protected): void
{
+ $config = Configuration::loadFromArray(
+ [
+ 'module.enable' => ['saml' => true],
+ 'enable.saml20-idp' => true,
+ 'admin.protectmetadata' => $protected,
+ ],
+ '[ARRAY]',
+ 'simplesaml'
+ );
+ Configuration::setPreLoadedConfig($config, 'config.php');
+
$request = Request::create(
'/idp/metadata',
'GET',
);
- $c = new Controller\Metadata($this->config, $this->session);
+ $c = new Controller\Metadata($config, $this->session);
+ $c->setMetadataStorageHandler($this->mdh);
- if ($authenticated === true || $protected === false) {
+ if ($authenticated === true) {
// Bypass authentication - mock being authenticated
$c->setAuthUtils($this->authUtils);
}
$result = $c->metadata($request);
- if ($authenticated !== false && $protected !== true) {
- // ($authenticated === true) or ($protected === false)
- // Should lead to a Response
- $this->assertInstanceOf(Response::class, $result);
- } else {
+ if ($protected && !$authenticated) {
$this->assertInstanceOf(RunnableResponse::class, $result);
+ $this->assertEquals("requireAdmin", $result->getCallable()[1]);
+ } else {
+ $this->assertInstanceOf(Response::class, $result);
}
}
-
- /**
- * @return array
- */
public function provideMetadataAccess(): array
{
return [
@@ -121,4 +185,117 @@ class MetadataTest extends TestCase
[true, true],
];
}
+
+ /**
+ * Test that saml20-idp setting disabled disables access
+ */
+ public function testDisabledSAML20IDPReturnsNoAccess(): void
+ {
+ $config = Configuration::loadFromArray(
+ [
+ 'module.enable' => ['saml' => true],
+ 'enable.saml20-idp' => false,
+ ],
+ '[ARRAY]',
+ 'simplesaml'
+ );
+ Configuration::setPreLoadedConfig($config, 'config.php');
+
+ $request = Request::create(
+ '/idp/metadata',
+ 'GET',
+ );
+
+ $c = new Controller\Metadata($config, $this->session);
+
+ $this->expectException(\SimpleSAML\Error\Error::class);
+ $this->expectExceptionMessage('NOACCESS');
+ $result = $c->metadata($request);
+ }
+
+ /**
+ * Test that requesting a non-existing entityID throws an exception
+ */
+ public function testMetadataUnknownEntityThrowsError(): void
+ {
+ $request = Request::create(
+ '/idp/metadata?idpentityid=https://example.org/notexist',
+ 'GET',
+ );
+
+ $c = new Controller\Metadata($this->config, $this->session);
+ $c->setMetadataStorageHandler($this->mdh);
+
+ $this->expectException(\SimpleSAML\Error\Error::class);
+ $this->expectExceptionMessage('METADATA');
+ $result = $c->metadata($request);
+ }
+
+ /**
+ * Basic smoke test of generated metadata
+ */
+ public function testMetadataYieldsContent(): void
+ {
+ $request = Request::create(
+ '/idp/metadata?idpentityid=urn:example:simplesaml:idp',
+ 'GET',
+ );
+
+ $c = new Controller\Metadata($this->config, $this->session);
+ $c->setMetadataStorageHandler($this->mdh);
+
+ $result = $c->metadata($request);
+ $this->assertEquals('application/samlmetadata+xml', $result->headers->get('Content-Type'));
+ $this->assertEquals('attachment; filename="idp-metadata.xml"', $result->headers->get('Content-Disposition'));
+ $content = $result->getContent();
+
+ $expect = 'entityID="urn:example:simplesaml:idp"';
+ $this->assertStringContainsString($expect, $content);
+
+ $expect = '<md:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location=';
+ $this->assertStringContainsString($expect, $content);
+ }
+
+ /**
+ * Test not specifying explict entityID falls back to a default
+ */
+ public function testMetadataDefaultIdPYieldsContent(): void
+ {
+ $request = Request::create(
+ '/idp/metadata',
+ 'GET',
+ );
+
+ $c = new Controller\Metadata($this->config, $this->session);
+ $c->setMetadataStorageHandler($this->mdh);
+
+ $result = $c->metadata($request);
+ $this->assertEquals('application/samlmetadata+xml', $result->headers->get('Content-Type'));
+ $this->assertEquals('attachment; filename="idp-metadata.xml"', $result->headers->get('Content-Disposition'));
+ $content = $result->getContent();
+
+ $expect = 'entityID="urn:example:simplesaml:another:idp"';
+ $this->assertStringContainsString($expect, $content);
+
+ $expect = '<md:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location=';
+ $this->assertStringContainsString($expect, $content);
+ }
+
+ /**
+ * Check if caching headers are set
+ */
+ public function testMetadataCachingHeaders(): void
+ {
+ $request = Request::create(
+ '/idp/metadata',
+ 'GET',
+ );
+
+ $c = new Controller\Metadata($this->config, $this->session);
+ $c->setMetadataStorageHandler($this->mdh);
+
+ $result = $c->metadata($request);
+ $this->assertTrue($result->headers->has('ETag'));
+ $this->assertEquals('public', $result->headers->get('Cache-Control'));
+ }
}
diff --git a/tests/modules/saml/src/Controller/ServiceProviderTest.php b/tests/modules/saml/src/Controller/ServiceProviderTest.php
index 5636553c5..0ca01986f 100644
--- a/tests/modules/saml/src/Controller/ServiceProviderTest.php
+++ b/tests/modules/saml/src/Controller/ServiceProviderTest.php
@@ -34,7 +34,6 @@ class ServiceProviderTest extends TestCase
/** @var \SimpleSAML\Utils\Auth */
protected Utils\Auth $authUtils;
-
/**
* Set up for each test.
*/
@@ -46,7 +45,7 @@ class ServiceProviderTest extends TestCase
$this->config = Configuration::loadFromArray(
[
'module.enable' => ['saml' => true],
- 'admin.protectmetadata' => true,
+ 'admin.protectmetadata' => false,
],
'[ARRAY]',
'simplesaml'
@@ -75,6 +74,8 @@ class ServiceProviderTest extends TestCase
// stub
}
};
+
+ $_SERVER['REQUEST_URI'] = '/dummy';
}
@@ -398,34 +399,41 @@ XML;
* and admin.protectmetadata set to true or false is handled properly
*
* @dataProvider provideMetadataAccess
- * @param bool $protected
- * @param bool $authenticated
- * @return void
*/
public function testMetadataAccess(bool $authenticated, bool $protected): void
{
- $c = new Controller\ServiceProvider($this->config, $this->session);
+ $config = Configuration::loadFromArray(
+ [
+ 'module.enable' => ['saml' => true],
+ 'admin.protectmetadata' => $protected,
+ ],
+ '[ARRAY]',
+ 'simplesaml'
+ );
+ Configuration::setPreLoadedConfig($config, 'config.php');
+
+ $request = Request::create(
+ '/sp/metadata/phpunit',
+ 'GET',
+ );
+
+ $c = new Controller\ServiceProvider($config, $this->session);
if ($authenticated === true || $protected === false) {
// Bypass authentication - mock being authenticated
$c->setAuthUtils($this->authUtils);
}
- $result = $c->metadata('phpunit');
+ $result = $c->metadata($request, 'phpunit');
- if ($authenticated !== false && $protected !== true) {
- // ($authenticated === true) or ($protected === false)
- // Should lead to a Response
- $this->assertInstanceOf(Response::class, $result);
- } else {
+ if ($protected && !$authenticated) {
$this->assertInstanceOf(RunnableResponse::class, $result);
+ $this->assertEquals("requireAdmin", $result->getCallable()[1]);
+ } else {
+ $this->assertInstanceOf(Response::class, $result);
}
}
-
- /**
- * @return array
- */
public function provideMetadataAccess(): array
{
return [
@@ -436,4 +444,58 @@ XML;
[true, true],
];
}
+
+ /**
+ * Test that requesting a non-existing authsource yields an error
+ */
+ public function testMetadataUnknownEntityThrowsError(): void
+ {
+ $request = Request::create(
+ '/sp/metadata/phpnonunit',
+ 'GET',
+ );
+
+ $c = new Controller\ServiceProvider($this->config, $this->session);
+
+ $this->expectException(\SimpleSAML\Error\Error::class);
+ $this->expectExceptionMessage("Error with authentication source 'phpnonunit': Could not find authentication source.");
+ $result = $c->metadata($request, 'phpnonunit');
+ }
+
+ /**
+ * Basic smoke test of generated metadata
+ */
+ public function testMetadataYieldsContent(): void
+ {
+ $request = Request::create(
+ '/sp/metadata/phpunit',
+ 'GET',
+ );
+
+ $c = new Controller\ServiceProvider($this->config, $this->session);
+
+ $result = $c->metadata($request, 'phpunit');
+ $this->assertEquals('application/samlmetadata+xml', $result->headers->get('Content-Type'));
+ $this->assertEquals('attachment; filename="phpunit.xml"', $result->headers->get('Content-Disposition'));
+ $content = $result->getContent();
+ $expect = 'entityID="urn:x-simplesamlphp:example-sp"';
+ $this->assertStringContainsString($expect, $content);
+ }
+
+ /**
+ * Check if caching headers are set
+ */
+ public function testMetadataCachingHeaders(): void
+ {
+ $request = Request::create(
+ '/sp/metadata/phpunit',
+ 'GET',
+ );
+
+ $c = new Controller\ServiceProvider($this->config, $this->session);
+
+ $result = $c->metadata($request, 'phpunit');
+ $this->assertTrue($result->headers->has('ETag'));
+ $this->assertEquals('public', $result->headers->get('Cache-Control'));
+ }
}
--
GitLab