From 40909c3dea5d308159849c6ac34190d97c416384 Mon Sep 17 00:00:00 2001 From: Koen Cornelis Date: Tue, 11 May 2021 22:15:54 +0200 Subject: [PATCH 01/15] Add a consentHashService Prior to this commit, there was no service specifically for hashing attribute arrays for consent. This commit adds such a service with both the old hashing algorithm and a stable hashing algorithm. Pivotal ticket: https://www.pivotaltracker.com/story/show/176513931 --- config/services/compat.yml | 1 + config/services/services.yml | 4 + library/EngineBlock/Corto/Model/Consent.php | 92 ++-- .../Corto/Model/Consent/Factory.php | 13 +- .../Service/Consent/ConsentHashService.php | 147 ++++++ .../Service/{ => Consent}/ConsentService.php | 0 .../{ => Consent}/ConsentServiceInterface.php | 0 .../Consent/ConsentHashServiceTest.php | 446 ++++++++++++++++++ 8 files changed, 670 insertions(+), 33 deletions(-) create mode 100644 src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php rename src/OpenConext/EngineBlock/Service/{ => Consent}/ConsentService.php (100%) rename src/OpenConext/EngineBlock/Service/{ => Consent}/ConsentServiceInterface.php (100%) create mode 100644 tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php diff --git a/config/services/compat.yml b/config/services/compat.yml index 26b8946cf6..984752fec2 100644 --- a/config/services/compat.yml +++ b/config/services/compat.yml @@ -51,6 +51,7 @@ services: arguments: - "@engineblock.compat.corto_filter_command_factory" - "@engineblock.compat.database_connection_factory" + - "@engineblock.service.consent.ConsentHashService" engineblock.compat.saml2_id_generator: public: true diff --git a/config/services/services.yml b/config/services/services.yml index 9cecb712ef..3059dfb981 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -75,6 +75,10 @@ services: - '@OpenConext\EngineBlock\Metadata\LoaRepository' - '@logger' + engineblock.service.consent.ConsentHashService: + class: OpenConext\EngineBlock\Service\Consent\ConsentHashService + public: false + OpenConext\EngineBlock\Service\ConsentService: arguments: - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository' diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 40bd3e1ef6..b60f260807 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -19,6 +19,7 @@ use Doctrine\DBAL\Statement; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Service\Consent\ConsentHashService; class EngineBlock_Corto_Model_Consent { @@ -37,7 +38,7 @@ class EngineBlock_Corto_Model_Consent */ private $_response; /** - * @var array + * @var array All attributes as an associative array. */ private $_responseAttributes; @@ -60,6 +61,11 @@ class EngineBlock_Corto_Model_Consent */ private $_consentEnabled; + /** + * @var ConsentHashService + */ + private $_hashService; + /** * @param string $tableName * @param bool $mustStoreValues @@ -68,6 +74,7 @@ class EngineBlock_Corto_Model_Consent * @param EngineBlock_Database_ConnectionFactory $databaseConnectionFactory * @param bool $amPriorToConsentEnabled Is the run_all_manipulations_prior_to_consent feature enabled or not * @param bool $consentEnabled Is the feature_enable_consent feature enabled or not + * @param ConsentHashService $hashService */ public function __construct( $tableName, @@ -76,7 +83,8 @@ public function __construct( array $responseAttributes, EngineBlock_Database_ConnectionFactory $databaseConnectionFactory, $amPriorToConsentEnabled, - $consentEnabled + $consentEnabled, + $hashService ) { $this->_tableName = $tableName; @@ -85,33 +93,54 @@ public function __construct( $this->_responseAttributes = $responseAttributes; $this->_databaseConnectionFactory = $databaseConnectionFactory; $this->_amPriorToConsentEnabled = $amPriorToConsentEnabled; + $this->_hashService = $hashService; $this->_consentEnabled = $consentEnabled; } - public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider) + public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool { return !$this->_consentEnabled || $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); } - public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider) + public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool { return !$this->_consentEnabled || $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); } - public function giveExplicitConsentFor(ServiceProvider $serviceProvider) + public function giveExplicitConsentFor(ServiceProvider $serviceProvider): bool { return !$this->_consentEnabled || $this->_storeConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); } - public function giveImplicitConsentFor(ServiceProvider $serviceProvider) + public function giveImplicitConsentFor(ServiceProvider $serviceProvider): bool { return !$this->_consentEnabled || $this->_storeConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); } + /** + * @throws EngineBlock_Exception + */ + public function countTotalConsent(): int + { + $dbh = $this->_getConsentDatabaseConnection(); + $hashedUserId = sha1($this->_getConsentUid()); + $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ?"; + $parameters = array($hashedUserId); + $statement = $dbh->prepare($query); + if (!$statement) { + throw new EngineBlock_Exception( + "Unable to create a prepared statement to count consent?!", EngineBlock_Exception::CODE_ALERT + ); + } + /** @var $statement PDOStatement */ + $statement->execute($parameters); + return (int)$statement->fetchColumn(); + } + /** * @return Doctrine\DBAL\Connection */ @@ -129,21 +158,17 @@ protected function _getConsentUid() return $this->_response->getNameIdValue(); } - protected function _getAttributesHash($attributes) + protected function _getAttributesHash($attributes): string { - $hashBase = NULL; - if ($this->_mustStoreValues) { - ksort($attributes); - $hashBase = serialize($attributes); - } else { - $names = array_keys($attributes); - sort($names); - $hashBase = implode('|', $names); - } - return sha1($hashBase); + return $this->_hashService->getUnstableAttributesHash($attributes, $this->_mustStoreValues); + } + + protected function _getStableAttributesHash($attributes): string + { + return $this->_hashService->getStableAttributesHash($attributes, $this->_mustStoreValues); } - private function _storeConsent(ServiceProvider $serviceProvider, $consentType) + private function _storeConsent(ServiceProvider $serviceProvider, $consentType): bool { $dbh = $this->_getConsentDatabaseConnection(); if (!$dbh) { @@ -161,7 +186,7 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType) $parameters = array( sha1($consentUuid), $serviceProvider->entityId, - $this->_getAttributesHash($this->_responseAttributes), + $this->_getStableAttributesHash($this->_responseAttributes), $consentType, ); @@ -189,21 +214,28 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType) return true; } - private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType) + private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): bool { - try { - $dbh = $this->_getConsentDatabaseConnection(); - if (!$dbh) { - return false; - } + $dbh = $this->_getConsentDatabaseConnection(); + if (!$dbh) { + return false; + } - $attributesHash = $this->_getAttributesHash($this->_responseAttributes); + $unstableConsentHash = $this->_getAttributesHash($this->_responseAttributes); + $hasUnstableConsentHash = $this->retrieveConsentHashFromDb($dbh, $serviceProvider, $consentType, $unstableConsentHash); - $consentUuid = $this->_getConsentUid(); - if (!is_string($consentUuid)) { - return false; - } + if ($hasUnstableConsentHash) { + return true; + } + + $stableConsentHash = $this->_getStableAttributesHash($this->_responseAttributes); + return $this->retrieveConsentHashFromDb($dbh, $serviceProvider, $consentType, $stableConsentHash); + } + private function retrieveConsentHashFromDb(PDO $dbh, ServiceProvider $serviceProvider, $consentType, $attributesHash): bool + { + try { + $consentUuid = $this->_getConsentUid(); $query = " SELECT * FROM {$this->_tableName} diff --git a/library/EngineBlock/Corto/Model/Consent/Factory.php b/library/EngineBlock/Corto/Model/Consent/Factory.php index 80be173e81..b135ed9c82 100644 --- a/library/EngineBlock/Corto/Model/Consent/Factory.php +++ b/library/EngineBlock/Corto/Model/Consent/Factory.php @@ -27,18 +27,24 @@ class EngineBlock_Corto_Model_Consent_Factory /** @var EngineBlock_Database_ConnectionFactory */ private $_databaseConnectionFactory; + /** + * @var ConsentHashService + */ + private $_hashService; - /** + /** * @param EngineBlock_Corto_Filter_Command_Factory $filterCommandFactory * @param EngineBlock_Database_ConnectionFactory $databaseConnectionFactory */ public function __construct( EngineBlock_Corto_Filter_Command_Factory $filterCommandFactory, - EngineBlock_Database_ConnectionFactory $databaseConnectionFactory + EngineBlock_Database_ConnectionFactory $databaseConnectionFactory, + ConsentHashService $hashService ) { $this->_filterCommandFactory = $filterCommandFactory; $this->_databaseConnectionFactory = $databaseConnectionFactory; + $this->_hashService = $hashService; } /** @@ -70,7 +76,8 @@ public function create( $attributes, $this->_databaseConnectionFactory, $amPriorToConsent, - $consentEnabled + $consentEnabled, + $this->_hashService ); } } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php new file mode 100644 index 0000000000..298ec00d2e --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -0,0 +1,147 @@ +caseNormalizeStringArray($attributes); + $hashBase = $mustStoreValues ? $this->createHashBaseWithValues($lowerCasedAttributes) : $this->createHashBaseWithoutValues($lowerCasedAttributes); + + return sha1($hashBase); + } + + private function createHashBaseWithValues(array $lowerCasedAttributes): string + { + return serialize($this->sortArray($lowerCasedAttributes)); + } + + private function createHashBaseWithoutValues(array $lowerCasedAttributes): string + { + $noEmptyAttributes = $this->removeEmptyAttributes($lowerCasedAttributes); + $sortedAttributes = $this->sortArray(array_keys($noEmptyAttributes)); + return implode('|', $sortedAttributes); + } + + /** + * Lowercases all array keys and values. + * Performs the lowercasing on a copy (which is returned), to avoid side-effects. + */ + private function caseNormalizeStringArray(array $original): array + { + return unserialize(strtolower(serialize($original))); + } + + /** + * Recursively sorts an array via the ksort function. Performs the sort on a copy to avoid side-effects. + */ + private function sortArray(array $sortMe): array + { + $copy = unserialize(serialize($sortMe)); + $sortFunction = 'ksort'; + $copy = $this->removeEmptyAttributes($copy); + + if($this->isSequentialArray($copy)){ + $sortFunction = 'sort'; + $copy = $this->renumberIndices($copy); + } + + $sortFunction($copy); + foreach ($copy as $key => $value) { + if (is_array($value)) { + $sortFunction($value); + $copy[$key] = $this->sortArray($value); + } + } + + return $copy; + } + + /** + * Determines whether an array is sequential, by checking to see if there's at no string keys in it. + */ + private function isSequentialArray(array $array): bool + { + return count(array_filter(array_keys($array), 'is_string')) === 0; + } + + /** + * Reindexes the values of the array so that any skipped numeric indexes are removed. + */ + private function renumberIndices(array $array): array + { + return array_values($array); + } + + /** + * Iterate over an array and unset any empty values. + */ + private function removeEmptyAttributes(array $array): array + { + $copy = unserialize(serialize($array)); + + foreach ($copy as $key => $value) { + if ($this->is_blank($value)) { + unset($copy[$key]); + } + } + + return $copy; + } + + /** + * Checks if a value is empty, but allowing 0 as an integer, float and string. This means the following are allowed: + * - 0 + * - 0.0 + * - "0" + * @param $value array|string|integer|float + */ + private function is_blank($value): bool { + return empty($value) && !is_numeric($value); + } +} diff --git a/src/OpenConext/EngineBlock/Service/ConsentService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php similarity index 100% rename from src/OpenConext/EngineBlock/Service/ConsentService.php rename to src/OpenConext/EngineBlock/Service/Consent/ConsentService.php diff --git a/src/OpenConext/EngineBlock/Service/ConsentServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php similarity index 100% rename from src/OpenConext/EngineBlock/Service/ConsentServiceInterface.php rename to src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php new file mode 100644 index 0000000000..d2454e3556 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -0,0 +1,446 @@ +chs = new ConsentHashService(); + } + + public function test_stable_attribute_hash_switched_order_associative_array() + { + $attributes = [ + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['Doe'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:isMemberOf' => [ + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + ]; + $attributesSwitchedOrder = [ + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['Doe'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:isMemberOf' => [ + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.ORG', + ], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesSwitchedOrder, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesSwitchedOrder, true)); + } + + public function test_stable_attribute_hash_switched_order_sequential_array() + { + $attributes = [ + ['John Doe'], + ['joe-f12'], + ['John Doe'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['j.doe@example.com'], + ['example.com'], + [ + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + ]; + $attributesSwitchedOrder = [ + ['John Doe'], + ['John Doe'], + ['joe-f12'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['example.com'], + ['j.doe@example.com'], + [ + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.ORG', + ], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesSwitchedOrder, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesSwitchedOrder, true)); + } + + public function test_stable_attribute_hash_switched_order_and_different_casing_associative_array() + { + $attributes = [ + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['Doe'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:isMemberOf' => [ + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + ]; + $attributesSwitchedOrderAndCasing = [ + 'urn:mace:dir:attribute-def:sn' => ['DOE'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:CN' => ['John Doe'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-DEF:displayName' => ['John Doe'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:isMemberOf' => [ + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + 'urn:mace:dir:attribute-def:UID' => ['joe-f12'], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesSwitchedOrderAndCasing, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesSwitchedOrderAndCasing, true)); + } + + public function test_stable_attribute_hash_switched_order_and_different_casing_sequential_array() + { + $attributes = [ + ['John Doe'], + ['joe-f12'], + ['John Doe'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['j.doe@example.com'], + ['example.com'], + [ + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab2:org:vm.openconext.ORG', + 'urn:collab3:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab1:org:vm.openconext.org', + 'urn:collab2:org:vm.openconext.org', + 'urn:collab3:org:vm.openconext.org', + ], + ]; + $attributesSwitchedOrderAndCasing = [ + ['joe-f12'], + ['John Doe'], + ['John Doe'], + ['j.doe@example.com'], + ['John'], + ['EXample.com'], + ['j.doe@example.com'], + [ + 'URN:collab2:org:vm.openconext.ORG', + 'urn:collab2:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.Org', + 'urn:collaboration:organisation:VM.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab3:org:vm.openconext.org', + 'urn:collab3:org:vm.openconext.ORG', + 'urn:collab1:org:vm.openconext.org', + ], + ['DOE'], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesSwitchedOrderAndCasing, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesSwitchedOrderAndCasing, true)); + } + + public function test_stable_attribute_hash_different_casing_associative_array() + { + $attributes = [ + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['Doe'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:isMemberOf' => [ + 'urn:collab:ORG:vm.openconext.ORG', + 'urn:collab:ORG:vm.openconext.ORG', + 'urn:collab:ORG:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + ]; + $attributesDifferentCasing = [ + 'urn:mace:dir:attribute-def:DISPLAYNAME' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['DOE'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:ISMemberOf' => [ + 'URN:collab:org:VM.openconext.org', + 'URN:collab:org:VM.openconext.org', + 'URN:collab:org:VM.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesDifferentCasing, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesDifferentCasing, true)); + } + + public function test_stable_attribute_hash_different_casing_sequential_array() + { + $attributes = [ + ['John Doe'], + ['joe-f12'], + ['John Doe'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['j.doe@example.com'], + ['example.com'], + [ + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + ]; + $attributesDifferentCasing = [ + ['JOHN Doe'], + ['joe-f12'], + ['John DOE'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['j.doe@example.com'], + ['example.com'], + [ + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:VM.openconext.ORG', + 'urn:collab:org:VM.openconext.org', + 'urn:collaboration:organisation:VM.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:COLLAB:org:vm.openconext.org', + ], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesDifferentCasing, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesDifferentCasing, true)); + } + + public function test_stable_attribute_hash_reordering_sparse_sequential_arrays() + { + $attributes = [ "AttributeA" => [ 0 => "aap", 1 => "noot"] ]; + $attributesDifferentCasing = + [ "AttributeA" => [ 0 => "aap", 2 => "noot"] ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesDifferentCasing, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesDifferentCasing, true)); + } + + public function test_stable_attribute_hash_remove_empty_attributes() + { + $attributes = [ "AttributeA" => [ 0 => "aap", 1 => "noot"], "AttributeB" => [], "AttributeC" => 0 ]; + $attributesDifferentNoEmptyValues = + [ "AttributeA" => [ 0 => "aap", 2 => "noot"], "AttributeC" => 0 ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesDifferentNoEmptyValues, false)); + $this->assertEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesDifferentNoEmptyValues, true)); + } + + public function test_stable_attribute_hash_two_different_arrays_yield_different_hashes_associative() + { + $attributes = [ + 'a' => ['John Doe'], + 'b' => ['joe-f12'], + 'c' => ['John Doe'], + 'd' => ['Doe'], + 'e' => ['j.doe@example.com'], + 'f' => ['John'], + 'g' => ['j.doe@example.com'], + 'h' => ['example.com'], + ]; + $differentAttributes = [ + 'i' => 'urn:collab:org:vm.openconext.ORG', + 'j' => 'urn:collab:org:vm.openconext.ORG', + 'k' => 'urn:collab:org:vm.openconext.ORG', + 'l' => 'urn:collab:org:vm.openconext.org', + 'm' => 'urn:collaboration:organisation:vm.openconext.org', + 'n' => 'urn:collab:org:vm.openconext.org', + 'o' => 'urn:collab:org:vm.openconext.org', + 'p' => 'urn:collab:org:vm.openconext.org', + ]; + $this->assertNotEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($differentAttributes, false)); + $this->assertNotEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($differentAttributes, true)); + } + + public function test_stable_attribute_hash_two_different_arrays_yield_different_hashes_sequential() + { + $attributes = [ + ['John Doe'], + ['joe-f12'], + ['John Doe'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['j.doe@example.com'], + ['example.com'], + ]; + $differentAttributes = [ + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ]; + // two sequential arrays with the same amount of attributes will yield the exact same hash if no values must be stored. todo: check if we want this? + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($differentAttributes, false)); + $this->assertNotEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($differentAttributes, true)); + } + + public function test_stable_attribute_hash_multiple_value_vs_single_value_associative_array() + { + $attributes = [ + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['Doe'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:isMemberOf' => [ + 'urn:collab:ORG:vm.openconext.ORG', + 'urn:collab:ORG:vm.openconext.ORG', + 'urn:collab:ORG:vm.openconext.ORG', + 'urn:collab:org:vm.openconext.org', + 'urn:collaboration:organisation:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + 'urn:collab:org:vm.openconext.org', + ], + ]; + $attributesSingleValue = [ + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['Doe'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'urn:mace:dir:attribute-def:isMemberOf' => [ + 'urn:collab:org:vm.openconext.org', + ], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesSingleValue, false)); + $this->assertNotEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesSingleValue, true)); + } + + public function test_stable_attribute_hash_multiple_value_vs_single_value_sequential_array() + { + $attributes = [ + ['John Doe'], + ['joe-f12'], + ['John Doe'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['j.doe@example.com'], + ['example.com', 'j.doe@example.com', 'jane'], + ]; + $attributesSingleValue = [ + ['John Doe'], + ['joe-f12'], + ['John Doe'], + ['Doe'], + ['j.doe@example.com'], + ['John'], + ['j.doe@example.com'], + ['example.com'], + ]; + $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesSingleValue, false)); + $this->assertNotEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesSingleValue, true)); + } +} From 7f8df3c88a1eb494899ab045ee5daabb9e41155a Mon Sep 17 00:00:00 2001 From: Koen Cornelis Date: Thu, 20 May 2021 11:31:33 +0200 Subject: [PATCH 02/15] Move DB logic to ConsentHashRepository --- library/EngineBlock/Corto/Model/Consent.php | 127 ++++-------------- .../Corto/Model/Consent/Factory.php | 2 + .../Service/Consent/ConsentHashRepository.php | 111 +++++++++++++++ .../Service/Consent/ConsentHashService.php | 39 +++++- .../Consent/ConsentHashServiceTest.php | 7 +- 5 files changed, 181 insertions(+), 105 deletions(-) create mode 100644 src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index b60f260807..23e52348a2 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -67,26 +67,18 @@ class EngineBlock_Corto_Model_Consent private $_hashService; /** - * @param string $tableName - * @param bool $mustStoreValues - * @param EngineBlock_Saml2_ResponseAnnotationDecorator $response - * @param array $responseAttributes - * @param EngineBlock_Database_ConnectionFactory $databaseConnectionFactory * @param bool $amPriorToConsentEnabled Is the run_all_manipulations_prior_to_consent feature enabled or not - * @param bool $consentEnabled Is the feature_enable_consent feature enabled or not - * @param ConsentHashService $hashService */ public function __construct( - $tableName, - $mustStoreValues, + string $tableName, + bool $mustStoreValues, EngineBlock_Saml2_ResponseAnnotationDecorator $response, array $responseAttributes, EngineBlock_Database_ConnectionFactory $databaseConnectionFactory, - $amPriorToConsentEnabled, - $consentEnabled, - $hashService - ) - { + bool $amPriorToConsentEnabled, + bool $consentEnabled, + ConsentHashService $hashService + ) { $this->_tableName = $tableName; $this->_mustStoreValues = $mustStoreValues; $this->_response = $response; @@ -121,24 +113,15 @@ public function giveImplicitConsentFor(ServiceProvider $serviceProvider): bool $this->_storeConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); } - /** - * @throws EngineBlock_Exception - */ public function countTotalConsent(): int { $dbh = $this->_getConsentDatabaseConnection(); - $hashedUserId = sha1($this->_getConsentUid()); - $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ?"; - $parameters = array($hashedUserId); - $statement = $dbh->prepare($query); - if (!$statement) { - throw new EngineBlock_Exception( - "Unable to create a prepared statement to count consent?!", EngineBlock_Exception::CODE_ALERT - ); + if (!$dbh) { + return 0; } - /** @var $statement PDOStatement */ - $statement->execute($parameters); - return (int)$statement->fetchColumn(); + + $consentUid = $this->_getConsentUid(); + return $this->_hashService->countTotalConsent($dbh, $consentUid); } /** @@ -176,13 +159,10 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): } $consentUuid = $this->_getConsentUid(); - if(! is_string($consentUuid)){ + if (!\is_string($consentUuid)) { return false; } - $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, consent_type, consent_date, deleted_at) - VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') - ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), consent_type=VALUES(consent_type), consent_date=NOW()"; $parameters = array( sha1($consentUuid), $serviceProvider->entityId, @@ -190,28 +170,7 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): $consentType, ); - $statement = $dbh->prepare($query); - if (!$statement) { - throw new EngineBlock_Exception( - "Unable to create a prepared statement to insert consent?!", - EngineBlock_Exception::CODE_CRITICAL - ); - } - - assert($statement instanceof Statement); - try{ - foreach ($parameters as $index => $parameter){ - $statement->bindValue($index + 1, $parameter); - } - - $statement->executeStatement(); - }catch (\Doctrine\DBAL\Exception $e){ - throw new EngineBlock_Corto_Module_Services_Exception( - sprintf('Error storing consent: "%s"', var_export($e->getMessage(), true)), - EngineBlock_Exception::CODE_CRITICAL - ); - } - return true; + return $this->_hashService->storeConsentHashInDb($dbh, $parameters); } private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): bool @@ -221,56 +180,26 @@ private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentTyp return false; } - $unstableConsentHash = $this->_getAttributesHash($this->_responseAttributes); - $hasUnstableConsentHash = $this->retrieveConsentHashFromDb($dbh, $serviceProvider, $consentType, $unstableConsentHash); + $parameters = array( + sha1($this->_getConsentUid()), + $serviceProvider->entityId, + $this->_getAttributesHash($this->_responseAttributes), + $consentType, + ); + + $hasUnstableConsentHash = $this->_hashService->retrieveConsentHashFromDb($dbh, $parameters); if ($hasUnstableConsentHash) { return true; } - $stableConsentHash = $this->_getStableAttributesHash($this->_responseAttributes); - return $this->retrieveConsentHashFromDb($dbh, $serviceProvider, $consentType, $stableConsentHash); - } - - private function retrieveConsentHashFromDb(PDO $dbh, ServiceProvider $serviceProvider, $consentType, $attributesHash): bool - { - try { - $consentUuid = $this->_getConsentUid(); - $query = " - SELECT * - FROM {$this->_tableName} - WHERE hashed_user_id = ? - AND service_id = ? - AND attribute = ? - AND consent_type = ? - AND deleted_at IS NULL - "; - $hashedUserId = sha1($consentUuid); - $parameters = array( - $hashedUserId, - $serviceProvider->entityId, - $attributesHash, - $consentType, - ); - - $statement = $dbh->prepare($query); - assert($statement instanceof Statement); - foreach ($parameters as $position => $parameter) { - $statement->bindValue($position + 1, $parameter); - } - $rows = $statement->executeQuery(); - - if ($rows->rowCount() < 1) { - // No stored consent found - return false; - } + $parameters[2] = array( + sha1($this->_getConsentUid()), + $serviceProvider->entityId, + $this->_getStableAttributesHash($this->_responseAttributes), + $consentType, + ); - return true; - } catch (PDOException $e) { - throw new EngineBlock_Corto_ProxyServer_Exception( - sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage()), - EngineBlock_Exception::CODE_ALERT - ); - } + return $this->_hashService->retrieveConsentHashFromDb($dbh, $parameters); } } diff --git a/library/EngineBlock/Corto/Model/Consent/Factory.php b/library/EngineBlock/Corto/Model/Consent/Factory.php index b135ed9c82..6629d943ff 100644 --- a/library/EngineBlock/Corto/Model/Consent/Factory.php +++ b/library/EngineBlock/Corto/Model/Consent/Factory.php @@ -16,6 +16,8 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Service\Consent\ConsentHashService; + /** * @todo write a test */ diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php new file mode 100644 index 0000000000..20ce7251ff --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php @@ -0,0 +1,111 @@ +_tableName} + WHERE hashed_user_id = ? + AND service_id = ? + AND attribute = ? + AND consent_type = ? + AND deleted_at IS NULL + "; + /** @var $statement PDOStatement */ + $statement = $dbh->prepare($query); + $statement->execute($parameters); + $rows = $statement->fetchAll(); + + if (count($rows) < 1) { + // No stored consent found + return false; + } + + return true; + } catch (PDOException $e) { + throw new EngineBlock_Corto_ProxyServer_Exception( + sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage()), + EngineBlock_Exception::CODE_ALERT + ); + } + } + + /** + * @throws EngineBlock_Corto_Module_Services_Exception + * @throws EngineBlock_Exception + */ + public function storeConsentHashInDb(PDO $dbh, array $parameters): bool + { + $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, consent_type, consent_date, deleted_at) + VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') + ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), consent_type=VALUES(consent_type), consent_date=NOW()"; + + $statement = $dbh->prepare($query); + if (!$statement) { + throw new EngineBlock_Exception( + "Unable to create a prepared statement to insert consent?!", + EngineBlock_Exception::CODE_CRITICAL + ); + } + + /** @var $statement PDOStatement */ + if (!$statement->execute($parameters)) { + throw new EngineBlock_Corto_Module_Services_Exception( + sprintf('Error storing consent: "%s"', var_export($statement->errorInfo(), true)), + EngineBlock_Exception::CODE_CRITICAL + ); + } + + return true; + } + + /** + * @throws EngineBlock_Exception + */ + public function countTotalConsent(PDO $dbh, $consentUid): int + { + $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ? AND deleted_at IS NULL"; + $parameters = array(sha1($consentUid)); + $statement = $dbh->prepare($query); + if (!$statement) { + throw new EngineBlock_Exception( + "Unable to create a prepared statement to count consent?!", + EngineBlock_Exception::CODE_ALERT + ); + } + /** @var $statement PDOStatement */ + $statement->execute($parameters); + return (int)$statement->fetchColumn(); + } +} diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index 298ec00d2e..4e0a0ca767 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -18,6 +18,7 @@ namespace OpenConext\EngineBlock\Service\Consent; +use PDO; use function array_filter; use function array_keys; use function array_values; @@ -34,7 +35,32 @@ final class ConsentHashService { - public function getUnstableAttributesHash(array $attributes,bool $mustStoreValues): string + /** + * @var ConsentHashRepository + */ + private $consentHashRepository; + + public function __construct(ConsentHashRepository $consentHashRepository) + { + $this->consentHashRepository = $consentHashRepository; + } + + public function retrieveConsentHashFromDb(PDO $dbh, array $parameters): bool + { + return $this->consentHashRepository->retrieveConsentHashFromDb($dbh, $parameters); + } + + public function storeConsentHashInDb(PDO $dbh, array $parameters): bool + { + return $this->consentHashRepository->storeConsentHashInDb($dbh, $parameters); + } + + public function countTotalConsent(PDO $dbh, $consentUid): int + { + return $this->consentHashRepository->countTotalConsent($dbh, $consentUid); + } + + public function getUnstableAttributesHash(array $attributes, bool $mustStoreValues): string { $hashBase = null; if ($mustStoreValues) { @@ -51,7 +77,9 @@ public function getUnstableAttributesHash(array $attributes,bool $mustStoreValue public function getStableAttributesHash(array $attributes, bool $mustStoreValues) : string { $lowerCasedAttributes = $this->caseNormalizeStringArray($attributes); - $hashBase = $mustStoreValues ? $this->createHashBaseWithValues($lowerCasedAttributes) : $this->createHashBaseWithoutValues($lowerCasedAttributes); + $hashBase = $mustStoreValues + ? $this->createHashBaseWithValues($lowerCasedAttributes) + : $this->createHashBaseWithoutValues($lowerCasedAttributes); return sha1($hashBase); } @@ -86,7 +114,7 @@ private function sortArray(array $sortMe): array $sortFunction = 'ksort'; $copy = $this->removeEmptyAttributes($copy); - if($this->isSequentialArray($copy)){ + if ($this->isSequentialArray($copy)) { $sortFunction = 'sort'; $copy = $this->renumberIndices($copy); } @@ -126,7 +154,7 @@ private function removeEmptyAttributes(array $array): array $copy = unserialize(serialize($array)); foreach ($copy as $key => $value) { - if ($this->is_blank($value)) { + if ($this->isBlank($value)) { unset($copy[$key]); } } @@ -141,7 +169,8 @@ private function removeEmptyAttributes(array $array): array * - "0" * @param $value array|string|integer|float */ - private function is_blank($value): bool { + private function isBlank($value): bool + { return empty($value) && !is_numeric($value); } } diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index d2454e3556..c0e8f87d9f 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -18,10 +18,14 @@ namespace OpenConext\EngineBlock\Service\Consent; +use Mockery as m; +use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use PHPUnit\Framework\TestCase; class ConsentHashServiceTest extends TestCase { + use MockeryPHPUnitIntegration; + /** * @var ConsentHashService */ @@ -29,7 +33,8 @@ class ConsentHashServiceTest extends TestCase public function setUp() { - $this->chs = new ConsentHashService(); + $mockConsentHashRepository = m::mock('OpenConext\EngineBlock\Service\Consent\ConsentHashRepository'); + $this->chs = new ConsentHashService($mockConsentHashRepository); } public function test_stable_attribute_hash_switched_order_associative_array() From a81aa1118a67eba1eeed82416cbe27621bac1394 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 09:10:01 +0200 Subject: [PATCH 03/15] Move consent queries to existing repository The consent queries added to the ConsentRepository.php file (deleted) should have been added to the existing DbalConsentRepository. While at it, naming conventions have been applied to the query names. And the service config was updated. --- .../Repository/ConsentRepository.php | 6 + .../Service/Consent/ConsentHashRepository.php | 111 ------------------ .../Service/Consent/ConsentHashService.php | 22 ++-- .../Repository/DbalConsentRepository.php | 82 ++++++++++++- .../Consent/ConsentHashServiceTest.php | 3 +- 5 files changed, 99 insertions(+), 125 deletions(-) delete mode 100644 src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php diff --git a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php index 709ba9a7e7..b6a17a16ec 100644 --- a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php +++ b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php @@ -37,4 +37,10 @@ public function findAllFor($userId); public function deleteAllFor($userId); public function deleteOneFor(string $userId, string $serviceProviderEntityId): bool; + + public function hasConsentHash(array $parameters): bool; + + public function storeConsentHash(array $parameters): bool; + + public function countTotalConsent($consentUid): int; } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php deleted file mode 100644 index 20ce7251ff..0000000000 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashRepository.php +++ /dev/null @@ -1,111 +0,0 @@ -_tableName} - WHERE hashed_user_id = ? - AND service_id = ? - AND attribute = ? - AND consent_type = ? - AND deleted_at IS NULL - "; - /** @var $statement PDOStatement */ - $statement = $dbh->prepare($query); - $statement->execute($parameters); - $rows = $statement->fetchAll(); - - if (count($rows) < 1) { - // No stored consent found - return false; - } - - return true; - } catch (PDOException $e) { - throw new EngineBlock_Corto_ProxyServer_Exception( - sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage()), - EngineBlock_Exception::CODE_ALERT - ); - } - } - - /** - * @throws EngineBlock_Corto_Module_Services_Exception - * @throws EngineBlock_Exception - */ - public function storeConsentHashInDb(PDO $dbh, array $parameters): bool - { - $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, consent_type, consent_date, deleted_at) - VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') - ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), consent_type=VALUES(consent_type), consent_date=NOW()"; - - $statement = $dbh->prepare($query); - if (!$statement) { - throw new EngineBlock_Exception( - "Unable to create a prepared statement to insert consent?!", - EngineBlock_Exception::CODE_CRITICAL - ); - } - - /** @var $statement PDOStatement */ - if (!$statement->execute($parameters)) { - throw new EngineBlock_Corto_Module_Services_Exception( - sprintf('Error storing consent: "%s"', var_export($statement->errorInfo(), true)), - EngineBlock_Exception::CODE_CRITICAL - ); - } - - return true; - } - - /** - * @throws EngineBlock_Exception - */ - public function countTotalConsent(PDO $dbh, $consentUid): int - { - $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ? AND deleted_at IS NULL"; - $parameters = array(sha1($consentUid)); - $statement = $dbh->prepare($query); - if (!$statement) { - throw new EngineBlock_Exception( - "Unable to create a prepared statement to count consent?!", - EngineBlock_Exception::CODE_ALERT - ); - } - /** @var $statement PDOStatement */ - $statement->execute($parameters); - return (int)$statement->fetchColumn(); - } -} diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index 4e0a0ca767..d84e210205 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -18,7 +18,7 @@ namespace OpenConext\EngineBlock\Service\Consent; -use PDO; +use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use function array_filter; use function array_keys; use function array_values; @@ -36,28 +36,28 @@ final class ConsentHashService { /** - * @var ConsentHashRepository + * @var ConsentRepository */ - private $consentHashRepository; + private $consentRepository; - public function __construct(ConsentHashRepository $consentHashRepository) + public function __construct(ConsentRepository $consentHashRepository) { - $this->consentHashRepository = $consentHashRepository; + $this->consentRepository = $consentHashRepository; } - public function retrieveConsentHashFromDb(PDO $dbh, array $parameters): bool + public function retrieveConsentHashFromDb(array $parameters): bool { - return $this->consentHashRepository->retrieveConsentHashFromDb($dbh, $parameters); + return $this->consentRepository->hasConsentHash($parameters); } - public function storeConsentHashInDb(PDO $dbh, array $parameters): bool + public function storeConsentHashInDb(array $parameters): bool { - return $this->consentHashRepository->storeConsentHashInDb($dbh, $parameters); + return $this->consentRepository->storeConsentHash($parameters); } - public function countTotalConsent(PDO $dbh, $consentUid): int + public function countTotalConsent($consentUid): int { - return $this->consentHashRepository->countTotalConsent($dbh, $consentUid); + return $this->consentRepository->countTotalConsent($consentUid); } public function getUnstableAttributesHash(array $attributes, bool $mustStoreValues): string diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index f323c5e751..f4f8b17b78 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -26,6 +26,8 @@ use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use OpenConext\EngineBlock\Authentication\Value\ConsentType; use OpenConext\EngineBlock\Exception\RuntimeException; +use PDO; +use PDOException; use Psr\Log\LoggerInterface; use function sha1; @@ -61,7 +63,7 @@ public function __construct(ManagerRegistry $registry, LoggerInterface $logger) */ public function findAllFor($userId) { - $sql = ' + $sql = ' SELECT service_id , consent_date @@ -134,7 +136,8 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b hashed_user_id = :hashed_user_id AND service_id = :service_id - AND deleted_at IS NULL + AND + deleted_at IS NULL '; try { $result = $this->connection->executeQuery( @@ -166,4 +169,79 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b ); } } + + /** + * @throws RuntimeException + */ + public function hasConsentHash(array $parameters): bool + { + try { + $query = " + SELECT + * + FROM + {$this->_tableName} + WHERE + hashed_user_id = ? + AND + service_id = ? + AND + attribute = ? + AND + consent_type = ? + AND + deleted_at IS NULL + "; + + $statement = $this->connection->prepare($query); + $statement->execute($parameters); + $rows = $statement->fetchAll(); + + if (count($rows) < 1) { + // No stored consent found + return false; + } + + return true; + } catch (PDOException $e) { + throw new RuntimeException(sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage())); + } + } + + /** + * @throws RuntimeException + */ + public function storeConsentHash(array $parameters): bool + { + $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, consent_type, consent_date, deleted_at) + VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') + ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), consent_type=VALUES(consent_type), consent_date=NOW()"; + $statement = $this->connection->prepare($query); + if (!$statement) { + throw new RuntimeException("Unable to create a prepared statement to insert consent?!"); + } + + if (!$statement->execute($parameters)) { + throw new RuntimeException( + sprintf('Error storing consent: "%s"', var_export($statement->errorInfo(), true)) + ); + } + + return true; + } + + /** + * @throws RuntimeException + */ + public function countTotalConsent($consentUid): int + { + $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ? AND deleted_at IS NULL"; + $parameters = array(sha1($consentUid)); + $statement = $this->connection->prepare($query); + if (!$statement) { + throw new RuntimeException("Unable to create a prepared statement to count consent?!"); + } + $statement->execute($parameters); + return (int)$statement->fetchColumn(); + } } diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index c0e8f87d9f..ace9c0a432 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -20,6 +20,7 @@ use Mockery as m; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; +use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use PHPUnit\Framework\TestCase; class ConsentHashServiceTest extends TestCase @@ -33,7 +34,7 @@ class ConsentHashServiceTest extends TestCase public function setUp() { - $mockConsentHashRepository = m::mock('OpenConext\EngineBlock\Service\Consent\ConsentHashRepository'); + $mockConsentHashRepository = m::mock(ConsentRepository::class); $this->chs = new ConsentHashService($mockConsentHashRepository); } From 8dc0d3627e997ada36017708c6d969fc604183f8 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 09:49:02 +0200 Subject: [PATCH 04/15] Extract ConsentHashServiceInterface Extracting this interface from the existing service is allowing us to more easily test the service. As mocking a final class is not possible. --- library/EngineBlock/Corto/Model/Consent.php | 46 ++++--------------- .../Corto/Model/Consent/Factory.php | 10 +--- .../Service/Consent/ConsentHashService.php | 6 +-- .../Consent/ConsentHashServiceInterface.php | 32 +++++++++++++ .../Test/Corto/Model/ConsentTest.php | 31 +++++++++---- 5 files changed, 65 insertions(+), 60 deletions(-) create mode 100644 src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 23e52348a2..470e82558b 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -19,7 +19,7 @@ use Doctrine\DBAL\Statement; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; -use OpenConext\EngineBlock\Service\Consent\ConsentHashService; +use OpenConext\EngineBlock\Service\ConsentHashServiceInterface; class EngineBlock_Corto_Model_Consent { @@ -42,11 +42,6 @@ class EngineBlock_Corto_Model_Consent */ private $_responseAttributes; - /** - * @var EngineBlock_Database_ConnectionFactory - */ - private $_databaseConnectionFactory; - /** * A reflection of the eb.run_all_manipulations_prior_to_consent feature flag * @@ -62,7 +57,7 @@ class EngineBlock_Corto_Model_Consent private $_consentEnabled; /** - * @var ConsentHashService + * @var ConsentHashServiceInterface */ private $_hashService; @@ -74,16 +69,14 @@ public function __construct( bool $mustStoreValues, EngineBlock_Saml2_ResponseAnnotationDecorator $response, array $responseAttributes, - EngineBlock_Database_ConnectionFactory $databaseConnectionFactory, bool $amPriorToConsentEnabled, bool $consentEnabled, - ConsentHashService $hashService + ConsentHashServiceInterface $hashService ) { $this->_tableName = $tableName; $this->_mustStoreValues = $mustStoreValues; $this->_response = $response; $this->_responseAttributes = $responseAttributes; - $this->_databaseConnectionFactory = $databaseConnectionFactory; $this->_amPriorToConsentEnabled = $amPriorToConsentEnabled; $this->_hashService = $hashService; $this->_consentEnabled = $consentEnabled; @@ -115,21 +108,8 @@ public function giveImplicitConsentFor(ServiceProvider $serviceProvider): bool public function countTotalConsent(): int { - $dbh = $this->_getConsentDatabaseConnection(); - if (!$dbh) { - return 0; - } - $consentUid = $this->_getConsentUid(); - return $this->_hashService->countTotalConsent($dbh, $consentUid); - } - - /** - * @return Doctrine\DBAL\Connection - */ - protected function _getConsentDatabaseConnection() - { - return $this->_databaseConnectionFactory->create(); + return $this->_hashService->countTotalConsent($consentUid); } protected function _getConsentUid() @@ -153,13 +133,8 @@ protected function _getStableAttributesHash($attributes): string private function _storeConsent(ServiceProvider $serviceProvider, $consentType): bool { - $dbh = $this->_getConsentDatabaseConnection(); - if (!$dbh) { - return false; - } - $consentUuid = $this->_getConsentUid(); - if (!\is_string($consentUuid)) { + if (!is_string($consentUuid)) { return false; } @@ -170,16 +145,11 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): $consentType, ); - return $this->_hashService->storeConsentHashInDb($dbh, $parameters); + return $this->_hashService->storeConsentHash($parameters); } private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): bool { - $dbh = $this->_getConsentDatabaseConnection(); - if (!$dbh) { - return false; - } - $parameters = array( sha1($this->_getConsentUid()), $serviceProvider->entityId, @@ -187,7 +157,7 @@ private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentTyp $consentType, ); - $hasUnstableConsentHash = $this->_hashService->retrieveConsentHashFromDb($dbh, $parameters); + $hasUnstableConsentHash = $this->_hashService->retrieveConsentHash($parameters); if ($hasUnstableConsentHash) { return true; @@ -200,6 +170,6 @@ private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentTyp $consentType, ); - return $this->_hashService->retrieveConsentHashFromDb($dbh, $parameters); + return $this->_hashService->retrieveConsentHash($parameters); } } diff --git a/library/EngineBlock/Corto/Model/Consent/Factory.php b/library/EngineBlock/Corto/Model/Consent/Factory.php index 6629d943ff..d046b5428b 100644 --- a/library/EngineBlock/Corto/Model/Consent/Factory.php +++ b/library/EngineBlock/Corto/Model/Consent/Factory.php @@ -26,9 +26,6 @@ class EngineBlock_Corto_Model_Consent_Factory /** @var EngineBlock_Corto_Filter_Command_Factory */ private $_filterCommandFactory; - /** @var EngineBlock_Database_ConnectionFactory */ - private $_databaseConnectionFactory; - /** * @var ConsentHashService */ @@ -36,16 +33,12 @@ class EngineBlock_Corto_Model_Consent_Factory /** * @param EngineBlock_Corto_Filter_Command_Factory $filterCommandFactory - * @param EngineBlock_Database_ConnectionFactory $databaseConnectionFactory */ public function __construct( EngineBlock_Corto_Filter_Command_Factory $filterCommandFactory, - EngineBlock_Database_ConnectionFactory $databaseConnectionFactory, ConsentHashService $hashService - ) - { + ) { $this->_filterCommandFactory = $filterCommandFactory; - $this->_databaseConnectionFactory = $databaseConnectionFactory; $this->_hashService = $hashService; } @@ -76,7 +69,6 @@ public function create( $proxyServer->getConfig('ConsentStoreValues', true), $response, $attributes, - $this->_databaseConnectionFactory, $amPriorToConsent, $consentEnabled, $this->_hashService diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index d84e210205..628f911767 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -33,7 +33,7 @@ use function strtolower; use function unserialize; -final class ConsentHashService +final class ConsentHashService implements ConsentHashServiceInterface { /** * @var ConsentRepository @@ -45,12 +45,12 @@ public function __construct(ConsentRepository $consentHashRepository) $this->consentRepository = $consentHashRepository; } - public function retrieveConsentHashFromDb(array $parameters): bool + public function retrieveConsentHash(array $parameters): bool { return $this->consentRepository->hasConsentHash($parameters); } - public function storeConsentHashInDb(array $parameters): bool + public function storeConsentHash(array $parameters): bool { return $this->consentRepository->storeConsentHash($parameters); } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php new file mode 100644 index 0000000000..3e475d31a6 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php @@ -0,0 +1,32 @@ +mockedDatabaseConnection = Phake::mock('EngineBlock_Database_ConnectionFactory'); $mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + $this->consentService = Mockery::mock(ConsentHashServiceInterface::class); + $this->consentDisabled = new EngineBlock_Corto_Model_Consent( "consent", true, $mockedResponse, [], - $this->mockedDatabaseConnection, false, - false + false, + $this->consentService ); $this->consent = new EngineBlock_Corto_Model_Consent( @@ -45,31 +50,37 @@ public function setUp(): void true, $mockedResponse, [], - $this->mockedDatabaseConnection, false, - true + true, + $this->consentService ); } public function testConsentDisabledDoesNotWriteToDatabase() { $serviceProvider = new ServiceProvider("service-provider-entity-id"); + + $this->consentService->shouldReceive('getUnstableAttributesHash'); + $this->consentService->shouldNotReceive('storeConsentHash'); $this->consentDisabled->explicitConsentWasGivenFor($serviceProvider); $this->consentDisabled->implicitConsentWasGivenFor($serviceProvider); $this->consentDisabled->giveExplicitConsentFor($serviceProvider); $this->consentDisabled->giveImplicitConsentFor($serviceProvider); - - Phake::verify($this->mockedDatabaseConnection, Phake::times(0))->create(); } public function testConsentWriteToDatabase() { $serviceProvider = new ServiceProvider("service-provider-entity-id"); + + $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); + $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(sha1('unstable')); $this->consent->explicitConsentWasGivenFor($serviceProvider); + + $this->consentService->shouldReceive('getStableAttributesHash')->andReturn(sha1('stable')); $this->consent->implicitConsentWasGivenFor($serviceProvider); + + $this->consentService->shouldReceive('storeConsentHash')->andReturn(true); $this->consent->giveExplicitConsentFor($serviceProvider); $this->consent->giveImplicitConsentFor($serviceProvider); - - Phake::verify($this->mockedDatabaseConnection, Phake::times(4))->create(); } } From 5398abb4103abf5b23a85e61daaa97a807fdbb75 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 15:53:36 +0200 Subject: [PATCH 05/15] Integrate upstream consent changes --- config/services/compat.yml | 1 - library/EngineBlock/Application/DiContainer.php | 2 +- library/EngineBlock/Corto/Model/Consent.php | 2 +- library/EngineBlock/Corto/Module/Service/ProvideConsent.php | 2 +- .../EngineBlock/Service/Consent/ConsentService.php | 5 +++-- .../EngineBlock/Service/Consent/ConsentServiceInterface.php | 2 +- .../Authentication/Repository/DbalConsentRepository.php | 5 ++--- .../EngineBlockBundle/Controller/Api/ConsentController.php | 3 +-- tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php | 2 +- .../Test/Corto/Module/Service/ProvideConsentTest.php | 2 +- .../OpenConext/EngineBlock/Service/ConsentServiceTest.php | 1 + 11 files changed, 13 insertions(+), 14 deletions(-) diff --git a/config/services/compat.yml b/config/services/compat.yml index 984752fec2..883cd1d12b 100644 --- a/config/services/compat.yml +++ b/config/services/compat.yml @@ -50,7 +50,6 @@ services: class: EngineBlock_Corto_Model_Consent_Factory arguments: - "@engineblock.compat.corto_filter_command_factory" - - "@engineblock.compat.database_connection_factory" - "@engineblock.service.consent.ConsentHashService" engineblock.compat.saml2_id_generator: diff --git a/library/EngineBlock/Application/DiContainer.php b/library/EngineBlock/Application/DiContainer.php index 0916e348a8..ecf3ff2b81 100644 --- a/library/EngineBlock/Application/DiContainer.php +++ b/library/EngineBlock/Application/DiContainer.php @@ -161,7 +161,7 @@ public function getAuthenticationLoopGuard() } /** - * @return OpenConext\EngineBlock\Service\ConsentService + * @return OpenConext\EngineBlock\Service\Consent\ConsentService */ public function getConsentService() { diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 470e82558b..e52197e718 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -19,7 +19,7 @@ use Doctrine\DBAL\Statement; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; -use OpenConext\EngineBlock\Service\ConsentHashServiceInterface; +use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; class EngineBlock_Corto_Model_Consent { diff --git a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php index 767650ae83..45f56c0253 100644 --- a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php @@ -19,7 +19,7 @@ use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface; -use OpenConext\EngineBlock\Service\ConsentServiceInterface; +use OpenConext\EngineBlock\Service\Consent\ConsentServiceInterface; use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface; use OpenConext\EngineBlockBundle\Service\DiscoverySelectionService; use Psr\Log\LogLevel; diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php index b73b6b805c..2fce493864 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php @@ -16,7 +16,7 @@ * limitations under the License. */ -namespace OpenConext\EngineBlock\Service; +namespace OpenConext\EngineBlock\Service\Consent; use Exception; use OpenConext\EngineBlock\Authentication\Dto\Consent; @@ -25,6 +25,7 @@ use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use OpenConext\EngineBlock\Authentication\Value\CollabPersonId; use OpenConext\EngineBlock\Exception\RuntimeException; +use OpenConext\EngineBlock\Service\MetadataServiceInterface; use OpenConext\Value\Saml\EntityId; use Psr\Log\LoggerInterface; use function sprintf; @@ -37,7 +38,7 @@ final class ConsentService implements ConsentServiceInterface private $consentRepository; /** - * @var MetadataService + * @var MetadataServiceInterface */ private $metadataService; diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php index 4ebb10c832..32f800ec9a 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php @@ -16,7 +16,7 @@ * limitations under the License. */ -namespace OpenConext\EngineBlock\Service; +namespace OpenConext\EngineBlock\Service\Consent; use OpenConext\EngineBlock\Authentication\Dto\ConsentList; use OpenConext\EngineBlock\Authentication\Value\CollabPersonId; diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index f4f8b17b78..4781453f6a 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -176,11 +176,10 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b public function hasConsentHash(array $parameters): bool { try { - $query = " - SELECT + $query = " SELECT * FROM - {$this->_tableName} + consent WHERE hashed_user_id = ? AND diff --git a/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php b/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php index 3f1ce4d40a..4d9e34056b 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php @@ -19,8 +19,7 @@ namespace OpenConext\EngineBlockBundle\Controller\Api; use OpenConext\EngineBlock\Exception\RuntimeException; -use OpenConext\EngineBlock\Http\Exception\AccessDeniedException; -use OpenConext\EngineBlock\Service\ConsentServiceInterface; +use OpenConext\EngineBlock\Service\Consent\ConsentServiceInterface; use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; use OpenConext\EngineBlockBundle\Factory\CollabPersonIdFactory; use OpenConext\EngineBlockBundle\Http\Exception\ApiAccessDeniedHttpException; diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index 38f00216d8..a948b31f41 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -18,7 +18,7 @@ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; -use OpenConext\EngineBlock\Service\ConsentHashServiceInterface; +use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; use PHPUnit\Framework\TestCase; class EngineBlock_Corto_Model_ConsentTest extends TestCase diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php index 10e78a8d7e..971d39945f 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php @@ -23,7 +23,7 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\MetadataRepository\InMemoryMetadataRepository; use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface; -use OpenConext\EngineBlock\Service\ConsentServiceInterface; +use OpenConext\EngineBlock\Service\Consent\ConsentServiceInterface; use OpenConext\EngineBlock\Service\Dto\ProcessingStateStep; use OpenConext\EngineBlock\Service\ProcessingStateHelper; use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface; diff --git a/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php index 9e77de6c2d..c1d5801b9b 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php @@ -24,6 +24,7 @@ use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use OpenConext\EngineBlock\Authentication\Value\CollabPersonId; use OpenConext\EngineBlock\Authentication\Value\CollabPersonUuid; +use OpenConext\EngineBlock\Service\Consent\ConsentService; use PHPUnit\Framework\Attributes\DoesNotPerformAssertions; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; From c007bb0cf3550fec8c5a160f395892bbb30bcecc Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 08:41:04 +0200 Subject: [PATCH 06/15] Add comment to getUnstableAttributesHash This might prove usefull down the road --- .../EngineBlock/Service/Consent/ConsentHashService.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index 628f911767..876f84993a 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -60,9 +60,13 @@ public function countTotalConsent($consentUid): int return $this->consentRepository->countTotalConsent($consentUid); } + /** + * The old way of calculating the attribute hash, this is not stable as a change of the attribute order, + * change of case, stray/empty attributes, and renumbered indexes can cause the hash to change. Leaving the + * user to give consent once again for a service she previously gave consent for. + */ public function getUnstableAttributesHash(array $attributes, bool $mustStoreValues): string { - $hashBase = null; if ($mustStoreValues) { ksort($attributes); $hashBase = serialize($attributes); From f0558798995b98e6bf6a365436b79f3b994b77b2 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:26:01 +0200 Subject: [PATCH 07/15] Add the `attribute_stable` column to `consent` --- .../Version20220503092351.php | 30 +++++++++++++++++++ .../Authentication/Entity/Consent.php | 6 ++++ 2 files changed, 36 insertions(+) create mode 100644 database/DoctrineMigrations/Version20220503092351.php diff --git a/database/DoctrineMigrations/Version20220503092351.php b/database/DoctrineMigrations/Version20220503092351.php new file mode 100644 index 0000000000..6218a8a936 --- /dev/null +++ b/database/DoctrineMigrations/Version20220503092351.php @@ -0,0 +1,30 @@ +abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.'); + + $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) NOT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL'); + } + + public function down(Schema $schema) : void + { + // this down() migration is auto-generated, please modify it to your needs + $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.'); + + $this->addSql('ALTER TABLE consent DROP attribute_stable, CHANGE attribute attribute VARCHAR(80) CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`'); + } +} diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php index b23788f512..b8d79912e8 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php @@ -58,6 +58,12 @@ class Consent #[ORM\Column(type: \Doctrine\DBAL\Types\Types::STRING, length: 80)] public ?string $attribute = null; + /** + * @var string + */ + #[ORM\Column(name: 'attribute_stable', type: \Doctrine\DBAL\Types\Types::STRING, length: 80, nullable: true)] + public ?string $attributeStable = null; + /** * @var string */ From b0471d11d235ec11778b13013ee426a9ceb2e6d0 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:26:52 +0200 Subject: [PATCH 08/15] Add ConsentVersion value object This represents the consent type at hand (implicit or explicit consent). A third option is that no consent has been given. --- .../Authentication/Value/ConsentVersion.php | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 src/OpenConext/EngineBlock/Authentication/Value/ConsentVersion.php diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentVersion.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentVersion.php new file mode 100644 index 0000000000..1257140e8c --- /dev/null +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentVersion.php @@ -0,0 +1,77 @@ +consentVersion = $consentVersion; + } + + public function given(): bool + { + return $this->consentVersion !== self::NOT_GIVEN; + } + + /** + * @return string + */ + public function __toString() + { + return $this->consentVersion; + } + + public function isUnstable(): bool + { + return $this->consentVersion === self::UNSTABLE; + } +} From 916c05d0c7d93f69cfc012a750ea1b8301ad3330 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:28:08 +0200 Subject: [PATCH 09/15] Integrate stable attribute hash requirements The requirements are stated in the story: https://www.pivotaltracker.com/story/show/176513931 and specifically these points: A challenge is that we do not want to invalidate all given consents with the current algorithm. So we implement it as follows: * We do not touch the existing consent hashing method at all * We create a new hashing method that is more stable. * We cover this new method with an abundance of unit tests to verify the stability given all sorts of inputs. * We change the consent query from (pseudocode): SELECT * FROM consent WHERE user = me AND consenthash = hashfromoldmethod OR consenthash = hashfromnewmethod * Newly given consent will be stored with the new hash. * When old consent matched, still generate new consent hash (without showing consent screen --- library/EngineBlock/Corto/Model/Consent.php | 62 ++++- .../Corto/Module/Service/ProcessConsent.php | 2 + .../Corto/Module/Service/ProvideConsent.php | 4 + .../Repository/ConsentRepository.php | 24 ++ .../Service/Consent/ConsentHashService.php | 10 + .../Consent/ConsentHashServiceInterface.php | 10 + .../Repository/DbalConsentRepository.php | 70 ++++- .../Controller/Api/ConsentControllerTest.php | 4 +- .../Api/DeprovisionControllerTest.php | 1 + .../Corto/Model/ConsentIntegrationTest.php | 262 ++++++++++++++++++ .../Test/Corto/Model/ConsentTest.php | 8 +- 11 files changed, 437 insertions(+), 20 deletions(-) create mode 100644 tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index e52197e718..7134b6ed86 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -16,7 +16,7 @@ * limitations under the License. */ -use Doctrine\DBAL\Statement; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; @@ -84,14 +84,27 @@ public function __construct( public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool { - return !$this->_consentEnabled || - $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); + $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); + return !$this->_consentEnabled || $consent->given(); + } + + /** + * Although the user has given consent previously we want to upgrade the deprecated unstable consent + * to the new stable consent type. + * https://www.pivotaltracker.com/story/show/176513931 + */ + public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string $consentType): void + { + $consentVersion = $this->_hasStoredConsent($serviceProvider, $consentType); + if ($consentVersion->isUnstable()) { + $this->_updateConsent($serviceProvider, $consentType); + } } public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool { - return !$this->_consentEnabled || - $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); + $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); + return !$this->_consentEnabled || $consent->given(); } public function giveExplicitConsentFor(ServiceProvider $serviceProvider): bool @@ -148,28 +161,47 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): return $this->_hashService->storeConsentHash($parameters); } - private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): bool + private function _updateConsent(ServiceProvider $serviceProvider, $consentType): bool { $parameters = array( + $this->_getStableAttributesHash($this->_responseAttributes), + $this->_getAttributesHash($this->_responseAttributes), sha1($this->_getConsentUid()), $serviceProvider->entityId, - $this->_getAttributesHash($this->_responseAttributes), $consentType, ); - $hasUnstableConsentHash = $this->_hashService->retrieveConsentHash($parameters); + return $this->_hashService->updateConsentHash($parameters); + } - if ($hasUnstableConsentHash) { - return true; + private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): ConsentVersion + { + $consentUuid = sha1($this->_getConsentUid()); + $parameters = [ + $consentUuid, + $serviceProvider->entityId, + $this->_getStableAttributesHash($this->_responseAttributes), + $consentType, + ]; + $hasStableConsentHash = $this->_hashService->retrieveStableConsentHash($parameters); + + if ($hasStableConsentHash) { + return ConsentVersion::stable(); } - $parameters[2] = array( - sha1($this->_getConsentUid()), + $parameters = [ + $consentUuid, $serviceProvider->entityId, - $this->_getStableAttributesHash($this->_responseAttributes), + $this->_getAttributesHash($this->_responseAttributes), $consentType, - ); + ]; + + $hasUnstableConsentHash = $this->_hashService->retrieveConsentHash($parameters); + + if ($hasUnstableConsentHash) { + return ConsentVersion::unstable(); + } - return $this->_hashService->retrieveConsentHash($parameters); + return ConsentVersion::notGiven(); } } diff --git a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php index 97cf0300b6..4dc3381efb 100644 --- a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php @@ -16,6 +16,7 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Authentication\Value\ConsentType; use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface; use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface; use SAML2\Constants; @@ -102,6 +103,7 @@ public function serve($serviceName, Request $httpRequest) if (!$consentRepository->explicitConsentWasGivenFor($serviceProvider)) { $consentRepository->giveExplicitConsentFor($destinationMetadata); } + $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT); $response->setConsent(Constants::CONSENT_OBTAINED); $response->setDestination($response->getReturn()); diff --git a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php index 45f56c0253..2e8addfa41 100644 --- a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php @@ -16,6 +16,7 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Authentication\Value\ConsentType; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface; @@ -149,6 +150,7 @@ public function serve($serviceName, Request $httpRequest) if (!$consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata)) { $consentRepository->giveImplicitConsentFor($serviceProviderMetadata); } + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT); $response->setConsent(Constants::CONSENT_INAPPLICABLE); $response->setDestination($response->getReturn()); @@ -166,6 +168,8 @@ public function serve($serviceName, Request $httpRequest) $priorConsent = $consentRepository->explicitConsentWasGivenFor($serviceProviderMetadata); if ($priorConsent) { + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_EXPLICIT); + $response->setConsent(Constants::CONSENT_PRIOR); $response->setDestination($response->getReturn()); diff --git a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php index b6a17a16ec..3361a8f6cc 100644 --- a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php +++ b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php @@ -38,9 +38,33 @@ public function deleteAllFor($userId); public function deleteOneFor(string $userId, string $serviceProviderEntityId): bool; + /** + * Test if the consent row is set with the legacy (unstable) consent hash + * This is the consent hash that was originally created by EB. It can change + * based on factors that should not result in a hash change per se. Think of the + * change of the attribute ordering, case change or the existence of empty + * attribute values. + */ public function hasConsentHash(array $parameters): bool; + /** + * Tests the presence of the stable consent hash + * + * The stable consent hash is used by default, it is not affected by attribute order, case change + * or other irrelevant factors that could result in a changed hash calculation. + */ + public function hasStableConsentHash(array $parameters): bool; + + /** + * By default stores the stable consent hash. The legacy consent hash is left. + */ public function storeConsentHash(array $parameters): bool; + /** + * When a deprecated unstable consent hash is encoutered, we upgrade it to the new format using this + * update consent hash method. + */ + public function updateConsentHash(array $parameters): bool; + public function countTotalConsent($consentUid): int; } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index 876f84993a..12c88a184b 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -50,11 +50,21 @@ public function retrieveConsentHash(array $parameters): bool return $this->consentRepository->hasConsentHash($parameters); } + public function retrieveStableConsentHash(array $parameters): bool + { + return $this->consentRepository->hasStableConsentHash($parameters); + } + public function storeConsentHash(array $parameters): bool { return $this->consentRepository->storeConsentHash($parameters); } + public function updateConsentHash(array $parameters): bool + { + return $this->consentRepository->updateConsentHash($parameters); + } + public function countTotalConsent($consentUid): int { return $this->consentRepository->countTotalConsent($consentUid); diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php index 3e475d31a6..17a777823a 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php @@ -20,10 +20,20 @@ interface ConsentHashServiceInterface { + /** + * Retrieve the old-style (deprecated) unstable consent hash + */ public function retrieveConsentHash(array $parameters): bool; + /** + * Retrieve the stable consent hash + */ + public function retrieveStableConsentHash(array $parameters): bool; + public function storeConsentHash(array $parameters): bool; + public function updateConsentHash(array $parameters): bool; + public function countTotalConsent($consentUid): int; public function getUnstableAttributesHash(array $attributes, bool $mustStoreValues): string; diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index 4781453f6a..ccfaa177e1 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -206,15 +206,46 @@ public function hasConsentHash(array $parameters): bool throw new RuntimeException(sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage())); } } + /** + * @throws RuntimeException + */ + public function hasStableConsentHash(array $parameters): bool + { + try { + $query = " SELECT + * + FROM + consent + WHERE + hashed_user_id = ? + AND + service_id = ? + AND + attribute_stable = ? + AND + consent_type = ? + AND + deleted_at IS NULL + "; + + $statement = $this->connection->prepare($query); + $statement->execute($parameters); + $rows = $statement->fetchAll(); + + return count($rows) >= 1; + } catch (PDOException $e) { + throw new RuntimeException(sprintf('Consent retrieval on stable consent hash failed! Error: "%s"', $e->getMessage())); + } + } /** * @throws RuntimeException */ public function storeConsentHash(array $parameters): bool { - $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, consent_type, consent_date, deleted_at) + $query = "INSERT INTO consent (hashed_user_id, service_id, attribute_stable, consent_type, consent_date, deleted_at) VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') - ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), consent_type=VALUES(consent_type), consent_date=NOW()"; + ON DUPLICATE KEY UPDATE attribute_stable=VALUES(attribute_stable), consent_type=VALUES(consent_type), consent_date=NOW()"; $statement = $this->connection->prepare($query); if (!$statement) { throw new RuntimeException("Unable to create a prepared statement to insert consent?!"); @@ -229,6 +260,41 @@ public function storeConsentHash(array $parameters): bool return true; } + /** + * @throws RuntimeException + */ + public function updateConsentHash(array $parameters): bool + { + $query = " + UPDATE + consent + SET + attribute_stable = ? + WHERE + attribute = ? + AND + hashed_user_id = ? + AND + service_id = ? + AND + consent_type = ? + AND + deleted_at IS NULL + "; + $statement = $this->connection->prepare($query); + if (!$statement) { + throw new RuntimeException("Unable to create a prepared statement to update consent?!"); + } + + if (!$statement->execute($parameters)) { + throw new RuntimeException( + sprintf('Error storing updated consent: "%s"', var_export($statement->errorInfo(), true)) + ); + } + + return true; + } + /** * @throws RuntimeException */ diff --git a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php index ab6860cf48..3a493cbc1e 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php @@ -530,6 +530,7 @@ private function addConsentFixture($userId, $serviceId, $attributeHash, $consent 'hashed_user_id' => ':user_id', 'service_id' => ':service_id', 'attribute' => ':attribute', + 'attribute_stable' => ':attribute_stable', 'consent_type' => ':consent_type', 'consent_date' => ':consent_date', 'deleted_at' => ':deleted_at', @@ -537,7 +538,8 @@ private function addConsentFixture($userId, $serviceId, $attributeHash, $consent ->setParameters([ 'user_id' => sha1($userId), 'service_id' => $serviceId, - 'attribute' => $attributeHash, + 'attribute' => '', + 'attribute_stable' => $attributeHash, 'consent_type' => $consentType, 'consent_date' => $consentDate, 'deleted_at' => $deletedAt, diff --git a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php index 61a1646a3f..d5db74afae 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php @@ -407,6 +407,7 @@ private function addConsentFixture($userId, $serviceId, $attributeHash, $consent 'hashed_user_id' => ':user_id', 'service_id' => ':service_id', 'attribute' => ':attribute', + 'attribute_stable' => ':attribute', 'consent_type' => ':consent_type', 'consent_date' => ':consent_date', 'deleted_at' => '"0000-00-00 00:00:00"', diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php new file mode 100644 index 0000000000..96d6909147 --- /dev/null +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -0,0 +1,262 @@ +response = Mockery::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class); + $this->consentRepository = Mockery::mock(ConsentRepository::class); + $this->consentService = new ConsentHashService($this->consentRepository); + + $this->consent = new EngineBlock_Corto_Model_Consent( + "consent", + true, + $this->response, + [], + false, + true, + $this->consentService + ); + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_no_previous_consent_given($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + // No consent is given previously + $this->consentRepository + ->shouldReceive('hasConsentHash') + ->once() + ->andReturn(false); + $this->consentRepository + ->shouldReceive('hasStableConsentHash') + ->once() + ->andReturn(false); + switch ($consentType) { + case ConsentType::TYPE_EXPLICIT: + $this->assertFalse($this->consent->explicitConsentWasGivenFor($serviceProvider)); + break; + case ConsentType::TYPE_IMPLICIT: + $this->assertFalse($this->consent->implicitConsentWasGivenFor($serviceProvider)); + break; + } + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_unstable_previous_consent_given($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + // Stable consent is not yet stored + $this->consentRepository + ->shouldReceive('hasStableConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->once() + ->andReturn(false); + // Old-style (unstable) consent was given previously + $this->consentRepository + ->shouldReceive('hasConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->once() + ->andReturn(true); + + + switch ($consentType) { + case ConsentType::TYPE_EXPLICIT: + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)); + break; + case ConsentType::TYPE_IMPLICIT: + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)); + break; + } + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_stable_consent_given($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + // Stable consent is not yet stored + $this->consentRepository + ->shouldReceive('hasStableConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->once() + ->andReturn(true); + // Old-style (unstable) consent was given previously + $this->consentRepository + ->shouldNotReceive('hasConsentHash'); + + switch ($consentType) { + case ConsentType::TYPE_EXPLICIT: + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)); + break; + case ConsentType::TYPE_IMPLICIT: + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)); + break; + } + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_give_consent_no_unstable_consent_given($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + // Now assert that the new stable consent hash is going to be set + $this->consentRepository + ->shouldReceive('storeConsentHash') + ->once() + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->andReturn(true); + + switch ($consentType) { + case ConsentType::TYPE_EXPLICIT: + $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); + break; + case ConsentType::TYPE_IMPLICIT: + $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); + break; + } + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_give_consent_unstable_consent_given($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + // Now assert that the new stable consent hash is going to be set + $this->consentRepository + ->shouldReceive('storeConsentHash') + ->once() + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->andReturn(true); + + switch ($consentType) { + case ConsentType::TYPE_EXPLICIT: + $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); + break; + case ConsentType::TYPE_IMPLICIT: + $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); + break; + } + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_upgrade_to_stable_consent($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->twice() + ->andReturn('collab:person:id:org-a:joe-a'); + // Stable consent is not yet stored + $this->consentRepository + ->shouldReceive('hasStableConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->once() + ->andReturn(false); + // Old-style (unstable) consent was given previously + $this->consentRepository + ->shouldReceive('hasConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->once() + ->andReturn(true); + // Now assert that the new stable consent hash is going to be set + $this->consentRepository + ->shouldReceive('updateConsentHash') + ->once() + ->with(['8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', '0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', $consentType]) + ->andReturn(true); + + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_upgrade_to_stable_consent_not_applied_when_stable($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + // Stable consent is not yet stored + $this->consentRepository + ->shouldReceive('hasStableConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->once() + ->andReturn(true); + // Now assert that the new stable consent hash is NOT going to be set + $this->consentRepository + ->shouldNotReceive('storeConsentHash'); + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); + } + + public function consentTypeProvider() + { + yield [ConsentType::TYPE_IMPLICIT]; + yield [ConsentType::TYPE_EXPLICIT]; + } +} diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index a948b31f41..8e8f6aa738 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -61,7 +61,10 @@ public function testConsentDisabledDoesNotWriteToDatabase() $serviceProvider = new ServiceProvider("service-provider-entity-id"); $this->consentService->shouldReceive('getUnstableAttributesHash'); - $this->consentService->shouldNotReceive('storeConsentHash'); + $this->consentService->shouldReceive('getStableAttributesHash'); + $this->consentService->shouldReceive('retrieveStableConsentHash'); + $this->consentService->shouldReceive('retrieveConsentHash'); + $this->consentService->shouldReceive('storeConsentHash'); $this->consentDisabled->explicitConsentWasGivenFor($serviceProvider); $this->consentDisabled->implicitConsentWasGivenFor($serviceProvider); $this->consentDisabled->giveExplicitConsentFor($serviceProvider); @@ -71,9 +74,10 @@ public function testConsentDisabledDoesNotWriteToDatabase() public function testConsentWriteToDatabase() { $serviceProvider = new ServiceProvider("service-provider-entity-id"); - + $this->consentService->shouldReceive('getStableAttributesHash'); $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(sha1('unstable')); + $this->consentService->shouldReceive('retrieveStableConsentHash'); $this->consent->explicitConsentWasGivenFor($serviceProvider); $this->consentService->shouldReceive('getStableAttributesHash')->andReturn(sha1('stable')); From 53c387606b8e3d39c340964dd07328513f907bcc Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 4 May 2022 11:58:58 +0200 Subject: [PATCH 10/15] Support NameId objects in consent hash generator The NameID objects where causing issues when creating a stabilized consent hash. The objects can not be serialized/unserialized. By normalizing them before starting to perform other normalization tasks on the attribute array, this issue can be prevented. --- .../Service/Consent/ConsentHashService.php | 27 ++++++++++++++++--- .../Consent/ConsentHashServiceTest.php | 25 +++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index 12c88a184b..7846d2e780 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -19,6 +19,8 @@ namespace OpenConext\EngineBlock\Service\Consent; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; +use OpenConext\UserLifecycle\Domain\ValueObject\Client\Name; +use SAML2\XML\saml\NameID; use function array_filter; use function array_keys; use function array_values; @@ -30,6 +32,7 @@ use function serialize; use function sha1; use function sort; +use function str_replace; use function strtolower; use function unserialize; @@ -90,7 +93,8 @@ public function getUnstableAttributesHash(array $attributes, bool $mustStoreValu public function getStableAttributesHash(array $attributes, bool $mustStoreValues) : string { - $lowerCasedAttributes = $this->caseNormalizeStringArray($attributes); + $nameIdNormalizedAttributes = $this->nameIdNormalize($attributes); + $lowerCasedAttributes = $this->caseNormalizeStringArray($nameIdNormalizedAttributes); $hashBase = $mustStoreValues ? $this->createHashBaseWithValues($lowerCasedAttributes) : $this->createHashBaseWithoutValues($lowerCasedAttributes); @@ -112,11 +116,13 @@ private function createHashBaseWithoutValues(array $lowerCasedAttributes): strin /** * Lowercases all array keys and values. - * Performs the lowercasing on a copy (which is returned), to avoid side-effects. */ private function caseNormalizeStringArray(array $original): array { - return unserialize(strtolower(serialize($original))); + $serialized = serialize($original); + $lowerCased = strtolower($serialized); + $unserialized = unserialize($lowerCased); + return $unserialized; } /** @@ -187,4 +193,19 @@ private function isBlank($value): bool { return empty($value) && !is_numeric($value); } + + /** + * NameId objects can not be serialized/unserialized after being lower cased + * Thats why the object is converted to a simple array representation where only the + * relevant NameID aspects are stored. + */ + private function nameIdNormalize(array $attributes): array + { + array_walk_recursive($attributes, function (&$value) { + if ($value instanceof NameID) { + $value = ['value' => $value->getValue(), 'Format' => $value->getFormat()]; + } + }); + return $attributes; + } } diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index ace9c0a432..51464edda0 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -22,6 +22,7 @@ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use PHPUnit\Framework\TestCase; +use SAML2\XML\saml\NameID; class ConsentHashServiceTest extends TestCase { @@ -449,4 +450,28 @@ public function test_stable_attribute_hash_multiple_value_vs_single_value_sequen $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($attributesSingleValue, false)); $this->assertNotEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($attributesSingleValue, true)); } + + public function test_stable_attribute_hash_can_handle_nameid_objects() + { + $nameId = new NameID(); + $nameId->setValue('83aa0a79363edcf872c966b0d6eaf3f5e26a6a77'); + $nameId->setFormat('urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'); + + $attributes = [ + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:cn' => ['John Doe'], + 'urn:mace:dir:attribute-def:sn' => ['Doe'], + 'urn:mace:dir:attribute-def:eduPersonPrincipalName' => ['j.doe@example.com'], + 'urn:mace:dir:attribute-def:givenName' => ['John'], + 'urn:mace:dir:attribute-def:mail' => ['j.doe@example.com'], + 'urn:mace:terena.org:attribute-def:schacHomeOrganization' => ['example.com'], + 'nl:surf:test:something' => [0 => 'arbitrary-value'], + 'urn:mace:dir:attribute-def:eduPersonTargetedID' => [$nameId], + 'urn:oid:1.3.6.1.4.1.5923.1.1.1.10' => [$nameId], + ]; + + $hash = $this->chs->getStableAttributesHash($attributes, false); + $this->assertTrue(is_string($hash)); + } } From da2ca634d945485048c206827b3433d8169cd0bc Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Thu, 5 May 2022 10:55:14 +0200 Subject: [PATCH 11/15] Optimize the consent was given methods Only get the stored consent when consent is enabled. The other methods need no adjusting as the short-circuiting by the first condition prevents those expressions from being executed. Consider: `(true || $this->expensiveMethodCall())` The expensiveMethodCall is never called as the first condition already decided the rest of the condition. https://www.php.net/manual/en/language.operators.logical.php --- library/EngineBlock/Corto/Model/Consent.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 7134b6ed86..049ede388c 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -84,8 +84,11 @@ public function __construct( public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool { + if (!$this->_consentEnabled) { + return true; + } $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); - return !$this->_consentEnabled || $consent->given(); + return $consent->given(); } /** @@ -103,8 +106,11 @@ public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool { + if (!$this->_consentEnabled) { + return true; + } $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); - return !$this->_consentEnabled || $consent->given(); + return $consent->given(); } public function giveExplicitConsentFor(ServiceProvider $serviceProvider): bool From d05aad43614216399ad1fcad21e74a48f5c04dbb Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Thu, 5 May 2022 11:40:36 +0200 Subject: [PATCH 12/15] Retrieve old/new style attribute hash in one go Previously when testing if previous consent was given was based on testing for old-style attribute hash calculation. And if that one did not exist, the new (stable) attribute hash was tested. Having that in two queries made little sense as it can easily be performed in one action. The interface of the repo and service changed slightly, as now a value object is returned instead of a boolean value. The value object reflects the version (type of attribute hash) of consent that was given. That can either be: unstable, stable or not given at all. --- library/EngineBlock/Corto/Model/Consent.php | 22 +-------- .../Repository/ConsentRepository.php | 17 ++----- .../Service/Consent/ConsentHashService.php | 8 +--- .../Consent/ConsentHashServiceInterface.php | 11 ++--- .../Repository/DbalConsentRepository.php | 43 ++++-------------- .../Corto/Model/ConsentIntegrationTest.php | 45 ++++++------------- .../Test/Corto/Model/ConsentTest.php | 4 +- 7 files changed, 34 insertions(+), 116 deletions(-) diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 049ede388c..e289593b99 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -183,31 +183,13 @@ private function _updateConsent(ServiceProvider $serviceProvider, $consentType): private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): ConsentVersion { $consentUuid = sha1($this->_getConsentUid()); - $parameters = [ - $consentUuid, - $serviceProvider->entityId, - $this->_getStableAttributesHash($this->_responseAttributes), - $consentType, - ]; - $hasStableConsentHash = $this->_hashService->retrieveStableConsentHash($parameters); - - if ($hasStableConsentHash) { - return ConsentVersion::stable(); - } - $parameters = [ $consentUuid, $serviceProvider->entityId, $this->_getAttributesHash($this->_responseAttributes), + $this->_getStableAttributesHash($this->_responseAttributes), $consentType, ]; - - $hasUnstableConsentHash = $this->_hashService->retrieveConsentHash($parameters); - - if ($hasUnstableConsentHash) { - return ConsentVersion::unstable(); - } - - return ConsentVersion::notGiven(); + return $this->_hashService->retrieveConsentHash($parameters); } } diff --git a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php index 3361a8f6cc..72ceada16a 100644 --- a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php +++ b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php @@ -19,6 +19,7 @@ namespace OpenConext\EngineBlock\Authentication\Repository; use OpenConext\EngineBlock\Authentication\Model\Consent; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; interface ConsentRepository { @@ -39,21 +40,9 @@ public function deleteAllFor($userId); public function deleteOneFor(string $userId, string $serviceProviderEntityId): bool; /** - * Test if the consent row is set with the legacy (unstable) consent hash - * This is the consent hash that was originally created by EB. It can change - * based on factors that should not result in a hash change per se. Think of the - * change of the attribute ordering, case change or the existence of empty - * attribute values. + * Test if the consent row is set with an attribute hash either stable or unstable */ - public function hasConsentHash(array $parameters): bool; - - /** - * Tests the presence of the stable consent hash - * - * The stable consent hash is used by default, it is not affected by attribute order, case change - * or other irrelevant factors that could result in a changed hash calculation. - */ - public function hasStableConsentHash(array $parameters): bool; + public function hasConsentHash(array $parameters): ConsentVersion; /** * By default stores the stable consent hash. The legacy consent hash is left. diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index 7846d2e780..cfe25b286e 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -19,6 +19,7 @@ namespace OpenConext\EngineBlock\Service\Consent; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\UserLifecycle\Domain\ValueObject\Client\Name; use SAML2\XML\saml\NameID; use function array_filter; @@ -48,16 +49,11 @@ public function __construct(ConsentRepository $consentHashRepository) $this->consentRepository = $consentHashRepository; } - public function retrieveConsentHash(array $parameters): bool + public function retrieveConsentHash(array $parameters): ConsentVersion { return $this->consentRepository->hasConsentHash($parameters); } - public function retrieveStableConsentHash(array $parameters): bool - { - return $this->consentRepository->hasStableConsentHash($parameters); - } - public function storeConsentHash(array $parameters): bool { return $this->consentRepository->storeConsentHash($parameters); diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php index 17a777823a..99140782d1 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php @@ -18,17 +18,14 @@ namespace OpenConext\EngineBlock\Service\Consent; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; + interface ConsentHashServiceInterface { /** - * Retrieve the old-style (deprecated) unstable consent hash - */ - public function retrieveConsentHash(array $parameters): bool; - - /** - * Retrieve the stable consent hash + * Retrieve the consent hash */ - public function retrieveStableConsentHash(array $parameters): bool; + public function retrieveConsentHash(array $parameters): ConsentVersion; public function storeConsentHash(array $parameters): bool; diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index ccfaa177e1..901bc3900d 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -25,6 +25,7 @@ use OpenConext\EngineBlock\Authentication\Model\Consent; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Exception\RuntimeException; use PDO; use PDOException; @@ -173,7 +174,7 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b /** * @throws RuntimeException */ - public function hasConsentHash(array $parameters): bool + public function hasConsentHash(array $parameters): ConsentVersion { try { $query = " SELECT @@ -185,7 +186,7 @@ public function hasConsentHash(array $parameters): bool AND service_id = ? AND - attribute = ? + (attribute = ? OR attribute_stable = ?) AND consent_type = ? AND @@ -198,45 +199,17 @@ public function hasConsentHash(array $parameters): bool if (count($rows) < 1) { // No stored consent found - return false; + return ConsentVersion::notGiven(); } - return true; + if ($rows[0]['attribute_stable'] !== '') { + return ConsentVersion::stable(); + } + return ConsentVersion::unstable(); } catch (PDOException $e) { throw new RuntimeException(sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage())); } } - /** - * @throws RuntimeException - */ - public function hasStableConsentHash(array $parameters): bool - { - try { - $query = " SELECT - * - FROM - consent - WHERE - hashed_user_id = ? - AND - service_id = ? - AND - attribute_stable = ? - AND - consent_type = ? - AND - deleted_at IS NULL - "; - - $statement = $this->connection->prepare($query); - $statement->execute($parameters); - $rows = $statement->fetchAll(); - - return count($rows) >= 1; - } catch (PDOException $e) { - throw new RuntimeException(sprintf('Consent retrieval on stable consent hash failed! Error: "%s"', $e->getMessage())); - } - } /** * @throws RuntimeException diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index 96d6909147..a411cde945 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -20,6 +20,7 @@ use Mockery\MockInterface; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Consent\ConsentHashService; use OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository; @@ -73,11 +74,7 @@ public function test_no_previous_consent_given($consentType) $this->consentRepository ->shouldReceive('hasConsentHash') ->once() - ->andReturn(false); - $this->consentRepository - ->shouldReceive('hasStableConsentHash') - ->once() - ->andReturn(false); + ->andReturn(ConsentVersion::notGiven()); switch ($consentType) { case ConsentType::TYPE_EXPLICIT: $this->assertFalse($this->consent->explicitConsentWasGivenFor($serviceProvider)); @@ -98,18 +95,11 @@ public function test_unstable_previous_consent_given($consentType) ->once() ->andReturn('collab:person:id:org-a:joe-a'); // Stable consent is not yet stored - $this->consentRepository - ->shouldReceive('hasStableConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) - ->once() - ->andReturn(false); - // Old-style (unstable) consent was given previously $this->consentRepository ->shouldReceive('hasConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) ->once() - ->andReturn(true); - + ->andReturn(ConsentVersion::unstable()); switch ($consentType) { case ConsentType::TYPE_EXPLICIT: @@ -132,13 +122,10 @@ public function test_stable_consent_given($consentType) ->andReturn('collab:person:id:org-a:joe-a'); // Stable consent is not yet stored $this->consentRepository - ->shouldReceive('hasStableConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->shouldReceive('hasConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) ->once() - ->andReturn(true); - // Old-style (unstable) consent was given previously - $this->consentRepository - ->shouldNotReceive('hasConsentHash'); + ->andReturn(ConsentVersion::stable()); switch ($consentType) { case ConsentType::TYPE_EXPLICIT: @@ -211,18 +198,12 @@ public function test_upgrade_to_stable_consent($consentType) $this->response->shouldReceive('getNameIdValue') ->twice() ->andReturn('collab:person:id:org-a:joe-a'); - // Stable consent is not yet stored - $this->consentRepository - ->shouldReceive('hasStableConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) - ->once() - ->andReturn(false); // Old-style (unstable) consent was given previously $this->consentRepository ->shouldReceive('hasConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) ->once() - ->andReturn(true); + ->andReturn(ConsentVersion::unstable()); // Now assert that the new stable consent hash is going to be set $this->consentRepository ->shouldReceive('updateConsentHash') @@ -242,12 +223,12 @@ public function test_upgrade_to_stable_consent_not_applied_when_stable($consentT $this->response->shouldReceive('getNameIdValue') ->once() ->andReturn('collab:person:id:org-a:joe-a'); - // Stable consent is not yet stored + // Stable consent is stored $this->consentRepository - ->shouldReceive('hasStableConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->shouldReceive('hasConsentHash') + ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) ->once() - ->andReturn(true); + ->andReturn(ConsentVersion::stable()); // Now assert that the new stable consent hash is NOT going to be set $this->consentRepository ->shouldNotReceive('storeConsentHash'); diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index 8e8f6aa738..a82a8ff328 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -17,6 +17,7 @@ */ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; use PHPUnit\Framework\TestCase; @@ -76,8 +77,7 @@ public function testConsentWriteToDatabase() $serviceProvider = new ServiceProvider("service-provider-entity-id"); $this->consentService->shouldReceive('getStableAttributesHash'); $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); - $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(sha1('unstable')); - $this->consentService->shouldReceive('retrieveStableConsentHash'); + $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(ConsentVersion::stable()); $this->consent->explicitConsentWasGivenFor($serviceProvider); $this->consentService->shouldReceive('getStableAttributesHash')->andReturn(sha1('stable')); From bd88e678a15a5bf918aeae50b7d379cdd1f9d183 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Fri, 6 Mar 2026 13:40:39 +0100 Subject: [PATCH 13/15] Fix consent hash implementation and code quality - Guard upgradeAttributeHashFor against consent-disabled state: returns early without hitting the DB or recalculating hashes when consent is off - Fix DIP violation in Factory.php: type-hint ConsentHashServiceInterface instead of concrete ConsentHashService - Fix countAllFor memory issue: delegate to COUNT(*) query instead of loading all consent rows into memory - Fix null-guards in _hasStoredConsent() and _updateConsent(): return early when getConsentUid() returns null, consistent with _storeConsent() - Fix risky PHPUnit tests in ConsentTest: add assertions, use shouldNotReceive() - findAllFor: select attribute_stable alongside attribute, prefer it for new-format rows (fixes null attribute_hash in /api/consent response) - updateConsentHash: check rowCount(), log warning and return false on 0 rows - hasConsentHash: SELECT attribute_stable instead of SELECT * - ProcessConsent/ProvideConsent: skip upgradeAttributeHashFor on fresh consent - ConsentVersion: add isStable() counterpart to isUnstable() - Restore display_errors=0 in phpunit.xml to prevent PHP deprecation noise --- config/services/controllers/api.yml | 2 +- config/services/services.yml | 4 +- .../Version20220503092351.php | 4 +- .../EngineBlock/Application/DiContainer.php | 2 +- library/EngineBlock/Corto/Model/Consent.php | 55 +++++++---- .../Corto/Model/Consent/Factory.php | 6 +- .../Corto/Module/Service/ProcessConsent.php | 3 +- .../Corto/Module/Service/ProvideConsent.php | 3 +- .../Repository/ConsentRepository.php | 9 +- .../Authentication/Value/ConsentHashQuery.php | 31 ++++++ .../Value/ConsentStoreParameters.php | 30 ++++++ .../Value/ConsentUpdateParameters.php | 31 ++++++ .../Authentication/Value/ConsentVersion.php | 5 + .../Service/Consent/ConsentHashService.php | 12 ++- .../Consent/ConsentHashServiceInterface.php | 9 +- .../Service/Consent/ConsentService.php | 6 +- .../Repository/DbalConsentRepository.php | 96 ++++++++++++------- .../Corto/Model/ConsentIntegrationTest.php | 2 +- .../Test/Corto/Model/ConsentTest.php | 45 +++++---- tests/phpunit.xml | 1 + .../Consent/ConsentHashServiceTest.php | 2 +- 21 files changed, 260 insertions(+), 98 deletions(-) create mode 100644 src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php create mode 100644 src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php create mode 100644 src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php diff --git a/config/services/controllers/api.yml b/config/services/controllers/api.yml index 90686d60ff..6a88a67570 100644 --- a/config/services/controllers/api.yml +++ b/config/services/controllers/api.yml @@ -16,7 +16,7 @@ services: - '@security.token_storage' - '@security.access.decision_manager' - '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration' - - '@OpenConext\EngineBlock\Service\ConsentService' + - '@OpenConext\EngineBlock\Service\Consent\ConsentService' OpenConext\EngineBlockBundle\Controller\Api\DeprovisionController: arguments: diff --git a/config/services/services.yml b/config/services/services.yml index 3059dfb981..344336ab6d 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -78,8 +78,10 @@ services: engineblock.service.consent.ConsentHashService: class: OpenConext\EngineBlock\Service\Consent\ConsentHashService public: false + arguments: + - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository' - OpenConext\EngineBlock\Service\ConsentService: + OpenConext\EngineBlock\Service\Consent\ConsentService: arguments: - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository' - '@OpenConext\EngineBlock\Service\MetadataService' diff --git a/database/DoctrineMigrations/Version20220503092351.php b/database/DoctrineMigrations/Version20220503092351.php index 6218a8a936..fbf96c95d1 100644 --- a/database/DoctrineMigrations/Version20220503092351.php +++ b/database/DoctrineMigrations/Version20220503092351.php @@ -7,7 +7,7 @@ /** * Change to the consent schema - * 1. Added the `attribute_stable` column, string(80), not null + * 1. Added the `attribute_stable` column, string(80), nullable * 2. Changed the `attribute` column, has been made nullable */ final class Version20220503092351 extends AbstractMigration @@ -17,7 +17,7 @@ public function up(Schema $schema) : void // this up() migration is auto-generated, please modify it to your needs $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.'); - $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) NOT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL'); + $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) DEFAULT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL'); } public function down(Schema $schema) : void diff --git a/library/EngineBlock/Application/DiContainer.php b/library/EngineBlock/Application/DiContainer.php index ecf3ff2b81..03d4a6f3e6 100644 --- a/library/EngineBlock/Application/DiContainer.php +++ b/library/EngineBlock/Application/DiContainer.php @@ -165,7 +165,7 @@ public function getAuthenticationLoopGuard() */ public function getConsentService() { - return $this->container->get(\OpenConext\EngineBlock\Service\ConsentService::class); + return $this->container->get(\OpenConext\EngineBlock\Service\Consent\ConsentService::class); } /** diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index e289593b99..62f6ed488e 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -16,6 +16,9 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; @@ -98,6 +101,9 @@ public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): bo */ public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string $consentType): void { + if (!$this->_consentEnabled) { + return; + } $consentVersion = $this->_hasStoredConsent($serviceProvider, $consentType); if ($consentVersion->isUnstable()) { $this->_updateConsent($serviceProvider, $consentType); @@ -157,11 +163,11 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): return false; } - $parameters = array( - sha1($consentUuid), - $serviceProvider->entityId, - $this->_getStableAttributesHash($this->_responseAttributes), - $consentType, + $parameters = new ConsentStoreParameters( + hashedUserId: sha1($consentUuid), + serviceId: $serviceProvider->entityId, + attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), + consentType: $consentType, ); return $this->_hashService->storeConsentHash($parameters); @@ -169,12 +175,17 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): private function _updateConsent(ServiceProvider $serviceProvider, $consentType): bool { - $parameters = array( - $this->_getStableAttributesHash($this->_responseAttributes), - $this->_getAttributesHash($this->_responseAttributes), - sha1($this->_getConsentUid()), - $serviceProvider->entityId, - $consentType, + $consentUid = $this->_getConsentUid(); + if (!is_string($consentUid)) { + return false; + } + + $parameters = new ConsentUpdateParameters( + attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), + attributeHash: $this->_getAttributesHash($this->_responseAttributes), + hashedUserId: sha1($consentUid), + serviceId: $serviceProvider->entityId, + consentType: $consentType, ); return $this->_hashService->updateConsentHash($parameters); @@ -182,14 +193,18 @@ private function _updateConsent(ServiceProvider $serviceProvider, $consentType): private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): ConsentVersion { - $consentUuid = sha1($this->_getConsentUid()); - $parameters = [ - $consentUuid, - $serviceProvider->entityId, - $this->_getAttributesHash($this->_responseAttributes), - $this->_getStableAttributesHash($this->_responseAttributes), - $consentType, - ]; - return $this->_hashService->retrieveConsentHash($parameters); + $consentUid = $this->_getConsentUid(); + if (!is_string($consentUid)) { + return ConsentVersion::notGiven(); + } + + $query = new ConsentHashQuery( + hashedUserId: sha1($consentUid), + serviceId: $serviceProvider->entityId, + attributeHash: $this->_getAttributesHash($this->_responseAttributes), + attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), + consentType: $consentType, + ); + return $this->_hashService->retrieveConsentHash($query); } } diff --git a/library/EngineBlock/Corto/Model/Consent/Factory.php b/library/EngineBlock/Corto/Model/Consent/Factory.php index d046b5428b..5efe60ab2c 100644 --- a/library/EngineBlock/Corto/Model/Consent/Factory.php +++ b/library/EngineBlock/Corto/Model/Consent/Factory.php @@ -16,7 +16,7 @@ * limitations under the License. */ -use OpenConext\EngineBlock\Service\Consent\ConsentHashService; +use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; /** * @todo write a test @@ -27,7 +27,7 @@ class EngineBlock_Corto_Model_Consent_Factory private $_filterCommandFactory; /** - * @var ConsentHashService + * @var ConsentHashServiceInterface */ private $_hashService; @@ -36,7 +36,7 @@ class EngineBlock_Corto_Model_Consent_Factory */ public function __construct( EngineBlock_Corto_Filter_Command_Factory $filterCommandFactory, - ConsentHashService $hashService + ConsentHashServiceInterface $hashService ) { $this->_filterCommandFactory = $filterCommandFactory; $this->_hashService = $hashService; diff --git a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php index 4dc3381efb..16614facfd 100644 --- a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php @@ -102,8 +102,9 @@ public function serve($serviceName, Request $httpRequest) if (!$consentRepository->explicitConsentWasGivenFor($serviceProvider)) { $consentRepository->giveExplicitConsentFor($destinationMetadata); + } else { + $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT); } - $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT); $response->setConsent(Constants::CONSENT_OBTAINED); $response->setDestination($response->getReturn()); diff --git a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php index 2e8addfa41..66f1e48cc7 100644 --- a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php @@ -149,8 +149,9 @@ public function serve($serviceName, Request $httpRequest) if ($this->isConsentDisabled($spMetadataChain, $identityProvider)) { if (!$consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata)) { $consentRepository->giveImplicitConsentFor($serviceProviderMetadata); + } else { + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT); } - $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT); $response->setConsent(Constants::CONSENT_INAPPLICABLE); $response->setDestination($response->getReturn()); diff --git a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php index 72ceada16a..9c38b588c5 100644 --- a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php +++ b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php @@ -19,6 +19,9 @@ namespace OpenConext\EngineBlock\Authentication\Repository; use OpenConext\EngineBlock\Authentication\Model\Consent; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; interface ConsentRepository @@ -42,18 +45,18 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b /** * Test if the consent row is set with an attribute hash either stable or unstable */ - public function hasConsentHash(array $parameters): ConsentVersion; + public function hasConsentHash(ConsentHashQuery $query): ConsentVersion; /** * By default stores the stable consent hash. The legacy consent hash is left. */ - public function storeConsentHash(array $parameters): bool; + public function storeConsentHash(ConsentStoreParameters $parameters): bool; /** * When a deprecated unstable consent hash is encoutered, we upgrade it to the new format using this * update consent hash method. */ - public function updateConsentHash(array $parameters): bool; + public function updateConsentHash(ConsentUpdateParameters $parameters): bool; public function countTotalConsent($consentUid): int; } diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php new file mode 100644 index 0000000000..2e93727e83 --- /dev/null +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php @@ -0,0 +1,31 @@ +consentVersion === self::UNSTABLE; } + + public function isStable(): bool + { + return $this->consentVersion === self::STABLE; + } } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index cfe25b286e..a261a60b0f 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -19,6 +19,9 @@ namespace OpenConext\EngineBlock\Service\Consent; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\UserLifecycle\Domain\ValueObject\Client\Name; use SAML2\XML\saml\NameID; @@ -49,17 +52,17 @@ public function __construct(ConsentRepository $consentHashRepository) $this->consentRepository = $consentHashRepository; } - public function retrieveConsentHash(array $parameters): ConsentVersion + public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion { - return $this->consentRepository->hasConsentHash($parameters); + return $this->consentRepository->hasConsentHash($query); } - public function storeConsentHash(array $parameters): bool + public function storeConsentHash(ConsentStoreParameters $parameters): bool { return $this->consentRepository->storeConsentHash($parameters); } - public function updateConsentHash(array $parameters): bool + public function updateConsentHash(ConsentUpdateParameters $parameters): bool { return $this->consentRepository->updateConsentHash($parameters); } @@ -138,7 +141,6 @@ private function sortArray(array $sortMe): array $sortFunction($copy); foreach ($copy as $key => $value) { if (is_array($value)) { - $sortFunction($value); $copy[$key] = $this->sortArray($value); } } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php index 99140782d1..66f09d0996 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php @@ -18,6 +18,9 @@ namespace OpenConext\EngineBlock\Service\Consent; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; interface ConsentHashServiceInterface @@ -25,11 +28,11 @@ interface ConsentHashServiceInterface /** * Retrieve the consent hash */ - public function retrieveConsentHash(array $parameters): ConsentVersion; + public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion; - public function storeConsentHash(array $parameters): bool; + public function storeConsentHash(ConsentStoreParameters $parameters): bool; - public function updateConsentHash(array $parameters): bool; + public function updateConsentHash(ConsentUpdateParameters $parameters): bool; public function countTotalConsent($consentUid): int; diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php index 2fce493864..f4b97796f9 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php @@ -83,16 +83,14 @@ public function findAllFor($userId) public function countAllFor($userId) { try { - $consents = $this->consentRepository->findAllFor($userId); + return $this->consentRepository->countTotalConsent($userId); } catch (Exception $e) { throw new RuntimeException( - sprintf('An exception occurred while fetching consents the user has given ("%s")', $e->getMessage()), + sprintf('An exception occurred while counting consents the user has given ("%s")', $e->getMessage()), 0, $e ); } - - return count($consents); } public function deleteOneConsentFor(CollabPersonId $id, string $serviceProviderEntityId): bool diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index 901bc3900d..04304c52e6 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -21,14 +21,16 @@ use DateTime; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; use Doctrine\DBAL\Connection as DbalConnection; +use Doctrine\DBAL\Exception; use Doctrine\Persistence\ManagerRegistry; use OpenConext\EngineBlock\Authentication\Model\Consent; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Exception\RuntimeException; -use PDO; -use PDOException; use Psr\Log\LoggerInterface; use function sha1; @@ -70,6 +72,7 @@ public function findAllFor($userId) , consent_date , consent_type , attribute + , attribute_stable FROM consent WHERE @@ -81,7 +84,7 @@ public function findAllFor($userId) try { $statement = $this->connection->executeQuery($sql, ['hashed_user_id' => sha1($userId)]); $rows = $statement->fetchAllAssociative(); - } catch (\Doctrine\DBAL\Exception $exception) { + } catch (Exception $exception) { throw new RuntimeException('Could not fetch user consents from the database', 0, $exception); } @@ -92,7 +95,7 @@ function (array $row) use ($userId) { $row['service_id'], new DateTime($row['consent_date']), new ConsentType($row['consent_type']), - $row['attribute'] + $row['attribute_stable'] ?? $row['attribute'] ); }, $rows @@ -111,7 +114,7 @@ public function deleteAllFor($userId) try { $this->connection->executeQuery($sql, ['hashed_user_id' => sha1($userId)]); $this->logger->notice(sprintf('Removed consent for hashed user id %s (%s)', sha1($userId), $userId)); - } catch (\Doctrine\DBAL\Exception $exception) { + } catch (Exception $exception) { throw new RuntimeException( sprintf( 'Could not delete user consents from the database for user %s', @@ -158,7 +161,7 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b ); return $result->rowCount() > 0; - } catch (\Doctrine\DBAL\Exception $exception) { + } catch (Exception $exception) { throw new RuntimeException( sprintf( 'Could not delete user %s consent from the database for a specific SP %s', @@ -174,11 +177,11 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b /** * @throws RuntimeException */ - public function hasConsentHash(array $parameters): ConsentVersion + public function hasConsentHash(ConsentHashQuery $query): ConsentVersion { try { - $query = " SELECT - * + $sql = " SELECT + attribute_stable FROM consent WHERE @@ -193,20 +196,24 @@ public function hasConsentHash(array $parameters): ConsentVersion deleted_at IS NULL "; - $statement = $this->connection->prepare($query); - $statement->execute($parameters); - $rows = $statement->fetchAll(); + $rows = $this->connection->executeQuery($sql, [ + $query->hashedUserId, + $query->serviceId, + $query->attributeHash, + $query->attributeStableHash, + $query->consentType, + ])->fetchAllAssociative(); if (count($rows) < 1) { // No stored consent found return ConsentVersion::notGiven(); } - if ($rows[0]['attribute_stable'] !== '') { + if (!empty($rows[0]['attribute_stable'])) { return ConsentVersion::stable(); } return ConsentVersion::unstable(); - } catch (PDOException $e) { + } catch (Exception $e) { throw new RuntimeException(sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage())); } } @@ -214,19 +221,22 @@ public function hasConsentHash(array $parameters): ConsentVersion /** * @throws RuntimeException */ - public function storeConsentHash(array $parameters): bool + public function storeConsentHash(ConsentStoreParameters $parameters): bool { $query = "INSERT INTO consent (hashed_user_id, service_id, attribute_stable, consent_type, consent_date, deleted_at) VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') ON DUPLICATE KEY UPDATE attribute_stable=VALUES(attribute_stable), consent_type=VALUES(consent_type), consent_date=NOW()"; - $statement = $this->connection->prepare($query); - if (!$statement) { - throw new RuntimeException("Unable to create a prepared statement to insert consent?!"); - } - if (!$statement->execute($parameters)) { + try { + $this->connection->executeStatement($query, [ + $parameters->hashedUserId, + $parameters->serviceId, + $parameters->attributeStableHash, + $parameters->consentType, + ]); + } catch (Exception $e) { throw new RuntimeException( - sprintf('Error storing consent: "%s"', var_export($statement->errorInfo(), true)) + sprintf('Error storing consent: "%s"', $e->getMessage()) ); } @@ -236,7 +246,7 @@ public function storeConsentHash(array $parameters): bool /** * @throws RuntimeException */ - public function updateConsentHash(array $parameters): bool + public function updateConsentHash(ConsentUpdateParameters $parameters): bool { $query = " UPDATE @@ -254,17 +264,33 @@ public function updateConsentHash(array $parameters): bool AND deleted_at IS NULL "; - $statement = $this->connection->prepare($query); - if (!$statement) { - throw new RuntimeException("Unable to create a prepared statement to update consent?!"); - } - if (!$statement->execute($parameters)) { + try { + $affected = $this->connection->executeStatement($query, [ + $parameters->attributeStableHash, + $parameters->attributeHash, + $parameters->hashedUserId, + $parameters->serviceId, + $parameters->consentType, + ]); + } catch (Exception $e) { throw new RuntimeException( - sprintf('Error storing updated consent: "%s"', var_export($statement->errorInfo(), true)) + sprintf('Error storing updated consent: "%s"', $e->getMessage()) ); } + if ($affected === 0) { + $this->logger->warning( + sprintf( + 'Could not upgrade unstable consent hash for user "%s" and service "%s": no matching row found. ' . + 'The user\'s attributes may have changed since consent was given.', + $parameters->hashedUserId, + $parameters->serviceId + ) + ); + return false; + } + return true; } @@ -274,12 +300,14 @@ public function updateConsentHash(array $parameters): bool public function countTotalConsent($consentUid): int { $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ? AND deleted_at IS NULL"; - $parameters = array(sha1($consentUid)); - $statement = $this->connection->prepare($query); - if (!$statement) { - throw new RuntimeException("Unable to create a prepared statement to count consent?!"); + $parameters = [sha1($consentUid)]; + + try { + return (int) $this->connection->executeQuery($query, $parameters)->fetchOne(); + } catch (Exception $e) { + throw new RuntimeException( + sprintf('Error counting consent: "%s"', $e->getMessage()) + ); } - $statement->execute($parameters); - return (int)$statement->fetchColumn(); } } diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index a411cde945..fe7881fc75 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -44,7 +44,7 @@ class EngineBlock_Corto_Model_Consent_Integration_Test extends TestCase */ private $response; - public function setup() + public function setup(): void { $this->response = Mockery::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class); $this->consentRepository = Mockery::mock(ConsentRepository::class); diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index a82a8ff328..7cec978f7a 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -17,6 +17,7 @@ */ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; +use OpenConext\EngineBlock\Authentication\Value\ConsentType; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; @@ -33,6 +34,7 @@ class EngineBlock_Corto_Model_ConsentTest extends TestCase public function setUp(): void { $mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($mockedResponse)->getNameIdValue()->thenReturn('urn:collab:person:example.org:user1'); $this->consentService = Mockery::mock(ConsentHashServiceInterface::class); @@ -61,30 +63,39 @@ public function testConsentDisabledDoesNotWriteToDatabase() { $serviceProvider = new ServiceProvider("service-provider-entity-id"); - $this->consentService->shouldReceive('getUnstableAttributesHash'); - $this->consentService->shouldReceive('getStableAttributesHash'); - $this->consentService->shouldReceive('retrieveStableConsentHash'); - $this->consentService->shouldReceive('retrieveConsentHash'); - $this->consentService->shouldReceive('storeConsentHash'); - $this->consentDisabled->explicitConsentWasGivenFor($serviceProvider); - $this->consentDisabled->implicitConsentWasGivenFor($serviceProvider); - $this->consentDisabled->giveExplicitConsentFor($serviceProvider); - $this->consentDisabled->giveImplicitConsentFor($serviceProvider); + $this->consentService->shouldNotReceive('storeConsentHash'); + $this->consentService->shouldNotReceive('retrieveConsentHash'); + $this->consentService->shouldNotReceive('updateConsentHash'); + + $this->assertTrue($this->consentDisabled->explicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consentDisabled->implicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consentDisabled->giveExplicitConsentFor($serviceProvider)); + $this->assertTrue($this->consentDisabled->giveImplicitConsentFor($serviceProvider)); + } + + public function testUpgradeAttributeHashSkippedWhenConsentDisabled() + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + + $this->consentService->shouldNotReceive('retrieveConsentHash'); + $this->consentService->shouldNotReceive('updateConsentHash'); + + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT); + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_IMPLICIT); } public function testConsentWriteToDatabase() { $serviceProvider = new ServiceProvider("service-provider-entity-id"); - $this->consentService->shouldReceive('getStableAttributesHash'); - $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); - $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(ConsentVersion::stable()); - $this->consent->explicitConsentWasGivenFor($serviceProvider); + $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); $this->consentService->shouldReceive('getStableAttributesHash')->andReturn(sha1('stable')); - $this->consent->implicitConsentWasGivenFor($serviceProvider); - + $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(ConsentVersion::stable()); $this->consentService->shouldReceive('storeConsentHash')->andReturn(true); - $this->consent->giveExplicitConsentFor($serviceProvider); - $this->consent->giveImplicitConsentFor($serviceProvider); + + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); + $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); } } diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 662a1f5503..25e9cc097e 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -48,5 +48,6 @@ + diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index 51464edda0..33f68f779b 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -33,7 +33,7 @@ class ConsentHashServiceTest extends TestCase */ private $chs; - public function setUp() + public function setUp(): void { $mockConsentHashRepository = m::mock(ConsentRepository::class); $this->chs = new ConsentHashService($mockConsentHashRepository); From f3f418583e04d06f6f8220c43d66792c0e12dd80 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Fri, 6 Mar 2026 16:01:57 +0100 Subject: [PATCH 14/15] Fix database schema and migrations for consent hash - Move Version20220503092351 to migrations/ so Doctrine picks it up; add preUp skip guard to avoid duplicate column on existing installs; remove deprecated abortIf() calls - Make 'attribute' column nullable in baseline migration and Doctrine entity (storeConsentHash sets attribute_stable only, not attribute) - Add 'attribute_stable' column to baseline CREATE TABLE statement - Fix integration test class name to match filename (PHPUnit warning) - Make consentTypeProvider() static (PHPUnit deprecation) --- .../Version20220503092351.php | 30 ----------- .../Version20220503092351.php | 53 +++++++++++++++++++ .../Version20260210000000.php | 3 +- .../Authentication/Entity/Consent.php | 2 +- .../Corto/Model/ConsentIntegrationTest.php | 4 +- 5 files changed, 58 insertions(+), 34 deletions(-) delete mode 100644 database/DoctrineMigrations/Version20220503092351.php create mode 100644 migrations/DoctrineMigrations/Version20220503092351.php diff --git a/database/DoctrineMigrations/Version20220503092351.php b/database/DoctrineMigrations/Version20220503092351.php deleted file mode 100644 index fbf96c95d1..0000000000 --- a/database/DoctrineMigrations/Version20220503092351.php +++ /dev/null @@ -1,30 +0,0 @@ -abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.'); - - $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) DEFAULT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL'); - } - - public function down(Schema $schema) : void - { - // this down() migration is auto-generated, please modify it to your needs - $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.'); - - $this->addSql('ALTER TABLE consent DROP attribute_stable, CHANGE attribute attribute VARCHAR(80) CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`'); - } -} diff --git a/migrations/DoctrineMigrations/Version20220503092351.php b/migrations/DoctrineMigrations/Version20220503092351.php new file mode 100644 index 0000000000..557685d1e4 --- /dev/null +++ b/migrations/DoctrineMigrations/Version20220503092351.php @@ -0,0 +1,53 @@ +connection->createSchemaManager()->listTableNames(); + $tableExists = in_array('consent', $tables, true); + + if (!$tableExists) { + $this->skipIf(true, 'Table consent does not exist yet (fresh install, baseline will create it). Skipping.'); + return; + } + + $columnExists = (bool) $this->connection->fetchOne( + "SELECT COUNT(*) FROM information_schema.COLUMNS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = 'consent' + AND COLUMN_NAME = 'attribute_stable'" + ); + $this->skipIf( + $columnExists, + 'Column attribute_stable already exists (fresh install via baseline). Skipping.' + ); + } + + public function up(Schema $schema): void + { + $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) DEFAULT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL'); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE consent DROP attribute_stable, CHANGE attribute attribute VARCHAR(80) CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`'); + } +} diff --git a/migrations/DoctrineMigrations/Version20260210000000.php b/migrations/DoctrineMigrations/Version20260210000000.php index b9655fb790..94c5f3e173 100644 --- a/migrations/DoctrineMigrations/Version20260210000000.php +++ b/migrations/DoctrineMigrations/Version20260210000000.php @@ -36,7 +36,8 @@ public function up(Schema $schema): void `consent_date` datetime NOT NULL, `hashed_user_id` varchar(80) NOT NULL, `service_id` varchar(255) NOT NULL, - `attribute` varchar(80) NOT NULL, + `attribute` varchar(80) DEFAULT NULL, + `attribute_stable` varchar(80) DEFAULT NULL, `consent_type` varchar(20) DEFAULT \'explicit\', `deleted_at` datetime NOT NULL DEFAULT \'0000-00-00 00:00:00\', PRIMARY KEY (`hashed_user_id`,`service_id`,`deleted_at`), diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php index b8d79912e8..67f28df632 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php @@ -55,7 +55,7 @@ class Consent /** * @var string */ - #[ORM\Column(type: \Doctrine\DBAL\Types\Types::STRING, length: 80)] + #[ORM\Column(type: \Doctrine\DBAL\Types\Types::STRING, length: 80, nullable: true)] public ?string $attribute = null; /** diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index fe7881fc75..5b39454fbb 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -26,7 +26,7 @@ use OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository; use PHPUnit\Framework\TestCase; -class EngineBlock_Corto_Model_Consent_Integration_Test extends TestCase +class ConsentIntegrationTest extends TestCase { use MockeryPHPUnitIntegration; @@ -235,7 +235,7 @@ public function test_upgrade_to_stable_consent_not_applied_when_stable($consentT $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); } - public function consentTypeProvider() + public static function consentTypeProvider(): iterable { yield [ConsentType::TYPE_IMPLICIT]; yield [ConsentType::TYPE_EXPLICIT]; From c3faaec8f5e68c738a6f68aea99e66da00d804a9 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Fri, 6 Mar 2026 18:56:52 +0100 Subject: [PATCH 15/15] Add missing edge-case tests for consent hash implementation Based on requirements from issue #1721, PR #1137 review threads, and newly added code paths: ConsentIntegrationTest: - test_upgrade_not_applied_when_no_consent_given: upgradeAttributeHashFor does nothing when hasConsentHash returns notGiven() - test_upgrade_continues_gracefully_when_attributes_changed: when updateConsentHash returns false (0 rows, attributes changed since consent was given), upgradeAttributeHashFor must not throw ConsentTest (eb4): - testNullNameIdReturnsNoConsentWithoutCallingRepository: when getNameIdValue() returns null, all three private guards (_storeConsent, _hasStoredConsent, _updateConsent) must return early without touching the repository ConsentVersionTest (new): - isStable/isUnstable/given behaviour for all three states - invalid value throws InvalidArgumentException ConsentHashServiceTest: - test_stable_attribute_hash_attribute_name_casing_normalized: issue #1721 explicitly requires 'Case normalize all attribute names'; verifies keys like 'displayName' and 'DISPLAYNAME' produce the same hash - test_stable_attribute_hash_attribute_name_ordering_normalized: issue #1721 requires 'Sort all attribute names'; verifies key order is stable --- .../Repository/DbalConsentRepository.php | 3 +- .../Corto/Model/ConsentIntegrationTest.php | 122 +++++++++++---- .../Test/Corto/Model/ConsentTest.php | 79 ++++++++++ .../Value/ConsentVersionTest.php | 62 ++++++++ .../Consent/ConsentHashServiceTest.php | 140 +++++++++++++++++- 5 files changed, 373 insertions(+), 33 deletions(-) create mode 100644 tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index 04304c52e6..0b5e0d640c 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -252,7 +252,8 @@ public function updateConsentHash(ConsentUpdateParameters $parameters): bool UPDATE consent SET - attribute_stable = ? + attribute_stable = ?, + attribute = NULL WHERE attribute = ? AND diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index 5b39454fbb..5739f8fca9 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -19,7 +19,10 @@ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use Mockery\MockInterface; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Consent\ConsentHashService; @@ -46,6 +49,10 @@ class ConsentIntegrationTest extends TestCase public function setup(): void { + Mockery::getConfiguration()->setDefaultMatcher(ConsentHashQuery::class, \Mockery\Matcher\IsEqual::class); + Mockery::getConfiguration()->setDefaultMatcher(ConsentStoreParameters::class, \Mockery\Matcher\IsEqual::class); + Mockery::getConfiguration()->setDefaultMatcher(ConsentUpdateParameters::class, \Mockery\Matcher\IsEqual::class); + $this->response = Mockery::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class); $this->consentRepository = Mockery::mock(ConsentRepository::class); $this->consentService = new ConsentHashService($this->consentRepository); @@ -97,7 +104,13 @@ public function test_unstable_previous_consent_given($consentType) // Stable consent is not yet stored $this->consentRepository ->shouldReceive('hasConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->with(new ConsentHashQuery( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType, + )) ->once() ->andReturn(ConsentVersion::unstable()); @@ -123,7 +136,13 @@ public function test_stable_consent_given($consentType) // Stable consent is not yet stored $this->consentRepository ->shouldReceive('hasConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->with(new ConsentHashQuery( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType, + )) ->once() ->andReturn(ConsentVersion::stable()); @@ -150,7 +169,12 @@ public function test_give_consent_no_unstable_consent_given($consentType) $this->consentRepository ->shouldReceive('storeConsentHash') ->once() - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->with(new ConsentStoreParameters( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType, + )) ->andReturn(true); switch ($consentType) { @@ -166,50 +190,83 @@ public function test_give_consent_no_unstable_consent_given($consentType) /** * @dataProvider consentTypeProvider */ - public function test_give_consent_unstable_consent_given($consentType) + public function test_upgrade_to_stable_consent($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); $this->response->shouldReceive('getNameIdValue') - ->once() + ->twice() ->andReturn('collab:person:id:org-a:joe-a'); + // Old-style (unstable) consent was given previously + $this->consentRepository + ->shouldReceive('hasConsentHash') + ->with(new ConsentHashQuery( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType, + )) + ->once() + ->andReturn(ConsentVersion::unstable()); // Now assert that the new stable consent hash is going to be set $this->consentRepository - ->shouldReceive('storeConsentHash') + ->shouldReceive('updateConsentHash') ->once() - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->with(new ConsentUpdateParameters( + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + consentType: $consentType, + )) ->andReturn(true); - switch ($consentType) { - case ConsentType::TYPE_EXPLICIT: - $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); - break; - case ConsentType::TYPE_IMPLICIT: - $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); - break; - } + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); } /** * @dataProvider consentTypeProvider */ - public function test_upgrade_to_stable_consent($consentType) + public function test_upgrade_to_stable_consent_not_applied_when_stable($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); $this->response->shouldReceive('getNameIdValue') - ->twice() + ->once() ->andReturn('collab:person:id:org-a:joe-a'); - // Old-style (unstable) consent was given previously + // Stable consent is stored $this->consentRepository ->shouldReceive('hasConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) + ->with(new ConsentHashQuery( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType, + )) ->once() - ->andReturn(ConsentVersion::unstable()); - // Now assert that the new stable consent hash is going to be set + ->andReturn(ConsentVersion::stable()); + // Now assert that the new stable consent hash is NOT going to be set $this->consentRepository - ->shouldReceive('updateConsentHash') + ->shouldNotReceive('storeConsentHash'); + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); + } + + /** + * @dataProvider consentTypeProvider + */ + public function test_upgrade_not_applied_when_no_consent_given($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') ->once() - ->with(['8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', '0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', $consentType]) - ->andReturn(true); + ->andReturn('collab:person:id:org-a:joe-a'); + // No consent has been given at all + $this->consentRepository + ->shouldReceive('hasConsentHash') + ->once() + ->andReturn(ConsentVersion::notGiven()); + // No update should be triggered + $this->consentRepository->shouldNotReceive('updateConsentHash'); $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); } @@ -217,21 +274,24 @@ public function test_upgrade_to_stable_consent($consentType) /** * @dataProvider consentTypeProvider */ - public function test_upgrade_to_stable_consent_not_applied_when_stable($consentType) + public function test_upgrade_continues_gracefully_when_attributes_changed($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); $this->response->shouldReceive('getNameIdValue') - ->once() + ->twice() ->andReturn('collab:person:id:org-a:joe-a'); - // Stable consent is stored + // Old-style (unstable) consent is found $this->consentRepository ->shouldReceive('hasConsentHash') - ->with(['0e54805079c56c2b1c1197a760af86ac337b7bac', 'service-provider-entity-id', '8739602554c7f3241958e3cc9b57fdecb474d508', '8739602554c7f3241958e3cc9b57fdecb474d508', $consentType]) ->once() - ->andReturn(ConsentVersion::stable()); - // Now assert that the new stable consent hash is NOT going to be set + ->andReturn(ConsentVersion::unstable()); + // But the UPDATE matches 0 rows (attributes changed since consent was given) $this->consentRepository - ->shouldNotReceive('storeConsentHash'); + ->shouldReceive('updateConsentHash') + ->once() + ->andReturn(false); + + // Must not throw; the warning is logged inside the repository $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); } diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index 7cec978f7a..c0b073c343 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -98,4 +98,83 @@ public function testConsentWriteToDatabase() $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); } + + public function testCountTotalConsent() + { + // Arrange + $this->consentService->shouldReceive('countTotalConsent') + ->with('urn:collab:person:example.org:user1') + ->once() + ->andReturn(5); + + // Act + Assert + $this->assertEquals(5, $this->consent->countTotalConsent()); + } + + public function testConsentUidFromAmPriorToConsentEnabled() + { + // When amPriorToConsentEnabled is true the consent UID must come from + // getOriginalResponse()->getCollabPersonId(), NOT from getNameIdValue(). + $originalResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($originalResponse)->getCollabPersonId()->thenReturn('urn:collab:person:example.org:user-am'); + + $mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($mockedResponse)->getOriginalResponse()->thenReturn($originalResponse); + + $consentWithAmPrior = new EngineBlock_Corto_Model_Consent( + 'consent', + true, + $mockedResponse, + [], + true, // amPriorToConsentEnabled = true + true, + $this->consentService + ); + + $serviceProvider = new ServiceProvider('service-provider-entity-id'); + + $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); + $this->consentService->shouldReceive('getStableAttributesHash')->andReturn(sha1('stable')); + $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(ConsentVersion::stable()); + + // Act: trigger a code path that calls _getConsentUid() + $result = $consentWithAmPrior->explicitConsentWasGivenFor($serviceProvider); + + // Assert: consent check succeeded and the AM-prior uid path was used + $this->assertTrue($result); + Phake::verify($originalResponse)->getCollabPersonId(); + Phake::verify($mockedResponse, Phake::never())->getNameIdValue(); + } + + public function testNullNameIdReturnsNoConsentWithoutCallingRepository() + { + $mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($mockedResponse)->getNameIdValue()->thenReturn(null); + + $consentWithNullUid = new EngineBlock_Corto_Model_Consent( + "consent", + true, + $mockedResponse, + [], + false, + true, + $this->consentService + ); + + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + + // No DB calls should occur when the NameID is null + $this->consentService->shouldNotReceive('retrieveConsentHash'); + $this->consentService->shouldNotReceive('storeConsentHash'); + $this->consentService->shouldNotReceive('updateConsentHash'); + + // _hasStoredConsent returns notGiven() when UID is null -> consent methods return false + $this->assertFalse($consentWithNullUid->explicitConsentWasGivenFor($serviceProvider)); + $this->assertFalse($consentWithNullUid->implicitConsentWasGivenFor($serviceProvider)); + // giveConsent returns false when UID is null (_storeConsent returns early) + $this->assertFalse($consentWithNullUid->giveExplicitConsentFor($serviceProvider)); + $this->assertFalse($consentWithNullUid->giveImplicitConsentFor($serviceProvider)); + // upgradeAttributeHashFor should not throw when UID is null + $consentWithNullUid->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT); + } } diff --git a/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php b/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php new file mode 100644 index 0000000000..7c6f26e1c1 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php @@ -0,0 +1,62 @@ +assertTrue($version->given()); + $this->assertTrue($version->isStable()); + $this->assertFalse($version->isUnstable()); + $this->assertSame('stable', (string) $version); + } + + public function testUnstableIsGiven(): void + { + $version = ConsentVersion::unstable(); + + $this->assertTrue($version->given()); + $this->assertFalse($version->isStable()); + $this->assertTrue($version->isUnstable()); + $this->assertSame('unstable', (string) $version); + } + + public function testNotGivenIsNotGiven(): void + { + $version = ConsentVersion::notGiven(); + + $this->assertFalse($version->given()); + $this->assertFalse($version->isStable()); + $this->assertFalse($version->isUnstable()); + $this->assertSame('not-given', (string) $version); + } + + public function testInvalidVersionThrows(): void + { + $this->expectException(InvalidArgumentException::class); + new ConsentVersion('invalid'); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index 33f68f779b..43579c62a6 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -381,7 +381,12 @@ public function test_stable_attribute_hash_two_different_arrays_yield_different_ 'urn:collab:org:vm.openconext.org', 'urn:collab:org:vm.openconext.org', ]; - // two sequential arrays with the same amount of attributes will yield the exact same hash if no values must be stored. todo: check if we want this? + // Known limitation: when mustStoreValues=false the hash is built from attribute *names* only. + // For sequential (numerically-indexed) arrays the "names" are just integer indices [0, 1, 2, …], + // so two sequential arrays with the same count but completely different values produce the same hash. + // This is accepted because in practice all SAML attributes are keyed by URN strings + // (e.g. 'urn:mace:dir:attribute-def:displayName'), not by integer indices, making this + // collision path unreachable in production. $this->assertEquals($this->chs->getStableAttributesHash($attributes, false), $this->chs->getStableAttributesHash($differentAttributes, false)); $this->assertNotEquals($this->chs->getStableAttributesHash($attributes, true), $this->chs->getStableAttributesHash($differentAttributes, true)); } @@ -474,4 +479,137 @@ public function test_stable_attribute_hash_can_handle_nameid_objects() $hash = $this->chs->getStableAttributesHash($attributes, false); $this->assertTrue(is_string($hash)); } + + public function test_stable_attribute_hash_attribute_name_casing_normalized() + { + // Issue requirement: "Case normalize all attribute names" + // Attribute names (keys) differing only in casing must yield the same hash + $lowercase = [ + 'urn:mace:dir:attribute-def:displayname' => ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + ]; + $mixed = [ + 'urn:mace:dir:attribute-def:displayName' => ['John Doe'], + 'URN:MACE:DIR:ATTRIBUTE-DEF:UID' => ['joe-f12'], + ]; + + $this->assertEquals( + $this->chs->getStableAttributesHash($lowercase, true), + $this->chs->getStableAttributesHash($mixed, true) + ); + $this->assertEquals( + $this->chs->getStableAttributesHash($lowercase, false), + $this->chs->getStableAttributesHash($mixed, false) + ); + } + + public function test_stable_attribute_hash_attribute_name_ordering_normalized() + { + // Issue requirement: "Sort all attribute names" + $alphabetical = [ + 'urn:attribute:a' => ['value1'], + 'urn:attribute:b' => ['value2'], + 'urn:attribute:c' => ['value3'], + ]; + $reversed = [ + 'urn:attribute:c' => ['value3'], + 'urn:attribute:b' => ['value2'], + 'urn:attribute:a' => ['value1'], + ]; + + $this->assertEquals( + $this->chs->getStableAttributesHash($alphabetical, true), + $this->chs->getStableAttributesHash($reversed, true) + ); + $this->assertEquals( + $this->chs->getStableAttributesHash($alphabetical, false), + $this->chs->getStableAttributesHash($reversed, false) + ); + } + + // ------------------------------------------------------------------------- + // Unstable hash algorithm — getUnstableAttributesHash + // ------------------------------------------------------------------------- + + public function test_unstable_attribute_hash_mustStoreValues_false_uses_keys_only() + { + // When mustStoreValues=false the hash is based on attribute names only. + // Two arrays with the same keys but different values must yield the same hash. + $attributes = ['urn:attr:a' => ['Alice'], 'urn:attr:b' => ['Bob']]; + $differentValues = ['urn:attr:a' => ['Charlie'], 'urn:attr:b' => ['Dave']]; + + $this->assertEquals( + $this->chs->getUnstableAttributesHash($attributes, false), + $this->chs->getUnstableAttributesHash($differentValues, false) + ); + } + + public function test_unstable_attribute_hash_mustStoreValues_true_includes_values() + { + // When mustStoreValues=true, attribute values are part of the hash. + // Two arrays with the same keys but different values must yield a different hash. + $attributes = ['urn:attr:a' => ['Alice'], 'urn:attr:b' => ['Bob']]; + $differentValues = ['urn:attr:a' => ['Charlie'], 'urn:attr:b' => ['Dave']]; + + $this->assertNotEquals( + $this->chs->getUnstableAttributesHash($attributes, true), + $this->chs->getUnstableAttributesHash($differentValues, true) + ); + } + + public function test_unstable_attribute_hash_key_order_normalized_in_names_only_mode() + { + // When mustStoreValues=false the implementation sorts attribute names, + // so reversed key order must produce the same hash. + $attributes = ['urn:attr:a' => ['Alice'], 'urn:attr:b' => ['Bob']]; + $reversed = ['urn:attr:b' => ['Bob'], 'urn:attr:a' => ['Alice']]; + + $this->assertEquals( + $this->chs->getUnstableAttributesHash($attributes, false), + $this->chs->getUnstableAttributesHash($reversed, false) + ); + } + + // ------------------------------------------------------------------------- + // isBlank / removeEmptyAttributes edge cases + // ------------------------------------------------------------------------- + + public function test_stable_attribute_hash_empty_array_produces_consistent_hash() + { + // An empty attribute array must not throw and must be idempotent. + $hashWithValues = $this->chs->getStableAttributesHash([], true); + $hashWithoutValues = $this->chs->getStableAttributesHash([], false); + + $this->assertIsString($hashWithValues); + $this->assertSame($hashWithValues, $this->chs->getStableAttributesHash([], true)); + $this->assertIsString($hashWithoutValues); + $this->assertSame($hashWithoutValues, $this->chs->getStableAttributesHash([], false)); + } + + public function test_stable_attribute_hash_zero_string_not_removed_as_empty() + { + // "0" is truthy via is_numeric(), so it must NOT be removed by removeEmptyAttributes. + // An attribute with value "0" must produce a different hash than an empty attribute set. + $withZeroString = ['urn:attr:count' => '0']; + $withoutAttr = []; + + $this->assertNotEquals( + $this->chs->getStableAttributesHash($withZeroString, true), + $this->chs->getStableAttributesHash($withoutAttr, true) + ); + } + + public function test_stable_attribute_hash_zero_float_not_removed_as_empty() + { + // 0.0 is numeric, so it must NOT be removed by removeEmptyAttributes. + // An attribute with value 0.0 must produce a stable, non-empty hash. + $withZeroFloat = ['urn:attr:count' => 0.0]; + + $hash = $this->chs->getStableAttributesHash($withZeroFloat, true); + + $this->assertIsString($hash); + $this->assertNotEmpty($hash); + // Must be idempotent + $this->assertSame($hash, $this->chs->getStableAttributesHash($withZeroFloat, true)); + } }