Skip to content

Commit c02a27a

Browse files
aslilacaslilac
committed
Merge branch 'main' into lilac/vite-7
2 parents c31a088 + 167522e commit c02a27a

File tree

23 files changed

+999
-813
lines changed

23 files changed

+999
-813
lines changed

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 276 additions & 376 deletions
Large diffs are not rendered by default.

‎coderd/database/dump.sql

Lines changed: 4 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000358_failed_ext_auth_error.down.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
ALTER TABLE external_auth_links
2+
DROP COLUMN oauth_refresh_failure_reason
3+
;

‎coderd/database/migrations/000358_failed_ext_auth_error.up.sql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
ALTER TABLE external_auth_links
2+
ADD COLUMN oauth_refresh_failure_reason TEXT NOT NULL DEFAULT ''
3+
;
4+
5+
COMMENT ON COLUMN external_auth_links.oauth_refresh_failure_reason IS
6+
'This error means the refresh token is invalid. Cached so we can avoid calling the external provider again for the same error.'
7+
;

‎coderd/database/models.go

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 28 additions & 15 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/externalauth.sql

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,20 @@ UPDATE external_auth_links SET
4040
oauth_refresh_token = $6,
4141
oauth_refresh_token_key_id = $7,
4242
oauth_expiry = $8,
43-
oauth_extra = $9
43+
oauth_extra = $9,
44+
-- Only 'UpdateExternalAuthLinkRefreshToken' supports updating the oauth_refresh_failure_reason.
45+
-- Any updates to the external auth link, will be assumed to change the state and clear
46+
-- any cached errors.
47+
oauth_refresh_failure_reason = ''
4448
WHERE provider_id = $1 AND user_id = $2 RETURNING *;
4549

4650
-- name: UpdateExternalAuthLinkRefreshToken :exec
4751
UPDATE
4852
external_auth_links
4953
SET
54+
-- oauth_refresh_failure_reason can be set to cache the failure reason
55+
-- for subsequent refresh attempts.
56+
oauth_refresh_failure_reason = @oauth_refresh_failure_reason,
5057
oauth_refresh_token = @oauth_refresh_token,
5158
updated_at = @updated_at
5259
WHERE

‎coderd/externalauth/externalauth.go

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"strings"
1515
"time"
1616

17+
"github.com/dustin/go-humanize"
1718
"golang.org/x/oauth2"
1819
"golang.org/x/xerrors"
1920

@@ -28,6 +29,13 @@ import (
2829
"github.com/coder/retry"
2930
)
3031

32+
const (
33+
// failureReasonLimit is the maximum text length of an error to be cached to the
34+
// database for a failed refresh token. In rare cases, the error could be a large
35+
// HTML payload.
36+
failureReasonLimit = 400
37+
)
38+
3139
// Config is used for authentication for Git operations.
3240
type Config struct {
3341
promoauth.InstrumentedOAuth2Config
@@ -121,11 +129,12 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
121129
return externalAuthLink, InvalidTokenError("token expired, refreshing is either disabled or refreshing failed and will not be retried")
122130
}
123131

132+
refreshToken := externalAuthLink.OAuthRefreshToken
133+
124134
// This is additional defensive programming. Because TokenSource is an interface,
125135
// we cannot be sure that the implementation will treat an 'IsZero' time
126136
// as "not-expired". The default implementation does, but a custom implementation
127137
// might not. Removing the refreshToken will guarantee a refresh will fail.
128-
refreshToken := externalAuthLink.OAuthRefreshToken
129138
if c.NoRefresh {
130139
refreshToken = ""
131140
}
@@ -136,15 +145,30 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
136145
Expiry: externalAuthLink.OAuthExpiry,
137146
}
138147

