UHF-6865, UHF-7347: Removed RedirectResponseSubscriber because it conflicts with other redirects, like Tunnistamo logout

helfi_proxy.services.yml

@@ -1,8 +1,4 @@
 parameters:
-  helfi_proxy.valid_proxy_domains:
-    - www.hel.fi
-    - www-test.hel.fi
-    - helfi-proxy.docker.so
   helfi_proxy.valid_origin_domains:
     - hel.fi
     - docker.so
@@ -33,7 +29,7 @@ services:
 
   helfi_proxy.redirect_subscriber:
     class: Drupal\helfi_proxy\EventSubscriber\RedirectResponseSubscriber
-    arguments: ['@helfi_proxy.proxy_manager', '%helfi_proxy.valid_proxy_domains%']
+    arguments: ['@helfi_proxy.proxy_manager']
     tags:
       - { name: event_subscriber, priority: 100 }
 

src/EventSubscriber/RedirectResponseSubscriber.php

@@ -5,6 +5,7 @@
 namespace Drupal\helfi_proxy\EventSubscriber;
 
 use Drupal\Core\Routing\TrustedRedirectResponse;
+use Drupal\Core\Url;
 use Drupal\helfi_proxy\ProxyManagerInterface;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Symfony\Component\HttpFoundation\RedirectResponse;
