Redirect anonymous users to login page from an exception listener instead of in MaintenanceModeSubscriber and restrict access to the my-account link to authenticated users

Hm. This change seems to imply that

1. while you can access /user as anonymous, you are always redirected to /user/login
2. while you can access /user as authenticated, you are always redirected to /user/{id}
3. /user always redirects and no longer shows page output on its own.

I can't remember why we have the special output behavior on /user. IIRC, there have been attempts to change it in similar ways in the past already but they were shot down. Might be worth to search for old/closed issues with keywords "make /user path always redirect"…

#2288911 introduces an exception subscriber (AccessDeniedSubscriber) to redirect authenticated users from user/login and user/register to the users profile page and profile form respectively. Let's also remove the redirection of anonymous users from user to user/login from the maintenance mode subscriber to the UserController::userPage.

What about for symmetry, moving this redirect into an AccessDeniedSubscriber rather than UserController::userPage(), so that we can set access for /user to be only for authenticated users? That would allow us to remove the stupid hack we currently have in user_translated_menu_link_alter(), and the corollary concept of a MenuLinkInterface::isHidden() being carried over into #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module and that per #2256521-91: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module will cause us grief in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

I can't remember why we have the special output behavior on /user

Yeah, I'd love to know that. HEAD's behavior of /user duplicating rather than redirecting to /user/login seems stupid to me, so if digging through issue history turns up something useful, it would be good to get that info into the docs of UserController::userPage().

I think I like the solution in #2306991: MyAccountMenuLink should dynamically adjust route parameters, not hidden property better so we can actually link to the user's account and avoid any redirects.

This looks really great!

