[Login] Add login suggestion and redirection for error 403 #9041
Conversation
maximemulder
changed the title
src/Http/Error.php
Outdated
| // Variables used to suggest the user to login and later redirect them if they | ||
| // are not authenticated in a 403. | ||
| $tpl_data['anonymous'] = $user instanceof \LORIS\AnonymousUser; | ||
| $tpl_data['url'] = urlencode($_SERVER['REQUEST_URI']); |
There was a problem hiding this comment.
Also, I used $_SERVER['REQUEST_URI'] instead of the $uri object here because the latter seems to return a lorispath=url instead of the URL I want directly. I haven't been able to find why there is this lorispath so if anyone has an explanation I am all ears.
There was a problem hiding this comment.
That sounds like the mod_rewrite rule from Apache leaking through somewhere. That should not happen. How were you testing it / in what kind of environment that you had that problem?
There was a problem hiding this comment.
That sounds like the
mod_rewriterule from Apache leaking through somewhere. That should not happen. How were you testing it / in what kind of environment that you had that problem?
I am using a MCIN VM with Apache 2.4 and PHP 8.1. I did not notice anything obviously wrong with my installation so the fact that this is abnormal is certainly puzzling.
maximemulder
force-pushed
the
403-login
branch
2 times, most recently
from
266299b
to
b5a8cfc
Compare
maximemulder
force-pushed
the
403-login
branch
from
b5a8cfc
to
d79feee
Compare
src/Http/Error.php
Outdated
| // Variables used to suggest the user to login and later redirect them if they | ||
| // are not authenticated in a 403. | ||
| $tpl_data['anonymous'] = $user instanceof \LORIS\AnonymousUser; | ||
| $tpl_data['url'] = urlencode($_SERVER['REQUEST_URI']); |
There was a problem hiding this comment.
This should be using the URI from the ServerRequestInterface passed as an argument, not the PHP superglobal.
There was a problem hiding this comment.
I just did some tests with @ridz1208, who uses Apache similarly to me, and his $uri->__toString() also contains the ?lorispath=, which we obviously don't want in the redirection URL. Therefore, I don't think we can get the URL in the backend without using the PHP superglobal, unless we prevent lorispath from leaking (which may be a good idea but would require dedicated work on it).
I can get the URL in the front-end so it's either that or the superglobal, both are kinda hackish but could be removed once we Reactify the errors imo.
There was a problem hiding this comment.
If it's a bug unrelated to your PR, just use $uri->__toString and create an issue about it.
There was a problem hiding this comment.
I am not sure we should use $uri->__toString() before the issue is fixed (if we decide to fix it) as it seems to happen on most setups except for yours and Github Actions' ones. I personally think we should either fix the issue first or add a comment and merge. I'll try to see if there is a clean quick fix for the issue if I have some time today.
I created the issue at #9049.
|
@ridz1208 can you review? |
|
I just noticed that I did this PR quite some time ago and wasn't aware of the testing plan at the time, gonna add it before merging. |
kongtiaowang
added
the
Passed Manual Tests
Brief summary of changes
Whenever an unauthenticated gets a 403 error, they can try logging in and be redirected to the page they got the 403 on.
Testing instructions (if applicable)
I implemented this using an URL query parameter, but I guess using
window.historycould also have worked. If anyone has an opinion on this please tell me.