coder · Emyrk · Jun 3, 2024 · May 29, 2024 · May 29, 2024 · May 29, 2024
diff --git a/coderd/externalauth.go b/coderd/externalauth.go
@@ -351,15 +351,17 @@ func (api *API) listUserExternalAuths(rw http.ResponseWriter, r *http.Request) {
 		if link.OAuthAccessToken != "" {
 			cfg, ok := configs[link.ProviderID]
 			if ok {
-				newLink, valid, err := cfg.RefreshToken(ctx, api.Database, link)
+				newLink, err := cfg.RefreshToken(ctx, api.Database, link)
 				meta := db2sdk.ExternalAuthMeta{
-					Authenticated: valid,
+					Authenticated: err == nil,
 				}
 				if err != nil {
 					meta.ValidateError = err.Error()
 				}
+				linkMeta[link.ProviderID] = meta
+
 				// Update the link if it was potentially refreshed.
-				if err == nil && valid {
+				if err == nil {
 					links[i] = newLink
 				}
 			}

diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
@@ -95,17 +95,31 @@ func (c *Config) GenerateTokenExtra(token *oauth2.Token) (pqtype.NullRawMessage,
 	}, nil
 }
 
+// InvalidTokenError is a case where the "RefreshToken" failed to complete
+// as a result of invalid credentials. Error contains the reason of the failure.
+type InvalidTokenError string
+
+func (e InvalidTokenError) Error() string {
+	return string(e)
+}
+
+func IsInvalidTokenError(err error) bool {
+	var invalidTokenError InvalidTokenError
+	return xerrors.As(err, &invalidTokenError)
+}
+
 // RefreshToken automatically refreshes the token if expired and permitted.
-// It returns the token and a bool indicating if the token is valid.
-func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, bool, error) {
+// If an error is returned, the token is either invalid, or an error occurred.
+// Use 'IsInvalidTokenError(err)' to determine the difference.
+func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, error) {
 	// If the token is expired and refresh is disabled, we prompt
 	// the user to authenticate again.
 	if c.NoRefresh &&
 		// If the time is set to 0, then it should never expire.
 		// This is true for github, which has no expiry.
 		!externalAuthLink.OAuthExpiry.IsZero() &&
 		externalAuthLink.OAuthExpiry.Before(dbtime.Now()) {
-		return externalAuthLink, false, nil
+		return externalAuthLink, InvalidTokenError("token expired, refreshing is disabled")
 	}
 
 	// 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
 		Expiry:       externalAuthLink.OAuthExpiry,
 	}).Token()
 	if err != nil {
-		// Even if the token fails to be obtained, we still return false because
-		// we aren't trying to surface an error, we're just trying to obtain a valid token.
-		return externalAuthLink, false, nil
+		// Even if the token fails to be obtained, do not return the error as an error.
+		// TokenSource(...).Token() will always return the current token if the token is not expired.
+		// If it is expired, it will attempt to refresh the token, and if it cannot, it will fail with
+		// an error. This error is a reason the token is invalid.
+		return externalAuthLink, InvalidTokenError(fmt.Sprintf("refresh token: %s", err.Error()))
 	}
 
 	extra, err := c.GenerateTokenExtra(token)
 	if err != nil {
-		return externalAuthLink, false, xerrors.Errorf("generate token extra: %w", err)
+		return externalAuthLink, xerrors.Errorf("generate token extra: %w", err)
 	}
 
 	r := retry.New(50*time.Millisecond, 200*time.Millisecond)
@@ -140,7 +156,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 validate:
 	valid, _, err := c.ValidateToken(ctx, token)
 	if err != nil {
-		return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err)
+		return externalAuthLink, xerrors.Errorf("validate external auth token: %w", err)
 	}
 	if !valid {
 		// A customer using GitHub in Australia reported that validating immediately
@@ -154,7 +170,7 @@ validate:
 			goto validate
 		}
 		// The token is no longer valid!
