403 and 404 pages get redirected

Now I see why I couldn't reproduce. It only happens if you have a custom 403/404 page defined. I think the patch is close but I'm going to make a few changes. I'll report back this evening with a new update and probably an official release.

@bogdan.dinu: Thanks for your efforts!

I've been too busy to test the latest patch. I will try and review today or tomorrow.

Need to clean up the code and check with phpcs. Initial testing seems to work but would like to fit in a few more test cases.

Hi!

I checked module with phpcs and found these items:

I fixed these items and the patch with the fix follow in attachment.

Regards.

Thanks for the patch!
I did a quick review:

+++ b/src/EventSubscriber/RequireLoginSubscriber.php
@@ -84,14 +90,39 @@ class RequireLoginSubscriber implements EventSubscriberInterface {
+      'system.403',
+      'system.404',
+    ];
+    if (in_array($route_name, $excluded_routes)) {
+      // return;
+    }

I don't know what you really want here

Hi guys!

To help, exists a new patch with the fix in attachment.

Thanks.

Regards.

I'm dealing with the same thing here. I have a custom route with access permissions on it and a custom User Login Path configured at admin/config/system/require-login.

Without #13 I get the too many redirects issue. With this patch, I just get the Access Denied page.

A nitpick, but cleaning up/adding comments to the patch aren't necessary. I believe a separate issue should be created for this.

These @file aren't required here.

1. +++ b/src/EventSubscriber/RequireLoginSubscriber.php
   @@ -1,7 +1,13 @@
   +/**
   + * @file
   + * Contains \Drupal\require_login\EventSubscriber\RequireLoginSubscriber.
   + */
   +
   

2. +++ b/src/Form/RequireLoginSettingsForm.php
   @@ -1,5 +1,10 @@
   +/**
   + * @file
   + * Contains \Drupal\require_login\Form\RequireLoginSettingsForm.
   + */
   +
   

   Same here

+++ b/src/EventSubscriber/RequireLoginSubscriber.php
@@ -84,14 +85,39 @@ class RequireLoginSubscriber implements EventSubscriberInterface {
+    if (in_array($route_name, $excluded_routes)) {
+      //return;
+    }

FYI, with this left commented out - it works for me.

I am working on a patch, but also leaving comments here for others. We also need to add, at least, user.login.http to the excluded routes, otherwise login via endpoint won't work.

As far as I know, we aren't using custom 403/404 pages. I applied patch 13 and at least it fixed the infinite redirect. But when I tried a path other than the home page (as anonymous visitor), instead of going to the login page, it went to the "Access denied" page.

@ExTexan, you can try what I've done here: https://gist.github.com/clrockwell79/b17a83d61972ee403c1dccaf68af1ea5.

@chrisrockwell, thanks for that. I tried that version of the RequireLoginSubscriber.php, but I still just get Access Denied instead of being redirected to the login page.

Was there more to your patch in git that I missed?

Oops, scratch that. I had already disabled the require_login module (so I could do it in a custom module myself)... forgot to re-enable it when I put in the RequireLoginSubscriber.php file. But when I enabled the module just now, I got this...

Recoverable fatal error: Argument 1 passed to Drupal\require_login\EventSubscriber\RequireLoginSubscriber::__construct() must be an instance of Drupal\Core\Config\ConfigFactory, none given, called in /Users/Doug/Projects/Sunland/Site/core/lib/Drupal/Component/DependencyInjection/Container.php on line 264 and defined in Drupal\require_login\EventSubscriber\RequireLoginSubscriber->__construct() (line 30 of modules/require_login/src/EventSubscriber/RequireLoginSubscriber.php).

@ExTexan, no nothing different. I have a couple site specific modifications (just uploaded them so I could see the diff in gist) but this has been working on a production site.

I did find another issue, which you can see in my newest gist in that requests for API are getting access denied, I had to hack together a fix for that. If I ever have time, I'd like to come back to this. Not certain the module owner has time/interest in getting at this though (which I 100% understand and sympathize with).

I came across this issue because I was having infinite redirect loops. I actually want my 403 pages to redirect to /user/login instead of displaying Access Denied. I applied the patch and set the default 403 (access denied) page to / which actually does trigger the login form and redirect that I want.

The only problem is the error in the log:

RuntimeException: Failed to start the session because headers have already been sent by "…/vendor/symfony/http-foundation/Response.php" at line 383. in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start() (line 145 of …/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php) #0 …/web/core/lib/Drupal/Core/Session/SessionManager.php(163): Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start() #1 …/web/core/lib/Drupal/Core/Session/SessionManager.php(194): Drupal\Core\Session\SessionManager->startNow() #2 …/vendor/symfony/http-foundation/Session/Session.php(181): Drupal\Core\Session\SessionManager->save() #3 …/web/core/lib/Drupal/Core/StackMiddleware/Session.php(60): Symfony\Component\HttpFoundation\Session\Session->save() #4 …/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #5 …/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(207): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #6 …/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(121): Drupal\page_cache\StackMiddleware\PageCache->fetch(Object(Symfony\Component\HttpFoundation\Request), 1, true) #7 …/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(75): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true) #8 …/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #9 …/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #10 …/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #11 …/web/core/lib/Drupal/Core/DrupalKernel.php(656): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #12 …/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #13 {main}.

I’m not sure if this is because of my configuration or a problem with the module or patch.

I have created an updated patch that also includes the more recent work by @cuignet.g at https://www.drupal.org/project/require_login/issues/2881063 (Infinite redirect loop) which adds an ImmutableTargetRedirectResponse. That patch and the patch from #13 have some overlap, so I've combined them here using the best of both patches. Hope that helps!

See #2881063: Infinite redirect loop

Applied and tested the patch in #26 and works perfect.

I have applied the latest patch in 26 but instead of redirecting I now get access denied.

I have added user/login as an exclusion but that's the only configuration I've added

Drupal 8.5.0

I went a different route to resolve this issue. The checkUserRedirect() event subscriber is now also checking for the 403/404 HTTP status codes on the current page. Those changes have been committed to the 8.x branch and will be included in a release today.

Committed. Included in release 8.x-1.11.

The new version 8.x-1.11 still caused infinite redirect for me. I've patched it so it now correctly detects the exception code under a 403/404.