fix(auth): preserve user token state on refresh failure - #1820
Open
xu91102 wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates UAT refresh handling to use injectable stored-token accessors, preserve local token state across refresh failures, and return structured refresh errors. Adds tests covering token reuse, expiry, transport failures, and error shaping. ChangesUAT refresh token handling
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/uat_client_test.go`:
- Around line 134-143: The missing-authorization test in `uat_client_test.go`
only checks `errors.As` and token removal; extend this assertion to verify the
typed metadata on the returned `NeedAuthorizationError` using `errs.ProblemOf`.
Locate the existing error-path assertions in the relevant test and add checks
for the expected category/subtype mapping so the test confirms the error still
resolves to the right `errs.ProblemOf` classification.
- Around line 18-19: The withStubbedUATStore test helper does not isolate the
auth config directory, so refresh-path lock files can leak into shared config
state. Update withStubbedUATStore to set LARKSUITE_CLI_CONFIG_DIR to a temp
directory via t.Setenv before any code that may call core.GetConfigDir(), so the
UAT refresh tests stay self-contained and do not affect developer/CI config.
In `@internal/auth/uat_client.go`:
- Around line 315-346: The refresh failure path in buildRefreshFailureError
should distinguish OAuth-style responses that only contain
error/error_description from real API errors with a numeric code. Update the
function to bypass errclass.BuildAPIError when code is missing or -1 and instead
return the existing SubtypeRefreshServerError AuthenticationError path,
preserving opts.UserOpenID and the fallback hint. When a real code exists, keep
using BuildAPIError but pass the original response payload (not just code/msg)
so fields like log_id are retained for diagnostics.
- Around line 260-262: The refresh retry path in uat_client’s token refresh flow
is returning raw transport errors from callEndpoint, which bypasses the
command-facing typed error contract. Update the retry failure handling in the
refresh logic to wrap the underlying error with an errs.* type, specifically
using errs.NewNetworkError(...).WithCause(err), and keep the existing cause so
automation can classify it correctly. Make the change in the branch that logs
the retry network warning and returns the error, preserving the current
token-state behavior while ensuring the returned error is typed.
- Line 125: The waiting branch in getValidAccessToken is incorrectly treating
the preserved expired token from getStoredUAToken as a successful refresh
result. Update the logic around getStoredUAToken and the post-refresh wait path
so that if the stored token is expired or the refresh failed,
GetValidAccessToken does not return it with a nil error; instead, propagate the
refresh failure or force a real re-fetch before returning a token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8774b8bc-7ef1-429d-af09-50232dda4d82
📒 Files selected for processing (2)
internal/auth/uat_client.gointernal/auth/uat_client_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/auth/uat_client.go (1)
325-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the preservation hint into a shared constant.
The same recovery hint is duplicated verbatim in
preservedRefreshStateError(Line 328) and twice inbuildRefreshFailureError(Lines 359 and 367). A single package-level constant avoids drift if the guidance is reworded later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/uat_client.go` around lines 325 - 367, The refresh-state recovery hint is duplicated in preservedRefreshStateError and buildRefreshFailureError, so extract it into a single shared package-level constant and reuse it in both helpers. Update preservedRefreshStateError, wrapRefreshTransportError if needed for consistency, and the AuthenticationError path in buildRefreshFailureError to reference that constant so the guidance stays identical and easy to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/auth/uat_client.go`:
- Around line 325-367: The refresh-state recovery hint is duplicated in
preservedRefreshStateError and buildRefreshFailureError, so extract it into a
single shared package-level constant and reuse it in both helpers. Update
preservedRefreshStateError, wrapRefreshTransportError if needed for consistency,
and the AuthenticationError path in buildRefreshFailureError to reference that
constant so the guidance stays identical and easy to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e295eb9-b6d1-458f-b101-4714ddc41ab2
📒 Files selected for processing (2)
internal/auth/uat_client.gointernal/auth/uat_client_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@855b9e3a69acef9ed1f38654afa70d7beae6d71f🧩 Skill updatenpx skills add xu91102/cli#fix/auth-refresh-preserve-token-state -y -g |
Collaborator
|
@xu91102 不过PR代码还是存在一些需要修改的地方：
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Preserve local user auth state when a refresh endpoint failure happens before the locally recorded
refreshExpiresAt, so a failed refresh no longer collapses immediately totoken_missing.Changes
refreshExpiresAtis already expired.Test Plan
GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./internal/auth -run 'TestGetValidAccessToken_PreservesUnexpiredRefreshTokenOnRefreshReused|TestGetValidAccessToken_ClearsLocallyExpiredRefreshToken' -count=1GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./internal/auth -count=1GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./cmd/auth ./internal/credential -count=1GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./internal/errclass -count=1GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go build -o ./lark-cli .git diff --checkRelated Issues
Summary by CodeRabbit