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

[UI] Remove broken Log out; provide another option #14639

Merged
merged 4 commits into from
Aug 7, 2024

Conversation

cjllanwarne
Copy link
Collaborator

@cjllanwarne cjllanwarne commented Jul 26, 2024

Fixes #14635. Logout is only possible from auth pages due to per-subdomain CRSF tokens. Security/design thought process as documented in a comment on the issue: #14635 (comment)

@cjllanwarne
Copy link
Collaborator Author

Examples

"User" section in the Hail UI header (top-right)

Before

image

After

image

@cjllanwarne
Copy link
Collaborator Author

Examples

"User" page (eg auth.hail.is/user)

Before

image

After

image

{% else %}
<a href="{{ auth_base_url }}/signup">
Sign Up
</a>
<span>|</span>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just adding a | between the two options in the logged-out view, equivalent to what used to be there between the username and Log out in the logged-in view

@cjllanwarne cjllanwarne requested a review from iris-garden July 29, 2024 19:57
@iris-garden
Copy link
Contributor

Thanks for working on this!

Would it be possible to keep the logout button on every page but add a step where it takes the user to the Auth UI to make it work? Specifically, I'm thinking we could add logic to the /user route in auth/auth/auth.py such that if we pass in the query parameter logout, it calls the same code as the /logout endpoint, and then replace the form and button with something like:

<a href="https://auth.hail.is/user?logout">Log out</a>

The tricky part of that might be getting the CSRF token, but since the /user page is only accessible by logged in users (because of the authenticated_users_only decorator), I think there should always be a CSRF token accessible via request.cookies["_csrf"] (e.g. https://github.com/hail-is/hail/blob/main/web_common/web_common/web_common.py#L93).

Copy link
Contributor

@iris-garden iris-garden left a comment

Choose a reason for hiding this comment

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

See comment.

@cjllanwarne
Copy link
Collaborator Author

@iris-garden as I understand it, the root of this problem is that batch.hail.is and auth.hail.is are no longer (since #14180) sharing the same CSRF tokens, so as far as auth is concerned any action from batch.hail.is is treated the same as an action/redirect from www.evilsite.com. So I think our options would be -

  1. Logout redirects to an auth page, but we need one more user action from the page they land on to confirm log out
  2. Logout from batch can redirect to an auth page which automatically logs them out, but so could a redirect from evilsite.com, or someone could embed a <img src="https://auth.hail.is/user?logout" /> tag in an email you receive.
  • Do we care if people can log other users out by sending a bad email? Feels like "yes" but not the absolute end of the world

@iris-garden
Copy link
Contributor

That is a good point! I think we could work around that by passing in the CSRF token from Batch in the query parameters as well, and then having the Auth service ping the Batch API with it to verify the token is valid (or perhaps we could just make this UI a single page application instead of a bunch of pages on different subdomains that resemble one) but I think for the short term this is a good fix!

Unrelated process note: I think in order to link the issue to the PR successfully, you have to use a verb like "fixes" or "closes" in the PR description, rather than "addresses".

Copy link
Contributor

@iris-garden iris-garden left a comment

Choose a reason for hiding this comment

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

See comment; approved!

@cjllanwarne cjllanwarne requested a review from chrisvittal August 5, 2024 18:15
@cjllanwarne
Copy link
Collaborator Author

Having the Auth service ping the Batch API with it to verify the token is valid

I believe the way our CRSF is implemented, we don't actually ever "validate" the tokens, we only check that the token in the formdata matches the token in the cookie.

Or perhaps we could just make this UI a single page application instead of a bunch of pages on different subdomains that resemble one.

This would be wonderful! Sort of similar-but-better to my thought of hosting the "top menu bar" as a separate iframe that always comes from auth. For the same reason (in particular, the apparently lack of regular usage of the logout button), that kind of change is probably larger than the scope of getting this bug fixed... but would cool to look into some day!

@hail-ci-robot hail-ci-robot merged commit fca1b1a into hail-is:main Aug 7, 2024
3 checks passed
@cjllanwarne cjllanwarne deleted the cjl_logout_bug branch August 9, 2024 14:35
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.

Logout is unauthorized except from auth.hail.is pages
4 participants