Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit78f9f43

Browse files
authored
chore: do not refresh tokens that have already failed refreshing (#15608)
Once a token refresh fails, we remove the `oauth_refresh_token` from thedatabase. This will prevent the token from hitting the IDP forsubsequent refresh attempts.Without this change, a bad script can cause a failing token to hit aremote IDP repeatedly with each `git` operation. With this change, afterthe first hit, subsequent hits will fail locally, and never contact theIDP.The solution in both cases is to authenticate the external auth link. Sothe resolution is the same as before.
1 parentdcbcf67 commit78f9f43

File tree

11 files changed

+274
-16
lines changed

11 files changed

+274
-16
lines changed

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -775,7 +775,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
775775
iff.hookWellKnown!=nil {
776776
err:=f.hookWellKnown(r,&cpy)
777777
iferr!=nil {
778-
http.Error(rw,err.Error(),http.StatusInternalServerError)
778+
httpError(rw,http.StatusInternalServerError,err)
779779
return
780780
}
781781
}
@@ -792,7 +792,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
792792

793793
clientID:=r.URL.Query().Get("client_id")
794794
if!assert.Equal(t,f.clientID,clientID,"unexpected client_id") {
795-
http.Error(rw,"invalid client_id",http.StatusBadRequest)
795+
httpError(rw,http.StatusBadRequest,xerrors.New("invalid client_id"))
796796
return
797797
}
798798

@@ -818,7 +818,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
818818
err:=f.hookValidRedirectURL(redirectURI)
819819
iferr!=nil {
820820
t.Errorf("not authorized redirect_uri by custom hook %q: %s",redirectURI,err.Error())
821-
http.Error(rw,fmt.Sprintf("invalid redirect_uri: %s",err.Error()),httpErrorCode(http.StatusBadRequest,err))
821+
httpError(rw,http.StatusBadRequest,xerrors.Errorf("invalid redirect_uri: %w",err))
822822
return
823823
}
824824

@@ -853,7 +853,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
853853
)...)
854854

855855
iferr!=nil {
856-
http.Error(rw,fmt.Sprintf("invalid token request: %s",err.Error()),httpErrorCode(http.StatusBadRequest,err))
856+
httpError(rw,http.StatusBadRequest,err)
857857
return
858858
}
859859
getEmail:=func(claims jwt.MapClaims)string {
@@ -914,7 +914,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
914914
claims=idTokenClaims
915915
err:=f.hookOnRefresh(getEmail(claims))
916916
iferr!=nil {
917-
http.Error(rw,fmt.Sprintf("refresh hook blocked refresh: %s",err.Error()),httpErrorCode(http.StatusBadRequest,err))
917+
httpError(rw,http.StatusBadRequest,xerrors.Errorf("refresh hook blocked refresh: %w",err))
918918
return
919919
}
920920

@@ -1036,7 +1036,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
10361036

10371037
claims,err:=f.hookUserInfo(email)
10381038
iferr!=nil {
1039-
http.Error(rw,fmt.Sprintf("user info hook returned error: %s",err.Error()),httpErrorCode(http.StatusBadRequest,err))
1039+
httpError(rw,http.StatusBadRequest,xerrors.Errorf("user info hook returned error: %w",err))
10401040
return
10411041
}
10421042
_=json.NewEncoder(rw).Encode(claims)
@@ -1499,13 +1499,33 @@ func slogRequestFields(r *http.Request) []any {
14991499
}
15001500
}
15011501

