feat: keep original token refresh error in external auth

Emyrk · Emyrk · commit 4f8075bf938d · 2025-08-13T10:30:27.000-05:00
External auth refresh errors lose the original error thrown on the
first refresh. This PR saves that error to the database to be raised
on subsequent refresh attempts
diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000358_failed_ext_auth_error.down.sql b/coderd/database/migrations/000358_failed_ext_auth_error.down.sql
@@ -0,0 +1,3 @@
+ALTER TABLE external_auth_links
+	DROP COLUMN oauth_refresh_failure_reason
+;
diff --git a/coderd/database/migrations/000358_failed_ext_auth_error.up.sql b/coderd/database/migrations/000358_failed_ext_auth_error.up.sql
@@ -0,0 +1,7 @@
+ALTER TABLE external_auth_links
+	ADD COLUMN oauth_refresh_failure_reason TEXT NOT NULL DEFAULT ''
+;
+
+COMMENT ON COLUMN external_auth_links.oauth_refresh_failure_reason IS
+	'This error means the refresh token is invalid. Cached so we can avoid calling the external provider again for the same error.'
+;
diff --git a/coderd/database/models.go b/coderd/database/models.go
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/externalauth.sql b/coderd/database/queries/externalauth.sql
@@ -40,13 +40,16 @@ UPDATE external_auth_links SET
     oauth_refresh_token = $6,
     oauth_refresh_token_key_id = $7,
     oauth_expiry = $8,
-	oauth_extra = $9
+	oauth_extra = $9,
+	-- Only 'UpdateExternalAuthLinkRefreshToken' supports updating the oauth_refresh_failure_reason
+	oauth_refresh_failure_reason = ''
 WHERE provider_id = $1 AND user_id = $2 RETURNING *;
 
 -- name: UpdateExternalAuthLinkRefreshToken :exec
 UPDATE
 	external_auth_links
 SET
+	oauth_refresh_failure_reason = @oauth_refresh_failure_reason,
 	oauth_refresh_token = @oauth_refresh_token,
 	updated_at = @updated_at
 WHERE
diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
@@ -28,6 +28,10 @@ import (
 	"github.com/coder/retry"
 )
 
+const (
+	failureReasonLimit = 200
+)
+
 // Config is used for authentication for Git operations.
 type Config struct {
 	promoauth.InstrumentedOAuth2Config
@@ -130,6 +134,12 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 		refreshToken = ""
 	}
 
