chore: merge apikey/token session config values #12817
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.
This is what has been bothering me. I commented it on the new struct to hopefully control any more spread of this.
Fixing it altogether is going to require a larger refactor.
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.
This should be called MaximumDuration instead of max_token_lifetime. Lifetime is already in the struct name.
There was a problem hiding this comment.
Token should be in the name though because this only applies to tokens, not api_keys.
I'll make it MaximumTokenDuration.
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
3c8eb4f to
d3b5575
Compare
| @@ -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.
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.
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 omit MaximumTokenDuration 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 the createAPIKey function doesn't lend itself well to it at present.
I could rename MaximumTokenDuration -> MaxTokenLifetime. It just feels werid to say "MaxLifetime" as we allow refreshing some sessions, but not others. I like the word Duration more personally.
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