Use RunnableResponse instead of redirect

simplesamlphp · Dec 15, 2024 · 801c4fd · 801c4fd
1 parent 640c7eb
commit 801c4fd
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 37 deletions.
diff --git a/src/Controller/LogoutController.php b/src/Controller/LogoutController.php
@@ -5,15 +5,15 @@
 namespace SimpleSAML\Module\casserver\Controller;
 
 use SimpleSAML\Auth\Simple;
-use SimpleSAML\Compat\SspContainer;
 use SimpleSAML\Configuration;
+use SimpleSAML\HTTP\RunnableResponse;
 use SimpleSAML\Logger;
 use SimpleSAML\Module;
 use SimpleSAML\Module\casserver\Cas\Factories\TicketFactory;
 use SimpleSAML\Module\casserver\Cas\Ticket\TicketStore;
 use SimpleSAML\Module\casserver\Controller\Traits\UrlTrait;
 use SimpleSAML\Session;
-use Symfony\Component\HttpFoundation\RedirectResponse;
+use SimpleSAML\Utils;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Attribute\AsController;
 use Symfony\Component\HttpKernel\Attribute\MapQueryParameter;
@@ -35,17 +35,18 @@ class LogoutController
     /** @var Simple  */
     protected Simple $authSource;
 
-    /** @var SspContainer */
-    protected SspContainer $container;
+    /** @var Utils\HTTP */
+    protected Utils\HTTP $httpUtils;
 
     /** @var TicketStore */
     protected TicketStore $ticketStore;
 
 
     /**
+     * @param   Configuration       $sspConfig
      * @param   Configuration|null  $casConfig
      * @param   Simple|null         $source
-     * @param   SspContainer|null   $container
+     * @param   Utils\HTTP|null     $httpUtils
      *
      * @throws \Exception
      */
@@ -54,15 +55,15 @@ public function __construct(
         // Facilitate testing
         Configuration $casConfig = null,
         Simple $source = null,
-        SspContainer $container = null,
+        Utils\HTTP $httpUtils = null,
     ) {
         // We are using this work around in order to bypass Symfony's autowiring for cas configuration. Since
         // the configuration class is the same, it loads the ssp configuration twice. Still, we need the constructor
         // argument in order to facilitate testin.
         $this->casConfig = ($casConfig === null || $casConfig === $sspConfig)
             ? Configuration::getConfig('module_casserver.php') : $casConfig;
         $this->authSource = $source ?? new Simple($this->casConfig->getValue('authsource'));
-        $this->container = $container ?? new SspContainer();
+        $this->httpUtils = $httpUtils ?? new Utils\HTTP();
 
         /* Instantiate ticket factory */
         $this->ticketFactory = new TicketFactory($this->casConfig);
@@ -80,12 +81,12 @@ public function __construct(
      * @param   Request      $request
      * @param   string|null  $url
      *
-     * @return RedirectResponse|null
+     * @return RunnableResponse|null
      */
     public function logout(
         Request $request,
         #[MapQueryParameter] ?string $url = null,
-    ): RedirectResponse|null {
+    ): RunnableResponse|null {
         if (!$this->casConfig->getOptionalValue('enable_logout', false)) {
             $this->handleExceptionThrown('Logout not allowed');
         }
@@ -115,7 +116,7 @@ public function logout(
 
         // Redirect
         if (!$this->authSource->isAuthenticated()) {
-            $this->container->redirect($logoutRedirectUrl, $params);
+            return new RunnableResponse([$this->httpUtils, 'redirectTrustedURL'], [$logoutRedirectUrl, $params]);
         }
 
         // Logout and redirect

diff --git a/tests/src/Controller/LogoutControllerTest.php b/tests/src/Controller/LogoutControllerTest.php
@@ -7,11 +7,11 @@
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use SimpleSAML\Auth\Simple;
-use SimpleSAML\Compat\SspContainer;
 use SimpleSAML\Configuration;
 use SimpleSAML\Module;
 use SimpleSAML\Module\casserver\Controller\LogoutController;
 use SimpleSAML\Session;
+use SimpleSAML\Utils;
 use Symfony\Component\HttpFoundation\Request;
 
 class LogoutControllerTest extends TestCase
@@ -20,7 +20,7 @@ class LogoutControllerTest extends TestCase
 
     private Simple|MockObject $authSimpleMock;
 
-    private SspContainer|MockObject $sspContainer;
+    private Utils\HTTP $httpUtils;
 
     private Configuration $sspConfig;
 
@@ -31,18 +31,14 @@ protected function setUp(): void
             ->onlyMethods(['logout', 'isAuthenticated'])
             ->getMock();
 
-        $this->sspContainer = $this->getMockBuilder(SspContainer::class)
-            ->disableOriginalConstructor()
-            ->onlyMethods(['redirect'])
-            ->getMock();
-
         $this->moduleConfig = [
             'ticketstore' => [
                 'class' => 'casserver:FileSystemTicketStore', //Not intended for production
                 'directory' => __DIR__ . '../../../../tests/ticketcache',
             ],
         ];
 
+        $this->httpUtils = new Utils\HTTP();
         $this->sspConfig = Configuration::getConfig('config.php');
     }
 
@@ -91,20 +87,23 @@ public function testLogoutWithRedirectUrlOnSkipLogout(): void
 
         // Unauthenticated
         $this->authSimpleMock->expects($this->once())->method('isAuthenticated')->willReturn(false);
-        $this->sspContainer->expects($this->once())->method('redirect')->with(
-            $this->equalTo($urlParam),
-            [],
-        );
 
-        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->sspContainer);
+        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->httpUtils);
 
         $logoutUrl = Module::getModuleURL('casserver/logout.php');
 
         $request = Request::create(
             uri: $logoutUrl,
             parameters: ['url' => $urlParam],
         );
-        $controller->logout($request, $urlParam);
+        $response = $controller->logout($request, $urlParam);
+
+        $callable = $response->getCallable();
+        $method = is_array($callable) ? $callable[1] : 'unknown';
+        $arguments = $response->getArguments();
+        $this->assertEquals('redirectTrustedURL', $method);
+        $this->assertEquals(200, $response->getStatusCode());
+        $this->assertEquals($urlParam, $arguments[0]);
     }
 
     public function testLogoutNoRedirectUrlOnNoSkipLogoutUnAuthenticated(): void
