Problem/Motivation
The logout link can only be requested between the time a page was requested and when a session expires. If a user agent has a stale page, such as due to caching or a long lived page, then attempts to logout after the session expires, he will receive a 403 access denied.
Proposed resolution
if an anonymous session attempts to access user/logout, the site will redirect to the front page without fuss.
Behaviour changes
User redirected to <front>
route instead of 403
Tested the patch in #61 on 9.1.x and can confirm that, as anonymous, an attempt to logout resulted in a 403 and after applying the patch an attempt to logout redirected to the front page.
Original report by jim_at_miramontes
I don't think this is a duplicate of any previous issues; I'm encountering this situation with a D7 site: a user:
- leaves a browser window open on the site,
- gets timed out on the back end for having been inactive too long,
- returns to the browser after that long delay, and
- without refreshing the page, tries to log out via a link on that page to
user/logout
.
The access callback for user/logout
is user_is_logged_in()
, which fails in this situation since the user is logged out. As a result, the user gets the results of a 403 error rather than being thrown back to the home page via the drupal_goto()
in user_logout()
.
This is admittedly a minor point, but it wouldn't be hard to address:
Give user_logout()
an open access callback so it can run even when the user is logged out, and
Modify user_logout()
do only do its work if there is a logged-in $user
(and maybe raise an error message via drupal_set_message
if the user was already logged out).
(See https://drupal.org/node/2192401 for a bit more discussion of this, btw.)
Comment | File | Size | Author |
---|---|---|---|
#66 | BeforeApplyPatch61.png | 29.2 KB | paulocs |
#66 | AfterApplyPatch61.png | 27.71 KB | paulocs |
#61 | interdiff.2193803.58-61.txt | 573 bytes | abhisekmazumdar |
#61 | 2193803-61-anonymous_users_receive_access_denied_when_attempting_logout.patch | 1.72 KB | abhisekmazumdar |
#58 | interdiff_52-58.txt | 548 bytes | ridhimaabrol24 |
Comments
Comment #1
Jooblay.net CreditAttribution: Jooblay.net commentedDo you have any rules setup on user logout?
Comment #2
jim_at_miramontes CreditAttribution: jim_at_miramontes commentedNo, I don't.
Comment #3
Charles BelovReproduced 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.
Comment #4
Charles BelovDuplicate of [#2192401] https://drupal.org/node/2192401
Comment #5
Charles BelovComment #6
Charles BelovClosed by mistake. #2192401 is a support request, not an issue.
Comment #7
mglamanHere 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.
Comment #8
mglamanWrong patch. Here's right one.
Comment #9
dawehnerI 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.
Comment #11
mglamanI see. I'm not sure how we could make the best of both worlds happen, I'll try to do some research.
Comment #15
dpiReworded IS/title
I'm not sure I follow #2193803-9: Anonymous users receive access denied when attempting to logout. Here is a new patch.
Comment #16
wturrell CreditAttribution: wturrell as a volunteer commentedPartial 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?
Comment #18
wturrell CreditAttribution: wturrell as a volunteer commentedTest added.
Comment #19
dpi+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
)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,
assertNot
ing 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.Assert message shouldn't be multiline. Strict 80 char wrapping is for PHP comments.
Comment #20
wturrell CreditAttribution: wturrell as a volunteer commented[EDIT - ignore - wrong upload]
Comment #22
wturrell CreditAttribution: wturrell as a volunteer commentedTry again…
Comment #23
dpiInline comment is unnecessary given the method comment directly above.
empty array for first arg.
+ public function testUserLogout() {
Perhaps 'authenticated' is better than 'user'. Makes for easier comparison.
Comment #24
dpiEmpty method.
Comment #25
wturrell CreditAttribution: wturrell as a volunteer commentedYep, all sensible.
Comment #26
wturrell CreditAttribution: wturrell as a volunteer commentedTest-only patch at @dpi's request
Comment #28
dpi#26 Thanks! Looks good
Comment #29
catchWe'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.
Comment #30
wturrell CreditAttribution: wturrell as a volunteer commentedaddressEquals() assertions added.
Comment #31
dpi+ $this->assertSession()->addressEquals('/');
Url::fromRoute('<front>')->toString();
reads better / self documentingComment #32
wturrell CreditAttribution: wturrell as a volunteer commentedGenerate URLs for the tests with Url::fromRoute()
Comment #34
wturrell CreditAttribution: wturrell as a volunteer commentedFailure was:
Attempting to fix by copying this patch…
Comment #35
dpiMethod 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:
Comment #36
wturrell CreditAttribution: wturrell as a volunteer commentedCoding style changed as per #35.
Comment #37
dawehnerNote: BrowserTests should be placed inside
user/tests/src/Functional
Comment #38
dpi+ $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.
Comment #40
wturrell CreditAttribution: wturrell as a volunteer commentedMoved from src/Tests to /tests/src/Functional (#37)
Reordered statements (#38)
Believe previous test fail was random (passed locally)
Comment #41
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedAfter 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.
Comment #42
dpi@41
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.
Comment #46
bvoynickPatch #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?
Comment #47
goodboyResponse::HTTP_OK
instead of200
Comment #50
manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commentedPatch for 8.9 Drupal release.
Comment #52
narendra.rajwar27coding standards fixes only
Comment #53
pankaj.singh CreditAttribution: pankaj.singh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedTested 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.
Comment #54
Liam MorlandCode looks good. Works for me.
Comment #55
catchThis 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.
Comment #56
surya.s CreditAttribution: surya.s at Zyxware Technologies commentedVerified the patch.Works fine. Not getting access denied page and its redirecting to page.
Comment #57
amateescu CreditAttribution: amateescu as a volunteer commentedBack to NW per #55.
Comment #58
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedAdded a test for this change. Please review!
Comment #60
Liam MorlandLooks good. I would mark this RTBC except that I suggest adding this to the test:
Comment #61
abhisekmazumdarAdd the suggestion given by @Liam Morland.
Thank you.
Comment #62
Liam MorlandLooks good. Thanks.
Comment #63
quietone CreditAttribution: quietone as a volunteer commentedThere 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.
Comment #64
quietone CreditAttribution: quietone as a volunteer commentedSetting NR while waiting on tests
Comment #65
quietone CreditAttribution: quietone as a volunteer commentedNice. 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.
Comment #66
paulocsPatch #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.
Comment #68
catchCommitted 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.