-
Notifications
You must be signed in to change notification settings - Fork 955
Open
Description
Given the following block of code that handles the refresh of the access token:
Lines 334 to 347 in 2a430ab
// 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:
coder/coderd/externalauth/externalauth.go
Lines 140 to 159 in 2a430ab
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
Labels
No labels