Skip to content

Request for consistent handling of OAuth refresh failures #19054

@claudioballardini01

Description

@claudioballardini01

Given the following block of code that handles the refresh of the access token:

// We have a refresh token, so let's try it
token, err := oauthConfig.TokenSource(r.Context(), &oauth2.Token{
AccessToken: link.OAuthAccessToken,
RefreshToken: link.OAuthRefreshToken,
Expiry: link.OAuthExpiry,
}).Token()
if err != nil {
return write(http.StatusUnauthorized, codersdk.Response{
Message: fmt.Sprintf(
"Could not refresh expired %s token. Try re-authenticating to resolve this issue.",
friendlyName),
Detail: err.Error(),
})
}

I propose aligning the behavior of this block with the logic implemented and described in the following block of code:

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.
if isFailedRefresh(existingToken, err) {
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,
})
if dbExecErr != nil {
// This error should be rare.
return externalAuthLink, InvalidTokenError(fmt.Sprintf("refresh token failed: %q, then removing refresh token failed: %q", err.Error(), dbExecErr.Error()))
}
// The refresh token was cleared
externalAuthLink.OAuthRefreshToken = ""
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions