Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graceful recovery for issue: You are not permitted to use that link to directly access ... #10753

Closed
wants to merge 2 commits into from

Conversation

ggppdk
Copy link
Contributor

@ggppdk ggppdk commented Jun 7, 2016

Add some fault-tolerance to the system and recalculate edit permission (also checking out the record)

CASE 1: The initial "edit-task", failed to update session properly, due to race-conditions
CASE 2: The initial "edit-task", was never called because the HTTP request was cached by the browser for any reason

See issue here for a detailed discussion: #8757

NOTE:
3rd party extensions that extend JControllerLegacy need to re-implement this,

  • if they need to pass to allowEdit more than just: "id"=>NNN

Maybe best solution would be instatiate the view inside the controller add / edit task,

  • but this means that all tasks would need to re-direct to the edit task instead of the view ... e.g. the 'apply' task, etc, which is a major B/C break, so i did not make such a PR !
  • if you do not like something this PR, then please comment and help fix / improve it

Testing:
Try adding this to your .htaccess to force browser caching of the requests that try to add article/record "is-editable" into the sessions (NOTE: the bad effects for the effected URLs will persist for an hour, so do not add more to it)

ExpiresDefault "now plus 1 hour"

…s that page

Add some fault-tolerance to the system and (re-) calculate edit permission (also checking out the record)

CASE 1: The initial "edit-task", failed to update session properly, due to race-conditions
CASE 2: The initial "edit-task", was never called because the HTTP request was cached by the browser for any reason

See issue here for a detailed discussion:
joomla#8757

NOTE:
3rd party extensions that extend JControllerLegacy need to re-implement this, 
- if they need to pass to allowEdit more than just: "id"=>NNN

Maybe best solution would be instatiate the view inside the controller add / edit task,
- but this means that all tasks would need to re-direct to the edit task instead of the view ... e.g. the 'apply' task, etc 

which is a major B/C break, so i did not make such a PR !

If you don't like the way i include the controller, just tell me how to improve / correct it

even if you do not like this PR, it is good to be here as a reference ...
@wojsmol
Copy link
Contributor

wojsmol commented Jun 7, 2016

@ggppdk See CS PR ggppdk#1

@ggppdk
Copy link
Contributor Author

ggppdk commented Jun 8, 2016

@wojsmol
thanks, for the code styling fix,

next time i will use try to use
https://github.com/joomla/coding-standards

About testing this PR

and add a

return; 

at the start of the function, thus the id will never be added into the session as editable,
then test all backend screens: articles, categories, etc for being able to edit the records and save the edit form

@edzz83
Copy link

edzz83 commented Jun 23, 2016

