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

Prevent mixing up session and service tokens, fix admin login, remove authenticate via API key #164

Conversation

neelvirdy
Copy link
Contributor

@neelvirdy neelvirdy commented Jan 10, 2023

Blocked on application-research/estuary#861

Along with application-research/estuary#861, fixes #99 and application-research/estuary#815

  1. Do not render session tokens in api admin page (they should only be managed via sign in and sign out)
  2. Admin login fix taken from Admin login credentials incompatible with estuary-www estuary#815 (comment)
  3. Remove authenticate via API key since it's unnecessary now that admin login is fixed, and to prevent mixing up session and service tokens

Warning: if any estuary node operators have an admin user with a password that doesn't meet the 8 alphanumeric characters bcrypt requirement, they will end up getting locked out of that user when that token expires. They can create a new estuary admin by re-running setup but there may still be files tied to the original admin user. Alternatively, they can hit the change password endpoint while they have a valid token for that user and change it to a valid password.


New login screen (checkbox is unchecked by default):
image
Note: I'm also open to not having a checkbox and just attempting the admin login on any auth failure.

No keys will be rendered if users haven't made any service tokens explicitly:
image

Expected user pattern is to label your service tokens clearly here and they will be the only tokens present:
image

I can no longer authenticate into estuary-www with that service token, so there's no risk of mixing up that token with my session. This removes the risk of signing out with a service token in your cookie and accidentally revoking it, which would break your service's integration with estuary.

@neelvirdy neelvirdy requested a review from en0ma January 19, 2023 02:52
@neelvirdy neelvirdy removed the blocked label Jan 30, 2023
@neelvirdy neelvirdy marked this pull request as ready for review January 30, 2023 20:49
Copy link
Contributor

@en0ma en0ma left a comment

Choose a reason for hiding this comment

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

LGTM

@neelvirdy neelvirdy merged commit e93b6a3 into application-research:master Feb 2, 2023
@neelvirdy neelvirdy deleted the nvirdy/session-vs-service-tokens branch February 2, 2023 18:19
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.

Treat browser tokens and service tokens separately
3 participants