Skip to content

Commit 6b21da8

Browse files
EmyrkEmyrk
committed
Make invalid not an error
1 parent 0f35152 commit 6b21da8

File tree

6 files changed

+56
-44
lines changed

6 files changed

+56
-44
lines changed

‎coderd/externalauth.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,15 +351,16 @@ 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, invalidReason, err := cfg.RefreshToken(ctx, api.Database, link)
355355
meta := db2sdk.ExternalAuthMeta{
356-
Authenticated: valid,
356+
Authenticated: invalidReason.Valid(),
357+
ValidateError: invalidReason.String(),
357358
}
358359
if err != nil {
359360
meta.ValidateError = err.Error()
360361
}
361362
// Update the link if it was potentially refreshed.
362-
if err == nil && valid {
363+
if err == nil && invalidReason.Valid() {
363364
links[i] = newLink
364365
}
365366
}

‎coderd/externalauth/externalauth.go

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

98+
// InvalidReason is intentionally typed and not an error. An invalid token is not an error,
99+
// but a state. A boolean true/false omits context and information that could be helpful in debugging.
100+
// So a string is returned with some additional information.
101+
type InvalidReason string
102+
103+
// Valid returns true if there is no reason to be invalid.
104+
func (r InvalidReason) Valid() bool {
105+
return r == ""
106+
}
107+
108+
// Invalid is easier to read than '!r.Valid()'
109+
func (r InvalidReason) Invalid() bool { return !r.Valid() }
110+
func (r InvalidReason) String() string { return string(r) }
111+
98112
// RefreshToken automatically refreshes the token if expired and permitted.
99113
// 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) {
114+
func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, InvalidReason, error) {
115+
const validReason InvalidReason = ""
116+
101117
// If the token is expired and refresh is disabled, we prompt
102118
// the user to authenticate again.
103119
if c.NoRefresh &&
104120
// If the time is set to 0, then it should never expire.
105121
// This is true for github, which has no expiry.
106122
!externalAuthLink.OAuthExpiry.IsZero() &&
107123
externalAuthLink.OAuthExpiry.Before(dbtime.Now()) {
108-
return externalAuthLink, false, nil
124+
return externalAuthLink, "token expired, refreshing is disabled", nil
109125
}
110126

111127
// This is additional defensive programming. Because TokenSource is an interface,
@@ -123,16 +139,16 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
123139
Expiry: externalAuthLink.OAuthExpiry,
124140
}).Token()
125141
if err != nil {
126-
// TokenSource will always return the current status token if not-expired.
127-
// If the token is expired, it will attempt to refresh. An error is returned
128-
// if the refresh fails, meaning the existing token is expired and this function
129-
// was unable to obtain a valid one.
130-
return externalAuthLink, false, err
142+
// Even if the token fails to be obtained, do not return the error as an error.
143+
// TokenSource(...).Token() will always return the current token if the token is not expired.
144+
// If it is expired, it will attempt to refresh the token, and if it cannot, it will fail with
145+
// an error. This error is a reason the token is invalid.
146+
return externalAuthLink, InvalidReason(fmt.Sprintf("refresh token: %s", err.Error())), nil
131147
}
132148

133149
extra, err := c.GenerateTokenExtra(token)
134150
if err != nil {
135-
return externalAuthLink, false, xerrors.Errorf("generate token extra: %w", err)
151+
return externalAuthLink, "generate extra token fields", xerrors.Errorf("generate token extra: %w", err)
136152
}
137153

138154
r := retry.New(50*time.Millisecond, 200*time.Millisecond)
@@ -142,7 +158,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
142158
validate:
143159
valid, _, err := c.ValidateToken(ctx, token)
144160
if err != nil {
145-
return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err)
161+
return externalAuthLink, "token failed to validate", xerrors.Errorf("validate external auth token: %w", err)
146162
}
147163
if !valid {
148164
// A customer using GitHub in Australia reported that validating immediately
@@ -156,7 +172,7 @@ validate:
156172
goto validate
157173
}
158174
// The token is no longer valid!
159-
return externalAuthLink, false, nil
175+
return externalAuthLink, "token failed to validate", nil
160176
}
161177

162178
if token.AccessToken != externalAuthLink.OAuthAccessToken {
@@ -172,11 +188,11 @@ validate:
172188
OAuthExtra: extra,
173189
})
174190
if err != nil {
175-
return updatedAuthLink, false, xerrors.Errorf("update external auth link: %w", err)
191+
return updatedAuthLink, "failed to update token", xerrors.Errorf("update external auth link: %w", err)
176192
}
177193
externalAuthLink = updatedAuthLink
178194
}
179-
return externalAuthLink, true, nil
195+
return externalAuthLink, validReason, nil
180196
}
181197

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

