From f5d5dcd49df23c6338c22fa9148544c2ee47ab09 Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Wed, 29 May 2024 15:53:05 -0500
Subject: [PATCH 1/6] chore: return failed refresh errors on external auth

Failed refreshes should return errors. These errors are captured
as validate errors. A fair classification of the the error
---
 coderd/externalauth/externalauth.go | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
index 85e53f2e91f33..19f96b82b3f3b 100644
--- a/coderd/externalauth/externalauth.go
+++ b/coderd/externalauth/externalauth.go
@@ -123,9 +123,11 @@ 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
+		// TokenSource will always return the current status token if not-expired.
+		// If the token is expired, it will attempt to refresh. An error is returned
+		// if the refresh fails, meaning the existing token is expired and this function
+		// was unable to obtain a valid one.
+		return externalAuthLink, false, err
 	}
 
 	extra, err := c.GenerateTokenExtra(token)

From 0f35152a6b04309bace7ae00c6a4f4592042ec0a Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Wed, 29 May 2024 16:00:35 -0500
Subject: [PATCH 2/6] Fix unit test

---
 coderd/externalauth/externalauth_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go
index 88f3b7a3b59e9..03d6448f724b7 100644
--- a/coderd/externalauth/externalauth_test.go
+++ b/coderd/externalauth/externalauth_test.go
@@ -108,7 +108,7 @@ func TestRefreshToken(t *testing.T) {
 		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
 			OAuthExpiry: expired,
 		})
-		require.NoError(t, err)
+		require.Error(t, err)
 		require.False(t, refreshed)
 	})
 

From 6b21da81c25baa238de6a587537e6c2dab8aa88f Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Wed, 29 May 2024 16:17:58 -0500
Subject: [PATCH 3/6] Make invalid not an error

---
 coderd/externalauth.go                        |  7 ++--
 coderd/externalauth/externalauth.go           | 40 +++++++++++++------
 coderd/externalauth/externalauth_test.go      | 34 ++++++++--------
 .../provisionerdserver/provisionerdserver.go  |  4 +-
 coderd/templateversions.go                    | 11 ++---
 coderd/workspaceagents.go                     |  4 +-
 6 files changed, 56 insertions(+), 44 deletions(-)

diff --git a/coderd/externalauth.go b/coderd/externalauth.go
index a2d017ed43e0e..9bfe338b7bfc8 100644
--- a/coderd/externalauth.go
+++ b/coderd/externalauth.go
@@ -351,15 +351,16 @@ 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, invalidReason, err := cfg.RefreshToken(ctx, api.Database, link)
 				meta := db2sdk.ExternalAuthMeta{
-					Authenticated: valid,
+					Authenticated: invalidReason.Valid(),
+					ValidateError: invalidReason.String(),
 				}
 				if err != nil {
 					meta.ValidateError = err.Error()
 				}
 				// Update the link if it was potentially refreshed.
-				if err == nil && valid {
+				if err == nil && invalidReason.Valid() {
 					links[i] = newLink
 				}
 			}
diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
index 19f96b82b3f3b..8966c005a7d81 100644
--- a/coderd/externalauth/externalauth.go
+++ b/coderd/externalauth/externalauth.go
@@ -95,9 +95,25 @@ func (c *Config) GenerateTokenExtra(token *oauth2.Token) (pqtype.NullRawMessage,
 	}, nil
 }
 
+// InvalidReason is intentionally typed and not an error. An invalid token is not an error,
+// but a state. A boolean true/false omits context and information that could be helpful in debugging.
+// So a string is returned with some additional information.
+type InvalidReason string
+
+// Valid returns true if there is no reason to be invalid.
+func (r InvalidReason) Valid() bool {
+	return r == ""
+}
+
+// Invalid is easier to read than '!r.Valid()'
+func (r InvalidReason) Invalid() bool  { return !r.Valid() }
+func (r InvalidReason) String() string { return string(r) }
+
 // 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) {