@@ -21,51 +22,12 @@ final class RedirectResponseSubscriber implements EventSubscriberInterface {
    *
    * @param \Drupal\helfi_proxy\ProxyManagerInterface $proxyManager
    *   The proxy manager.
-   * @param array $validProxyDomains
-   *   The valid proxy domains.
    */
   public function __construct(
     private ProxyManagerInterface $proxyManager,
-    private array $validProxyDomains
   ) {
   }
 
-  /**
-   * Checks whether we need to perform a redirect.
-   *
-   * @param string $url
-   *   The url to check.
-   *
-   * @return bool
-   *   TRUE if page should be redirected.
-   */
-  private function needsRedirect(string $url) : bool {
-    return !in_array(parse_url($url, PHP_URL_HOST), $this->validProxyDomains);
-  }
-
-  /**
-   * Builds the redirect url.
-   *
-   * @param string $url
-   *   The URL to parse.
-   *
-   * @return string
-   *   The redirect URL.
-   */
-  private function buildRedirectUrl(string $url) : string {
-    $uriParts = parse_url($url);
-
-    $responseUrl = vsprintf('https://%s/%s', [
-      $this->proxyManager->getConfig(ProxyManagerInterface::DEFAULT_PROXY_DOMAIN),
-      ltrim($uriParts['path'], '/'),
-    ]);
-
-    if (isset($uriParts['query'])) {
-      $responseUrl = sprintf('%s?%s', $responseUrl, $uriParts['query']);
-    }
-    return $responseUrl;
-  }
-
   /**
    * Redirects to proxy domain by default.
    *
@@ -74,32 +36,31 @@ private function buildRedirectUrl(string $url) : string {
    */
   public function onResponse(ResponseEvent $event) : void {
     if (
-      !$this->validProxyDomains ||
       !$this->proxyManager->isConfigured(ProxyManagerInterface::DEFAULT_PROXY_DOMAIN) ||
-      !$event->getRequest()->isMethod('GET')
+      $event->getResponse() instanceof RedirectResponse
     ) {
-      // Nothing to do if default proxy domain is not defined.
-      // Only redirect on GET requests as well.
+      // Nothing to do if default proxy domain is not defined or the response is
+      // a redirect response already.
       return;
     }
-    $response = $event->getResponse();
+    $request = $event->getRequest();
+    $proxyDomain = $this->proxyManager->getConfig(ProxyManagerInterface::DEFAULT_PROXY_DOMAIN);
 
-    if ($response instanceof RedirectResponse) {
-      $url = $response->getTargetUrl();
+    if ($request->getHttpHost() === $proxyDomain) {
+      // The host matches proxy domain already.
+      return;
     }
-    else {
-      $request = $event->getRequest();
+    $uriParts = parse_url($request->getRequestUri());
+    $options = [];
 
-      $url = vsprintf('%s%s', [
-        $request->getSchemeAndHttpHost(),
-        $request->getRequestUri(),
-      ]);
+    if (isset($uriParts['query'])) {
+      $options['query'] = $uriParts['query'];
     }
+    $url = Url::fromUri(sprintf('https://%s/%s', $proxyDomain, ltrim($uriParts['path'], '/')), $options);
 
-    if (!$this->needsRedirect($url)) {
-      return;
-    }
-    $redirect = new TrustedRedirectResponse($this->buildRedirectUrl($url));
+    $redirect = new TrustedRedirectResponse($url->toString(TRUE)->getGeneratedUrl());
+    $redirect->addCacheableDependency($url)
+      ->addCacheableDependency($this->proxyManager);
     $event->setResponse(
       $redirect
     );
@@ -109,9 +70,7 @@ public function onResponse(ResponseEvent $event) : void {
    * {@inheritdoc}
    */
   public static function getSubscribedEvents() : array {
-    // This must be before core's RedirectResponseSubscriber, so we don't break
-    // any other redirects made by other modules, like ?destination.
-    $events[KernelEvents::RESPONSE][] = ['onResponse', 1];
+    $events[KernelEvents::RESPONSE][] = ['onResponse'];
 
     return $events;
   }

tests/src/Kernel/RedirectResponseSubscriberTest.php

@@ -102,17 +102,6 @@ private function createRequest(string $uri, array $parameters = []) : Request {
     ]);
   }
 
-  /**
-   * Test expected default values.
-   */
-  public function testDefaults() : void {
-    $this->assertEquals([
-      'www.hel.fi',
-      'www-test.hel.fi',
-      'helfi-proxy.docker.so',
-    ], $this->container->getParameter('helfi_proxy.valid_proxy_domains'));
-  }
-
   /**
    * Make sure we get an HTTP 200 response when proxy is not enabled.
    *
@@ -127,7 +116,7 @@ public function testNoRedirect() : void {
   }
 
   /**
-   * Tests that existing response url is used for redirect responses.
+   * Tests redirects.
    *
    * @covers ::getSubscribedEvents
    * @covers ::onResponse
@@ -137,39 +126,16 @@ public function testRedirectResponse() : void {
     $request = $this->createRequest('/user');
     $response = $this->getHttpKernelResponse($request);
 
+    // Make sure other redirects are dealt first, like /user -> /en/user/login
+    // before redirecting to proxy domain.
     $this->assertEquals(Response::HTTP_FOUND, $response->getStatusCode());
-    $this->assertEquals('https://www.hel.fi/en/user/login', $response->headers->get('location'));
-  }
-
-  /**
-   * Make sure we always get redirected to proxy path.
-   *
-   * @covers ::onResponse
-   * @covers ::getSubscribedEvents
-   */
-  public function testRedirectProxyPaths() : void {
-    $prefixes = [
-      'fi' => 'test-fi',
-      'sv' => 'test-sv',
-      'en' => 'test-en',
-    ];
-    $this->setProxyDomain('www.hel.fi');
-    $this->config('helfi_proxy.settings')
-      ->set(ProxyManagerInterface::PREFIXES, $prefixes)
-      ->save();
-
-    $expectedPaths = ['user' => 'user/login', 'user/login' => 'user/login'];
-
-    foreach ($expectedPaths as $path => $expectedPath) {
-      foreach ($prefixes as $language => $prefix) {
-        $request = $this->createRequest(sprintf('/%s/%s', $language, $path));
-        $response = $this->getHttpKernelResponse($request);
+    $this->assertEquals('http://localhost:8888/en/user/login', $response->headers->get('location'));
 
-        $expectedUrl = sprintf('https://www.hel.fi/%s/%s/%s', $language, $prefix, $expectedPath);
-        $this->assertEquals(Response::HTTP_FOUND, $response->getStatusCode());
-        $this->assertEquals($expectedUrl, $response->headers->get('location'));
-      }
-    }
+    // Make sure we get redirected to proxy domain after other redirects.
+    $request = $this->createRequest('/en/user/login');
+    $response = $this->getHttpKernelResponse($request);
+    $this->assertEquals(Response::HTTP_FOUND, $response->getStatusCode());
+    $this->assertEquals('https://www.hel.fi/en/user/login', $response->headers->get('location'));
   }
 
 }

tests/src/Unit/AssetHttpMiddlewareTest.php

@@ -7,6 +7,7 @@
 use Drupal\helfi_proxy\HttpMiddleware\AssetHttpMiddleware;
 use Drupal\helfi_proxy\ProxyManagerInterface;
 use Drupal\Tests\UnitTestCase;
+use Prophecy\PhpUnit\ProphecyTrait;
 use Symfony\Component\HttpFoundation\ParameterBag;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
@@ -20,6 +21,8 @@
  */
 class AssetHttpMiddlewareTest extends UnitTestCase {
 
+  use ProphecyTrait;
+
   /**
    * Tests that XML response stays intact.
    *