148+
// Note: The TokenSource(...) method will make no remote HTTP requests if the
149+
// token is expired and no refresh token is set. This is important to prevent
150+
// spamming the API, consuming rate limits, when the token is known to fail.
139151
token, err := c.TokenSource(ctx, existingToken).Token()
140152
if err != nil {
141153
// TokenSource can fail for numerous reasons. If it fails because of
142154
// a bad refresh token, then the refresh token is invalid, and we should
143155
// get rid of it. Keeping it around will cause additional refresh
144156
// attempts that will fail and cost us api rate limits.
157+
//
158+
// The error message is saved for debugging purposes.
145159
if isFailedRefresh(existingToken, err) {
160+
reason := err.Error()
161+
if len(reason) > failureReasonLimit {
162+
// Limit the length of the error message to prevent
163+
// spamming the database with long error messages.
164+
reason = reason[:failureReasonLimit]
165+
}
146166
dbExecErr := db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{
147-
OAuthRefreshToken: "", // It is better to clear the refresh token than to keep retrying.
167+
// Adding a reason will prevent further attempts to try and refresh the token.
168+
OauthRefreshFailureReason: reason,
169+
// Remove the invalid refresh token so it is never used again. The cached
170+
// `reason` can be used to know why this field was zeroed out.
171+
OAuthRefreshToken: "",
148172
OAuthRefreshTokenKeyID: externalAuthLink.OAuthRefreshTokenKeyID.String,
149173
UpdatedAt: dbtime.Now(),
150174
ProviderID: externalAuthLink.ProviderID,
@@ -156,12 +180,28 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
156180
}
157181
// The refresh token was cleared
158182
externalAuthLink.OAuthRefreshToken = ""
183+
externalAuthLink.UpdatedAt = dbtime.Now()
159184
}
160185

161186
// Unfortunately have to match exactly on the error message string.
162187
// Improve the error message to account refresh tokens are deleted if
163188
// invalid on our end.
189+
//
190+
// This error messages comes from the oauth2 package on our client side.
191+
// So this check is not against a server generated error message.
192+
// Error source: https://github.com/golang/oauth2/blob/master/oauth2.go#L277
164193
if err.Error() == "oauth2: token expired and refresh token is not set" {
194+
if externalAuthLink.OauthRefreshFailureReason != "" {
195+
// A cached refresh failure error exists. So the refresh token was set, but was invalid, and zeroed out.
196+
// Return this cached error for the original refresh attempt.
197+
return externalAuthLink, InvalidTokenError(fmt.Sprintf("token expired and refreshing failed %s with: %s",
198+
// Do not return the exact time, because then we have to know what timezone the
199+
// user is in. This approximate time is good enough.
200+
humanize.Time(externalAuthLink.UpdatedAt),
201+
externalAuthLink.OauthRefreshFailureReason,
202+
))
203+
}
204+
165205
return externalAuthLink, InvalidTokenError("token expired, refreshing is either disabled or refreshing failed and will not be retried")
166206
}
167207

‎coderd/externalauth/externalauth_test.go

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -177,37 +177,44 @@ func TestRefreshToken(t *testing.T) {
177177
link.OAuthExpiry = expired
178178

179179
// Make the failure a server internal error. Not related to the token
180+
// This should be retried since this error is temporary.
180181
refreshErr = &oauth2.RetrieveError{
181182
Response: &http.Response{
182183
StatusCode: http.StatusInternalServerError,
183184
},
184185
ErrorCode: "internal_error",
185186
}
186-
_, err := config.RefreshToken(ctx, mDB, link)
187-
require.Error(t, err)
188-
require.True(t, externalauth.IsInvalidTokenError(err))
189-
require.Equal(t, refreshCount, 1)
187+
totalRefreshes := 0
188+
for i := 0; i < 3; i++ {
189+
// Each loop will hit the temporary error and retry.
190+
_, err := config.RefreshToken(ctx, mDB, link)
191+
require.Error(t, err)
192+
totalRefreshes++
193+
require.True(t, externalauth.IsInvalidTokenError(err))
194+
require.Equal(t, refreshCount, totalRefreshes)
195+
}
190196

191-
// Try again with a bad refresh token error
192-
// Expect DB call to remove the refresh token
197+
// Try again with a bad refresh token error. This will invalidate the
198+
// refresh token, and not retry again. Expect DB call to remove the refresh token
193199
mDB.EXPECT().UpdateExternalAuthLinkRefreshToken(gomock.Any(), gomock.Any()).Return(nil).Times(1)
194200
refreshErr = &oauth2.RetrieveError{ // github error
195201
Response: &http.Response{
196202
StatusCode: http.StatusOK,
197203
},
198204
ErrorCode: "bad_refresh_token",
199205
}
200-
_, err = config.RefreshToken(ctx, mDB, link)
206+
_, err := config.RefreshToken(ctx, mDB, link)
201207
require.Error(t, err)
208+
totalRefreshes++
202209
require.True(t, externalauth.IsInvalidTokenError(err))
203-
require.Equal(t, refreshCount, 2)
210+
require.Equal(t, refreshCount, totalRefreshes)
204211

205212
// When the refresh token is empty, no api calls should be made
206213
link.OAuthRefreshToken = "" // mock'd db, so manually set the token to ''
207214
_, err = config.RefreshToken(ctx, mDB, link)
208215
require.Error(t, err)
209216
require.True(t, externalauth.IsInvalidTokenError(err))
210-
require.Equal(t, refreshCount, 2)
217+
require.Equal(t, refreshCount, totalRefreshes)
211218
})
212219

213220
// ValidateFailure tests if the token is no longer valid with a 401 response.

0 commit comments

Comments
 (0)