- Notifications
You must be signed in to change notification settings - Fork928
chore: merge apikey/token session config values#12817
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
codersdk/deployment.go Outdated
| // SessionLifetime refers to "sessions" authenticating into Coderd. Coder has | ||
| // multiple different session types: api keys, tokens, workspace app tokens, | ||
| // agent tokens, etc. This configuration struct should be used to group all | ||
| // settings referring to any of these session lifetime controls. | ||
| // TODO: These config options were created back when coder only had api keys. | ||
| // Today, the config is ambigously used for all of them. For example: | ||
| // - cli based api keys ignore all settings | ||
| // - login uses the default lifetime, not the MaxTokenLifetime | ||
| // - Tokens use the Default & MaxTokenLifetime | ||
| // - ... etc ... | ||
| // The rational behind each decision is undocumented. The naming behind these | ||
| // config options is also confusing without any clear documentation. | ||
| // 'CreateAPIKey' is used to make all sessions, and it's parameters are just | ||
| // 'LifetimeSeconds' and 'DefaultLifetime'. Which does not directly correlate to | ||
| // the config options here. | ||
| type SessionLifetime struct { | ||
| // DisableSessionExpiryRefresh will disable automatically refreshing api | ||
| // keys when they are used from the api. This means the api key lifetime at | ||
| // creation is the lifetime of the api key. | ||
| DisableSessionExpiryRefresh serpent.Bool `json:"disable_session_expiry_refresh,omitempty" typescript:",notnull"` | ||
| // DefaultSessionDuration is for api keys, not tokens. | ||
| DefaultSessionDuration serpent.Duration `json:"max_session_expiry" typescript:",notnull"` | ||
| MaxTokenLifetime serpent.Duration `json:"max_token_lifetime,omitempty" typescript:",notnull"` | ||
| } |
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.
This is what has been bothering me. I commented it on the new struct tohopefully control any more spread of this.
Fixing it altogether is going to require a larger refactor.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
codersdk/deployment.go Outdated
| // DefaultSessionDuration is for api keys, not tokens. | ||
| DefaultSessionDuration serpent.Duration `json:"max_session_expiry" typescript:",notnull"` | ||
| MaxTokenLifetime serpent.Duration `json:"max_token_lifetime,omitempty" typescript:",notnull"` |
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.
This should be calledMaximumDuration instead ofmax_token_lifetime. Lifetime is already in the struct name.
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.
Token should be in the name though because this only applies totokens, notapi_keys.
I'll make itMaximumTokenDuration.
There is a confusing difference between an apikey and a token. Thisdifference leaks into our configs. This change does not resolve thedifference. It only groups the config values to try and manage anybloat that occurs from adding more similar config values
3c8eb4f tod3b5575Compare| @@ -354,7 +354,7 @@ func (api *API) tokenConfig(rw http.ResponseWriter, r *http.Request) { | |||
| httpapi.Write( | |||
| r.Context(), rw, http.StatusOK, | |||
| codersdk.TokenConfig{ | |||
| MaxTokenLifetime: values.MaxTokenLifetime.Value(), | |||
| MaxTokenLifetime: values.Sessions.MaximumTokenDuration.Value(), | |||
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.
Instead of grouping this config value with sessions, should we leave it as is before? The naming discrepancy here is confusing
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.
I really want to group all concepts of a "Session" from the configuration perspective.
Essentially we have these different things, "token", vs "cli auth", vs "browser api key". They are all "Sessions", but we do not have good documentation what the differences are.
If I omitMaximumTokenDuration from the grouping, it's continuing the scattered concept.
I agree the current names are confusing, and I actually had this PR fixing the names, but thecreateAPIKey function doesn't lend itself well to it at present.
I could renameMaximumTokenDuration ->MaxTokenLifetime. It just feels werid to say "MaxLifetime" as we allow refreshingsome sessions, but not others. I like the wordDuration more personally.
Uh oh!
There was an error while loading.Please reload this page.
There is a confusing difference between an apikey and a token. This difference leaks into our configs. This change does not resolve the difference. It only groups the config values to try and manage any bloat that occurs from adding more similar config values.
This only documents and groups the setting ambiguity. It does not fix it.
I took a stab at fixing it in a simple way, but it's a little hairy. I think it would be better to come up with a better strategy to managing all the different session types.
Closes#11693