-
Notifications
You must be signed in to change notification settings - Fork 26
add use IDP LOA (authnContextClassReff) in Stepup Decision #1893
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
37f5f8a
96ed6bb
ec6ed28
24345c2
0c885a2
04b50a9
213e327
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving the new I also get that you might want to bundle all LoA associated params together. So feel free to leave as is. |
||
| $requiredLoa = $stepupDecision->getStepupLoa(); | ||
| $receivedLoa = $this->_stepupGatewayCallOutHelper->getEbLoa( | ||
| $receivedResponse->getAssertion()->getAuthnContextClassRef() | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,10 @@ class StepupDecision | |||||
| * @var Loa[] | ||||||
| */ | ||||||
| private $pdpLoas = []; | ||||||
| /** | ||||||
| * @var string|null | ||||||
| */ | ||||||
| private $idpResponseLoa = null; | ||||||
| /** | ||||||
| * @var bool | ||||||
| */ | ||||||
|
|
@@ -58,6 +62,7 @@ public function __construct( | |||||
| ServiceProvider $sp, | ||||||
| array $authnRequestLoas, | ||||||
| array $pdpLoas, | ||||||
| string|null $idpResponseLoa, | ||||||
| LoaRepository $loaRepository, | ||||||
| LoggerInterface $logger | ||||||
| ) { | ||||||
|
|
@@ -78,6 +83,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 +108,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 +167,19 @@ public function getStepupLoa(): ?Loa | |||||
| return $highestLevel; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Check if the IDP response LoA is sufficient for the requested LoA. | ||||||
| */ | ||||||
| private function checkIDPLoaIsSufficient($isLoaAsked): bool | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| { | ||||||
| if (!$this->idpResponseLoa) { | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| return $this->idpResponseLoa->levelIsHigherOrEqualTo($isLoaAsked); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| public function allowNoToken(): bool | ||||||
| { | ||||||
| if ($this->isLoaRequirementSet()) { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -69,7 +70,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 +161,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')] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would personally add a few use-statements on the top of this file for these attributes and the exception below to keep the code better readable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only added 1 for the exception. That is more in line with the other tests.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough! I'm a nitpicker anyway :D |
||
| 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 LoaNotFoundException('not found')); | ||
|
|
||
| $logger = m::mock(LoggerInterface::class)->shouldIgnoreMissing(); | ||
|
|
||
| $stepupDecision = new StepupDecision($idp, $sp, [], [], 'bad-loa', $repo, $logger); | ||
|
|
||
| $this->assertTrue($stepupDecision->shouldUseStepup()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The:
Thinking about this that makes little sense. I would expect that in the case of an IdP handled 1st and 2nd factor authentication both the first and second factor class ref be present in the Assertion. But the SAML2 lib certainly exposes only one. (
\SAML2\Assertion::getAuthnContextClassRef)The XSD for the Assertion does imply that multiple authnContextClassRef can be stated. See: https://docs.oasis-open.org/security/saml/v2.0/saml-schema-assertion-2.0.xsd
I started thinking about this after reading this: https://security.stackexchange.com/a/227882
@pmeulen @baszoetekouw Am I making things up here?