I can confirm having tested this on 3 3.5.1 installations that this worked for me with following modules issue:
Install of 3.5.1, logout/login did not fix issue in this instance.
Using front end editing with open in administrator on in module config.
When press EDIT button on module on front end template, would ALWAYS get permission error message. You are not permitted to use that link to directly access that page (#'module number').
.. im not sure if its a certain combination of plugins/login items we are using causing it to happen in every case for us.
Hoping this gets put into next release asap as this is very annoying for those using front end editing.

@ggppdk
Copy link
Contributor Author

ggppdk commented Jun 23, 2016

@edzz83

Hoping this gets put into next release asap as this is very annoying for those using front end editing.

that is one way, every one that needs a fix copying the suggested code into their web-sites

  • since you have tested this, you should mark your test as successful here:

https://issues.joomla.org/tracker/joomla-cms/10753

by clicking on the button "Test this" button, and selecting "Test succesfully",

if you and other just copy the code but don't mark test as successful then it will not be accepted

@edzz83
Copy link

edzz83 commented Jun 28, 2016

I have tested this item ✅ successfully on 80a5498

Tested on 3 installations we run (sites we have built from scratch running multiple components and such). Fix worked on all 3 sites successfully. This issue appeared from us when trying to edit a module on front end with it set to 'administrator' mode (so when clicking edit it should open module in administrator) this would come up with the error every time regardless of browser cache status. The fix resolved this issue.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10753.

@brianteeman
Copy link
Contributor

@PhilETaylor can you test this please

if ( $optionName == reset($context_arr) && $viewName == end($context_arr) )
{
$controller_name = ucfirst($this->getName()) . 'Controller' . ucfirst($viewName);
$controller_path = JPATH::clean(JPATH_COMPONENT . '/controllers/' . $viewName . '.php');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a massive assumption that this will ALWAYS be a "subtask" (for lack of better terms). Say a component doesn't have a article.edit task like com_content and ContentControllerArticle and just has a single controller.php file that its edit task routes through; this case won't be handled.

Also, is it really necessary to instantiate another controller to get the data you need with this implementation? You aren't inflecting anything anywhere so it seems like you're deriving the class name for the current controller and instantiating a second instance of it to find the model and re-run the allowEdit() method. Seems like major overkill here, especially because it introduces a bunch of extra logic that really should just call for JControllerLegacy::getInstance() to be re-used but alas that is terribly hardcoded to a global singleton instance...

@durian808

This comment was marked as abuse.

@ggppdk
Copy link
Contributor Author

ggppdk commented Aug 1, 2016

@mbabker

... and just has a single controller.php file

... the code is inside checkEditId() and it is for classes inheriting from JControllerLegacy

Please provide an example (steps) that this code will do something worse than what is currently happening

  • thus i can improve this PR, testing the case that you have in mind

Also, is it really necessary to instantiate another controller to get the data you need with this implementation?

Yes it is because the controller that made the check is in generally different than the current controller (as far as i remember when making this PR)

@durian808

One question that comes to mind is, has @ggppdk 's fix been tested in an environment where the "You are not permitted..." issue is easily reproduced? Because if not, testing the fix has no meaning.

  • If something has not been tested, then there is no meaning to try test it, ... aaa ... hhhmm ... ok ... i will figure it out eventually ... still thinking ... no ... i don't understand

but to answer your question Yes and you can create such an environment yourself please read my other messages about forcing the "easily reproduced" thing by instructing browser to cache the 1st request

Adding a patch to "avoid" something,

The term is "fault-tolerant" sound better ?
a fault will occur try to recover from it , the best way possible
Let's not play with words,

  • are "recover" or "prevent" a better title ?

@ggppdk
Copy link
Contributor Author

ggppdk commented Aug 1, 2016

@mbabker

the solution i have in my extension is to make the checks in the same step that instatiates the view without a redirection

  • i almost made a PR for this

it was working for core components, but it was really breaking B/C (really bad break)

@mbabker
Copy link
Contributor

mbabker commented Aug 1, 2016

Right, I get this method is in the base controller, but your logic for
instantiating the second controller assumes ContentController would never
have an edit task like ContentControllerArticle, just as an example. Which
is probably true, but it still feels wrong to introduce something with
partial class logic.

As for the second controller thing, can you point out how that's
happening? JControllerLegacy::getInstance() only supports a single
controller instance for the entire app so you can't use that to create two
controllers and unless I've missed something somewhere there isn't any
other code manually instantiating new controllers outside that for the
legacy MVC tree.

On Monday, August 1, 2016, Georgios Papadakis [email protected]
wrote:

@mbabker https://github.com/mbabker

... and just has a single controller.php file

... the code is inside checkEditId() and it is for classes inheriting
from JControllerLegacy

Please provide an example (steps) that this code will do something worse
than what is currently happening

  • thus i can improve this PR, testing the case that you have in mind

Also, is it really necessary to instantiate another controller to get the
data you need with this implementation?

Yes it is because the controller that made the check is in generally
different than the current controller (as far as i remember when making
this PR)

@durian808 https://github.com/durian808

One question that comes to mind is, has @ggppdk
https://github.com/ggppdk 's fix been tested in an environment where
the "You are not permitted..." issue is easily reproduced? Because if not,
testing the fix has no meaning.

  • If something has not been tested, then there is no meaning to try
    test it
    , ... aaa ... hhhmm ... ok ... i will figure it out eventually
    ... still thinking ... no ... i don't understand

but to answer your question Yes and you can create such an environment
yourself please read my other messages
about forcing the "easily
reproduced" thing by instructing browser to cache the 1st request

Adding a patch to "avoid" something,

The term is "fault-tolerant" sound better ?
a fault will occur try to recover from it , the best way possible


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10753 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoQ3DuIGwssHzyOOi58AWuro2NGG3ks5qbnBfgaJpZM4IwcBB
.

@mbabker
Copy link
Contributor

mbabker commented Aug 1, 2016

Wait, I just realized this is the second check after the redirect. So ya,
there is a second controller invoked.

On Monday, August 1, 2016, Georgios Papadakis [email protected]
wrote:

@mbabker https://github.com/mbabker

the solution i have in my extension is to make the checks in the same step
that instatiates the view without a redirection

  • i almost made a PR for this

it was working for core components, but it was really breaking B/C


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10753 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoQh7THxV3r3c1iTrUsVO_k9d435lks5qbnJ5gaJpZM4IwcBB
.

@ggppdk
Copy link
Contributor Author

ggppdk commented Aug 1, 2016

but it still feels wrong to introduce something with partial class logic.

The solution should work in 99% of cases, i say this percentage, since the code is inside checkEditId(), and inside JControllerLegacy,

  • if the functon call to allowEdit() needs more parameters to work ! then it should be the responsibility of the 3rd party developer to override the function and extend it, and call proper code, i speak of 3rd part apps that may have an overriden allowEdit(), that receives more parameters, or using some other function
  • furthermore, even if in some 3rd party the new code triggers a failure, then it will be triggered on an already erroneus situation, and not in normal situations

JControllerLegacy::getInstance() only supports a single controller instance for the entire app

  • yes you are right, that is why the code does not use it, instead it does:
$controller = new $controller_name;

@ggppdk ggppdk changed the title Avoid issue: You are not permitted to use that link to directly acces… Graceful recovery for issue: You are not permitted to use that link to directly access ... Aug 1, 2016
@durian808

This comment was marked as abuse.

@durian808

This comment was marked as abuse.

@durian808

This comment was marked as abuse.

@lyquix-owner
Copy link

To reproduce this problem all you need to do is have Apache server configured to add Cache and Expires headers on the HTTP responses. Then in Joomla just open a module and then close it, and try to open it again.

I tested this PR and it solves the problem, even with Cache and Expires headers.

A couple comments:

  1. Would it make sense, as an additional precaution, to force PHP to set the HTTP headers specifically for redirect responses?
<?php
header("Cache-Control: no-cache, must-revalidate"); // HTTP/1.1
header("Expires: Sat, 26 Jul 1997 05:00:00 GMT"); // Date in the past
?>
  1. Would it make sense to use HTTP response code 302 instead of 301 or 303? Even in cases when the server doesn't have Cache and Expires headers, the browser may proactively cache 301 and 303 redirects, but not 302 responses.

I hope this is useful.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@ggppdk
Copy link
Contributor Author

ggppdk commented Aug 2, 2016

IMHO the correct approach is more down the line of ensuring that Joomla's internal redirects are not 301 redirect that can be cached.

not possible, please correct me, but you can not override all caching cases, that happen due to configuration in .htaccess, (maybe it is possible in some server configurations ? i am not sure, someone please comment if it is possible and provide documentation link)

About if this fix is correct approach ...

  • it runs code that was supposed to have run already
  1. check if editable / not checked out
  2. add to session as editable and marked it as checked out

Thus i argue it is not bad approach in what it does

The thing i do not like about this PR and any PR that will depend on the 2nd call, was not mentioned by anyone, and is this

That it will encourage BAD .htaccess configurations that do this kind of caching, which may create similar problems to other cases too that will need similar patching.

  • If it was only the rarely occuring issue of this message due to race conditions (which is not that important since you can re-click to edit the record) then this PR would be more easy to accept and also more easy to remove later too, but if this PR encourages such .htaccess configurations, removing it later will create problems !! because more and more web-sites will start to depend on it and use such .htaccess configurations

Please decide something,

  • for me, a BEST solution is to improve the message too, which is really wrong:

You are not permitted to use that link to directly access this page

It is totally wrong for the user, it means nothing to the user, it does not inform the user of the real reasons. It is a message for programmers that have studied Joomla code

Proper:

Edit request has failed temporarily. Please try again.

If this is repeated for the same record multiple times, then it means that your server has been misconfigured and tells browser to cache edit requests.

Please contact your administrator to disable caching via HTTP headers (e.g. in .htaccess)


ok, not exactly the above, but something like it

@durian808

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

brianteeman commented Aug 2, 2016 via email

@gwsdesk
Copy link

gwsdesk commented Jan 6, 2017

@durian808 I cannot reproduce the issue as well as already expressed in the referred thread #10757 . I believe you when you tell the world that you are experiencing this issue but again posting access details to your Siteground or joomla.com account won't help you resolve this issue which has not been identified as a bug or whatever you wish to call it by the JBS nor the CMS Release Team so only if we can replicate this by following steps to reproduce as Thomas mentions we can start working on a solution

@lyquix-owner
Copy link

lyquix-owner commented Jan 6, 2017

Steps to reproduce the error:

Environment
Ubuntu 14.04
Apache 2.4.7
MySQL 5.5.53
PHP 5.5.9

apache2.conf settings (different from fresh installation)

MaxKeepAliveRequests 0
<Directory /srv/www/>
    Options FollowSymLinks
    AllowOverride all
    Require all granted
</Directory>
# Browser Caching #
ExpiresActive On
ExpiresDefault "access plus 30 days"
ExpiresByType text/html "access plus 15 minutes"
Header unset Last-Modified
Header unset ETag
FileETag None

Steps

  • Fresh Joomla 3.6.5 installation
  • Log in to Administrator
  • Go to Modules
  • Click on the Breadcrumbs module
  • Click Save & Close
  • Click on the Breadcrumbs module again
  • You get the error message You are not permitted to use that link to directly access that page (Move garbagecron.txt to the cli folder. #17). and the list of modules is displayed again
  • If you click on the Breadcrumbs module again no warning or error message is displayed this time and it shows the list of modules again

You can view the sequence of HTTP requests and responses here:
http://imgur.com/a/h8QJe

  1. Display list of modules
    GET: ?option=com_modules
    Response: 200

  2. Click on Breadcrumbs module:
    GET: ?option=com_modules&task=module.edit&id=17
    Response: 303 / Location: ?option=com_modules&view=module&layout=edit&id=17
    GET: ?option=com_modules&view=module&layout=edit&id=17
    Response: 200

  3. Save & Close module
    POST: ?option=com_modules&layout=edit&id=17
    Response: 303 / Location: ?option=com_modules&view=modules
    GET: ?option=com_modules&view=modules
    Response: 200

  4. Click on Breadcrumbs module again:
    GET: ?option=com_modules&task=module.edit&id=17
    Response: 303 / Location: ?option=com_modules&view=module&layout=edit&id=17
    GET: ?option=com_modules&view=module&layout=edit&id=17
    Response: 303 / Location: ?option=com_modules&view=modules

    GET: ?option=com_modules&view=modules
    Response: 200

So, the first question is why request ?option=com_modules&view=module&layout=edit&id=17 produces a 200 response the first time, and a 303 response the second time redirecting to the modules list?

The second question is why requests with 200 responses include the heading Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0 but the same heading is not enforced on 303 responses?

It seems to me that the browser caching issue comes after these initial problems, and it just makes it worse by getting the browser stuck in using the same cached 303 response.

In my opinion this is a real bug and it should not be dismissed.

@mbabker
Copy link
Contributor

mbabker commented Jan 6, 2017

Except for one person, nobody's tried to dismiss it. But as this and other threads have pointed out it's not exactly the easiest thing to reproduce.

So, the first question is why request ?option=com_modules&view=module&layout=edit&id=17 produces a 202 response the first time, and a 303 response the second time redirecting to the modules list?

That's coming from the browser or web server configuration, not Joomla. Joomla doesn't have response caching like this. That is in part why this issue exists.

The second question is why requests with 202 responses include the heading Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0 but the same heading is not enforced on 303 responses?

Depends if it's Joomla or something else issuing the redirect I guess. If it's Joomla issuing the redirect then perhaps the right headers aren't being attached (JApplicationWeb::redirect() doesn't send the full header set the same way a "regular" response does, the cache headers are set at JApplicationWeb::respond()). Anything that's not in the scope of the Joomla code I'm not even going to start speculating about.

@gwsdesk
Copy link

gwsdesk commented Jan 6, 2017 via email

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@lyquix-owner
Copy link

@PhilETaylor can you provide an example of how to fix an edit link? I am no expert but I am happy to help with this issue, if I can get some direction from you or @ggppdk. You both seem to have an understanding of the problem and its solution.

@gwsdesk
Copy link

gwsdesk commented Jan 6, 2017

@PhilETaylor why have you not proposed (!) a solution yet knowing so (seemingly) well what the issue is as you emphasize ..... Curious (and not a personal attack) ?

@PhilETaylor

This comment was marked as abuse.

@gwsdesk
Copy link

gwsdesk commented Jan 6, 2017

Oh Phil...I do follow you believe it or not......Yeah you described that in "CASE 2: " https://github.com/joomla/joomla-cms/pull/10753#issuecomment-236978677 Question is why nobody followed up on that?

@ggppdk
Copy link
Contributor Author

ggppdk commented Jan 6, 2017

CASE 1:

  • The consistent reproduction due to server (e.g. .htaccess) configurations, that instruct the browser to cache the redirect,

CASE 2:

  • Quickly (middle) clicking in 10 / 15 edit links and thus having the RACE-conditions with the session data being overwritten so you can get some failures

Solution

CASE 1:

  • This is server configuration, we can only be worked-around by like this PR tries to do, or just make some only documentation about it and every time some asks or googles the issue , then the documentation can be found, and thus fix his/her server configuration problem

CASE 2:

  • It is just a small rare annoyance, when people try to open many edit links in very short time, it can be documented too

So besides the above,
I confirm what @lyquix-owner said

The 303 redirect response , that redirects from edit-task to edit-layout ("view") is missing both headers:
Pragma: no-cache
Expires: ...

Which brings us to the 3rd case??
of some rare cases that chrome can decide to cache the redirect ?
could it be because of the above headers missing ?

  • if the above is true then this last 3rd case can be addressed

@ggppdk ggppdk reopened this Jan 6, 2017
@lyquix-owner
Copy link

A small suggestion (bear in mind I am no expert). Add the following lines to

libraries/joomla/application/web.php, line 542:

$this->header('Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0');
$this->header('Pragma: no-cache');

This adds the headers on all redirects. Problem solved. I am unaware of issues that this may cause. Experts can weigh in.

@PhilETaylor

This comment was marked as abuse.

@ggppdk
Copy link
Contributor Author

ggppdk commented Jan 6, 2017

This adds the headers on all redirects
Which is why it is a bad idea :-)

The place to add it is here:
https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L420
and similar in other controllers

Like this:

header('Expires: Wed, 17 Aug 2005 00:00:00 GMT');
header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT');
header('Cache-Control', 'no-store, no-cache, must-revalidate, post-check=0, pre-check=0');
header('Pragma: no-cache');

$this->setRedirect(
	JRoute::_(
		'index.php?option=' . $this->option . '&view=' . $this->view_item
		. $this->getRedirectToItemAppend($recordId, $urlVar), false
	)
);

[EDIT]
i know of that cache / header methods exist in JApplication, still i used PHP header method for headers above

@PhilETaylor

This comment was marked as abuse.

@dgrammatiko
Copy link
Contributor

There is a Response/Redirect API within the Joomla API, this should be configurable to output the correct headers when used

Until someone extend that to incorporate such functionality, the only way to do it is the one @ggppdk proposed. My 2c

@ggppdk
Copy link
Contributor Author

ggppdk commented Jan 6, 2017

wrong again :-)
There is a Response/Redirect API within the Joomla API, this should be configurable to output the correct headers when used...