-		return externalAuthLink, false, nil
+		return externalAuthLink, InvalidTokenError("token failed to validate")
 	}
 
 	if token.AccessToken != externalAuthLink.OAuthAccessToken {
@@ -170,11 +186,11 @@ validate:
 			OAuthExtra:             extra,
 		})
 		if err != nil {
-			return updatedAuthLink, false, xerrors.Errorf("update external auth link: %w", err)
+			return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err)
 		}
 		externalAuthLink = updatedAuthLink
 	}
-	return externalAuthLink, true, nil
+	return externalAuthLink, nil
 }
 
 // ValidateToken ensures the Git token provided is valid!

diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go
@@ -59,9 +59,10 @@ func TestRefreshToken(t *testing.T) {
 		// Expire the link
 		link.OAuthExpiry = expired
 
-		_, refreshed, err := config.RefreshToken(ctx, nil, link)
-		require.NoError(t, err)
-		require.False(t, refreshed)
+		_, err := config.RefreshToken(ctx, nil, link)
+		require.Error(t, err)
+		require.True(t, externalauth.IsInvalidTokenError(err))
+		require.Contains(t, err.Error(), "refreshing is disabled")
 	})
 
 	// NoRefreshNoExpiry tests that an oauth token without an expiry is always valid.
@@ -90,9 +91,8 @@ func TestRefreshToken(t *testing.T) {
 
 		// Zero time used
 		link.OAuthExpiry = time.Time{}
-		_, refreshed, err := config.RefreshToken(ctx, nil, link)
+		_, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, refreshed, "token without expiry is always valid")
 		require.True(t, validated, "token should have been validated")
 	})
 
@@ -105,11 +105,12 @@ func TestRefreshToken(t *testing.T) {
 				},
 			},
 		}
-		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
+		_, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
 			OAuthExpiry: expired,
 		})
-		require.NoError(t, err)
-		require.False(t, refreshed)
+		require.Error(t, err)
+		require.True(t, externalauth.IsInvalidTokenError(err))
+		require.Contains(t, err.Error(), "failure")
 	})
 
 	t.Run("ValidateServerError", func(t *testing.T) {
@@ -131,8 +132,12 @@ func TestRefreshToken(t *testing.T) {
 		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 		link.OAuthExpiry = expired
 
-		_, _, err := config.RefreshToken(ctx, nil, link)
+		_, err := config.RefreshToken(ctx, nil, link)
 		require.ErrorContains(t, err, staticError)
+		// Unsure if this should be the correct behavior. It's an invalid token because
+		// 'ValidateToken()' failed with a runtime error. This was the previous behavior,
+		// so not going to change it.
+		require.False(t, externalauth.IsInvalidTokenError(err))
 		require.True(t, validated, "token should have been attempted to be validated")
 	})
 
@@ -156,9 +161,9 @@ func TestRefreshToken(t *testing.T) {
 		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 		link.OAuthExpiry = expired
 
-		_, refreshed, err := config.RefreshToken(ctx, nil, link)
-		require.NoError(t, err, staticError)
-		require.False(t, refreshed)
+		_, err := config.RefreshToken(ctx, nil, link)
+		require.ErrorContains(t, err, "token failed to validate")
+		require.True(t, externalauth.IsInvalidTokenError(err))
 		require.True(t, validated, "token should have been attempted to be validated")
 	})
 
@@ -191,9 +196,8 @@ func TestRefreshToken(t *testing.T) {
 		// Unlimited lifetime, this is what GitHub returns tokens as
 		link.OAuthExpiry = time.Time{}
 
-		_, ok, err := config.RefreshToken(ctx, nil, link)
+		_, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, ok)
 		require.Equal(t, 2, validateCalls, "token should have been attempted to be validated more than once")
 	})
 
@@ -219,9 +223,8 @@ func TestRefreshToken(t *testing.T) {
 
 		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 
-		_, ok, err := config.RefreshToken(ctx, nil, link)
+		_, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, ok)
 		require.Equal(t, 1, validateCalls, "token is validated")
 	})
 