+func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, InvalidReason, error) {
+	const validReason InvalidReason = ""
+
 	// If the token is expired and refresh is disabled, we prompt
 	// the user to authenticate again.
 	if c.NoRefresh &&
@@ -105,7 +121,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 		// This is true for github, which has no expiry.
 		!externalAuthLink.OAuthExpiry.IsZero() &&
 		externalAuthLink.OAuthExpiry.Before(dbtime.Now()) {
-		return externalAuthLink, false, nil
+		return externalAuthLink, "token expired, refreshing is disabled", nil
 	}
 
 	// This is additional defensive programming. Because TokenSource is an interface,
@@ -123,16 +139,16 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 		Expiry:       externalAuthLink.OAuthExpiry,
 	}).Token()
 	if err != nil {
-		// TokenSource will always return the current status token if not-expired.
-		// If the token is expired, it will attempt to refresh. An error is returned
-		// if the refresh fails, meaning the existing token is expired and this function
-		// was unable to obtain a valid one.
-		return externalAuthLink, false, err
+		// 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, InvalidReason(fmt.Sprintf("refresh token: %s", err.Error())), nil
 	}
 
 	extra, err := c.GenerateTokenExtra(token)
 	if err != nil {
-		return externalAuthLink, false, xerrors.Errorf("generate token extra: %w", err)
+		return externalAuthLink, "generate extra token fields", xerrors.Errorf("generate token extra: %w", err)
 	}
 
 	r := retry.New(50*time.Millisecond, 200*time.Millisecond)
@@ -142,7 +158,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, "token failed to validate", xerrors.Errorf("validate external auth token: %w", err)
 	}
 	if !valid {
 		// A customer using GitHub in Australia reported that validating immediately
@@ -156,7 +172,7 @@ validate:
 			goto validate
 		}
 		// The token is no longer valid!
-		return externalAuthLink, false, nil
+		return externalAuthLink, "token failed to validate", nil
 	}
 
 	if token.AccessToken != externalAuthLink.OAuthAccessToken {
@@ -172,11 +188,11 @@ validate:
 			OAuthExtra:             extra,
 		})
 		if err != nil {
-			return updatedAuthLink, false, xerrors.Errorf("update external auth link: %w", err)
+			return updatedAuthLink, "failed to update token", xerrors.Errorf("update external auth link: %w", err)
 		}
 		externalAuthLink = updatedAuthLink
 	}
-	return externalAuthLink, true, nil
+	return externalAuthLink, validReason, nil
 }
 
 // ValidateToken ensures the Git token provided is valid!
diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go
index 03d6448f724b7..aeb24dee87e6f 100644
--- a/coderd/externalauth/externalauth_test.go
+++ b/coderd/externalauth/externalauth_test.go
@@ -59,9 +59,9 @@ func TestRefreshToken(t *testing.T) {
 		// Expire the link
 		link.OAuthExpiry = expired
 
-		_, refreshed, err := config.RefreshToken(ctx, nil, link)
+		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.False(t, refreshed)
+		require.False(t, invalidReason.Valid())
 	})
 
 	// NoRefreshNoExpiry tests that an oauth token without an expiry is always valid.
@@ -90,9 +90,9 @@ func TestRefreshToken(t *testing.T) {
 
 		// Zero time used
 		link.OAuthExpiry = time.Time{}
-		_, refreshed, err := config.RefreshToken(ctx, nil, link)
+		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, refreshed, "token without expiry is always valid")
+		require.True(t, invalidReason.Valid(), "token without expiry is always valid")
 		require.True(t, validated, "token should have been validated")
 	})
 
@@ -105,11 +105,11 @@ func TestRefreshToken(t *testing.T) {
 				},
 			},
 		}
-		_, refreshed, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
+		_, invalidReason, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
 			OAuthExpiry: expired,
 		})
-		require.Error(t, err)
-		require.False(t, refreshed)
+		require.NoError(t, err)
+		require.False(t, invalidReason.Valid())
 	})
 
 	t.Run("ValidateServerError", func(t *testing.T) {
@@ -156,9 +156,9 @@ func TestRefreshToken(t *testing.T) {
 		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 		link.OAuthExpiry = expired
 
-		_, refreshed, err := config.RefreshToken(ctx, nil, link)
+		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err, staticError)
-		require.False(t, refreshed)
+		require.False(t, invalidReason.Valid())
 		require.True(t, validated, "token should have been attempted to be validated")
 	})
 
@@ -191,9 +191,9 @@ 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)
+		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, ok)
+		require.True(t, invalidReason.Valid())
 		require.Equal(t, 2, validateCalls, "token should have been attempted to be validated more than once")
 	})
 
@@ -219,9 +219,9 @@ func TestRefreshToken(t *testing.T) {
 
 		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 
-		_, ok, err := config.RefreshToken(ctx, nil, link)
+		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, ok)
+		require.True(t, invalidReason.Valid())
 		require.Equal(t, 1, validateCalls, "token is validated")
 	})
 