‎coderd/externalauth/externalauth_test.go

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

62-
_, refreshed, err := config.RefreshToken(ctx, nil, link)
62+
_, invalidReason, err := config.RefreshToken(ctx, nil, link)
6363
require.NoError(t, err)
64-
require.False(t, refreshed)
64+
require.False(t, invalidReason.Valid())
6565
})
6666

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

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

@@ -105,11 +105,11 @@ func TestRefreshToken(t *testing.T) {
105105
},
106106
},
107107
}
108-
_, refreshed, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
108+
_, invalidReason, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
109109
OAuthExpiry: expired,
110110
})
111-
require.Error(t, err)
112-
require.False(t, refreshed)
111+
require.NoError(t, err)
112+
require.False(t, invalidReason.Valid())
113113
})
114114

115115
t.Run("ValidateServerError", func(t *testing.T) {
@@ -156,9 +156,9 @@ func TestRefreshToken(t *testing.T) {
156156
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
157157
link.OAuthExpiry = expired
158158

159-
_, refreshed, err := config.RefreshToken(ctx, nil, link)
159+
_, invalidReason, err := config.RefreshToken(ctx, nil, link)
160160
require.NoError(t, err, staticError)
161-
require.False(t, refreshed)
161+
require.False(t, invalidReason.Valid())
162162
require.True(t, validated, "token should have been attempted to be validated")
163163
})
164164

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

194-
_, ok, err := config.RefreshToken(ctx, nil, link)
194+
_, invalidReason, err := config.RefreshToken(ctx, nil, link)
195195
require.NoError(t, err)
196-
require.True(t, ok)
196+
require.True(t, invalidReason.Valid())
197197
require.Equal(t, 2, validateCalls, "token should have been attempted to be validated more than once")
198198
})
199199

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

220220
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
221221

222-
_, ok, err := config.RefreshToken(ctx, nil, link)
222+
_, invalidReason, err := config.RefreshToken(ctx, nil, link)
223223
require.NoError(t, err)
224-
require.True(t, ok)
224+
require.True(t, invalidReason.Valid())
225225
require.Equal(t, 1, validateCalls, "token is validated")
226226
})
227227

@@ -253,9 +253,9 @@ func TestRefreshToken(t *testing.T) {
253253
// Force a refresh
254254
link.OAuthExpiry = expired
255255

256-
updated, ok, err := config.RefreshToken(ctx, db, link)
256+
updated, invalidReason, err := config.RefreshToken(ctx, db, link)
257257
require.NoError(t, err)
258-
require.True(t, ok)
258+
require.True(t, invalidReason.Valid())
259259
require.Equal(t, 1, validateCalls, "token is validated")
260260
require.Equal(t, 1, refreshCalls, "token is refreshed")
261261
require.NotEqualf(t, link.OAuthAccessToken, updated.OAuthAccessToken, "token is updated")
@@ -292,9 +292,9 @@ func TestRefreshToken(t *testing.T) {
292292
// Force a refresh
293293
link.OAuthExpiry = expired
294294

295-
updated, ok, err := config.RefreshToken(ctx, db, link)
295+
updated, invalidReason, err := config.RefreshToken(ctx, db, link)
296296
require.NoError(t, err)
297-
require.True(t, ok)
297+
require.True(t, invalidReason.Valid())
298298
require.True(t, updated.OAuthExtra.Valid)
299299
extra := map[string]interface{}{}
300300
require.NoError(t, json.Unmarshal(updated.OAuthExtra.RawMessage, &extra))

‎coderd/provisionerdserver/provisionerdserver.go

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

562-
link, valid, err := config.RefreshToken(ctx, s.Database, link)
562+
link, invalidReason, err := config.RefreshToken(ctx, s.Database, link)
563563
if err != nil {
564564
return nil, failJob(fmt.Sprintf("refresh external auth link %q: %s", p.ID, err))
565565
}
566-
if !valid {
566+
if invalidReason.Invalid() {
567567
continue
568568
}
569569
externalAuthProviders = append(externalAuthProviders, &sdkproto.ExternalAuthProvider{

‎coderd/templateversions.go

Lines changed: 3 additions & 8 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)
356+
_, invalidReason, err := config.RefreshToken(ctx, api.Database, authLink)
357357
if err != nil {
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 = invalidReason.Valid()
371366
providers = append(providers, provider)
372367
}
373368

‎coderd/workspaceagents.go

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

1915-
externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
1915+
externalAuthLink, invalidReason, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
19161916
if err != nil {
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 invalidReason.Invalid() {
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.

0 commit comments

Comments
 (0)