From 3b6a39f4e7e04262648f81f093222d5149207103 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 22 Jan 2024 16:23:55 -0600 Subject: [PATCH 1/9] fix: always attempt external auth refresh when fetching --- coderd/workspaceagents.go | 154 +++++++++++++++++++++----------------- 1 file changed, 84 insertions(+), 70 deletions(-) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index d438d6663d161..0cebd32cb1707 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -2031,78 +2031,25 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ return } - if listen { - // Since we're ticking frequently and this sign-in operation is rare, - // we are OK with polling to avoid the complexity of pubsub. - ticker, done := api.NewTicker(time.Second) - defer done() - var previousToken database.ExternalAuthLink - for { - select { - case <-ctx.Done(): - return - case <-ticker: - } - externalAuthLink, err := api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{ - ProviderID: externalAuthConfig.ID, - UserID: workspace.OwnerID, - }) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - continue - } - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to get external auth link.", - Detail: err.Error(), - }) - return - } - - // Expiry may be unset if the application doesn't configure tokens - // to expire. - // See - // https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-user-access-token-for-a-github-app. - if externalAuthLink.OAuthExpiry.Before(dbtime.Now()) && !externalAuthLink.OAuthExpiry.IsZero() { - continue - } - - // Only attempt to revalidate an oauth token if it has actually changed. - // No point in trying to validate the same token over and over again. - if previousToken.OAuthAccessToken == externalAuthLink.OAuthAccessToken && - previousToken.OAuthRefreshToken == externalAuthLink.OAuthRefreshToken && - previousToken.OAuthExpiry == externalAuthLink.OAuthExpiry { - continue - } - - valid, _, err := externalAuthConfig.ValidateToken(ctx, externalAuthLink.OAuthAccessToken) - if err != nil { - api.Logger.Warn(ctx, "failed to validate external auth token", - slog.F("workspace_owner_id", workspace.OwnerID.String()), - slog.F("validate_url", externalAuthConfig.ValidateURL), - slog.Error(err), - ) - } - previousToken = externalAuthLink - if !valid { - continue - } - resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to create external auth response.", - Detail: err.Error(), - }) - return - } - httpapi.Write(ctx, rw, http.StatusOK, resp) + // handleRetrying will attempt to continually check for a new token + // if listen is true. This is useful if an error is encountered in the + // original single flow. + // + // By default, if no errors are encountered, then the single flow response + // is returned. + handleRetrying := func(code int, response any) { + if !listen { + httpapi.Write(ctx, rw, code, response) return } + + api.workspaceAgentsExternalAuthListen(rw, ctx, externalAuthConfig, workspace) } // This is the URL that will redirect the user with a state token. redirectURL, err := api.AccessURL.Parse(fmt.Sprintf("/external-auth/%s", externalAuthConfig.ID)) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + handleRetrying(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to parse access URL.", Detail: err.Error(), }) @@ -2115,14 +2062,14 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ }) if err != nil { if !errors.Is(err, sql.ErrNoRows) { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + handleRetrying(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to get external auth link.", Detail: err.Error(), }) return } - httpapi.Write(ctx, rw, http.StatusOK, agentsdk.ExternalAuthResponse{ + handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{ URL: redirectURL.String(), }) return @@ -2130,21 +2077,21 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ externalAuthLink, updated, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + handleRetrying(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to refresh external auth token.", Detail: err.Error(), }) return } if !updated { - httpapi.Write(ctx, rw, http.StatusOK, agentsdk.ExternalAuthResponse{ + handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{ URL: redirectURL.String(), }) return } resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + handleRetrying(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to create external auth response.", Detail: err.Error(), }) @@ -2153,6 +2100,73 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ httpapi.Write(ctx, rw, http.StatusOK, resp) } +func (api *API) workspaceAgentsExternalAuthListen(rw http.ResponseWriter, ctx context.Context, externalAuthConfig *externalauth.Config, workspace database.Workspace) { + // Since we're ticking frequently and this sign-in operation is rare, + // we are OK with polling to avoid the complexity of pubsub. + ticker, done := api.NewTicker(time.Second) + defer done() + var previousToken database.ExternalAuthLink + for { + select { + case <-ctx.Done(): + return + case <-ticker: + } + externalAuthLink, err := api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{ + ProviderID: externalAuthConfig.ID, + UserID: workspace.OwnerID, + }) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + continue + } + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to get external auth link.", + Detail: err.Error(), + }) + return + } + + // Expiry may be unset if the application doesn't configure tokens + // to expire. + // See + // https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-user-access-token-for-a-github-app. + if externalAuthLink.OAuthExpiry.Before(dbtime.Now()) && !externalAuthLink.OAuthExpiry.IsZero() { + continue + } + + // Only attempt to revalidate an oauth token if it has actually changed. + // No point in trying to validate the same token over and over again. + if previousToken.OAuthAccessToken == externalAuthLink.OAuthAccessToken && + previousToken.OAuthRefreshToken == externalAuthLink.OAuthRefreshToken && + previousToken.OAuthExpiry == externalAuthLink.OAuthExpiry { + continue + } + + valid, _, err := externalAuthConfig.ValidateToken(ctx, externalAuthLink.OAuthAccessToken) + if err != nil { + api.Logger.Warn(ctx, "failed to validate external auth token", + slog.F("workspace_owner_id", workspace.OwnerID.String()), + slog.F("validate_url", externalAuthConfig.ValidateURL), + slog.Error(err), + ) + } + previousToken = externalAuthLink + if !valid { + continue + } + resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to create external auth response.", + Detail: err.Error(), + }) + return + } + httpapi.Write(ctx, rw, http.StatusOK, resp) + } +} + // createExternalAuthResponse creates an ExternalAuthResponse based on the // provider type. This is to support legacy `/workspaceagents/me/gitauth` // which uses `Username` and `Password`. From 5d0489b9f9cb2d888c8894b0f3e64976836cedb8 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 22 Jan 2024 16:51:07 -0600 Subject: [PATCH 2/9] refactor validate to check expiary --- coderd/database/modelmethods.go | 9 +++++++++ coderd/externalauth.go | 2 +- coderd/externalauth/externalauth.go | 13 ++++++++++--- coderd/promoauth/oauth2_test.go | 2 +- coderd/workspaceagents.go | 2 +- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index fd679115f4762..beaac600a6214 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -6,6 +6,7 @@ import ( "time" "golang.org/x/exp/maps" + "golang.org/x/oauth2" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/rbac" @@ -268,6 +269,14 @@ func (u ExternalAuthLink) RBACObject() rbac.Object { return rbac.ResourceUserData.WithID(u.UserID).WithOwner(u.UserID.String()) } +func (u ExternalAuthLink) OAuthToken() *oauth2.Token { + return &oauth2.Token{ + AccessToken: u.OAuthAccessToken, + RefreshToken: u.OAuthRefreshToken, + Expiry: u.OAuthExpiry, + } +} + func (u UserLink) RBACObject() rbac.Object { // I assume UserData is ok? return rbac.ResourceUserData.WithOwner(u.UserID.String()).WithID(u.UserID) diff --git a/coderd/externalauth.go b/coderd/externalauth.go index 001592e04e7db..a2d017ed43e0e 100644 --- a/coderd/externalauth.go +++ b/coderd/externalauth.go @@ -57,7 +57,7 @@ func (api *API) externalAuthByID(w http.ResponseWriter, r *http.Request) { } var eg errgroup.Group eg.Go(func() (err error) { - res.Authenticated, res.User, err = config.ValidateToken(ctx, link.OAuthAccessToken) + res.Authenticated, res.User, err = config.ValidateToken(ctx, link.OAuthToken()) return err }) eg.Go(func() (err error) { diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index d4d9f060e65d8..7b2a1b633db7c 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -138,7 +138,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu retryCtx, retryCtxCancel := context.WithTimeout(ctx, time.Second) defer retryCtxCancel() validate: - valid, _, err := c.ValidateToken(ctx, token.AccessToken) + valid, _, err := c.ValidateToken(ctx, token) if err != nil { return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err) } @@ -179,7 +179,14 @@ validate: // ValidateToken ensures the Git token provided is valid! // The user is optionally returned if the provider supports it. -func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *codersdk.ExternalAuthUser, error) { +func (c *Config) ValidateToken(ctx context.Context, link *oauth2.Token) (bool, *codersdk.ExternalAuthUser, error) { + if link == nil { + return false, nil, xerrors.New("validate external auth token: token is nil") + } + if !link.Expiry.IsZero() && link.Expiry.Before(dbtime.Now()) { + return false, nil, nil + } + if c.ValidateURL == "" { // Default that the token is valid if no validation URL is provided. return true, nil, nil @@ -189,7 +196,7 @@ func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *coders return false, nil, err } - req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", link.AccessToken)) res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceValidateToken, req) if err != nil { return false, nil, err diff --git a/coderd/promoauth/oauth2_test.go b/coderd/promoauth/oauth2_test.go index 0ee9c6fe6a6a3..4dce3d624824b 100644 --- a/coderd/promoauth/oauth2_test.go +++ b/coderd/promoauth/oauth2_test.go @@ -75,7 +75,7 @@ func TestInstrument(t *testing.T) { require.Equal(t, count("TokenSource"), 1) // Try a validate - valid, _, err := cfg.ValidateToken(ctx, refreshed.AccessToken) + valid, _, err := cfg.ValidateToken(ctx, refreshed) require.NoError(t, err) require.True(t, valid) require.Equal(t, count("ValidateToken"), 1) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 0cebd32cb1707..9ffff28d5a003 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -2143,7 +2143,7 @@ func (api *API) workspaceAgentsExternalAuthListen(rw http.ResponseWriter, ctx co continue } - valid, _, err := externalAuthConfig.ValidateToken(ctx, externalAuthLink.OAuthAccessToken) + valid, _, err := externalAuthConfig.ValidateToken(ctx, externalAuthLink.OAuthToken()) if err != nil { api.Logger.Warn(ctx, "failed to validate external auth token", slog.F("workspace_owner_id", workspace.OwnerID.String()), From 778184db53fa86ad0d58a7383a3c209bac815fda Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 23 Jan 2024 10:25:19 -0600 Subject: [PATCH 3/9] linting --- coderd/workspaceagents.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 9ffff28d5a003..f8835b12d1cc2 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -2100,7 +2100,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ httpapi.Write(ctx, rw, http.StatusOK, resp) } -func (api *API) workspaceAgentsExternalAuthListen(rw http.ResponseWriter, ctx context.Context, externalAuthConfig *externalauth.Config, workspace database.Workspace) { +func (api *API) workspaceAgentsExternalAuthListen(ctx context.Context, rw http.ResponseWriter, externalAuthConfig *externalauth.Config, workspace database.Workspace) { // Since we're ticking frequently and this sign-in operation is rare, // we are OK with polling to avoid the complexity of pubsub. ticker, done := api.NewTicker(time.Second) From 2281db28f4e2928a21f84645b067c36b6ea108f6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 23 Jan 2024 10:30:05 -0600 Subject: [PATCH 4/9] compile fix --- coderd/workspaceagents.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index f8835b12d1cc2..7afdd05aaabec 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -2043,7 +2043,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ return } - api.workspaceAgentsExternalAuthListen(rw, ctx, externalAuthConfig, workspace) + api.workspaceAgentsExternalAuthListen(ctx, rw, externalAuthConfig, workspace) } // This is the URL that will redirect the user with a state token. From 189c8ae588eca4be1e6da21fe700be5f6c5906cd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 23 Jan 2024 11:51:32 -0600 Subject: [PATCH 5/9] fix return --- coderd/workspaceagents.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 7afdd05aaabec..04ddc03aad15a 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -2164,6 +2164,7 @@ func (api *API) workspaceAgentsExternalAuthListen(ctx context.Context, rw http.R return } httpapi.Write(ctx, rw, http.StatusOK, resp) + return } } From 531f45e8061e4d8734a8493a711ad0b8a702940b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Jan 2024 14:36:42 -0600 Subject: [PATCH 6/9] fix extra re-validate call --- coderd/workspaceagents.go | 20 ++++++++++++++++---- coderd/workspaceagents_test.go | 5 +++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 04ddc03aad15a..1c328925aa015 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -2031,6 +2031,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ return } + var previousToken *database.ExternalAuthLink // handleRetrying will attempt to continually check for a new token // if listen is true. This is useful if an error is encountered in the // original single flow. @@ -2043,7 +2044,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ return } - api.workspaceAgentsExternalAuthListen(ctx, rw, externalAuthConfig, workspace) + api.workspaceAgentsExternalAuthListen(ctx, rw, previousToken, externalAuthConfig, workspace) } // This is the URL that will redirect the user with a state token. @@ -2075,7 +2076,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ return } - externalAuthLink, updated, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink) + externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink) if err != nil { handleRetrying(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to refresh external auth token.", @@ -2083,7 +2084,11 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ }) return } - if !updated { + if !valid { + // Set the previous token so the retry logic will skip validating the + // same token again. This should only be set if the token is invalid and there + // was no error. If it is invalid because of an error, then we should recheck. + previousToken = &externalAuthLink handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{ URL: redirectURL.String(), }) @@ -2100,12 +2105,19 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ httpapi.Write(ctx, rw, http.StatusOK, resp) } -func (api *API) workspaceAgentsExternalAuthListen(ctx context.Context, rw http.ResponseWriter, externalAuthConfig *externalauth.Config, workspace database.Workspace) { +func (api *API) workspaceAgentsExternalAuthListen(ctx context.Context, rw http.ResponseWriter, previous *database.ExternalAuthLink, externalAuthConfig *externalauth.Config, workspace database.Workspace) { // Since we're ticking frequently and this sign-in operation is rare, // we are OK with polling to avoid the complexity of pubsub. ticker, done := api.NewTicker(time.Second) defer done() + // If we have a previous token that is invalid, we should not check this again. + // This serves to prevent doing excessive unauthorized requests to the external + // auth provider. For github, this limit is 60 per hour, so saving a call + // per invalid token can be significant. var previousToken database.ExternalAuthLink + if previous != nil { + previousToken = *previous + } for { select { case <-ctx.Done(): diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 9d5fd8da1befd..e4efe164e8799 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1577,7 +1577,7 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) { }, ExternalAuthConfigs: []*externalauth.Config{ fake.ExternalAuthConfig(t, providerID, nil, func(cfg *externalauth.Config) { - cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + cfg.Type = codersdk.EnhancedExternalAuthProviderGitLab.String() }), }, }) @@ -1623,7 +1623,8 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) { ticks <- time.Now() } cancel() - // We expect only 1 + // We expect only 2. One from the initial "Refresh" attempt, and + // another from the first tick. Ideally this would be just one. // In a failed test, you will likely see 9, as the last one // gets canceled. require.Equal(t, 1, validateCalls, "validate calls duplicated on same token") From 68b8792e6f8d094b354e4045b92381268bc6d9a6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Jan 2024 15:29:04 -0600 Subject: [PATCH 7/9] handle no expiry --- coderd/externalauth/externalauth.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 7b2a1b633db7c..5ab113ede59e0 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -403,10 +403,15 @@ func (c *DeviceAuth) ExchangeDeviceCode(ctx context.Context, deviceCode string) if body.Error != "" { return nil, xerrors.New(body.Error) } + // If expiresIn is 0, then the token never expires. + expires := dbtime.Now().Add(time.Duration(body.ExpiresIn) * time.Second) + if body.ExpiresIn == 0 { + expires = time.Time{} + } return &oauth2.Token{ AccessToken: body.AccessToken, RefreshToken: body.RefreshToken, - Expiry: dbtime.Now().Add(time.Duration(body.ExpiresIn) * time.Second), + Expiry: expires, }, nil } From 0879fd6614f37e7df6b04c2c675278b2f3e565c9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 10:15:21 -0600 Subject: [PATCH 8/9] Fix comment --- coderd/workspaceagents_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index e4efe164e8799..ba688b936dd0b 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1623,8 +1623,8 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) { ticks <- time.Now() } cancel() - // We expect only 2. One from the initial "Refresh" attempt, and - // another from the first tick. Ideally this would be just one. + // We expect only 1. One from the initial "Refresh" attempt, and the + // other sshould be skipped. // In a failed test, you will likely see 9, as the last one // gets canceled. require.Equal(t, 1, validateCalls, "validate calls duplicated on same token") From 3d28fe756eb63ba2ddd2b12e42804d09fd0a075f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 10:16:22 -0600 Subject: [PATCH 9/9] fix typo --- coderd/workspaceagents_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index ba688b936dd0b..c85e8f7da830f 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1624,7 +1624,7 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) { } cancel() // We expect only 1. One from the initial "Refresh" attempt, and the - // other sshould be skipped. + // other should be skipped. // In a failed test, you will likely see 9, as the last one // gets canceled. require.Equal(t, 1, validateCalls, "validate calls duplicated on same token")