@@ -253,9 +253,9 @@ func TestRefreshToken(t *testing.T) {
 		// Force a refresh
 		link.OAuthExpiry = expired
 
-		updated, ok, err := config.RefreshToken(ctx, db, link)
+		updated, invalidReason, err := config.RefreshToken(ctx, db, link)
 		require.NoError(t, err)
-		require.True(t, ok)
+		require.True(t, invalidReason.Valid())
 		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 +292,9 @@ func TestRefreshToken(t *testing.T) {
 		// Force a refresh
 		link.OAuthExpiry = expired
 
-		updated, ok, err := config.RefreshToken(ctx, db, link)
+		updated, invalidReason, err := config.RefreshToken(ctx, db, link)
 		require.NoError(t, err)
-		require.True(t, ok)
+		require.True(t, invalidReason.Valid())
 		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
index 3f5876d644617..0d6d4e015e51c 100644
--- a/coderd/provisionerdserver/provisionerdserver.go
+++ b/coderd/provisionerdserver/provisionerdserver.go
@@ -559,11 +559,11 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo
 				continue
 			}
 
-			link, valid, err := config.RefreshToken(ctx, s.Database, link)
+			link, invalidReason, err := config.RefreshToken(ctx, s.Database, link)
 			if err != nil {
 				return nil, failJob(fmt.Sprintf("refresh external auth link %q: %s", p.ID, err))
 			}
-			if !valid {
+			if invalidReason.Invalid() {
 				continue
 			}
 			externalAuthProviders = append(externalAuthProviders, &sdkproto.ExternalAuthProvider{
diff --git a/coderd/templateversions.go b/coderd/templateversions.go
index 788a01ba353b1..d3e0dedead298 100644
--- a/coderd/templateversions.go
+++ b/coderd/templateversions.go
@@ -353,7 +353,7 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ
 			return
 		}
 
-		_, updated, err := config.RefreshToken(ctx, api.Database, authLink)
+		_, invalidReason, err := config.RefreshToken(ctx, api.Database, authLink)
 		if err != nil {
 			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 				Message: "Failed to refresh external auth token.",
@@ -361,13 +361,8 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ
 			})
 			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 = invalidReason.Valid()
 		providers = append(providers, provider)
 	}
 
diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go
index 1821948572e29..019e27b0b3e1f 100644
--- a/coderd/workspaceagents.go
+++ b/coderd/workspaceagents.go
@@ -1912,7 +1912,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
+	externalAuthLink, invalidReason, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
 	if err != nil {
 		handleRetrying(http.StatusInternalServerError, codersdk.Response{
 			Message: "Failed to refresh external auth token.",
@@ -1920,7 +1920,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
 		})
 		return
 	}
-	if !valid {
+	if invalidReason.Invalid() {
 		// 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.

From 1dab79fc7e83ce5ff7bea7716f2b34b6d0f2f5e0 Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Fri, 31 May 2024 09:55:00 -0500
Subject: [PATCH 4/6] Switch to a sentinel error

---
 coderd/externalauth.go                        |  9 ++--
 coderd/externalauth/externalauth.go           | 43 ++++++++++---------
 coderd/externalauth/externalauth_test.go      | 35 +++++++--------
 .../provisionerdserver/provisionerdserver.go  |  9 ++--
 coderd/templateversions.go                    |  6 +--
 coderd/workspaceagents.go                     | 10 ++---
 6 files changed, 56 insertions(+), 56 deletions(-)

diff --git a/coderd/externalauth.go b/coderd/externalauth.go
index 9bfe338b7bfc8..8f8514fa17442 100644
--- a/coderd/externalauth.go
+++ b/coderd/externalauth.go
@@ -351,16 +351,17 @@ func (api *API) listUserExternalAuths(rw http.ResponseWriter, r *http.Request) {
 		if link.OAuthAccessToken != "" {
 			cfg, ok := configs[link.ProviderID]
 			if ok {
-				newLink, invalidReason, err := cfg.RefreshToken(ctx, api.Database, link)
+				newLink, err := cfg.RefreshToken(ctx, api.Database, link)
 				meta := db2sdk.ExternalAuthMeta{
-					Authenticated: invalidReason.Valid(),
-					ValidateError: invalidReason.String(),
+					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 && invalidReason.Valid() {
+				if err == nil {
 					links[i] = newLink
 				}
 			}
diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
index 8966c005a7d81..5fc118614bc56 100644
--- a/coderd/externalauth/externalauth.go
+++ b/coderd/externalauth/externalauth.go
@@ -95,25 +95,26 @@ func (c *Config) GenerateTokenExtra(token *oauth2.Token) (pqtype.NullRawMessage,
 	}, nil
 }
 
-// InvalidReason is intentionally typed and not an error. An invalid token is not an error,
-// but a state. A boolean true/false omits context and information that could be helpful in debugging.
-// So a string is returned with some additional information.
-type InvalidReason string
+// 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
 
-// Valid returns true if there is no reason to be invalid.
-func (r InvalidReason) Valid() bool {
-	return r == ""
+func (e InvalidTokenError) Error() string {
+	return string(e)
 }
 
-// Invalid is easier to read than '!r.Valid()'
-func (r InvalidReason) Invalid() bool  { return !r.Valid() }
-func (r InvalidReason) String() string { return string(r) }
+func IsInvalidTokenError(err error) bool {
+	var invalidTokenError InvalidTokenError
+	if xerrors.As(err, &invalidTokenError) {
+		return true
+	}
+	return false
+}
 
 // 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, InvalidReason, error) {
-	const validReason InvalidReason = ""
-
+// 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 &&
@@ -121,7 +122,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 		// This is true for github, which has no expiry.
 		!externalAuthLink.OAuthExpiry.IsZero() &&
 		externalAuthLink.OAuthExpiry.Before(dbtime.Now()) {
-		return externalAuthLink, "token expired, refreshing is disabled", nil
+		return externalAuthLink, InvalidTokenError("token expired, refreshing is disabled")
 	}
 
 	// This is additional defensive programming. Because TokenSource is an interface,
@@ -143,12 +144,12 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 		// 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, InvalidReason(fmt.Sprintf("refresh token: %s", err.Error())), nil
+		return externalAuthLink, InvalidTokenError(fmt.Sprintf("refresh token: %s", err.Error()))
 	}
 
 	extra, err := c.GenerateTokenExtra(token)
 	if err != nil {
-		return externalAuthLink, "generate extra token fields", 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)
@@ -158,7 +159,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 validate:
 	valid, _, err := c.ValidateToken(ctx, token)
 	if err != nil {
-		return externalAuthLink, "token failed to validate", 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
@@ -172,7 +173,7 @@ validate:
 			goto validate
 		}
 		// The token is no longer valid!
-		return externalAuthLink, "token failed to validate", nil
+		return externalAuthLink, InvalidTokenError("token failed to validate")
 	}
 
 	if token.AccessToken != externalAuthLink.OAuthAccessToken {
@@ -188,11 +189,11 @@ validate:
 			OAuthExtra:             extra,
 		})
 		if err != nil {
-			return updatedAuthLink, "failed to update token", xerrors.Errorf("update external auth link: %w", err)
+			return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err)
 		}
 		externalAuthLink = updatedAuthLink
 	}
-	return externalAuthLink, validReason, 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
index aeb24dee87e6f..7a199d95a317b 100644
--- 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
 
-		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
-		require.NoError(t, err)
-		require.False(t, invalidReason.Valid())
+		_, 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{}
-		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
+		_, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, invalidReason.Valid(), "token without expiry is always valid")
 		require.True(t, validated, "token should have been validated")
 	})
 
