"Login" link shows up for logged-in users as well

Cannot replicate here today - can you provide a screenshot of where you see this link?

Can replicate this behavior. See the updated issue description. As possibly quite a lot of websites rely on this behavior I'd suggest to have the same behavior in D8?

Not sure if it's the right approach, but this patch makes the old behavior available. Can somebody please review the patch. Also not 100% sure about naming conventions etc. Thanks!

By the way I tried using:

requirements:
    _user_is_logged_in: 'FALSE'

But that wouldn't work. I assume because LoginStatusCheck.php returns static::DENY in this case and to allow something based on a DENY would be inconsequent? Not sure .. :-)

Found out why:

$request->attributes->get('_menu_admin')

has to be added as well. So you're actually allowed to create that link in the admin UI. This patch fixes it.

Even if your condition would successfully validate for anonymous users, you would still have the problem, that you can't even add this link in the admin ui anymore, as you're not allowed to access that path.

+++ b/core/modules/user/user.routing.yml
@@ -145,7 +145,7 @@ user.login:
+    _user_is_logged_in: 'FALSE'

See \Drupal\user\Access\LoginStatusCheck, it doesn't actually care about the value. Only the key matters. so 'TRUE' and 'FALSE' are the same here.

The patch by @pancho was great, I have made a few tweaks:

• Moved the new access check class to the correct folder (now that PSR4 is in)
• Renamed the class to AnonymousStatusCheck and also renamed LoginStatusCheck to AuthenticatedStatusCheck for consistency. I believe the previous naming was going to be confusing in the future.
• Added missing docblock

Created a new menu link that pointed to user/login and verified that only anonymous users could see this in the menu.

17: 2267603-17.patch queued for re-testing.

This patch applied and worked properly for me. Login menu item displays for anonymous users and not for logged in users. Someone else might want to take a look at the code, but so far as I can tell, this is good.

$request->attributes->get('_menu_admin') 

Is something that needs to be killed, though I'm not sure how.

Just rerolled of #17.

Please assign a ticket to yourself er.pushpinderrana when you start working on it. I just wasted half an hour or so. :-/

The problem with:

$request->attributes->get('_menu_admin')

is imho out of scope of this ticket, as this feature is also used by core\modules\menu_ui\src\MenuForm.php.

I just tested the patch from #25 and it works as expected. Can somebody else review it please? I think it's ready to be RBTC then.

@Stefan Lehmann, sorry for that. Unfortunately I started on this issue when this was not assigned to you and I would assuming it just take a while to rerolled this so forget to assign to me. Thankyou for your time and kindness.

No problemo er.pushpinderrana :-) ..

Ok .. actually I think this is finally RBTC!

We need some tests for this - so we don;t break it again.

I had a crack at this, 3 tests still fail, and I have no idea what I am doing wrong. Can someone more fluent in the menu system help here? Only marking as needs review to kick the testbot.

Tests run with

sudo -u www-data php core/scripts/run-tests.sh --verbose --url http://d8.localhost/ --file core/modules/user/src/Tests/UserMenuLinksTest.php

This issue is strongly related to #2306991: MyAccountMenuLink should dynamically adjust route parameters, not hidden property

@Pancho, regarding the comment #7, I think we can remove the appended text. They are unnecessary and waste computing power to render them.

+++ b/core/modules/user/user.routing.yml
@@ -145,7 +145,7 @@ user.login:
-    _access: 'TRUE'
+    _user_is_anonymous: 'TRUE'

Current on dev HEAD is:
_user_is_logged_in: 'FALSE'

So, we can use that status to exclude link from showing up on logged in user.

I think we do have a bug here.
_user_is_logged_in: 'FALSE' is not working properly, while _user_is_logged_in: 'TRUE' is working.
(Change it to TRUE, we can make the login link show up for authenticated user only. But not working on the other way.)

Need to redo the patch.

Hi @tim.plunkett regarding #12, I think Value TURE and FALSE is making a difference.

In file user.routing.yml if we change

user.logout:
  path: '/user/logout'
  defaults:
    _controller: '\Drupal\user\Controller\UserController::logout'
  requirements:
    _user_is_logged_in: 'TRUE'

To:

