Skip to content

Commit 882403b

Browse files
johanibjohanib
committed
Revert feature flag to enable/disable content hash cleanup
The `feature_stable_consent_hash_migration` feature flag was confusing, and setting the old hash to null provided no real value
1 parent 1cdd470 commit 882403b

File tree

10 files changed

+39
-195
lines changed

10 files changed

+39
-195
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,7 @@ Changes:
2929

3030
* Stabilized consent checks
3131
* In order to make the consent hashes more robust, a more consistent way of hashing the user attributes has been introduced
32-
* This feature automatically migrates from the old hashes to the new hashes, cleaning up the old hash.
33-
* 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.
34-
* 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.
32+
* This feature automatically migrates from the old hashes to the new hashes upon login.
3533

3634
## 7.1.0
3735
[SBS](https://github.com/SURFscz/SBS) integration

‎config/packages/engineblock_features.yaml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,3 @@ parameters:
1616
eb.stepup.sfo.override_engine_entityid: "%feature_stepup_sfo_override_engine_entityid%"
1717
eb.stepup.send_user_attributes: "%feature_stepup_send_user_attributes%"
1818
eb.feature_enable_sram_interrupt: "%feature_enable_sram_interrupt%"
19-
eb.stable_consent_hash_migration: "%feature_stable_consent_hash_migration%"

‎config/packages/parameters.yml.dist‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,6 @@ parameters:
227227
feature_stepup_sfo_override_engine_entityid: false
228228
feature_stepup_send_user_attributes: false
229229
feature_enable_sram_interrupt: false
230-
feature_stable_consent_hash_migration: false
231230

232231
##########################################################################################
233232
## PROFILE SETTINGS

‎config/services/services.yml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,6 @@ services:
7979
public: false
8080
arguments:
8181
- '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository'
82-
- '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration'
8382

8483
OpenConext\EngineBlock\Service\Consent\ConsentService:
8584
arguments:

‎src/OpenConext/EngineBlock/Authentication/Value/ConsentStoreParameters.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public function __construct(
2626
public readonly string $attributeStableHash,
2727
public readonly string $consentType,
2828
/** @deprecated Remove after stable consent hash is running in production */
29-
public readonly ?string $attributeHash = null,
29+
public readonly string $attributeHash,
3030
) {
3131
}
3232
}

‎src/OpenConext/EngineBlock/Authentication/Value/ConsentUpdateParameters.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,6 @@ public function __construct(
2727
public readonly string $hashedUserId,
2828
public readonly string $serviceId,
2929
public readonly string $consentType,
30-
/** @deprecated Remove after stable consent hash is running in production */
31-
public readonly bool $clearLegacyHash = false,
3230
) {
3331
}
3432
}

‎src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php‎

Lines changed: 1 addition & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters;
2424
use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters;
2525
use OpenConext\EngineBlock\Authentication\Value\ConsentVersion;
26-
use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface;
2726
use function array_keys;
2827
use function implode;
2928
use function ksort;
@@ -32,23 +31,14 @@
3231

3332
final class ConsentHashService implements ConsentHashServiceInterface
3433
{
35-
/** @deprecated Remove after stable consent hash is running in production */
36-
private const FEATURE_MIGRATION = 'eb.stable_consent_hash_migration';
37-
3834
/**
3935
* @var ConsentRepository
4036
*/
4137
private $consentRepository;
4238

43-
/**
44-
* @var FeatureConfigurationInterface
45-
*/
46-
private $featureConfiguration;
47-
48-
public function __construct(ConsentRepository $consentHashRepository, FeatureConfigurationInterface $featureConfiguration)
39+
public function __construct(ConsentRepository $consentHashRepository)
4940
{
5041
$this->consentRepository = $consentHashRepository;
51-
$this->featureConfiguration = $featureConfiguration;
5242
}
5343

5444
public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion
@@ -58,36 +48,11 @@ public function retrieveConsentHash(ConsentHashQuery $query): ConsentVersion
5848

5949
public function storeConsentHash(ConsentStoreParameters $parameters): bool
6050
{
61-
$migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION);
62-
63-
if ($migrationEnabled) {
64-
$parameters = new ConsentStoreParameters(
65-
hashedUserId: $parameters->hashedUserId,
66-
serviceId: $parameters->serviceId,
67-
attributeStableHash: $parameters->attributeStableHash,
68-
consentType: $parameters->consentType,
69-
attributeHash: null,
70-
);
71-
}
72-
7351
return $this->consentRepository->storeConsentHash($parameters);
7452
}
7553