@@ -105,11 +105,10 @@ func TestRefreshToken(t *testing.T) {
 				},
 			},
 		}
-		_, invalidReason, 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, invalidReason.Valid())
 	})
 
 	t.Run("ValidateServerError", func(t *testing.T) {
@@ -131,8 +130,9 @@ 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)
+		require.True(t, externalauth.IsInvalidTokenError(err))
 		require.True(t, validated, "token should have been attempted to be validated")
 	})
 
@@ -156,9 +156,9 @@ func TestRefreshToken(t *testing.T) {
 		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 		link.OAuthExpiry = expired
 
-		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
-		require.NoError(t, err, staticError)
-		require.False(t, invalidReason.Valid())
+		_, err := config.RefreshToken(ctx, nil, link)
+		require.ErrorContains(t, err, staticError)
+		require.True(t, externalauth.IsInvalidTokenError(err))
 		require.True(t, validated, "token should have been attempted to be validated")
 	})
 
@@ -191,9 +191,8 @@ func TestRefreshToken(t *testing.T) {
 		// Unlimited lifetime, this is what GitHub returns tokens as
 		link.OAuthExpiry = time.Time{}
 
-		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
+		_, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, invalidReason.Valid())
 		require.Equal(t, 2, validateCalls, "token should have been attempted to be validated more than once")
 	})
 
