diff --git a/CHANGELOG.md b/CHANGELOG.md index b1a78d749..2eb48f344 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,9 +29,7 @@ Changes: * 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. + * This feature automatically migrates from the old hashes to the new hashes upon login. ## 7.1.0 [SBS](https://github.com/SURFscz/SBS) integration diff --git a/config/packages/engineblock_features.yaml b/config/packages/engineblock_features.yaml index b1a3d003d..8e7ed423c 100644 --- a/config/packages/engineblock_features.yaml +++ b/config/packages/engineblock_features.yaml @@ -16,4 +16,3 @@ 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 d533f16c1..0dc1693cf 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -227,7 +227,6 @@ parameters: feature_stepup_sfo_override_engine_entityid: false feature_stepup_send_user_attributes: false feature_enable_sram_interrupt: false - feature_stable_consent_hash_migration: false ########################################################################################## ## PROFILE SETTINGS diff --git a/config/services/services.yml b/config/services/services.yml index 10a8a326f..2af8da7cb 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -79,7 +79,6 @@ services: public: false arguments: - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository' - - '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration' OpenConext\EngineBlock\Service\Consent\ConsentService: arguments: diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php index db63bbab9..ea78137e0 100644 --- a/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php @@ -26,7 +26,7 @@ public function __construct( public readonly string $attributeStableHash, public readonly string $consentType, /** @deprecated Remove after stable consent hash is running in production */ - public readonly ?string $attributeHash = null, + public readonly string $attributeHash, ) { } } diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php index a0d08ca3b..ad657127c 100644 --- a/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php @@ -27,8 +27,6 @@ public function __construct( public readonly string $hashedUserId, public readonly string $serviceId, public readonly string $consentType, - /** @deprecated Remove after stable consent hash is running in production */ - public readonly bool $clearLegacyHash = false, ) { } } diff --git a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php index 5b73cf731..ffc84b5bf 100644 --- a/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php +++ b/src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php @@ -23,7 +23,6 @@ use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters; use OpenConext\EngineBlock\Authentication\Value\ConsentVersion; -use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; use function array_keys; use function implode; use function ksort; @@ -32,23 +31,14 @@ final class ConsentHashService implements ConsentHashServiceInterface { - /** @deprecated Remove after stable consent hash is running in production */ - private const FEATURE_MIGRATION = 'eb.stable_consent_hash_migration'; - /** * @var ConsentRepository */ private $consentRepository; - /** - * @var FeatureConfigurationInterface - */ - private $featureConfiguration; - - public function __construct(ConsentRepository $consentHashRepository, FeatureConfigurationInterface $featureConfiguration) + public function __construct(ConsentRepository $consentHashRepository) { $this->consentRepository = $consentHashRepository; - $this->featureConfiguration = $featureConfiguration; } public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion @@ -58,36 +48,11 @@ public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion public function storeConsentHash(ConsentStoreParameters $parameters): bool { - $migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION); - - if ($migrationEnabled) { - $parameters = new ConsentStoreParameters( - hashedUserId: $parameters->hashedUserId, - serviceId: $parameters->serviceId, - attributeStableHash: $parameters->attributeStableHash, - consentType: $parameters->consentType, - attributeHash: null, - ); - } - return $this->consentRepository->storeConsentHash($parameters); } public function updateConsentHash(ConsentUpdateParameters $parameters): bool { - $migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION); - - if ($migrationEnabled) { - $parameters = new ConsentUpdateParameters( - attributeStableHash: $parameters->attributeStableHash, - attributeHash: $parameters->attributeHash, - hashedUserId: $parameters->hashedUserId, - serviceId: $parameters->serviceId, - consentType: $parameters->consentType, - clearLegacyHash: true, - ); - } - return $this->consentRepository->updateConsentHash($parameters); } diff --git a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index caa6e0171..64ac008cf 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -226,30 +226,17 @@ public function hasConsentHash(ConsentHashQuery $query): ConsentVersion */ 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, - ]; - } + $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, + ]; try { $this->connection->executeStatement($query, $bindings); @@ -267,42 +254,22 @@ public function storeConsentHash(ConsentStoreParameters $parameters): bool */ 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 - "; - } + $query = " + UPDATE + consent + SET + attribute_stable = ? + WHERE + attribute = ? + AND + hashed_user_id = ? + AND + service_id = ? + AND + consent_type = ? + AND + deleted_at IS NULL + "; try { $affected = $this->connection->executeStatement($query, [ diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index ce7b2711c..90c42df5f 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -27,7 +27,6 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Consent\ConsentHashService; use OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository; -use OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -58,19 +57,7 @@ public function setup(): void $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->consentService = new ConsentHashService($this->consentRepository); $this->consent = new EngineBlock_Corto_Model_Consent( true, $this->response, @@ -164,49 +151,13 @@ public function test_stable_consent_given($consentType) } /** - * Toggle ON (migration enabled): new consent stores only the stable hash. - * The legacy attribute column must be left NULL so fully-migrated deployments - * don't accumulate unnecessary data in the old column. + * New consent always stores both the stable and legacy hashes so that old + * instances can still find the consent record during a rolling deploy, and + * so the legacy column is never wiped prematurely. */ #[DataProvider('consentTypeProvider')] - public function test_give_consent_toggle_on_stores_only_stable_hash($consentType) + public function test_give_consent_stores_both_hashes($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() @@ -234,41 +185,12 @@ public function test_give_consent_toggle_off_stores_both_hashes($consentType) } /** - * 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. + * Upgrading an unstable consent always preserves the legacy `attribute` column + * so that old instances keep working during a rolling deploy. */ #[DataProvider('consentTypeProvider')] - public function test_upgrade_toggle_on_clears_legacy_hash($consentType) + public function test_upgrade_preserves_legacy_hash($consentType) { - // setUp already builds with migrationEnabled=true $serviceProvider = new ServiceProvider("service-provider-entity-id"); $this->response->shouldReceive('getNameIdValue') ->once() @@ -282,7 +204,6 @@ public function test_upgrade_toggle_on_clears_legacy_hash($consentType) hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', serviceId: 'service-provider-entity-id', consentType: $consentType->value, - clearLegacyHash: true, )) ->andReturn(true); diff --git a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php index a4f3089bb..1d8aba613 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php @@ -21,7 +21,6 @@ use Mockery as m; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository; -use OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration; use PHPUnit\Framework\TestCase; class ConsentHashServiceTest extends TestCase @@ -36,8 +35,7 @@ class ConsentHashServiceTest extends TestCase public function setUp(): void { $mockConsentHashRepository = m::mock(ConsentRepository::class); - $featureConfig = new FeatureConfiguration(['eb.stable_consent_hash_migration' => false]); - $this->chs = new ConsentHashService($mockConsentHashRepository, $featureConfig); + $this->chs = new ConsentHashService($mockConsentHashRepository); } // -------------------------------------------------------------------------