Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion config/packages/engineblock_features.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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%"
1 change: 0 additions & 1 deletion config/packages/parameters.yml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion config/services/services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ services:
public: false
arguments:
- '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository'
- '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration'

OpenConext\EngineBlock\Service\Consent\ConsentService:
arguments:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
) {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
) {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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, [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}

// -------------------------------------------------------------------------
Expand Down