@@ -253,9 +256,8 @@ func TestRefreshToken(t *testing.T) {
 		// Force a refresh
 		link.OAuthExpiry = expired
 
-		updated, ok, err := config.RefreshToken(ctx, db, link)
+		updated, err := config.RefreshToken(ctx, db, link)
 		require.NoError(t, err)
-		require.True(t, ok)
 		require.Equal(t, 1, validateCalls, "token is validated")
 		require.Equal(t, 1, refreshCalls, "token is refreshed")
 		require.NotEqualf(t, link.OAuthAccessToken, updated.OAuthAccessToken, "token is updated")
@@ -292,9 +294,9 @@ func TestRefreshToken(t *testing.T) {
 		// Force a refresh
 		link.OAuthExpiry = expired
 
-		updated, ok, err := config.RefreshToken(ctx, db, link)
+		updated, err := config.RefreshToken(ctx, db, link)
 		require.NoError(t, err)
-		require.True(t, ok)
+
 		require.True(t, updated.OAuthExtra.Valid)
 		extra := map[string]interface{}{}
 		require.NoError(t, json.Unmarshal(updated.OAuthExtra.RawMessage, &extra))

diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go
@@ -559,16 +559,17 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo
 				continue
 			}
 
-			link, valid, err := config.RefreshToken(ctx, s.Database, link)
-			if err != nil {
+			refreshed, err := config.RefreshToken(ctx, s.Database, link)
+			if err != nil && !externalauth.IsInvalidTokenError(err) {
 				return nil, failJob(fmt.Sprintf("refresh external auth link %q: %s", p.ID, err))
 			}
-			if !valid {
+			if err != nil {
+				// Invalid tokens are skipped
 				continue
 			}
 			externalAuthProviders = append(externalAuthProviders, &sdkproto.ExternalAuthProvider{
 				Id:          p.ID,
-				AccessToken: link.OAuthAccessToken,
+				AccessToken: refreshed.OAuthAccessToken,
 			})
 		}
 

diff --git a/coderd/templateversions.go b/coderd/templateversions.go
@@ -353,21 +353,16 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ
 			return
 		}
 
-		_, updated, err := config.RefreshToken(ctx, api.Database, authLink)
-		if err != nil {
+		_, err = config.RefreshToken(ctx, api.Database, authLink)
+		if err != nil && !externalauth.IsInvalidTokenError(err) {
 			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 				Message: "Failed to refresh external auth token.",
 				Detail:  err.Error(),
 			})
 			return
 		}
-		// If the token couldn't be validated, then we assume the user isn't
-		// authenticated and return early.
-		if !updated {
-			providers = append(providers, provider)
-			continue
-		}
-		provider.Authenticated = true
+
+		provider.Authenticated = err == nil
 		providers = append(providers, provider)
 	}
 

diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go
@@ -1912,25 +1912,25 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
-	if err != nil {
+	refreshedLink, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
+	if err != nil && !externalauth.IsInvalidTokenError(err) {
 		handleRetrying(http.StatusInternalServerError, codersdk.Response{
 			Message: "Failed to refresh external auth token.",
 			Detail:  err.Error(),
 		})
 		return
 	}
-	if !valid {
+	if err != nil {
 		// Set the previous token so the retry logic will skip validating the
 		// same token again. This should only be set if the token is invalid and there
 		// was no error. If it is invalid because of an error, then we should recheck.
-		previousToken = &externalAuthLink
+		previousToken = &refreshedLink
 		handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
 			URL: redirectURL.String(),
 		})
 		return
 	}
-	resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra)
+	resp, err := createExternalAuthResponse(externalAuthConfig.Type, refreshedLink.OAuthAccessToken, refreshedLink.OAuthExtra)
 	if err != nil {
 		handleRetrying(http.StatusInternalServerError, codersdk.Response{
 			Message: "Failed to create external auth response.",