1502-
funchttpErrorCode(defaultCodeint,errerror)int {
1503-
varstatusErrstatusHookError
1502+
// httpError handles better formatted custom errors.
1503+
funchttpError(rw http.ResponseWriter,defaultCodeint,errerror) {
15041504
status:=defaultCode
1505+
1506+
varstatusErrstatusHookError
15051507
iferrors.As(err,&statusErr) {
15061508
status=statusErr.HTTPStatusCode
15071509
}
1508-
returnstatus
1510+
1511+
varoauthErr*oauth2.RetrieveError
1512+
iferrors.As(err,&oauthErr) {
1513+
ifoauthErr.Response.StatusCode!=0 {
1514+
status=oauthErr.Response.StatusCode
1515+
}
1516+
1517+
rw.Header().Set("Content-Type","application/x-www-form-urlencoded; charset=utf-8")
1518+
form:= url.Values{
1519+
"error": {oauthErr.ErrorCode},
1520+
"error_description": {oauthErr.ErrorDescription},
1521+
"error_uri": {oauthErr.ErrorURI},
1522+
}
1523+
rw.WriteHeader(status)
1524+
_,_=rw.Write([]byte(form.Encode()))
1525+
return
1526+
}
1527+
1528+
http.Error(rw,err.Error(),status)
15091529
}
15101530

15111531
typefakeRoundTripperstruct {

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3319,6 +3319,13 @@ func (q *querier) RegisterWorkspaceProxy(ctx context.Context, arg database.Regis
33193319
returnupdateWithReturn(q.log,q.auth,fetch,q.db.RegisterWorkspaceProxy)(ctx,arg)
33203320
}
33213321

3322+
func (q*querier)RemoveRefreshToken(ctx context.Context,arg database.RemoveRefreshTokenParams)error {
3323+
fetch:=func(ctx context.Context,arg database.RemoveRefreshTokenParams) (database.ExternalAuthLink,error) {
3324+
returnq.db.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{UserID:arg.UserID,ProviderID:arg.ProviderID})
3325+
}
3326+
returnfetchAndExec(q.log,q.auth,policy.ActionUpdatePersonal,fetch,q.db.RemoveRefreshToken)(ctx,arg)
3327+
}
3328+
33223329
func (q*querier)RemoveUserFromAllGroups(ctx context.Context,userID uuid.UUID)error {
33233330
// This is a system function to clear user groups in group sync.
33243331
iferr:=q.authorizeContext(ctx,policy.ActionUpdate,rbac.ResourceSystem);err!=nil {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,6 +1269,14 @@ func (s *MethodTestSuite) TestUser() {
12691269
UserID:u.ID,
12701270
}).Asserts(u,policy.ActionUpdatePersonal)
12711271
}))
1272+
s.Run("RemoveRefreshToken",s.Subtest(func(db database.Store,check*expects) {
1273+
link:=dbgen.ExternalAuthLink(s.T(),db, database.ExternalAuthLink{})
1274+
check.Args(database.RemoveRefreshTokenParams{
1275+
ProviderID:link.ProviderID,
1276+
UserID:link.UserID,
1277+
UpdatedAt:link.UpdatedAt,
1278+
}).Asserts(rbac.ResourceUserObject(link.UserID),policy.ActionUpdatePersonal)
1279+
}))
12721280
s.Run("UpdateExternalAuthLink",s.Subtest(func(db database.Store,check*expects) {
12731281
link:=dbgen.ExternalAuthLink(s.T(),db, database.ExternalAuthLink{})
12741282
check.Args(database.UpdateExternalAuthLinkParams{

‎coderd/database/dbmem/dbmem.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8512,6 +8512,29 @@ func (q *FakeQuerier) RegisterWorkspaceProxy(_ context.Context, arg database.Reg
85128512
return database.WorkspaceProxy{},sql.ErrNoRows
85138513
}
85148514

8515+
func (q*FakeQuerier)RemoveRefreshToken(_ context.Context,arg database.RemoveRefreshTokenParams)error {
8516+
iferr:=validateDatabaseType(arg);err!=nil {
8517+
returnerr
8518+
}
8519+
8520+
q.mutex.Lock()
8521+
deferq.mutex.Unlock()
8522+
forindex,gitAuthLink:=rangeq.externalAuthLinks {
8523+
ifgitAuthLink.ProviderID!=arg.ProviderID {
8524+
continue
8525+
}
8526+
ifgitAuthLink.UserID!=arg.UserID {
8527+
continue
8528+
}
8529+
gitAuthLink.UpdatedAt=arg.UpdatedAt
8530+
gitAuthLink.OAuthRefreshToken=""
8531+
q.externalAuthLinks[index]=gitAuthLink
8532+
8533+
returnnil
8534+
}
8535+
returnsql.ErrNoRows
8536+
}
8537+
85158538
func (q*FakeQuerier)RemoveUserFromAllGroups(_ context.Context,userID uuid.UUID)error {
85168539
q.mutex.Lock()
85178540
deferq.mutex.Unlock()

‎coderd/database/dbmetrics/querymetrics.go

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

‎coderd/database/dbmock/dbmock.go

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

‎coderd/database/querier.go

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

‎coderd/database/queries.sql.go

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

‎coderd/database/queries/externalauth.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,14 @@ UPDATE external_auth_links SET
4242
oauth_expiry= $8,
4343
oauth_extra= $9
4444
WHERE provider_id= $1AND user_id= $2 RETURNING*;
45+
46+
-- name: RemoveRefreshToken :exec
47+
-- Removing the refresh token disables the refresh behavior for a given
48+
-- auth token. If a refresh token is marked invalid, it is better to remove it
49+
-- then continually attempt to refresh the token.
50+
UPDATE
51+
external_auth_links
52+
SET
53+
oauth_refresh_token='',
54+
updated_at= @updated_at
55+
WHERE provider_id= @provider_idAND user_id= @user_id;

‎coderd/externalauth/externalauth.go

Lines changed: 78 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
118118
// This is true for github, which has no expiry.
119119
!externalAuthLink.OAuthExpiry.IsZero()&&
120120
externalAuthLink.OAuthExpiry.Before(dbtime.Now()) {
121-
returnexternalAuthLink,InvalidTokenError("token expired, refreshing is disabled")
121+
returnexternalAuthLink,InvalidTokenError("token expired, refreshing iseitherdisabled or refreshing failed and will not be retried")
122122
}
123123

124124
// This is additional defensive programming. Because TokenSource is an interface,
@@ -130,16 +130,41 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
130130
refreshToken=""
131131
}
132132

133-
token,err:=c.TokenSource(ctx,&oauth2.Token{
133+
existingToken:=&oauth2.Token{
134134
AccessToken:externalAuthLink.OAuthAccessToken,
135135
RefreshToken:refreshToken,
136136
Expiry:externalAuthLink.OAuthExpiry,
137-
}).Token()
137+
}
138+
139+
token,err:=c.TokenSource(ctx,existingToken).Token()
138140
iferr!=nil {
139-
// Even if the token fails to be obtained, do not return the error as an error.
141+
// TokenSource can fail for numerous reasons. If it fails because of
142+
// a bad refresh token, then the refresh token is invalid, and we should
143+
// get rid of it. Keeping it around will cause additional refresh
144+
// attempts that will fail and cost us api rate limits.
145+
ifisFailedRefresh(existingToken,err) {
146+
dbExecErr:=db.RemoveRefreshToken(ctx, database.RemoveRefreshTokenParams{
147+
UpdatedAt:dbtime.Now(),
148+
ProviderID:externalAuthLink.ProviderID,
149+
UserID:externalAuthLink.UserID,
150+
})
151+
ifdbExecErr!=nil {
152+
// This error should be rare.
153+
returnexternalAuthLink,InvalidTokenError(fmt.Sprintf("refresh token failed: %q, then removing refresh token failed: %q",err.Error(),dbExecErr.Error()))
154+
}
155+
// The refresh token was cleared
156+
externalAuthLink.OAuthRefreshToken=""
157+
}
158+
159+
// Unfortunately have to match exactly on the error message string.
160+
// Improve the error message to account refresh tokens are deleted if
161+
// invalid on our end.
162+
iferr.Error()=="oauth2: token expired and refresh token is not set" {
163+
returnexternalAuthLink,InvalidTokenError("token expired, refreshing is either disabled or refreshing failed and will not be retried")
164+
}
165+
140166
// TokenSource(...).Token() will always return the current token if the token is not expired.
141-
// If it is expired, it will attempt to refresh the token, and if it cannot, it will fail with
142-
// an error. This error is a reason the token is invalid.
167+
// So this error is only returned if a refresh of the token failed.
143168
returnexternalAuthLink,InvalidTokenError(fmt.Sprintf("refresh token: %s",err.Error()))
144169
}
145170

@@ -973,3 +998,50 @@ func IsGithubDotComURL(str string) bool {
973998
}
974999
returnghURL.Host=="github.com"
9751000
}
1001+
1002+
// isFailedRefresh returns true if the error returned by the TokenSource.Token()
1003+
// is due to a failed refresh. The failure being the refresh token itself.
1004+
// If this returns true, no amount of retries will fix the issue.
1005+
//
1006+
// Notes: Provider responses are not uniform. Here are some examples:
1007+
// Github
1008+
// - Returns a 200 with Code "bad_refresh_token" and Description "The refresh token passed is incorrect or expired."
1009+
//
1010+
// Gitea [TODO: get an expired refresh token]
1011+
// - [Bad JWT] Returns 400 with Code "unauthorized_client" and Description "unable to parse refresh token"
1012+
//
1013+
// Gitlab
1014+
// - Returns 400 with Code "invalid_grant" and Description "The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client."
1015+
funcisFailedRefresh(existingToken*oauth2.Token,errerror)bool {
1016+
ifexistingToken.RefreshToken=="" {
1017+
returnfalse// No refresh token, so this cannot be refreshed
1018+
}
1019+
1020+
ifexistingToken.Valid() {
1021+
returnfalse// Valid tokens are not refreshed
1022+
}
1023+
1024+
varoauthErr*oauth2.RetrieveError
1025+
ifxerrors.As(err,&oauthErr) {
1026+
switchoauthErr.ErrorCode {
1027+
// Known error codes that indicate a failed refresh.
1028+
// 'Spec' means the code is defined in the spec.
1029+
case"bad_refresh_token",// Github
1030+
"invalid_grant",// Gitlab & Spec
1031+
"unauthorized_client",// Gitea & Spec
1032+
"unsupported_grant_type":// Spec, refresh not supported
1033+
returntrue
1034+
}
1035+
1036+
switchoauthErr.Response.StatusCode {
1037+
casehttp.StatusBadRequest,http.StatusUnauthorized,http.StatusForbidden,http.StatusOK:
1038+
// Status codes that indicate the request was processed, and rejected.
1039+
returntrue
1040+
casehttp.StatusInternalServerError,http.StatusTooManyRequests:
1041+
// These do not indicate a failed refresh, but could be a temporary issue.
1042+
returnfalse
1043+
}
1044+
}
1045+
1046+
returnfalse
1047+
}

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp