Revert feature flag to enable/disable content hash cleanup

johanib · johanib · commit 882403b91552 · 2026-04-08T15:46:28.000+02:00
The `feature_stable_consent_hash_migration` feature flag was confusing, and setting the old hash to null provided no real value
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
     }
 
     // -------------------------------------------------------------------------

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ public function __construct(`
`26`	`26`	`public readonly string $attributeStableHash,`
`27`	`27`	`public readonly string $consentType,`
`28`	`28`	`/** @deprecated Remove after stable consent hash is running in production */`
`29`		`- public readonly ?string $attributeHash = null,`
	`29`	`+ public readonly string $attributeHash,`
`30`	`30`	`) {`
`31`	`31`	`}`
`32`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -27,8 +27,6 @@ public function __construct(`
`27`	`27`	`public readonly string $hashedUserId,`
`28`	`28`	`public readonly string $serviceId,`
`29`	`29`	`public readonly string $consentType,`
`30`		`- /** @deprecated Remove after stable consent hash is running in production */`
`31`		`- public readonly bool $clearLegacyHash = false,`
`32`	`30`	`) {`
`33`	`31`	`}`
`34`	`32`	`}`