Skip to content

feat: keep original token refresh error in external auth #19339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 14, 2025
Merged

feat: keep original token refresh error in external auth #19339

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aec8e95
feat: keep original token refresh error in external auth
Emyrk Aug 13, 2025
9460b39
add comments
Emyrk Aug 13, 2025
ee6d3fb
add comments
Emyrk Aug 13, 2025
14ff391
make gen
Emyrk Aug 13, 2025
b0a1694
bump max error size
Emyrk Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add comments
  • Loading branch information
@Emyrk
Emyrk committed Aug 14, 2025
commit ee6d3fb4480a4ba4b8ca67a17cab6613d86db103
33 changes: 17 additions & 16 deletions coderd/externalauth/externalauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,36 +129,33 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
return externalAuthLink, InvalidTokenError("token expired, refreshing is either disabled or refreshing failed and will not be retried")
}

refreshToken := externalAuthLink.OAuthRefreshToken

// This is additional defensive programming. Because TokenSource is an interface,
// we cannot be sure that the implementation will treat an 'IsZero' time
// as "not-expired". The default implementation does, but a custom implementation
// might not. Removing the refreshToken will guarantee a refresh will fail.
refreshToken := externalAuthLink.OAuthRefreshToken
if c.NoRefresh {
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.
//
// An empty refresh token will cause `TokenSource(...).Token()` to fail
// without sending a request to the IdP if the token is expired.
refreshToken = ""
}

existingToken := &oauth2.Token{
AccessToken: externalAuthLink.OAuthAccessToken,
RefreshToken: refreshToken,
Expiry: externalAuthLink.OAuthExpiry,
}

// Note: The TokenSource(...) method will make no remote HTTP requests if the
// token is expired and no refresh token is set. This is important to prevent
// spamming the API, consuming rate limits, when the token is known to fail.
token, err := c.TokenSource(ctx, existingToken).Token()
if err != nil {
// TokenSource can fail for numerous reasons. If it fails because of
// a bad refresh token, then the refresh token is invalid, and we should
// get rid of it. Keeping it around will cause additional refresh
// attempts that will fail and cost us api rate limits.
//
// The error message is saved for debugging purposes.
if isFailedRefresh(existingToken, err) {
reason := err.Error()
if len(reason) > failureReasonLimit {
Expand All @@ -169,10 +166,13 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
dbExecErr := db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{
// 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,
// Remove the invalid refresh token so it is never used again. The cached
// `reason` can be used to know why this field was zeroed out.
OAuthRefreshToken: "",
OAuthRefreshTokenKeyID: externalAuthLink.OAuthRefreshTokenKeyID.String,
UpdatedAt: dbtime.Now(),
ProviderID: externalAuthLink.ProviderID,
UserID: externalAuthLink.UserID,
})
if dbExecErr != nil {
// This error should be rare.
Expand All @@ -189,10 +189,11 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
//
// This error messages comes from the oauth2 package on our client side.
// So this check is not against a server generated error message.
// Error source: https://github.com/golang/oauth2/blob/master/oauth2.go#L277
if err.Error() == "oauth2: token expired and refresh token is not set" {
if externalAuthLink.OauthRefreshFailureReason != "" {
// A cached refresh failure error exists. So the refresh token was set, but was invalid.
// Return this cached error for the original refresh attempt. This token will never again be valid.
// A cached refresh failure error exists. So the refresh token was set, but was invalid, and zeroed out.
// Return this cached error for the original refresh attempt.
return externalAuthLink, InvalidTokenError(fmt.Sprintf("token expired and refreshing failed %s with: %s",
// Do not return the exact time, because then we have to know what timezone the
// user is in. This approximate time is good enough.
Expand Down
13 changes: 0 additions & 13 deletions scripts/testidp/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"encoding/json"
"flag"
"log"
"net/http"
"os"
"os/signal"
"strings"
Expand All @@ -14,7 +13,6 @@ 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"
Expand Down Expand Up @@ -98,17 +96,6 @@ 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),
Expand Down