No no

  • currently , the only way to do it , is the way i suggested above

because JApplication::redirect is either bogus or at least let's say "incomplete"

You cannot use (due the current implementation of JApplication::redirect())
Neither:

JFactory::getApplication()->allowCache(false);

nor:

JFactory::getApplication()->setHeader('...', '...', true);

Because both the variables that are set by the above methods, both of them are ignored by JApplication::redirect()

even if you use
JFactory::getApplication()->setHeader('...', '...', true);
then you still need to call:
JFactory::getApplication()->sendHeaders()

  • which will prevent JApplication::redirect() from setting any headers because it does a "check-headers-sent"

Best solution is to "fix" JApplication::redirect()
to

  1. use JFactory::getApplication()->setHeader() for adding its headers
  2. then call in it JFactory::getApplication()->sendHeaders()

@mbabker
Copy link
Contributor

mbabker commented Jan 6, 2017

Best solution is to "fix" JApplication::redirect()

Bingo! Stop thinking about local hacks and fixes, these aren't your extensions you're working with folks. Fix core the "right" way.

@ggppdk
Copy link
Contributor Author

ggppdk commented Jan 6, 2017

Bingo! Stop thinking about local hacks and fixes, these aren't your extensions you're working with folks. Fix core the "right" way.

Yes, and also fix JApplication::redirect()
to respect the setting set by:
JFactory::getApplication()->allowCache(...);

