diff --git a/coderd/externalauth.go b/coderd/externalauth.go index a2d017ed43e0e..8f8514fa17442 100644 --- a/coderd/externalauth.go +++ b/coderd/externalauth.go @@ -351,15 +351,17 @@ func (api *API) listUserExternalAuths(rw http.ResponseWriter, r *http.Request) { if link.OAuthAccessToken != "" { cfg, ok := configs[link.ProviderID] if ok { - newLink, valid, err := cfg.RefreshToken(ctx, api.Database, link) + newLink, err := cfg.RefreshToken(ctx, api.Database, link) meta := db2sdk.ExternalAuthMeta{ - Authenticated: valid, + Authenticated: err == nil, } if err != nil { meta.ValidateError = err.Error() } + linkMeta[link.ProviderID] = meta + // Update the link if it was potentially refreshed. - if err == nil && valid { + if err == nil { links[i] = newLink } } diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 85e53f2e91f33..4852de3e860ce 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -95,9 +95,23 @@ func (c *Config) GenerateTokenExtra(token *oauth2.Token) (pqtype.NullRawMessage, }, nil } +// InvalidTokenError is a case where the "RefreshToken" failed to complete +// as a result of invalid credentials. Error contains the reason of the failure. +type InvalidTokenError string + +func (e InvalidTokenError) Error() string { + return string(e) +} + +func IsInvalidTokenError(err error) bool { + var invalidTokenError InvalidTokenError + return xerrors.As(err, &invalidTokenError) +} + // RefreshToken automatically refreshes the token if expired and permitted. -// It returns the token and a bool indicating if the token is valid. -func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, bool, error) { +// If an error is returned, the token is either invalid, or an error occurred. +// Use 'IsInvalidTokenError(err)' to determine the difference. +func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, error) { // If the token is expired and refresh is disabled, we prompt // the user to authenticate again. if c.NoRefresh && @@ -105,7 +119,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu // This is true for github, which has no expiry. !externalAuthLink.OAuthExpiry.IsZero() && externalAuthLink.OAuthExpiry.Before(dbtime.Now()) { - return externalAuthLink, false, nil + return externalAuthLink, InvalidTokenError("token expired, refreshing is disabled") } // This is additional defensive programming. Because TokenSource is an interface, @@ -123,14 +137,16 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu Expiry: externalAuthLink.OAuthExpiry, }).Token() if err != nil { - // Even if the token fails to be obtained, we still return false because - // we aren't trying to surface an error, we're just trying to obtain a valid token. - return externalAuthLink, false, nil + // Even if the token fails to be obtained, do not return the error as an error. + // TokenSource(...).Token() will always return the current token if the token is not expired. + // If it is expired, it will attempt to refresh the token, and if it cannot, it will fail with + // an error. This error is a reason the token is invalid. + return externalAuthLink, InvalidTokenError(fmt.Sprintf("refresh token: %s", err.Error())) } extra, err := c.GenerateTokenExtra(token) if err != nil { - return externalAuthLink, false, xerrors.Errorf("generate token extra: %w", err) + return externalAuthLink, xerrors.Errorf("generate token extra: %w", err) } r := retry.New(50*time.Millisecond, 200*time.Millisecond) @@ -140,7 +156,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu validate: valid, _, err := c.ValidateToken(ctx, token) if err != nil { - return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err) + return externalAuthLink, xerrors.Errorf("validate external auth token: %w", err) } if !valid { // A customer using GitHub in Australia reported that validating immediately @@ -154,7 +170,7 @@ validate: goto validate } // The token is no longer valid! - return externalAuthLink, false, nil + return externalAuthLink, InvalidTokenError("token failed to validate") } if token.AccessToken != externalAuthLink.OAuthAccessToken { @@ -170,11 +186,11 @@ validate: OAuthExtra: extra, }) if err != nil { - return updatedAuthLink, false, xerrors.Errorf("update external auth link: %w", err) + return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err) } externalAuthLink = updatedAuthLink } - return externalAuthLink, true, nil + return externalAuthLink, nil } // ValidateToken ensures the Git token provided is valid! diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 88f3b7a3b59e9..fbc1cab4b7091 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -59,9 +59,10 @@ func TestRefreshToken(t *testing.T) { // Expire the link link.OAuthExpiry = expired - _, refreshed, err := config.RefreshToken(ctx, nil, link) - require.NoError(t, err) - require.False(t, refreshed) + _, err := config.RefreshToken(ctx, nil, link) + require.Error(t, err) + require.True(t, externalauth.IsInvalidTokenError(err)) + require.Contains(t, err.Error(), "refreshing is disabled") }) // NoRefreshNoExpiry tests that an oauth token without an expiry is always valid. @@ -90,9 +91,8 @@ func TestRefreshToken(t *testing.T) { // Zero time used link.OAuthExpiry = time.Time{} - _, refreshed, err := config.RefreshToken(ctx, nil, link) + _, err := config.RefreshToken(ctx, nil, link) require.NoError(t, err) - require.True(t, refreshed, "token without expiry is always valid") require.True(t, validated, "token should have been validated") }) @@ -105,11 +105,12 @@ func TestRefreshToken(t *testing.T) { }, }, } - _, refreshed, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{ + _, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{ OAuthExpiry: expired, }) - require.NoError(t, err) - require.False(t, refreshed) + require.Error(t, err) + require.True(t, externalauth.IsInvalidTokenError(err)) + require.Contains(t, err.Error(), "failure") }) t.Run("ValidateServerError", func(t *testing.T) { @@ -131,8 +132,12 @@ func TestRefreshToken(t *testing.T) { ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) link.OAuthExpiry = expired - _, _, err := config.RefreshToken(ctx, nil, link) + _, err := config.RefreshToken(ctx, nil, link) require.ErrorContains(t, err, staticError) + // Unsure if this should be the correct behavior. It's an invalid token because + // 'ValidateToken()' failed with a runtime error. This was the previous behavior, + // so not going to change it. + require.False(t, externalauth.IsInvalidTokenError(err)) require.True(t, validated, "token should have been attempted to be validated") }) @@ -156,9 +161,9 @@ func TestRefreshToken(t *testing.T) { ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) link.OAuthExpiry = expired - _, refreshed, err := config.RefreshToken(ctx, nil, link) - require.NoError(t, err, staticError) - require.False(t, refreshed) + _, err := config.RefreshToken(ctx, nil, link) + require.ErrorContains(t, err, "token failed to validate") + require.True(t, externalauth.IsInvalidTokenError(err)) require.True(t, validated, "token should have been attempted to be validated") }) @@ -191,9 +196,8 @@ func TestRefreshToken(t *testing.T) { // Unlimited lifetime, this is what GitHub returns tokens as link.OAuthExpiry = time.Time{} - _, ok, err := config.RefreshToken(ctx, nil, link) + _, err := config.RefreshToken(ctx, nil, link) require.NoError(t, err) - require.True(t, ok) require.Equal(t, 2, validateCalls, "token should have been attempted to be validated more than once") }) @@ -219,9 +223,8 @@ func TestRefreshToken(t *testing.T) { ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil)) - _, ok, err := config.RefreshToken(ctx, nil, link) + _, err := config.RefreshToken(ctx, nil, link) require.NoError(t, err) - require.True(t, ok) require.Equal(t, 1, validateCalls, "token is validated") }) @@ -253,9 +256,8 @@ func TestRefreshToken(t *testing.T) { // Force a refresh link.OAuthExpiry = expired - updated, ok, err := config.RefreshToken(ctx, db, link) + updated, err := config.RefreshToken(ctx, db, link) require.NoError(t, err) - require.True(t, ok) require.Equal(t, 1, validateCalls, "token is validated") require.Equal(t, 1, refreshCalls, "token is refreshed") require.NotEqualf(t, link.OAuthAccessToken, updated.OAuthAccessToken, "token is updated") @@ -292,9 +294,9 @@ func TestRefreshToken(t *testing.T) { // Force a refresh link.OAuthExpiry = expired - updated, ok, err := config.RefreshToken(ctx, db, link) + updated, err := config.RefreshToken(ctx, db, link) require.NoError(t, err) - require.True(t, ok) + require.True(t, updated.OAuthExtra.Valid) extra := map[string]interface{}{} require.NoError(t, json.Unmarshal(updated.OAuthExtra.RawMessage, &extra)) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 3f5876d644617..8c8a3f64beed1 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -559,16 +559,17 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo continue } - link, valid, err := config.RefreshToken(ctx, s.Database, link) - if err != nil { + refreshed, err := config.RefreshToken(ctx, s.Database, link) + if err != nil && !externalauth.IsInvalidTokenError(err) { return nil, failJob(fmt.Sprintf("refresh external auth link %q: %s", p.ID, err)) } - if !valid { + if err != nil { + // Invalid tokens are skipped continue } externalAuthProviders = append(externalAuthProviders, &sdkproto.ExternalAuthProvider{ Id: p.ID, - AccessToken: link.OAuthAccessToken, + AccessToken: refreshed.OAuthAccessToken, }) } diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 788a01ba353b1..1c9131ef0d17c 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -353,21 +353,16 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ return } - _, updated, err := config.RefreshToken(ctx, api.Database, authLink) - if err != nil { + _, err = config.RefreshToken(ctx, api.Database, authLink) + if err != nil && !externalauth.IsInvalidTokenError(err) { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to refresh external auth token.", Detail: err.Error(), }) return } - // If the token couldn't be validated, then we assume the user isn't - // authenticated and return early. - if !updated { - providers = append(providers, provider) - continue - } - provider.Authenticated = true + + provider.Authenticated = err == nil providers = append(providers, provider) } diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 1821948572e29..753e3aeaa0639 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -1912,25 +1912,25 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ return } - externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink) - if err != nil { + refreshedLink, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink) + if err != nil && !externalauth.IsInvalidTokenError(err) { handleRetrying(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to refresh external auth token.", Detail: err.Error(), }) return } - if !valid { + if err != nil { // 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 + previousToken = &refreshedLink handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{ URL: redirectURL.String(), }) return } - resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra) + resp, err := createExternalAuthResponse(externalAuthConfig.Type, refreshedLink.OAuthAccessToken, refreshedLink.OAuthExtra) if err != nil { handleRetrying(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to create external auth response.",