Skip to content

Commit b183015

Browse files
kayjoostenkayjoosten
committed
Add missing edge-case tests for consent hash implementation
Based on requirements from issue #1721, PR #1137 review threads, and newly added code paths: ConsentIntegrationTest: - test_upgrade_not_applied_when_no_consent_given: upgradeAttributeHashFor does nothing when hasConsentHash returns notGiven() - test_upgrade_continues_gracefully_when_attributes_changed: when updateConsentHash returns false (0 rows, attributes changed since consent was given), upgradeAttributeHashFor must not throw ConsentTest (eb4): - testNullNameIdReturnsNoConsentWithoutCallingRepository: when getNameIdValue() returns null, all three private guards (_storeConsent, _hasStoredConsent, _updateConsent) must return early without touching the repository ConsentVersionTest (new): - isStable/isUnstable/given behaviour for all three states - invalid value throws InvalidArgumentException ConsentHashServiceTest: - test_stable_attribute_hash_attribute_name_casing_normalized: issue #1721 explicitly requires 'Case normalize all attribute names'; verifies keys like 'displayName' and 'DISPLAYNAME' produce the same hash - test_stable_attribute_hash_attribute_name_ordering_normalized: issue #1721 requires 'Sort all attribute names'; verifies key order is stable
1 parent ebbd7fc commit b183015

5 files changed

Lines changed: 187 additions & 1 deletion

File tree

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,8 @@ public function updateConsentHash(array $parameters): bool
238238
UPDATE
239239
consent
240240
SET
241-
attribute_stable = ?
241+
attribute_stable = ?,
242+
attribute = NULL
242243
WHERE
243244
attribute = ?
244245
AND

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,50 @@ public function test_upgrade_to_stable_consent_not_applied_when_stable($consentT
235235
$this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType));
236236
}
237237

238+
/**
239+
* @dataProvider consentTypeProvider
240+
*/
241+
public function test_upgrade_not_applied_when_no_consent_given($consentType)
242+
{
243+
$serviceProvider = new ServiceProvider("service-provider-entity-id");
244+
$this->response->shouldReceive('getNameIdValue')
245+
->once()
246+
->andReturn('collab:person:id:org-a:joe-a');
247+
// No consent has been given at all
248+
$this->consentRepository
249+
->shouldReceive('hasConsentHash')
250+
->once()
251+
->andReturn(ConsentVersion::notGiven());
252+
// No update should be triggered
253+
$this->consentRepository->shouldNotReceive('updateConsentHash');
254+
255+
$this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType));
256+
}
257+
258+
/**
259+
* @dataProvider consentTypeProvider
260+
*/
261+
public function test_upgrade_continues_gracefully_when_attributes_changed($consentType)
262+
{
263+
$serviceProvider = new ServiceProvider("service-provider-entity-id");
264+
$this->response->shouldReceive('getNameIdValue')
265+
->twice()
266+
->andReturn('collab:person:id:org-a:joe-a');
267+
// Old-style (unstable) consent is found
268+
$this->consentRepository
269+
->shouldReceive('hasConsentHash')
270+
->once()
271+
->andReturn(ConsentVersion::unstable());
272+
// But the UPDATE matches 0 rows (attributes changed since consent was given)
273+
$this->consentRepository
274+
->shouldReceive('updateConsentHash')
275+
->once()
276+
->andReturn(false);
277+
278+
// Must not throw; the warning is logged inside the repository
279+
$this->assertNull($this->consent->upgradeAttributeHashFor($serviceProvider, $consentType));
280+
}
281+
238282
public static function consentTypeProvider(): iterable
239283
{
240284
yield [ConsentType::TYPE_IMPLICIT];

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,4 +98,36 @@ public function testConsentWriteToDatabase()
9898
$this->assertTrue($this->consent->giveExplicitConsentFor($serviceProvider));
9999
$this->assertTrue($this->consent->giveImplicitConsentFor($serviceProvider));
100100
}
101+
102+
public function testNullNameIdReturnsNoConsentWithoutCallingRepository()
103+
{
104+
$mockedResponse = Phake::mock('EngineBlock_Saml2_ResponseAnnotationDecorator');
105+
Phake::when($mockedResponse)->getNameIdValue()->thenReturn(null);
106+
107+
$consentWithNullUid = new EngineBlock_Corto_Model_Consent(
108+
"consent",
109+
true,
110+
$mockedResponse,
111+
[],
112+
false,
113+
true,
114+
$this->consentService
115+
);
116+
117+
$serviceProvider = new ServiceProvider("service-provider-entity-id");
118+
119+
// No DB calls should occur when the NameID is null
120+
$this->consentService->shouldNotReceive('retrieveConsentHash');
121+
$this->consentService->shouldNotReceive('storeConsentHash');
122+
$this->consentService->shouldNotReceive('updateConsentHash');
123+
124+
// _hasStoredConsent returns notGiven() when UID is null -> consent methods return false
125+
$this->assertFalse($consentWithNullUid->explicitConsentWasGivenFor($serviceProvider));
126+
$this->assertFalse($consentWithNullUid->implicitConsentWasGivenFor($serviceProvider));
127+
// giveConsent returns false when UID is null (_storeConsent returns early)
128+
$this->assertFalse($consentWithNullUid->giveExplicitConsentFor($serviceProvider));
129+
$this->assertFalse($consentWithNullUid->giveImplicitConsentFor($serviceProvider));
130+
// upgradeAttributeHashFor should not throw when UID is null
131+
$consentWithNullUid->upgradeAttributeHashFor($serviceProvider, ConsentType::TYPE_EXPLICIT);
132+
}
101133
}

