Skip to content

Commit d66e6e7

Browse files
EmyrkEmyrk
authored
fix: always attempt external auth refresh when fetching (#11762) (#11830)
* fix: always attempt external auth refresh when fetching * refactor validate to check expiry when considering "valid"
1 parent eeef56a commit d66e6e7

File tree

6 files changed

+129
-80
lines changed

6 files changed

+129
-80
lines changed

‎coderd/database/modelmethods.go

+9
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"time"
77

88
"golang.org/x/exp/maps"
9+
"golang.org/x/oauth2"
910

1011
"github.com/coder/coder/v2/coderd/database/dbtime"
1112
"github.com/coder/coder/v2/coderd/rbac"
@@ -268,6 +269,14 @@ func (u ExternalAuthLink) RBACObject() rbac.Object {
268269
return rbac.ResourceUserData.WithID(u.UserID).WithOwner(u.UserID.String())
269270
}
270271

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+
271280
func (u UserLink) RBACObject() rbac.Object {
272281
// I assume UserData is ok?
273282
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.OAuthAccessToken)
60+
res.Authenticated, res.User, err = config.ValidateToken(ctx, link.OAuthToken())
6161
return err
6262
})
6363
eg.Go(func() (err error) {

‎coderd/externalauth/externalauth.go

+16-4
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.AccessToken)
141+
valid, _, err := c.ValidateToken(ctx, token)
142142
if err != nil {
143143
return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err)
144144
}
@@ -179,7 +179,14 @@ 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, token string) (bool, *codersdk.ExternalAuthUser, error) {
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+
183190
if c.ValidateURL == "" {
184191
// Default that the token is valid if no validation URL is provided.
185192
return true, nil, nil
@@ -189,7 +196,7 @@ func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *coders
189196
return false, nil, err
190197
}
191198

192-
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
199+
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", link.AccessToken))
193200
res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceValidateToken, req)
194201
if err != nil {
195202
return false, nil, err
@@ -396,10 +403,15 @@ func (c *DeviceAuth) ExchangeDeviceCode(ctx context.Context, deviceCode string)
396403
if body.Error != "" {
397404
return nil, xerrors.New(body.Error)
398405
}
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+
}
399411
return &oauth2.Token{
400412
AccessToken: body.AccessToken,
401413
RefreshToken: body.RefreshToken,
402-
Expiry: dbtime.Now().Add(time.Duration(body.ExpiresIn) * time.Second),
414+
Expiry: expires,
403415
}, nil
404416
}
405417

‎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.AccessToken)
78+
valid, _, err := cfg.ValidateToken(ctx, refreshed)
7979
require.NoError(t, err)
8080
require.True(t, valid)
8181
require.Equal(t, count("ValidateToken"), 1)

‎coderd/workspaceagents.go

+99-72
Original file line numberDiff line numberDiff line change
@@ -2037,78 +2037,26 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
20372037
return
20382038
}
20392039

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

21082056
// This is the URL that will redirect the user with a state token.
21092057
redirectURL, err := api.AccessURL.Parse(fmt.Sprintf("/external-auth/%s", externalAuthConfig.ID))
21102058
if err != nil {
2111-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
2059+
handleRetrying(http.StatusInternalServerError, codersdk.Response{
21122060
Message: "Failed to parse access URL.",
21132061
Detail: err.Error(),
21142062
})
@@ -2121,36 +2069,40 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
21212069
})
21222070
if err != nil {
21232071
if !errors.Is(err, sql.ErrNoRows) {
2124-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
2072+
handleRetrying(http.StatusInternalServerError, codersdk.Response{
21252073
Message: "Failed to get external auth link.",
21262074
Detail: err.Error(),
21272075
})
21282076
return
21292077
}
21302078

2131-
httpapi.Write(ctx, rw, http.StatusOK, agentsdk.ExternalAuthResponse{
2079+
handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
21322080
URL: redirectURL.String(),
21332081
})
21342082
return
21352083
}
21362084

2137-
externalAuthLink, updated, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
2085+
externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
21382086
if err != nil {
2139-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
2087+
handleRetrying(http.StatusInternalServerError, codersdk.Response{
21402088
Message: "Failed to refresh external auth token.",
21412089
Detail: err.Error(),
21422090
})
21432091
return
21442092
}
2145-
if !updated {
2146-
httpapi.Write(ctx, rw, http.StatusOK, agentsdk.ExternalAuthResponse{
2093+
if !valid {
2094+
// Set the previous token so the retry logic will skip validating the
2095+
// same token again. This should only be set if the token is invalid and there
2096+
// was no error. If it is invalid because of an error, then we should recheck.
2097+
previousToken = &externalAuthLink
2098+
handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
21472099
URL: redirectURL.String(),
21482100
})
21492101
return
21502102
}
21512103
resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra)
21522104
if err != nil {
2153-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
2105+
handleRetrying(http.StatusInternalServerError, codersdk.Response{
21542106
Message: "Failed to create external auth response.",
21552107
Detail: err.Error(),
21562108
})
@@ -2159,6 +2111,81 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
21592111
httpapi.Write(ctx, rw, http.StatusOK, resp)
21602112
}
21612113

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

‎coderd/workspaceagents_test.go

+3-2
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.EnhancedExternalAuthProviderGitHub.String()
1580+
cfg.Type = codersdk.EnhancedExternalAuthProviderGitLab.String()
15811581
}),
15821582
},
15831583
})
@@ -1623,7 +1623,8 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) {
16231623
ticks <- time.Now()
16241624
}
16251625
cancel()
1626-
// We expect only 1
1626+
// We expect only 1. One from the initial "Refresh" attempt, and the
1627+
// other should be skipped.
16271628
// In a failed test, you will likely see 9, as the last one
16281629
// gets canceled.
16291630
require.Equal(t, 1, validateCalls, "validate calls duplicated on same token")

0 commit comments

Comments
 (0)