Skip to content

CW-3104_pairwiseid-pick-spentityid-not-idpentityid #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pradtke merged 3 commits into from
Apr 14, 2026
Merged

CW-3104_pairwiseid-pick-spentityid-not-idpentityid #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b0d08c
Fix pairwise-id SP entityID extraction + tests
ioigoume Apr 10, 2026
083532c
Use Destination.entityid for pairwise-id SP binding and assert pairwi…
ioigoume Apr 14, 2026
7cdcd90
Remove redendant test
ioigoume Apr 14, 2026
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
38 changes: 20 additions & 18 deletions phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8" ?>
<?xml version="1.0" encoding="utf-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
colors="true"
bootstrap="./tests/bootstrap.php"
Expand All @@ -7,22 +7,24 @@
displayDetailsOnTestsThatTriggerWarnings="true"
displayDetailsOnTestsThatTriggerDeprecations="true"
backupGlobals="false"
failOnWarning="false"
>
<testsuites>
<testsuite name="The project's test suite">
<directory>./tests</directory>
</testsuite>
</testsuites>
failOnWarning="false">
<testsuites>
<testsuite name="The project's test suite">
<directory>./tests</directory>
</testsuite>
</testsuites>

<coverage includeUncoveredFiles="true">
<include>
<directory>./src</directory>
</include>
<report>
<cobertura outputFile="build/logs/cobertura.xml"/>
<clover outputFile="build/logs/clover.xml"/>
<html outputDirectory="build/coverage/html"/>
</report>
</coverage>
<coverage includeUncoveredFiles="true">
<report>
<cobertura outputFile="build/logs/cobertura.xml"/>
<clover outputFile="build/logs/clover.xml"/>
<html outputDirectory="build/coverage/html"/>
</report>
</coverage>

<source>
<include>
<directory>./src</directory>
</include>
</source>
</phpunit>
34 changes: 29 additions & 5 deletions src/Auth/Process/PairwiseID.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,7 @@ public function process(array &$state): void
throw new \InvalidArgumentException('Missing or invalid attribute array in state.');
}

if (!isset($state['Source']['entityid']) || !is_string($state['Source']['entityid'])) {
throw new \InvalidArgumentException('Missing or invalid Source/entityid in state.');
}

$spEntityId = $this->extractSpEntityId($state);
$secretSalt = $this->getSecretSalt();

if (empty($secretSalt)) {
Expand All @@ -99,7 +96,7 @@ public function process(array &$state): void
$pairwiseId = $this->generatePairwiseId(
$state['Attributes'],
$this->attribute,
(string)$state['Source']['entityid'],
$spEntityId,
$secretSalt,
$this->algorithm,
$this->scope,
Expand Down Expand Up @@ -215,4 +212,31 @@ private function computeShibbolethStyleHmacSha256Reference(
}
return $b32;
}

/**
* Determine which SP entityID to bind a pairwise identifier to.
*
* Product position:
* - Pairwise IDs MUST be generated for the SP that is directly integrated with us (the immediate relying party),
* not for an "ultimate/original requester" in proxied/hub scenarios.
*
* In SimpleSAMLphp state, that direct SP is represented as:
* - `$state['Destination']['entityid']`
*
* @param array $state The SimpleSAMLphp state array for the current request.
* @return string The selected SP entityID.
*/
private function extractSpEntityId(array $state): string
{
if (
isset($state['Destination']['entityid'])
&& is_array($state['Destination'])
&& is_string($state['Destination']['entityid'])
&& $state['Destination']['entityid'] !== ''
) {
return $state['Destination']['entityid'];
}

throw new \InvalidArgumentException('Missing SP entityID (Destination[entityid]).');
}
}
76 changes: 58 additions & 18 deletions tests/src/Auth/Process/PairwiseIDTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,19 @@ class PairwiseIDTest extends TestCase
private array $config = [
'attribute' => 'uid',
'scope' => 'example.com',
'algorithm' => 'sha1'
'algorithm' => 'sha1',
];

private array $state = [
'Attributes' => [
'uid' => ['774333']
],
'Destination' =>
[
'entityid' => 'https://somesp.edugain.example.edu/sp'
],
'Source' =>
[
'entityid' => 'https://idp.example.edu/shibboleth'
]
// Pairwise ID is computed per *directly integrated* SP; in SSP IdP state this is Destination[entityid].
'Destination' => [
'entityid' => 'https://somesp.edugain.example.edu/sp',
],
// Kept only for legacy/back-compat state shape; no longer used for pairwise-id selection.
'core:SP' => 'https://somesp.edugain.example.edu/sp',
];

