From ede32a1a3edc3a5aa0061be2584b9d498270fc70 Mon Sep 17 00:00:00 2001 From: Koen Cornelis Date: Tue, 11 May 2021 22:15:54 +0200 Subject: [PATCH 01/12] 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 --- library/EngineBlock/Corto/Model/Consent.php | 88 +++- .../Corto/Model/Consent/Factory.php | 13 +- .../Service/Consent/ConsentHashService.php | 147 ++++++ .../Service/{ => Consent}/ConsentService.php | 0 .../{ => Consent}/ConsentServiceInterface.php | 0 .../Resources/config/compat.yml | 1 + .../Resources/config/services.yml | 4 + .../Consent/ConsentHashServiceTest.php | 446 ++++++++++++++++++ 8 files changed, 670 insertions(+), 29 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/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 96c013357d..5a4c43c52a 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -18,6 +18,7 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Service\Consent\ConsentHashService; class EngineBlock_Corto_Model_Consent { @@ -36,7 +37,7 @@ class EngineBlock_Corto_Model_Consent */ private $_response; /** - * @var array + * @var array All attributes as an associative array. */ private $_responseAttributes; @@ -59,6 +60,11 @@ class EngineBlock_Corto_Model_Consent */ private $_consentEnabled; + /** + * @var ConsentHashService + */ + private $_hashService; + /** * @param string $tableName * @param bool $mustStoreValues @@ -67,6 +73,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, @@ -75,7 +82,8 @@ public function __construct( array $responseAttributes, EngineBlock_Database_ConnectionFactory $databaseConnectionFactory, $amPriorToConsentEnabled, - $consentEnabled + $consentEnabled, + $hashService ) { $this->_tableName = $tableName; @@ -84,33 +92,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 bool|PDO */ @@ -128,21 +157,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); } - private function _storeConsent(ServiceProvider $serviceProvider, $consentType) + protected function _getStableAttributesHash($attributes): string + { + return $this->_hashService->getStableAttributesHash($attributes, $this->_mustStoreValues); + } + + private function _storeConsent(ServiceProvider $serviceProvider, $consentType): bool { $dbh = $this->_getConsentDatabaseConnection(); if (!$dbh) { @@ -155,7 +180,7 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType) $parameters = array( sha1($this->_getConsentUid()), $serviceProvider->entityId, - $this->_getAttributesHash($this->_responseAttributes), + $this->_getStableAttributesHash($this->_responseAttributes), $consentType, ); @@ -177,16 +202,27 @@ 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; + } + + $unstableConsentHash = $this->_getAttributesHash($this->_responseAttributes); + $hasUnstableConsentHash = $this->retrieveConsentHashFromDb($dbh, $serviceProvider, $consentType, $unstableConsentHash); + + if ($hasUnstableConsentHash) { + return true; + } - $attributesHash = $this->_getAttributesHash($this->_responseAttributes); + $stableConsentHash = $this->_getStableAttributesHash($this->_responseAttributes); + return $this->retrieveConsentHashFromDb($dbh, $serviceProvider, $consentType, $stableConsentHash); + } + private function retrieveConsentHashFromDb(PDO $dbh, ServiceProvider $serviceProvider, $consentType, $attributesHash): bool + { + try { $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/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml b/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml index a9e13f431b..49e8ca980c 100644 --- a/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml +++ b/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml @@ -48,6 +48,7 @@ services: arguments: - "@engineblock.compat.corto_filter_command_factory" - "@engineblock.compat.database_connection_factory" + - "@engineblock.service.consent.ConsentHashService" engineblock.compat.saml2_id_generator: class: EngineBlock_Saml2_IdGenerator_Default diff --git a/src/OpenConext/EngineBlockBundle/Resources/config/services.yml b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml index e5ad223fbb..7c5f87dfab 100644 --- a/src/OpenConext/EngineBlockBundle/Resources/config/services.yml +++ b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml @@ -366,3 +366,7 @@ services: - "@translator" tags: - { name: 'twig.extension' } + + engineblock.service.consent.ConsentHashService: + class: OpenConext\EngineBlock\Service\Consent\ConsentHashService + public: false 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 e4a0b2f636bd35db943b01628c850225e7567df8 Mon Sep 17 00:00:00 2001 From: Koen Cornelis Date: Thu, 20 May 2021 11:31:33 +0200 Subject: [PATCH 02/12] Move DB logic to ConsentHashRepository --- library/EngineBlock/Corto/Model/Consent.php | 116 ++++-------------- .../Corto/Model/Consent/Factory.php | 2 + .../Service/Consent/ConsentHashRepository.php | 111 +++++++++++++++++ .../Service/Consent/ConsentHashService.php | 39 +++++- .../Resources/config/services.yml | 6 +- .../Consent/ConsentHashServiceTest.php | 7 +- 6 files changed, 185 insertions(+), 96 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 5a4c43c52a..7be5e3d79d 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -66,26 +66,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; @@ -120,24 +112,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); } /** @@ -174,9 +157,6 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): 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($this->_getConsentUid()), $serviceProvider->entityId, @@ -184,22 +164,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 - ); - } - - /** @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; + return $this->_hashService->storeConsentHashInDb($dbh, $parameters); } private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): bool @@ -209,53 +174,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 { - $query = " - SELECT * - FROM {$this->_tableName} - WHERE hashed_user_id = ? - AND service_id = ? - AND attribute = ? - AND consent_type = ? - AND deleted_at IS NULL - "; - $hashedUserId = sha1($this->_getConsentUid()); - $parameters = array( - $hashedUserId, - $serviceProvider->entityId, - $attributesHash, - $consentType, - ); - - /** @var $statement PDOStatement */ - $statement = $dbh->prepare($query); - $statement->execute($parameters); - $rows = $statement->fetchAll(); - - if (count($rows) < 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/src/OpenConext/EngineBlockBundle/Resources/config/services.yml b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml index 7c5f87dfab..30f2e263d2 100644 --- a/src/OpenConext/EngineBlockBundle/Resources/config/services.yml +++ b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml @@ -367,6 +367,10 @@ services: tags: - { name: 'twig.extension' } + engineblock.service.consent.ConsentHashRepository: + class: OpenConext\EngineBlock\Service\Consent\ConsentHashRepository + engineblock.service.consent.ConsentHashService: class: OpenConext\EngineBlock\Service\Consent\ConsentHashService - public: false + arguments: + - "engineblock.service.consent.ConsentHashRepository" 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 b0a67c2c5414183d50dde80b24c90435840f9cc5 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 09:10:01 +0200 Subject: [PATCH 03/12] 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 | 81 ++++++++++++- .../Resources/config/services.yml | 5 +- .../Consent/ConsentHashServiceTest.php | 3 +- 6 files changed, 99 insertions(+), 129 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 881a7dacfe..cd0a00aa2f 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\Value\ConsentType; use OpenConext\EngineBlock\Exception\RuntimeException; use PDO; +use PDOException; use Psr\Log\LoggerInterface; use function sha1; @@ -56,7 +57,7 @@ public function __construct(DbalConnection $connection, LoggerInterface $logger) */ public function findAllFor($userId) { - $sql = ' + $sql = ' SELECT service_id , consent_date @@ -129,7 +130,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( @@ -161,4 +163,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/src/OpenConext/EngineBlockBundle/Resources/config/services.yml b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml index 30f2e263d2..4a5623ad33 100644 --- a/src/OpenConext/EngineBlockBundle/Resources/config/services.yml +++ b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml @@ -367,10 +367,7 @@ services: tags: - { name: 'twig.extension' } - engineblock.service.consent.ConsentHashRepository: - class: OpenConext\EngineBlock\Service\Consent\ConsentHashRepository - engineblock.service.consent.ConsentHashService: class: OpenConext\EngineBlock\Service\Consent\ConsentHashService arguments: - - "engineblock.service.consent.ConsentHashRepository" + - "@engineblock.repository.consent" 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 499ac7e99a53188c6f6e4f932297aa0026b3e201 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 09:49:02 +0200 Subject: [PATCH 04/12] 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 | 44 +++---------------- .../Corto/Model/Consent/Factory.php | 10 +---- .../Service/Consent/ConsentHashService.php | 6 +-- .../Consent/ConsentHashServiceInterface.php | 32 ++++++++++++++ .../Test/Corto/Model/ConsentTest.php | 31 ++++++++----- 5 files changed, 64 insertions(+), 59 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 7be5e3d79d..0a568146e7 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -18,7 +18,7 @@ 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 { @@ -41,11 +41,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 * @@ -61,7 +56,7 @@ class EngineBlock_Corto_Model_Consent private $_consentEnabled; /** - * @var ConsentHashService + * @var ConsentHashServiceInterface */ private $_hashService; @@ -73,16 +68,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; @@ -114,21 +107,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 bool|PDO - */ - protected function _getConsentDatabaseConnection() - { - return $this->_databaseConnectionFactory->create(); + return $this->_hashService->countTotalConsent($consentUid); } protected function _getConsentUid() @@ -152,11 +132,6 @@ protected function _getStableAttributesHash($attributes): string private function _storeConsent(ServiceProvider $serviceProvider, $consentType): bool { - $dbh = $this->_getConsentDatabaseConnection(); - if (!$dbh) { - return false; - } - $parameters = array( sha1($this->_getConsentUid()), $serviceProvider->entityId, @@ -164,16 +139,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, @@ -181,7 +151,7 @@ private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentTyp $consentType, ); - $hasUnstableConsentHash = $this->_hashService->retrieveConsentHashFromDb($dbh, $parameters); + $hasUnstableConsentHash = $this->_hashService->retrieveConsentHash($parameters); if ($hasUnstableConsentHash) { return true; @@ -194,6 +164,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() 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 6a5bcd366bb6e19ebaf1d212611aaff02cf6550d Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 2 May 2022 15:53:36 +0200 Subject: [PATCH 05/12] Integrate upstream consent changes --- 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 +- src/OpenConext/EngineBlockBundle/Resources/config/compat.yml | 1 - .../EngineBlockBundle/Resources/config/services.yml | 2 +- tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php | 2 +- .../Test/Corto/Module/Service/ProvideConsentTest.php | 2 +- .../OpenConext/EngineBlock/Service/ConsentServiceTest.php | 1 + 12 files changed, 14 insertions(+), 14 deletions(-) diff --git a/library/EngineBlock/Application/DiContainer.php b/library/EngineBlock/Application/DiContainer.php index bf08ac5797..8270b749f7 100644 --- a/library/EngineBlock/Application/DiContainer.php +++ b/library/EngineBlock/Application/DiContainer.php @@ -152,7 +152,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 0a568146e7..0a9516b472 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -18,7 +18,7 @@ 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 c41cd9bec5..8cdedc4ca8 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 SAML2\Constants; use Symfony\Component\HttpFoundation\Request; diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php index b73b6b805c..2fce493864 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentService.php @@ -16,7 +16,7 @@ * limitations under the License. */ -namespace OpenConext\EngineBlock\Service; +namespace OpenConext\EngineBlock\Service\Consent; use Exception; use OpenConext\EngineBlock\Authentication\Dto\Consent; @@ -25,6 +25,7 @@ use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use OpenConext\EngineBlock\Authentication\Value\CollabPersonId; use OpenConext\EngineBlock\Exception\RuntimeException; +use OpenConext\EngineBlock\Service\MetadataServiceInterface; use OpenConext\Value\Saml\EntityId; use Psr\Log\LoggerInterface; use function sprintf; @@ -37,7 +38,7 @@ final class ConsentService implements ConsentServiceInterface private $consentRepository; /** - * @var MetadataService + * @var MetadataServiceInterface */ private $metadataService; diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php index 4ebb10c832..32f800ec9a 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php @@ -16,7 +16,7 @@ * limitations under the License. */ -namespace OpenConext\EngineBlock\Service; +namespace OpenConext\EngineBlock\Service\Consent; use OpenConext\EngineBlock\Authentication\Dto\ConsentList; use OpenConext\EngineBlock\Authentication\Value\CollabPersonId; diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index cd0a00aa2f..722d15929b 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -170,11 +170,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 c1e4ef3d5e..4d44644a5c 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\Exception\InvalidArgumentException as EngineBlockInvalidArgumentException; use OpenConext\EngineBlockBundle\Factory\CollabPersonIdFactory; diff --git a/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml b/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml index 49e8ca980c..0e0d7301ca 100644 --- a/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml +++ b/src/OpenConext/EngineBlockBundle/Resources/config/compat.yml @@ -47,7 +47,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/src/OpenConext/EngineBlockBundle/Resources/config/services.yml b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml index 4a5623ad33..20ccb596c4 100644 --- a/src/OpenConext/EngineBlockBundle/Resources/config/services.yml +++ b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml @@ -72,7 +72,7 @@ services: - "@engineblock.configuration.stepup.loa_repository" engineblock.service.consent: - class: OpenConext\EngineBlock\Service\ConsentService + class: OpenConext\EngineBlock\Service\Consent\ConsentService arguments: - "@engineblock.repository.consent" - "@engineblock.service.metadata" diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index da7c5bc5b2..394c24b9b6 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_Consent_Test 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 806a5cdaaa..a8aa0f3ffa 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 84e37e7dee..9c959ada67 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\TestCase; use Psr\Log\LoggerInterface; From 109e0681b5a647735c2e9b857220b0ba64b4067a Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 08:41:04 +0200 Subject: [PATCH 06/12] 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 745243cbc130a549eafbeee6f24b6d907689f396 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:26:01 +0200 Subject: [PATCH 07/12] Add the `attribute_stable` column to `consent` --- .../Version20220503092351.php | 30 +++++++++++++++++++ .../Authentication/Entity/Consent.php | 9 +++++- 2 files changed, 38 insertions(+), 1 deletion(-) 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 9380af890b..c9baf52314 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php @@ -60,10 +60,17 @@ class Consent /** * @var string * - * @ORM\Column(type="string", length=80) + * @ORM\Column(type="string", length=80, nullable=true) */ public $attribute; + /** + * @var string + * + * @ORM\Column(type="string", length=80) + */ + public $attributeStable; + /** * @var string * From d796dfd23402b9dfc779facdf7e901d4cf434965 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:26:52 +0200 Subject: [PATCH 08/12] 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 3197e18f84661f7a41eca6ac63153c6e5f0afc1c Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 3 May 2022 15:28:08 +0200 Subject: [PATCH 09/12] 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 | 61 +++- .../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 | 14 +- .../Api/DeprovisionControllerTest.php | 1 + .../Corto/Model/ConsentIntegrationTest.php | 262 ++++++++++++++++++ .../Test/Corto/Model/ConsentTest.php | 8 +- 11 files changed, 445 insertions(+), 21 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 0a9516b472..323c58de52 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -16,6 +16,7 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; @@ -83,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 @@ -142,28 +156,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 8cdedc4ca8..51c241a043 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; @@ -136,6 +137,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()); @@ -153,6 +155,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 722d15929b..f69fce9d69 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -200,15 +200,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?!"); @@ -223,6 +254,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 702a170d8f..aa426f122d 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php @@ -507,8 +507,14 @@ private function disableConsentApiFeatureFor(Client $client) $container->set('engineblock.features', $featureToggles); } - private function addConsentFixture($userId, $serviceId, $attributeHash, $consentType, $consentDate, $deletedAt) - { + private function addConsentFixture( + $userId, + $serviceId, + $attributeHash, + $consentType, + $consentDate, + $deletedAt + ) { $queryBuilder = $this->getContainer()->get('doctrine')->getConnection()->createQueryBuilder(); $queryBuilder ->insert('consent') @@ -516,6 +522,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', @@ -523,7 +530,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 891ab4fc72..68699c6cf7 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php @@ -397,6 +397,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 394c24b9b6..9f32e5c158 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 2be36251cad648ab15c236a7fad86cbc58cb8a80 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 4 May 2022 11:58:58 +0200 Subject: [PATCH 10/12] 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 54433bf7263ce9134fee3df9c66858916b17a38c Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Thu, 5 May 2022 10:55:14 +0200 Subject: [PATCH 11/12] 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 323c58de52..bfffc05b15 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 145ebd9076e6502833f5979009ecd0e12ec442fe Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Thu, 5 May 2022 11:40:36 +0200 Subject: [PATCH 12/12] 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 bfffc05b15..014e225e47 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -178,31 +178,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 f69fce9d69..9ed4853024 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -24,6 +24,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; @@ -167,7 +168,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 @@ -179,7 +180,7 @@ public function hasConsentHash(array $parameters): bool AND service_id = ? AND - attribute = ? + (attribute = ? OR attribute_stable = ?) AND consent_type = ? AND @@ -192,45 +193,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 9f32e5c158..96d03cf82d 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'));