Skip to content

Commit 647abf0

Browse files
kayjoostenkayjoosten
committed
Fix consent hash implementation and code quality
- Guard upgradeAttributeHashFor against consent-disabled state: returns early without hitting the DB or recalculating hashes when consent is off - Fix DIP violation in Factory.php: type-hint ConsentHashServiceInterface instead of concrete ConsentHashService - Fix countAllFor memory issue: delegate to COUNT(*) query instead of loading all consent rows into memory - Fix null-guards in _hasStoredConsent() and _updateConsent(): return early when getConsentUid() returns null, consistent with _storeConsent() - Fix risky PHPUnit tests in ConsentTest: add assertions, use shouldNotReceive() - findAllFor: select attribute_stable alongside attribute, prefer it for new-format rows (fixes null attribute_hash in /api/consent response) - updateConsentHash: check rowCount(), log warning and return false on 0 rows - hasConsentHash: SELECT attribute_stable instead of SELECT * - ProcessConsent/ProvideConsent: skip upgradeAttributeHashFor on fresh consent - ConsentVersion: add isStable() counterpart to isUnstable() - Restore display_errors=0 in phpunit.xml to prevent PHP deprecation noise
1 parent f998ae5 commit 647abf0

File tree

21 files changed

+288
-126
lines changed

21 files changed

+288
-126
lines changed

‎config/services/controllers/api.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ services:
1616
- '@security.token_storage'
1717
- '@security.access.decision_manager'
1818
- '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration'
19-
- '@OpenConext\EngineBlock\Service\ConsentService'
19+
- '@OpenConext\EngineBlock\Service\Consent\ConsentService'
2020

2121
OpenConext\EngineBlockBundle\Controller\Api\DeprovisionController:
2222
arguments:

‎config/services/services.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,10 @@ services:
7878
engineblock.service.consent.ConsentHashService:
7979
class: OpenConext\EngineBlock\Service\Consent\ConsentHashService
8080
public: false
81+
arguments:
82+
- '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository'
8183

82-
OpenConext\EngineBlock\Service\ConsentService:
84+
OpenConext\EngineBlock\Service\Consent\ConsentService:
8385
arguments:
8486
- '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository'
8587
- '@OpenConext\EngineBlock\Service\MetadataService'

‎database/DoctrineMigrations/Version20220503092351.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
/**
99
* Change to the consent schema
10-
* 1. Added the `attribute_stable` column, string(80), not null
10+
* 1. Added the `attribute_stable` column, string(80), nullable
1111
* 2. Changed the `attribute` column, has been made nullable
1212
*/
1313
final class Version20220503092351 extends AbstractMigration
@@ -17,7 +17,7 @@ public function up(Schema $schema) : void
1717
// this up() migration is auto-generated, please modify it to your needs
1818
$this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');
1919

20-
$this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) NOT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL');
20+
$this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) DEFAULT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL');
2121
}
2222

2323
public function down(Schema $schema) : void

‎library/EngineBlock/Application/DiContainer.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ public function getAuthenticationLoopGuard()
165165
*/
166166
public function getConsentService()
167167
{
168-
return $this->container->get(\OpenConext\EngineBlock\Service\ConsentService::class);
168+
return $this->container->get(\OpenConext\EngineBlock\Service\Consent\ConsentService::class);
169169
}
170170

171171
/**

‎library/EngineBlock/Corto/Model/Consent.php‎

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
* limitations under the License.
1717
*/
1818

19+
use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery;
20+
use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters;
21+
use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters;
1922
use OpenConext\EngineBlock\Authentication\Value\ConsentVersion;
2023
use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
2124
use OpenConext\EngineBlock\Authentication\Value\ConsentType;
@@ -98,6 +101,9 @@ public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): bo
98101
*/
99102
public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string $consentType): void
100103
{
104+
if (!$this->_consentEnabled) {
105+
return;
106+
}
101107
$consentVersion = $this->_hasStoredConsent($serviceProvider, $consentType);
102108
if ($consentVersion->isUnstable()) {
103109
$this->_updateConsent($serviceProvider, $consentType);
@@ -157,39 +163,48 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType):
157163
return false;
158164
}
159165

160-
$parameters = array(
161-
sha1($consentUuid),
162-
$serviceProvider->entityId,
163-
$this->_getStableAttributesHash($this->_responseAttributes),
164-
$consentType,
166+
$parameters = new ConsentStoreParameters(
167+
hashedUserId: sha1($consentUuid),
168+
serviceId: $serviceProvider->entityId,
169+
attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes),
170+
consentType: $consentType,
165171
);
166172

167173
return $this->_hashService->storeConsentHash($parameters);
168174
}
169175

170176
private function _updateConsent(ServiceProvider $serviceProvider, $consentType): bool
171177
{
172-
$parameters = array(
173-
$this->_getStableAttributesHash($this->_responseAttributes),
174-
$this->_getAttributesHash($this->_responseAttributes),
175-
sha1($this->_getConsentUid()),
176-
$serviceProvider->entityId,
177-
$consentType,
178+
$consentUid = $this->_getConsentUid();
179+
if (!is_string($consentUid)) {
180+
return false;
181+
}
182+
183+
$parameters = new ConsentUpdateParameters(
184+
attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes),
185+
attributeHash: $this->_getAttributesHash($this->_responseAttributes),
186+
hashedUserId: sha1($consentUid),
187+
serviceId: $serviceProvider->entityId,
188+
consentType: $consentType,
178189
);
179190

180191
return $this->_hashService->updateConsentHash($parameters);
181192
}
182193

