From 899f259c0c4c5627950c33255b4243b25044bf93 Mon Sep 17 00:00:00 2001 From: Jonathan Hedstrom Date: Mon, 4 May 2015 12:10:57 -0700 Subject: [PATCH] Remove flood-based redirect loop detection. --- redirect.services.yml | 2 +- .../RedirectRequestSubscriber.php | 13 +------ src/RedirectChecker.php | 27 +------------ tests/src/Unit/RedirectCheckerTest.php | 39 +++---------------- 4 files changed, 8 insertions(+), 73 deletions(-) diff --git a/redirect.services.yml b/redirect.services.yml index f7dbb57..9892484 100644 --- a/redirect.services.yml +++ b/redirect.services.yml @@ -6,7 +6,7 @@ services: - { name: backend_overridable } redirect.checker: class: Drupal\redirect\RedirectChecker - arguments: ['@config.factory', '@state', '@flood'] + arguments: ['@config.factory', '@state'] redirect.request_subscriber: class: Drupal\redirect\EventSubscriber\RedirectRequestSubscriber arguments: ['@redirect.repository', '@language_manager', '@config.factory', '@redirect.checker', '@router.request_context'] diff --git a/src/EventSubscriber/RedirectRequestSubscriber.php b/src/EventSubscriber/RedirectRequestSubscriber.php index 3974ba3..00f9aa6 100644 --- a/src/EventSubscriber/RedirectRequestSubscriber.php +++ b/src/EventSubscriber/RedirectRequestSubscriber.php @@ -91,8 +91,7 @@ public function onKernelRequestCheckRedirect(GetResponseEvent $event) { $this->context->fromRequest($request); try { - $redirect = $this->redirectRepository->findMatchingRedirect($path, $request_query, $this->languageManager->getCurrentLanguage() - ->getId()); + $redirect = $this->redirectRepository->findMatchingRedirect($path, $request_query, $this->languageManager->getCurrentLanguage()->getId()); } catch (RedirectLoopException $e) { \Drupal::logger('redirect')->warning($e->getMessage()); @@ -105,16 +104,6 @@ public function onKernelRequestCheckRedirect(GetResponseEvent $event) { if (!empty($redirect)) { - // If we are in a loop log it and send 503 response. - if ($this->checker->isLoop($request)) { - \Drupal::logger('redirect')->warning('Redirect loop identified at %path for redirect %id', array('%path' => $request->getRequestUri(), '%id' => $redirect->id())); - $response = new Response(); - $response->setStatusCode(503); - $response->setContent('Service unavailable'); - $event->setResponse($response); - return; - } - // Handle internal path. $url = $redirect->getRedirectUrl(); if ($this->config->get('passthrough_querystring')) { diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php index d5f1551..c78ed9b 100644 --- a/src/RedirectChecker.php +++ b/src/RedirectChecker.php @@ -7,7 +7,6 @@ namespace Drupal\redirect; use Drupal\Core\Config\ConfigFactoryInterface; -use Drupal\Core\Flood\FloodInterface; use Drupal\Core\State\StateInterface; use Symfony\Cmf\Component\Routing\RouteObjectInterface; use Symfony\Component\HttpFoundation\Request; @@ -27,15 +26,9 @@ class RedirectChecker { */ protected $state; - /** - * @var \Drupal\Core\Flood\FloodInterface - */ - protected $flood; - - public function __construct(ConfigFactoryInterface $config, StateInterface $state, FloodInterface $flood) { + public function __construct(ConfigFactoryInterface $config, StateInterface $state) { $this->config = $config->get('redirect.settings'); $this->state = $state; - $this->flood = $flood; } /** @@ -75,22 +68,4 @@ public function canRedirect(Request $request) { return $can_redirect; } - /** - * Checks if for the current session we are not in a loop. - * - * @param Request $request - * Current request object. - * - * @return bool - * TRUE if a redirect loop has been identified. - */ - public function isLoop(Request $request) { - if ($this->flood->isAllowed('redirection', 5, 15)) { - $this->flood->register('redirection', 60); - return FALSE; - } - else { - return TRUE; - } - } } diff --git a/tests/src/Unit/RedirectCheckerTest.php b/tests/src/Unit/RedirectCheckerTest.php index 2a9fb1f..acf5a9c 100644 --- a/tests/src/Unit/RedirectCheckerTest.php +++ b/tests/src/Unit/RedirectCheckerTest.php @@ -34,10 +34,7 @@ public function testCanRedirect() { ->with('system.maintenance_mode') ->will($this->returnValue(FALSE)); - $flood = $this->getMockBuilder('Drupal\Core\Flood\FloodInterface') - ->getMock(); - - $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $flood); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); // All fine - we can redirect. $request = $this->getRequestStub('index.php', 'GET'); @@ -59,7 +56,7 @@ public function testCanRedirect() { ->with('system.maintenance_mode') ->will($this->returnValue(TRUE)); - $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $flood); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); $request = $this->getRequestStub('index.php', 'GET'); $this->assertFalse($checker->canRedirect($request), 'Cannot redirect if maintenance mode is on'); @@ -72,7 +69,7 @@ public function testCanRedirect() { ->with('system.maintenance_mode') ->will($this->returnValue(FALSE)); - $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $flood); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); $route = $this->getMockBuilder('Symfony\Component\Routing\Route') ->disableOriginalConstructor() @@ -88,40 +85,13 @@ public function testCanRedirect() { // We are at admin path with global_admin_paths set to TRUE. $config['redirect.settings']['global_admin_paths'] = TRUE; - $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $flood); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); $request = $this->getRequestStub('index.php', 'GET', array(RouteObjectInterface::ROUTE_OBJECT => $route)); $this->assertTrue($checker->canRedirect($request), 'Can redirect a admin with global_admin_paths set to TRUE'); } - /** - * Tests the loop checker. - */ - public function testIsLoop() { - $state = $this->getMockBuilder('Drupal\Core\State\StateInterface') - ->getMock(); - $request = $this->getRequestStub('index.php', 'GET'); - - $flood = $this->getMockBuilder('Drupal\Core\Flood\FloodInterface') - ->getMock(); - $flood->expects($this->any()) - ->method('isAllowed') - ->will($this->returnValue(TRUE)); - - $checker = new RedirectChecker($this->getConfigFactoryStub(), $state, $flood); - $this->assertFalse($checker->isLoop($request), 'Not in a loop'); - - $flood = $this->getMockBuilder('Drupal\Core\Flood\FloodInterface') - ->getMock(); - $flood->expects($this->any()) - ->method('isAllowed') - ->will($this->returnValue(FALSE)); - - $checker = new RedirectChecker($this->getConfigFactoryStub(), $state, $flood); - $this->assertTrue($checker->isLoop($request), 'In a loop'); - } - /** * Gets request mock object. * @@ -149,4 +119,5 @@ protected function getRequestStub($script_name, $method, array $attributes = arr return $request; } + }