feat(token): mint TAT via unified OAuth v3 Token Endpoint#1408
Open
albertnusouo wants to merge 5 commits into
Open
feat(token): mint TAT via unified OAuth v3 Token Endpoint#1408albertnusouo wants to merge 5 commits into
albertnusouo wants to merge 5 commits into
Conversation
Switch tenant-access-token minting from the legacy
/open-apis/auth/v3/tenant_access_token/internal endpoint to the unified
OAuth 2.0 Token Endpoint (accounts.<brand>/oauth/v3/token) using the
client_credentials grant with client_secret_post auth.
- core: add OAuthTokenV3Path constant
- credential: FetchTAT/fetchTAT POST a form-encoded
grant_type/client_id/client_secret to the accounts domain; parse the
OAuth {code, access_token, error, error_description} response; keep
5xx/server_error untyped so probe callers stay silent; classify
invalid_client/unauthorized_client as CategoryConfig/InvalidClient
- credential: scope-bound TAT (TokenSpec.Scopes); scoped mints bypass the
unscoped tatOnce cache
- client: Agent Employee (hybrid TAT) missing-scope auto-retry — on
99991679, re-mint a TAT carrying the app's full granted scopes and
retry once
- config: refresh init-probe wording for the OAuth2 error model
Tests cover the v3 URL/form/response shape, OAuth error classification,
scope-bound vs unscoped mint, and the missing-scope retry.
github-actions
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates tenant access token acquisition to OAuth 2.0 v3 token endpoint with form-encoded client_credentials, updates deterministic error classification to use OAuth2 error strings (e.g., invalid_client/unauthorized_client → ConfigError/SubtypeInvalidClient), and updates probe integration and tests accordingly. ChangesOAuth v3 TAT Endpoint Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b7db0f7cdc7a057ce0a28ea34532ddda53526eea🧩 Skill updatenpx skills add larksuite/cli#feat/token_endpoint_oauth_v3 -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1408 +/- ##
==========================================
+ Coverage 72.75% 72.83% +0.07%
==========================================
Files 730 732 +2
Lines 69034 69157 +123
==========================================
+ Hits 50228 50369 +141
+ Misses 15034 15005 -29
- Partials 3772 3783 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/credential/default_provider_test.go (1)
28-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
errs.ProblemOffor typed error assertions in these error-path tests.These tests currently rely on
errors.As(and in Line 66 path, only “not ConfigError”), which can miss regressions where the error stops being typed. Please assert typed metadata viaerrs.ProblemOfand then check category/subtype from the returned problem.As per coding guidelines: “Error-path tests must assert typed metadata via
errs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone.”Suggested assertion pattern update
func TestClassifyTATResponseCode_InvalidClient_MapsToInvalidClient(t *testing.T) { err := classifyTATResponseCode(0, "invalid_client", "client authentication failed", "feishu", "cli_app_x") if err == nil { t.Fatal("expected non-nil error for invalid_client") } - var cfgErr *errs.ConfigError - if !errors.As(err, &cfgErr) { - t.Fatalf("expected *errs.ConfigError, got %T: %v", err, err) - } - if cfgErr.Category != errs.CategoryConfig { - t.Errorf("Category = %q, want %q", cfgErr.Category, errs.CategoryConfig) - } - if cfgErr.Subtype != errs.SubtypeInvalidClient { - t.Errorf("Subtype = %q, want %q", cfgErr.Subtype, errs.SubtypeInvalidClient) + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed errs.* error, got %T: %v", err, err) + } + if p.Category != errs.CategoryConfig { + t.Errorf("Category = %q, want %q", p.Category, errs.CategoryConfig) + } + if p.Subtype != errs.SubtypeInvalidClient { + t.Errorf("Subtype = %q, want %q", p.Subtype, errs.SubtypeInvalidClient) } - if cfgErr.Hint == "" { + if p.Hint == "" { t.Error("Hint must be non-empty so the user gets a recovery action") } } func TestClassifyTATResponseCode_OtherErrorFallsThrough(t *testing.T) { err := classifyTATResponseCode(20068, "invalid_scope", "unauthorized scope", "feishu", "cli_app_x") if err == nil { t.Fatal("expected non-nil error for invalid_scope") } - var cfgErr *errs.ConfigError - if errors.As(err, &cfgErr) { - t.Fatalf("invalid_scope must not be classified as ConfigError, got %T", err) + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed errs.* error, got %T: %v", err, err) + } + if p.Category == errs.CategoryConfig && p.Subtype == errs.SubtypeInvalidClient { + t.Fatalf("invalid_scope must not be classified as ConfigError/InvalidClient") } }🤖 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/credential/default_provider_test.go` around lines 28 - 73, Replace the current errors.As-based assertions in the three tests that call classifyTATResponseCode with calls to errs.ProblemOf: call p := errs.ProblemOf(err) and assert p is non-nil for cases that should yield a typed problem (TestClassifyTATResponseCode_InvalidClient_MapsToInvalidClient and TestClassifyTATResponseCode_UnauthorizedClient_MapsToInvalidClient), then check p.Category == errs.CategoryConfig and p.Subtype == errs.SubtypeInvalidClient; for TestClassifyTATResponseCode_OtherErrorFallsThrough assert errs.ProblemOf(err) is nil or that p.Category != errs.CategoryConfig to ensure it isn’t classified as a ConfigError; also ensure the original error is still the cause (err wraps the problem) when applicable.Source: Coding guidelines
🧹 Nitpick comments (2)
internal/credential/tat_fetch_test.go (1)
89-98: ⚡ Quick winUse
errs.ProblemOffor typed metadata assertions in error-path tests.These assertions currently validate typed errors via
errors.As/errs.IsTyped, but the repo test rule requires asserting category/subtype througherrs.ProblemOfon error paths.Suggested test-assertion adjustment
- var cfgErr *errs.ConfigError - if !errors.As(err, &cfgErr) { - t.Fatalf("error not *errs.ConfigError: %T %v", err, err) - } - if cfgErr.Category != errs.CategoryConfig { - t.Errorf("Category = %q, want %q", cfgErr.Category, errs.CategoryConfig) - } - if cfgErr.Subtype != errs.SubtypeInvalidClient { - t.Errorf("Subtype = %q, want %q", cfgErr.Subtype, errs.SubtypeInvalidClient) - } + prob, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T %v", err, err) + } + if prob.Category != errs.CategoryConfig { + t.Errorf("Category = %q, want %q", prob.Category, errs.CategoryConfig) + } + if prob.Subtype != errs.SubtypeInvalidClient { + t.Errorf("Subtype = %q, want %q", prob.Subtype, errs.SubtypeInvalidClient) + } @@ - if !errs.IsTyped(err) { - t.Fatalf("expected a typed errs.* error, got %T %v", err, err) - } + prob, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T %v", err, err) + } + if prob.Category == errs.CategoryConfig && prob.Subtype == errs.SubtypeInvalidClient { + t.Errorf("invalid_scope must not map to Config/InvalidClient") + }As per coding guidelines: "
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone."Also applies to: 113-119
🤖 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/credential/tat_fetch_test.go` around lines 89 - 98, Replace the current errors.As based assertions in the test that extract cfgErr and compare Category/Subtype with direct usage of errs.ProblemOf on the returned error; specifically, stop using errors.As/errs.IsTyped with cfgErr and instead call errs.ProblemOf(err) to retrieve the Problem value and assert Problem.Category == errs.CategoryConfig and Problem.Subtype == errs.SubtypeInvalidClient (and assert the preserved cause if applicable). Update both the block referencing cfgErr (the errors.As call and subsequent Category/Subtype checks) and the similar assertions at lines 113-119 to use errs.ProblemOf for typed metadata assertions and verify cause preservation.Source: Coding guidelines
cmd/config/init_probe_test.go (1)
92-106: ⚡ Quick winAlign typed error assertions with
errs.ProblemOfin probe error-path tests.For these error-path tests, prefer asserting category/subtype through
errs.ProblemOfinstead of only concrete type checks (errors.As) orerrs.IsTypedpresence checks.As per coding guidelines: "
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone."Also applies to: 165-167
🤖 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 `@cmd/config/init_probe_test.go` around lines 92 - 106, Replace the concrete-type assertions in assertConfigRejection to use errs.ProblemOf to validate the error's typed metadata (category, subtype and params) and ensure the underlying cause is preserved: call errs.ProblemOf(err) and assert the returned Problem has Category == errs.CategoryConfig and Subtype == errs.SubtypeInvalidClient and that Problem.Cause matches the original cause (or use errors.Is on the cause); remove or keep a minimal errors.As check only if you still need the concrete type but rely on errs.ProblemOf for metadata checks; apply the same change to the similar assertions around lines 165-167.Source: Coding guidelines
🤖 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/client/client_test.go`:
- Around line 667-690: Add tests that exercise the actual retry behavior in
DoSDKRequest instead of only respMissingScope: write one test that simulates an
initial API response with code 99991679, verifies DoSDKRequest detects
respMissingScope, triggers a scoped bot TAT mint, retries the request
successfully, and then reuses that scoped token for a subsequent DoSDKRequest
call (assert the mint was called only once and the second call used the scoped
token). Also add a test where the retry also returns 99991679 to assert
DoSDKRequest does not loop (only one retry) and surfaces the error; use the same
test harness/mocks used elsewhere in client_test.go to stub responses and count
minting attempts, referencing DoSDKRequest and respMissingScope to locate the
logic to exercise.
In `@internal/client/client.go`:
- Around line 50-54: The botScopedTok cache currently returns a stored scoped
TAT forever; modify resolveAccessToken (and related code paths that set
fullScopeBotToken / botScopedTok) to store token expiry/issued-at metadata when
caching, or else clear/invalidate botScopedTok under botScopedMu when API
responses indicate token invalid/expired (e.g., token-invalid, 401/403 or the
99991679 missing_scope flow), so subsequent calls re-run
Credential.ResolveToken; ensure both the lazy-set site (where
fullScopeBotToken/botScopedTok is populated) and the DoSDKRequest error handling
paths that detect invalid tokens clear the cached token under the same mutex so
the process can recover without restart.
---
Outside diff comments:
In `@internal/credential/default_provider_test.go`:
- Around line 28-73: Replace the current errors.As-based assertions in the three
tests that call classifyTATResponseCode with calls to errs.ProblemOf: call p :=
errs.ProblemOf(err) and assert p is non-nil for cases that should yield a typed
problem (TestClassifyTATResponseCode_InvalidClient_MapsToInvalidClient and
TestClassifyTATResponseCode_UnauthorizedClient_MapsToInvalidClient), then check
p.Category == errs.CategoryConfig and p.Subtype == errs.SubtypeInvalidClient;
for TestClassifyTATResponseCode_OtherErrorFallsThrough assert
errs.ProblemOf(err) is nil or that p.Category != errs.CategoryConfig to ensure
it isn’t classified as a ConfigError; also ensure the original error is still
the cause (err wraps the problem) when applicable.
---
Nitpick comments:
In `@cmd/config/init_probe_test.go`:
- Around line 92-106: Replace the concrete-type assertions in
assertConfigRejection to use errs.ProblemOf to validate the error's typed
metadata (category, subtype and params) and ensure the underlying cause is
preserved: call errs.ProblemOf(err) and assert the returned Problem has Category
== errs.CategoryConfig and Subtype == errs.SubtypeInvalidClient and that
Problem.Cause matches the original cause (or use errors.Is on the cause); remove
or keep a minimal errors.As check only if you still need the concrete type but
rely on errs.ProblemOf for metadata checks; apply the same change to the similar
assertions around lines 165-167.
In `@internal/credential/tat_fetch_test.go`:
- Around line 89-98: Replace the current errors.As based assertions in the test
that extract cfgErr and compare Category/Subtype with direct usage of
errs.ProblemOf on the returned error; specifically, stop using
errors.As/errs.IsTyped with cfgErr and instead call errs.ProblemOf(err) to
retrieve the Problem value and assert Problem.Category == errs.CategoryConfig
and Problem.Subtype == errs.SubtypeInvalidClient (and assert the preserved cause
if applicable). Update both the block referencing cfgErr (the errors.As call and
subsequent Category/Subtype checks) and the similar assertions at lines 113-119
to use errs.ProblemOf for typed metadata assertions and verify cause
preservation.
🪄 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: 75da74f1-057f-4193-8b30-c04c283c6a81
📒 Files selected for processing (11)
cmd/config/init_probe.gocmd/config/init_probe_test.gointernal/client/client.gointernal/client/client_test.gointernal/core/types.gointernal/core/types_test.gointernal/credential/default_provider.gointernal/credential/default_provider_test.gointernal/credential/tat_fetch.gointernal/credential/tat_fetch_test.gointernal/credential/types.go
Keep only the core TAT → unified OAuth v3 Token Endpoint swap; remove the Agent Employee (hybrid TAT) missing-scope auto-retry, which is out of scope for this change. Removed: - client: botScopedTok cache, resolveAccessToken cache check, DoSDKRequest 99991679 auto-retry, respMissingScope / fullScopeBotToken / appGrantedScopes - credential: TokenSpec.Scopes; scope parameter on FetchTAT / resolveTAT / doResolveTAT (fetchTAT folded back into FetchTAT) - tests: missing-scope retry tests, scope-bound mint tests Retained: v3 endpoint (client_credentials / client_secret_post / form-encoded), OAuth error classification (invalid_client / unauthorized_client -> InvalidClient) with the code-0 backstop, and init-probe wording.
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
1 participant
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.
What
Migrate tenant-access-token (TAT) minting from the legacy per-token endpoint (
/open-apis/auth/v3/tenant_access_token/internal) to the unified OAuth 2.0 Token Endpoint (accounts.<brand>/oauth/v3/token). This swaps only the TAT request interface — no identity, caching, or scope behavior changes.Why
/oauth/v3/tokenis the platform direction for all grant types and the path the server now expects forclient_credentials.Changes
OAuthTokenV3Path = "/oauth/v3/token".FetchTATPOSTs a form-encodedgrant_type=client_credentials+client_id+client_secret(client_secret_post) to the accounts domain; parses the OAuth{code, access_token, error, error_description}response; transient5xx/server_errorstay untyped so the post-config initprobe stays silent.invalid_client/unauthorized_client→CategoryConfig/InvalidClient; every other deterministic error → typed viaBuildAPIError, with a code-0 backstop so an OAuth error carrying no numeric code (e.g.invalid_scope) is never swallowed into a("", nil)empty-token "success".Not in scope (explored then removed on this branch)
The Agent Employee (hybrid TAT) missing-scope auto-retry / scope-bound TAT minting was added in earlier commits and then removed (
revert(token): drop Agent Employee hybrid-TAT scope chain). Net effect of the branch is the endpoint swap + error handling only —internal/client/client.goandinternal/credential/types.goare unchanged vs the base.Testing
go build ./...,go vet,gofmt -lclean.invalid_client/unauthorized_client/ other, including the code-0 path), transient-untyped handling, and the post-config initprobe propagation. All green.config init/--as botcalls hitPOST accounts.feishu.cn/oauth/v3/tokenwithgrant_type=client_credentials+client_id+client_secret(captured via proxy).Notes
accounts.feishu.cn/accounts.larksuite.com); no BOE/staging hardcoding.client_secret_post.