@@ -219,9 +218,8 @@ func TestRefreshToken(t *testing.T) {
 
 		ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
 
-		_, invalidReason, err := config.RefreshToken(ctx, nil, link)
+		_, err := config.RefreshToken(ctx, nil, link)
 		require.NoError(t, err)
-		require.True(t, invalidReason.Valid())
 		require.Equal(t, 1, validateCalls, "token is validated")
 	})
 
@@ -253,9 +251,8 @@ func TestRefreshToken(t *testing.T) {
 		// Force a refresh
 		link.OAuthExpiry = expired
 
-		updated, invalidReason, err := config.RefreshToken(ctx, db, link)
+		updated, err := config.RefreshToken(ctx, db, link)
 		require.NoError(t, err)
-		require.True(t, invalidReason.Valid())
 		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 +289,9 @@ func TestRefreshToken(t *testing.T) {
 		// Force a refresh
 		link.OAuthExpiry = expired
 
-		updated, invalidReason, err := config.RefreshToken(ctx, db, link)
+		updated, err := config.RefreshToken(ctx, db, link)
 		require.NoError(t, err)
-		require.True(t, invalidReason.Valid())
+
 		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
index 0d6d4e015e51c..8c8a3f64beed1 100644
--- 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, invalidReason, 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 invalidReason.Invalid() {
+			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
index d3e0dedead298..1c9131ef0d17c 100644
--- a/coderd/templateversions.go
+++ b/coderd/templateversions.go
@@ -353,8 +353,8 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ
 			return
 		}
 
-		_, invalidReason, 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(),
@@ -362,7 +362,7 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ
 			return
 		}
 
-		provider.Authenticated = invalidReason.Valid()
+		provider.Authenticated = err == nil
 		providers = append(providers, provider)
 	}
 
diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go
index 019e27b0b3e1f..753e3aeaa0639 100644
--- a/coderd/workspaceagents.go
+++ b/coderd/workspaceagents.go
@@ -1912,25 +1912,25 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	externalAuthLink, invalidReason, 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 invalidReason.Invalid() {
+	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.",

From 85d1fde0b3647cdeb9df66c0b1cfcfbbc44372da Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Fri, 31 May 2024 10:15:25 -0500
Subject: [PATCH 5/6] fix tests

---
 coderd/externalauth/externalauth_test.go | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go
index 7a199d95a317b..fbc1cab4b7091 100644
--- a/coderd/externalauth/externalauth_test.go
+++ b/coderd/externalauth/externalauth_test.go
@@ -108,7 +108,9 @@ func TestRefreshToken(t *testing.T) {
 		_, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
 			OAuthExpiry: expired,
 		})
-		require.NoError(t, err)
+		require.Error(t, err)
+		require.True(t, externalauth.IsInvalidTokenError(err))
+		require.Contains(t, err.Error(), "failure")
 	})
 
 	t.Run("ValidateServerError", func(t *testing.T) {
@@ -132,7 +134,10 @@ func TestRefreshToken(t *testing.T) {
 
 		_, err := config.RefreshToken(ctx, nil, link)
 		require.ErrorContains(t, err, staticError)
-		require.True(t, externalauth.IsInvalidTokenError(err))
+		// 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")
 	})
 
@@ -157,7 +162,7 @@ func TestRefreshToken(t *testing.T) {
 		link.OAuthExpiry = expired
 
 		_, err := config.RefreshToken(ctx, nil, link)
-		require.ErrorContains(t, err, staticError)
+		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")
 	})

From 91a57417357f693287f73a031dc0759c8b56317c Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Fri, 31 May 2024 10:44:10 -0500
Subject: [PATCH 6/6] linting

---
 coderd/externalauth/externalauth.go | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
index 5fc118614bc56..4852de3e860ce 100644
--- a/coderd/externalauth/externalauth.go
+++ b/coderd/externalauth/externalauth.go
@@ -105,10 +105,7 @@ func (e InvalidTokenError) Error() string {
 
 func IsInvalidTokenError(err error) bool {
 	var invalidTokenError InvalidTokenError
-	if xerrors.As(err, &invalidTokenError) {
-		return true
-	}
-	return false
+	return xerrors.As(err, &invalidTokenError)
 }
 
 // RefreshToken automatically refreshes the token if expired and permitted.