Skip to content

Commit 1dab79f

Browse files
EmyrkEmyrk
committed
Switch to a sentinel error
1 parent 6b21da8 commit 1dab79f

File tree

6 files changed

+56
-56
lines changed

6 files changed

+56
-56
lines changed

‎coderd/externalauth.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -351,16 +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, invalidReason, err := cfg.RefreshToken(ctx, api.Database, link)
354+
newLink, err := cfg.RefreshToken(ctx, api.Database, link)
355355
meta := db2sdk.ExternalAuthMeta{
356-
Authenticated: invalidReason.Valid(),
357-
ValidateError: invalidReason.String(),
356+
Authenticated: err == nil,
358357
}
359358
if err != nil {
360359
meta.ValidateError = err.Error()
361360
}
361+
linkMeta[link.ProviderID] = meta
362+
362363
// Update the link if it was potentially refreshed.
363-
if err == nil && invalidReason.Valid() {
364+
if err == nil {
364365
links[i] = newLink
365366
}
366367
}

‎coderd/externalauth/externalauth.go

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -95,33 +95,34 @@ 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
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
102101

103-
// Valid returns true if there is no reason to be invalid.
104-
func (r InvalidReason) Valid() bool {
105-
return r == ""
102+
func (e InvalidTokenError) Error() string {
103+
return string(e)
106104
}
107105

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) }
106+
func IsInvalidTokenError(err error) bool {
107+
var invalidTokenError InvalidTokenError
108+
if xerrors.As(err, &invalidTokenError) {
109+
return true
110+
}
111+
return false
112+
}
111113

112114
// RefreshToken automatically refreshes the token if expired and permitted.
113-
// It returns the token and a bool indicating if the token is valid.
114-
func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, InvalidReason, error) {
115-
const validReason InvalidReason = ""
116-
115+
// If an error is returned, the token is either invalid, or an error occurred.
116+
// Use 'IsInvalidTokenError(err)' to determine the difference.
117+
func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, error) {
117118
// If the token is expired and refresh is disabled, we prompt
118119
// the user to authenticate again.
119120
if c.NoRefresh &&
120121
// If the time is set to 0, then it should never expire.
121122
// This is true for github, which has no expiry.
122123
!externalAuthLink.OAuthExpiry.IsZero() &&
123124
externalAuthLink.OAuthExpiry.Before(dbtime.Now()) {
124-
return externalAuthLink, "token expired, refreshing is disabled", nil
125+
return externalAuthLink, InvalidTokenError("token expired, refreshing is disabled")
125126
}
126127

127128
// This is additional defensive programming. Because TokenSource is an interface,
@@ -143,12 +144,12 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
143144
// TokenSource(...).Token() will always return the current token if the token is not expired.
144145
// If it is expired, it will attempt to refresh the token, and if it cannot, it will fail with
145146
// an error. This error is a reason the token is invalid.
146-
return externalAuthLink, InvalidReason(fmt.Sprintf("refresh token: %s", err.Error())), nil
147+
return externalAuthLink, InvalidTokenError(fmt.Sprintf("refresh token: %s", err.Error()))
147148
}
148149

149150
extra, err := c.GenerateTokenExtra(token)
150151
if err != nil {
151-
return externalAuthLink, "generate extra token fields", xerrors.Errorf("generate token extra: %w", err)
152+
return externalAuthLink, xerrors.Errorf("generate token extra: %w", err)
152153
}
153154

154155
r := retry.New(50*time.Millisecond, 200*time.Millisecond)
@@ -158,7 +159,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
158159
validate:
159160
valid, _, err := c.ValidateToken(ctx, token)
160161
if err != nil {
161-
return externalAuthLink, "token failed to validate", xerrors.Errorf("validate external auth token: %w", err)
162+
return externalAuthLink, xerrors.Errorf("validate external auth token: %w", err)
162163
}
163164
if !valid {
164165
// A customer using GitHub in Australia reported that validating immediately
@@ -172,7 +173,7 @@ validate:
172173
goto validate
173174
}
174175
// The token is no longer valid!
175-
return externalAuthLink, "token failed to validate", nil
176+
return externalAuthLink, InvalidTokenError("token failed to validate")
176177
}
177178

