From e6ffa0ddbb40b73cd212477998781dad9e626b6a Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Fri, 6 Mar 2026 14:39:23 +0100 Subject: [PATCH] refactor: convert ConsentType to a PHP 8.1 backed enum Replace the ConsentType value class (with its deprecated public constructor since ~2015) with a proper backed enum. - ConsentType is now `enum ConsentType: string` with cases Explicit = 'explicit' and Implicit = 'implicit' - The deprecated public constructor, the explicit()/implicit() named constructors and the equals() method are removed - DbalConsentRepository uses ConsentType::from() to hydrate from DB - Legacy Consent model methods now accept ConsentType and pass ->value into SQL parameter arrays - All call sites updated from TYPE_EXPLICIT/TYPE_IMPLICIT constants and named constructors to enum cases - ConsentTypeTest updated: invalid-string rejection tested via from(), equality via assertSame() on enum singletons --- library/EngineBlock/Corto/Model/Consent.php | 22 +++--- .../Corto/Module/Service/ProcessConsent.php | 2 +- .../Corto/Module/Service/ProvideConsent.php | 4 +- .../Authentication/Value/ConsentType.php | 69 ++----------------- .../Repository/DbalConsentRepository.php | 2 +- .../Corto/Model/ConsentIntegrationTest.php | 36 +++++----- .../Test/Corto/Model/ConsentTest.php | 6 +- .../Authentication/Dto/ConsentTest.php | 8 +-- .../Authentication/Value/ConsentTypeTest.php | 38 ++++++---- 9 files changed, 70 insertions(+), 117 deletions(-) diff --git a/library/EngineBlock/Corto/Model/Consent.php b/library/EngineBlock/Corto/Model/Consent.php index 65e76239d3..90ade14418 100644 --- a/library/EngineBlock/Corto/Model/Consent.php +++ b/library/EngineBlock/Corto/Model/Consent.php @@ -91,7 +91,7 @@ public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): Co // Consent disabled: treat as already given (stable — no upgrade needed) return ConsentVersion::stable(); } - return $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT); + return $this->_hasStoredConsent($serviceProvider, ConsentType::Explicit); } /** @@ -102,7 +102,7 @@ public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): Co * The caller must pass the ConsentVersion already retrieved by explicitConsentWasGivenFor or * implicitConsentWasGivenFor to avoid a second identical DB query. */ - public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string $consentType, ConsentVersion $consentVersion): void + public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, ConsentType $consentType, ConsentVersion $consentVersion): void { if (!$this->_consentEnabled) { return; @@ -117,19 +117,19 @@ public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): Co if (!$this->_consentEnabled) { return ConsentVersion::stable(); } - return $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); + return $this->_hasStoredConsent($serviceProvider, ConsentType::Implicit); } 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): bool { return !$this->_consentEnabled || - $this->_storeConsent($serviceProvider, ConsentType::TYPE_IMPLICIT); + $this->_storeConsent($serviceProvider, ConsentType::Implicit); } public function countTotalConsent(): int @@ -157,7 +157,7 @@ protected function _getStableAttributesHash($attributes): string return $this->_hashService->getStableAttributesHash($attributes, $this->_mustStoreValues); } - private function _storeConsent(ServiceProvider $serviceProvider, $consentType): bool + private function _storeConsent(ServiceProvider $serviceProvider, ConsentType $consentType): bool { $consentUuid = $this->_getConsentUid(); if (!is_string($consentUuid)) { @@ -168,14 +168,14 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType): hashedUserId: sha1($consentUuid), serviceId: $serviceProvider->entityId, attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), - consentType: $consentType, + consentType: $consentType->value, attributeHash: $this->_getAttributesHash($this->_responseAttributes), ); return $this->_hashService->storeConsentHash($parameters); } - private function _updateConsent(ServiceProvider $serviceProvider, $consentType): bool + private function _updateConsent(ServiceProvider $serviceProvider, ConsentType $consentType): bool { $consentUid = $this->_getConsentUid(); if (!is_string($consentUid)) { @@ -187,13 +187,13 @@ private function _updateConsent(ServiceProvider $serviceProvider, $consentType): attributeHash: $this->_getAttributesHash($this->_responseAttributes), hashedUserId: sha1($consentUid), serviceId: $serviceProvider->entityId, - consentType: $consentType, + consentType: $consentType->value, ); return $this->_hashService->updateConsentHash($parameters); } - private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): ConsentVersion + private function _hasStoredConsent(ServiceProvider $serviceProvider, ConsentType $consentType): ConsentVersion { $consentUid = $this->_getConsentUid(); if (!is_string($consentUid)) { @@ -205,7 +205,7 @@ private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentTyp serviceId: $serviceProvider->entityId, attributeHash: $this->_getAttributesHash($this->_responseAttributes), attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes), - consentType: $consentType, + consentType: $consentType->value, ); return $this->_hashService->retrieveConsentHash($query); } diff --git a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php index 69fdf3eda8..dd882adcc2 100644 --- a/library/EngineBlock/Corto/Module/Service/ProcessConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProcessConsent.php @@ -104,7 +104,7 @@ public function serve($serviceName, Request $httpRequest) if (!$explicitConsent->given()) { $consentRepository->giveExplicitConsentFor($destinationMetadata); } else { - $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT, $explicitConsent); + $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 f14668c5c8..902d988d9b 100644 --- a/library/EngineBlock/Corto/Module/Service/ProvideConsent.php +++ b/library/EngineBlock/Corto/Module/Service/ProvideConsent.php @@ -151,7 +151,7 @@ public function serve($serviceName, Request $httpRequest) if (!$implicitConsent->given()) { $consentRepository->giveImplicitConsentFor($serviceProviderMetadata); } else { - $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT, $implicitConsent); + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::Implicit, $implicitConsent); } $response->setConsent(Constants::CONSENT_INAPPLICABLE); @@ -170,7 +170,7 @@ public function serve($serviceName, Request $httpRequest) $priorConsent = $consentRepository->explicitConsentWasGivenFor($serviceProviderMetadata); if ($priorConsent->given()) { - $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_EXPLICIT, $priorConsent); + $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::Explicit, $priorConsent); $response->setConsent(Constants::CONSENT_PRIOR); diff --git a/src/OpenConext/EngineBlock/Authentication/Value/ConsentType.php b/src/OpenConext/EngineBlock/Authentication/Value/ConsentType.php index 7896d835dd..599ad2196e 100644 --- a/src/OpenConext/EngineBlock/Authentication/Value/ConsentType.php +++ b/src/OpenConext/EngineBlock/Authentication/Value/ConsentType.php @@ -19,73 +19,14 @@ namespace OpenConext\EngineBlock\Authentication\Value; use JsonSerializable; -use OpenConext\EngineBlock\Assert\Assertion; -final class ConsentType implements JsonSerializable +enum ConsentType: string implements JsonSerializable { - const TYPE_EXPLICIT = 'explicit'; - const TYPE_IMPLICIT = 'implicit'; + case Explicit = 'explicit'; + case Implicit = 'implicit'; - /** - * @var string - */ - private $consentType; - - /** - * @return ConsentType - */ - public static function explicit() - { - return new self(self::TYPE_EXPLICIT); - } - - /** - * @return ConsentType - */ - public static function implicit() - { - return new self(self::TYPE_IMPLICIT); - } - - /** - * @param ConsentType::TYPE_EXPLICIT|ConsentType::TYPE_IMPLICIT $consentType - * - * @deprecated Use the implicit and explicit named constructors. Will be removed - * when Doctrine ORM is implemented. - */ - public function __construct($consentType) - { - Assertion::choice( - $consentType, - [self::TYPE_EXPLICIT, self::TYPE_IMPLICIT], - 'ConsentType must be one of ConsentType::TYPE_EXPLICIT, ConsentType::TYPE_IMPLICIT' - ); - - $this->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/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php index edbdac5135..8415bbead2 100644 --- a/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php +++ b/src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php @@ -96,7 +96,7 @@ function (array $row) use ($userId) { $userId, $row['service_id'], new DateTime($row['consent_date']), - new ConsentType($row['consent_type']), + ConsentType::from($row['consent_type']), $row['attribute_stable'] ?? $row['attribute'] ); }, diff --git a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php index e979ddbd7a..f4d2634250 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentIntegrationTest.php @@ -95,10 +95,10 @@ public function test_no_previous_consent_given($consentType) ->once() ->andReturn(ConsentVersion::notGiven()); switch ($consentType) { - case ConsentType::TYPE_EXPLICIT: + case ConsentType::Explicit: $this->assertFalse($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); break; - case ConsentType::TYPE_IMPLICIT: + case ConsentType::Implicit: $this->assertFalse($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); break; } @@ -119,16 +119,16 @@ public function test_unstable_previous_consent_given($consentType) serviceId: 'service-provider-entity-id', attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', - consentType: $consentType, + consentType: $consentType->value, )) ->once() ->andReturn(ConsentVersion::unstable()); switch ($consentType) { - case ConsentType::TYPE_EXPLICIT: + case ConsentType::Explicit: $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); break; - case ConsentType::TYPE_IMPLICIT: + case ConsentType::Implicit: $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); break; } @@ -149,16 +149,16 @@ public function test_stable_consent_given($consentType) serviceId: 'service-provider-entity-id', attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', - consentType: $consentType, + consentType: $consentType->value, )) ->once() ->andReturn(ConsentVersion::stable()); switch ($consentType) { - case ConsentType::TYPE_EXPLICIT: + case ConsentType::Explicit: $this->assertTrue($this->consent->explicitConsentWasGivenFor($serviceProvider)->given()); break; - case ConsentType::TYPE_IMPLICIT: + case ConsentType::Implicit: $this->assertTrue($this->consent->implicitConsentWasGivenFor($serviceProvider)->given()); break; } @@ -184,16 +184,16 @@ public function test_give_consent_toggle_on_stores_only_stable_hash($consentType hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', serviceId: 'service-provider-entity-id', attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', - consentType: $consentType, + consentType: $consentType->value, attributeHash: null, )) ->andReturn(true); switch ($consentType) { - case ConsentType::TYPE_EXPLICIT: + case ConsentType::Explicit: $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); break; - case ConsentType::TYPE_IMPLICIT: + case ConsentType::Implicit: $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); break; } @@ -219,16 +219,16 @@ public function test_give_consent_toggle_off_stores_both_hashes($consentType) hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', serviceId: 'service-provider-entity-id', attributeStableHash: '8739602554c7f3241958e3cc9b57fdecb474d508', - consentType: $consentType, + consentType: $consentType->value, attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', )) ->andReturn(true); switch ($consentType) { - case ConsentType::TYPE_EXPLICIT: + case ConsentType::Explicit: $this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider)); break; - case ConsentType::TYPE_IMPLICIT: + case ConsentType::Implicit: $this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider)); break; } @@ -254,7 +254,7 @@ public function test_upgrade_toggle_off_preserves_legacy_hash($consentType) attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', serviceId: 'service-provider-entity-id', - consentType: $consentType, + consentType: $consentType->value, clearLegacyHash: false, )) ->andReturn(true); @@ -282,7 +282,7 @@ public function test_upgrade_toggle_on_clears_legacy_hash($consentType) attributeHash: '8739602554c7f3241958e3cc9b57fdecb474d508', hashedUserId: '0e54805079c56c2b1c1197a760af86ac337b7bac', serviceId: 'service-provider-entity-id', - consentType: $consentType, + consentType: $consentType->value, clearLegacyHash: true, )) ->andReturn(true); @@ -354,7 +354,7 @@ public function test_store_consent_hash_sql_resets_deleted_at_on_duplicate(): vo public static function consentTypeProvider(): iterable { - yield [ConsentType::TYPE_IMPLICIT]; - yield [ConsentType::TYPE_EXPLICIT]; + 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 8cd8a7f022..529d3dc394 100644 --- a/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php +++ b/tests/library/EngineBlock/Test/Corto/Model/ConsentTest.php @@ -80,8 +80,8 @@ public function testUpgradeAttributeHashSkippedWhenConsentDisabled() $this->consentService->shouldNotReceive('retrieveConsentHash'); $this->consentService->shouldNotReceive('updateConsentHash'); - $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT, ConsentVersion::stable()); - $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_IMPLICIT, ConsentVersion::stable()); + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::Explicit, ConsentVersion::stable()); + $this->consentDisabled->upgradeAttributeHashFor($serviceProvider, ConsentType::Implicit, ConsentVersion::stable()); } public function testConsentWriteToDatabase() @@ -175,6 +175,6 @@ public function testNullNameIdReturnsNoConsentWithoutCallingRepository() $this->assertFalse($consentWithNullUid->giveExplicitConsentFor($serviceProvider)); $this->assertFalse($consentWithNullUid->giveImplicitConsentFor($serviceProvider)); // upgradeAttributeHashFor should not throw when UID is null - $consentWithNullUid->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT, ConsentVersion::notGiven()); + $consentWithNullUid->upgradeAttributeHashFor($serviceProvider, ConsentType::Explicit, ConsentVersion::notGiven()); } } 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 index 2dc9d894be..a74d703b72 100644 --- a/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentTypeTest.php +++ b/tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentTypeTest.php @@ -29,8 +29,6 @@ class ConsentTypeTest extends TestCase { /** - * - * * @param mixed $invalid */ #[DataProviderExternal(TestDataProvider::class, 'notStringOrEmptyString')] @@ -40,9 +38,20 @@ class ConsentTypeTest extends TestCase #[Test] public function cannot_be_other_than_implicit_or_explicit($invalid) { - $this->expectException(InvalidArgumentException::class); + $this->expectException(\Throwable::class); - new ConsentType($invalid); + ConsentType::from($invalid); + } + + #[\PHPUnit\Framework\Attributes\Group('EngineBlock')] + #[\PHPUnit\Framework\Attributes\Group('Authentication')] + #[\PHPUnit\Framework\Attributes\Group('Value')] + #[\PHPUnit\Framework\Attributes\Test] + public function invalid_string_is_rejected() + { + $this->expectException(\ValueError::class); + + ConsentType::from('not-a-valid-consent-type'); } #[Group('EngineBlock')] @@ -51,11 +60,7 @@ public function cannot_be_other_than_implicit_or_explicit($invalid) #[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)); + $this->assertNotSame(ConsentType::Explicit, ConsentType::Implicit); } #[Group('EngineBlock')] @@ -64,10 +69,17 @@ public function different_consent_types_are_not_equal() #[Test] public function same_type_of_consent_types_are_equal() { - $explicit = ConsentType::explicit(); - $implicit = ConsentType::implicit(); + $this->assertSame(ConsentType::Explicit, ConsentType::Explicit); + $this->assertSame(ConsentType::Implicit, ConsentType::Implicit); + } - $this->assertTrue($explicit->equals(ConsentType::explicit())); - $this->assertTrue($implicit->equals(ConsentType::implicit())); + #[\PHPUnit\Framework\Attributes\Group('EngineBlock')] + #[\PHPUnit\Framework\Attributes\Group('Authentication')] + #[\PHPUnit\Framework\Attributes\Group('Value')] + #[\PHPUnit\Framework\Attributes\Test] + public function can_be_created_from_string() + { + $this->assertSame(ConsentType::Explicit, ConsentType::from('explicit')); + $this->assertSame(ConsentType::Implicit, ConsentType::from('implicit')); } }