Merge pull request #188 from marcovdb/fix/oidc-auth-nonce

Fix: pass the existing nonce back to the tool when performing OIDC authentication
oat-sa · Nov 7, 2024 · edfa0f4 · edfa0f4
2 parents ab569d5 + 6e35d7b
commit edfa0f4
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 10 deletions.
diff --git 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
@@ -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) {

diff --git 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