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

Commit79568bf

Browse files
committed
Revert "fix: always attempt external auth refresh when fetching (#11762)"
This reverts commit0befc08.
1 parent0befc08 commit79568bf

File tree

6 files changed

+80
-129
lines changed

6 files changed

+80
-129
lines changed

‎coderd/database/modelmethods.go

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"time"
77

88
"golang.org/x/exp/maps"
9-
"golang.org/x/oauth2"
109

1110
"github.com/coder/coder/v2/coderd/database/dbtime"
1211
"github.com/coder/coder/v2/coderd/rbac"
@@ -269,14 +268,6 @@ func (u ExternalAuthLink) RBACObject() rbac.Object {
269268
returnrbac.ResourceUserData.WithID(u.UserID).WithOwner(u.UserID.String())
270269
}
271270

272-
func (uExternalAuthLink)OAuthToken()*oauth2.Token {
273-
return&oauth2.Token{
274-
AccessToken:u.OAuthAccessToken,
275-
RefreshToken:u.OAuthRefreshToken,
276-
Expiry:u.OAuthExpiry,
277-
}
278-
}
279-
280271
func (uUserLink)RBACObject() rbac.Object {
281272
// I assume UserData is ok?
282273
returnrbac.ResourceUserData.WithOwner(u.UserID.String()).WithID(u.UserID)

‎coderd/externalauth.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func (api *API) externalAuthByID(w http.ResponseWriter, r *http.Request) {
5757
}
5858
vareg errgroup.Group
5959
eg.Go(func() (errerror) {
60-
res.Authenticated,res.User,err=config.ValidateToken(ctx,link.OAuthToken())
60+
res.Authenticated,res.User,err=config.ValidateToken(ctx,link.OAuthAccessToken)
6161
returnerr
6262
})
6363
eg.Go(func() (errerror) {

‎coderd/externalauth/externalauth.go

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
138138
retryCtx,retryCtxCancel:=context.WithTimeout(ctx,time.Second)
139139
deferretryCtxCancel()
140140
validate:
141-
valid,_,err:=c.ValidateToken(ctx,token)
141+
valid,_,err:=c.ValidateToken(ctx,token.AccessToken)
142142
iferr!=nil {
143143
returnexternalAuthLink,false,xerrors.Errorf("validate external auth token: %w",err)
144144
}
@@ -179,14 +179,7 @@ validate:
179179

180180
// ValidateToken ensures the Git token provided is valid!
181181
// The user is optionally returned if the provider supports it.
182-
func (c*Config)ValidateToken(ctx context.Context,link*oauth2.Token) (bool,*codersdk.ExternalAuthUser,error) {
183-
iflink==nil {
184-
returnfalse,nil,xerrors.New("validate external auth token: token is nil")
185-
}
186-
if!link.Expiry.IsZero()&&link.Expiry.Before(dbtime.Now()) {
187-
returnfalse,nil,nil
188-
}
189-
182+
func (c*Config)ValidateToken(ctx context.Context,tokenstring) (bool,*codersdk.ExternalAuthUser,error) {
190183
ifc.ValidateURL=="" {
191184
// Default that the token is valid if no validation URL is provided.
192185
returntrue,nil,nil
@@ -196,7 +189,7 @@ func (c *Config) ValidateToken(ctx context.Context, link *oauth2.Token) (bool, *
196189
returnfalse,nil,err
197190
}
198191

199-
req.Header.Set("Authorization",fmt.Sprintf("Bearer %s",link.AccessToken))
192+
req.Header.Set("Authorization",fmt.Sprintf("Bearer %s",token))
200193
res,err:=c.InstrumentedOAuth2Config.Do(ctx,promoauth.SourceValidateToken,req)
201194
iferr!=nil {
202195
returnfalse,nil,err
@@ -403,15 +396,10 @@ func (c *DeviceAuth) ExchangeDeviceCode(ctx context.Context, deviceCode string)
403396
ifbody.Error!="" {
404397
returnnil,xerrors.New(body.Error)
405398
}
406-
// If expiresIn is 0, then the token never expires.
407-
expires:=dbtime.Now().Add(time.Duration(body.ExpiresIn)*time.Second)
408-
ifbody.ExpiresIn==0 {
409-
expires= time.Time{}
410-
}
411399
return&oauth2.Token{
412400
AccessToken:body.AccessToken,
413401
RefreshToken:body.RefreshToken,
414-
Expiry:expires,
402+
Expiry:dbtime.Now().Add(time.Duration(body.ExpiresIn)*time.Second),
415403
},nil
416404
}
417405

‎coderd/promoauth/oauth2_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func TestInstrument(t *testing.T) {
7575
require.Equal(t,count("TokenSource"),1)
7676

7777
// Try a validate
78-
valid,_,err:=cfg.ValidateToken(ctx,refreshed)
78+
valid,_,err:=cfg.ValidateToken(ctx,refreshed.AccessToken)
7979
require.NoError(t,err)
8080
require.True(t,valid)
8181
require.Equal(t,count("ValidateToken"),1)

‎coderd/workspaceagents.go

Lines changed: 72 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -2031,26 +2031,78 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
20312031
return
20322032
}
20332033

2034-
varpreviousToken*database.ExternalAuthLink
2035-
// handleRetrying will attempt to continually check for a new token
2036-
// if listen is true. This is useful if an error is encountered in the
2037-
// original single flow.
2038-
//
2039-
// By default, if no errors are encountered, then the single flow response
2040-
// is returned.
2041-
handleRetrying:=func(codeint,responseany) {
2042-
if!listen {
2043-
httpapi.Write(ctx,rw,code,response)
2034+
iflisten {
2035+
// Since we're ticking frequently and this sign-in operation is rare,
2036+
// we are OK with polling to avoid the complexity of pubsub.
2037+
ticker,done:=api.NewTicker(time.Second)
2038+
deferdone()
2039+
varpreviousToken database.ExternalAuthLink
2040+
for {
2041+
select {
2042+
case<-ctx.Done():
2043+
return
2044+
case<-ticker:
2045+
}
2046+
externalAuthLink,err:=api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{
2047+
ProviderID:externalAuthConfig.ID,
2048+
UserID:workspace.OwnerID,
2049+
})
2050+
iferr!=nil {
2051+
iferrors.Is(err,sql.ErrNoRows) {
2052+
continue
2053+
}
2054+
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
2055+
Message:"Failed to get external auth link.",
2056+
Detail:err.Error(),
2057+
})
2058+
return
2059+
}
2060+
2061+
// Expiry may be unset if the application doesn't configure tokens
2062+
// to expire.
2063+
// See
2064+
// https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-user-access-token-for-a-github-app.
2065+
ifexternalAuthLink.OAuthExpiry.Before(dbtime.Now())&&!externalAuthLink.OAuthExpiry.IsZero() {
2066+
continue
2067+
}
2068+
2069+
// Only attempt to revalidate an oauth token if it has actually changed.
2070+
// No point in trying to validate the same token over and over again.
2071+
ifpreviousToken.OAuthAccessToken==externalAuthLink.OAuthAccessToken&&
2072+
previousToken.OAuthRefreshToken==externalAuthLink.OAuthRefreshToken&&
2073+
previousToken.OAuthExpiry==externalAuthLink.OAuthExpiry {
2074+
continue
2075+
}
2076+
2077+
valid,_,err:=externalAuthConfig.ValidateToken(ctx,externalAuthLink.OAuthAccessToken)
2078+
iferr!=nil {
2079+
api.Logger.Warn(ctx,"failed to validate external auth token",
2080+
slog.F("workspace_owner_id",workspace.OwnerID.String()),
2081+
slog.F("validate_url",externalAuthConfig.ValidateURL),
2082+
slog.Error(err),
2083+
)
2084+
}
2085+
previousToken=externalAuthLink
2086+
if!valid {
2087+
continue
2088+
}
2089+
resp,err:=createExternalAuthResponse(externalAuthConfig.Type,externalAuthLink.OAuthAccessToken,externalAuthLink.OAuthExtra)
2090+
iferr!=nil {
2091+
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
2092+
Message:"Failed to create external auth response.",
2093+
Detail:err.Error(),
2094+
})
2095+
return
2096+
}
2097+
httpapi.Write(ctx,rw,http.StatusOK,resp)
20442098
return
20452099
}
2046-
2047-
api.workspaceAgentsExternalAuthListen(ctx,rw,previousToken,externalAuthConfig,workspace)
20482100
}
20492101

20502102
// This is the URL that will redirect the user with a state token.
20512103
redirectURL,err:=api.AccessURL.Parse(fmt.Sprintf("/external-auth/%s",externalAuthConfig.ID))
20522104
iferr!=nil {
2053-
handleRetrying(http.StatusInternalServerError, codersdk.Response{
2105+
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
20542106
Message:"Failed to parse access URL.",
20552107
Detail:err.Error(),
20562108
})
@@ -2063,40 +2115,36 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
20632115
})
20642116
iferr!=nil {
20652117
if!errors.Is(err,sql.ErrNoRows) {
2066-
handleRetrying(http.StatusInternalServerError, codersdk.Response{
2118+
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
20672119
Message:"Failed to get external auth link.",
20682120
Detail:err.Error(),
20692121
})
20702122
return
20712123
}
20722124

2073-
handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
2125+
httpapi.Write(ctx,rw,http.StatusOK, agentsdk.ExternalAuthResponse{
20742126
URL:redirectURL.String(),
20752127
})
20762128
return
20772129
}
20782130

2079-
externalAuthLink,valid,err:=externalAuthConfig.RefreshToken(ctx,api.Database,externalAuthLink)
2131+
externalAuthLink,updated,err:=externalAuthConfig.RefreshToken(ctx,api.Database,externalAuthLink)
20802132
iferr!=nil {
2081-
handleRetrying(http.StatusInternalServerError, codersdk.Response{
2133+
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
20822134
Message:"Failed to refresh external auth token.",
20832135
Detail:err.Error(),
20842136
})
20852137
return
20862138
}
2087-
if!valid {
2088-
// Set the previous token so the retry logic will skip validating the
2089-
// same token again. This should only be set if the token is invalid and there
2090-
// was no error. If it is invalid because of an error, then we should recheck.
2091-
previousToken=&externalAuthLink
2092-
handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
2139+
if!updated {
2140+
httpapi.Write(ctx,rw,http.StatusOK, agentsdk.ExternalAuthResponse{
20932141
URL:redirectURL.String(),
20942142
})
20952143
return
20962144
}
20972145
resp,err:=createExternalAuthResponse(externalAuthConfig.Type,externalAuthLink.OAuthAccessToken,externalAuthLink.OAuthExtra)
20982146
iferr!=nil {
2099-
handleRetrying(http.StatusInternalServerError, codersdk.Response{
2147+
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
21002148
Message:"Failed to create external auth response.",
21012149
Detail:err.Error(),
21022150
})
@@ -2105,81 +2153,6 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
21052153
httpapi.Write(ctx,rw,http.StatusOK,resp)
21062154
}
21072155

2108-
func (api*API)workspaceAgentsExternalAuthListen(ctx context.Context,rw http.ResponseWriter,previous*database.ExternalAuthLink,externalAuthConfig*externalauth.Config,workspace database.Workspace) {
2109-
// Since we're ticking frequently and this sign-in operation is rare,
2110-
// we are OK with polling to avoid the complexity of pubsub.
2111-
ticker,done:=api.NewTicker(time.Second)
2112-
deferdone()
2113-
// If we have a previous token that is invalid, we should not check this again.
2114-
// This serves to prevent doing excessive unauthorized requests to the external
2115-
// auth provider. For github, this limit is 60 per hour, so saving a call
2116-
// per invalid token can be significant.
2117-
varpreviousToken database.ExternalAuthLink
2118-
ifprevious!=nil {
2119-
previousToken=*previous
2120-
}
2121-
for {
2122-
select {
2123-
case<-ctx.Done():
2124-
return
2125-
case<-ticker:
2126-
}
2127-
externalAuthLink,err:=api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{
2128-
ProviderID:externalAuthConfig.ID,
2129-
UserID:workspace.OwnerID,
2130-
})
2131-
iferr!=nil {
2132-
iferrors.Is(err,sql.ErrNoRows) {
2133-
continue
2134-
}
2135-
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
2136-
Message:"Failed to get external auth link.",
2137-
Detail:err.Error(),
2138-
})
2139-
return
2140-
}
2141-
2142-
// Expiry may be unset if the application doesn't configure tokens
2143-
// to expire.
2144-
// See
2145-
// https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-user-access-token-for-a-github-app.
2146-
ifexternalAuthLink.OAuthExpiry.Before(dbtime.Now())&&!externalAuthLink.OAuthExpiry.IsZero() {
2147-
continue
2148-
}
2149-
2150-
// Only attempt to revalidate an oauth token if it has actually changed.
2151-
// No point in trying to validate the same token over and over again.
2152-
ifpreviousToken.OAuthAccessToken==externalAuthLink.OAuthAccessToken&&
2153-
previousToken.OAuthRefreshToken==externalAuthLink.OAuthRefreshToken&&
2154-
previousToken.OAuthExpiry==externalAuthLink.OAuthExpiry {
2155-
continue
2156-
}
2157-
2158-
valid,_,err:=externalAuthConfig.ValidateToken(ctx,externalAuthLink.OAuthToken())
2159-
iferr!=nil {
2160-
api.Logger.Warn(ctx,"failed to validate external auth token",
2161-
slog.F("workspace_owner_id",workspace.OwnerID.String()),
2162-
slog.F("validate_url",externalAuthConfig.ValidateURL),
2163-
slog.Error(err),
2164-
)
2165-
}
2166-
previousToken=externalAuthLink
2167-
if!valid {
2168-
continue
2169-
}
2170-
resp,err:=createExternalAuthResponse(externalAuthConfig.Type,externalAuthLink.OAuthAccessToken,externalAuthLink.OAuthExtra)
2171-
iferr!=nil {
2172-
httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{
2173-
Message:"Failed to create external auth response.",
2174-
Detail:err.Error(),
2175-
})
2176-
return
2177-
}
2178-
httpapi.Write(ctx,rw,http.StatusOK,resp)
2179-
return
2180-
}
2181-
}
2182-
21832156
// createExternalAuthResponse creates an ExternalAuthResponse based on the
21842157
// provider type. This is to support legacy `/workspaceagents/me/gitauth`
21852158
// which uses `Username` and `Password`.

‎coderd/workspaceagents_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1577,7 +1577,7 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) {
15771577
},
15781578
ExternalAuthConfigs: []*externalauth.Config{
15791579
fake.ExternalAuthConfig(t,providerID,nil,func(cfg*externalauth.Config) {
1580-
cfg.Type=codersdk.EnhancedExternalAuthProviderGitLab.String()
1580+
cfg.Type=codersdk.EnhancedExternalAuthProviderGitHub.String()
15811581
}),
15821582
},
15831583
})
@@ -1623,8 +1623,7 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) {
16231623
ticks<-time.Now()
16241624
}
16251625
cancel()
1626-
// We expect only 1. One from the initial "Refresh" attempt, and the
1627-
// other should be skipped.
1626+
// We expect only 1
16281627
// In a failed test, you will likely see 9, as the last one
16291628
// gets canceled.
16301629
require.Equal(t,1,validateCalls,"validate calls duplicated on same token")

0 commit comments

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