+	if externalAuthLink.OauthRefreshFailureReason != "" {
+		// If the refresh token is invalid, do not try to keep using it. This will
+		// prevent spamming the IdP with refresh attempts that will fail.
+		refreshToken = ""
+	}
+
 	existingToken := &oauth2.Token{
 		AccessToken:  externalAuthLink.OAuthAccessToken,
 		RefreshToken: refreshToken,
@@ -143,12 +153,19 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
 		// get rid of it. Keeping it around will cause additional refresh
 		// attempts that will fail and cost us api rate limits.
 		if isFailedRefresh(existingToken, err) {
+			reason := err.Error()
+			if len(reason) > failureReasonLimit {
+				// Limit the length of the error message to prevent
+				// spamming the database with long error messages.
+				reason = reason[:failureReasonLimit]
+			}
 			dbExecErr := db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{
-				OAuthRefreshToken:      "", // It is better to clear the refresh token than to keep retrying.
-				OAuthRefreshTokenKeyID: externalAuthLink.OAuthRefreshTokenKeyID.String,
-				UpdatedAt:              dbtime.Now(),
-				ProviderID:             externalAuthLink.ProviderID,
-				UserID:                 externalAuthLink.UserID,
+				// Adding a reason will prevent further attempts to try and refresh the token.
+				OauthRefreshFailureReason: reason,
+				OAuthRefreshTokenKeyID:    externalAuthLink.OAuthRefreshTokenKeyID.String,
+				UpdatedAt:                 dbtime.Now(),
+				ProviderID:                externalAuthLink.ProviderID,
+				UserID:                    externalAuthLink.UserID,
 			})
 			if dbExecErr != nil {
 				// This error should be rare.
diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go
@@ -177,37 +177,44 @@ func TestRefreshToken(t *testing.T) {
 		link.OAuthExpiry = expired
 
 		// Make the failure a server internal error. Not related to the token
+		// This should be retried since this error is temporary.
 		refreshErr = &oauth2.RetrieveError{
 			Response: &http.Response{
 				StatusCode: http.StatusInternalServerError,
 			},
 			ErrorCode: "internal_error",
 		}
-		_, err := config.RefreshToken(ctx, mDB, link)
-		require.Error(t, err)
-		require.True(t, externalauth.IsInvalidTokenError(err))
-		require.Equal(t, refreshCount, 1)
+		totalRefreshes := 0
+		for i := 0; i < 3; i++ {
+			// Each loop will hit the temporary error and retry.
+			_, err := config.RefreshToken(ctx, mDB, link)
+			require.Error(t, err)
+			totalRefreshes++
+			require.True(t, externalauth.IsInvalidTokenError(err))
+			require.Equal(t, refreshCount, totalRefreshes)
+		}
 
-		// Try again with a bad refresh token error
-		// Expect DB call to remove the refresh token
+		// Try again with a bad refresh token error. This will invalidate the
+		// refresh token, and not retry again. Expect DB call to remove the refresh token
 		mDB.EXPECT().UpdateExternalAuthLinkRefreshToken(gomock.Any(), gomock.Any()).Return(nil).Times(1)
 		refreshErr = &oauth2.RetrieveError{ // github error
 			Response: &http.Response{
 				StatusCode: http.StatusOK,
 			},
 			ErrorCode: "bad_refresh_token",
 		}
-		_, err = config.RefreshToken(ctx, mDB, link)
+		_, err := config.RefreshToken(ctx, mDB, link)
 		require.Error(t, err)
+		totalRefreshes++
 		require.True(t, externalauth.IsInvalidTokenError(err))
-		require.Equal(t, refreshCount, 2)
+		require.Equal(t, refreshCount, totalRefreshes)
 
 		// When the refresh token is empty, no api calls should be made
 		link.OAuthRefreshToken = "" // mock'd db, so manually set the token to ''
 		_, err = config.RefreshToken(ctx, mDB, link)
 		require.Error(t, err)
 		require.True(t, externalauth.IsInvalidTokenError(err))
-		require.Equal(t, refreshCount, 2)
+		require.Equal(t, refreshCount, totalRefreshes)
 	})
 
 	// ValidateFailure tests if the token is no longer valid with a 401 response.
diff --git a/scripts/testidp/main.go b/scripts/testidp/main.go
@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"flag"
 	"log"
+	"net/http"
 	"os"
 	"os/signal"
 	"strings"
@@ -13,6 +14,7 @@ import (
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/oauth2"
 
 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/sloghuman"
@@ -96,6 +98,17 @@ func RunIDP() func(t *testing.T) {
 				"groups":             []string{"testidp", "qa", "engineering"},
 				"roles":              []string{"testidp", "admin", "higher_power"},
 			}),
+			oidctest.WithRefresh(func(email string) error {
+				return &oauth2.RetrieveError{
+					Response: &http.Response{
+						StatusCode: http.StatusBadRequest,
+					},
+					Body:             nil,
+					ErrorCode:        "bad_refresh_token",
+					ErrorDescription: "refreshing is set to fail for testing purposes",
+					ErrorURI:         "",
+				}
+			}),
 			oidctest.WithDefaultIDClaims(jwt.MapClaims{}),
 			oidctest.WithDefaultExpire(*expiry),
 			oidctest.WithStaticCredentials(*clientID, *clientSecret),

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+ALTER TABLE external_auth_links`
	`2`	`+ DROP COLUMN oauth_refresh_failure_reason`
	`3`	`+;`