user.logout:
  path: '/user/logout'
  defaults:
    _controller: '\Drupal\user\Controller\UserController::logout'
  requirements:
    _user_is_logged_in: 'FALSE'

The "log out" link will appear for anonymous users.

Well, this change will prevent a user from logging out. We can do something to fix it in LoginStatusCheck.php.

It only happens to the role having permission 'link to any page'. Here is the patch to remove this check.

If we want to keep the way it is now. Then we can close this ticket. The regular authenticated users do not have this problem. The only user has the permission to access link to any page have the "problem".

Here is a least impact change which replace the checkAccess with checkDisplay method.

This is a serious regression, suppose there should be a fix without API changes, maybe menu caching is broken somehow.

Added screenshot to IS

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -200,9 +200,9 @@ protected function collectNodeLinks(array &$tree, array &$node_links) {
   -  protected function menuLinkCheckAccess(MenuLinkInterface $instance) {
   +  protected function menuLinkCheckAccess(MenuLinkInterface $instance, $nonedisplay = TRUE) {
   

   I find this argument name non-descriptive
   also that needs doc-blco update, change record about AOI change

2. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -265,4 +265,33 @@ public function flatten(array $tree) {
   +  public function checkDisplay(array $tree) {
   

   That's another api change

3. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -265,4 +265,33 @@ public function flatten(array $tree) {
   +      // Other menu tree manipulators may already have calculated access, do not
   +      // overwrite the existing value in that case.
   +      if (!isset($element->access)) {
   +        $tree[$key]->access = $this->menuLinkCheckAccess($element->link, FALSE);
   +      }
   

   Looks that a reason - do calculate access only once

I have to ask: does this really need to be fixed? If it only affects users with the 'link to any page' permission, which should be really rare, the only probable impact is on the admin role. And to be honest, what's the big deal?

Hi @andypost,

Thanks for the comment. Is your attached image from applying patch of #38, or #37 or none of them? I thought change at #38 had been safe enough not causing a regression.

@wuinfo I added a screenshot without patch to show the regression

-  protected function menuLinkCheckAccess(MenuLinkInterface $instance) {
+  protected function menuLinkCheckAccess(MenuLinkInterface $instance, $nonedisplay = TRUE) {

I find this argument name non-descriptive

@andypost, agree the argument name needs to be changed. What about 'permission_override' or just 'override'.

We have one more problem here: Can create menu link with an invalid URL

Here is the test patch.

Test failed as we expected.

#2503755: Switch from user login block to login menu link and search block in standard profile is in but behavior remains same for this issue.

The problem here is that you have to reuse the existing user login link which is provided by default.
With that particular link, it works as expected.

Indeed. And that was fixed in #2503755: Switch from user login block to login menu link and search block in standard profile.

Sorry, if you create a menu with a link '/user/login' it will still be displayed even though you are logged in. So the behavior hasn't changed.

@dcrocks
Right you need to reuse the existing login link.

Still don't quite understand what you mean. How do you 'reuse' the link? The link is part of the user account block, which is not what is wanted. Simply adding /user/login' link to a new or existing menu creates the problem. Placing the mouse over the link always produces '/.../user/login'.

Any news on this?

@dawehner: maybe we need to add some explanation to the expected behaviour.

For example, if we need to avoid the "log out" link to appear in the user menu (or any other menu), this is not possible to use the default link.

So logically we disable the default link and create a new menu item with the path /user/login, so anonymous users can log in. As authenticated users are not able to log in twice, the link should not appear...

I hope those explanations may help to understand the criticality of this issue, as we need to do some hacks to get the expected result...

I've also just ran into this issue, which is very un-intuitive and restrictive in the current functionality. The text of the default link can't be (within the interface) altered, which some projects will need the ability to change. You can't also duplicate this link without recreating it in custom code, meaning you only get a simple Log In/Out link in one place on the site. To me, the most intuitive and flexible option for site builders is to have this check on the routes. `/user/logout` already seems to do this, if you're not logged in that route doesn't show, because an anonymous user can't log out. Shouldn't the `/user/login` route function with the same logic?

Re-rolled #38 for D8.8.x

Hi, I applied patch #65 to my Drupal 8.8.1 but it does not work for me. I still have the issue.

The logout link works for me. It is shown only for logged in users and hidden for anonymous users. But the login link is always shown even for logged in users.

Any ideas? Thanks in advance!

Hii @ajFernandez, @mcgowanm i checked wrt 8.8.x and patch #65 working fine for me.
moving in Needs review.

Hi @swatichouhan012, I am trying to create a link to /user/login in my main menu, but this link is always visible. However, a link to /user/logout in my main menu is only visible when user is logged in. Could you try if the patch is working with /user/login link in any menu or it only works with user menu?

Thanks in advance!

Rerolled against 8.9.x ,

+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -196,9 +196,9 @@ protected function collectNodeLinks(array &$tree, array &$node_links) {
+  protected function menuLinkCheckAccess(MenuLinkInterface $instance, $nonedisplay = TRUE) {

An @param needs to be added to the doc block. Surely, the name 'nonedisplay' can be improved.

I added a before and after screenshot. The IS needs an update too. Also needs a test. Based on the original IS this is a regression, adding that tag too.

#70 working fine. Re-adding for 9.1.x with suggestion #72

@Bunty Badgujar, Tested on 9.1.x

Patch didn't work for me, the issue still persists on 9.1.x

@pankaj.singh, Thanks for testing the patch. The problem is seen on the home page. Did you follow the steps to reproduce in the IS? I've updated it.

+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -192,13 +192,15 @@ protected function collectNodeLinks(array &$tree, array &$node_links) {
+   *   Allow force check on menu link.

This is an area I don't know much about but that comment is vague to me. I want to know why type of check is being forced. It is currently also optional, so needs (optional). Something like "(optional) Set TRUE to .... on the menu link. Defaults to FALSE."

This still needs a test.

@quietone, I think I attached the wrong screenshot in my last comment #74.
Attaching the correct screenshot here.
As the issue still visible, also once clicked on the login button it redirects to my account page.

@pankaj.singh, I've just checked again and the patch works correctly and the screen shots I uploaded in #72 are still correct. Did you clear cache after applying the patch?

@quieton, yes I cleared the cache post applying the patch. Also, I retested again but had no luck.

Hi @pankaj.singh,

First replicate this issue:
- Goto: /admin/structure/menu/manage/main
- Add a menu link "Login" to the path "user/login"
- Go to the home page, , whilst logged in, you should have 2 links "Home" and "Login"

Then apply the patch and clear site level cache and browser cache ( ctrl + R ). Then it will work for you. Please try again. If not then let us know all environment versions.

I've tested #73, it's working fine for me.

@quieton @pradeepjha I was looking for the possible reasons why it's not happening on my end. So, I just re-setup 9.1.x-dev on my local reproduced the issue and applied the patch given in #73.

Patch worked for me as well, apologies for the inconvenience. RTBC+1

Moving need review. Lets see if we can move it to RTBC.

Verified and tested by applying the patch #73.It looks good.Moving it to RTBC.

Steps to test -
1. Go to admin site.
2. Go to /admin/structure/menu/manage/main
3. Add a menu link "Login" to the path "user/login"
4. Go to the home page, , whilst logged in, you should have 2 links "Home" and "Login"
5. Verify

Patch Applied -

Configuration -

Before Patch -

After Patch -

1. This feels like there are competing priorities - there are other times when you'd want a link to show if the user has the link to any page permission. I feel that maybe a better way to go is to special case the log in link in menuLinkCheckAccess.
2. We still need to add some automated tests for this.

3. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -192,13 +192,15 @@ protected function collectNodeLinks(array &$tree, array &$node_links) {
   +   * @param bool $force_access_check
   +   *   Allow force check on menu link.
   

   Allow force check on menu link can be improved. I think we should document that this results in the check ignoring the 'link to any page' permission. Also the name of the variable is not great because checking the permission is a kind of access check. Not sure what a better name is.

4. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -192,13 +192,15 @@ protected function collectNodeLinks(array &$tree, array &$node_links) {
   -    if ($this->account->hasPermission('link to any page')) {
   +    if ($this->account->hasPermission('link to any page') && !$force_access_check) {
   

   This check should be the other way around - we should check the local variable first.

Working on automated tests.

tried to cover point 85.2,85.4 and 85.3 but

Allow force check on menu link.

still needs to replace.

Replaced deprecated methods from #86 and also moved code to a specific method.

+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
@@ -110,6 +119,7 @@ public function testMenu() {
+    $this->testLoginLinkVisibility();

@@ -1047,4 +1057,19 @@ public function testMenuUiWithPendingRevisions() {
+  /**
+   * Verify if any menu link with login URL is displayed after login.
+   */
+  public function testLoginLinkVisibility() {

This test is now running twice - because it starts with test... the best thing here is to remove it from testMenu or call it doTest...

I think i'd remove the call in testMenu... and also move the user create into the test menu itself and out of setUp.

I still think we should address #85.1 by special casing the login and logout links and not add this whole extra menu manipulator,

Setting to NW for the points in #90.

Working on changes requested in #90

I have moved user with permission 'link to any page' from setUp to testLoginLinkVisibility() as this is the only test which uses this permission.
We will not be able to moe the other create user out of setUp as it has been used in many tests.

@alexpott, as per you comments in #85.1, are you suggesting that a separate permission for login link than using link to any page permission?

Tested via updated test steps and patch #93
It's working fine and can be moved to RTBC

Adding tage 'Bug Smash Initiative'.

Applied patch #93 . Its works fine.Now the login link will be gone
before patch :

after patch:

RTBC

re #93 - no I'm not suggesting a special permission. I'm suggesting the we hard code special behaviour for these links

+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -192,13 +192,15 @@ protected function collectNodeLinks(array &$tree, array &$node_links) {
-    if ($this->account->hasPermission('link to any page')) {
+    if ((!$link_access) && ($this->account->hasPermission('link to any page'))) {

Change to !in_array($link->getRouteName(), static::FORCE_ACCESS_CHECK, TRUE) && $this->account->hasPermission('link to any page')

and

static::FORCE_ACCESS_CHECK = [
  'user.logout',
  'user.login'
];

or something like that.

Addressed #99
Sorry paulocs, I have not assigned this issue to myself and started working on it.

Attaching a new patch based on comment #99. Ps: not sure if I declare the const in the right place, please review.

Also removing unnecessary tags.

So about patch #101, as @alexpott pointed on comment #90, there is no need to create another menu verification and that is why I removed checkDisplay() in patch #103.

Small typo:

diff -u b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
--- b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -42,4 +42,14 @@
 
   /**
+   * The routes to force to chack access.

+ * The routes to force to check access.

Worked on comment #105

I did some tests and if we just replace if ((!$link_access) && ($this->account->hasPermission('link to any page'))) to !in_array($link->getRouteName(), static::FORCE_ACCESS_CHECK, TRUE) && $this->account->hasPermission('link to any page'), the site administrator will not be able to see the login link in the menu link list. So patch #88 is a better solution.

One thing that patch #88 is missing is when the user is logged out, the logout link is still displayed. We should fix this and add tests for it.

Hi @paulocs,
I have added a patch for logout issue. Please have a look. But 2 test cases are failed. Will check that.

Patch #111 does not work because the login link are not displayed in the menu list.
I added a test case for when the user is logout and it should not show the logout link.

I did not addressed 85.1 or 99 (same observation) because I did not find a way to work it properly. I tried it in patch #103 but patch is not show the login link the menu list.

+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -212,6 +214,10 @@ protected function menuLinkCheckAccess(MenuLinkInterface $instance) {
+    // Checking access for Anonymous user logout.
+    if ($this->account->id() === 0 && $instance->getRouteName() === 'user.logout') {
+      $access_result = AccessResult::forbidden();
+    }

This can't be necessary given the route declaration.

user.logout:
  path: '/user/logout'
  defaults:
    _controller: '\Drupal\user\Controller\UserController::logout'
  requirements:
    _user_is_logged_in: 'TRUE'

There's part of me that I think given the it's 100% related to the link to any page permission that we're in works as designed territory. FWIW there are places other than system menu block that needs updating to use the new display manipulator... for example \Drupal\system\SystemManager::getAdminBlock() and the toolbar (\Drupal\toolbar\Controller\ToolbarController::preRenderAdministrationTray()) and System overview page (\Drupal\system\Controller\SystemController::overview())

So if it is worked as designed, we should add only the _user_is_logged_in: 'TRUE'requirement key to the user.logout route because for now, logout link is displayed when user not logged in.

I tried the suggestion in #114 and it worked for a non-admin user but not for the admin. For the admin. the 'login' link is always displayed.

Patch on comment #113 works fine. This issue is also resolved with the same patch: https://www.drupal.org/project/drupal/issues/3182970#comment-14037003

Patch #113 works as intended!

I tried to apply the patch and have the following message on the terminal.

$ git apply -v 2267603-113.patch
Checking patch core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php...
Hunk #1 succeeded at 190 (offset -2 lines).
Hunk #2 succeeded at 212 (offset -2 lines).
Hunk #3 succeeded at 264 (offset -2 lines).
Checking patch core/modules/menu_ui/tests/src/Functional/MenuUiTest.php...
Hunk #1 succeeded at 1071 (offset 23 lines).
Checking patch core/modules/system/src/Plugin/Block/SystemMenuBlock.php...
Applied patch core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php cleanly.
Applied patch core/modules/menu_ui/tests/src/Functional/MenuUiTest.php cleanly.
Applied patch core/modules/system/src/Plugin/Block/SystemMenuBlock.php cleanly.

I think This patch needs re-roll.

I have validated the patch by #114, and its working as expected.

This _user_is_logged_in: 'TRUE' argument only works with non -authenticated users. The logout issue is addressed here : https://www.drupal.org/project/drupal/issues/3182970#comment-14037003.

I have Rerolled the patch.

Uploaded the wrong patch in #121, Please follow this patch.

Thanks.

Verified and tested patch #122.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: /admin/structure/menu/manage/main
# Add a menu link "Login" to the path "user/login"
# Go to the home page with the logged-in user
# User should not see "Log in" navigation in Menu once the user is Authenticated.
# User should see "Log in" navigation in the menu once the user is Non-Authenticated.

Looks good to me.
Can be a move to RTBC
Please refer attached screenshots for Before and After patch.

See that this is RTBC I cam back to find what the resolution of comment #114 is. In #vakulrai says that they have validated the by #114, and its working as expected. Can you explain what 'validated by' means?

I think there needs to be answers to the questions in #114, particularly when a committer is suggesting this is working as designed. Why is that block of code necessary? And further what about the other places that this need to be fixed that are mentioned in the last paragraph on #114. Do they need fixes as well.

Setting back to needs work.

seems like the patch is not working in drupal 9.2.10

Applied patch #122

working and applied successfully
after patch login button has been gone
Thanks for the patch

Patch is not working for admin users in drupal 9.3.9.

@drupal-ramesh

I think this is because you are logged in as administrator (supe-user). Try a other non-admin account and you will see it is ok.
(I think?)

I can't reproduce this issue like reported anymore.
/user/login (set in the menu link) is only accessible for guests! So works as expected for me.

But now there's the issue, that /user page is accessible for guests (redirects to user/login), but the menu item linking /user (which is expected to do the same as the page) isn't visible for guests.
Is that a separate issue or part of the discussion here?

To sum up my tests:
Expected / actual for guests:

• /user => accessible | not accessible (bug)
• /user/login => accessible | accessible
• /user/logout => not accessible | not accessible
• /user/edit => not accessible | not accessible

Expected / actual for authenticated:

• /user => accessible | accessible
• /user/login => not accessible | not accessible
• /user/logout => accessible | accessible
• /user/edit => accessible | accessible

accessible = shown
not accessible = hidden

Can someone else confirm this in 11.x? How should we proceed?

I was having this issue in Drupal 10.0.8 - user/login path on menu item was showing for logged in user - Then I applied the patch in #122 and cleared my cache. The login link was not visible for logged in users and was visible for logged out users. Hope this is helpful in pushing this forward.

Struggling to see how this is different from #2463753: [regression] Do not bypass route access with 'link to any page' permissions for menu links. Seems like that solves this problem too?

#2463753: [regression] Do not bypass route access with 'link to any page' permissions for menu links fixes this and also adds test coverage, agreed with @bkosborne and marking duplicate.