From 73002479833eba9b7174ef23271fb6a6f2108b1a Mon Sep 17 00:00:00 2001 From: Marco van den Bout Date: Thu, 23 May 2024 10:19:49 +0200 Subject: [PATCH 1/2] Fix: use nonce from original OIDC request --- .../Payload/Builder/MessagePayloadBuilder.php | 14 ++++++-------- src/Security/Oidc/OidcAuthenticator.php | 7 ++++++- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/Message/Payload/Builder/MessagePayloadBuilder.php b/src/Message/Payload/Builder/MessagePayloadBuilder.php index 504b43f..41213a1 100644 --- a/src/Message/Payload/Builder/MessagePayloadBuilder.php +++ b/src/Message/Payload/Builder/MessagePayloadBuilder.php @@ -64,7 +64,7 @@ public function reset(): MessagePayloadBuilderInterface public function withClaims(array $claims): MessagePayloadBuilderInterface { - foreach ($claims as $claimName => $claimValue){ + foreach ($claims as $claimName => $claimValue) { if (is_a($claimValue, MessagePayloadClaimInterface::class)) { $this->withClaim($claimValue); } else { @@ -104,15 +104,13 @@ protected function getToken(KeyChainInterface $keyChain): TokenInterface MessagePayloadInterface::HEADER_KID => $keyChain->getIdentifier() ]; - $claims = array_merge( - $this->claims->all(), - [ - MessagePayloadInterface::CLAIM_NONCE => $this->generator->generate()->getValue() - ] - ); + $claims = $this->claims->all(); + // Do not generate a new nonce when one is already present (fixes issue #154) + if (!$this->claims->has(MessagePayloadInterface::CLAIM_NONCE)) { + $claims[MessagePayloadInterface::CLAIM_NONCE] = $this->generator->generate()->getValue(); + } return $this->builder->build($headers, $claims, $keyChain->getPrivateKey()); - } catch (Throwable $exception) { throw new LtiException( sprintf('Cannot generate message token: %s', $exception->getMessage()), diff --git a/src/Security/Oidc/OidcAuthenticator.php b/src/Security/Oidc/OidcAuthenticator.php index d3db234..ddb3189 100644 --- a/src/Security/Oidc/OidcAuthenticator.php +++ b/src/Security/Oidc/OidcAuthenticator.php @@ -108,6 +108,12 @@ public function authenticate(ServerRequestInterface $request): LtiMessageInterfa ->withClaim(LtiMessagePayloadInterface::CLAIM_ISS, $registration->getPlatform()->getAudience()) ->withClaim(LtiMessagePayloadInterface::CLAIM_AUD, $registration->getClientId()); + // If the original request contained a nonce, add it to the payload (fixes #154) + $nonce = $oidcRequest->getParameters()->get('nonce'); + if ($nonce) { + $this->builder->withClaim(LtiMessagePayloadInterface::CLAIM_NONCE, $nonce); + } + if (!$authenticationResult->isAnonymous()) { foreach ($authenticationResult->getUserIdentity()->normalize() as $claimName => $claimValue) { $this->builder->withClaim($claimName, $claimValue); @@ -123,7 +129,6 @@ public function authenticate(ServerRequestInterface $request): LtiMessageInterfa 'state' => $oidcRequest->getParameters()->getMandatory('state') ] ); - } catch (LtiExceptionInterface $exception) { throw $exception; } catch (Throwable $exception) { From 6e35d7b2bb26d043e70913b61e780e6c01ea2384 Mon Sep 17 00:00:00 2001 From: Marco van den Bout Date: Thu, 23 May 2024 10:28:37 +0200 Subject: [PATCH 2/2] Adjust unit test to check the nonce --- tests/Unit/Security/Oidc/OidcAuthenticatorTest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php b/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php index 56ad2c3..c97f7e2 100644 --- a/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php +++ b/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php @@ -57,7 +57,7 @@ protected function setUp(): void $this->repositoryMock = $this->createMock(RegistrationRepositoryInterface::class); $this->authenticatorMock = $this->createMock(UserAuthenticatorInterface::class); - $this->subject= new OidcAuthenticator($this->repositoryMock, $this->authenticatorMock); + $this->subject = new OidcAuthenticator($this->repositoryMock, $this->authenticatorMock); } public function testAuthenticationSuccess(): void @@ -73,6 +73,7 @@ public function testAuthenticationSuccess(): void [ LtiMessagePayloadInterface::CLAIM_LTI_TARGET_LINK_URI => 'target_link_uri', LtiMessagePayloadInterface::CLAIM_REGISTRATION_ID => $registration->getIdentifier(), + LtiMessagePayloadInterface::CLAIM_NONCE => 'nonce', ], $registration->getToolKeyChain()->getPrivateKey() @@ -106,6 +107,10 @@ public function testAuthenticationSuccess(): void $registration->getIdentifier(), $idToken->getClaims()->get(LtiMessagePayloadInterface::CLAIM_REGISTRATION_ID) ); + $this->assertEquals( + 'nonce', + $idToken->getClaims()->get(LtiMessagePayloadInterface::CLAIM_NONCE) + ); } public function testAuthenticationSuccessWithUserIdentityClaimsSanitization(): void