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

Commit24ba819

Browse files
authored
chore: return failed refresh errors on external auth as string (was boolean) (#13402)
* chore: return failed refresh errors on external authFailed refreshes should return errors. These errors are capturedas validate errors.
1 parentbf98b0d commit24ba819

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
iflink.OAuthAccessToken!="" {
352352
cfg,ok:=configs[link.ProviderID]
353353
ifok {
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
iferr!=nil {
359359
meta.ValidateError=err.Error()
360360
}
361+
linkMeta[link.ProviderID]=meta
362+
361363
// Update the link if it was potentially refreshed.
362-
iferr==nil&&valid{
364+
iferr==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+
typeInvalidTokenErrorstring
101+
102+
func (eInvalidTokenError)Error()string {
103+
returnstring(e)
104+
}
105+
106+
funcIsInvalidTokenError(errerror)bool {
107+
varinvalidTokenErrorInvalidTokenError
108+
returnxerrors.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
ifc.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-
returnexternalAuthLink,false,nil
122+
returnexternalAuthLink,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
iferr!=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-
returnexternalAuthLink,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+
returnexternalAuthLink,InvalidTokenError(fmt.Sprintf("refresh token: %s",err.Error()))
129145
}
130146

131147
extra,err:=c.GenerateTokenExtra(token)
132148
iferr!=nil {
133-
returnexternalAuthLink,false,xerrors.Errorf("generate token extra: %w",err)
149+
returnexternalAuthLink,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
iferr!=nil {
143-
returnexternalAuthLink,false,xerrors.Errorf("validate external auth token: %w",err)
159+
returnexternalAuthLink,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-
returnexternalAuthLink,false,nil
173+
returnexternalAuthLink,InvalidTokenError("token failed to validate")
158174
}
159175

160176
iftoken.AccessToken!=externalAuthLink.OAuthAccessToken {
@@ -170,11 +186,11 @@ validate:
170186
OAuthExtra:extra,
171187
})
172188
iferr!=nil {
173-
returnupdatedAuthLink,false,xerrors.Errorf("update external auth link: %w",err)
189+
returnupdatedAuthLink,xerrors.Errorf("update external auth link: %w",err)
174190
}
175191
externalAuthLink=updatedAuthLink
176192
}
177-
returnexternalAuthLink,true,nil
193+
returnexternalAuthLink,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-
iferr!=nil {
562+
refreshed,err:=config.RefreshToken(ctx,s.Database,link)
563+
iferr!=nil&&!externalauth.IsInvalidTokenError(err){
564564
returnnil,failJob(fmt.Sprintf("refresh external auth link %q: %s",p.ID,err))
565565
}
566-
if!valid {
566+
iferr!=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-
iferr!=nil {
356+
_,err=config.RefreshToken(ctx,api.Database,authLink)
357+
iferr!=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-
iferr!=nil {
1915+
refreshedLink,err:=externalAuthConfig.RefreshToken(ctx,api.Database,externalAuthLink)
1916+
iferr!=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+
iferr!=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
iferr!=nil {
19351935
handleRetrying(http.StatusInternalServerError, codersdk.Response{
19361936
Message:"Failed to create external auth response.",

0 commit comments

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