Skip to content

Commit 24ba819

Browse files
EmyrkEmyrk
authored
chore: return failed refresh errors on external auth as string (was boolean) (coder#13402)
* chore: return failed refresh errors on external auth Failed refreshes should return errors. These errors are captured as validate errors.
1 parent bf98b0d commit 24ba819

File tree

6 files changed

+68
-52
lines changed

6 files changed

+68
-52
lines changed

‎coderd/externalauth.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,15 +351,17 @@ func (api *API) listUserExternalAuths(rw http.ResponseWriter, r *http.Request) {
351351
if link.OAuthAccessToken != "" {
352352
cfg, ok := configs[link.ProviderID]
353353
if ok {
354-
newLink, valid, err := cfg.RefreshToken(ctx, api.Database, link)
354+
newLink, err := cfg.RefreshToken(ctx, api.Database, link)
355355
meta := db2sdk.ExternalAuthMeta{
356-
Authenticated: valid,
356+
Authenticated: err == nil,
357357
}
358358
if err != nil {
359359
meta.ValidateError = err.Error()
360360
}
361+
linkMeta[link.ProviderID] = meta
362+
361363
// Update the link if it was potentially refreshed.
362-
if err == nil && valid {
364+
if err == nil {
363365
links[i] = newLink
364366
}
365367
}

‎coderd/externalauth/externalauth.go

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -95,17 +95,31 @@ func (c *Config) GenerateTokenExtra(token *oauth2.Token) (pqtype.NullRawMessage,
9595
}, nil
9696
}
9797

98+
// InvalidTokenError is a case where the "RefreshToken" failed to complete
99+
// as a result of invalid credentials. Error contains the reason of the failure.
100+
type InvalidTokenError string
101+
102+
func (e InvalidTokenError) Error() string {
103+
return string(e)
104+
}
105+
106+
func IsInvalidTokenError(err error) bool {
107+
var invalidTokenError InvalidTokenError
108+
return xerrors.As(err, &invalidTokenError)
109+
}
110+
98111
// RefreshToken automatically refreshes the token if expired and permitted.
99-
// It returns the token and a bool indicating if the token is valid.
100-
func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, bool, error) {
112+
// If an error is returned, the token is either invalid, or an error occurred.
113+
// Use 'IsInvalidTokenError(err)' to determine the difference.
114+
func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, error) {
101115
// If the token is expired and refresh is disabled, we prompt
102116
// the user to authenticate again.
103117
if c.NoRefresh &&
104118
// If the time is set to 0, then it should never expire.
105119
// This is true for github, which has no expiry.
106120
!externalAuthLink.OAuthExpiry.IsZero() &&
107121
externalAuthLink.OAuthExpiry.Before(dbtime.Now()) {
108-
return externalAuthLink, false, nil
122+
return externalAuthLink, InvalidTokenError("token expired, refreshing is disabled")
109123
}
110124

111125
// 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
123137
Expiry: externalAuthLink.OAuthExpiry,
124138
}).Token()
125139
if err != nil {
126-
// Even if the token fails to be obtained, we still return false because
127-
// we aren't trying to surface an error, we're just trying to obtain a valid token.
128-
return externalAuthLink, false, nil
140+
// Even if the token fails to be obtained, do not return the error as an error.
141+
// TokenSource(...).Token() will always return the current token if the token is not expired.
142+
// If it is expired, it will attempt to refresh the token, and if it cannot, it will fail with
143+
// an error. This error is a reason the token is invalid.
144+
return externalAuthLink, InvalidTokenError(fmt.Sprintf("refresh token: %s", err.Error()))
129145
}
130146

131147
extra, err := c.GenerateTokenExtra(token)
132148
if err != nil {
133-
return externalAuthLink, false, xerrors.Errorf("generate token extra: %w", err)
149+
return externalAuthLink, xerrors.Errorf("generate token extra: %w", err)
134150
}
135151

136152
r := retry.New(50*time.Millisecond, 200*time.Millisecond)
@@ -140,7 +156,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
140156
validate:
141157
valid, _, err := c.ValidateToken(ctx, token)
142158
if err != nil {
143-
return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err)
159+
return externalAuthLink, xerrors.Errorf("validate external auth token: %w", err)
144160
}
145161
if !valid {
146162
// A customer using GitHub in Australia reported that validating immediately
@@ -154,7 +170,7 @@ validate:
154170
goto validate
155171
}
156172
// The token is no longer valid!
157-
return externalAuthLink, false, nil
173+
return externalAuthLink, InvalidTokenError("token failed to validate")
158174
}
159175

160176
if token.AccessToken != externalAuthLink.OAuthAccessToken {
@@ -170,11 +186,11 @@ validate:
170186
OAuthExtra: extra,
171187
})
172188
if err != nil {
173-
return updatedAuthLink, false, xerrors.Errorf("update external auth link: %w", err)
189+
return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err)
174190
}
175191
externalAuthLink = updatedAuthLink
176192
}
177-
return externalAuthLink, true, nil
193+
return externalAuthLink, nil
178194
}
179195

180196
// ValidateToken ensures the Git token provided is valid!

‎coderd/externalauth/externalauth_test.go

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,10 @@ func TestRefreshToken(t *testing.T) {
5959
// Expire the link
6060
link.OAuthExpiry = expired
6161

62-
_, refreshed, err := config.RefreshToken(ctx, nil, link)
63-
require.NoError(t, err)
64-
require.False(t, refreshed)
62+
_, err := config.RefreshToken(ctx, nil, link)
63+
require.Error(t, err)
64+
require.True(t, externalauth.IsInvalidTokenError(err))
65+
require.Contains(t, err.Error(), "refreshing is disabled")
6566
})
6667

6768
// NoRefreshNoExpiry tests that an oauth token without an expiry is always valid.
@@ -90,9 +91,8 @@ func TestRefreshToken(t *testing.T) {
9091

9192
// Zero time used
9293
link.OAuthExpiry = time.Time{}
93-
_, refreshed, err := config.RefreshToken(ctx, nil, link)
94+
_, err := config.RefreshToken(ctx, nil, link)
9495
require.NoError(t, err)
95-
require.True(t, refreshed, "token without expiry is always valid")
9696
require.True(t, validated, "token should have been validated")
9797
})
9898

@@ -105,11 +105,12 @@ func TestRefreshToken(t *testing.T) {
105105
},
106106
},
107107
}
108-
_, refreshed, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
108+
_, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
109109
OAuthExpiry: expired,
110110
})
111-
require.NoError(t, err)
112-
require.False(t, refreshed)
111+
require.Error(t, err)
112+
require.True(t, externalauth.IsInvalidTokenError(err))
113+
require.Contains(t, err.Error(), "failure")
113114
})
114115

115116
t.Run("ValidateServerError", func(t *testing.T) {
@@ -131,8 +132,12 @@ func TestRefreshToken(t *testing.T) {
131132
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
132133
link.OAuthExpiry = expired
133134

134-
_, _, err := config.RefreshToken(ctx, nil, link)
135+
_, err := config.RefreshToken(ctx, nil, link)
135136
require.ErrorContains(t, err, staticError)
137+
// Unsure if this should be the correct behavior. It's an invalid token because
138+
// 'ValidateToken()' failed with a runtime error. This was the previous behavior,
139+
// so not going to change it.
140+
require.False(t, externalauth.IsInvalidTokenError(err))
136141
require.True(t, validated, "token should have been attempted to be validated")
137142
})
138143

@@ -156,9 +161,9 @@ func TestRefreshToken(t *testing.T) {
156161
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
157162
link.OAuthExpiry = expired
158163

159-
_, refreshed, err := config.RefreshToken(ctx, nil, link)
160-
require.NoError(t, err, staticError)
161-
require.False(t, refreshed)
164+
_, err := config.RefreshToken(ctx, nil, link)
165+
require.ErrorContains(t, err, "token failed to validate")
166+
require.True(t, externalauth.IsInvalidTokenError(err))
162167
require.True(t, validated, "token should have been attempted to be validated")
163168
})
164169

@@ -191,9 +196,8 @@ func TestRefreshToken(t *testing.T) {
191196
// Unlimited lifetime, this is what GitHub returns tokens as
192197
link.OAuthExpiry = time.Time{}
193198

194-
_, ok, err := config.RefreshToken(ctx, nil, link)
199+
_, err := config.RefreshToken(ctx, nil, link)
195200
require.NoError(t, err)
196-
require.True(t, ok)
197201
require.Equal(t, 2, validateCalls, "token should have been attempted to be validated more than once")
198202
})
199203

@@ -219,9 +223,8 @@ func TestRefreshToken(t *testing.T) {
219223

220224
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
221225

222-
_, ok, err := config.RefreshToken(ctx, nil, link)
226+
_, err := config.RefreshToken(ctx, nil, link)
223227
require.NoError(t, err)
224-
require.True(t, ok)
225228
require.Equal(t, 1, validateCalls, "token is validated")
226229
})
227230

@@ -253,9 +256,8 @@ func TestRefreshToken(t *testing.T) {
253256
// Force a refresh
254257
link.OAuthExpiry = expired
255258

256-
updated, ok, err := config.RefreshToken(ctx, db, link)
259+
updated, err := config.RefreshToken(ctx, db, link)
257260
require.NoError(t, err)
258-
require.True(t, ok)
259261
require.Equal(t, 1, validateCalls, "token is validated")
260262
require.Equal(t, 1, refreshCalls, "token is refreshed")
261263
require.NotEqualf(t, link.OAuthAccessToken, updated.OAuthAccessToken, "token is updated")
@@ -292,9 +294,9 @@ func TestRefreshToken(t *testing.T) {
292294
// Force a refresh
293295
link.OAuthExpiry = expired
294296

295-
updated, ok, err := config.RefreshToken(ctx, db, link)
297+
updated, err := config.RefreshToken(ctx, db, link)
296298
require.NoError(t, err)
297-
require.True(t, ok)
299+
298300
require.True(t, updated.OAuthExtra.Valid)
299301
extra := map[string]interface{}{}
300302
require.NoError(t, json.Unmarshal(updated.OAuthExtra.RawMessage, &extra))

‎coderd/provisionerdserver/provisionerdserver.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -559,16 +559,17 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo
559559
continue
560560
}
561561

562-
link, valid, err := config.RefreshToken(ctx, s.Database, link)
563-
if err != nil {
562+
refreshed, err := config.RefreshToken(ctx, s.Database, link)
563+
if err != nil && !externalauth.IsInvalidTokenError(err) {
564564
return nil, failJob(fmt.Sprintf("refresh external auth link %q: %s", p.ID, err))
565565
}
566-
if !valid {
566+
if err != nil {
567+
// Invalid tokens are skipped
567568
continue
568569
}
569570
externalAuthProviders = append(externalAuthProviders, &sdkproto.ExternalAuthProvider{
570571
Id: p.ID,
571-
AccessToken: link.OAuthAccessToken,
572+
AccessToken: refreshed.OAuthAccessToken,
572573
})
573574
}
574575

‎coderd/templateversions.go

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -353,21 +353,16 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ
353353
return
354354
}
355355

356-
_, updated, err := config.RefreshToken(ctx, api.Database, authLink)
357-
if err != nil {
356+
_, err = config.RefreshToken(ctx, api.Database, authLink)
357+
if err != nil && !externalauth.IsInvalidTokenError(err) {
358358
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
359359
Message: "Failed to refresh external auth token.",
360360
Detail: err.Error(),
361361
})
362362
return
363363
}
364-
// If the token couldn't be validated, then we assume the user isn't
365-
// authenticated and return early.
366-
if !updated {
367-
providers = append(providers, provider)
368-
continue
369-
}
370-
provider.Authenticated = true
364+
365+
provider.Authenticated = err == nil
371366
providers = append(providers, provider)
372367
}
373368

‎coderd/workspaceagents.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1912,25 +1912,25 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
19121912
return
19131913
}
19141914

1915-
externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
1916-
if err != nil {
1915+
refreshedLink, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
1916+
if err != nil && !externalauth.IsInvalidTokenError(err) {
19171917
handleRetrying(http.StatusInternalServerError, codersdk.Response{
19181918
Message: "Failed to refresh external auth token.",
19191919
Detail: err.Error(),
19201920
})
19211921
return
19221922
}
1923-
if !valid {
1923+
if err != nil {
19241924
// Set the previous token so the retry logic will skip validating the
19251925
// same token again. This should only be set if the token is invalid and there
19261926
// was no error. If it is invalid because of an error, then we should recheck.
1927-
previousToken = &externalAuthLink
1927+
previousToken = &refreshedLink
19281928
handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
19291929
URL: redirectURL.String(),
19301930
})
19311931
return
19321932
}
1933-
resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra)
1933+
resp, err := createExternalAuthResponse(externalAuthConfig.Type, refreshedLink.OAuthAccessToken, refreshedLink.OAuthExtra)
19341934
if err != nil {
19351935
handleRetrying(http.StatusInternalServerError, codersdk.Response{
19361936
Message: "Failed to create external auth response.",

0 commit comments

Comments
 (0)