diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 3138fd03..7c984630 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -146,6 +146,13 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) { return "", nil } + // Older CLI versions stored the literal string "auto-login" when + // `brev login --token` had no real refresh token to save. Treat it as + // absent so we do not attempt to exchange it with the IdP and fail. + if tokens.RefreshToken == autoLoginSentinel { + tokens.RefreshToken = "" + } + // should always at least have access token? if tokens.AccessToken == "" { breverrors.GetDefaultErrorReporter().ReportMessage("access token is an empty string but shouldn't be") @@ -154,7 +161,13 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) { if err != nil { return "", breverrors.WrapAndTrace(err) } - if !isAccessTokenValid && tokens.RefreshToken != "" { + if !isAccessTokenValid { + if tokens.RefreshToken == "" { + // Access token is expired and we have no refresh token. Returning + // the expired token here would just cause a 401 on the next API + // call; return empty so callers can prompt for re-login instead. + return "", nil + } tokens, err = t.getNewTokensWithRefreshOrNil(tokens.RefreshToken) if err != nil { return "", breverrors.WrapAndTrace(err) @@ -162,8 +175,6 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) { if tokens == nil { return "", nil } - } else if tokens.RefreshToken == "" && tokens.AccessToken == "" { - return "", nil } return tokens.AccessToken, nil } @@ -197,22 +208,39 @@ func shouldLogin() (bool, error) { return trimmed == "y" || trimmed == "", nil } +// autoLoginSentinel is a legacy value older CLI versions stored in place of +// a real token when `brev login --token` was used. It is not a valid token of +// any kind; treat it as "absent" on read. +const autoLoginSentinel = "auto-login" + func (t Auth) LoginWithToken(token string) error { valid, err := isAccessTokenValid(token) if err != nil { return breverrors.WrapAndTrace(err) } if valid { + // The token is a self-contained JWT access token with no accompanying + // refresh token. Previously we stored the string "auto-login" in the + // RefreshToken slot; when the access token expired the refresh path + // then attempted to exchange that sentinel with the IdP, which always + // failed, logging the user out every time the short-lived access + // token aged out. Store an empty RefreshToken instead so the refresh + // path correctly recognizes there is nothing to refresh and prompts + // for a fresh login exactly once. + fmt.Fprintln(os.Stderr, "Note: tokens from --token cannot be refreshed; re-run `brev login` when the session expires.") err := t.authStore.SaveAuthTokens(entity.AuthTokens{ AccessToken: token, - RefreshToken: "auto-login", + RefreshToken: "", }) if err != nil { return breverrors.WrapAndTrace(err) } } else { + // The token is not a JWT, assume it is a refresh token. The access + // token slot is filled with the sentinel so the first API call + // triggers a refresh to populate a real access token. err := t.authStore.SaveAuthTokens(entity.AuthTokens{ - AccessToken: "auto-login", + AccessToken: autoLoginSentinel, RefreshToken: token, }) if err != nil { diff --git a/pkg/store/http.go b/pkg/store/http.go index 60884f81..1bb26497 100644 --- a/pkg/store/http.go +++ b/pkg/store/http.go @@ -104,7 +104,7 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err } attemptsThresh := 1 s.authHTTPClient.restyClient.OnAfterResponse(func(c *resty.Client, r *resty.Response) error { - if r.StatusCode() == http.StatusForbidden && r.Request.Attempt < attemptsThresh+1 { + if isAuthFailure(r.StatusCode()) && r.Request.Attempt < attemptsThresh+1 { err := handler() if err != nil { return breverrors.WrapAndTrace(err) @@ -117,7 +117,7 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err if e != nil { return false } - return r.StatusCode() == http.StatusForbidden + return isAuthFailure(r.StatusCode()) }) s.authHTTPClient.restyClient.SetRetryCount(attemptsThresh) @@ -125,6 +125,14 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err return nil } +// isAuthFailure reports whether an HTTP status code indicates the caller's +// credentials are missing, invalid, or expired. Both 401 Unauthorized and +// 403 Forbidden can signal an expired access token from Brev's APIs, so we +// treat both as triggers for the refresh-and-retry path. +func isAuthFailure(code int) bool { + return code == http.StatusUnauthorized || code == http.StatusForbidden +} + type AuthHTTPClient struct { restyClient *resty.Client auth Auth