From 37f5f8aec0ca19fce045ed4fbd230407b66d01fb Mon Sep 17 00:00:00 2001 From: Arnout van der Knaap Date: Sat, 29 Nov 2025 10:53:19 +0100 Subject: [PATCH 1/7] add use IDP LOA (authnContextClassReff) in Stepup Decision --- config/packages/engineblock_features.yaml | 1 + config/packages/parameters.yml.dist | 1 + .../Module/Service/AssertionConsumer.php | 9 +- .../Service/StepupAssertionConsumer.php | 2 +- .../EngineBlock/Stepup/StepupDecision.php | 40 +++++++ .../Stepup/StepupGatewayCallOutHelper.php | 7 +- .../EngineBlock/Stepup/StepupDecisionTest.php | 101 +++++++++++++++++- 7 files changed, 156 insertions(+), 5 deletions(-) diff --git a/config/packages/engineblock_features.yaml b/config/packages/engineblock_features.yaml index 467d8c190..1e5e69055 100644 --- a/config/packages/engineblock_features.yaml +++ b/config/packages/engineblock_features.yaml @@ -15,3 +15,4 @@ parameters: eb.feature_enable_idp_initiated_flow: "%feature_enable_idp_initiated_flow%" eb.stepup.sfo.override_engine_entityid: "%feature_stepup_sfo_override_engine_entityid%" eb.stepup.send_user_attributes: "%feature_stepup_send_user_attributes%" + eb.stepup_use_idp_response_loa: "%feature_use_idp_loa_in_stepup_decision%" diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index f8fe5741e..d1de69950 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -223,6 +223,7 @@ parameters: feature_enable_idp_initiated_flow: true feature_stepup_sfo_override_engine_entityid: false feature_stepup_send_user_attributes: false + feature_use_idp_loa_in_stepup_decision: false ########################################################################################## ## PROFILE SETTINGS diff --git a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php index 42f99d996..680743b40 100644 --- a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php @@ -188,8 +188,15 @@ public function serve($serviceName, Request $httpRequest) $pdpLoas = $receivedResponse->getPdpRequestedLoas(); $loaRepository = $application->getDiContainer()->getLoaRepository(); $authnRequestLoas = $receivedRequest->getStepupObligations($loaRepository->getStepUpLoas()); + + $idpResponseLoa = null; + // Determine the IdP response LoA if feature is enabled + if($application->getDiContainer()->getFeatureConfiguration()->isEnabled('eb.stepup_use_idp_response_loa')) { + $idpResponseLoa = $originalAssertions->getAuthnContextClassRef(); + } + // Goto consent if no Stepup authentication is needed - if (!$this->_stepupGatewayCallOutHelper->shouldUseStepup($idp, $sp, $authnRequestLoas, $pdpLoas)) { + if (!$this->_stepupGatewayCallOutHelper->shouldUseStepup($idp, $sp, $authnRequestLoas, $pdpLoas, $idpResponseLoa)) { $this->_server->sendConsentAuthenticationRequest($receivedResponse, $receivedRequest, $currentProcessStep->getRole(), $this->getAuthenticationState()); return; } diff --git a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php index 62b7d55d6..e95a94dc6 100644 --- a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php @@ -298,7 +298,7 @@ private function verifyReceivedLoa( $pdpLoas = $originalResponse->getPdpRequestedLoas(); $authnRequestLoas = $receivedRequest->getStepupObligations($this->_loaRepository->getStepUpLoas()); - $stepupDecision = new StepupDecision($idp, $sp, $authnRequestLoas, $pdpLoas, $this->_loaRepository, $log); + $stepupDecision = new StepupDecision($idp, $sp, $authnRequestLoas, $pdpLoas, null ,$this->_loaRepository, $log); $requiredLoa = $stepupDecision->getStepupLoa(); $receivedLoa = $this->_stepupGatewayCallOutHelper->getEbLoa( $receivedResponse->getAssertion()->getAuthnContextClassRef() diff --git a/src/OpenConext/EngineBlock/Stepup/StepupDecision.php b/src/OpenConext/EngineBlock/Stepup/StepupDecision.php index b446b4136..ca7b1064e 100644 --- a/src/OpenConext/EngineBlock/Stepup/StepupDecision.php +++ b/src/OpenConext/EngineBlock/Stepup/StepupDecision.php @@ -46,6 +46,11 @@ class StepupDecision /** * @var bool */ + /** + * @var string|null + */ + private $idpResponseLoa = null; + private $spNoToken; private $logger; @@ -58,6 +63,7 @@ public function __construct( ServiceProvider $sp, array $authnRequestLoas, array $pdpLoas, + string|null $idpResponseLoa, LoaRepository $loaRepository, LoggerInterface $logger ) { @@ -78,6 +84,16 @@ public function __construct( $this->spNoToken = $sp->getCoins()->stepupAllowNoToken(); + // Only set idpResponseLoa if provided and valid. Use getByIdentifier and ignore when not found. + if ($idpResponseLoa) { + try { + $this->idpResponseLoa = $loaRepository->getByIdentifier($idpResponseLoa); + } catch (\Exception $e) { + // The repository will throw when identifier is not known; log and ignore invalid response LoA + $this->logger->debug(sprintf('StepupDecision: IdP Response LoA "%s" is invalid and will be ignored', $idpResponseLoa)); + } + } + foreach ($pdpLoas as $loaId) { $this->pdpLoas[] = $loaRepository->getByIdentifier($loaId); } @@ -93,6 +109,11 @@ public function shouldUseStepup(): bool if ($isLoaAsked && $isLoaAsked->getLevel() === Loa::LOA_1) { return false; } + // If the Loa is reached by the IDP, no stepup callout is required. + if ($this->checkIDPLoaIsSufficient($isLoaAsked)) { + $this->logger->info('StepupDecision: IdP Response LoA is sufficient, no Stepup required'); + return false; + } return $isLoaAsked instanceof Loa; } @@ -147,6 +168,25 @@ public function getStepupLoa(): ?Loa return $highestLevel; } + /** + * Check if the IDP response LoA is sufficient for the requested LoA. + */ + private function checkIDPLoaIsSufficient($isLoaAsked): bool + { + if ($this->idpResponseLoa) { + return false; + } + + // If the IdP did not provide a LoA in the response, it cannot satisfy the requirement + if (!$this->idpResponseLoa) { + return false; + } + + // Check whether the IdP response LoA level meets or exceeds the requested LoA + return $this->idpResponseLoa->levelIsHigherOrEqualTo($isLoaAsked); + } + + public function allowNoToken(): bool { if ($this->isLoaRequirementSet()) { diff --git a/src/OpenConext/EngineBlock/Stepup/StepupGatewayCallOutHelper.php b/src/OpenConext/EngineBlock/Stepup/StepupGatewayCallOutHelper.php index 6ffacca96..77de17283 100644 --- a/src/OpenConext/EngineBlock/Stepup/StepupGatewayCallOutHelper.php +++ b/src/OpenConext/EngineBlock/Stepup/StepupGatewayCallOutHelper.php @@ -53,13 +53,15 @@ public function shouldUseStepup( IdentityProvider $identityProvider, ServiceProvider $serviceProvider, array $authnRequestLoas, - array $pdpLoas + array $pdpLoas, + string|null $idpResponseLoa ) : bool { $stepupDecision = new StepupDecision( $identityProvider, $serviceProvider, $authnRequestLoas, $pdpLoas, + $idpResponseLoa, $this->loaRepository, $this->logger ); @@ -77,6 +79,7 @@ public function getStepupLoa( $serviceProvider, $authnRequestLoas, $pdpLoas, + null, $this->loaRepository, $this->logger ); @@ -100,7 +103,7 @@ public function getEbLoa(string $gatewayLoa) : Loa public function allowNoToken(IdentityProvider $identityProvider, ServiceProvider $serviceProvider) : bool { - $stepupDecision = new StepupDecision($identityProvider, $serviceProvider, [], [], $this->loaRepository, $this->logger); + $stepupDecision = new StepupDecision($identityProvider, $serviceProvider, [], [], null, $this->loaRepository, $this->logger); return $stepupDecision->allowNoToken(); } } diff --git a/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php b/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php index 6074b8c7b..0558f6551 100644 --- a/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php +++ b/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php @@ -69,7 +69,7 @@ public function the_correct_stepup_decision_should_be_made_based_on_a_coin_data( $repo = $this->buildMockRepository($input); $logger = m::mock(LoggerInterface::class)->shouldIgnoreMissing(); - $stepupDecision = new StepupDecision($idp, $sp, $input[2], $input[3], $repo, $logger); + $stepupDecision = new StepupDecision($idp, $sp, $input[2], $input[3], null, $repo, $logger); $useStepup = $stepupDecision->shouldUseStepup(); $stepupLoa = $stepupDecision->getStepupLoa(); @@ -160,4 +160,103 @@ private static function buildLoa(string $loaLevel): Loa $loa = Loa::create($level, $loaLevel); return $loa; } + + #[\PHPUnit\Framework\Attributes\Test] + #[\PHPUnit\Framework\Attributes\Group('Stepup')] + public function idp_response_loa_is_sufficient_prevents_stepup() + { + $sp = Utils::instantiate( + ServiceProvider::class, + [ + 'entityId' => 'sp', + 'stepupRequireLoa' => 'loa20', + 'stepupAllowNoToken' => false, + ] + ); + + $idp = Utils::instantiate( + IdentityProvider::class, + [ + 'entityId' => 'idp', + 'stepupConnections' => new StepupConnections([]), + ] + ); + + $repo = m::mock(LoaRepository::class); + $repo->shouldReceive('getByIdentifier')->with('loa20')->andReturn(Loa::create(20, 'loa20')); + $repo->shouldReceive('getByIdentifier')->with('loa30')->andReturn(Loa::create(30, 'loa30')); + + $logger = m::mock(LoggerInterface::class)->shouldIgnoreMissing(); + + $stepupDecision = new StepupDecision($idp, $sp, [], [], 'loa30', $repo, $logger); + + $this->assertFalse($stepupDecision->shouldUseStepup()); + } + + #[\PHPUnit\Framework\Attributes\Test] + #[\PHPUnit\Framework\Attributes\Group('Stepup')] + public function idp_response_loa_is_insufficient_triggers_stepup() + { + $sp = Utils::instantiate( + ServiceProvider::class, + [ + 'entityId' => 'sp', + 'stepupRequireLoa' => 'loa30', + 'stepupAllowNoToken' => false, + ] + ); + + $idp = Utils::instantiate( + IdentityProvider::class, + [ + 'entityId' => 'idp', + 'stepupConnections' => new StepupConnections([]), + ] + ); + + $repo = m::mock(LoaRepository::class); + $repo->shouldReceive('getByIdentifier')->with('loa30')->andReturn(Loa::create(30, 'loa30')); + $repo->shouldReceive('getByIdentifier')->with('loa20')->andReturn(Loa::create(20, 'loa20')); + + $logger = m::mock(LoggerInterface::class)->shouldIgnoreMissing(); + + $stepupDecision = new StepupDecision($idp, $sp, [], [], 'loa20', $repo, $logger); + + $this->assertTrue($stepupDecision->shouldUseStepup()); + $this->assertEquals('loa30', $stepupDecision->getStepupLoa()->getIdentifier()); + } + + #[\PHPUnit\Framework\Attributes\Test] + #[\PHPUnit\Framework\Attributes\Group('Stepup')] + public function unknown_idp_response_loa_is_ignored_and_triggers_stepup() + { + $sp = Utils::instantiate( + ServiceProvider::class, + [ + 'entityId' => 'sp', + 'stepupRequireLoa' => 'loa30', + 'stepupAllowNoToken' => false, + ] + ); + + $idp = Utils::instantiate( + IdentityProvider::class, + [ + 'entityId' => 'idp', + 'stepupConnections' => new StepupConnections([]), + ] + ); + + $repo = m::mock(LoaRepository::class); + // SP expects loa30 + $repo->shouldReceive('getByIdentifier')->with('loa30')->andReturn(Loa::create(30, 'loa30')); + // The idp response contains an unknown LoA + $repo->shouldReceive('getByIdentifier')->with('bad-loa')->andThrow(new \OpenConext\EngineBlock\Exception\LoaNotFoundException('not found')); + + $logger = m::mock(LoggerInterface::class)->shouldIgnoreMissing(); + + $stepupDecision = new StepupDecision($idp, $sp, [], [], 'bad-loa', $repo, $logger); + + $this->assertTrue($stepupDecision->shouldUseStepup()); + } } From 96ed6bb6a515909af69f68e88201c95b4caf3ae8 Mon Sep 17 00:00:00 2001 From: Arnout van der Knaap Date: Sat, 29 Nov 2025 11:28:01 +0100 Subject: [PATCH 2/7] fix StepupDecision I thought was already fixed and typo --- .../Corto/Module/Service/StepupAssertionConsumer.php | 2 +- src/OpenConext/EngineBlock/Stepup/StepupDecision.php | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php index e95a94dc6..ee7f025a9 100644 --- a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php @@ -298,7 +298,7 @@ private function verifyReceivedLoa( $pdpLoas = $originalResponse->getPdpRequestedLoas(); $authnRequestLoas = $receivedRequest->getStepupObligations($this->_loaRepository->getStepUpLoas()); - $stepupDecision = new StepupDecision($idp, $sp, $authnRequestLoas, $pdpLoas, null ,$this->_loaRepository, $log); + $stepupDecision = new StepupDecision($idp, $sp, $authnRequestLoas, $pdpLoas, null, $this->_loaRepository, $log); $requiredLoa = $stepupDecision->getStepupLoa(); $receivedLoa = $this->_stepupGatewayCallOutHelper->getEbLoa( $receivedResponse->getAssertion()->getAuthnContextClassRef() diff --git a/src/OpenConext/EngineBlock/Stepup/StepupDecision.php b/src/OpenConext/EngineBlock/Stepup/StepupDecision.php index ca7b1064e..dea7034ad 100644 --- a/src/OpenConext/EngineBlock/Stepup/StepupDecision.php +++ b/src/OpenConext/EngineBlock/Stepup/StepupDecision.php @@ -173,16 +173,10 @@ public function getStepupLoa(): ?Loa */ private function checkIDPLoaIsSufficient($isLoaAsked): bool { - if ($this->idpResponseLoa) { - return false; - } - - // If the IdP did not provide a LoA in the response, it cannot satisfy the requirement if (!$this->idpResponseLoa) { return false; } - // Check whether the IdP response LoA level meets or exceeds the requested LoA return $this->idpResponseLoa->levelIsHigherOrEqualTo($isLoaAsked); } From ec6ed28afbb1f87d9cf291dbe1d4325db7300245 Mon Sep 17 00:00:00 2001 From: Arnout van der Knaap Date: Sat, 29 Nov 2025 15:21:12 +0100 Subject: [PATCH 3/7] changed feature name in code for consistency --- config/packages/engineblock_features.yaml | 2 +- library/EngineBlock/Corto/Module/Service/AssertionConsumer.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/packages/engineblock_features.yaml b/config/packages/engineblock_features.yaml index 1e5e69055..d28f7a9e2 100644 --- a/config/packages/engineblock_features.yaml +++ b/config/packages/engineblock_features.yaml @@ -15,4 +15,4 @@ parameters: eb.feature_enable_idp_initiated_flow: "%feature_enable_idp_initiated_flow%" eb.stepup.sfo.override_engine_entityid: "%feature_stepup_sfo_override_engine_entityid%" eb.stepup.send_user_attributes: "%feature_stepup_send_user_attributes%" - eb.stepup_use_idp_response_loa: "%feature_use_idp_loa_in_stepup_decision%" + eb.stepup.use_idp_response_loa: "%feature_use_idp_loa_in_stepup_decision%" diff --git a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php index 680743b40..7ce884670 100644 --- a/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/AssertionConsumer.php @@ -191,7 +191,7 @@ public function serve($serviceName, Request $httpRequest) $idpResponseLoa = null; // Determine the IdP response LoA if feature is enabled - if($application->getDiContainer()->getFeatureConfiguration()->isEnabled('eb.stepup_use_idp_response_loa')) { + if($application->getDiContainer()->getFeatureConfiguration()->isEnabled('eb.stepup.use_idp_response_loa')) { $idpResponseLoa = $originalAssertions->getAuthnContextClassRef(); } From 24345c239b03dec5832aa55c0795b8c077cdc0ef Mon Sep 17 00:00:00 2001 From: Arnout van der Knaap Date: Sat, 29 Nov 2025 17:08:05 +0100 Subject: [PATCH 4/7] add feature to TestFeatureConfiguration --- .../Configuration/TestFeatureConfiguration.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php index 9866bd8f7..5181fb03e 100644 --- a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php +++ b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php @@ -48,6 +48,8 @@ public function __construct() $this->setFeature(new Feature('eb.stepup.sfo.override_engine_entityid', false)); $this->setFeature(new Feature('eb.feature_enable_idp_initiated_flow', true)); $this->setFeature(new Feature('eb.stepup.send_user_attributes', true)); + $this->setFeature(new Feature('eb.stepup.use_idp_response_loa', false)); + } } public function setFeature(Feature $feature): void From 0c885a2e7bb7029e8e680dfa7cd6c5199e89c5a1 Mon Sep 17 00:00:00 2001 From: ArnoutvdKnaap Date: Sat, 29 Nov 2025 17:21:58 +0100 Subject: [PATCH 5/7] Fix extra } added by vscode --- .../EngineBlockBundle/Configuration/TestFeatureConfiguration.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php index 5181fb03e..b724a8a9b 100644 --- a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php +++ b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php @@ -50,7 +50,6 @@ public function __construct() $this->setFeature(new Feature('eb.stepup.send_user_attributes', true)); $this->setFeature(new Feature('eb.stepup.use_idp_response_loa', false)); } - } public function setFeature(Feature $feature): void { From 04b50a960acaafa3fb3358397c427d2b3f420219 Mon Sep 17 00:00:00 2001 From: Arnout van der Knaap Date: Sun, 30 Nov 2025 19:34:46 +0100 Subject: [PATCH 6/7] changed misplaced commentblock --- src/OpenConext/EngineBlock/Stepup/StepupDecision.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/OpenConext/EngineBlock/Stepup/StepupDecision.php b/src/OpenConext/EngineBlock/Stepup/StepupDecision.php index dea7034ad..75aded339 100644 --- a/src/OpenConext/EngineBlock/Stepup/StepupDecision.php +++ b/src/OpenConext/EngineBlock/Stepup/StepupDecision.php @@ -43,14 +43,13 @@ class StepupDecision * @var Loa[] */ private $pdpLoas = []; - /** - * @var bool - */ /** * @var string|null */ private $idpResponseLoa = null; - + /** + * @var bool + */ private $spNoToken; private $logger; From 213e327fa64e7cc3ff0b5cbf04d242ffd082cfa1 Mon Sep 17 00:00:00 2001 From: Arnout van der Knaap Date: Sun, 30 Nov 2025 19:38:50 +0100 Subject: [PATCH 7/7] added use statement for LoaNotFoundException in line with other tests. --- .../unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php b/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php index 0558f6551..2fa95bb99 100644 --- a/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php +++ b/tests/unit/OpenConext/EngineBlock/Stepup/StepupDecisionTest.php @@ -21,6 +21,7 @@ use Mockery as m; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Exception\InvalidStepupConfigurationException; +use OpenConext\EngineBlock\Exception\LoaNotFoundException; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\Loa; @@ -251,7 +252,7 @@ public function unknown_idp_response_loa_is_ignored_and_triggers_stepup() // SP expects loa30 $repo->shouldReceive('getByIdentifier')->with('loa30')->andReturn(Loa::create(30, 'loa30')); // The idp response contains an unknown LoA - $repo->shouldReceive('getByIdentifier')->with('bad-loa')->andThrow(new \OpenConext\EngineBlock\Exception\LoaNotFoundException('not found')); + $repo->shouldReceive('getByIdentifier')->with('bad-loa')->andThrow(new LoaNotFoundException('not found')); $logger = m::mock(LoggerInterface::class)->shouldIgnoreMissing();