183194
private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): ConsentVersion
184195
{
185-
$consentUuid = sha1($this->_getConsentUid());
186-
$parameters = [
187-
$consentUuid,
188-
$serviceProvider->entityId,
189-
$this->_getAttributesHash($this->_responseAttributes),
190-
$this->_getStableAttributesHash($this->_responseAttributes),
191-
$consentType,
192-
];
193-
return $this->_hashService->retrieveConsentHash($parameters);
196+
$consentUid = $this->_getConsentUid();
197+
if (!is_string($consentUid)) {
198+
return ConsentVersion::notGiven();
199+
}
200+
201+
$query = new ConsentHashQuery(
202+
hashedUserId: sha1($consentUid),
203+
serviceId: $serviceProvider->entityId,
204+
attributeHash: $this->_getAttributesHash($this->_responseAttributes),
205+
attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes),
206+
consentType: $consentType,
207+
);
208+
return $this->_hashService->retrieveConsentHash($query);
194209
}
195210
}

‎library/EngineBlock/Corto/Model/Consent/Factory.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
* limitations under the License.
1717
*/
1818

19-
use OpenConext\EngineBlock\Service\Consent\ConsentHashService;
19+
use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface;
2020

2121
/**
2222
* @todo write a test
@@ -27,7 +27,7 @@ class EngineBlock_Corto_Model_Consent_Factory
2727
private $_filterCommandFactory;
2828

2929
/**
30-
* @var ConsentHashService
30+
* @var ConsentHashServiceInterface
3131
*/
3232
private $_hashService;
3333

@@ -36,7 +36,7 @@ class EngineBlock_Corto_Model_Consent_Factory
3636
*/
3737
public function __construct(
3838
EngineBlock_Corto_Filter_Command_Factory $filterCommandFactory,
39-
ConsentHashService $hashService
39+
ConsentHashServiceInterface $hashService
4040
) {
4141
$this->_filterCommandFactory = $filterCommandFactory;
4242
$this->_hashService = $hashService;

‎library/EngineBlock/Corto/Module/Service/ProcessConsent.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,9 @@ public function serve($serviceName, Request $httpRequest)
102102

103103
if (!$consentRepository->explicitConsentWasGivenFor($serviceProvider)) {
104104
$consentRepository->giveExplicitConsentFor($destinationMetadata);
105+
} else {
106+
$consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT);
105107
}
106-
$consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT);
107108

108109
$response->setConsent(Constants::CONSENT_OBTAINED);
109110
$response->setDestination($response->getReturn());

‎library/EngineBlock/Corto/Module/Service/ProvideConsent.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,9 @@ public function serve($serviceName, Request $httpRequest)
149149
if ($this->isConsentDisabled($spMetadataChain, $identityProvider)) {
150150
if (!$consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata)) {
151151
$consentRepository->giveImplicitConsentFor($serviceProviderMetadata);
152+
} else {
153+
$consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT);
152154
}
153-
$consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT);
154155

155156
$response->setConsent(Constants::CONSENT_INAPPLICABLE);
156157
$response->setDestination($response->getReturn());

‎src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@
1919
namespace OpenConext\EngineBlock\Authentication\Repository;
2020

2121
use OpenConext\EngineBlock\Authentication\Model\Consent;
22+
use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery;
23+
use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters;
24+
use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters;
2225
use OpenConext\EngineBlock\Authentication\Value\ConsentVersion;
2326

2427
interface ConsentRepository
@@ -42,18 +45,18 @@ public function deleteOneFor(string $userId, string $serviceProviderEntityId): b
4245
/**
4346
* Test if the consent row is set with an attribute hash either stable or unstable
4447
*/
45-
public function hasConsentHash(array $parameters): ConsentVersion;
48+
public function hasConsentHash(ConsentHashQuery $query): ConsentVersion;
4649

4750
/**
4851
* By default stores the stable consent hash. The legacy consent hash is left.
4952
*/
50-
public function storeConsentHash(array $parameters): bool;
53+
public function storeConsentHash(ConsentStoreParameters $parameters): bool;
5154

5255
/**
5356
* When a deprecated unstable consent hash is encoutered, we upgrade it to the new format using this
5457
* update consent hash method.
5558
*/
56-
public function updateConsentHash(array $parameters): bool;
59+
public function updateConsentHash(ConsentUpdateParameters $parameters): bool;
5760

5861
public function countTotalConsent($consentUid): int;
5962
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?php
2+
3+
/**
4+
* Copyright 2010 SURFnet B.V.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
namespace OpenConext\EngineBlock\Authentication\Value;
20+
21+
final class ConsentHashQuery
22+
{
23+
public function __construct(
24+
public readonly string $hashedUserId,
25+
public readonly string $serviceId,
26+
public readonly string $attributeHash,
27+
public readonly string $attributeStableHash,
28+
public readonly string $consentType,
29+
) {
30+
}
31+
}

0 commit comments

Comments
 (0)