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

IsSession on auth token and validate admin creds on setup #861

Merged
merged 7 commits into from
Jan 19, 2023

Conversation

neelvirdy
Copy link
Contributor

@neelvirdy neelvirdy commented Jan 10, 2023

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

  1. Adds IsSession field on AuthToken table and populates with false for tokens created via api admin page, and true for logins/signups. Session tokens will not be rendered in the api admin page so users cannot mix up their service and session tokens. The frontend will also no longer support authenticating via API key to prevent using a service token as a session token. That feature is also unnecessary when admin login is fixed.
  2. When running ./estuary setup, validates admin username and password to be compatible with bcrypt's requirement of 1-32 alphanumeric characters on usernames and 8+ alphanumeric characters on passwords.

image
image

I got a success with a valid creds, but omitted it from the screenshot.

@neelvirdy neelvirdy changed the title Nvirdy/session tokens IsSession on auth token and validate admin passwords on setup Jan 10, 2023
@neelvirdy neelvirdy marked this pull request as ready for review January 10, 2023 21:33
@neelvirdy neelvirdy force-pushed the nvirdy/session-tokens branch 2 times, most recently from 37c7eff to f928c3d Compare January 10, 2023 21:57
@neelvirdy neelvirdy requested a review from anjor January 10, 2023 21:58
@neelvirdy neelvirdy changed the title IsSession on auth token and validate admin passwords on setup IsSession on auth token and validate admin creds on setup Jan 10, 2023
@en0ma
Copy link
Contributor

en0ma commented Jan 11, 2023

@neelvirdy how did you come up with the term session token for the admin API key?

@en0ma
Copy link
Contributor

en0ma commented Jan 11, 2023

Session tokens will not be rendered in the api admin page so users cannot mix up their service and session tokens

Makes sense, since it's a one-time setup key (session?) no need to show it in the UI. My question now is, does this mean the admin after logging in will have to then create a service API key if they need a key?

@en0ma
Copy link
Contributor

en0ma commented Jan 11, 2023

When running ./estuary setup, validates admin username and password to be compatible with bcrypt's requirement of 1-32 alphanumeric characters on usernames and 8+ alphanumeric characters on passwords.

Would this play nicely with already existing credentials?

@neelvirdy
Copy link
Contributor Author

neelvirdy commented Jan 11, 2023

@neelvirdy how did you come up with the term session token for the admin API key?

The session field actually doesn't correspond to admin vs regular user. It's purely whether or not the token was generated through login/signup, or if it was a manually provisioned key. In otherwords, session tokens will live in browser cookies exclusively. Service tokens are for all other use cases exclusively (scripts, curls, and other services calling estuary).

The admin creds are relevant to this change because admin login on estuary-www was not working, which forced us to authenticate via API key instead. But, authenticating via API key introduces a risk of mixing up session vs service tokens which defeats some of the purpose of the distinction. This is why the linked estuary-www PR fixes the admin login in addition to making the change to not render session tokens.

Session tokens will not be rendered in the api admin page so users cannot mix up their service and session tokens

Makes sense, since it's a one-time setup key (session?) no need to show it in the UI. My question now is, does this mean the admin after logging in will have to then create a service API key if they need a key?

Login creates a key, so the admin will get their first key simply by logging in with the credentials they ran ./estuary setup with.

When running ./estuary setup, validates admin username and password to be compatible with bcrypt's requirement of 1-32 alphanumeric characters on usernames and 8+ alphanumeric characters on passwords.

Would this play nicely with already existing credentials?

Bcrypt already throws an error if these requirements are not met, we are simply catching it earlier so the user can't create a user in DB with invalid credentials, only to get a login failure down the road.

@neelvirdy neelvirdy requested a review from en0ma January 11, 2023 16:56
main.go Outdated Show resolved Hide resolved
en0ma
en0ma previously approved these changes Jan 17, 2023
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 requested a review from en0ma January 18, 2023 18:02
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

Copy link
Contributor

@alvin-reyes alvin-reyes 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 15fd59a into application-research:dev Jan 19, 2023
@neelvirdy neelvirdy deleted the nvirdy/session-tokens branch January 19, 2023 16:24
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