7654
public function updateConsentHash(ConsentUpdateParameters $parameters): bool
7755
{
78-
$migrationEnabled = $this->featureConfiguration->isEnabled(self::FEATURE_MIGRATION);
79-
80-
if ($migrationEnabled) {
81-
$parameters = new ConsentUpdateParameters(
82-
attributeStableHash: $parameters->attributeStableHash,
83-
attributeHash: $parameters->attributeHash,
84-
hashedUserId: $parameters->hashedUserId,
85-
serviceId: $parameters->serviceId,
86-
consentType: $parameters->consentType,
87-
clearLegacyHash: true,
88-
);
89-
}
90-
9156
return $this->consentRepository->updateConsentHash($parameters);
9257
}
9358

‎src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php‎

Lines changed: 27 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -226,30 +226,17 @@ public function hasConsentHash(ConsentHashQuery $query): ConsentVersion
226226
*/
227227
public function storeConsentHash(ConsentStoreParameters $parameters): bool
228228
{
229-
if ($parameters->attributeHash !== null) {
230-
$query = "INSERT INTO consent (hashed_user_id, service_id, attribute, attribute_stable, consent_type, consent_date, deleted_at)
231-
VALUES (?, ?, ?, ?, ?, NOW(), '0000-00-00 00:00:00')
232-
ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), attribute_stable=VALUES(attribute_stable),
233-
consent_type=VALUES(consent_type), consent_date=NOW(), deleted_at='0000-00-00 00:00:00'";
234-
$bindings = [
235-
$parameters->hashedUserId,
236-
$parameters->serviceId,
237-
$parameters->attributeHash,
238-
$parameters->attributeStableHash,
239-
$parameters->consentType,
240-
];
241-
} else {
242-
$query = "INSERT INTO consent (hashed_user_id, service_id, attribute_stable, consent_type, consent_date, deleted_at)
243-
VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00')
244-
ON DUPLICATE KEY UPDATE attribute_stable=VALUES(attribute_stable),
245-
consent_type=VALUES(consent_type), consent_date=NOW(), deleted_at='0000-00-00 00:00:00'";
246-
$bindings = [
247-
$parameters->hashedUserId,
248-
$parameters->serviceId,
249-
$parameters->attributeStableHash,
250-
$parameters->consentType,
251-
];
252-
}
229+
$query = "INSERT INTO consent (hashed_user_id, service_id, attribute, attribute_stable, consent_type, consent_date, deleted_at)
230+
VALUES (?, ?, ?, ?, ?, NOW(), '0000-00-00 00:00:00')
231+
ON DUPLICATE KEY UPDATE attribute=VALUES(attribute), attribute_stable=VALUES(attribute_stable),
232+
consent_type=VALUES(consent_type), consent_date=NOW(), deleted_at='0000-00-00 00:00:00'";
233+
$bindings = [
234+
$parameters->hashedUserId,
235+
$parameters->serviceId,
236+
$parameters->attributeHash,
237+
$parameters->attributeStableHash,
238+
$parameters->consentType,
239+
];
253240