public function testNoConfigOptions(): void
Expand All @@ -40,8 +38,8 @@ public function testNoConfigOptions(): void
$pairwiseId->process($localState);

$localState = $this->state;
unset($localState['Source']);
$this->expectExceptionMessage('Missing or invalid Source/entityid in state.');
unset($localState['Destination']);
$this->expectExceptionMessage('Missing SP entityID (Destination[entityid]).');
$pairwiseId->process($localState);
}

Expand Down Expand Up @@ -77,19 +75,19 @@ public function testDefaultConfig(): void
* @throws Exception
* @throws \Exception
*/
public function testPairwiseID()
public function testPairwiseID(): void
{
$pairwiseId = new PairwiseID($this->config, null);
$localState = $this->state;

$pairwiseId = new PairwiseID($this->config, null);
$pairwiseId->process($localState);
$this->assertArrayHasKey(PairwiseID::PAIRWISEID_ATTR_NAME, $localState['Attributes']);
$this->assertStringEndsWith(
'@example.com',
$localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0],
);

$expectedPairwiseId = 'XEFBRGW7UTQJ6EKRXF6Q4K6FOQG32ZX3@example.com';
$expectedPairwiseId = 'B7VDEFQKNFXREJWWRDH3FKXBU4S3YGOY@example.com';

$this->assertEquals($expectedPairwiseId, $localState['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0]);
Comment thread
ioigoume marked this conversation as resolved.
}
Expand Down Expand Up @@ -149,7 +147,7 @@ public function testGeneratePairwiseIdHmacSha256MatchesReference(): void
/**
* @throws \Exception
*/
public function testPairwiseIDFailOnEmptyAttribute()
public function testPairwiseIDFailOnEmptyAttribute(): void
{
$pairwiseId = new PairwiseID($this->config, null);
$localState = $this->state;
Expand Down Expand Up @@ -203,17 +201,17 @@ public function testInvalidAlgorithmThrows(): void

$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage("Invalid algorithm. Allowed: 'sha1', 'hmac-sha256'.");
$pairwise->generatePairwiseId($attributes, 'uid', $sp, $salt, $scope, 'md5');
$pairwise->generatePairwiseId($attributes, 'uid', $sp, $salt, 'md5', $scope);
}

public function testMissingAlgorithmConfigurationThrows(): void
{
$config = $this->config;
unset($config['algorithm']);

$this->expectException(\UnexpectedValueException::class);
$this->expectException(\Exception::class);
$this->expectExceptionMessage("Could not retrieve the required option 'algorithm'");
$pairwise = new PairwiseID($config, null);
new PairwiseID($config, null);
}

public function testAlgorithmsProduceDifferentOutputs(): void
Expand All @@ -231,4 +229,46 @@ public function testAlgorithmsProduceDifferentOutputs(): void

$this->assertNotSame($sha1, $hmac, 'SHA-1 and HMAC-SHA256 outputs must differ');
}

public function testPairwiseIdChangesWhenDestinationEntityIdChanges(): void
{
$pairwise = $this->getMockBuilder(PairwiseID::class)
->setConstructorArgs([$this->config, null])
->onlyMethods(['getSecretSalt'])
->getMock();

$pairwise->method('getSecretSalt')->willReturn('secretsalt');

$stateOne = $this->state;
$stateOne['Destination']['entityid'] = 'https://destination-one.example.org/sp';
$pairwise->process($stateOne);
$valueOne = $stateOne['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0];

$stateTwo = $this->state;
$stateTwo['Destination']['entityid'] = 'https://destination-two.example.org/sp';
$pairwise->process($stateTwo);
$valueTwo = $stateTwo['Attributes'][PairwiseID::PAIRWISEID_ATTR_NAME][0];

$this->assertNotSame($valueOne, $valueTwo, 'Pairwise ID must change when Destination[entityid] changes');

$expectedOne = $pairwise->generatePairwiseId(
['uid' => ['774333']],
'uid',
'https://destination-one.example.org/sp',
'secretsalt',
'sha1',
'example.com',
);
$expectedTwo = $pairwise->generatePairwiseId(
['uid' => ['774333']],
'uid',
'https://destination-two.example.org/sp',
'secretsalt',
'sha1',
'example.com',
);

$this->assertSame($expectedOne, $valueOne);
$this->assertSame($expectedTwo, $valueTwo);
}
}
Loading