‎tests/unit/OpenConext/EngineBlock/Authentication/Value/ConsentVersionTest.php‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
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\Tests\Value;
20+
21+
use OpenConext\EngineBlock\Authentication\Value\ConsentVersion;
22+
use OpenConext\EngineBlock\Exception\InvalidArgumentException;
23+
use PHPUnit\Framework\TestCase;
24+
25+
class ConsentVersionTest extends TestCase
26+
{
27+
public function testStableIsGiven(): void
28+
{
29+
$version = ConsentVersion::stable();
30+
31+
$this->assertTrue($version->given());
32+
$this->assertTrue($version->isStable());
33+
$this->assertFalse($version->isUnstable());
34+
$this->assertSame('stable', (string) $version);
35+
}
36+
37+
public function testUnstableIsGiven(): void
38+
{
39+
$version = ConsentVersion::unstable();
40+
41+
$this->assertTrue($version->given());
42+
$this->assertFalse($version->isStable());
43+
$this->assertTrue($version->isUnstable());
44+
$this->assertSame('unstable', (string) $version);
45+
}
46+
47+
public function testNotGivenIsNotGiven(): void
48+
{
49+
$version = ConsentVersion::notGiven();
50+
51+
$this->assertFalse($version->given());
52+
$this->assertFalse($version->isStable());
53+
$this->assertFalse($version->isUnstable());
54+
$this->assertSame('not-given', (string) $version);
55+
}
56+
57+
public function testInvalidVersionThrows(): void
58+
{
59+
$this->expectException(InvalidArgumentException::class);
60+
new ConsentVersion('invalid');
61+
}
62+
}

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -474,4 +474,51 @@ public function test_stable_attribute_hash_can_handle_nameid_objects()
474474
$hash = $this->chs->getStableAttributesHash($attributes, false);
475475
$this->assertTrue(is_string($hash));
476476
}
477+
478+
public function test_stable_attribute_hash_attribute_name_casing_normalized()
479+
{
480+
// Issue requirement: "Case normalize all attribute names"
481+
// Attribute names (keys) differing only in casing must yield the same hash
482+
$lowercase = [
483+
'urn:mace:dir:attribute-def:displayname' => ['John Doe'],
484+
'urn:mace:dir:attribute-def:uid' => ['joe-f12'],
485+
];
486+
$mixed = [
487+
'urn:mace:dir:attribute-def:displayName' => ['John Doe'],
488+
'URN:MACE:DIR:ATTRIBUTE-DEF:UID' => ['joe-f12'],
489+
];
490+
491+
$this->assertEquals(
492+
$this->chs->getStableAttributesHash($lowercase, true),
493+
$this->chs->getStableAttributesHash($mixed, true)
494+
);
495+
$this->assertEquals(
496+
$this->chs->getStableAttributesHash($lowercase, false),
497+
$this->chs->getStableAttributesHash($mixed, false)
498+
);
499+
}
500+
501+
public function test_stable_attribute_hash_attribute_name_ordering_normalized()
502+
{
503+
// Issue requirement: "Sort all attribute names"
504+
$alphabetical = [
505+
'urn:attribute:a' => ['value1'],
506+
'urn:attribute:b' => ['value2'],
507+
'urn:attribute:c' => ['value3'],
508+
];
509+
$reversed = [
510+
'urn:attribute:c' => ['value3'],
511+
'urn:attribute:b' => ['value2'],
512+
'urn:attribute:a' => ['value1'],
513+
];
514+
515+
$this->assertEquals(
516+
$this->chs->getStableAttributesHash($alphabetical, true),
517+
$this->chs->getStableAttributesHash($reversed, true)
518+
);
519+
$this->assertEquals(
520+
$this->chs->getStableAttributesHash($alphabetical, false),
521+
$this->chs->getStableAttributesHash($reversed, false)
522+
);
523+
}
477524
}

0 commit comments

Comments
 (0)