254241
try {
255242
$this->connection->executeStatement($query, $bindings);
@@ -267,42 +254,22 @@ public function storeConsentHash(ConsentStoreParameters $parameters): bool
267254
*/
268255
public function updateConsentHash(ConsentUpdateParameters $parameters): bool
269256
{
270-
if ($parameters->clearLegacyHash) {
271-
$query = "
272-
UPDATE
273-
consent
274-
SET
275-
attribute_stable = ?,
276-
attribute = NULL
277-
WHERE
278-
attribute = ?
279-
AND
280-
hashed_user_id = ?
281-
AND
282-
service_id = ?
283-
AND
284-
consent_type = ?
285-
AND
286-
deleted_at IS NULL
287-
";
288-
} else {
289-
$query = "
290-
UPDATE
291-
consent
292-
SET
293-
attribute_stable = ?
294-
WHERE
295-
attribute = ?
296-
AND
297-
hashed_user_id = ?
298-
AND
299-
service_id = ?
300-
AND
301-
consent_type = ?
302-
AND
303-
deleted_at IS NULL
304-
";
305-
}
257+
$query = "
258+
UPDATE
259+
consent
260+
SET
261+
attribute_stable = ?
262+
WHERE
263+
attribute = ?
264+
AND
265+
hashed_user_id = ?
266+
AND
267+
service_id = ?
268+
AND
269+
consent_type = ?
270+
AND
271+
deleted_at IS NULL
272+
";
306273

307274
try {
308275
$affected = $this->connection->executeStatement($query, [

‎tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php‎

Lines changed: 8 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
2828
use OpenConext\EngineBlock\Service\Consent\ConsentHashService;
2929
use OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository;
30-
use OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration;
3130
use PHPUnit\Framework\Attributes\DataProvider;
3231
use PHPUnit\Framework\TestCase;
3332

@@ -58,19 +57,7 @@ public function setup(): void
5857
$this->response = Mockery::mock(EngineBlock_Saml2_ResponseAnnotationDecorator::class);
5958
$this->consentRepository = Mockery::mock(ConsentRepository::class);
6059

61-
$this->buildConsentAndService(migrationEnabled: true);
62-
}
63-
64-
/**
65-
* Rebuilds $this->consentService and $this->consent with the given toggle state.
66-
* Call this in tests that need a specific toggle setting different from setUp's default.
67-
*/
68-
private function buildConsentAndService(bool $migrationEnabled): void
69-
{
70-
$featureConfig = new FeatureConfiguration([
71-
'eb.stable_consent_hash_migration' => $migrationEnabled,
72-
]);
73-
$this->consentService = new ConsentHashService($this->consentRepository, $featureConfig);
60+
$this->consentService = new ConsentHashService($this->consentRepository);
7461
$this->consent = new EngineBlock_Corto_Model_Consent(
7562
true,
7663
$this->response,
@@ -164,49 +151,13 @@ public function test_stable_consent_given($consentType)
164151
}
165152

166153
/**
167-
* Toggle ON (migration enabled): new consent stores only the stable hash.
168-
* The legacy attribute column must be left NULL so fully-migrated deployments
169-
* don't accumulate unnecessary data in the old column.
154+
* New consent always stores both the stable and legacy hashes so that old
155+
* instances can still find the consent record during a rolling deploy, and
156+
* so the legacy column is never wiped prematurely.
170157
*/
171158
#[DataProvider('consentTypeProvider')]
172-
public function test_give_consent_toggle_on_stores_only_stable_hash($consentType)
159+
public function test_give_consent_stores_both_hashes($consentType)
173160
{
174-
// setUp already builds with migrationEnabled=true
175-
$serviceProvider = new ServiceProvider("service-provider-entity-id");
176-
$this->response->shouldReceive('getNameIdValue')
177-
->once()
178-
->andReturn('collab:person:id:org-a:joe-a');
179-
$this->consentRepository
180-
->shouldReceive('storeConsentHash')
181-
->once()
182-
->with(new ConsentStoreParameters(
183-
hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac',
184-
serviceId: 'service-provider-entity-id',
185-
attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508',
186-
consentType: $consentType->value,
187-
attributeHash: null,
188-
))
189-
->andReturn(true);
190-
191-
switch ($consentType) {
192-
case ConsentType::Explicit:
193-
$this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider));
194-
break;
195-
case ConsentType::Implicit:
196-
$this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider));
197-
break;
198-
}
199-
}
200-
201-
/**
202-
* Toggle OFF (migration disabled): new consent stores BOTH hashes so that
203-
* old EB instances (still reading only the `attribute` column) can still
204-
* find the consent record during a rolling deploy.
205-
*/
206-
#[DataProvider('consentTypeProvider')]
207-
public function test_give_consent_toggle_off_stores_both_hashes($consentType)
208-
{
209-
$this->buildConsentAndService(migrationEnabled: false);
210161
$serviceProvider = new ServiceProvider("service-provider-entity-id");
211162
$this->response->shouldReceive('getNameIdValue')
212163
->once()
@@ -234,41 +185,12 @@ public function test_give_consent_toggle_off_stores_both_hashes($consentType)
234185
}
235186

236187
/**
237-
* Toggle OFF (migration disabled): upgrading an old unstable consent leaves
238-
* the legacy `attribute` column intact so old instances keep working.
239-
*/
240-
#[DataProvider('consentTypeProvider')]
241-
public function test_upgrade_toggle_off_preserves_legacy_hash($consentType)
242-
{
243-
$this->buildConsentAndService(migrationEnabled: false);
244-
$serviceProvider = new ServiceProvider("service-provider-entity-id");
245-
$this->response->shouldReceive('getNameIdValue')
246-
->once()
247-
->andReturn('collab:person:id:org-a:joe-a');
248-
$this->consentRepository
249-
->shouldReceive('updateConsentHash')
250-
->once()
251-
->with(new ConsentUpdateParameters(
252-
attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508',
253-
attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508',
254-
hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac',
255-
serviceId: 'service-provider-entity-id',
256-
consentType: $consentType->value,
257-
clearLegacyHash: false,
258-
))
259-
->andReturn(true);
260-
261-
$this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType, ConsentVersion::Unstable));
262-
}
263-
264-
/**
265-
* Toggle ON (migration enabled): upgrading an old unstable consent nulls the
266-
* legacy `attribute` column so the old column is cleaned up over time.
188+
* Upgrading an unstable consent always preserves the legacy `attribute` column
189+
* so that old instances keep working during a rolling deploy.
267190
*/
268191
#[DataProvider('consentTypeProvider')]
269-
public function test_upgrade_toggle_on_clears_legacy_hash($consentType)
192+
public function test_upgrade_preserves_legacy_hash($consentType)
270193
{
271-
// setUp already builds with migrationEnabled=true
272194
$serviceProvider = new ServiceProvider("service-provider-entity-id");
273195
$this->response->shouldReceive('getNameIdValue')
274196
->once()
@@ -282,7 +204,6 @@ public function test_upgrade_toggle_on_clears_legacy_hash($consentType)
282204
hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac',
283205
serviceId: 'service-provider-entity-id',
284206
consentType: $consentType->value,
285-
clearLegacyHash: true,
286207
))
287208
->andReturn(true);
288209

‎tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
use Mockery as m;
2222
use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
2323
use OpenConext\EngineBlock\Authentication\Repository\ConsentRepository;
24-
use OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration;
2524
use PHPUnit\Framework\TestCase;
2625

2726
class ConsentHashServiceTest extends TestCase
@@ -36,8 +35,7 @@ class ConsentHashServiceTest extends TestCase
3635
public function setUp(): void
3736
{
3837
$mockConsentHashRepository = m::mock(ConsentRepository::class);
39-
$featureConfig = new FeatureConfiguration(['eb.stable_consent_hash_migration' => false]);
40-
$this->chs = new ConsentHashService($mockConsentHashRepository, $featureConfig);
38+
$this->chs = new ConsentHashService($mockConsentHashRepository);
4139
}
4240

4341
// -------------------------------------------------------------------------

0 commit comments

Comments
 (0)