@@ -115,13 +114,16 @@ public function testLogoutNoRedirectUrlOnNoSkipLogoutUnAuthenticated(): void
 
         // Unauthenticated
         $this->authSimpleMock->expects($this->once())->method('isAuthenticated')->willReturn(false);
-        $this->sspContainer->expects($this->once())->method('redirect')->with(
-            $this->equalTo('http://localhost/module.php/casserver/loggedOut'),
-            [],
-        );
 
-        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->sspContainer);
-        $controller->logout(Request::create('/'));
+        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->httpUtils);
+        $response = $controller->logout(Request::create('/'));
+
+        $callable = $response->getCallable();
+        $method = is_array($callable) ? $callable[1] : 'unknown';
+        $arguments = $response->getArguments();
+        $this->assertEquals('redirectTrustedURL', $method);
+        $this->assertEquals(200, $response->getStatusCode());
+        $this->assertEquals('http://localhost/module.php/casserver/loggedOut', $arguments[0]);
     }
 
     public function testLogoutWithRedirectUrlOnNoSkipLogoutUnAuthenticated(): void
@@ -134,17 +136,20 @@ public function testLogoutWithRedirectUrlOnNoSkipLogoutUnAuthenticated(): void
 
         // Unauthenticated
         $this->authSimpleMock->expects($this->once())->method('isAuthenticated')->willReturn(false);
-        $this->sspContainer->expects($this->once())->method('redirect')->with(
-            $this->equalTo($logoutUrl),
-            ['url' => $urlParam],
-        );
 
-        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->sspContainer);
+        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->httpUtils);
         $request = Request::create(
             uri: $logoutUrl,
             parameters: ['url' => $urlParam],
         );
-        $controller->logout($request, $urlParam);
+        $response = $controller->logout($request, $urlParam);
+
+        $callable = $response->getCallable();
+        $method = is_array($callable) ? $callable[1] : 'unknown';
+        $arguments = $response->getArguments();
+        $this->assertEquals('redirectTrustedURL', $method);
+        $this->assertEquals(200, $response->getStatusCode());
+        $this->assertEquals('http://localhost/module.php/casserver/loggedOut', $arguments[0]);
     }
 
     public function testLogoutNoRedirectUrlOnNoSkipLogoutAuthenticated(): void
@@ -158,7 +163,7 @@ public function testLogoutNoRedirectUrlOnNoSkipLogoutAuthenticated(): void
         $this->authSimpleMock->expects($this->once())->method('logout')
             ->with('http://localhost/module.php/casserver/loggedOut');
 
-        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->sspContainer);
+        $controller = new LogoutController($this->sspConfig, $config, $this->authSimpleMock, $this->httpUtils);
         $controller->logout(Request::create('/'));
     }
 
@@ -169,7 +174,7 @@ public function testTicketIdGetsDeletedOnLogout(): void
         $config = Configuration::loadFromArray($this->moduleConfig);
 
         $controllerMock = $this->getMockBuilder(LogoutController::class)
-            ->setConstructorArgs([$this->sspConfig, $config, $this->authSimpleMock, $this->sspContainer])
+            ->setConstructorArgs([$this->sspConfig, $config, $this->authSimpleMock, $this->httpUtils])
             ->onlyMethods(['getSession'])
             ->getMock();