Skip to content

Commit ee6d3fb

Browse files
EmyrkEmyrk
committed
add comments
1 parent 9460b39 commit ee6d3fb

File tree

2 files changed

+17
-29
lines changed

2 files changed

+17
-29
lines changed

‎coderd/externalauth/externalauth.go

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -129,36 +129,33 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
129129
return externalAuthLink, InvalidTokenError("token expired, refreshing is either disabled or refreshing failed and will not be retried")
130130
}
131131

132+
refreshToken := externalAuthLink.OAuthRefreshToken
133+
132134
// This is additional defensive programming. Because TokenSource is an interface,
133135
// we cannot be sure that the implementation will treat an 'IsZero' time
134136
// as "not-expired". The default implementation does, but a custom implementation
135137
// might not. Removing the refreshToken will guarantee a refresh will fail.
136-
refreshToken := externalAuthLink.OAuthRefreshToken
137138
if c.NoRefresh {
138139
refreshToken = ""
139140
}
140141

141-
if externalAuthLink.OauthRefreshFailureReason != "" {
142-
// If the refresh token is invalid, do not try to keep using it. This will
143-
// prevent spamming the IdP with refresh attempts that will fail.
144-
//
145-
// An empty refresh token will cause `TokenSource(...).Token()` to fail
146-
// without sending a request to the IdP if the token is expired.
147-
refreshToken = ""
148-
}
149-
150142
existingToken := &oauth2.Token{
151143
AccessToken: externalAuthLink.OAuthAccessToken,
152144
RefreshToken: refreshToken,
153145
Expiry: externalAuthLink.OAuthExpiry,
154146
}
155147

148+
// Note: The TokenSource(...) method will make no remote HTTP requests if the
149+
// token is expired and no refresh token is set. This is important to prevent
150+
// spamming the API, consuming rate limits, when the token is known to fail.
156151
token, err := c.TokenSource(ctx, existingToken).Token()
157152
if err != nil {
158153
// TokenSource can fail for numerous reasons. If it fails because of
159154
// a bad refresh token, then the refresh token is invalid, and we should
160155
// get rid of it. Keeping it around will cause additional refresh
161156
// attempts that will fail and cost us api rate limits.
157+
//
158+
// The error message is saved for debugging purposes.
162159
if isFailedRefresh(existingToken, err) {
163160
reason := err.Error()
164161
if len(reason) > failureReasonLimit {
@@ -169,10 +166,13 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
169166
dbExecErr := db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{
170167
// Adding a reason will prevent further attempts to try and refresh the token.
171168
OauthRefreshFailureReason: reason,
172-
OAuthRefreshTokenKeyID: externalAuthLink.OAuthRefreshTokenKeyID.String,
173-
UpdatedAt: dbtime.Now(),
174-
ProviderID: externalAuthLink.ProviderID,
175-
UserID: externalAuthLink.UserID,
169+
// Remove the invalid refresh token so it is never used again. The cached
170+
// `reason` can be used to know why this field was zeroed out.
171+
OAuthRefreshToken: "",
172+
OAuthRefreshTokenKeyID: externalAuthLink.OAuthRefreshTokenKeyID.String,
173+
UpdatedAt: dbtime.Now(),
174+
ProviderID: externalAuthLink.ProviderID,
175+
UserID: externalAuthLink.UserID,
176176
})
177177
if dbExecErr != nil {
178178
// This error should be rare.
@@ -189,10 +189,11 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
189189
//
190190
// This error messages comes from the oauth2 package on our client side.
191191
// So this check is not against a server generated error message.
192+
// Error source: https://github.com/golang/oauth2/blob/master/oauth2.go#L277
192193
if err.Error() == "oauth2: token expired and refresh token is not set" {
193194
if externalAuthLink.OauthRefreshFailureReason != "" {
194-
// A cached refresh failure error exists. So the refresh token was set, but was invalid.
195-
// Return this cached error for the original refresh attempt. This token will never again be valid.
195+
// A cached refresh failure error exists. So the refresh token was set, but was invalid, and zeroed out.
196+
// Return this cached error for the original refresh attempt.
196197
return externalAuthLink, InvalidTokenError(fmt.Sprintf("token expired and refreshing failed %s with: %s",
197198
// Do not return the exact time, because then we have to know what timezone the
198199
// user is in. This approximate time is good enough.

‎scripts/testidp/main.go

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"encoding/json"
55
"flag"
66
"log"
7-
"net/http"
87
"os"
98
"os/signal"
109
"strings"
@@ -14,7 +13,6 @@ import (
1413
"github.com/golang-jwt/jwt/v4"
1514
"github.com/google/uuid"
1615
"github.com/stretchr/testify/require"
17-
"golang.org/x/oauth2"
1816

1917
"cdr.dev/slog"
2018
"cdr.dev/slog/sloggers/sloghuman"
@@ -98,17 +96,6 @@ func RunIDP() func(t *testing.T) {
9896
"groups": []string{"testidp", "qa", "engineering"},
9997
"roles": []string{"testidp", "admin", "higher_power"},
10098
}),
101-
oidctest.WithRefresh(func(email string) error {
102-
return &oauth2.RetrieveError{
103-
Response: &http.Response{
104-
StatusCode: http.StatusBadRequest,
105-
},
106-
Body: nil,
107-
ErrorCode: "bad_refresh_token",
108-
ErrorDescription: "refreshing is set to fail for testing purposes",
109-
ErrorURI: "",
110-
}
111-
}),
11299
oidctest.WithDefaultIDClaims(jwt.MapClaims{}),
113100
oidctest.WithDefaultExpire(*expiry),
114101
oidctest.WithStaticCredentials(*clientID, *clientSecret),

0 commit comments

Comments
 (0)