403 error on cookie login when visiting a protected page after session expires

sakicnet commented 7 years ago

Steps to reproduce the issue

Install Joomla 3.7 (or 3.7.1-dev)
Create a menu link with Access set to Registered (e.g. /test)
Enable System - Language Filter plugin (required for multi-lingual sites)
Login on frontend with "Remember me" checkbox ticked
Wait 15 minutes for session to expire ;) (not really, just remove your session cookie, or delete the session record from #__session table)
Visit the protected page (e.g. /en/test)

Expected result

You should be automatically logged in and land on open protected page.

Actual result

You are redirected to frontpage with message: Error: You are not authorised to view this resource. If your homepage requires access then you end up on 403 page.

System information (as much as possible)

What happens is that the language filter plugin builds the menu with access levels before the remember plugin had a chance to login the user. The menu is built with access levels of the guest user and updates only on refresh. So, on first visit, after the session expires, the menu thinks it's a guest and denies the access.

The quick fix is to re-build the menu after the remember plugin has logged in the user. Here is the Gist, line 66: https://gist.github.com/sakicnet/f2b8e2486011093d08e544423d8e5124

Additional comments

brianteeman commented 7 years ago

@sakicnet can you submit this as a PR please Emir

alikon commented 7 years ago

i wonder, can we do in an alternative way just loading first plg_system_remember plugin and after plg_system_languagefilter changing the system plugin order?

sakicnet commented 7 years ago

@brianteeman Hi Brian, is this enough? https://github.com/sakicnet/joomla-cms/commit/d6fa7f9b4e6f206a4fadc9ba9ea275c4308faf36

@alikon: no, not really. Remember plugin does the login, which triggers onUserLogin events, which builds the menu before user is logged in. So it would involve bigger structural changes to fix it.

mbabker commented 7 years ago

We shouldn't be direct calling __construct() again after an object is already constructed. Especially because it could result in class properties being changed. What you really need to do is call the load() method.

sakicnet commented 7 years ago

@mbabker that would probably require extending the JMenu class because its $user property is protected?

mbabker commented 7 years ago

JMenu::load() is public. There's no need to make any other changes than to (re-)call the load() method.

mbabker commented 7 years ago

Wait, nevermind. Realizing you need an updated JUser object too.

mbabker commented 7 years ago

Either way, you shouldn't reconstruct an object. A new one should be loaded in replacing the existing one.

sakicnet commented 7 years ago

JMenu::load() doesn't fix the issue. What we need is $menu->user->load($id) OK, will try that.

mbabker commented 7 years ago

Putting public setters for the properties (at least a setUser() method) would help. Then you'd just need to do $this->app->getMenu()->setUser($user)->load();.

sakicnet commented 7 years ago

If we can change JMenu then great. New PR: https://github.com/joomla/joomla-cms/pull/15839 Feel free to modify / improve. Thanks.