Skip to content

add login screen, redirect when unauthorized #302

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

rvyhnal
Copy link
Contributor

@rvyhnal rvyhnal commented Sep 24, 2024

This PR is taking out the login modal from base.mako into a new template login.mako. The redirect happens as part of the authenticatedAjax function.

#295

Comment on lines 578 to 587
function requireLogin(destinationUrl){
let currentPath = window.location.pathname.substring(1)
let loginUrl = "/login.html"

if ( ! ["login.html", "index.html"].includes(currentPath)){
loginUrl += "?r=" + encodeURIComponent(destinationUrl || currentPath)
}
window.location.href = loginUrl
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this function won't work in multiple cases. For example, IntelMQ manager in all instances I manage is deployed under a path /intelmq-manager. In addition, the main page is accessible directly under the path, without redirecting to index.html.

I've checked how the code would behave on the main page on one of my instances (in the browser's console):

> let currentPath = window.location.pathname.substring(1) 
> let loginUrl = "/login.html"
> currentPath
"intelmq-manager/"
> window.location.href = loginUrl
# Redirects to the main domain, not under the manager's path

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 have modified the redirects so that all work with relative paths


let url = new URL(window.location.href)
let redirect = new URLSearchParams(url.search).get('r')
window.location.replace(redirect || "/index.html");
Copy link
Contributor

Choose a reason for hiding this comment

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

As a minimum precaution, we should verify the redirect target is related to the current path, to avoid an open redirect issue.

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 was assuming this wouldn't be an issue as the redirect happens only after the user is authenticated, anyways I have made a slight change, so that the redirect target is treated as relative path to the current location.

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.

2 participants