From 1630c55e2b93ca3aeb7d7c45ef7c017a81c890c6 Mon Sep 17 00:00:00 2001
From: Tim van Dijen <tvdijen@gmail.com>
Date: Mon, 30 Aug 2021 23:50:37 +0200
Subject: [PATCH] Pull configuration out of factory
---
lib/SimpleSAML/SessionHandler.php | 5 ++-
lib/SimpleSAML/Store/StoreFactory.php | 7 ++-
modules/saml/lib/Auth/Source/SP.php | 5 ++-
modules/saml/lib/IdP/SQLNameID.php | 16 ++++---
modules/saml/lib/SP/LogoutStore.php | 11 ++++-
modules/saml/www/sp/metadata.php | 4 +-
modules/saml/www/sp/saml2-acs.php | 6 ++-
.../lib/SimpleSAML/Store/StoreFactoryTest.php | 43 +++++++++++++++----
tests/modules/saml/lib/IdP/SQLNameIDTest.php | 10 +++--
www/saml2/idp/ArtifactResolutionService.php | 3 +-
10 files changed, 81 insertions(+), 29 deletions(-)
diff --git a/lib/SimpleSAML/SessionHandler.php b/lib/SimpleSAML/SessionHandler.php
index b26bc16ad..c69e8f233 100644
--- a/lib/SimpleSAML/SessionHandler.php
+++ b/lib/SimpleSAML/SessionHandler.php
@@ -135,7 +135,10 @@ abstract class SessionHandler
*/
private static function createSessionHandler(): void
{
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
+ $store = StoreFactory::getInstance($storeType);
if ($store === false) {
self::$sessionHandler = new SessionHandlerPHP();
} else {
diff --git a/lib/SimpleSAML/Store/StoreFactory.php b/lib/SimpleSAML/Store/StoreFactory.php
index f3aa7ea76..9f3c93f49 100644
--- a/lib/SimpleSAML/Store/StoreFactory.php
+++ b/lib/SimpleSAML/Store/StoreFactory.php
@@ -30,19 +30,17 @@ abstract class StoreFactory implements Utils\ClearableState
/**
* Retrieve our singleton instance.
*
+ * @param string $storeType The type of store we need to instantiate
* @return \SimpleSAML\Store\StoreInterface|false The data store, or false if it isn't enabled.
*
* @throws \SimpleSAML\Error\CriticalConfigurationError
*/
- public static function getInstance()
+ public static function getInstance(string $storeType)
{
if (self::$instance !== null) {
return self::$instance;
}
- $config = Configuration::getInstance();
- $storeType = $config->getString('store.type', 'phpsession');
-
switch ($storeType) {
case 'phpsession':
// we cannot support advanced features with the PHP session store
@@ -62,6 +60,7 @@ abstract class StoreFactory implements Utils\ClearableState
try {
$className = Module::resolveClass($storeType, 'StoreInterface');
} catch (Exception $e) {
+ $config = Configuration::getInstance();
$c = $config->toArray();
$c['store.type'] = 'phpsession';
throw new Error\CriticalConfigurationError(
diff --git a/modules/saml/lib/Auth/Source/SP.php b/modules/saml/lib/Auth/Source/SP.php
index 0bfb57c54..93e40e922 100644
--- a/modules/saml/lib/Auth/Source/SP.php
+++ b/modules/saml/lib/Auth/Source/SP.php
@@ -386,7 +386,10 @@ class SP extends \SimpleSAML\Auth\Source
*/
private function getSLOEndpoints(): array
{
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
+ $store = StoreFactory::getInstance($storeType);
$bindings = $this->metadata->getArray(
'SingleLogoutServiceBinding',
[
diff --git a/modules/saml/lib/IdP/SQLNameID.php b/modules/saml/lib/IdP/SQLNameID.php
index 604da6d20..7a43e1016 100644
--- a/modules/saml/lib/IdP/SQLNameID.php
+++ b/modules/saml/lib/IdP/SQLNameID.php
@@ -158,12 +158,16 @@ class SQLNameID
*/
private static function getStore(): Store\SQLStore
{
- $store = StoreFactory::getInstance();
- if (!($store instanceof Store\SQLStore)) {
- throw new Error\Exception(
- 'SQL NameID store requires SimpleSAMLphp to be configured with a SQL datastore.'
- );
- }
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
+ $store = StoreFactory::getInstance($storeType);
+ Assert::isInstanceOf(
+ $store,
+ Store\SQLStore::class,
+ 'SQL NameID store requires SimpleSAMLphp to be configured with a SQL datastore.',
+ Error\Exception::class
+ );
return $store;
}
diff --git a/modules/saml/lib/SP/LogoutStore.php b/modules/saml/lib/SP/LogoutStore.php
index b021f2420..e6205ce6e 100644
--- a/modules/saml/lib/SP/LogoutStore.php
+++ b/modules/saml/lib/SP/LogoutStore.php
@@ -8,6 +8,7 @@ use Exception;
use PDO;
use SAML2\XML\saml\NameID;
use SimpleSAML\Assert\Assert;
+use SimpleSAML\Configuration;
use SimpleSAML\Logger;
use SimpleSAML\Session;
use SimpleSAML\Store;
@@ -209,7 +210,10 @@ class LogoutStore
$sessionIndex = $randomUtils->generateID();
}
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
+ $store = StoreFactory::getInstance($storeType);
if ($store === false) {
// We don't have a datastore.
return;
@@ -245,7 +249,10 @@ class LogoutStore
*/
public static function logoutSessions(string $authId, NameID $nameId, array $sessionIndexes)
{
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
+ $store = StoreFactory::getInstance($storeType);
if ($store === false) {
// We don't have a datastore
return false;
diff --git a/modules/saml/www/sp/metadata.php b/modules/saml/www/sp/metadata.php
index 473117602..4a9fabcb5 100644
--- a/modules/saml/www/sp/metadata.php
+++ b/modules/saml/www/sp/metadata.php
@@ -37,7 +37,9 @@ if (!($source instanceof Module\saml\Auth\Source\SP)) {
$entityId = $source->getEntityId();
$spconfig = $source->getMetadata();
$metaArray20 = $source->getHostedMetadata();
-$store = StoreFactory::getInstance();
+
+$storeType = $config->getString('store.type', 'phpsession');
+$store = StoreFactory::getInstance($storeType);
$metaBuilder = new Metadata\SAMLBuilder($entityId);
$metaBuilder->addMetadataSP20($metaArray20, $source->getSupportedProtocols());
diff --git a/modules/saml/www/sp/saml2-acs.php b/modules/saml/www/sp/saml2-acs.php
index 56bc5eedb..23aad3391 100644
--- a/modules/saml/www/sp/saml2-acs.php
+++ b/modules/saml/www/sp/saml2-acs.php
@@ -10,6 +10,7 @@ use SAML2\HTTPArtifact;
use SAML2\Response;
use SimpleSAML\Assert\Assert;
use SimpleSAML\Auth;
+use SimpleSAML\Configuration;
use SimpleSAML\Error;
use SimpleSAML\Module;
use SimpleSAML\Logger;
@@ -159,7 +160,10 @@ $foundAuthnStatement = false;
foreach ($assertions as $assertion) {
// check for duplicate assertion (replay attack)
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
+ $store = StoreFactory::getInstance($storeType);
if ($store !== false) {
$aID = $assertion->getId();
if ($store->get('saml.AssertionReceived', $aID) !== null) {
diff --git a/tests/lib/SimpleSAML/Store/StoreFactoryTest.php b/tests/lib/SimpleSAML/Store/StoreFactoryTest.php
index 347cf3992..3164d5f1c 100644
--- a/tests/lib/SimpleSAML/Store/StoreFactoryTest.php
+++ b/tests/lib/SimpleSAML/Store/StoreFactoryTest.php
@@ -31,8 +31,11 @@ class StoreFactoryTest extends TestCase
{
Configuration::loadFromArray([], '[ARRAY]', 'simplesaml');
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
/** @var false $store */
- $store = StoreFactory::getInstance();
+ $store = StoreFactory::getInstance($storeType);
$this->assertFalse($store);
}
@@ -43,10 +46,15 @@ class StoreFactoryTest extends TestCase
*/
public function phpSessionStore(): void
{
- Configuration::loadFromArray([], '[ARRAY]', 'simplesaml');
+ Configuration::loadFromArray([
+ 'store.type' => 'phpsession',
+ ], '[ARRAY]', 'simplesaml');
+
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
/** @var false $store */
- $store = StoreFactory::getInstance();
+ $store = StoreFactory::getInstance($storeType);
$this->assertFalse($store);
}
@@ -61,7 +69,10 @@ class StoreFactoryTest extends TestCase
'store.type' => 'memcache',
], '[ARRAY]', 'simplesaml');
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
+
+ $store = StoreFactory::getInstance($storeType);
$this->assertInstanceOf(Store\MemcacheStore::class, $store);
}
@@ -77,8 +88,11 @@ class StoreFactoryTest extends TestCase
'store.redis.prefix' => 'phpunit_',
], '[ARRAY]', 'simplesaml');
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
+
/** @psalm-var \SimpleSAML\Store\RedisStore $store */
- $store = StoreFactory::getInstance();
+ $store = StoreFactory::getInstance($storeType);
$store->redis = $this->getMockBuilder(Client::class)
->setMethods(['get', 'set', 'setex', 'del', 'disconnect', '__destruct'])
->disableOriginalConstructor()
@@ -99,7 +113,10 @@ class StoreFactoryTest extends TestCase
'store.sql.prefix' => 'phpunit_',
], '[ARRAY]', 'simplesaml');
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
+
+ $store = StoreFactory::getInstance($storeType);
$this->assertInstanceOf(Store\SQLStore::class, $store);
}
@@ -116,7 +133,10 @@ class StoreFactoryTest extends TestCase
'store.sql.prefix' => 'phpunit_',
], '[ARRAY]', 'simplesaml');
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
+
+ $store = StoreFactory::getInstance($storeType);
$this->assertInstanceOf(Store\SQLStore::class, $store);
}
@@ -134,7 +154,10 @@ class StoreFactoryTest extends TestCase
'store.sql.prefix' => 'phpunit_',
], '[ARRAY]', 'simplesaml');
- StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
+
+ StoreFactory::getInstance($storeType);
}
@@ -143,8 +166,10 @@ class StoreFactoryTest extends TestCase
protected function tearDown(): void
{
$config = Configuration::getInstance();
+ $storeType = $config->getString('store.type', 'phpsession');
+
/** @var \SimpleSAML\Store\StoreInterface $store */
- $store = StoreFactory::getInstance();
+ $store = StoreFactory::getInstance($storeType);
$this->clearInstance($config, Configuration::class);
$this->clearInstance($store, StoreFactory::class);
diff --git a/tests/modules/saml/lib/IdP/SQLNameIDTest.php b/tests/modules/saml/lib/IdP/SQLNameIDTest.php
index 098a68702..e12629ae7 100644
--- a/tests/modules/saml/lib/IdP/SQLNameIDTest.php
+++ b/tests/modules/saml/lib/IdP/SQLNameIDTest.php
@@ -46,8 +46,9 @@ class SQLNameIDTest extends TestCase
], '[ARRAY]', 'simplesaml');
$this->addGetDelete();
$config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
/** @var \SimpleSAML\Store\StoreInterface $store */
- $store = StoreFactory::getInstance();
+ $store = StoreFactory::getInstance($storeType);
$this->clearInstance($config, Configuration::class);
$this->clearInstance($store, StoreFactory::class);
}
@@ -62,13 +63,16 @@ class SQLNameIDTest extends TestCase
Configuration::loadFromArray([
'store.type' => 'memcache',
], '[ARRAY]', 'simplesaml');
- $store = StoreFactory::getInstance();
+ $config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
+ $store = StoreFactory::getInstance($storeType);
$this->assertInstanceOf(Store\MemcacheStore::class, $store);
$this->expectException(Error\Exception::class);
$this->addGetDelete();
$config = Configuration::getInstance();
+ $storeType = $config->getString('store.type');
/** @var \SimpleSAML\Store\StoreInterface $store */
- $store = StoreFactory::getInstance();
+ $store = StoreFactory::getInstance($storeType);
$this->clearInstance($config, Configuration::class);
$this->clearInstance($store, StoreFactory::class);
}
diff --git a/www/saml2/idp/ArtifactResolutionService.php b/www/saml2/idp/ArtifactResolutionService.php
index be3d121db..5e91f3fc3 100644
--- a/www/saml2/idp/ArtifactResolutionService.php
+++ b/www/saml2/idp/ArtifactResolutionService.php
@@ -35,7 +35,8 @@ if (!$idpMetadata->getBoolean('saml20.sendartifact', false)) {
throw new Error\Error('NOACCESS');
}
-$store = StoreFactory::getInstance();
+$storeType = $config->getString('store.type', 'phpsession');
+$store = StoreFactory::getInstance($storeType);
if ($store === false) {
throw new Exception('Unable to send artifact without a datastore configured.');
}
--
GitLab