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.

Sign up for GitHub

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

Jump to bottom

[Login] Add login suggestion and redirection for error 403 #9041

Merged
merged 3 commits into from
May 16, 2024
Merged

[Login] Add login suggestion and redirection for error 403 #9041

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d79feee
Add 403 login suggestion and redirection
Feb 2, 2024
169b91e
use request object instead of php superglobal
Feb 14, 2024
61e8c0d
Add test plan
Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions modules/login/jsx/loginIndex.js
View file
Open in desktop
Expand Up @@ -136,9 +136,11 @@ class Login extends Component {
})
.then((response) => {
if (response.ok) {
response.json().then((data) => {
// success - refresh page and user is logged in.
window.location.href = window.location.origin;
response.json().then(() => {
// Redirect if there is a "redirect" param, refresh the page otherwise
window.location.href = this.props.redirect !== null
? this.props.redirect
: window.location.origin;
});
} else {
response.json().then((data) => {
Expand Down Expand Up @@ -337,6 +339,7 @@ Login.propTypes = {
defaultRequestFirstName: PropTypes.string,
defaultRequestLastName: PropTypes.string,
defaultRequestEmail: PropTypes.string,
redirect: PropTypes.string,
};

window.addEventListener('load', () => {
Expand All @@ -353,6 +356,7 @@ window.addEventListener('load', () => {
defaultRequestFirstName={getParam('firstname', '')}
defaultRequestLastName={getParam('lastname', '')}
defaultRequestEmail={getParam('email', '')}
redirect={getParam('redirect', null)}
module={'login'}
/>
);
Expand Down
9 changes: 8 additions & 1 deletion smarty/templates/403.tpl
View file
Open in desktop
@@ -1,4 +1,11 @@
<div class="container">
<h2>403: Forbidden</h2>
<h3>{$message}</h3>
<div><a href="{$baseurl}">Go to main page</a> | <a onclick="window.history.back();" href="#">Go back one page</a></div>
<div>
<a href="{$baseurl}">Go to main page</a>
| <a onclick="window.history.back();" href="#">Go back one page</a>
{if $anonymous}
| <a href="/login?redirect={$url}">Try logging in</a>
{/if}
</div>
</div>
5 changes: 5 additions & 0 deletions src/Http/Error.php
View file
Open in desktop
Expand Up @@ -55,6 +55,11 @@ public function __construct(
$lorisInstance = $request->getAttribute('loris');
$user = $request->getAttribute('user') ?? new \LORIS\AnonymousUser();

// 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']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be using the URI from the ServerRequestInterface passed as an argument, not the PHP superglobal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@driusan driusan Feb 9, 2024 •
edited

Choose a reason for hiding this comment

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

If it's a bug unrelated to your PR, just use $uri->__toString and create an issue about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// Add a link to the issue tracker as long as a LORIS Instance object
// is present in the request.
if (! $user instanceof \LORIS\AnonymousUser
Expand Down