From 95bd0336007528151027e1703943086ac69470a3 Mon Sep 17 00:00:00 2001
From: Tim van Dijen <tim.dijen@minbzk.nl>
Date: Fri, 21 Aug 2020 18:04:24 +0200
Subject: [PATCH] Replace userid.attribute with a config setting local to this
authpproc (#1363)
---
docs/simplesamlphp-upgrade-notes-2.0.md | 3 +-
modules/core/docs/authproc_targetedid.md | 2 +
modules/core/lib/Auth/Process/TargetedID.php | 95 ++---
modules/saml/lib/IdP/SAML2.php | 19 +-
.../core/lib/Auth/Process/TargetedIDTest.php | 377 +++++++++++-------
5 files changed, 282 insertions(+), 214 deletions(-)
diff --git a/docs/simplesamlphp-upgrade-notes-2.0.md b/docs/simplesamlphp-upgrade-notes-2.0.md
index 53f322d21..4d7fc489a 100644
--- a/docs/simplesamlphp-upgrade-notes-2.0.md
+++ b/docs/simplesamlphp-upgrade-notes-2.0.md
@@ -1,7 +1,7 @@
Upgrade notes for SimpleSAMLphp 2.0
====================================
-- The minimum PHP version required is now PHP 7.2.
+- The minimum PHP version required is now PHP 7.4.
- Old JSON-formatted dictionaries have been replaced by gettext / .po-files;
You can find a migration guide here: https://github.com/simplesamlphp/simplesamlphp/wiki/Migrating-translations-(pre-migration)
- Old PHP templates have been replaced by Twig-templates; you can find a migration
@@ -10,3 +10,4 @@ Upgrade notes for SimpleSAMLphp 2.0
make sure you change them to reflect the method signatures of the base classes.
- If you used some of the modules that were shipped with SimpleSAMLphp, you now have to manually install them using Composer;
For example, to use the ldap-module: bin/composer.phar require simplesamlphp/simplesamlphp-module-ldap --update-no-dev
+- If you're using the core:TargetedID authproc-filter, note that the `attributename` setting has been renamed to `identifyingAttribute`.
diff --git a/modules/core/docs/authproc_targetedid.md b/modules/core/docs/authproc_targetedid.md
index f6cea7da7..9f30500c6 100644
--- a/modules/core/docs/authproc_targetedid.md
+++ b/modules/core/docs/authproc_targetedid.md
@@ -13,6 +13,8 @@ Parameters
: The name of the attribute we should use for the unique user identifier.
Optional, will use the attribute set by the `userid.attribute` metadata option by default.
+ Note: only the first value of the specified attribute is being used for the generation of the identifier.
+
`nameId`
: Set this option to `TRUE` to generate the attribute as in SAML 2 NameID format.
This can be used to generate an Internet2 compatible `eduPersonTargetedID` attribute.
diff --git a/modules/core/lib/Auth/Process/TargetedID.php b/modules/core/lib/Auth/Process/TargetedID.php
index 6aff8258d..c66cef40f 100644
--- a/modules/core/lib/Auth/Process/TargetedID.php
+++ b/modules/core/lib/Auth/Process/TargetedID.php
@@ -15,25 +15,17 @@ use SimpleSAML\Utils;
* Filter to generate the eduPersonTargetedID attribute.
*
* By default, this filter will generate the ID based on the UserID of the current user.
- * This is by default generated from the attribute configured in 'userid.attribute' in the
- * metadata. If this attribute isn't present, the userid will be generated from the
- * eduPersonPrincipalName attribute, if it is present.
+ * This is generated from the attribute configured in 'identifyingAttribute' in the
+ * authproc-configuration.
*
- * It is possible to generate this attribute from another attribute by specifying this attribute
- * in this configuration.
- *
- * Example - generate from user ID:
- * <code>
- * 'authproc' => array(
- * 50 => 'core:TargetedID',
- * )
- * </code>
- *
- * Example - generate from mail-attribute:
+ * Example - generate from attribute:
* <code>
- * 'authproc' => array(
- * 50 => array('class' => 'core:TargetedID' , 'attributename' => 'mail'),
- * ),
+ * 'authproc' => [
+ * 50 => [
+ * 'core:TargetedID',
+ * 'identifyingAttribute' => 'mail',
+ * ]
+ * ]
* </code>
*
* @author Olav Morken, UNINETT AS.
@@ -42,12 +34,11 @@ use SimpleSAML\Utils;
class TargetedID extends Auth\ProcessingFilter
{
/**
- * The attribute we should generate the targeted id from, or NULL if we should use the
- * UserID.
+ * The attribute we should generate the targeted id from.
*
- * @var string|null
+ * @var string
*/
- private $attribute = null;
+ private $identifyingAttribute;
/**
* Whether the attribute should be generated as a NameID value, or as a simple string.
@@ -56,6 +47,11 @@ class TargetedID extends Auth\ProcessingFilter
*/
private $generateNameId = false;
+ /**
+ * @var \SimpleSAML\Utils\Config|string
+ * @psalm-var \SimpleSAML\Utils\Config|class-string
+ */
+ protected $configUtils = Utils\Config::class;
/**
* Initialize this filter.
@@ -67,11 +63,15 @@ class TargetedID extends Auth\ProcessingFilter
{
parent::__construct($config, $reserved);
- if (array_key_exists('attributename', $config)) {
- $this->attribute = $config['attributename'];
- if (!is_string($this->attribute)) {
- throw new Exception('Invalid attribute name given to core:TargetedID filter.');
- }
+ Assert::keyExists($config, 'identifyingAttribute', "Missing mandatory 'identifyingAttribute' config setting.");
+ Assert::stringNotEmpty(
+ $config['identifyingAttribute'],
+ "TargetedID: 'identifyingAttribute' must be a non-empty string."
+ );
+
+ $this->identifyingAttribute = $config['identifyingAttribute'];
+ if (!is_string($this->identifyingAttribute)) {
+ throw new Exception('Invalid attribute name given to core:TargetedID filter.');
}
if (array_key_exists('nameId', $config)) {
@@ -83,6 +83,17 @@ class TargetedID extends Auth\ProcessingFilter
}
+ /**
+ * Inject the \SimpleSAML\Utils\Config dependency.
+ *
+ * @param \SimpleSAML\Utils\Config $configUtils
+ */
+ public function setConfigUtils(Utils\Config $configUtils): void
+ {
+ $this->configUtils = $configUtils;
+ }
+
+
/**
* Apply filter to add the targeted ID.
*
@@ -92,26 +103,17 @@ class TargetedID extends Auth\ProcessingFilter
public function process(array &$state): void
{
Assert::keyExists($state, 'Attributes');
-
- if ($this->attribute === null) {
- if (!array_key_exists('UserID', $state)) {
- throw new Exception('core:TargetedID: Missing UserID for this user. Please' .
- ' check the \'userid.attribute\' option in the metadata against the' .
- ' attributes provided by the authentication source.');
- }
-
- $userID = $state['UserID'];
- } else {
- if (!array_key_exists($this->attribute, $state['Attributes'])) {
- throw new Exception('core:TargetedID: Missing attribute \'' . $this->attribute .
- '\', which is needed to generate the targeted ID.');
- }
-
- $userID = $state['Attributes'][$this->attribute][0];
- }
-
-
- $secretSalt = Utils\Config::getSecretSalt();
+ Assert::keyExists(
+ $state['Attributes'],
+ $this->identifyingAttribute,
+ sprintf(
+ "core:TargetedID: Missing attribute '%s', which is needed to generate the targeted ID.",
+ $this->identifyingAttribute
+ )
+ );
+
+ $userID = $state['Attributes'][$this->identifyingAttribute][0];
+ Assert::stringNotEmpty($userID);
if (array_key_exists('Source', $state)) {
$srcID = self::getEntityId($state['Source']);
@@ -125,6 +127,7 @@ class TargetedID extends Auth\ProcessingFilter
$dstID = '';
}
+ $secretSalt = $this->configUtils::getSecretSalt();
$uidData = 'uidhashbase' . $secretSalt;
$uidData .= strlen($srcID) . ':' . $srcID;
$uidData .= strlen($dstID) . ':' . $dstID;
diff --git a/modules/saml/lib/IdP/SAML2.php b/modules/saml/lib/IdP/SAML2.php
index 13baa121e..e4814db36 100644
--- a/modules/saml/lib/IdP/SAML2.php
+++ b/modules/saml/lib/IdP/SAML2.php
@@ -936,23 +936,8 @@ class SAML2
if ($attribute === null) {
$attribute = $idpMetadata->getString('simplesaml.nameidattribute', null);
if ($attribute === null) {
- if (!isset($state['UserID'])) {
- Logger::error('Unable to generate NameID. Check the userid.attribute option.');
- return null;
- }
- $attributeValue = $state['UserID'];
- $idpEntityId = $idpMetadata->getString('entityid');
- $spEntityId = $spMetadata->getString('entityid');
-
- $secretSalt = Utils\Config::getSecretSalt();
-
- $uidData = 'uidhashbase' . $secretSalt;
- $uidData .= strlen($idpEntityId) . ':' . $idpEntityId;
- $uidData .= strlen($spEntityId) . ':' . $spEntityId;
- $uidData .= strlen($attributeValue) . ':' . $attributeValue;
- $uidData .= $secretSalt;
-
- return hash('sha1', $uidData);
+ Logger::error('Unable to generate NameID. Check the simplesaml.nameidattribute option.');
+ return null;
}
}
diff --git a/tests/modules/core/lib/Auth/Process/TargetedIDTest.php b/tests/modules/core/lib/Auth/Process/TargetedIDTest.php
index ff79c21b4..148d7541f 100644
--- a/tests/modules/core/lib/Auth/Process/TargetedIDTest.php
+++ b/tests/modules/core/lib/Auth/Process/TargetedIDTest.php
@@ -6,13 +6,41 @@ namespace SimpleSAML\Test\Module\core\Auth\Process;
use Exception;
use PHPUnit\Framework\TestCase;
+use SAML2\Constants;
+use SAML2\XML\saml\NameID;
+use SimpleSAML\Configuration;
use SimpleSAML\Module\core\Auth\Process\TargetedID;
+use SimpleSAML\Utils;
/**
* Test for the core:TargetedID filter.
*/
class TargetedIDTest extends TestCase
{
+ /** @var \SimpleSAML\Configuration */
+ protected $config;
+
+ /** @var \SimpleSAML\Utils\Config */
+ protected static $configUtils;
+
+ /**
+ * Set up for each test.
+ * @return void
+ */
+ protected function setUp(): void
+ {
+ parent::setUp();
+
+ self::$configUtils = new class () extends Utils\Config {
+ public static function getSecretSalt(): string
+ {
+ // stub
+ return 'secretsalt';
+ }
+ };
+ }
+
+
/**
* Helper function to run the filter with a given configuration.
*
@@ -23,158 +51,207 @@ class TargetedIDTest extends TestCase
private static function processFilter(array $config, array $request): array
{
$filter = new TargetedID($config, null);
+ $filter->setConfigUtils(self::$configUtils);
$filter->process($request);
return $request;
}
-// /**
-// * Test the most basic functionality
-// * @return void
-// */
-// public function testBasic()
-// {
-// $config = [];
-// $request = [
-// 'Attributes' => [],
-// 'UserID' => 'user2@example.org',
-// ];
-// $result = self::processFilter($config, $request);
-// $attributes = $result['Attributes'];
-// $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
-// $this->assertRegExp('/^[0-9a-f]{40}$/', $attributes['eduPersonTargetedID'][0]);
-// }
-//
-//
-// /**
-// * Test with src and dst entityIds.
-// * Make sure to overwrite any present eduPersonTargetedId
-// * @return void
-// */
-// public function testWithSrcDst()
-// {
-// $config = [];
-// $request = [
-// 'Attributes' => [
-// 'eduPersonTargetedID' => 'dummy',
-// ],
-// 'UserID' => 'user2@example.org',
-// 'Source' => [
-// 'metadata-set' => 'saml20-idp-hosted',
-// 'entityid' => 'urn:example:src:id',
-// ],
-// 'Destination' => [
-// 'metadata-set' => 'saml20-sp-remote',
-// 'entityid' => 'joe',
-// ],
-// ];
-// $result = self::processFilter($config, $request);
-// $attributes = $result['Attributes'];
-// $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
-// $this->assertRegExp('/^[0-9a-f]{40}$/', $attributes['eduPersonTargetedID'][0]);
-// }
-//
-//
-// /**
-// * Test with nameId config option set.
-// * @return void
-// */
-// public function testNameIdGeneration()
-// {
-// $config = [
-// 'nameId' => true,
-// ];
-// $request = array(
-// 'Attributes' => [],
-// 'UserID' => 'user2@example.org',
-// 'Source' => [
-// 'metadata-set' => 'saml20-idp-hosted',
-// 'entityid' => 'urn:example:src:id',
-// ],
-// 'Destination' => [
-// 'metadata-set' => 'saml20-sp-remote',
-// 'entityid' => 'joe',
-// ],
-// );
-// $result = self::processFilter($config, $request);
-// $attributes = $result['Attributes'];
-// $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
-// $this->assertRegExp(
-// '#^<saml:NameID xmlns:saml="urn:oasis:names:tc:SAML:2\.0:assertion" NameQualifier="urn:example:src:id"' .
-// ' SPNameQualifier="joe"' .
-// ' Format="urn:oasis:names:tc:SAML:2\.0:nameid-format:persistent">[0-9a-f]{40}</saml:NameID>$#',
-// $attributes['eduPersonTargetedID'][0]
-// );
-// }
-//
-//
-// /**
-// * Test that Id is the same for subsequent invocations with same input.
-// * @return void
-// */
-// public function testIdIsPersistent()
-// {
-// $config = [];
-// $request = [
-// 'Attributes' => [
-// 'eduPersonTargetedID' => 'dummy',
-// ],
-// 'UserID' => 'user2@example.org',
-// 'Source' => [
-// 'metadata-set' => 'saml20-idp-hosted',
-// 'entityid' => 'urn:example:src:id',
-// ],
-// 'Destination' => [
-// 'metadata-set' => 'saml20-sp-remote',
-// 'entityid' => 'joe',
-// ],
-// ];
-// for ($i = 0; $i < 10; ++$i) {
-// $result = self::processFilter($config, $request);
-// $attributes = $result['Attributes'];
-// $tid = $attributes['eduPersonTargetedID'][0];
-// if (isset($prevtid)) {
-// $this->assertEquals($prevtid, $tid);
-// $prevtid = $tid;
-// }
-// }
-// }
-//
-//
-// /**
-// * Test that Id is different for two different usernames and two different sp's
-// * @return void
-// */
-// public function testIdIsUnique()
-// {
-// $config = [];
-// $request = [
-// 'Attributes' => [],
-// 'UserID' => 'user2@example.org',
-// 'Source' => [
-// 'metadata-set' => 'saml20-idp-hosted',
-// 'entityid' => 'urn:example:src:id',
-// ],
-// 'Destination' => [
-// 'metadata-set' => 'saml20-sp-remote',
-// 'entityid' => 'joe',
-// ],
-// ];
-// $result = self::processFilter($config, $request);
-// $tid1 = $result['Attributes']['eduPersonTargetedID'][0];
-//
-// $request['UserID'] = 'user3@example.org';
-// $result = self::processFilter($config, $request);
-// $tid2 = $result['Attributes']['eduPersonTargetedID'][0];
-//
-// $this->assertNotEquals($tid1, $tid2);
-//
-// $request['Destination']['entityid'] = 'urn:example.org:another-sp';
-// $result = self::processFilter($config, $request);
-// $tid3 = $result['Attributes']['eduPersonTargetedID'][0];
-//
-// $this->assertNotEquals($tid2, $tid3);
-// }
+ /**
+ * Test the most basic functionality
+ * @return void
+ */
+ public function testBasic()
+ {
+ $config = ['identifyingAttribute' => 'uid'];
+ $request = [
+ 'Attributes' => ['uid' => ['user2@example.org']],
+ ];
+ $result = self::processFilter($config, $request);
+ $attributes = $result['Attributes'];
+ $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
+ $this->assertMatchesRegularExpression('/^[0-9a-f]{40}$/', $attributes['eduPersonTargetedID'][0]);
+ }
+
+
+ /**
+ * Test with src and dst entityIds.
+ * Make sure to overwrite any present eduPersonTargetedId
+ * @return void
+ */
+ public function testWithSrcDst()
+ {
+ $config = ['identifyingAttribute' => 'uid'];
+ $request = [
+ 'Attributes' => [
+ 'eduPersonTargetedID' => ['dummy'],
+ 'uid' => ['user2@example.org'],
+ ],
+ 'Source' => [
+ 'metadata-set' => 'saml20-idp-hosted',
+ 'entityid' => 'urn:example:src:id',
+ ],
+ 'Destination' => [
+ 'metadata-set' => 'saml20-sp-remote',
+ 'entityid' => 'joe',
+ ],
+ ];
+
+ $result = self::processFilter($config, $request);
+ $attributes = $result['Attributes'];
+
+ $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
+ $this->assertMatchesRegularExpression('/^[0-9a-f]{40}$/', $attributes['eduPersonTargetedID'][0]);
+ }
+
+
+ /**
+ * Test with nameId config option set.
+ * @return void
+ */
+ public function testNameIdGeneration()
+ {
+ $nameid = new NameID();
+ $nameid->setFormat(Constants::NAMEID_PERSISTENT);
+ $nameid->setNameQualifier('urn:example:src:id');
+ $nameid->setSPNameQualifier('joe');
+ $nameid->setValue('joe');
+
+ $config = [
+ 'nameId' => true,
+ 'identifyingAttribute' => 'eduPersonPrincipalName',
+ ];
+
+ $request = array(
+ 'Attributes' => [
+ 'eduPersonPrincipalName' => ['joe'],
+ 'eduPersonTargetedID' => [$nameid->toXML()->ownerDocument->saveXML()],
+ ],
+ 'Source' => [
+ 'metadata-set' => 'saml20-idp-hosted',
+ 'entityid' => 'urn:example:src:id',
+ ],
+ 'Destination' => [
+ 'metadata-set' => 'saml20-sp-remote',
+ 'entityid' => 'joe',
+ ],
+ );
+
+ $result = self::processFilter($config, $request);
+ $attributes = $result['Attributes'];
+
+ $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
+ $this->assertMatchesRegularExpression(
+ '#^<saml:NameID xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" NameQualifier="urn:example:src:id"' .
+ ' SPNameQualifier="joe"' .
+ ' Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent">[0-9a-f]{40}</saml:NameID>$#',
+ strval($attributes['eduPersonTargetedID'][0])
+ );
+ }
+
+
+ /**
+ * Test the outcome to make sure the algorithm remains unchanged
+ * @return void
+ */
+ public function testOutcome()
+ {
+ $config = ['identifyingAttribute' => 'uid'];
+ $request = [
+ 'Attributes' => ['uid' => ['user2@example.org']],
+ ];
+ $result = self::processFilter($config, $request);
+ $attributes = $result['Attributes'];
+ $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
+ $this->assertEquals('c1ae2c2ef77b73f7c47b700e42617117b6ec4adc', $attributes['eduPersonTargetedID'][0]);
+ }
+
+
+ /**
+ * Test the outcome when multiple values are given
+ * @return void
+ */
+ public function testOutcomeMultipleValues()
+ {
+ $config = ['identifyingAttribute' => 'uid'];
+ $request = [
+ 'Attributes' => ['uid' => ['user2@example.org', 'donald@duck.org']],
+ ];
+ $result = self::processFilter($config, $request);
+ $attributes = $result['Attributes'];
+ $this->assertArrayHasKey('eduPersonTargetedID', $attributes);
+ $this->assertEquals('c1ae2c2ef77b73f7c47b700e42617117b6ec4adc', $attributes['eduPersonTargetedID'][0]);
+ }
+
+
+ /**
+ * Test that Id is the same for subsequent invocations with same input.
+ * @return void
+ */
+ public function testIdIsPersistent()
+ {
+ $config = ['identifyingAttribute' => 'uid'];
+ $request = [
+ 'Attributes' => [
+ 'eduPersonTargetedID' => ['dummy'],
+ 'uid' => ['user2@example.org'],
+ ],
+ 'Source' => [
+ 'metadata-set' => 'saml20-idp-hosted',
+ 'entityid' => 'urn:example:src:id',
+ ],
+ 'Destination' => [
+ 'metadata-set' => 'saml20-sp-remote',
+ 'entityid' => 'joe',
+ ],
+ ];
+
+ for ($i = 0; $i < 10; ++$i) {
+ $result = self::processFilter($config, $request);
+ $attributes = $result['Attributes'];
+ $tid = $attributes['eduPersonTargetedID'][0];
+ if (isset($prevtid)) {
+ $this->assertEquals($prevtid, $tid);
+ }
+ $prevtid = $tid;
+ }
+ }
+
+
+ /**
+ * Test that Id is different for two different usernames and two different sp's
+ * @return void
+ */
+ public function testIdIsUnique()
+ {
+ $config = ['identifyingAttribute' => 'uid'];
+ $request = [
+ 'Attributes' => ['uid' => ['user2@example.org']],
+ 'Source' => [
+ 'metadata-set' => 'saml20-idp-hosted',
+ 'entityid' => 'urn:example:src:id',
+ ],
+ 'Destination' => [
+ 'metadata-set' => 'saml20-sp-remote',
+ 'entityid' => 'joe',
+ ],
+ ];
+
+ $result = self::processFilter($config, $request);
+ $tid1 = $result['Attributes']['eduPersonTargetedID'][0];
+
+ $request['Attributes']['uid'][0] = 'user3@example.org';
+ $result = self::processFilter($config, $request);
+ $tid2 = $result['Attributes']['eduPersonTargetedID'][0];
+
+ $this->assertNotEquals($tid1, $tid2);
+
+ $request['Destination']['entityid'] = 'urn:example.org:another-sp';
+ $result = self::processFilter($config, $request);
+ $tid3 = $result['Attributes']['eduPersonTargetedID'][0];
+
+ $this->assertNotEquals($tid2, $tid3);
+ }
/**
@@ -204,7 +281,7 @@ class TargetedIDTest extends TestCase
];
$request = [
'Attributes' => [
- 'displayName' => 'Jack Student',
+ 'displayName' => ['Jack Student'],
],
];
self::processFilter($config, $request);
@@ -223,7 +300,7 @@ class TargetedIDTest extends TestCase
];
$request = [
'Attributes' => [
- 'displayName' => 'Jack Student',
+ 'displayName' => ['Jack Student'],
],
];
self::processFilter($config, $request);
@@ -242,7 +319,7 @@ class TargetedIDTest extends TestCase
];
$request = [
'Attributes' => [
- 'displayName' => 'Jack Student',
+ 'displayName' => ['Jack Student'],
],
];
self::processFilter($config, $request);
--
GitLab