chore: return failed refresh errors on external auth as string (was boolean) (#13402)

* chore: return failed refresh errors on external auth

Failed refreshes should return errors. These errors are captured
as validate errors.
This commit is contained in:
Steven Masley
2024-06-03 09:33:49 -05:00
committed by GitHub
parent bf98b0dfe4
commit 24ba81930b
6 changed files with 68 additions and 52 deletions

View File

@ -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
}
}

View File

@ -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!

View File

@ -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))

View File

@ -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,
})
}

View File

@ -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)
}

View File

@ -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.",