Merge pull request #199 from oat-sa/fix/TR-6465/redirect-from-pltform…

…-launch-authentication

fix: #160 redirect to the actual redirect_uri after verifying that it matches the registered tool host

src/Security/Oidc/OidcAuthenticator.php

@@ -30,6 +30,7 @@
 use OAT\Library\Lti1p3Core\Message\Payload\Builder\MessagePayloadBuilder;
 use OAT\Library\Lti1p3Core\Message\Payload\Builder\MessagePayloadBuilderInterface;
 use OAT\Library\Lti1p3Core\Message\Payload\LtiMessagePayloadInterface;
+use OAT\Library\Lti1p3Core\Registration\RegistrationInterface;
 use OAT\Library\Lti1p3Core\Registration\RegistrationRepositoryInterface;
 use OAT\Library\Lti1p3Core\Security\Jwt\Parser\Parser;
 use OAT\Library\Lti1p3Core\Security\Jwt\Parser\ParserInterface;
@@ -99,6 +100,8 @@ public function authenticate(ServerRequestInterface $request): LtiMessageInterfa
                 throw new LtiBadRequestException('Invalid message hint');
             }
 
+            $redirectUri = $this->createToolRedirectUrl($registration, $oidcRequest);
+
             $authenticationResult = $this->authenticator->authenticate(
                 $registration,
                 $oidcRequest->getParameters()->getMandatory('login_hint')
@@ -128,7 +131,7 @@ public function authenticate(ServerRequestInterface $request): LtiMessageInterfa
             $payload = $this->builder->buildMessagePayload($registration->getPlatformKeyChain());
 
             return new LtiMessage(
-                $originalToken->getClaims()->getMandatory(LtiMessagePayloadInterface::CLAIM_LTI_TARGET_LINK_URI),
+                $redirectUri,
                 [
                     'id_token' => $payload->getToken()->toString(),
                     'state' => $oidcRequest->getParameters()->getMandatory('state')
@@ -153,4 +156,25 @@ private function sanitizeClaims(array $claims): array
 
         return $claims;
     }
+
+    private function createToolRedirectUrl(
+        RegistrationInterface $registration,
+        LtiMessageInterface $oidcRequest
+    ): string {
+        $uri = $oidcRequest->getParameters()->getMandatory('redirect_uri');
+        $host = parse_url($uri, PHP_URL_HOST);
+        /**
+         * redirect_uri must match one of the values associated with the tool's registration
+         * however, the library does not support multiple redirect_uris per registration,
+         * and relies on the launch URL to represent the tool's redirect_uri
+         *
+         * in order to support multiple redirect_uris, while still keeping the authentication
+         * relatively safe, the redirect_uri's host is compared against the pre-registered launch URL's host
+         */
+        if (!$host || $host !== parse_url($registration->getTool()->getLaunchUrl() ?? '', PHP_URL_HOST)) {
+            throw new LtiBadRequestException('Invalid redirect_uri');
+        }
+
+        return $uri;
+    }
 }

tests/Unit/Security/Oidc/OidcAuthenticatorTest.php

@@ -85,7 +85,8 @@ public function testAuthenticationSuccess(): void
             sprintf('http://platform.com/init?%s', http_build_query([
                 'login_hint' => 'login_hint',
                 'lti_message_hint' => $messageHint->toString(),
-                'state' => 'state'
+                'state' => 'state',
+                'redirect_uri' => 'http://tool.com/redirect',
             ]))
         );
 
@@ -112,6 +113,10 @@ public function testAuthenticationSuccess(): void
             'nonce',
             $idToken->getClaims()->get(LtiMessagePayloadInterface::CLAIM_NONCE)
         );
+        $this->assertEquals(
+            'http://tool.com/redirect',
+            $result->getUrl()
+        );
     }
 
     public function testAuthenticationSuccessWithUserIdentityClaimsSanitization(): void
@@ -145,7 +150,8 @@ public function testAuthenticationSuccessWithUserIdentityClaimsSanitization(): v
             sprintf('http://platform.com/init?%s', http_build_query([
                 'login_hint' => 'login_hint',
                 'lti_message_hint' => $messageHint->toString(),
-                'state' => 'state'
+                'state' => 'state',
+                'redirect_uri' => 'http://tool.com/redirect',
             ]))
         );
 
