Skip to content

Commit 79568bf

Browse files
ammarioammario
committed
Revert "fix: always attempt external auth refresh when fetching (#11762)"
This reverts commit 0befc08.
1 parent 0befc08 commit 79568bf

File tree

6 files changed

+80
-129
lines changed

6 files changed

+80
-129
lines changed

‎coderd/database/modelmethods.go

-9
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
return rbac.ResourceUserData.WithID(u.UserID).WithOwner(u.UserID.String())
270269
}
271270

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

‎coderd/externalauth.go

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

‎coderd/externalauth/externalauth.go

+4-16
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
defer retryCtxCancel()
140140
validate:
141-
valid, _, err := c.ValidateToken(ctx, token)
141+
valid, _, err := c.ValidateToken(ctx, token.AccessToken)
142142
if err != nil {
143143
return externalAuthLink, 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-
if link == nil {
184-
return false, nil, xerrors.New("validate external auth token: token is nil")
185-
}
186-
if !link.Expiry.IsZero() && link.Expiry.Before(dbtime.Now()) {
187-
return false, nil, nil
188-
}
189-
182+
func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *codersdk.ExternalAuthUser, error) {
190183
if c.ValidateURL == "" {
191184
// Default that the token is valid if no validation URL is provided.
192185
return true, nil, nil
@@ -196,7 +189,7 @@ func (c *Config) ValidateToken(ctx context.Context, link *oauth2.Token) (bool, *
196189
return false, 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
if err != nil {
202195
return false, nil, err
@@ -403,15 +396,10 @@ func (c *DeviceAuth) ExchangeDeviceCode(ctx context.Context, deviceCode string)
403396
if body.Error != "" {
404397
return nil, 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-
if body.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

+1-1
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

+72-99
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-
var previousToken *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(code int, response any) {
2042-
if !listen {
2043-
httpapi.Write(ctx, rw, code, response)
2034+
if listen {
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+
defer done()
2039+
var previousToken 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+
if err != nil {
2051+
if errors.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+
if externalAuthLink.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+
if previousToken.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+
if err != 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+
if err != 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
if err != 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
if err != 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
if err != 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
if err != 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-
defer done()
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-
var previousToken database.ExternalAuthLink
2118-
if previous != 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-
if err != nil {
2132-
if errors.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-
if externalAuthLink.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-
if previousToken.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-
if err != 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-
if err != 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

+2-3
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)