From d6390b7766d2678cec188bffb2f93c2c21142da8 Mon Sep 17 00:00:00 2001 From: Koen Cornelis Date: Tue, 11 May 2021 22:15:54 +0200 Subject: [PATCH 01/16] 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 2f18a55d9e4c4de162a9441293e7310857e68ec9 Mon Sep 17 00:00:00 2001 From: Koen Cornelis Date: Thu, 20 May 2021 11:31:33 +0200 Subject: [PATCH 02/16] 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 9ec21765a790ba125bf8070bd10ea037cc2a7a46 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 09:10:01 +0200 Subject: [PATCH 03/16] 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 2d422981f4..cffc53426c 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -27,6 +27,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; @@ -64,7 +66,7 @@ public function findAllFor($userId) { // deleted_at IS NULL matches active records whose deleted_at is '0000-00-00 00:00:00'. // See Consent::$deletedAt for full context. - $sql = ' + $sql = ' SELECT service_id , consent_date @@ -138,7 +140,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( @@ -170,4 +173,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 39c0310343839ed116f5c7bdc0be588f9a0d8265 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 09:49:02 +0200 Subject: [PATCH 04/16] 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 b07fc4cfe264330b9fa737685bbb988e2fca63b9 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 15:53:36 +0200 Subject: [PATCH 05/16] 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 | 2 +- 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(+), 13 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 8a0817011c..c5235d93c2 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 cffc53426c..a5c07e48ef 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -180,11 +180,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 4ef5e0d0a1..fa38378ea7 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php @@ -19,7 +19,7 @@ namespace OpenConext\EngineBlockBundle\Controller\Api; use OpenConext\EngineBlock\Exception\RuntimeException; -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 b68d0beb77..5318eccfc0 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\Attributes\Group; use PHPUnit\Framework\Attributes\Test; From 7a893f34c9e90a71b524cdb39e744d4de69ee321 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 08:41:04 +0200 Subject: [PATCH 06/16] 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 7a1f8f3e6786a174075a0372a889bb5d4c528a48 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:26:01 +0200 Subject: [PATCH 07/16] 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 4398e85f97..a6559029e7 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php @@ -59,6 +59,12 @@ class Consent #[ORM\Column(type: Types::STRING, length: 80)] public ?string $attribute = null; + /** + * @var string + */ + #[ORM\Column(name: 'attribute_stable', type: Types::STRING, length: 80, nullable: true)] + public ?string $attributeStable = null; + /** * @var string */ From f171be3daaa9e73dd20252530e351a453d6d6856 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:26:52 +0200 Subject: [PATCH 08/16] 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 09dffbd248f5c3706e1c6186d45349bc81938f6a Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:28:08 +0200 Subject: [PATCH 09/16] 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 a5c07e48ef..9b40eeda74 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -210,15 +210,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?!"); @@ -233,6 +264,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 399632ea5c..525d4d5b86 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php @@ -527,6 +527,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', @@ -534,7 +535,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 f2deb08f00..d9a046755d 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 cc703501d40fd319b5766b841e85b7043a457e0e Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 4 May 2022 11:58:58 +0200 Subject: [PATCH 10/16] 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 37805e83c0c471f88a55d0dc1463fc0ada0f2e0d Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Thu, 5 May 2022 10:55:14 +0200 Subject: [PATCH 11/16] 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 f998ae5f46d5dc055a8e979177f429ceba88e498 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Thu, 5 May 2022 11:40:36 +0200 Subject: [PATCH 12/16] 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 9b40eeda74..a1b1ee1a42 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -26,6 +26,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; @@ -177,7 +178,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 @@ -189,7 +190,7 @@ public function hasConsentHash(array $parameters): bool AND service_id = ? AND - attribute = ? + (attribute = ? OR attribute_stable = ?) AND consent_type = ? AND @@ -202,45 +203,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 ff2af3f21c59542d5b6574df1547bbb1f2691106 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Fri, 6 Mar 2026 13:40:39 +0100 Subject: [PATCH 13/16] feat(consent): implement consent hash versioning --- config/services/controllers/api.yml | 2 +- config/services/services.yml | 4 +- .../Version20220503092351.php | 30 ------- .../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 +- .../Version20220503092351.php | 53 +++++++++++ .../Version20260210000000.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 +- .../Authentication/Entity/Consent.php | 2 +- .../Repository/DbalConsentRepository.php | 89 ++++++++++++------- .../Corto/Model/ConsentIntegrationTest.php | 6 +- .../Test/Corto/Model/ConsentTest.php | 45 ++++++---- tests/phpunit.xml | 64 ++++++------- .../Consent/ConsentHashServiceTest.php | 2 +- 24 files changed, 344 insertions(+), 158 deletions(-) delete mode 100644 database/DoctrineMigrations/Version20220503092351.php create mode 100644 migrations/DoctrineMigrations/Version20220503092351.php 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 deleted file mode 100644 index 6218a8a936..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) 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/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/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 3277a46df5..adc81a0044 100644 --- a/migrations/DoctrineMigrations/Version20260210000000.php +++ b/migrations/DoctrineMigrations/Version20260210000000.php @@ -53,7 +53,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/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 c5235d93c2..9af6593d3c 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/Entity/Consent.php b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php index a6559029e7..d6f43378bc 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php @@ -56,7 +56,7 @@ class Consent /** * @var string */ - #[ORM\Column(type: Types::STRING, length: 80)] + #[ORM\Column(type: Types::STRING, length: 80, nullable: true)] public ?string $attribute = null; /** diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index a1b1ee1a42..33bb34e04e 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -25,11 +25,12 @@ 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; @@ -73,6 +74,7 @@ public function findAllFor($userId) , consent_date , consent_type , attribute + , attribute_stable FROM consent WHERE @@ -95,7 +97,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 @@ -178,11 +180,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 @@ -197,20 +199,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())); } } @@ -218,19 +224,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()) ); } @@ -240,7 +249,7 @@ public function storeConsentHash(array $parameters): bool /** * @throws RuntimeException */ - public function updateConsentHash(array $parameters): bool + public function updateConsentHash(ConsentUpdateParameters $parameters): bool { $query = " UPDATE @@ -258,17 +267,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; } @@ -278,12 +303,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..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; @@ -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); @@ -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]; 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 cca070da44..25e9cc097e 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -17,35 +17,37 @@ failOnRisky="true" > - - unit - - - integration - - - library - - - functional - - - - - - - - - - - src - library - - - - - - - - + + unit + + + integration + + + library + + + functional + + + + + + + + + + + + src + library + + + + + + + + + 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 069c5dbb8eecdd25bd74ca78be7e507b4a81613e Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Fri, 6 Mar 2026 18:56:52 +0100 Subject: [PATCH 14/16] test(consent): add edge-case coverage --- .../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 33bb34e04e..0e664fa039 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -255,7 +255,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)); + } } From f05b277e4f7061dfd8dc8c14f9a399a4b23425fd Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Mon, 16 Mar 2026 15:27:51 +0100 Subject: [PATCH 15/16] fix(consent): polish follow-up fixes and test configuration --- library/EngineBlock/Corto/Model/Consent.php | 21 +-- .../Corto/Module/Service/ProcessConsent.php | 5 +- .../Corto/Module/Service/ProvideConsent.php | 9 +- ...03092351.php => Version20260315000001.php} | 2 +- .../Repository/ConsentRepository.php | 2 +- .../Service/Consent/ConsentHashService.php | 29 ++-- .../Consent/ConsentHashServiceInterface.php | 2 +- .../Repository/DbalConsentRepository.php | 5 +- .../Corto/Model/ConsentIntegrationTest.php | 131 +++++++----------- .../Test/Corto/Model/ConsentTest.php | 20 +-- .../Module/Service/ProcessConsentTest.php | 4 +- .../Module/Service/ProvideConsentTest.php | 8 +- .../Test/Saml2/NameIdResolverMock.php | 12 +- .../Test/Saml2/NameIdResolverTest.php | 2 - tests/phpunit.xml | 2 +- .../Consent/ConsentHashServiceTest.php | 24 ++++ 16 files changed, 142 insertions(+), 136 deletions(-) rename migrations/DoctrineMigrations/{Version20220503092351.php => Version20260315000001.php} (96%) diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 62f6ed488e..da31206e40 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -85,38 +85,39 @@ public function __construct( $this->_consentEnabled = $consentEnabled; } - public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool + public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): ConsentVersion { if (!$this->_consentEnabled) { - return true; + // Consent disabled: treat as already given (stable — no upgrade needed) + return ConsentVersion::stable(); } - $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); - return $consent->given(); + return $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); } /** * 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 + * + * The caller must pass the ConsentVersion already retrieved by explicitConsentWasGivenFor or + * implicitConsentWasGivenFor to avoid a second identical DB query. */ - public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string $consentType): void + public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string $consentType, ConsentVersion $consentVersion): void { if (!$this->_consentEnabled) { return; } - $consentVersion = $this->_hasStoredConsent($serviceProvider, $consentType); if ($consentVersion->isUnstable()) { $this->_updateConsent($serviceProvider, $consentType); } } - public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool + public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): ConsentVersion { if (!$this->_consentEnabled) { - return true; + return ConsentVersion::stable(); } - $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); - return $consent->given(); + return $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); } public function giveExplicitConsentFor(ServiceProvider $serviceProvider): bool diff --git a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php index 16614facfd..69fdf3eda8 100644 --- a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php @@ -100,10 +100,11 @@ public function serve($serviceName, Request $httpRequest) $attributes = $response->getAssertion()->getAttributes(); $consentRepository = $this->_consentFactory->create($this->_server, $response, $attributes); - if (!$consentRepository->explicitConsentWasGivenFor($serviceProvider)) { + $explicitConsent = $consentRepository->explicitConsentWasGivenFor($serviceProvider); + if (!$explicitConsent->given()) { $consentRepository->giveExplicitConsentFor($destinationMetadata); } else { - $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT); + $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT, $explicitConsent); } $response->setConsent(Constants::CONSENT_OBTAINED); diff --git a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php index 66f1e48cc7..f14668c5c8 100644 --- a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php @@ -147,10 +147,11 @@ public function serve($serviceName, Request $httpRequest) if ($this->isConsentDisabled($spMetadataChain, $identityProvider)) { - if (!$consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata)) { + $implicitConsent = $consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata); + if (!$implicitConsent->given()) { $consentRepository->giveImplicitConsentFor($serviceProviderMetadata); } else { - $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT); + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT, $implicitConsent); } $response->setConsent(Constants::CONSENT_INAPPLICABLE); @@ -168,8 +169,8 @@ public function serve($serviceName, Request $httpRequest) } $priorConsent = $consentRepository->explicitConsentWasGivenFor($serviceProviderMetadata); - if ($priorConsent) { - $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_EXPLICIT); + if ($priorConsent->given()) { + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_EXPLICIT, $priorConsent); $response->setConsent(Constants::CONSENT_PRIOR); diff --git a/migrations/DoctrineMigrations/Version20220503092351.php b/migrations/DoctrineMigrations/Version20260315000001.php similarity index 96% rename from migrations/DoctrineMigrations/Version20220503092351.php rename to migrations/DoctrineMigrations/Version20260315000001.php index 557685d1e4..afccb8e634 100644 --- a/migrations/DoctrineMigrations/Version20220503092351.php +++ b/migrations/DoctrineMigrations/Version20260315000001.php @@ -12,7 +12,7 @@ * 1. Added the `attribute_stable` column, string(80), nullable * 2. Changed the `attribute` column, has been made nullable */ -final class Version20220503092351 extends AbstractMigration +final class Version20260315000001 extends AbstractMigration { public function getDescription(): string { diff --git a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php index 9c38b588c5..38b4af94de 100644 --- a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php +++ b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php @@ -58,5 +58,5 @@ public function storeConsentHash(ConsentStoreParameters $parameters): bool; */ public function updateConsentHash(ConsentUpdateParameters $parameters): bool; - public function countTotalConsent($consentUid): int; + public function countTotalConsent(string $consentUid): int; } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index a261a60b0f..e37bb342e8 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -23,7 +23,6 @@ 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; use function array_filter; use function array_keys; @@ -33,11 +32,10 @@ use function is_array; use function is_numeric; use function ksort; +use function mb_strtolower; use function serialize; use function sha1; use function sort; -use function str_replace; -use function strtolower; use function unserialize; final class ConsentHashService implements ConsentHashServiceInterface @@ -67,7 +65,7 @@ public function updateConsentHash(ConsentUpdateParameters $parameters): bool return $this->consentRepository->updateConsentHash($parameters); } - public function countTotalConsent($consentUid): int + public function countTotalConsent(string $consentUid): int { return $this->consentRepository->countTotalConsent($consentUid); } @@ -114,14 +112,27 @@ private function createHashBaseWithoutValues(array $lowerCasedAttributes): strin } /** - * Lowercases all array keys and values. + * Lowercases all array keys and string values recursively using mb_strtolower + * to handle multi-byte UTF-8 characters (e.g. Ü→ü, Arabic, Chinese — common in SAML). + * + * The previous implementation used serialize/strtolower/unserialize which corrupted + * PHP's s:N: byte-length markers for multi-byte values, causing unserialize() to silently + * return false and producing wrong hashes for any user with a non-ASCII attribute value. */ private function caseNormalizeStringArray(array $original): array { - $serialized = serialize($original); - $lowerCased = strtolower($serialized); - $unserialized = unserialize($lowerCased); - return $unserialized; + $result = []; + foreach ($original as $key => $value) { + $normalizedKey = is_string($key) ? mb_strtolower($key) : $key; + if (is_array($value)) { + $result[$normalizedKey] = $this->caseNormalizeStringArray($value); + } elseif (is_string($value)) { + $result[$normalizedKey] = mb_strtolower($value); + } else { + $result[$normalizedKey] = $value; + } + } + return $result; } /** diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php index 66f09d0996..520eb5229e 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php @@ -34,7 +34,7 @@ public function storeConsentHash(ConsentStoreParameters $parameters): bool; public function updateConsentHash(ConsentUpdateParameters $parameters): bool; - public function countTotalConsent($consentUid): int; + public function countTotalConsent(string $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 0e664fa039..e56872f643 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -228,7 +228,8 @@ 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()"; + ON DUPLICATE KEY UPDATE attribute_stable=VALUES(attribute_stable), + consent_type=VALUES(consent_type), consent_date=NOW(), deleted_at='0000-00-00 00:00:00'"; try { $this->connection->executeStatement($query, [ @@ -301,7 +302,7 @@ public function updateConsentHash(ConsentUpdateParameters $parameters): bool /** * @throws RuntimeException */ - public function countTotalConsent($consentUid): int + public function countTotalConsent(string $consentUid): int { $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ? AND deleted_at IS NULL"; $parameters = [sha1($consentUid)]; diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index 5739f8fca9..a31db407a9 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -27,6 +27,7 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Consent\ConsentHashService; use OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; class ConsentIntegrationTest extends TestCase @@ -68,9 +69,7 @@ public function setup(): void ); } - /** - * @dataProvider consentTypeProvider - */ + #[DataProvider('consentTypeProvider')] public function test_no_previous_consent_given($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); @@ -84,17 +83,15 @@ public function test_no_previous_consent_given($consentType) ->andReturn(ConsentVersion::notGiven()); switch ($consentType) { case ConsentType::TYPE_EXPLICIT: - $this->assertFalse($this->consent->explicitConsentWasGivenFor($serviceProvider)); + $this->assertFalse($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); break; case ConsentType::TYPE_IMPLICIT: - $this->assertFalse($this->consent->implicitConsentWasGivenFor($serviceProvider)); + $this->assertFalse($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); break; } } - /** - * @dataProvider consentTypeProvider - */ + #[DataProvider('consentTypeProvider')] public function test_unstable_previous_consent_given($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); @@ -116,17 +113,15 @@ public function test_unstable_previous_consent_given($consentType) switch ($consentType) { case ConsentType::TYPE_EXPLICIT: - $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); break; case ConsentType::TYPE_IMPLICIT: - $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); break; } } - /** - * @dataProvider consentTypeProvider - */ + #[DataProvider('consentTypeProvider')] public function test_stable_consent_given($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); @@ -148,17 +143,15 @@ public function test_stable_consent_given($consentType) switch ($consentType) { case ConsentType::TYPE_EXPLICIT: - $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); break; case ConsentType::TYPE_IMPLICIT: - $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); break; } } - /** - * @dataProvider consentTypeProvider - */ + #[DataProvider('consentTypeProvider')] public function test_give_consent_no_unstable_consent_given($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); @@ -187,27 +180,13 @@ public function test_give_consent_no_unstable_consent_given($consentType) } } - /** - * @dataProvider consentTypeProvider - */ + #[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'); - // 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()); + ->andReturn('collab:person:id:org-a:joe-a'); // Now assert that the new stable consent hash is going to be set $this->consentRepository ->shouldReceive('updateConsentHash') @@ -221,70 +200,42 @@ public function test_upgrade_to_stable_consent($consentType) )) ->andReturn(true); - $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); + // Pass the pre-fetched ConsentVersion (unstable) — no second DB query is made + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::unstable())); } - /** - * @dataProvider consentTypeProvider - */ + #[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 stored - $this->consentRepository - ->shouldReceive('hasConsentHash') - ->with(new ConsentHashQuery( - hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', - serviceId: 'service-provider-entity-id', - attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', - attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', - consentType: $consentType, - )) - ->once() - ->andReturn(ConsentVersion::stable()); - // 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)); + // No DB calls expected — stable consent does not trigger an update + $this->consentRepository->shouldNotReceive('hasConsentHash'); + $this->consentRepository->shouldNotReceive('storeConsentHash'); + $this->consentRepository->shouldNotReceive('updateConsentHash'); + + // Pass the pre-fetched ConsentVersion (stable) — no second DB query is made, no update triggered + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::stable())); } - /** - * @dataProvider consentTypeProvider - */ + #[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() - ->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 + // No DB calls expected — no consent means nothing to upgrade + $this->consentRepository->shouldNotReceive('hasConsentHash'); $this->consentRepository->shouldNotReceive('updateConsentHash'); - $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); + // Pass the pre-fetched ConsentVersion (notGiven) — no update should be triggered + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::notGiven())); } - /** - * @dataProvider consentTypeProvider - */ + #[DataProvider('consentTypeProvider')] public function test_upgrade_continues_gracefully_when_attributes_changed($consentType) { $serviceProvider = new ServiceProvider("service-provider-entity-id"); $this->response->shouldReceive('getNameIdValue') - ->twice() - ->andReturn('collab:person:id:org-a:joe-a'); - // Old-style (unstable) consent is found - $this->consentRepository - ->shouldReceive('hasConsentHash') ->once() - ->andReturn(ConsentVersion::unstable()); + ->andReturn('collab:person:id:org-a:joe-a'); // But the UPDATE matches 0 rows (attributes changed since consent was given) $this->consentRepository ->shouldReceive('updateConsentHash') @@ -292,7 +243,27 @@ public function test_upgrade_continues_gracefully_when_attributes_changed($conse ->andReturn(false); // Must not throw; the warning is logged inside the repository - $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType)); + // Pass the pre-fetched ConsentVersion (unstable) — no second DB query is made + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::unstable())); + } + + public function test_store_consent_hash_sql_resets_deleted_at_on_duplicate(): void + { + // The storeConsentHash SQL must reset deleted_at='0000-00-00 00:00:00' in the + // ON DUPLICATE KEY UPDATE clause so soft-deleted rows become active again. + // We verify this by checking the SQL string directly. + new \ReflectionClass(DbalConsentRepository::class); + + // Read the SQL from the source to verify it contains the deleted_at reset + // This is a documentation test — if the SQL is refactored, update it here too. + $source = file_get_contents( + __DIR__ . '/../../../../../../src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php' + ); + $this->assertStringContainsString( + "deleted_at='0000-00-00 00:00:00'", + $source, + 'ON DUPLICATE KEY UPDATE must reset deleted_at so soft-deleted re-consent rows become active' + ); } public static function consentTypeProvider(): iterable diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index c0b073c343..8cd8a7f022 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -67,8 +67,8 @@ public function testConsentDisabledDoesNotWriteToDatabase() $this->consentService->shouldNotReceive('retrieveConsentHash'); $this->consentService->shouldNotReceive('updateConsentHash'); - $this->assertTrue($this->consentDisabled->explicitConsentWasGivenFor($serviceProvider)); - $this->assertTrue($this->consentDisabled->implicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consentDisabled->explicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertTrue($this->consentDisabled->implicitConsentWasGivenFor($serviceProvider)->given()); $this->assertTrue($this->consentDisabled->giveExplicitConsentFor($serviceProvider)); $this->assertTrue($this->consentDisabled->giveImplicitConsentFor($serviceProvider)); } @@ -80,8 +80,8 @@ public function testUpgradeAttributeHashSkippedWhenConsentDisabled() $this->consentService->shouldNotReceive('retrieveConsentHash'); $this->consentService->shouldNotReceive('updateConsentHash'); - $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT); - $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_IMPLICIT); + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT, ConsentVersion::stable()); + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_IMPLICIT, ConsentVersion::stable()); } public function testConsentWriteToDatabase() @@ -93,8 +93,8 @@ public function testConsentWriteToDatabase() $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(ConsentVersion::stable()); $this->consentService->shouldReceive('storeConsentHash')->andReturn(true); - $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)); - $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)); + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); } @@ -141,7 +141,7 @@ public function testConsentUidFromAmPriorToConsentEnabled() $result = $consentWithAmPrior->explicitConsentWasGivenFor($serviceProvider); // Assert: consent check succeeded and the AM-prior uid path was used - $this->assertTrue($result); + $this->assertTrue($result->given()); Phake::verify($originalResponse)->getCollabPersonId(); Phake::verify($mockedResponse, Phake::never())->getNameIdValue(); } @@ -169,12 +169,12 @@ public function testNullNameIdReturnsNoConsentWithoutCallingRepository() $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)); + $this->assertFalse($consentWithNullUid->explicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertFalse($consentWithNullUid->implicitConsentWasGivenFor($serviceProvider)->given()); // 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); + $consentWithNullUid->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT, ConsentVersion::notGiven()); } } diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php index 18095329ff..190b7dff1f 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.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\Metadata\MetadataRepository\InMemoryMetadataRepository; use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface; @@ -148,6 +149,7 @@ public function testConsentIsStored() } public function testResponseIsSent() { + $this->mockConsent(); $processConsentService = $this->factoryService(); Phake::when($this->proxyServerMock) @@ -221,7 +223,7 @@ private function mockConsent() $consentMock = Phake::mock('EngineBlock_Corto_Model_Consent'); Phake::when($consentMock) ->explicitConsentWasGivenFor(Phake::anyParameters()) - ->thenReturn(false); + ->thenReturn(ConsentVersion::notGiven()); Phake::when($this->consentFactoryMock) ->create(Phake::anyParameters()) ->thenReturn($consentMock); diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php index 971d39945f..77bfb3f63c 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php @@ -17,6 +17,7 @@ */ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Coins; use OpenConext\EngineBlock\Metadata\ConsentSettings; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; @@ -123,7 +124,7 @@ public function testConsentIsSkippedWhenPriorConsentIsStored() Phake::when($this->consentMock) ->explicitConsentWasGivenFor(Phake::anyParameters()) - ->thenReturn(true); + ->thenReturn(ConsentVersion::stable()); $provideConsentService->serve(null, $this->httpRequestMock); @@ -279,7 +280,10 @@ private function mockConsent() $consentMock = Phake::mock('EngineBlock_Corto_Model_Consent'); Phake::when($consentMock) ->explicitConsentWasGivenFor(Phake::anyParameters()) - ->thenReturn(false); + ->thenReturn(ConsentVersion::notGiven()); + Phake::when($consentMock) + ->implicitConsentWasGivenFor(Phake::anyParameters()) + ->thenReturn(ConsentVersion::notGiven()); Phake::when($this->consentFactoryMock) ->create(Phake::anyParameters()) ->thenReturn($consentMock); diff --git a/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php b/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php index 3a4c614bde..05e70f620b 100644 --- a/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php +++ b/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php @@ -21,17 +21,9 @@ class EngineBlock_Test_Saml2_NameIdResolverMock extends EngineBlock_Saml2_NameId private $_serviceProviderUuids = array(); private $_persistentIds = array(); - protected function _getUserDirectory() + protected function _getUserUuid($collabPersonId) { - $mock = new EngineBlock_Test_UserDirectoryMock(); - $mock->setUser( - 'urn:collab:person:example.edu:mock1', - array( - 'collabpersonid' => 'urn:collab:person:example.edu:mock1', - 'collabpersonuuid' => '', - ) - ); - return $mock; + return sha1($collabPersonId); } protected function _fetchPersistentId($serviceProviderUuid, $userUuid) diff --git a/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php b/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php index 228d62336b..fa8760c3ee 100644 --- a/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php +++ b/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php @@ -162,8 +162,6 @@ public function testMetadataOverAuthnRequest() public function testPersistent(): void { - $this->markTestSkipped('Fails when switching to other backend, test should not rely on having fixed backend'); - // Input $nameIdFormat = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'; $this->serviceProvider->nameIdFormat = $nameIdFormat; diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 25e9cc097e..4e1bcf3fbe 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -37,7 +37,7 @@ - + src library diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index 43579c62a6..cb9e3c42fa 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -612,4 +612,28 @@ public function test_stable_attribute_hash_zero_float_not_removed_as_empty() // Must be idempotent $this->assertSame($hash, $this->chs->getStableAttributesHash($withZeroFloat, true)); } + + public function test_stable_attribute_hash_handles_multibyte_utf8_values(): void + { + // Arabic, Chinese, accented names — all common in European SAML federations + $attributes = [ + 'urn:mace:dir:attribute-def:sn' => ['Müller'], + 'urn:mace:dir:attribute-def:cn' => ['محمد'], + 'urn:mace:dir:attribute-def:displayName' => ['王芳'], + ]; + $hash = $this->chs->getStableAttributesHash($attributes, true); + $this->assertIsString($hash); + $this->assertEquals(40, strlen($hash), 'SHA1 hash must be 40 hex chars; a false return from unserialize produces a wrong hash'); + } + + public function test_stable_hash_is_case_insensitive_for_multibyte_strings(): void + { + $lower = ['urn:mace:dir:attribute-def:sn' => ['müller']]; + $upper = ['urn:mace:dir:attribute-def:sn' => ['Müller']]; + $this->assertEquals( + $this->chs->getStableAttributesHash($lower, true), + $this->chs->getStableAttributesHash($upper, true), + 'Stable hash must be case-insensitive for multi-byte characters' + ); + } } From 260c436047e840f1c93d8e67a2628458b748d77f Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Tue, 17 Mar 2026 13:57:42 +0100 Subject: [PATCH 16/16] feat(consent): add feature toggle for stable hash migration Introduces `eb.stable_consent_hash_migration` (default: false) to control when the legacy `attribute` column is phased out. Toggle OFF (default): new consent writes both `attribute` and `attribute_stable` so old EB instances still find records during a rolling deploy; upgrades leave `attribute` intact for the same reason. Toggle ON: new consent writes only `attribute_stable` (attribute=NULL) and upgrades null the old column, cleaning it up over time once all instances are on the new version. --- config/packages/engineblock_features.yaml | 1 + config/packages/parameters.yml.dist | 1 + config/services/services.yml | 1 + library/EngineBlock/Corto/Model/Consent.php | 1 + .../Version20260315000001.php | 4 +- .../Value/ConsentStoreParameters.php | 1 + .../Value/ConsentUpdateParameters.php | 1 + .../Service/Consent/ConsentHashService.php | 36 ++++++- .../Repository/DbalConsentRepository.php | 54 ++++++++-- .../Corto/Model/ConsentIntegrationTest.php | 98 +++++++++++++++++-- .../Consent/ConsentHashServiceTest.php | 4 +- 11 files changed, 183 insertions(+), 19 deletions(-) diff --git a/config/packages/engineblock_features.yaml b/config/packages/engineblock_features.yaml index 8e7ed423c1..b1a3d003d4 100644 --- a/config/packages/engineblock_features.yaml +++ b/config/packages/engineblock_features.yaml @@ -16,3 +16,4 @@ parameters: eb.stepup.sfo.override_engine_entityid: "%feature_stepup_sfo_override_engine_entityid%" eb.stepup.send_user_attributes: "%feature_stepup_send_user_attributes%" eb.feature_enable_sram_interrupt: "%feature_enable_sram_interrupt%" + eb.stable_consent_hash_migration: "%feature_stable_consent_hash_migration%" diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index cfc3509c5c..0e29ec405b 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -224,6 +224,7 @@ parameters: feature_stepup_sfo_override_engine_entityid: false feature_stepup_send_user_attributes: false feature_enable_sram_interrupt: false + feature_stable_consent_hash_migration: false ########################################################################################## ## PROFILE SETTINGS diff --git a/config/services/services.yml b/config/services/services.yml index 344336ab6d..1ee0e3032c 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -80,6 +80,7 @@ services: public: false arguments: - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository' + - '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration' OpenConext\EngineBlock\Service\Consent\ConsentService: arguments: diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index da31206e40..65e76239d3 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -169,6 +169,7 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): serviceId: $serviceProvider->entityId, attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), consentType: $consentType, + attributeHash: $this->_getAttributesHash($this->_responseAttributes), ); return $this->_hashService->storeConsentHash($parameters); diff --git a/migrations/DoctrineMigrations/Version20260315000001.php b/migrations/DoctrineMigrations/Version20260315000001.php index afccb8e634..c2cc56a79f 100644 --- a/migrations/DoctrineMigrations/Version20260315000001.php +++ b/migrations/DoctrineMigrations/Version20260315000001.php @@ -48,6 +48,8 @@ public function up(Schema $schema): void 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`'); + $this->addSql('UPDATE consent SET attribute = attribute_stable WHERE attribute IS NULL AND attribute_stable IS NOT NULL'); + $this->addSql('ALTER TABLE consent CHANGE attribute attribute VARCHAR(80) NOT NULL'); + $this->addSql('ALTER TABLE consent DROP attribute_stable'); } } diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php index a54b5d8d9e..adcef92194 100644 --- a/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php @@ -25,6 +25,7 @@ public function __construct( public readonly string $serviceId, public readonly string $attributeStableHash, public readonly string $consentType, + public readonly ?string $attributeHash = null, ) { } } diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php index 7df9239d08..b2c09432a9 100644 --- a/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php @@ -26,6 +26,7 @@ public function __construct( public readonly string $hashedUserId, public readonly string $serviceId, public readonly string $consentType, + public readonly bool $clearLegacyHash = false, ) { } } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index e37bb342e8..043f8a6ee4 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -23,6 +23,7 @@ use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; +use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; use SAML2\XML\saml\NameID; use function array_filter; use function array_keys; @@ -40,14 +41,22 @@ final class ConsentHashService implements ConsentHashServiceInterface { + private const FEATURE_MIGRATION = 'eb.stable_consent_hash_migration'; + /** * @var ConsentRepository */ private $consentRepository; - public function __construct(ConsentRepository $consentHashRepository) + /** + * @var FeatureConfigurationInterface + */ + private $featureConfiguration; + + public function __construct(ConsentRepository $consentHashRepository, FeatureConfigurationInterface $featureConfiguration) { $this->consentRepository = $consentHashRepository; + $this->featureConfiguration = $featureConfiguration; } public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion @@ -57,11 +66,36 @@ public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion public function storeConsentHash(ConsentStoreParameters $parameters): bool { + $migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION); + + if ($migrationEnabled) { + $parameters = new ConsentStoreParameters( + hashedUserId: $parameters->hashedUserId, + serviceId: $parameters->serviceId, + attributeStableHash: $parameters->attributeStableHash, + consentType: $parameters->consentType, + attributeHash: null, + ); + } + return $this->consentRepository->storeConsentHash($parameters); } public function updateConsentHash(ConsentUpdateParameters $parameters): bool { + $migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION); + + if ($migrationEnabled) { + $parameters = new ConsentUpdateParameters( + attributeStableHash: $parameters->attributeStableHash, + attributeHash: $parameters->attributeHash, + hashedUserId: $parameters->hashedUserId, + serviceId: $parameters->serviceId, + consentType: $parameters->consentType, + clearLegacyHash: true, + ); + } + return $this->consentRepository->updateConsentHash($parameters); } diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index e56872f643..edbdac5135 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -226,18 +226,33 @@ public function hasConsentHash(ConsentHashQuery $query): ConsentVersion */ 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(), deleted_at='0000-00-00 00:00:00'"; - - try { - $this->connection->executeStatement($query, [ + if ($parameters->attributeHash !== null) { + $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, attribute_stable, consent_type, consent_date, deleted_at) + VALUES (?, ?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') + ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), attribute_stable=VALUES(attribute_stable), + consent_type=VALUES(consent_type), consent_date=NOW(), deleted_at='0000-00-00 00:00:00'"; + $bindings = [ $parameters->hashedUserId, $parameters->serviceId, + $parameters->attributeHash, $parameters->attributeStableHash, $parameters->consentType, - ]); + ]; + } else { + $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(), deleted_at='0000-00-00 00:00:00'"; + $bindings = [ + $parameters->hashedUserId, + $parameters->serviceId, + $parameters->attributeStableHash, + $parameters->consentType, + ]; + } + + try { + $this->connection->executeStatement($query, $bindings); } catch (Exception $e) { throw new RuntimeException( sprintf('Error storing consent: "%s"', $e->getMessage()) @@ -252,7 +267,8 @@ public function storeConsentHash(ConsentStoreParameters $parameters): bool */ public function updateConsentHash(ConsentUpdateParameters $parameters): bool { - $query = " + if ($parameters->clearLegacyHash) { + $query = " UPDATE consent SET @@ -268,7 +284,25 @@ public function updateConsentHash(ConsentUpdateParameters $parameters): bool consent_type = ? AND deleted_at IS NULL - "; + "; + } else { + $query = " + UPDATE + consent + SET + attribute_stable = ? + WHERE + attribute = ? + AND + hashed_user_id = ? + AND + service_id = ? + AND + consent_type = ? + AND + deleted_at IS NULL + "; + } try { $affected = $this->connection->executeStatement($query, [ diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index a31db407a9..e979ddbd7a 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -27,6 +27,7 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Consent\ConsentHashService; use OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository; +use OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -56,8 +57,20 @@ public function setup(): void $this->response = Mockery::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class); $this->consentRepository = Mockery::mock(ConsentRepository::class); - $this->consentService = new ConsentHashService($this->consentRepository); + $this->buildConsentAndService(migrationEnabled: true); + } + + /** + * Rebuilds $this->consentService and $this->consent with the given toggle state. + * Call this in tests that need a specific toggle setting different from setUp's default. + */ + private function buildConsentAndService(bool $migrationEnabled): void + { + $featureConfig = new FeatureConfiguration([ + 'eb.stable_consent_hash_migration' => $migrationEnabled, + ]); + $this->consentService = new ConsentHashService($this->consentRepository, $featureConfig); $this->consent = new EngineBlock_Corto_Model_Consent( "consent", true, @@ -151,14 +164,19 @@ public function test_stable_consent_given($consentType) } } + /** + * Toggle ON (migration enabled): new consent stores only the stable hash. + * The legacy attribute column must be left NULL so fully-migrated deployments + * don't accumulate unnecessary data in the old column. + */ #[DataProvider('consentTypeProvider')] - public function test_give_consent_no_unstable_consent_given($consentType) + public function test_give_consent_toggle_on_stores_only_stable_hash($consentType) { + // setUp already builds with migrationEnabled=true $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() @@ -167,6 +185,7 @@ public function test_give_consent_no_unstable_consent_given($consentType) serviceId: 'service-provider-entity-id', attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', consentType: $consentType, + attributeHash: null, )) ->andReturn(true); @@ -180,14 +199,53 @@ public function test_give_consent_no_unstable_consent_given($consentType) } } + /** + * Toggle OFF (migration disabled): new consent stores BOTH hashes so that + * old EB instances (still reading only the `attribute` column) can still + * find the consent record during a rolling deploy. + */ #[DataProvider('consentTypeProvider')] - public function test_upgrade_to_stable_consent($consentType) + public function test_give_consent_toggle_off_stores_both_hashes($consentType) { + $this->buildConsentAndService(migrationEnabled: false); + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + $this->consentRepository + ->shouldReceive('storeConsentHash') + ->once() + ->with(new ConsentStoreParameters( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType, + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + )) + ->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; + } + } + + /** + * Toggle OFF (migration disabled): upgrading an old unstable consent leaves + * the legacy `attribute` column intact so old instances keep working. + */ + #[DataProvider('consentTypeProvider')] + public function test_upgrade_toggle_off_preserves_legacy_hash($consentType) + { + $this->buildConsentAndService(migrationEnabled: false); $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('updateConsentHash') ->once() @@ -197,10 +255,38 @@ public function test_upgrade_to_stable_consent($consentType) hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', serviceId: 'service-provider-entity-id', consentType: $consentType, + clearLegacyHash: false, + )) + ->andReturn(true); + + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::unstable())); + } + + /** + * Toggle ON (migration enabled): upgrading an old unstable consent nulls the + * legacy `attribute` column so the old column is cleaned up over time. + */ + #[DataProvider('consentTypeProvider')] + public function test_upgrade_toggle_on_clears_legacy_hash($consentType) + { + // setUp already builds with migrationEnabled=true + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + $this->consentRepository + ->shouldReceive('updateConsentHash') + ->once() + ->with(new ConsentUpdateParameters( + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + consentType: $consentType, + clearLegacyHash: true, )) ->andReturn(true); - // Pass the pre-fetched ConsentVersion (unstable) — no second DB query is made $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::unstable())); } diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index cb9e3c42fa..5c051d4d7f 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -21,6 +21,7 @@ use Mockery as m; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; +use OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration; use PHPUnit\Framework\TestCase; use SAML2\XML\saml\NameID; @@ -36,7 +37,8 @@ class ConsentHashServiceTest extends TestCase public function setUp(): void { $mockConsentHashRepository = m::mock(ConsentRepository::class); - $this->chs = new ConsentHashService($mockConsentHashRepository); + $featureConfig = new FeatureConfiguration(['eb.stable_consent_hash_migration' => false]); + $this->chs = new ConsentHashService($mockConsentHashRepository, $featureConfig); } public function test_stable_attribute_hash_switched_order_associative_array()