178179
if token.AccessToken != externalAuthLink.OAuthAccessToken {
@@ -188,11 +189,11 @@ validate:
188189
OAuthExtra: extra,
189190
})
190191
if err != nil {
191-
return updatedAuthLink, "failed to update token", xerrors.Errorf("update external auth link: %w", err)
192+
return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err)
192193
}
193194
externalAuthLink = updatedAuthLink
194195
}
195-
return externalAuthLink, validReason, nil
196+
return externalAuthLink, nil
196197
}
197198

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

‎coderd/externalauth/externalauth_test.go

Lines changed: 16 additions & 19 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-
_, invalidReason, err := config.RefreshToken(ctx, nil, link)
63-
require.NoError(t, err)
64-
require.False(t, invalidReason.Valid())
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-
_, invalidReason, err := config.RefreshToken(ctx, nil, link)
94+
_, err := config.RefreshToken(ctx, nil, link)
9495
require.NoError(t, err)
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,10 @@ func TestRefreshToken(t *testing.T) {
105105
},
106106
},
107107
}
108-
_, invalidReason, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
108+
_, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
109109
OAuthExpiry: expired,
110110
})
111111
require.NoError(t, err)
112-
require.False(t, invalidReason.Valid())
113112
})
114113

115114
t.Run("ValidateServerError", func(t *testing.T) {
@@ -131,8 +130,9 @@ func TestRefreshToken(t *testing.T) {
131130
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
132131
link.OAuthExpiry = expired
133132

134-
_, _, err := config.RefreshToken(ctx, nil, link)
133+
_, err := config.RefreshToken(ctx, nil, link)
135134
require.ErrorContains(t, err, staticError)
135+
require.True(t, externalauth.IsInvalidTokenError(err))
136136
require.True(t, validated, "token should have been attempted to be validated")
137137
})
138138

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

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

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

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

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

220219
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
221220

222-
_, invalidReason, err := config.RefreshToken(ctx, nil, link)
221+
_, err := config.RefreshToken(ctx, nil, link)
223222
require.NoError(t, err)
224-
require.True(t, invalidReason.Valid())
225223
require.Equal(t, 1, validateCalls, "token is validated")
226224
})
227225

@@ -253,9 +251,8 @@ func TestRefreshToken(t *testing.T) {
253251
// Force a refresh
254252
link.OAuthExpiry = expired
255253

256-
updated, invalidReason, err := config.RefreshToken(ctx, db, link)
254+
updated, err := config.RefreshToken(ctx, db, link)
257255
require.NoError(t, err)
258-
require.True(t, invalidReason.Valid())
259256
require.Equal(t, 1, validateCalls, "token is validated")
260257
require.Equal(t, 1, refreshCalls, "token is refreshed")
261258
require.NotEqualf(t, link.OAuthAccessToken, updated.OAuthAccessToken, "token is updated")
@@ -292,9 +289,9 @@ func TestRefreshToken(t *testing.T) {
292289
// Force a refresh
293290
link.OAuthExpiry = expired
294291

295-
updated, invalidReason, err := config.RefreshToken(ctx, db, link)
292+
updated, err := config.RefreshToken(ctx, db, link)
296293
require.NoError(t, err)
297-
require.True(t, invalidReason.Valid())
294+
298295
require.True(t, updated.OAuthExtra.Valid)
299296
extra := map[string]interface{}{}
300297
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, invalidReason, 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 invalidReason.Invalid() {
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: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,16 +353,16 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ
353353
return
354354
}
355355

356-
_, invalidReason, 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
}
364364

365-
provider.Authenticated = invalidReason.Valid()
365+
provider.Authenticated = err == nil
366366
providers = append(providers, provider)
367367
}
368368

‎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, invalidReason, 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 invalidReason.Invalid() {
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)