diff --git a/CHANGELOG.md b/CHANGELOG.md index e683afb1d0..908dd2bbab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,12 @@ Changes: * The `0000-00-00 00:00:00` is added for clarity/consistency, as this is probably the default behaviour of your database already. * Removed unused index `consent.deleted_at`. Delete this from your production database if it's there. +* Stabilized consent checks + * In order to make the consent hashes more robust, a more consistent way of hashing the user attributes has been introduced + * This feature automatically migrates from the old hashes to the new hashes, cleaning up the old hash. + * However, if blue/green deployments are used or if you want to keep the option open to roll back the EB release, keep the `feature_stable_consent_hash_migration` set to false in order to preserve the old consent hashes. + * Once the new release is fully rolled out, set `feature_stable_consent_hash_migration` to true. This will clean up the old consent hashes upon login. In the next EB release, the old consent hash column will be deleted. + ## 7.1.0 SRAM integration diff --git a/config/packages/engineblock_features.yaml b/config/packages/engineblock_features.yaml index 8e7ed423c1..b1a3d003d4 100644 --- a/config/packages/engineblock_features.yaml +++ b/config/packages/engineblock_features.yaml @@ -16,3 +16,4 @@ parameters: eb.stepup.sfo.override_engine_entityid: "%feature_stepup_sfo_override_engine_entityid%" eb.stepup.send_user_attributes: "%feature_stepup_send_user_attributes%" eb.feature_enable_sram_interrupt: "%feature_enable_sram_interrupt%" + eb.stable_consent_hash_migration: "%feature_stable_consent_hash_migration%" diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index cfc3509c5c..0e29ec405b 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -224,6 +224,7 @@ parameters: feature_stepup_sfo_override_engine_entityid: false feature_stepup_send_user_attributes: false feature_enable_sram_interrupt: false + feature_stable_consent_hash_migration: false ########################################################################################## ## PROFILE SETTINGS diff --git a/config/services/compat.yml b/config/services/compat.yml index 26b8946cf6..3c5db2e895 100644 --- a/config/services/compat.yml +++ b/config/services/compat.yml @@ -49,8 +49,7 @@ services: engineblock.compat.corto_model_consent_factory: 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: public: true diff --git a/config/services/controllers/api.yml b/config/services/controllers/api.yml index 90686d60ff..6a88a67570 100644 --- a/config/services/controllers/api.yml +++ b/config/services/controllers/api.yml @@ -16,7 +16,7 @@ services: - '@security.token_storage' - '@security.access.decision_manager' - '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration' - - '@OpenConext\EngineBlock\Service\ConsentService' + - '@OpenConext\EngineBlock\Service\Consent\ConsentService' OpenConext\EngineBlockBundle\Controller\Api\DeprovisionController: arguments: diff --git a/config/services/services.yml b/config/services/services.yml index 9cecb712ef..1ee0e3032c 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -75,7 +75,14 @@ services: - '@OpenConext\EngineBlock\Metadata\LoaRepository' - '@logger' - OpenConext\EngineBlock\Service\ConsentService: + engineblock.service.consent.ConsentHashService: + class: OpenConext\EngineBlock\Service\Consent\ConsentHashService + public: false + arguments: + - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository' + - '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration' + + OpenConext\EngineBlock\Service\Consent\ConsentService: arguments: - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository' - '@OpenConext\EngineBlock\Service\MetadataService' diff --git a/library/EngineBlock/Application/DiContainer.php b/library/EngineBlock/Application/DiContainer.php index 0916e348a8..03d4a6f3e6 100644 --- a/library/EngineBlock/Application/DiContainer.php +++ b/library/EngineBlock/Application/DiContainer.php @@ -161,11 +161,11 @@ public function getAuthenticationLoopGuard() } /** - * @return OpenConext\EngineBlock\Service\ConsentService + * @return OpenConext\EngineBlock\Service\Consent\ConsentService */ public function getConsentService() { - return $this->container->get(\OpenConext\EngineBlock\Service\ConsentService::class); + return $this->container->get(\OpenConext\EngineBlock\Service\Consent\ConsentService::class); } /** diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 40bd3e1ef6..6d9d65d879 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -16,17 +16,17 @@ * limitations under the License. */ -use Doctrine\DBAL\Statement; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Service\Consent\ConsentAttributes; +use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; class EngineBlock_Corto_Model_Consent { - /** - * @var string - */ - private $_tableName; - /** * @var bool */ @@ -37,15 +37,10 @@ class EngineBlock_Corto_Model_Consent */ private $_response; /** - * @var array + * @var array All attributes as an associative array. */ private $_responseAttributes; - /** - * @var EngineBlock_Database_ConnectionFactory - */ - private $_databaseConnectionFactory; - /** * A reflection of the eb.run_all_manipulations_prior_to_consent feature flag * @@ -61,63 +56,82 @@ class EngineBlock_Corto_Model_Consent private $_consentEnabled; /** - * @param string $tableName - * @param bool $mustStoreValues - * @param EngineBlock_Saml2_ResponseAnnotationDecorator $response - * @param array $responseAttributes - * @param EngineBlock_Database_ConnectionFactory $databaseConnectionFactory + * @var ConsentHashServiceInterface + */ + private $_hashService; + + /** * @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 */ public function __construct( - $tableName, - $mustStoreValues, + bool $mustStoreValues, EngineBlock_Saml2_ResponseAnnotationDecorator $response, array $responseAttributes, - EngineBlock_Database_ConnectionFactory $databaseConnectionFactory, - $amPriorToConsentEnabled, - $consentEnabled - ) - { - $this->_tableName = $tableName; + bool $amPriorToConsentEnabled, + bool $consentEnabled, + ConsentHashServiceInterface $hashService + ) { $this->_mustStoreValues = $mustStoreValues; $this->_response = $response; $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): ConsentVersion { - return !$this->_consentEnabled || - $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); + if (!$this->_consentEnabled) { + // Consent disabled: treat as already given (stable — no upgrade needed) + return ConsentVersion::stable(); + } + return $this->_hasStoredConsent($serviceProvider, ConsentType::Explicit); } - public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider) + /** + * Although the user has given consent previously we want to upgrade the deprecated unstable consent + * to the new stable consent type. + * https://www.pivotaltracker.com/story/show/176513931 + * + * The caller must pass the ConsentVersion already retrieved by explicitConsentWasGivenFor or + * implicitConsentWasGivenFor to avoid a second identical DB query. + * + * @deprecated Remove after stable consent hash is running in production + */ + public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, ConsentType $consentType, ConsentVersion $consentVersion): void { - return !$this->_consentEnabled || - $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); + if (!$this->_consentEnabled) { + return; + } + if ($consentVersion->isUnstable()) { + $this->_updateConsent($serviceProvider, $consentType); + } + } + + public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): ConsentVersion + { + if (!$this->_consentEnabled) { + return ConsentVersion::stable(); + } + return $this->_hasStoredConsent($serviceProvider, ConsentType::Implicit); } - public function giveExplicitConsentFor(ServiceProvider $serviceProvider) + public function giveExplicitConsentFor(ServiceProvider $serviceProvider): bool { return !$this->_consentEnabled || - $this->_storeConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); + $this->_storeConsent($serviceProvider, ConsentType::Explicit); } - public function giveImplicitConsentFor(ServiceProvider $serviceProvider) + public function giveImplicitConsentFor(ServiceProvider $serviceProvider): bool { return !$this->_consentEnabled || - $this->_storeConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); + $this->_storeConsent($serviceProvider, ConsentType::Implicit); } - /** - * @return Doctrine\DBAL\Connection - */ - protected function _getConsentDatabaseConnection() + public function countTotalConsent(): int { - return $this->_databaseConnectionFactory->create(); + $consentUid = $this->_getConsentUid(); + return $this->_hashService->countTotalConsent($consentUid); } protected function _getConsentUid() @@ -129,116 +143,72 @@ protected function _getConsentUid() return $this->_response->getNameIdValue(); } - protected function _getAttributesHash($attributes) + /** @deprecated Remove after stable consent hash is running in production */ + 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 { - $dbh = $this->_getConsentDatabaseConnection(); - if (!$dbh) { - return false; - } + $consentAttributes = $this->_mustStoreValues + ? ConsentAttributes::withValues($attributes) + : ConsentAttributes::namesOnly($attributes); + + return $this->_hashService->getStableConsentHash($consentAttributes); + } + private function _storeConsent(ServiceProvider $serviceProvider, ConsentType $consentType): bool + { $consentUuid = $this->_getConsentUid(); - if(! is_string($consentUuid)){ + if (!is_string($consentUuid)) { return false; } - $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, consent_type, consent_date, deleted_at) - VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') - ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), consent_type=VALUES(consent_type), consent_date=NOW()"; - $parameters = array( - sha1($consentUuid), - $serviceProvider->entityId, - $this->_getAttributesHash($this->_responseAttributes), - $consentType, + $parameters = new ConsentStoreParameters( + hashedUserId: sha1($consentUuid), + serviceId: $serviceProvider->entityId, + attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), + consentType: $consentType->value, + attributeHash: $this->_getAttributesHash($this->_responseAttributes), ); - $statement = $dbh->prepare($query); - if (!$statement) { - throw new EngineBlock_Exception( - "Unable to create a prepared statement to insert consent?!", - EngineBlock_Exception::CODE_CRITICAL - ); - } + return $this->_hashService->storeConsentHash($parameters); + } - assert($statement instanceof Statement); - try{ - foreach ($parameters as $index => $parameter){ - $statement->bindValue($index + 1, $parameter); - } - - $statement->executeStatement(); - }catch (\Doctrine\DBAL\Exception $e){ - throw new EngineBlock_Corto_Module_Services_Exception( - sprintf('Error storing consent: "%s"', var_export($e->getMessage(), true)), - EngineBlock_Exception::CODE_CRITICAL - ); + /** @deprecated Remove after stable consent hash is running in production */ + private function _updateConsent(ServiceProvider $serviceProvider, ConsentType $consentType): bool + { + $consentUid = $this->_getConsentUid(); + if (!is_string($consentUid)) { + return false; } - return true; + + $parameters = new ConsentUpdateParameters( + attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), + attributeHash: $this->_getAttributesHash($this->_responseAttributes), + hashedUserId: sha1($consentUid), + serviceId: $serviceProvider->entityId, + consentType: $consentType->value, + ); + + return $this->_hashService->updateConsentHash($parameters); } - private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType) + private function _hasStoredConsent(ServiceProvider $serviceProvider, ConsentType $consentType): ConsentVersion { - try { - $dbh = $this->_getConsentDatabaseConnection(); - if (!$dbh) { - return false; - } - - $attributesHash = $this->_getAttributesHash($this->_responseAttributes); - - $consentUuid = $this->_getConsentUid(); - if (!is_string($consentUuid)) { - return false; - } - - $query = " - SELECT * - FROM {$this->_tableName} - WHERE hashed_user_id = ? - AND service_id = ? - AND attribute = ? - AND consent_type = ? - AND deleted_at IS NULL - "; - $hashedUserId = sha1($consentUuid); - $parameters = array( - $hashedUserId, - $serviceProvider->entityId, - $attributesHash, - $consentType, - ); - - $statement = $dbh->prepare($query); - assert($statement instanceof Statement); - foreach ($parameters as $position => $parameter) { - $statement->bindValue($position + 1, $parameter); - } - $rows = $statement->executeQuery(); - - if ($rows->rowCount() < 1) { - // No stored consent found - return false; - } - - return true; - } catch (PDOException $e) { - throw new EngineBlock_Corto_ProxyServer_Exception( - sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage()), - EngineBlock_Exception::CODE_ALERT - ); + $consentUid = $this->_getConsentUid(); + if (!is_string($consentUid)) { + return ConsentVersion::notGiven(); } + + $query = new ConsentHashQuery( + hashedUserId: sha1($consentUid), + serviceId: $serviceProvider->entityId, + attributeHash: $this->_getAttributesHash($this->_responseAttributes), + attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), + consentType: $consentType->value, + ); + return $this->_hashService->retrieveConsentHash($query); } } diff --git a/library/EngineBlock/Corto/Model/Consent/Factory.php b/library/EngineBlock/Corto/Model/Consent/Factory.php index 80be173e81..834cfdfade 100644 --- a/library/EngineBlock/Corto/Model/Consent/Factory.php +++ b/library/EngineBlock/Corto/Model/Consent/Factory.php @@ -16,44 +16,30 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; + /** * @todo write a test */ class EngineBlock_Corto_Model_Consent_Factory { - /** @var EngineBlock_Corto_Filter_Command_Factory */ - private $_filterCommandFactory; - - /** @var EngineBlock_Database_ConnectionFactory */ - private $_databaseConnectionFactory; + private ConsentHashServiceInterface $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 - ) - { - $this->_filterCommandFactory = $filterCommandFactory; - $this->_databaseConnectionFactory = $databaseConnectionFactory; + ConsentHashServiceInterface $hashService + ) { + $this->hashService = $hashService; } /** - * Creates a new Consent instance - * - * @param EngineBlock_Corto_ProxyServer $proxyServer - * @param EngineBlock_Saml2_ResponseAnnotationDecorator $response * @param array $attributes - * @return EngineBlock_Corto_Model_Consent */ public function create( EngineBlock_Corto_ProxyServer $proxyServer, EngineBlock_Saml2_ResponseAnnotationDecorator $response, array $attributes - ) { + ): EngineBlock_Corto_Model_Consent { // If attribute manipulation was executed before consent, the NameId must be retrieved from the original response // object, in order to ensure correct 'hashed_user_id' generation. $featureConfiguration = EngineBlock_ApplicationSingleton::getInstance() @@ -64,13 +50,12 @@ public function create( $consentEnabled = $featureConfiguration->isEnabled('eb.feature_enable_consent'); return new EngineBlock_Corto_Model_Consent( - $proxyServer->getConfig('ConsentDbTable', 'consent'), $proxyServer->getConfig('ConsentStoreValues', true), $response, $attributes, - $this->_databaseConnectionFactory, $amPriorToConsent, - $consentEnabled + $consentEnabled, + $this->hashService ); } } diff --git a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php index 97cf0300b6..dd882adcc2 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; @@ -99,8 +100,11 @@ public function serve($serviceName, Request $httpRequest) $attributes = $response->getAssertion()->getAttributes(); $consentRepository = $this->_consentFactory->create($this->_server, $response, $attributes); - if (!$consentRepository->explicitConsentWasGivenFor($serviceProvider)) { + $explicitConsent = $consentRepository->explicitConsentWasGivenFor($serviceProvider); + if (!$explicitConsent->given()) { $consentRepository->giveExplicitConsentFor($destinationMetadata); + } else { + $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::Explicit, $explicitConsent); } $response->setConsent(Constants::CONSENT_OBTAINED); diff --git a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php index 767650ae83..902d988d9b 100644 --- a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php @@ -16,10 +16,11 @@ * 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; -use OpenConext\EngineBlock\Service\ConsentServiceInterface; +use OpenConext\EngineBlock\Service\Consent\ConsentServiceInterface; use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface; use OpenConext\EngineBlockBundle\Service\DiscoverySelectionService; use Psr\Log\LogLevel; @@ -146,8 +147,11 @@ public function serve($serviceName, Request $httpRequest) if ($this->isConsentDisabled($spMetadataChain, $identityProvider)) { - if (!$consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata)) { + $implicitConsent = $consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata); + if (!$implicitConsent->given()) { $consentRepository->giveImplicitConsentFor($serviceProviderMetadata); + } else { + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::Implicit, $implicitConsent); } $response->setConsent(Constants::CONSENT_INAPPLICABLE); @@ -165,7 +169,9 @@ public function serve($serviceName, Request $httpRequest) } $priorConsent = $consentRepository->explicitConsentWasGivenFor($serviceProviderMetadata); - if ($priorConsent) { + if ($priorConsent->given()) { + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::Explicit, $priorConsent); + $response->setConsent(Constants::CONSENT_PRIOR); $response->setDestination($response->getReturn()); diff --git a/migrations/DoctrineMigrations/Version20260210000000.php b/migrations/DoctrineMigrations/Version20260210000000.php index 3277a46df5..adc81a0044 100644 --- a/migrations/DoctrineMigrations/Version20260210000000.php +++ b/migrations/DoctrineMigrations/Version20260210000000.php @@ -53,7 +53,8 @@ public function up(Schema $schema): void `consent_date` datetime NOT NULL, `hashed_user_id` varchar(80) NOT NULL, `service_id` varchar(255) NOT NULL, - `attribute` varchar(80) NOT NULL, + `attribute` varchar(80) DEFAULT NULL, + `attribute_stable` varchar(80) DEFAULT NULL, `consent_type` varchar(20) DEFAULT \'explicit\', `deleted_at` datetime NOT NULL DEFAULT \'0000-00-00 00:00:00\', PRIMARY KEY (`hashed_user_id`,`service_id`,`deleted_at`), diff --git a/migrations/DoctrineMigrations/Version20260315000001.php b/migrations/DoctrineMigrations/Version20260315000001.php new file mode 100644 index 0000000000..4ee7a18648 --- /dev/null +++ b/migrations/DoctrineMigrations/Version20260315000001.php @@ -0,0 +1,71 @@ +connection->createSchemaManager()->listTableNames(); + $tableExists = in_array('consent', $tables, true); + + if (!$tableExists) { + $this->skipIf(true, 'Table consent does not exist yet (fresh install, baseline will create it). Skipping.'); + return; + } + + $columnExists = (bool) $this->connection->fetchOne( + "SELECT COUNT(*) FROM information_schema.COLUMNS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = 'consent' + AND COLUMN_NAME = 'attribute_stable'" + ); + $this->skipIf( + $columnExists, + 'Column attribute_stable already exists (fresh install via baseline). Skipping.' + ); + } + + public function up(Schema $schema): void + { + $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) DEFAULT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL'); + } + + public function down(Schema $schema): void + { + $this->addSql('UPDATE consent SET attribute = attribute_stable WHERE attribute IS NULL AND attribute_stable IS NOT NULL'); + $this->addSql('ALTER TABLE consent CHANGE attribute attribute VARCHAR(80) NOT NULL'); + $this->addSql('ALTER TABLE consent DROP attribute_stable'); + } +} diff --git a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php index 709ba9a7e7..38b4af94de 100644 --- a/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php +++ b/src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php @@ -19,6 +19,10 @@ namespace OpenConext\EngineBlock\Authentication\Repository; use OpenConext\EngineBlock\Authentication\Model\Consent; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; interface ConsentRepository { @@ -37,4 +41,22 @@ public function findAllFor($userId); public function deleteAllFor($userId); public function deleteOneFor(string $userId, string $serviceProviderEntityId): bool; + + /** + * Test if the consent row is set with an attribute hash either stable or unstable + */ + public function hasConsentHash(ConsentHashQuery $query): ConsentVersion; + + /** + * By default stores the stable consent hash. The legacy consent hash is left. + */ + public function storeConsentHash(ConsentStoreParameters $parameters): bool; + + /** + * When a deprecated unstable consent hash is encoutered, we upgrade it to the new format using this + * update consent hash method. + */ + public function updateConsentHash(ConsentUpdateParameters $parameters): bool; + + public function countTotalConsent(string $consentUid): int; } diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php new file mode 100644 index 0000000000..5921da0a1d --- /dev/null +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php @@ -0,0 +1,32 @@ +consentType = $consentType; - } - - /** - * @param ConsentType $other - * @return bool - */ - public function equals(ConsentType $other) - { - return $this->consentType === $other->consentType; - } - - /** - * @return string - */ - public function jsonSerialize(): mixed - { - return $this->consentType; - } - - /** - * @return string - */ - public function __toString() + public function jsonSerialize(): string { - return $this->consentType; + return $this->value; } } diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php new file mode 100644 index 0000000000..a0d08ca3b6 --- /dev/null +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php @@ -0,0 +1,34 @@ +consentVersion = $consentVersion; + } + + public function given(): bool + { + return $this->consentVersion !== self::NOT_GIVEN; + } + + /** + * @return string + */ + public function __toString() + { + return $this->consentVersion; + } + + /** @deprecated Remove after stable consent hash is running in production */ + public function isUnstable(): bool + { + return $this->consentVersion === self::UNSTABLE; + } + + public function isStable(): bool + { + return $this->consentVersion === self::STABLE; + } +} diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentAttributes.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentAttributes.php new file mode 100644 index 0000000000..347a6fe9e1 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentAttributes.php @@ -0,0 +1,165 @@ +compareValue = $compareValue; + } + + /** + * Build a ConsentAttributes where the compare value includes both attribute names and values. + * Use when consent_store_values=true. + */ + public static function withValues(array $raw): self + { + $normalized = self::normalize($raw); + return new self(serialize(self::sortRecursive(self::removeEmptyAttributes($normalized)))); + } + + /** + * Build a ConsentAttributes where the compare value includes attribute names only. + * Use when consent_store_values=false. + */ + public static function namesOnly(array $raw): self + { + $normalized = self::normalize($raw); + $sortedKeys = self::sortRecursive(array_keys(self::removeEmptyAttributes($normalized))); + return new self(implode('|', $sortedKeys)); + } + + public function getCompareValue(): string + { + return $this->compareValue; + } + + /** + * Applies all normalisation steps shared by both strategies. + */ + private static function normalize(array $raw): array + { + return self::caseNormalizeStringArray(self::nameIdNormalize($raw)); + } + + /** + * Converts NameID objects to a plain array so they survive serialisation and case-folding. + */ + private static function nameIdNormalize(array $attributes): array + { + array_walk_recursive($attributes, function (&$value) { + if ($value instanceof NameID) { + $value = ['value' => $value->getValue(), 'Format' => $value->getFormat()]; + } + }); + return $attributes; + } + + /** + * Lowercases all array keys and string values recursively using mb_strtolower + * to handle multi-byte UTF-8 characters (e.g. Ü→ü, Arabic, Chinese — common in SAML). + */ + private static function caseNormalizeStringArray(array $original): array + { + $result = []; + foreach ($original as $key => $value) { + $normalizedKey = is_string($key) ? mb_strtolower($key) : $key; + if (is_array($value)) { + $result[$normalizedKey] = self::caseNormalizeStringArray($value); + } elseif (is_string($value)) { + $result[$normalizedKey] = mb_strtolower($value); + } else { + $result[$normalizedKey] = $value; + } + } + return $result; + } + + /** + * Strips null, empty-string, and empty-array values recursively so that stray + * empty entries do not produce spurious re-consent, while a fully-empty attribute + * (key present but no values) is removed entirely and DOES trigger re-consent. + */ + private static function removeEmptyAttributes(array $array): array + { + foreach ($array as $key => $value) { + if ($value === null || $value === '' || $value === []) { + unset($array[$key]); + } elseif (is_array($value)) { + $array[$key] = self::removeEmptyAttributes($value); + } + } + return $array; + } + + /** + * Sorts arrays recursively: associative arrays by key (ksort), sequential arrays by value (sort). + * Re-indexes sequential arrays first to erase any sparse numeric keys. + */ + private static function sortRecursive(array $sortMe): array + { + $copy = $sortMe; + + if (self::isSequentialArray($copy)) { + $copy = array_values($copy); + sort($copy); + } else { + ksort($copy); + } + + foreach ($copy as $key => $value) { + if (is_array($value)) { + $copy[$key] = self::sortRecursive($value); + } + } + + return $copy; + } + + private static function isSequentialArray(array $array): bool + { + return count(array_filter(array_keys($array), 'is_string')) === 0; + } +} diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php new file mode 100644 index 0000000000..5b73cf7317 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -0,0 +1,123 @@ +consentRepository = $consentHashRepository; + $this->featureConfiguration = $featureConfiguration; + } + + public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion + { + return $this->consentRepository->hasConsentHash($query); + } + + public function storeConsentHash(ConsentStoreParameters $parameters): bool + { + $migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION); + + if ($migrationEnabled) { + $parameters = new ConsentStoreParameters( + hashedUserId: $parameters->hashedUserId, + serviceId: $parameters->serviceId, + attributeStableHash: $parameters->attributeStableHash, + consentType: $parameters->consentType, + attributeHash: null, + ); + } + + return $this->consentRepository->storeConsentHash($parameters); + } + + public function updateConsentHash(ConsentUpdateParameters $parameters): bool + { + $migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION); + + if ($migrationEnabled) { + $parameters = new ConsentUpdateParameters( + attributeStableHash: $parameters->attributeStableHash, + attributeHash: $parameters->attributeHash, + hashedUserId: $parameters->hashedUserId, + serviceId: $parameters->serviceId, + consentType: $parameters->consentType, + clearLegacyHash: true, + ); + } + + return $this->consentRepository->updateConsentHash($parameters); + } + + public function countTotalConsent(string $consentUid): int + { + return $this->consentRepository->countTotalConsent($consentUid); + } + + public function getStableConsentHash(ConsentAttributes $attributes): string + { + return sha1($attributes->getCompareValue()); + } + + /** + * @deprecated Remove after stable consent hash is running in production + * + * 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 + { + if ($mustStoreValues) { + ksort($attributes); + $hashBase = serialize($attributes); + } else { + $names = array_keys($attributes); + sort($names); + $hashBase = implode('|', $names); + } + return sha1($hashBase); + } +} diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php new file mode 100644 index 0000000000..bf93a82d02 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceInterface.php @@ -0,0 +1,43 @@ +consentRepository->findAllFor($userId); + return $this->consentRepository->countTotalConsent($userId); } catch (Exception $e) { throw new RuntimeException( - sprintf('An exception occurred while fetching consents the user has given ("%s")', $e->getMessage()), + sprintf('An exception occurred while counting consents the user has given ("%s")', $e->getMessage()), 0, $e ); } - - return count($consents); } public function deleteOneConsentFor(CollabPersonId $id, string $serviceProviderEntityId): bool diff --git a/src/OpenConext/EngineBlock/Service/ConsentServiceInterface.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php similarity index 95% rename from src/OpenConext/EngineBlock/Service/ConsentServiceInterface.php rename to src/OpenConext/EngineBlock/Service/Consent/ConsentServiceInterface.php index 4ebb10c832..32f800ec9a 100644 --- a/src/OpenConext/EngineBlock/Service/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/Entity/Consent.php b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php index 4398e85f97..d6f43378bc 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php @@ -56,9 +56,15 @@ class Consent /** * @var string */ - #[ORM\Column(type: Types::STRING, length: 80)] + #[ORM\Column(type: Types::STRING, length: 80, nullable: true)] public ?string $attribute = null; + /** + * @var string + */ + #[ORM\Column(name: 'attribute_stable', type: Types::STRING, length: 80, nullable: true)] + public ?string $attributeStable = null; + /** * @var string */ diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index 2d422981f4..8415bbead2 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -25,7 +25,11 @@ use Doctrine\Persistence\ManagerRegistry; use OpenConext\EngineBlock\Authentication\Model\Consent; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; +use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery; +use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Exception\RuntimeException; use Psr\Log\LoggerInterface; use function sha1; @@ -64,12 +68,13 @@ public function findAllFor($userId) { // deleted_at IS NULL matches active records whose deleted_at is '0000-00-00 00:00:00'. // See Consent::$deletedAt for full context. - $sql = ' + $sql = ' SELECT service_id , consent_date , consent_type , attribute + , attribute_stable FROM consent WHERE @@ -91,8 +96,8 @@ function (array $row) use ($userId) { $userId, $row['service_id'], new DateTime($row['consent_date']), - new ConsentType($row['consent_type']), - $row['attribute'] + ConsentType::from($row['consent_type']), + $row['attribute_stable'] ?? $row['attribute'] ); }, $rows @@ -138,7 +143,8 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b hashed_user_id = :hashed_user_id AND service_id = :service_id - AND deleted_at IS NULL + AND + deleted_at IS NULL '; try { $result = $this->connection->executeQuery( @@ -170,4 +176,177 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b ); } } + + /** + * @throws RuntimeException + */ + public function hasConsentHash(ConsentHashQuery $query): ConsentVersion + { + try { + $sql = " SELECT + attribute_stable + FROM + consent + WHERE + hashed_user_id = ? + AND + service_id = ? + AND + (attribute = ? OR attribute_stable = ?) + AND + consent_type = ? + AND + deleted_at IS NULL + "; + + $rows = $this->connection->executeQuery($sql, [ + $query->hashedUserId, + $query->serviceId, + $query->attributeHash, + $query->attributeStableHash, + $query->consentType, + ])->fetchAllAssociative(); + + if (count($rows) < 1) { + // No stored consent found + return ConsentVersion::notGiven(); + } + + if (!empty($rows[0]['attribute_stable'])) { + return ConsentVersion::stable(); + } + return ConsentVersion::unstable(); + } catch (Exception $e) { + throw new RuntimeException(sprintf('Consent retrieval failed! Error: "%s"', $e->getMessage())); + } + } + + /** + * @throws RuntimeException + */ + public function storeConsentHash(ConsentStoreParameters $parameters): bool + { + if ($parameters->attributeHash !== null) { + $query = "INSERT INTO consent (hashed_user_id, service_id, attribute, attribute_stable, consent_type, consent_date, deleted_at) + VALUES (?, ?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') + ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), attribute_stable=VALUES(attribute_stable), + consent_type=VALUES(consent_type), consent_date=NOW(), deleted_at='0000-00-00 00:00:00'"; + $bindings = [ + $parameters->hashedUserId, + $parameters->serviceId, + $parameters->attributeHash, + $parameters->attributeStableHash, + $parameters->consentType, + ]; + } else { + $query = "INSERT INTO consent (hashed_user_id, service_id, attribute_stable, consent_type, consent_date, deleted_at) + VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00') + ON DUPLICATE KEY UPDATE attribute_stable=VALUES(attribute_stable), + consent_type=VALUES(consent_type), consent_date=NOW(), deleted_at='0000-00-00 00:00:00'"; + $bindings = [ + $parameters->hashedUserId, + $parameters->serviceId, + $parameters->attributeStableHash, + $parameters->consentType, + ]; + } + + try { + $this->connection->executeStatement($query, $bindings); + } catch (Exception $e) { + throw new RuntimeException( + sprintf('Error storing consent: "%s"', $e->getMessage()) + ); + } + + return true; + } + + /** + * @throws RuntimeException + */ + public function updateConsentHash(ConsentUpdateParameters $parameters): bool + { + if ($parameters->clearLegacyHash) { + $query = " + UPDATE + consent + SET + attribute_stable = ?, + attribute = NULL + WHERE + attribute = ? + AND + hashed_user_id = ? + AND + service_id = ? + AND + consent_type = ? + AND + deleted_at IS NULL + "; + } else { + $query = " + UPDATE + consent + SET + attribute_stable = ? + WHERE + attribute = ? + AND + hashed_user_id = ? + AND + service_id = ? + AND + consent_type = ? + AND + deleted_at IS NULL + "; + } + + try { + $affected = $this->connection->executeStatement($query, [ + $parameters->attributeStableHash, + $parameters->attributeHash, + $parameters->hashedUserId, + $parameters->serviceId, + $parameters->consentType, + ]); + } catch (Exception $e) { + throw new RuntimeException( + sprintf('Error storing updated consent: "%s"', $e->getMessage()) + ); + } + + if ($affected === 0) { + $this->logger->warning( + sprintf( + 'Could not upgrade unstable consent hash for user "%s" and service "%s": no matching row found. ' . + 'The user\'s attributes may have changed since consent was given.', + $parameters->hashedUserId, + $parameters->serviceId + ) + ); + return false; + } + + return true; + } + + /** + * @throws RuntimeException + */ + public function countTotalConsent(string $consentUid): int + { + $query = "SELECT COUNT(*) FROM consent where hashed_user_id = ? AND deleted_at IS NULL"; + $parameters = [sha1($consentUid)]; + + try { + return (int) $this->connection->executeQuery($query, $parameters)->fetchOne(); + } catch (Exception $e) { + throw new RuntimeException( + sprintf('Error counting consent: "%s"', $e->getMessage()) + ); + } + } } diff --git a/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php b/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php index 4ef5e0d0a1..fa38378ea7 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/Api/ConsentController.php @@ -19,7 +19,7 @@ namespace OpenConext\EngineBlockBundle\Controller\Api; use OpenConext\EngineBlock\Exception\RuntimeException; -use OpenConext\EngineBlock\Service\ConsentServiceInterface; +use OpenConext\EngineBlock\Service\Consent\ConsentServiceInterface; use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; use OpenConext\EngineBlockBundle\Factory\CollabPersonIdFactory; use OpenConext\EngineBlockBundle\Http\Exception\ApiAccessDeniedHttpException; diff --git a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php index 399632ea5c..525d4d5b86 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConsentControllerTest.php @@ -527,6 +527,7 @@ private function addConsentFixture($userId, $serviceId, $attributeHash, $consent 'hashed_user_id' => ':user_id', 'service_id' => ':service_id', 'attribute' => ':attribute', + 'attribute_stable' => ':attribute_stable', 'consent_type' => ':consent_type', 'consent_date' => ':consent_date', 'deleted_at' => ':deleted_at', @@ -534,7 +535,8 @@ private function addConsentFixture($userId, $serviceId, $attributeHash, $consent ->setParameters([ 'user_id' => sha1($userId), 'service_id' => $serviceId, - 'attribute' => $attributeHash, + 'attribute' => '', + 'attribute_stable' => $attributeHash, 'consent_type' => $consentType, 'consent_date' => $consentDate, 'deleted_at' => $deletedAt, diff --git a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php index f2deb08f00..d9a046755d 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/DeprovisionControllerTest.php @@ -407,6 +407,7 @@ private function addConsentFixture($userId, $serviceId, $attributeHash, $consent 'hashed_user_id' => ':user_id', 'service_id' => ':service_id', 'attribute' => ':attribute', + 'attribute_stable' => ':attribute', 'consent_type' => ':consent_type', 'consent_date' => ':consent_date', 'deleted_at' => '"0000-00-00 00:00:00"', diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php new file mode 100644 index 0000000000..a627182563 --- /dev/null +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -0,0 +1,359 @@ +setDefaultMatcher(ConsentHashQuery::class, \Mockery\Matcher\IsEqual::class); + Mockery::getConfiguration()->setDefaultMatcher(ConsentStoreParameters::class, \Mockery\Matcher\IsEqual::class); + Mockery::getConfiguration()->setDefaultMatcher(ConsentUpdateParameters::class, \Mockery\Matcher\IsEqual::class); + + $this->response = Mockery::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class); + $this->consentRepository = Mockery::mock(ConsentRepository::class); + + $this->buildConsentAndService(migrationEnabled: true); + } + + /** + * Rebuilds $this->consentService and $this->consent with the given toggle state. + * Call this in tests that need a specific toggle setting different from setUp's default. + */ + private function buildConsentAndService(bool $migrationEnabled): void + { + $featureConfig = new FeatureConfiguration([ + 'eb.stable_consent_hash_migration' => $migrationEnabled, + ]); + $this->consentService = new ConsentHashService($this->consentRepository, $featureConfig); + $this->consent = new EngineBlock_Corto_Model_Consent( + 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(ConsentVersion::notGiven()); + switch ($consentType) { + case ConsentType::Explicit: + $this->assertFalse($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); + break; + case ConsentType::Implicit: + $this->assertFalse($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); + 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('hasConsentHash') + ->with(new ConsentHashQuery( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType->value, + )) + ->once() + ->andReturn(ConsentVersion::unstable()); + + switch ($consentType) { + case ConsentType::Explicit: + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); + break; + case ConsentType::Implicit: + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); + 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('hasConsentHash') + ->with(new ConsentHashQuery( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType->value, + )) + ->once() + ->andReturn(ConsentVersion::stable()); + + switch ($consentType) { + case ConsentType::Explicit: + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); + break; + case ConsentType::Implicit: + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); + break; + } + } + + /** + * Toggle ON (migration enabled): new consent stores only the stable hash. + * The legacy attribute column must be left NULL so fully-migrated deployments + * don't accumulate unnecessary data in the old column. + */ + #[DataProvider('consentTypeProvider')] + public function test_give_consent_toggle_on_stores_only_stable_hash($consentType) + { + // setUp already builds with migrationEnabled=true + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + $this->consentRepository + ->shouldReceive('storeConsentHash') + ->once() + ->with(new ConsentStoreParameters( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType->value, + attributeHash: null, + )) + ->andReturn(true); + + switch ($consentType) { + case ConsentType::Explicit: + $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); + break; + case ConsentType::Implicit: + $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); + break; + } + } + + /** + * Toggle OFF (migration disabled): new consent stores BOTH hashes so that + * old EB instances (still reading only the `attribute` column) can still + * find the consent record during a rolling deploy. + */ + #[DataProvider('consentTypeProvider')] + public function test_give_consent_toggle_off_stores_both_hashes($consentType) + { + $this->buildConsentAndService(migrationEnabled: false); + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + $this->consentRepository + ->shouldReceive('storeConsentHash') + ->once() + ->with(new ConsentStoreParameters( + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + consentType: $consentType->value, + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + )) + ->andReturn(true); + + switch ($consentType) { + case ConsentType::Explicit: + $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); + break; + case ConsentType::Implicit: + $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); + break; + } + } + + /** + * Toggle OFF (migration disabled): upgrading an old unstable consent leaves + * the legacy `attribute` column intact so old instances keep working. + */ + #[DataProvider('consentTypeProvider')] + public function test_upgrade_toggle_off_preserves_legacy_hash($consentType) + { + $this->buildConsentAndService(migrationEnabled: false); + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + $this->consentRepository + ->shouldReceive('updateConsentHash') + ->once() + ->with(new ConsentUpdateParameters( + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + consentType: $consentType->value, + clearLegacyHash: false, + )) + ->andReturn(true); + + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::unstable())); + } + + /** + * Toggle ON (migration enabled): upgrading an old unstable consent nulls the + * legacy `attribute` column so the old column is cleaned up over time. + */ + #[DataProvider('consentTypeProvider')] + public function test_upgrade_toggle_on_clears_legacy_hash($consentType) + { + // setUp already builds with migrationEnabled=true + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + $this->consentRepository + ->shouldReceive('updateConsentHash') + ->once() + ->with(new ConsentUpdateParameters( + attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', + hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', + serviceId: 'service-provider-entity-id', + consentType: $consentType->value, + clearLegacyHash: true, + )) + ->andReturn(true); + + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::unstable())); + } + + #[DataProvider('consentTypeProvider')] + public function test_upgrade_to_stable_consent_not_applied_when_stable($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + // No DB calls expected — stable consent does not trigger an update + $this->consentRepository->shouldNotReceive('hasConsentHash'); + $this->consentRepository->shouldNotReceive('storeConsentHash'); + $this->consentRepository->shouldNotReceive('updateConsentHash'); + + // Pass the pre-fetched ConsentVersion (stable) — no second DB query is made, no update triggered + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::stable())); + } + + #[DataProvider('consentTypeProvider')] + public function test_upgrade_not_applied_when_no_consent_given($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + // No DB calls expected — no consent means nothing to upgrade + $this->consentRepository->shouldNotReceive('hasConsentHash'); + $this->consentRepository->shouldNotReceive('updateConsentHash'); + + // Pass the pre-fetched ConsentVersion (notGiven) — no update should be triggered + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::notGiven())); + } + + #[DataProvider('consentTypeProvider')] + public function test_upgrade_continues_gracefully_when_attributes_changed($consentType) + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + $this->response->shouldReceive('getNameIdValue') + ->once() + ->andReturn('collab:person:id:org-a:joe-a'); + // But the UPDATE matches 0 rows (attributes changed since consent was given) + $this->consentRepository + ->shouldReceive('updateConsentHash') + ->once() + ->andReturn(false); + + // Must not throw; the warning is logged inside the repository + // Pass the pre-fetched ConsentVersion (unstable) — no second DB query is made + $this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::unstable())); + } + + public function test_store_consent_hash_sql_resets_deleted_at_on_duplicate(): void + { + // The storeConsentHash SQL must reset deleted_at='0000-00-00 00:00:00' in the + // ON DUPLICATE KEY UPDATE clause so soft-deleted rows become active again. + // We verify this by checking the SQL string directly. + new \ReflectionClass(DbalConsentRepository::class); + + // Read the SQL from the source to verify it contains the deleted_at reset + // This is a documentation test — if the SQL is refactored, update it here too. + $source = file_get_contents( + __DIR__ . '/../../../../../../src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php' + ); + $this->assertStringContainsString( + "deleted_at='0000-00-00 00:00:00'", + $source, + 'ON DUPLICATE KEY UPDATE must reset deleted_at so soft-deleted re-consent rows become active' + ); + } + + public static function consentTypeProvider(): iterable + { + yield [ConsentType::Implicit]; + yield [ConsentType::Explicit]; + } +} diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php index 47f34bf9ec..465114ed7f 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -16,60 +16,161 @@ * limitations under the License. */ +use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; +use OpenConext\EngineBlock\Authentication\Value\ConsentType; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; +use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface; use PHPUnit\Framework\TestCase; class EngineBlock_Corto_Model_ConsentTest extends TestCase { + use MockeryPHPUnitIntegration; + private $consentDisabled; private $consent; - private $mockedDatabaseConnection; + private $consentService; public function setUp(): void { - $this->mockedDatabaseConnection = Phake::mock('EngineBlock_Database_ConnectionFactory'); $mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($mockedResponse)->getNameIdValue()->thenReturn('urn:collab:person:example.org:user1'); + + $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( - "consent", true, $mockedResponse, [], - $this->mockedDatabaseConnection, false, - true + true, + $this->consentService ); } public function testConsentDisabledDoesNotWriteToDatabase() { $serviceProvider = new ServiceProvider("service-provider-entity-id"); - $this->consentDisabled->explicitConsentWasGivenFor($serviceProvider); - $this->consentDisabled->implicitConsentWasGivenFor($serviceProvider); - $this->consentDisabled->giveExplicitConsentFor($serviceProvider); - $this->consentDisabled->giveImplicitConsentFor($serviceProvider); - Phake::verify($this->mockedDatabaseConnection, Phake::times(0))->create(); + $this->consentService->shouldNotReceive('storeConsentHash'); + $this->consentService->shouldNotReceive('retrieveConsentHash'); + $this->consentService->shouldNotReceive('updateConsentHash'); + + $this->assertTrue($this->consentDisabled->explicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertTrue($this->consentDisabled->implicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertTrue($this->consentDisabled->giveExplicitConsentFor($serviceProvider)); + $this->assertTrue($this->consentDisabled->giveImplicitConsentFor($serviceProvider)); + } + + public function testUpgradeAttributeHashSkippedWhenConsentDisabled() + { + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + + $this->consentService->shouldNotReceive('retrieveConsentHash'); + $this->consentService->shouldNotReceive('updateConsentHash'); + + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::Explicit, ConsentVersion::stable()); + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::Implicit, ConsentVersion::stable()); } public function testConsentWriteToDatabase() { $serviceProvider = new ServiceProvider("service-provider-entity-id"); - $this->consent->explicitConsentWasGivenFor($serviceProvider); - $this->consent->implicitConsentWasGivenFor($serviceProvider); - $this->consent->giveExplicitConsentFor($serviceProvider); - $this->consent->giveImplicitConsentFor($serviceProvider); - Phake::verify($this->mockedDatabaseConnection, Phake::times(4))->create(); + $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); + $this->consentService->shouldReceive('getStableConsentHash')->andReturn(sha1('stable')); + $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(ConsentVersion::stable()); + $this->consentService->shouldReceive('storeConsentHash')->andReturn(true); + + $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); + $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); + } + + public function testCountTotalConsent() + { + // Arrange + $this->consentService->shouldReceive('countTotalConsent') + ->with('urn:collab:person:example.org:user1') + ->once() + ->andReturn(5); + + // Act + Assert + $this->assertEquals(5, $this->consent->countTotalConsent()); + } + + public function testConsentUidFromAmPriorToConsentEnabled() + { + // When amPriorToConsentEnabled is true the consent UID must come from + // getOriginalResponse()->getCollabPersonId(), NOT from getNameIdValue(). + $originalResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($originalResponse)->getCollabPersonId()->thenReturn('urn:collab:person:example.org:user-am'); + + $mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($mockedResponse)->getOriginalResponse()->thenReturn($originalResponse); + + $consentWithAmPrior = new EngineBlock_Corto_Model_Consent( + true, + $mockedResponse, + [], + true, // amPriorToConsentEnabled = true + true, + $this->consentService + ); + + $serviceProvider = new ServiceProvider('service-provider-entity-id'); + + $this->consentService->shouldReceive('getUnstableAttributesHash')->andReturn(sha1('unstable')); + $this->consentService->shouldReceive('getStableConsentHash')->andReturn(sha1('stable')); + $this->consentService->shouldReceive('retrieveConsentHash')->andReturn(ConsentVersion::stable()); + + // Act: trigger a code path that calls _getConsentUid() + $result = $consentWithAmPrior->explicitConsentWasGivenFor($serviceProvider); + + // Assert: consent check succeeded and the AM-prior uid path was used + $this->assertTrue($result->given()); + Phake::verify($originalResponse)->getCollabPersonId(); + Phake::verify($mockedResponse, Phake::never())->getNameIdValue(); + } + + public function testNullNameIdReturnsNoConsentWithoutCallingRepository() + { + $mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator'); + Phake::when($mockedResponse)->getNameIdValue()->thenReturn(null); + + $consentWithNullUid = new EngineBlock_Corto_Model_Consent( + true, + $mockedResponse, + [], + false, + true, + $this->consentService + ); + + $serviceProvider = new ServiceProvider("service-provider-entity-id"); + + // No DB calls should occur when the NameID is null + $this->consentService->shouldNotReceive('retrieveConsentHash'); + $this->consentService->shouldNotReceive('storeConsentHash'); + $this->consentService->shouldNotReceive('updateConsentHash'); + + // _hasStoredConsent returns notGiven() when UID is null -> consent methods return false + $this->assertFalse($consentWithNullUid->explicitConsentWasGivenFor($serviceProvider)->given()); + $this->assertFalse($consentWithNullUid->implicitConsentWasGivenFor($serviceProvider)->given()); + // giveConsent returns false when UID is null (_storeConsent returns early) + $this->assertFalse($consentWithNullUid->giveExplicitConsentFor($serviceProvider)); + $this->assertFalse($consentWithNullUid->giveImplicitConsentFor($serviceProvider)); + // upgradeAttributeHashFor should not throw when UID is null + $consentWithNullUid->upgradeAttributeHashFor($serviceProvider, ConsentType::Explicit, ConsentVersion::notGiven()); } } diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php index 18095329ff..190b7dff1f 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/ProcessConsentTest.php @@ -17,6 +17,7 @@ */ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\MetadataRepository\InMemoryMetadataRepository; use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface; @@ -148,6 +149,7 @@ public function testConsentIsStored() } public function testResponseIsSent() { + $this->mockConsent(); $processConsentService = $this->factoryService(); Phake::when($this->proxyServerMock) @@ -221,7 +223,7 @@ private function mockConsent() $consentMock = Phake::mock('EngineBlock_Corto_Model_Consent'); Phake::when($consentMock) ->explicitConsentWasGivenFor(Phake::anyParameters()) - ->thenReturn(false); + ->thenReturn(ConsentVersion::notGiven()); Phake::when($this->consentFactoryMock) ->create(Phake::anyParameters()) ->thenReturn($consentMock); diff --git a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php index 10e78a8d7e..77bfb3f63c 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/Service/ProvideConsentTest.php @@ -17,13 +17,14 @@ */ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; +use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; use OpenConext\EngineBlock\Metadata\Coins; use OpenConext\EngineBlock\Metadata\ConsentSettings; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; 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; @@ -123,7 +124,7 @@ public function testConsentIsSkippedWhenPriorConsentIsStored() Phake::when($this->consentMock) ->explicitConsentWasGivenFor(Phake::anyParameters()) - ->thenReturn(true); + ->thenReturn(ConsentVersion::stable()); $provideConsentService->serve(null, $this->httpRequestMock); @@ -279,7 +280,10 @@ private function mockConsent() $consentMock = Phake::mock('EngineBlock_Corto_Model_Consent'); Phake::when($consentMock) ->explicitConsentWasGivenFor(Phake::anyParameters()) - ->thenReturn(false); + ->thenReturn(ConsentVersion::notGiven()); + Phake::when($consentMock) + ->implicitConsentWasGivenFor(Phake::anyParameters()) + ->thenReturn(ConsentVersion::notGiven()); Phake::when($this->consentFactoryMock) ->create(Phake::anyParameters()) ->thenReturn($consentMock); diff --git a/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php b/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php index 3a4c614bde..05e70f620b 100644 --- a/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php +++ b/tests/library/EngineBlock/Test/Saml2/NameIdResolverMock.php @@ -21,17 +21,9 @@ class EngineBlock_Test_Saml2_NameIdResolverMock extends EngineBlock_Saml2_NameId private $_serviceProviderUuids = array(); private $_persistentIds = array(); - protected function _getUserDirectory() + protected function _getUserUuid($collabPersonId) { - $mock = new EngineBlock_Test_UserDirectoryMock(); - $mock->setUser( - 'urn:collab:person:example.edu:mock1', - array( - 'collabpersonid' => 'urn:collab:person:example.edu:mock1', - 'collabpersonuuid' => '', - ) - ); - return $mock; + return sha1($collabPersonId); } protected function _fetchPersistentId($serviceProviderUuid, $userUuid) diff --git a/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php b/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php index 228d62336b..fa8760c3ee 100644 --- a/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php +++ b/tests/library/EngineBlock/Test/Saml2/NameIdResolverTest.php @@ -162,8 +162,6 @@ public function testMetadataOverAuthnRequest() public function testPersistent(): void { - $this->markTestSkipped('Fails when switching to other backend, test should not rely on having fixed backend'); - // Input $nameIdFormat = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'; $this->serviceProvider->nameIdFormat = $nameIdFormat; diff --git a/tests/phpunit.xml b/tests/phpunit.xml index cca070da44..4e1bcf3fbe 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -17,35 +17,37 @@ failOnRisky="true" > - - unit - - - integration - - - library - - - functional - - - - - - - - - - - src - library - - - - - - - - + + unit + + + integration + + + library + + + functional + + + + + + + + + + + + src + library + + + + + + + + + diff --git a/tests/unit/OpenConext/EngineBlock/Authentication/Dto/ConsentTest.php b/tests/unit/OpenConext/EngineBlock/Authentication/Dto/ConsentTest.php index 54472e3fa6..92d32cebf7 100644 --- a/tests/unit/OpenConext/EngineBlock/Authentication/Dto/ConsentTest.php +++ b/tests/unit/OpenConext/EngineBlock/Authentication/Dto/ConsentTest.php @@ -91,7 +91,7 @@ public function all_values_are_serialized_to_json() { $serviceProvider = $this->createServiceProvider(); $consentGivenOn = new DateTime('20080624 10:00:00'); - $consentType = ConsentType::explicit(); + $consentType = ConsentType::Explicit; $consent = new Consent( new ConsentModel( @@ -135,7 +135,7 @@ public function test_display_name_of_organizations_works_as_intended( ) { $serviceProvider = $this->createServiceProvider($organizations); $consentGivenOn = new DateTime('20080624 10:00:00'); - $consentType = ConsentType::explicit(); + $consentType = ConsentType::Explicit; $consent = new Consent( new ConsentModel( @@ -165,7 +165,7 @@ public function display_name_falls_back_to_name_if_display_name_is_empty() $serviceProvider->nameNl = 'Name NL'; $consentGivenOn = new DateTime(); - $consentType = ConsentType::explicit(); + $consentType = ConsentType::Explicit; $consent = new Consent( new ConsentModel( @@ -196,7 +196,7 @@ public function display_name_falls_back_to_entity_id_if_name_is_empty() $serviceProvider->nameNl = ''; $consentGivenOn = new DateTime(); - $consentType = ConsentType::explicit(); + $consentType = ConsentType::Explicit; $consent = new Consent( new ConsentModel( diff --git a/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentTypeTest.php b/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentTypeTest.php deleted file mode 100644 index 2dc9d894be..0000000000 --- a/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentTypeTest.php +++ /dev/null @@ -1,73 +0,0 @@ -expectException(InvalidArgumentException::class); - - new ConsentType($invalid); - } - - #[Group('EngineBlock')] - #[Group('Authentication')] - #[Group('Value')] - #[Test] - public function different_consent_types_are_not_equal() - { - $explicit = ConsentType::explicit(); - $implicit = ConsentType::implicit(); - - $this->assertFalse($explicit->equals($implicit)); - $this->assertFalse($implicit->equals($explicit)); - } - - #[Group('EngineBlock')] - #[Group('Authentication')] - #[Group('Value')] - #[Test] - public function same_type_of_consent_types_are_equal() - { - $explicit = ConsentType::explicit(); - $implicit = ConsentType::implicit(); - - $this->assertTrue($explicit->equals(ConsentType::explicit())); - $this->assertTrue($implicit->equals(ConsentType::implicit())); - } -} diff --git a/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php b/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php new file mode 100644 index 0000000000..7c6f26e1c1 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php @@ -0,0 +1,62 @@ +assertTrue($version->given()); + $this->assertTrue($version->isStable()); + $this->assertFalse($version->isUnstable()); + $this->assertSame('stable', (string) $version); + } + + public function testUnstableIsGiven(): void + { + $version = ConsentVersion::unstable(); + + $this->assertTrue($version->given()); + $this->assertFalse($version->isStable()); + $this->assertTrue($version->isUnstable()); + $this->assertSame('unstable', (string) $version); + } + + public function testNotGivenIsNotGiven(): void + { + $version = ConsentVersion::notGiven(); + + $this->assertFalse($version->given()); + $this->assertFalse($version->isStable()); + $this->assertFalse($version->isUnstable()); + $this->assertSame('not-given', (string) $version); + } + + public function testInvalidVersionThrows(): void + { + $this->expectException(InvalidArgumentException::class); + new ConsentVersion('invalid'); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentAttributesTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentAttributesTest.php new file mode 100644 index 0000000000..625e2592ec --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentAttributesTest.php @@ -0,0 +1,473 @@ + ['John Doe'], + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + ]; + + $this->assertNotSame( + ConsentAttributes::withValues($raw)->getCompareValue(), + ConsentAttributes::namesOnly($raw)->getCompareValue(), + 'withValues and namesOnly must produce different compare values for the same non-empty input' + ); + } + + // ------------------------------------------------------------------------- + // Order invariance — withValues + // ------------------------------------------------------------------------- + + public function test_with_values_is_order_invariant_for_attribute_keys(): void + { + $alphabetical = [ + 'urn:attribute:a' => ['value1'], + 'urn:attribute:b' => ['value2'], + 'urn:attribute:c' => ['value3'], + ]; + $reversed = [ + 'urn:attribute:c' => ['value3'], + 'urn:attribute:b' => ['value2'], + 'urn:attribute:a' => ['value1'], + ]; + + $this->assertSame( + ConsentAttributes::withValues($alphabetical)->getCompareValue(), + ConsentAttributes::withValues($reversed)->getCompareValue() + ); + } + + public function test_with_values_is_order_invariant_for_attribute_values(): void + { + $forward = ['urn:attribute:a' => ['alice', 'bob', 'charlie']]; + $reversed = ['urn:attribute:a' => ['charlie', 'bob', 'alice']]; + + $this->assertSame( + ConsentAttributes::withValues($forward)->getCompareValue(), + ConsentAttributes::withValues($reversed)->getCompareValue() + ); + } + + // ------------------------------------------------------------------------- + // Order invariance — namesOnly + // ------------------------------------------------------------------------- + + public function test_names_only_is_order_invariant_for_attribute_keys(): void + { + $alphabetical = [ + 'urn:attribute:a' => ['value1'], + 'urn:attribute:b' => ['value2'], + 'urn:attribute:c' => ['value3'], + ]; + $reversed = [ + 'urn:attribute:c' => ['value3'], + 'urn:attribute:b' => ['value2'], + 'urn:attribute:a' => ['value1'], + ]; + + $this->assertSame( + ConsentAttributes::namesOnly($alphabetical)->getCompareValue(), + ConsentAttributes::namesOnly($reversed)->getCompareValue() + ); + } + + // ------------------------------------------------------------------------- + // Case normalisation — withValues + // ------------------------------------------------------------------------- + + public function test_with_values_normalises_key_casing(): void + { + $lower = ['urn:mace:dir:attribute-def:displayname' => ['John Doe']]; + $upper = ['URN:MACE:DIR:ATTRIBUTE-DEF:DISPLAYNAME' => ['John Doe']]; + + $this->assertSame( + ConsentAttributes::withValues($lower)->getCompareValue(), + ConsentAttributes::withValues($upper)->getCompareValue() + ); + } + + public function test_with_values_normalises_value_casing(): void + { + $lower = ['urn:mace:dir:attribute-def:sn' => ['doe']]; + $upper = ['urn:mace:dir:attribute-def:sn' => ['DOE']]; + + $this->assertSame( + ConsentAttributes::withValues($lower)->getCompareValue(), + ConsentAttributes::withValues($upper)->getCompareValue() + ); + } + + public function test_with_values_normalises_multibyte_value_casing(): void + { + $lower = ['urn:mace:dir:attribute-def:sn' => ['müller']]; + $upper = ['urn:mace:dir:attribute-def:sn' => ['Müller']]; + + $this->assertSame( + ConsentAttributes::withValues($lower)->getCompareValue(), + ConsentAttributes::withValues($upper)->getCompareValue(), + 'Case normalisation must handle multi-byte UTF-8 characters' + ); + } + + // ------------------------------------------------------------------------- + // Case normalisation — namesOnly + // ------------------------------------------------------------------------- + + public function test_names_only_normalises_key_casing(): void + { + $lower = ['urn:mace:dir:attribute-def:displayname' => ['John Doe']]; + $upper = ['URN:MACE:DIR:ATTRIBUTE-DEF:DISPLAYNAME' => ['John Doe']]; + + $this->assertSame( + ConsentAttributes::namesOnly($lower)->getCompareValue(), + ConsentAttributes::namesOnly($upper)->getCompareValue() + ); + } + + // ------------------------------------------------------------------------- + // Empty attribute stripping — withValues + // ------------------------------------------------------------------------- + + public function test_with_values_strips_empty_array_attribute(): void + { + $withEmpty = ['urn:attr:a' => ['value'], 'urn:attr:b' => []]; + $withoutEmpty = ['urn:attr:a' => ['value']]; + + $this->assertSame( + ConsentAttributes::withValues($withEmpty)->getCompareValue(), + ConsentAttributes::withValues($withoutEmpty)->getCompareValue() + ); + } + + public function test_with_values_losing_an_attribute_changes_compare_value(): void + { + $withValue = ['urn:mace:dir:attribute-def:displayName' => ['John Doe']]; + $withEmpty = ['urn:mace:dir:attribute-def:displayName' => []]; + + $this->assertNotSame( + ConsentAttributes::withValues($withValue)->getCompareValue(), + ConsentAttributes::withValues($withEmpty)->getCompareValue(), + 'An attribute going from a value to empty must change the compare value' + ); + } + + public function test_with_values_strips_stray_empty_sub_value(): void + { + $withStray = ['urn:mace:dir:attribute-def:displayName' => ['John Doe', '']]; + $withoutStray = ['urn:mace:dir:attribute-def:displayName' => ['John Doe']]; + + $this->assertSame( + ConsentAttributes::withValues($withStray)->getCompareValue(), + ConsentAttributes::withValues($withoutStray)->getCompareValue(), + 'Stray empty sub-values must be stripped and not affect the compare value' + ); + } + + // ------------------------------------------------------------------------- + // Empty attribute stripping — namesOnly + // ------------------------------------------------------------------------- + + public function test_names_only_strips_empty_array_attribute(): void + { + $withEmpty = ['urn:attr:a' => ['value'], 'urn:attr:b' => []]; + $withoutEmpty = ['urn:attr:a' => ['value']]; + + $this->assertSame( + ConsentAttributes::namesOnly($withEmpty)->getCompareValue(), + ConsentAttributes::namesOnly($withoutEmpty)->getCompareValue() + ); + } + + public function test_names_only_losing_an_attribute_changes_compare_value(): void + { + $withValue = ['urn:mace:dir:attribute-def:displayName' => ['John Doe']]; + $withEmpty = ['urn:mace:dir:attribute-def:displayName' => []]; + + $this->assertNotSame( + ConsentAttributes::namesOnly($withValue)->getCompareValue(), + ConsentAttributes::namesOnly($withEmpty)->getCompareValue(), + 'An attribute going from a value to empty must change the compare value in namesOnly mode' + ); + } + + // ------------------------------------------------------------------------- + // Zero values must NOT be stripped — withValues + // ------------------------------------------------------------------------- + + public function test_with_values_zero_string_not_stripped(): void + { + $withZero = ['urn:attr:count' => '0']; + $withoutAttr = []; + + $this->assertNotSame( + ConsentAttributes::withValues($withZero)->getCompareValue(), + ConsentAttributes::withValues($withoutAttr)->getCompareValue() + ); + } + + public function test_with_values_zero_int_not_stripped(): void + { + $withZero = ['urn:attr:count' => 0]; + $withoutAttr = []; + + $this->assertNotSame( + ConsentAttributes::withValues($withZero)->getCompareValue(), + ConsentAttributes::withValues($withoutAttr)->getCompareValue() + ); + } + + public function test_with_values_zero_float_not_stripped(): void + { + $withZero = ['urn:attr:count' => 0.0]; + $withoutAttr = []; + + $this->assertNotSame( + ConsentAttributes::withValues($withZero)->getCompareValue(), + ConsentAttributes::withValues($withoutAttr)->getCompareValue() + ); + } + + // ------------------------------------------------------------------------- + // NameID handling — withValues + // ------------------------------------------------------------------------- + + public function test_with_values_handles_nameid_objects(): void + { + $nameId = new NameID(); + $nameId->setValue('83aa0a79363edcf872c966b0d6eaf3f5e26a6a77'); + $nameId->setFormat('urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'); + + $attributes = [ + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:eduPersonTargetedID' => [$nameId], + ]; + + // Must not throw and must return a non-empty string + $compareValue = ConsentAttributes::withValues($attributes)->getCompareValue(); + $this->assertNotEmpty($compareValue); + } + + // ------------------------------------------------------------------------- + // Non-mutation + // ------------------------------------------------------------------------- + + public function test_with_values_does_not_mutate_input(): void + { + $raw = [ + 'urn:mace:dir:attribute-def:DisplayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:empty' => [], + 'urn:mace:dir:attribute-def:isMemberOf' => [2 => 'urn:collab:org:vm.openconext.ORG', 0 => 'urn:collab:org:vm.openconext.org'], + ]; + $snapshot = $raw; + + ConsentAttributes::withValues($raw); + + $this->assertSame($snapshot, $raw, 'withValues must not mutate the input array'); + } + + public function test_names_only_does_not_mutate_input(): void + { + $raw = [ + 'urn:mace:dir:attribute-def:DisplayName' => ['John Doe'], + 'urn:mace:dir:attribute-def:empty' => [], + ]; + $snapshot = $raw; + + ConsentAttributes::namesOnly($raw); + + $this->assertSame($snapshot, $raw, 'namesOnly must not mutate the input array'); + } + + // ------------------------------------------------------------------------- + // Sparse index normalisation + // ------------------------------------------------------------------------- + + public function test_with_values_sparse_sequential_indexes_are_normalised(): void + { + $dense = ['urn:attr:a' => [0 => 'aap', 1 => 'noot']]; + $sparse = ['urn:attr:a' => [0 => 'aap', 2 => 'noot']]; + + $this->assertSame( + ConsentAttributes::withValues($dense)->getCompareValue(), + ConsentAttributes::withValues($sparse)->getCompareValue() + ); + } + + // ------------------------------------------------------------------------- + // namesOnly — values are irrelevant + // ------------------------------------------------------------------------- + + public function test_names_only_values_are_irrelevant(): void + { + // namesOnly hashes attribute names only; different values for the same + // keys must produce the same compare value. + $original = ['urn:attr:a' => ['Alice'], 'urn:attr:b' => ['Bob']]; + $different = ['urn:attr:a' => ['Charlie'], 'urn:attr:b' => ['Dave']]; + + $this->assertSame( + ConsentAttributes::namesOnly($original)->getCompareValue(), + ConsentAttributes::namesOnly($different)->getCompareValue() + ); + } + + public function test_names_only_value_order_is_irrelevant(): void + { + // Reordering values within an attribute must not change the namesOnly compare value. + $forward = ['urn:attr:a' => ['alice', 'bob', 'charlie']]; + $reversed = ['urn:attr:a' => ['charlie', 'bob', 'alice']]; + + $this->assertSame( + ConsentAttributes::namesOnly($forward)->getCompareValue(), + ConsentAttributes::namesOnly($reversed)->getCompareValue() + ); + } + + // ------------------------------------------------------------------------- + // namesOnly — empty stripping and zero values + // ------------------------------------------------------------------------- + + public function test_names_only_strips_stray_empty_sub_value(): void + { + // A stray empty string inside an attribute's values must be stripped. + // The attribute key survives (it still has a real value), so no re-consent. + $withStray = ['urn:attr:a' => ['real-value', '']]; + $withoutStray = ['urn:attr:a' => ['real-value']]; + + $this->assertSame( + ConsentAttributes::namesOnly($withStray)->getCompareValue(), + ConsentAttributes::namesOnly($withoutStray)->getCompareValue(), + 'Stray empty sub-values must be stripped in namesOnly mode without affecting the compare value' + ); + } + + public function test_names_only_zero_string_keeps_the_attribute_key(): void + { + // '0' is not empty; an attribute with value '0' must keep its key and + // therefore produce a different compare value than an empty attribute set. + $withZero = ['urn:attr:count' => '0']; + $withoutAttr = []; + + $this->assertNotSame( + ConsentAttributes::namesOnly($withZero)->getCompareValue(), + ConsentAttributes::namesOnly($withoutAttr)->getCompareValue() + ); + } + + // ------------------------------------------------------------------------- + // Multibyte key case normalisation + // ------------------------------------------------------------------------- + + public function test_with_values_multibyte_key_case_normalised(): void + { + // mb_strtolower is applied to keys as well as values; a key that differs + // only in multibyte casing must produce the same compare value. + $lower = ['ärzte' => ['value']]; + $upper = ['Ärzte' => ['value']]; + + $this->assertSame( + ConsentAttributes::withValues($lower)->getCompareValue(), + ConsentAttributes::withValues($upper)->getCompareValue(), + 'Multibyte attribute key casing must be normalised in withValues mode' + ); + } + + public function test_names_only_multibyte_key_case_normalised(): void + { + $lower = ['ärzte' => ['value']]; + $upper = ['Ärzte' => ['value']]; + + $this->assertSame( + ConsentAttributes::namesOnly($lower)->getCompareValue(), + ConsentAttributes::namesOnly($upper)->getCompareValue(), + 'Multibyte attribute key casing must be normalised in namesOnly mode' + ); + } + + // ------------------------------------------------------------------------- + // NameID handling — namesOnly + // ------------------------------------------------------------------------- + + public function test_names_only_handles_nameid_objects(): void + { + // Under namesOnly the NameID object is still normalised (to avoid errors + // in the normalise pipeline), but only the outer attribute key is used. + $nameId = new NameID(); + $nameId->setValue('83aa0a79363edcf872c966b0d6eaf3f5e26a6a77'); + $nameId->setFormat('urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'); + + $attributes = [ + 'urn:mace:dir:attribute-def:uid' => ['joe-f12'], + 'urn:mace:dir:attribute-def:eduPersonTargetedID' => [$nameId], + ]; + + $compareValue = ConsentAttributes::namesOnly($attributes)->getCompareValue(); + $this->assertNotEmpty($compareValue); + } + + // ------------------------------------------------------------------------- + // Empty input + // ------------------------------------------------------------------------- + + public function test_with_values_empty_input_is_stable(): void + { + $first = ConsentAttributes::withValues([])->getCompareValue(); + $second = ConsentAttributes::withValues([])->getCompareValue(); + + $this->assertIsString($first); + $this->assertSame($first, $second, 'withValues([]) must be idempotent'); + } + + public function test_names_only_empty_input_is_stable(): void + { + $first = ConsentAttributes::namesOnly([])->getCompareValue(); + $second = ConsentAttributes::namesOnly([])->getCompareValue(); + + $this->assertIsString($first); + $this->assertSame($first, $second, 'namesOnly([]) must be idempotent'); + } + + // ------------------------------------------------------------------------- + // Null attribute value stripped + // ------------------------------------------------------------------------- + + public function test_with_values_null_attribute_value_is_stripped(): void + { + // A null value is treated as absent; the attribute must be removed entirely, + // producing the same compare value as an empty input. + $withNull = ['urn:attr:a' => null]; + $withoutAttr = []; + + $this->assertSame( + ConsentAttributes::withValues($withNull)->getCompareValue(), + ConsentAttributes::withValues($withoutAttr)->getCompareValue(), + 'A null attribute value must be stripped, equivalent to the attribute being absent' + ); + } +} 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..1580d5ddee --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -0,0 +1,85 @@ + false]); + $this->chs = new ConsentHashService($mockConsentHashRepository, $featureConfig); + } + + // ------------------------------------------------------------------------- + // Unstable hash algorithm — getUnstableAttributesHash + // ------------------------------------------------------------------------- + + public function test_unstable_attribute_hash_mustStoreValues_false_uses_keys_only() + { + // When mustStoreValues=false the hash is based on attribute names only. + // Two arrays with the same keys but different values must yield the same hash. + $attributes = ['urn:attr:a' => ['Alice'], 'urn:attr:b' => ['Bob']]; + $differentValues = ['urn:attr:a' => ['Charlie'], 'urn:attr:b' => ['Dave']]; + + $this->assertEquals( + $this->chs->getUnstableAttributesHash($attributes, false), + $this->chs->getUnstableAttributesHash($differentValues, false) + ); + } + + public function test_unstable_attribute_hash_mustStoreValues_true_includes_values() + { + // When mustStoreValues=true, attribute values are part of the hash. + // Two arrays with the same keys but different values must yield a different hash. + $attributes = ['urn:attr:a' => ['Alice'], 'urn:attr:b' => ['Bob']]; + $differentValues = ['urn:attr:a' => ['Charlie'], 'urn:attr:b' => ['Dave']]; + + $this->assertNotEquals( + $this->chs->getUnstableAttributesHash($attributes, true), + $this->chs->getUnstableAttributesHash($differentValues, true) + ); + } + + public function test_unstable_attribute_hash_key_order_normalized_in_names_only_mode() + { + // When mustStoreValues=false the implementation sorts attribute names, + // so reversed key order must produce the same hash. + $attributes = ['urn:attr:a' => ['Alice'], 'urn:attr:b' => ['Bob']]; + $reversed = ['urn:attr:b' => ['Bob'], 'urn:attr:a' => ['Alice']]; + + $this->assertEquals( + $this->chs->getUnstableAttributesHash($attributes, false), + $this->chs->getUnstableAttributesHash($reversed, false) + ); + } +} diff --git a/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php index b68d0beb77..5318eccfc0 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/ConsentServiceTest.php @@ -24,6 +24,7 @@ use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; use OpenConext\EngineBlock\Authentication\Value\CollabPersonId; use OpenConext\EngineBlock\Authentication\Value\CollabPersonUuid; +use OpenConext\EngineBlock\Service\Consent\ConsentService; use PHPUnit\Framework\Attributes\DoesNotPerformAssertions; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\Attributes\Test;