- Notifications
You must be signed in to change notification settings - Fork928
feat: Longer lived api keys for cli#1935
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| CASE @lifetime_seconds::bigint | ||
| WHEN0 THEN86400 | ||
| ELSE @lifetime_seconds::bigint | ||
| END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If the value is 0, we default to 24hrs
| lifeTime := time.Hour * 24 * 7 | ||
| sessionToken, created := api.createAPIKey(rw, r, database.InsertAPIKeyParams{ | ||
| UserID: user.ID, | ||
| LoginType: database.LoginTypePassword, | ||
| // All api generated keys will last 1 week. Browser login tokens have | ||
| // a shorter life. | ||
| ExpiresAt: database.Now().Add(lifeTime), | ||
| LifetimeSeconds: int64(lifeTime.Seconds()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
/login is still 24hrs. Creating api keys from the api (eg/cli-auth) is 1 week. Refreshes every hour as normal, so any activity every 7 days keeps it alive.
| CASE @lifetime_seconds::bigint | ||
| WHEN0 THEN86400 | ||
| ELSE @lifetime_seconds::bigint | ||
| END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't this be a default when creating the column rather than on insert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is a default on the column, but you cannot insert anil value to the auto-generated code. So the default zero value in the insert code is0. I want to treat that as a default of 24hrs. The column default was mainly for the migration to be happy.
The alternative is to put this default in the Go code, but we don't have a layer to gatekeep db functions. So I'd rather put this in the SQL where it's guaranteed to be checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If desired, we can fix this to useNULL to indicate a default value rather than0 when the next version of sqlc is released, with support forsqlc.narg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@dwahler yes. Ifnarg or I also sawsqlc.arg(name, nullable) is supported, then we can do exactly that 👍. And use the column default.
| // Default expires at to now+lifetime, or just 24hrs if not set | ||
| if params.ExpiresAt.IsZero() { | ||
| if params.LifetimeSeconds != 0 { | ||
| params.ExpiresAt = database.Now().Add(time.Duration(params.LifetimeSeconds) * time.Second) | ||
| } else { | ||
| params.ExpiresAt = database.Now().Add(24 * time.Hour) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you think we need an arbitrary amount of seconds for an API key to live for? I'm curious for@dwahler's thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We do if we want api keys to have different lifetimes. Currently the browser login api keys are valid for 24hrs. When we refresh them, we need to know how long to refresh them for (24hrs).
The cli keys will have a longer life (1 week). When you refresh, you need to know to add 1 week onto the expiresAt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure where it falls on the spectrum of "needs", but I can definitely imagine this being useful, especially to enterprise users.
If the point of expiring keys is to mitigate risk, then you might want to make that tradeoff differently when the risk level is different. For instance, shorter lifetimes for keys that are stored on laptops (potentially prone to theft) or for admin users (where the consequences of key compromise are greater).
For now, I like specifying the lifetime as a request parameter, because it seems like the simplest way to expose this functionality. In the future it might make more sense for admins to be able to control the lifetimes with some kind of policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@dwahler It's not a configurable param from the api atm. There is just 2 endpoints (login and cli-auth) that make keys. But this backend does lead to it being easily configurable in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, gotcha, I missed thatparams wasn't directly populated from the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@dwahler I didn't think it was necessary at this time, since it's 2 different routes.
| // Default expires at to now+lifetime, or just 24hrs if not set | ||
| if params.ExpiresAt.IsZero() { | ||
| if params.LifetimeSeconds != 0 { | ||
| params.ExpiresAt = database.Now().Add(time.Duration(params.LifetimeSeconds) * time.Second) | ||
| } else { | ||
| params.ExpiresAt = database.Now().Add(24 * time.Hour) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, gotcha, I missed thatparams wasn't directly populated from the request.
| CASE @lifetime_seconds::bigint | ||
| WHEN0 THEN86400 | ||
| ELSE @lifetime_seconds::bigint | ||
| END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If desired, we can fix this to useNULL to indicate a default value rather than0 when the next version of sqlc is released, with support forsqlc.narg.
* feat: Longer lived api keys for cli* feat: Refresh tokens based on their lifetime set in the db* test: Add unit test for refreshing
Uh oh!
There was an error while loading.Please reload this page.
#1269
What this does
Cli auth now lasts 1 week. Token refreshes use
lifetime_secondscolumn on the api key to know how long to refresh