8000 chore: merge apikey/token session config values by Emyrk · Pull Request #12817 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 10, 2024
Merged

chore: merge apikey/token session config values #12817

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f0710d9
chore: merge apikey/token session config values
Emyrk Mar 29, 2024
cdb9ed3
chore: add comment explaining all the ambiguity
Emyrk Mar 29, 2024
282d0cd
fix typo
Emyrk Mar 29, 2024
3aa8309
fix references
Emyrk Mar 29, 2024
863688f
make gen
Emyrk Mar 29, 2024
9ecae23
make fmt
Emyrk Mar 29, 2024
d609db3
renames
Emyrk Apr 2, 2024
d3b5575
make gen
Emyrk Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
chore: merge apikey/token session config values 
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
  • Loading branch information
@Emyrk
Emyrk committed Apr 3, 2024
commit f0710d916882af947c1c76774bfc4f0fbe57049b
6 changes: 3 additions & 3 deletions coderd/apikey.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.MaxTokenLifetime.Value(),
},
)
}
Expand All @@ -364,10 +364,10 @@ func (api *API) validateAPIKeyLifetime(lifetime time.Duration) error {
return xerrors.New("lifetime must be positive number greater than 0")
}

if lifetime > api.DeploymentValues.MaxTokenLifetime.Value() {
if lifetime > api.DeploymentValues.Sessions.MaxTokenLifetime.Value() {
return xerrors.Errorf(
"lifetime must be less than %v",
api.DeploymentValues.MaxTokenLifetime,
api.DeploymentValues.Sessions.MaxTokenLifetime,
)
}

Expand Down
30 changes: 15 additions & 15 deletions coderd/identityprovider/tokens.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ func extractTokenParams(r *http.Request, callbackURL *url.URL) (tokenParams, []c
return params, nil, nil
}

func Tokens(db database.Store, defaultLifetime time.Duration) http.HandlerFunc {
// Tokens
// TODO: the sessions lifetime config passed is for coder api tokens.
// Should er have a separate config for oauth2 tokens? They are related,
// but they are not the same.
func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerFunc {
return func(rw http.ResponseWriter, r *http.Request) {
ctx := r.Context()
app := httpmw.OAuth2ProviderApp(r)
Expand Down Expand Up @@ -104,9 +108,9 @@ func Tokens(db database.Store, defaultLifetime time.Duration) http.HandlerFunc {
switch params.grantType {
// TODO: Client creds, device code.
case codersdk.OAuth2ProviderGrantTypeRefreshToken:
token, err = refreshTokenGrant(ctx, db, app, defaultLifetime, params)
token, err = refreshTokenGrant(ctx, db, app, lifetimes, params)
case codersdk.OAuth2ProviderGrantTypeAuthorizationCode:
token, err = authorizationCodeGrant(ctx, db, app, defaultLifetime, params)
token, err = authorizationCodeGrant(ctx, db, app, lifetimes, params)
default:
// Grant types are validated by the parser, so getting through here means
// the developer added a type but forgot to add a case here.
Expand Down Expand Up @@ -137,7 +141,7 @@ func Tokens(db database.Store, defaultLifetime time.Duration) http.HandlerFunc {
}
}

func authorizationCodeGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, defaultLifetime time.Duration, params tokenParams) (oauth2.Token, error) {
func authorizationCodeGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, lifetimes codersdk.SessionLifetime, params tokenParams) (oauth2.Token, error) {
// Validate the client secret.
secret, err := parseSecret(params.clientSecret)
if err != nil {
Expand Down Expand Up @@ -195,11 +199,9 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
// TODO: We are ignoring scopes for now.
tokenName := fmt.Sprintf("%s_%s_oauth_session_token", dbCode.UserID, app.ID)
key, sessionToken, err := apikey.Generate(apikey.CreateParams{
UserID: dbCode.UserID,
LoginType: database.LoginTypeOAuth2ProviderApp,
// TODO: This is just the lifetime for api keys, maybe have its own config
// settings. #11693
DefaultLifetime: defaultLifetime,
UserID: dbCode.UserID,
LoginType: database.LoginTypeOAuth2ProviderApp,
SessionCfg: lifetimes,
// For now, we allow only one token per app and user at a time.
TokenName: tokenName,
})
Expand Down Expand Up @@ -271,7 +273,7 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
}, nil
}

func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, defaultLifetime time.Duration, params tokenParams) (oauth2.Token, error) {
func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, lifetimes codersdk.SessionLifetime, params tokenParams) (oauth2.Token, error) {
// Validate the token.
token, err := parseSecret(params.refreshToken)
if err != nil {
Expand Down Expand Up @@ -326,11 +328,9 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
// TODO: We are ignoring scopes for now.
tokenName := fmt.Sprintf("%s_%s_oauth_session_token", prevKey.UserID, app.ID)
key, sessionToken, err := apikey.Generate(apikey.CreateParams{
UserID: prevKey.UserID,
LoginType: database.LoginTypeOAuth2ProviderApp,
// TODO: This is just the lifetime for api keys, maybe have its own config
// settings. #11693
DefaultLifetime: defaultLifetime,
UserID: prevKey.UserID,
LoginType: database.LoginTypeOAuth2ProviderApp,
SessionCfg: lifetimes,
// For now, we allow only one token per app and user at a time.
TokenName: tokenName,
})
Expand Down
2 changes: 1 addition & 1 deletion coderd/oauth2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc {
// @Success 200 {object} oauth2.Token
// @Router /oauth2/tokens [post]
func (api *API) postOAuth2ProviderAppToken() http.HandlerFunc {
return identityprovider.Tokens(api.Database, api.DeploymentValues.SessionDuration.Value())
return identityprovider.Tokens(api.Database, api.DeploymentValues.Sessions)
}

// @Summary Delete OAuth2 application tokens.
Expand Down
9 changes: 4 additions & 5 deletions coderd/provisionerdserver/provisionerdserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1723,11 +1723,10 @@ func workspaceSessionTokenName(workspace database.Workspace) string {

func (s *server) regenerateSessionToken(ctx context.Context, user database.User, workspace database.Workspace) (string, error) {
newkey, sessionToken, err := apikey.Generate(apikey.CreateParams{
UserID: user.ID,
LoginType: user.LoginType,
DefaultLifetime: s.DeploymentValues.SessionDuration.Value(),
TokenName: workspaceSessionTokenName(workspace),
LifetimeSeconds: int64(s.DeploymentValues.MaxTokenLifetime.Value().Seconds()),
UserID: user.ID,
LoginType: user.LoginType,
SessionCfg: s.DeploymentValues.Sessions,
TokenName: workspaceSessionTokenName(workspace),
})
if err != nil {
return "", xerrors.Errorf("generate API key: %w", err)
Expand Down
2 changes: 1 addition & 1 deletion coderd/workspaceapps/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r *
DB: p.Database,
OAuth2Configs: p.OAuth2Configs,
RedirectToLogin: false,
DisableSessionExpiryRefresh: p.DeploymentValues.DisableSessionExpiryRefresh.Value(),
DisableSessionExpiryRefresh: p.DeploymentValues.Sessions.DisableSessionExpiryRefresh.Value(),
// Optional is true to allow for public apps. If the authorization check
// (later on) fails and the user is not authenticated, they will be
// redirected to the login page or app auth endpoint using code below.
Expand Down
19 changes: 13 additions & 6 deletions codersdk/deployment.go
View file
Open in desktop
A511
Original file line number Diff line number Diff line change
Expand Up @@ -182,13 +182,11 @@ type DeploymentValues struct {
RateLimit RateLimitConfig `json:"rate_limit,omitempty" typescript:",notnull"`
Experiments serpent.StringArray `json:"experiments,omitempty" typescript:",notnull"`
UpdateCheck serpent.Bool `json:"update_check,omitempty" typescript:",notnull"`
MaxTokenLifetime serpent.Duration `json:"max_token_lifetime,omitempty" typescript:",notnull"`
Swagger SwaggerConfig `json:"swagger,omitempty" typescript:",notnull"`
Logging LoggingConfig `json:"logging,omitempty" typescript:",notnull"`
Dangerous DangerousConfig `json:"dangerous,omitempty" typescript:",notnull"`
DisablePathApps serpent.Bool `json:"disable_path_apps,omitempty" typescript:",notnull"`
SessionDuration serpent.Duration `json:"max_session_expiry,omitempty" typescript:",notnull"`
DisableSessionExpiryRefresh serpent.Bool `json:"disable_session_expiry_refresh,omitempty" typescript:",notnull"`
Sessions SessionLifetime `json:"session_lifetime,omitempty" typescript:",notnull"`
DisablePasswordAuth serpent.Bool `json:"disable_password_auth,omitempty" typescript:",notnull"`
Support SupportConfig `json:"support,omitempty" typescript:",notnull"`
ExternalAuthConfigs serpent.Struct[[]ExternalAuthConfig] `json:"external_auth,omitempty" typescript:",notnull"`
Expand Down Expand Up @@ -244,6 +242,15 @@ func ParseSSHConfigOption(opt string) (key string, value string, err error) {
return opt[:idx], opt[idx+1:], nil
}

// SessionLifetime should be any configuration related to creating apikeys and tokens.
type SessionLifetime struct {
// DefaultSessionDuration is for api keys, not tokens.
DefaultSessionDuration serpent.Duration `json:"max_session_expiry" typescript:",notnull"`
DisableSessionExpiryRefresh serpent.Bool `json:"disable_session_expiry_refresh,omitempty" typescript:",notnull"`

MaxTokenLifetime serpent.Duration `json:"max_token_lifetime,omitempty" typescript:",notnull"`
Copy link
Member

Choose a reason for hiding this comment

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

This should be called MaximumDuration instead of max_token_lifetime. Lifetime is already in the struct name.

Copy link
Member Author

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 to tokens, not api_keys.

I'll make it MaximumTokenDuration.

}

type DERP struct {
Server DERPServerConfig `json:"server" typescript:",notnull"`
Config DERPConfig `json:"config" typescript:",notnull"`
Expand Down Expand Up @@ -1579,7 +1586,7 @@ when required by your organization's security policy.`,
// We have to add in the 25 leap days for the frontend to show the
// "100 years" correctly.
Default: ((100 * 365 * time.Hour * 24) + (25 * time.Hour * 24)).String(),
Value: &c.MaxTokenLifetime,
Value: &c.Sessions.MaxTokenLifetime,
Group: &deploymentGroupNetworkingHTTP,
YAML: "maxTokenLifetime",
Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"),
Expand Down Expand Up @@ -1773,7 +1780,7 @@ when required by your organization's security policy.`,
Flag: "session-duration",
Env: "CODER_SESSION_DURATION",
Default: (24 * time.Hour).String(),
Value: &c.SessionDuration,
Value: &c.Sessions.DefaultSessionDuration,
Group: &deploymentGroupNetworkingHTTP,
YAML: "sessionDuration",
Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"),
Expand All @@ -1784,7 +1791,7 @@ when required by your organization's security policy.`,
Flag: "disable-session-expiry-refresh",
Env: "CODER_DISABLE_SESSION_EXPIRY_REFRESH",

Value: &c.DisableSessionExpiryRefresh,
Value: &c.Sessions.DisableSessionExpiryRefresh,
Group: &deploymentGroupNetworkingHTTP,
YAML: "disableSessionExpiryRefresh",
},
Expand Down
0