@durian808

This comment was marked as abuse.

@lyquix-owner
Copy link

How about: on libraries/joomla/application/web.php, line 542, add:

if(!JFactory::getApplication()->allowCache()){
    $this->header('Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0');
    $this->header('Pragma: no-cache');
}

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@Bakual Bakual closed this Jan 8, 2017
@durian808

This comment was marked as abuse.

@Bakual
Copy link
Contributor

Bakual commented Jan 9, 2017

Open a new issue for that. Thanks.

rdeutz pushed a commit that referenced this pull request Jan 10, 2017
…13516)

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

Closes #8731 Dec 18, 2015
Closes #8757 Dec 21, 2015
Closes #9013 Jan 28, 2016
Closes #9145 Feb 17, 2016
Closes #9615 Mar 26, 2016
Closes #10753 Jun 7, 2016

* code style

* Unit test refactoring (cannot do it the old way as relative dynamic dates in headers)

* Remove duplicate header output for charset

* Better Unit Tests

* More unit tests :)

* Close after setting headers

* Do not cache the response to this, its a redirect
@orphicpixel
Copy link

Out of nowhere, while we are busy adding modules, working on some menus, adding articles, and there is a request to edit a long time existing module then suddenly we encountered this error.

Tried to log out, clear all cache - Joomla and browser. Empty the session table but not luck

I change the Administrator's template and I can confirm that this fix the problem on my side.

Can someone try also to change Admin template and edit those article or module?

@durian808

This comment was marked as abuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.