Anonymous users receive access denied when attempting to logout

Do you have any rules setup on user logout?

Reproduced in 8.0-alpha14 (also happens if logging out from 2 browser tabs). Alternative solution would be going to <front> if already logged out.

That said, while the current behavior is harmless, I find it disconcerting and unfriendly, so added the usability tag.

Duplicate of [#2192401] https://drupal.org/node/2192401

Closed by mistake. #2192401 is a support request, not an issue.

Here is a patch that removes requirements on user/logout route, and in user_logout() checks if the user is not anonymous to finish log out and session destroy.

Wrong patch. Here's right one.

+++ b/core/modules/user/user.routing.yml
@@ -10,8 +10,6 @@ user.logout:
   path: '/user/logout'
   defaults:
     _controller: '\Drupal\user\Controller\UserController::logout'
-  requirements:
-    _user_is_logged_in: 'TRUE'
 

I would love to have route level based checking, as it will just work better. For example the other code might return a 200 even it should return a 403. IN general this will fail completly, because you always need some access checking at least.

I see. I'm not sure how we could make the best of both worlds happen, I'll try to do some research.

Reworded IS/title

I'm not sure I follow #2193803-9: Anonymous users receive access denied when attempting to logout. Here is a new patch.

Partial review.

- Can reproduce initial problem
- issue fixed by patch #15 (8.2.x)
- Passes tests (+ no issues with manually logout as an authenticated user)
- Seems in scope
- Automated coding standard checks OK
- No UI changes

Probably needs a new test?

Test added.

+class UserLogoutTest extends WebTestBase {

New tests should use functional browserbase.

+ * Test anonymous users can visit /user/logout without Access Denied.

*~test both logged in, and not logged in can access /user/logout.

(create two test methods: testAnonymousUserLogout, testUserLogout)

+    $request = Request::create('/user/logout');
+    $request->setFormat('html', ['text/html']);
+
+    /** @var \Symfony\Component\HttpKernel\HttpKernelInterface $kernel */
+    $kernel = \Drupal::getContainer()->get('http_kernel');
+    $response = $kernel->handle($request)->prepare($request);

If you're going to fake a request, you may as well use kernel test base...

Though, I think these tests should be browser tests.

+ $this->assertNotEqual($response->getStatusCode(), Response::HTTP_FORBIDDEN,

assertNoting anything is dangerous. Avoid when you can.

This will no fail if for example, something bad is causing the system to 404 everything (or any other non 403 code)

The test is to assert that all users *can* access. So, assertEqual 200.

+ 'Anonymous users may view /user/logout without 403 error'
+ );

Assert message shouldn't be multiline. Strict 80 char wrapping is for PHP comments.

[EDIT - ignore - wrong upload]

Try again…

+   * Test authenticated users can logout successfully.
+   */
+  public function testUserLogout() {
+    // Login as an ordinary authenticated user.
+

Inline comment is unnecessary given the method comment directly above.

+    $account = $this->drupalCreateUser([]);

empty array for first arg.

+ public function testUserLogout() {

Perhaps 'authenticated' is better than 'user'. Makes for easier comparison.

+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+  }

Empty method.

Yep, all sensible.

Test-only patch at @dpi's request

#26 Thanks! Looks good

+++ b/core/modules/user/src/Tests/UserLogoutTest.php
@@ -0,0 +1,33 @@
+    $this->assertSession()->statusCodeEquals(200);

We've had bugs with fatal errors showing up as 200s before (and this still isn't 100% solved), so this could use a positive assertion, i.e. that we're on the front page, as well as the check for the 200.

I've been hit by this many, many times, so would be nice to fix it.

addressEquals() assertions added.

+ $this->assertSession()->addressEquals('/');

Url::fromRoute('<front>')->toString(); reads better / self documenting

Generate URLs for the tests with Url::fromRoute()

Failure was:

Current page is "/", but "/checkout/" expected.

Attempting to fix by copying this patch…

+    $this->assertSession()->addressEquals(Url::fromRoute('<front>')
+                                             ->setAbsolute()
+                                             ->toString());

Method call indents are maximum 2 spaces.

You can choose to do it all on one line, where the method calls are simple (usually just one method).

However, I suggest you using our chaining style, creating a variable beforehand:

$expected_url = j->k()
  ->l()
  ->m();
addressEquals($expected_url);

Coding style changed as per #35.

+++ b/core/modules/user/src/Controller/UserController.php
--- /dev/null
+++ b/core/modules/user/src/Tests/UserLogoutTest.php

+++ b/core/modules/user/src/Tests/UserLogoutTest.php
@@ -0,0 +1,44 @@
+class UserLogoutTest extends BrowserTestBase {

Note: BrowserTests should be placed inside user/tests/src/Functional

+ $expected_url = Url::fromRoute('<front>')

So close!

Place expected variable right before you use it. Doesnt make a lot of sense re-locating them so far back.

Moved from src/Tests to /tests/src/Functional (#37)
Reordered statements (#38)
Believe previous test fail was random (passed locally)

After applying the patch, i got the error as UserLogoutTest.php already exist, re -updating the patch by removing that, please suggest me if i am wrong.

@41

After applying the patch, i got the error as UserLogoutTest.php already exist, re -updating the patch by removing that, please suggest me if i am wrong.

The test certainly does not exist: http://cgit.drupalcode.org/drupal/tree/core/modules/user/tests/src/Funct.... Re-check your local environment.

In the future, if you need to check whether a patch still applies, use the retest facility.

Patch #40 (file is labeled 39) still passes, at least against 8.6/PHP 7.1. Does it need anything else before being a candidate for RBTC?

1. Delete the extra line

   +  public function testAnonymousLogout() {
   +
   +    $this->drupalGet('/user/logout');
   

2. Using Response::HTTP_OK instead of 200
3. Maybe it's debatable, but I would prefer a message like this.

        if ($this->currentUser()->isAuthenticated()) {
          user_logout();
        }
   +   else {
   +     \Drupal::logger('user')->notice('You are already logged out.');
   +   }
   

4. Change version to 8.7.x-dev

Patch for 8.9 Drupal release.

coding standards fixes only

Tested the patch given in #52, it's working fine. Post applying the patch and appending user/logout its re-direct to the front page.

Please refer to SS attached for ref.

Code looks good. Works for me.

This is a reasonable change, however it could use test coverage. - i.e. as an anonymous user, visit user/logout, and make sure you land on the front page with no errors.

Verified the patch.Works fine. Not getting access denied page and its redirecting to page.

Back to NW per #55.

Added a test for this change. Please review!

Looks good. I would mark this RTBC except that I suggest adding this to the test:

$this->assertSession()->statusCodeEquals(200);

Add the suggestion given by @Liam Morland.
Thank you.

Looks good. Thanks.

There is a test, so removing tag. This is a feature request, so pretty sure it needs to be fixes on HEAD, changing version and running tests. Correct me if I am wrong about that.

Setting NR while waiting on tests

Nice. This applies to 9.1.x and tests pass.

I also manually tested on 9.1.x and this works as expected, updated the IS with those results.

Since I haven't made any changes to the patch I am setting back to RTBC.

Patch #61 looks good and it is a more friendly approach than as it is now.
I attached images before and after I applied the patch.

RTBC +1

Cheers, Paulo.

Committed 1b628f4 and pushed to 9.1.x. Thanks!

Thought about backporting. For me this is an obvious bug, but there is a tiny chance that contrib or custom code is altering the route access, and changing the access could interfere with that. So... leaving in 9.1.x but if you feel strongly this should be backported please re-open and we can discuss a bit more.