1. +++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
   @@ -0,0 +1,90 @@
   +   * @todo: Use UrlGeneratorTrait (see https://www.drupal.org/node/2282161)
   +   */
   +  protected function url($route_name, $route_parameters = array(), $options = array()) {
   +    return $this->urlGenerator->generateFromRoute($route_name, $route_parameters, $options);
   +  }
   

   That todo is in already.

2. +++ b/core/modules/user/user.links.menu.yml
   @@ -3,7 +3,6 @@ user.page:
      menu_name: account
   -  class: Drupal\user\Plugin\Menu\MyAccountMenuLink
    user.logout:
   

   <3

+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php
@@ -39,7 +39,7 @@ public function processInbound($path, Request $request) {
       $path = $this->config->get('system.site')->get('page.front');
       if (empty($path)) {
-        $path = 'user';
+        $path = 'user/login';
       }

Didn't we agreed that should just drop the empty check here? If you specify "" as frontpage, well it is what you wanted, maybe.

    if ($this->account->isAuthenticated()) {
      if ($path == 'user/login') {
        // If user is logged in, redirect to 'user' instead of giving 403.
        $event->setResponse(new RedirectResponse(url('user', array('absolute' => TRUE))));
        return;
      }

There is no reason to redirect to user first in order to later redirect to user/{user}.
It is also odd that this lives inside the MaintenanceModeSubscriber in general, all those user/login user/register user.page etc.
directects should happen

But can't we simply redirect to user.page directly and be done? This doesn't seem to be a big conflict to be honest.

This really looks great!

+++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
@@ -69,7 +69,7 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
-        $event->setResponse(new RedirectResponse(url('user', array('absolute' => TRUE))));
+        $event->setResponse(new RedirectResponse(url('user/' . $this->account->id(), array('absolute' => TRUE))));

Can't we use the url gen. directly?

+++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
@@ -86,9 +86,13 @@ public function testInternalBrowser() {
-    $HTTP_path = $system_path .'/tests/http.php?q=node';
-    $https_path = $system_path .'/tests/https.php?q=node';
+    $HTTP_path = $system_path .'/tests/http.php/user/login';
+    $https_path = $system_path .'/tests/https.php/user/login';

Another dump question, are we sure we don't loose test coverage for sites without mod rewrite?

I really like the general idea to not make the default behaviour of the frontpage not special for anonymous users.

+++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
@@ -0,0 +1,97 @@
+  /**
+   * Returns a redirect response object for the specified route.
+   *
+   * @param string $route_name
+   *   The name of the route to which to redirect.
+   * @param array $route_parameters
+   *   Parameters for the route.
+   * @param int $status
+   *   The HTTP redirect status code for the redirect. The default is 303 See
+   *   Other.
+   *
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   *   A redirect response object.
+   */
+  public function redirect($route_name, array $route_parameters = [], $status = RESPONSE::HTTP_SEE_OTHER) {
+    $url = $this->urlGenerator->generateFromRoute($route_name, $route_parameters, ['absolute' => TRUE]);
+    return new RedirectResponse($url, $status);
+  }

Out of scope but maybe worthwhile for the future: I wonder whether we should move this into the UrlGeneratorTrait?

I think that's regression, /user now does 2 redirects and if user has no access to profiles then wtf could happen.
Also "/user" a path that pointed in a lot of manuals and ddos point (no cache) ...

+++ b/core/modules/system/config/install/system.site.yml
@@ -5,7 +5,7 @@ slogan: ''
-  front: user
+  front: user/login

+++ b/core/modules/user/src/Controller/UserController.php
@@ -131,20 +131,11 @@ public function resetPass($uid, $timestamp, $hash) {
   public function userPage() {
...
+    return $this->redirect('entity.user.canonical', array('user' => $this->currentUser()->id()));

+++ b/core/modules/user/user.routing.yml
@@ -129,9 +129,9 @@ user.page:
   path: '/user'
...
-    _title: 'Log in'
+    _title: 'My account'
...
-    _access: 'TRUE'
+    _user_is_logged_in: 'TRUE'

front page is for anonymous user?

Redirect anonymous

redirect authorized

This still needs an issue summary update.

It's not clear to me at all from the discussion why the extra redirect for anonymous users is necessary. We could override the route used for the page if we have to no?

In Drupal 7 we do not have any of these redirects (I think they mainly got added because the new routing system doesn't have an equivalent of to_arg).

Given #2346671: Two templates necessary in order to customize the login page I would consider that this solves a bug.

re #41

I admit that this looks silly. But please keep in mind that the front-page is set to node as soon as the node module is enabled.

I don't think this is true - in the standard profile the frontpage is node but just installing the node module in minimal does not change the frontpage. I think that this is the correct behaviour as I don't think installing a module should change the front page of your website.

This patch makes complete sense to me - having two login pages at /user and /user/login as we do in HEAD is silly.

The outstanding question is what happens if the user has no access to user profiles - I think that this question is moot because a user can always view their own profile and this patch does nothing to change that.

Assigning to @catch because of his concerns about redirects. I can't see a way of avoiding them.

+++ b/core/modules/user/src/Controller/UserController.php
@@ -131,20 +131,11 @@ public function resetPass($uid, $timestamp, $hash) {
+   *   Returns either a redirect to the user page or the to the user login form.

This doesn't redirect to either the user page or the login form, it's only redirecting to user.canonical.

That does make me wonder why we need to have the exception listener, as opposed to handling the redirect for anonymous users here. Having one redirect in the controller, and one in the exception listener, which checks whether we're on this route anyway seems very indirect.

Also the end result of this, regardless of the above, is that /user only ever redirects. What's the use case for the route now? Wondering if we could maybe get rid of it altogether in a follow-up.

When an anonymous user tries to access /user (the my-account-page), a 403 exception is thrown. Therefore the redirect for anonymous users needs to happen in the exception listener.

That's due to the access on the route though. If we're going to redirect from the route for both cases, we can change the access. The the title callback can switch between "My Account" and "Login".

The route is necessary for the my-account link. That link points to a different profile for each user.

Yeah I'm wondering if we can change that so it actually points to the specific URL for each user - would need some special handling but would likely be less complex than a route that only does a redirect and an exception handler.

This does not work in maintenance mode.

Why exactly doesn't it work?

duplicated redirect logic in the user-controller and maintenance mode subscriber

Returning the redirect response is only a single line of code, I don't think DRY applies here.

That's due to the access on the route though. If we're going to redirect from the route for both cases, we can change the access.

So long as we have a "My account" link that points to the /user route, and we want that link visible for authenticated and not visible for anonymous, then I think the best way to model that is via route access, as this patch is doing, which lets us remove the MyAccountMenuLink::isEnabled() hack as this patch does.

Yeah I'm wondering if we can change that so it actually points to the specific URL for each user

That might be good, but there's no working patch for that. If someone writes a patch for that, then great, but until that happens, I agree with #56's reasoning that #52 is an improvement over HEAD, and doesn't preclude removing the route entirely in a followup if someone wants to pursue that.

I updated the summary and opened #2360971: The user.page route only redirects, so remove it as a follow-up. I think all of catch's other concerns have been answered, but @catch: please correct me if I'm wrong on that.

I was about to RTBC this patch, except:

The behavior when accessing user/login as an authenticated user did not change, except that there is one redirection less.

From what I can tell, that's not true. There's the same amount of redirection because MaintenanceModeSubscriber has this code that runs regardless of maintenance status that is unchanged by this patch:

if ($this->account->isAuthenticated()) {
      if ($path == 'user/login') {
        // If user is logged in, redirect to 'user' instead of giving 403.
        $event->setResponse(new RedirectResponse($this->url('user.page', [], ['absolute' => TRUE])));
        return;
      }

However, due to the above, this code:

+++ b/core/modules/system/config/install/system.site.yml
@@ -5,7 +5,7 @@ slogan: ''
-  front: user
+  front: user/login

creates one additional redirection for authenticated users: instead of HEAD's (<front> = user) -> user/N, the patch does (<front> = user/login) -> user -> user/N. But I think this can be fixed by changing the MaintenanceModeSubscriber code to redirect to entity.user.canonical directly, so setting to "needs work" for that.

Thanks!

Sorry for unRTBCing, but I wanted to fix some docs. Please re-RTBC if these docs are ok.

OK with #2360971: The user.page route only redirects, so remove it open and the extra redirect removed I'm happy with this otherwise, thanks for all the clarifications.

New docs look good to me. Thanks @effulgentsia

index 11c66fd..e8ac5d9 100644
--- a/core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php
+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php
@@ -38,9 +38,6 @@ public function __construct(ConfigFactoryInterface $config) {

@@ -38,9 +38,6 @@ public function __construct(ConfigFactoryInterface $config) {
   public function processInbound($path, Request $request) {
     if (empty($path)) {
       $path = $this->config->get('system.site')->get('page.front');
-      if (empty($path)) {
-        $path = 'user';
-      }
     }
     return $path;
   }

YAY!

Committed/pushed to 8.0.x, thanks!