Can't logout with the menu item Users > Logout by demis-palma · Pull Request #12504 · joomla/joomla-cms

demis-palma · 2016-10-22T01:42:47Z

The menu item Users > Logout recently introduced highlights an old time latent problem related to the cache.
Basically, while JApplicationWeb::respond() respects the fact that the page can be cachable or not, JApplicationWeb::redirect() totally ignores that fact.
As a result, redirections can be cached causing weird bugs.
One of them, is the inability to execute the log-out a second time. That is, you can login, then logout, login again, but from here to the cache expiration you won't be able to logout furthermore.
We didn't noticed this up to now, because the "Logout button" submits a form, and a POST request is never cached. The new "Logout menu item" is a GET request instead, which on the contrary can be cached.

Cache problem are tricky to reproduce and debug, due to the different way the various cache layers work and react to different inputs, (such as a simple URL request vs a page refresh). For that reason I'll explain how to reproduce the problem step by step.

I've verified this problem with Google Chrome. In my tests, Mozilla Firefox is not affected. I haven't tested further browsers.

Since the logout button is not affected, the problem can be observed through the Logout menu item, so prepare it. New menu item > Type = Users - Logout. Save and close.

To test this, you need to activate a cache layer between the web server and the client.
While a proxy should be fine for this purpose, I've used Apache module mod_expires instead, which is a common environment in many hosting providers. It's easy con configure and from the browser's point of view, it has a predictable behaviour.
mod_expires configuration follows:
1- Ensure that Apache mod_expires is enabled.

a2enmod expires
service apache2 restart

2- Set up the cache in the .htaccess.

ExpiresActive On
ExpiresDefault "now plus 1 hour"

This sets a one hour cache to all resources, such as images, js, css. php would be affected as well, but note that Joomla, while generating its HTML output, overrides this general settings by turning off any cache through appropriate HTTP response headers (see JApplicationWeb::respond()).

Open the "Network" console in Google Chrome, and browse any page of the Joomla website.
It's headers contains all the necessary anti-cache elements:

Cache-Control: no-cache
Pragma: no-cache
Expires: (a date in the far past)

See screenshot n.1.
So far, so good.

Now, login and logout using the "Logout menu item". The logout is successful, but note the absence of the necessary anti-cache elements. Also note the "Expires" date in the future.
See screenshot n.2.
The page can now be cached. We will have big problems soon.

Login again. Now there is no way to Logout by the "Logout menu item". By pressing the Logout menu item we get the page from the cache, and no PHP code is executed server-side.
See screenshot n.3.

The PR proposed simply uniforms the way that JApplicationWeb::respond() and JApplicationWeb::redirect() handle the cache-related headers.
See screenshot n.4.
After applying the PR, the redirect contains all the necessary anti-cache elements.

…redirect() handle the cache-related headers

andrepereiradasilva · 2016-10-23T14:43:03Z

libraries/joomla/application/web.php

+	 *
+	 * @return  void
+	 *
+	 * @since 3.6.4


@since __DEPLOY_VERSION__

I don't understand the suggestion.
Should I replace "@SInCE 3.6.4" with "@SInCE DEPLOY_VERSION"?

@SInCE __DEPLOY_VERSION__

I suppose that the token __DEPLOY_VERSION__ will be automatically replaced.

Yes that's correct. It will be replaced with the correct version. (Which won't be 364)

andrepereiradasilva · 2016-10-23T14:43:17Z

you have a lot of unit tests failing here

demis-palma · 2016-10-23T16:51:07Z

I see that there are a lot of errors, but I don't understand the cause.

mbabker · 2016-10-23T16:53:10Z

The tests are validating the sent headers. You're making changes that add additional headers, so the expected return no longer matches the actual return.

mbabker · 2016-10-23T16:54:05Z

libraries/joomla/application/web.php

@@ -539,8 +517,12 @@ public function redirect($url, $status = 303)

 				// All other cases use the more efficient HTTP header for redirection.
 				$this->header($this->responseMap[$status]);


This should be changed to setHeader too since you're changing the others below it.

Yes, but I can't replace $this->header($this->responseMap[$status]); at the moment.
The reason is that sendHeaders() is not designed to deal with statuses as a string like responseMap[$status] are. For example 'HTTP/1.1 301 Moved Permanently'.
It is designed to hold a int code instead (see all occurrences of $app->setHeader('status', ...) everywhere)
So we should turn $responseMap into an array of int first, and I would leave this for a separate PR.