@@ -169,6 +175,113 @@ public function testAuthenticationSuccessWithUserIdentityClaimsSanitization(): v
         $this->assertEquals('userPicture', $idToken->getClaims()->get(LtiMessagePayloadInterface::CLAIM_USER_PICTURE));
     }
 
+    public function testAuthenticationEmptyRedirectUri(): void
+    {
+        $this->expectException(LtiException::class);
+        $this->expectExceptionMessage('OIDC authentication failed: Missing mandatory redirect_uri');
+
+        $registration = $this->createTestRegistration();
+
+        $messageHint = $this->buildJwt(
+            [
+                MessagePayloadInterface::HEADER_KID => $registration->getToolKeyChain()->getIdentifier()
+            ],
+            [
+                MessagePayloadInterface::CLAIM_REGISTRATION_ID => $registration->getIdentifier()
+            ],
+            $registration->getToolKeyChain()->getPrivateKey()
+        );
+
+        $this->repositoryMock
+            ->expects($this->once())
+            ->method('find')
+            ->with($registration->getIdentifier())
+            ->willReturn($registration);
+
+        $request = $this->createServerRequest(
+            'GET',
+            sprintf('http://platform.com/init?%s', http_build_query([
+                'login_hint' => 'login_hint',
+                'lti_message_hint' => $messageHint->toString(),
+                'state' => 'state',
+            ]))
+        );
+
+        $this->subject->authenticate($request);
+    }
+
+    public function testAuthenticationMalformedRedirectUri(): void
+    {
+        $this->expectException(LtiBadRequestException::class);
+        $this->expectExceptionMessage('Invalid redirect_uri');
+
+        $registration = $this->createTestRegistration();
+
+        $messageHint = $this->buildJwt(
+            [
+                MessagePayloadInterface::HEADER_KID => $registration->getToolKeyChain()->getIdentifier()
+            ],
+            [
+                MessagePayloadInterface::CLAIM_REGISTRATION_ID => $registration->getIdentifier()
+            ],
+            $registration->getToolKeyChain()->getPrivateKey()
+        );
+
+        $this->repositoryMock
+            ->expects($this->once())
+            ->method('find')
+            ->with($registration->getIdentifier())
+            ->willReturn($registration);
+
+        $request = $this->createServerRequest(
+            'GET',
+            sprintf('http://platform.com/init?%s', http_build_query([
+                'login_hint' => 'login_hint',
+                'lti_message_hint' => $messageHint->toString(),
+                'state' => 'state',
+                'redirect_uri' => 'tool.com/redirect'
+            ]))
+        );
+
+        $this->subject->authenticate($request);
+    }
+
+    public function testAuthenticationMismatchingRedirectUri(): void
+    {
+        $this->expectException(LtiBadRequestException::class);
+        $this->expectExceptionMessage('Invalid redirect_uri');
+
+        $registration = $this->createTestRegistration();
+
+        $messageHint = $this->buildJwt(
+            [
+                MessagePayloadInterface::HEADER_KID => $registration->getToolKeyChain()->getIdentifier()
+            ],
+            [
+                MessagePayloadInterface::CLAIM_REGISTRATION_ID => $registration->getIdentifier()
+            ],
+            $registration->getToolKeyChain()->getPrivateKey()
+        );
+
+        $this->repositoryMock
+            ->expects($this->once())
+            ->method('find')
+            ->with($registration->getIdentifier())
+            ->willReturn($registration);
+
+        $request = $this->createServerRequest(
+            'GET',
+            sprintf('http://platform.com/init?%s', http_build_query([
+                'login_hint' => 'login_hint',
+                'lti_message_hint' => $messageHint->toString(),
+                'state' => 'state',
+                'redirect_uri' => 'http://tool.malicious.com/redirect'
+            ]))
+        );
+
+        $this->subject->authenticate($request);
+    }
+
     public function testAuthenticationFailureOnExpiredMessageHint(): void
     {
         $this->expectException(LtiBadRequestException::class);
@@ -274,7 +387,8 @@ public function testAuthenticationFailureOnAuthenticationFailure(): void
             'GET',
             sprintf('http://platform.com/init?%s', http_build_query([
                 'lti_message_hint' => $messageHint->toString(),
-                'login_hint' => 'login_hint'
+                'login_hint' => 'login_hint',
+                'redirect_uri' => 'http://tool.com/redirect',
             ]))
         );