Well, the only issue is header() will immediately send the header while setHeader() enqueues it for sending. As long as that doesn't cause problems it's fine for now, but I'd suggest commenting in the code why it's this way (even if it's just temporary).

brianteeman · 2016-10-25T18:16:37Z

yes that is correct. That way the version will be correctly set when it is
merged and you dont have to redo this if it is not released n 3.6.4

On 23 October 2016 at 17:41, Demis Palma ツ notifications@github.com wrote:

@demis-palma commented on this pull request.

In libraries/joomla/application/web.php
#12504:

@@ -592,6 +574,43 @@ public function allowCache($allow = null)
return $this->response->cachable;
}

/**

* Set all the necessary cache-related headers, based on the current value of $this->response->cachable,

* which in turn can be set by the components controllers, in case of need, with JApplicationWeb::allowCache()

* @return void

* @SInCE 3.6.4

I don't understand the suggestion.
Should I replace "@SInCE https://github.com/since 3.6.4" with "@SInCE
https://github.com/since DEPLOY_VERSION"?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#12504, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8SJI-qOBEUlICBlVudGvsoezjnUrks5q244lgaJpZM4KduQ4
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

demis-palma · 2016-10-26T21:49:26Z

Ok, I used the placeholder __DEPLOY_VERSION__ and I modified the tests according with the modifications.
In my test environment, all the tests are successful now, while on travis there are still 2 failures. I don't understand why.

In addition, the tests are not satisfactory yet.
The problem is that the method to be tested and the method that runs the test, they both run gmdate() function independently. It's highly probable that they runs during the same second, and the test completes successfully, but chances are that the second call to gmdate() falls into the following second, causing the test failure.
Since the function gmdate('D, d M Y H:i:s') . ' GMT' is called several time along the code, I would suggest to call it once for all and set a constant, a singleton, or a global variable, indicating uniquely the time of the main HTTP request, so that everyone can access it and be sure to get back a consistent value.

How to proceed?

…sTest

demis-palma · 2016-10-28T16:23:52Z

Tests OK. To me the PR is complete for the moment.

demis-palma · 2017-03-07T12:26:46Z

I've found that 2 months later, this same patch has been proposed (and merged in 2 days) here: #13516

The reasons to keep a patch dormant for 4 months and counting, are beyond my understanding, but it's a pity that someone has to waste his time reinventing something that was already there, waiting to be merged.

Uniform the way that JApplicationWeb::respond() and JApplicationWeb::…

8e61aaf

…redirect() handle the cache-related headers

joomla-cms-bot added the PR-staging label Oct 22, 2016

Comply code style standards

f3a7f9a

andrepereiradasilva reviewed Oct 23, 2016

View reviewed changes

mbabker reviewed Oct 23, 2016

View reviewed changes

demis-palma added 3 commits October 26, 2016 19:16

Used __DEPLOY_VERSION__ placeholder

fffb2c7

Modified the tests accordingly

124468c

Removed comma at the end of the array

f21e48b

demis-palma added 2 commits October 28, 2016 16:12

Settle the tests. Attempt joomla#2

637db44

Settle the tests in JApplicationWebTest in addition to JApplicationCm…

00fb812

…sTest

demis-palma mentioned this pull request Mar 7, 2017

Fix “You are not authorised to view this” when mod_expires enabled #13516

Merged

zero-24 closed this Mar 7, 2017

joomla-cms-bot added the Unit/System Tests label Mar 7, 2017

		@@ -539,8 +517,12 @@ public function redirect($url, $status = 303)

		// All other cases use the more efficient HTTP header for redirection.
		$this->header($this->responseMap[$status]);

Uh oh!

Conversation

demis-palma commented Oct 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demis-palma Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Oct 23, 2016

Uh oh!

demis-palma commented Oct 23, 2016

Uh oh!

mbabker commented Oct 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianteeman commented Oct 25, 2016

@demis-palma commented on this pull request.

Uh oh!

demis-palma commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

demis-palma commented Oct 28, 2016

Uh oh!

demis-palma commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

demis-palma commented Oct 22, 2016 •

edited

Loading

demis-palma Oct 23, 2016 •

edited

Loading

demis-palma commented Oct 26, 2016 •

edited

Loading