feat: remote scopes.json for login and status_message passthrough#1799
feat: remote scopes.json for login and status_message passthrough#1799dc-bytedance wants to merge 6 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthrough
ChangesAuth login scope resolution and messaging
Registry helper consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant authLoginRun
participant FetchRemoteScopes
participant ScopesEndpoint
participant resolveScopesForDomains
authLoginRun->>FetchRemoteScopes: fetch brand-specific scopes.json
FetchRemoteScopes->>ScopesEndpoint: HTTP GET
ScopesEndpoint-->>FetchRemoteScopes: validated domain scope map
FetchRemoteScopes-->>authLoginRun: remote data or local fallback
authLoginRun->>resolveScopesForDomains: resolve selected legal domains
resolveScopesForDomains-->>authLoginRun: deduplicated sorted scopes
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1799 +/- ##
==========================================
+ Coverage 74.98% 75.06% +0.08%
==========================================
Files 895 895
Lines 94379 94284 -95
==========================================
+ Hits 70767 70776 +9
+ Misses 18197 18091 -106
- Partials 5415 5417 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@654ded683e913fd1874764c44dbe111e02ed9d1f🧩 Skill updatenpx skills add larksuite/cli#feat/optimize-auth-permission-strategy -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/auth/login.go (1)
595-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for
legalDomainsForThe existingresolveScopesForDomainscoverage doesn’t exercise this wrapper’s remote-key extraction/sort or the local fallback; add a minimal test for each branch.🤖 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/auth/login.go` around lines 595 - 613, Add direct unit tests for legalDomainsFor to cover both branches explicitly, since resolveScopesForDomains does not verify this wrapper’s behavior. Create one test with remoteOK true that asserts the returned membership set uses the remote map keys and the slice is sorted, and another with remoteOK false that asserts it falls back to allKnownDomains and sortedKnownDomains for the given core.LarkBrand. Keep the tests focused on the legalDomainsFor symbol so the remote-key extraction, sorting, and local fallback are all exercised directly.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.
Nitpick comments:
In `@cmd/auth/login.go`:
- Around line 595-613: Add direct unit tests for legalDomainsFor to cover both
branches explicitly, since resolveScopesForDomains does not verify this
wrapper’s behavior. Create one test with remoteOK true that asserts the returned
membership set uses the remote map keys and the slice is sorted, and another
with remoteOK false that asserts it falls back to allKnownDomains and
sortedKnownDomains for the given core.LarkBrand. Keep the tests focused on the
legalDomainsFor symbol so the remote-key extraction, sorting, and local fallback
are all exercised directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 307cabaa-5785-49ad-aedc-d99271e3de15
📒 Files selected for processing (14)
cmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_messages.gocmd/auth/login_messages_test.gocmd/auth/login_result.gocmd/auth/login_result_test.gocmd/auth/login_test.gointernal/auth/device_flow.gointernal/auth/device_flow_test.gointernal/auth/remote_scopes.gointernal/auth/remote_scopes_test.gointernal/registry/loader.gointernal/registry/registry_test.gointernal/registry/remote_test.go
💤 Files with no reviewable changes (5)
- cmd/auth/login_interactive.go
- internal/registry/loader.go
- cmd/auth/login_messages.go
- internal/registry/registry_test.go
- internal/registry/remote_test.go
05f4098 to
4f483b1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/login.go (1)
174-196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate user-provided domains before expanding
--domain all.Because the
--domain allexpansion replacesselectedDomainswithallLegalDomainsbefore validation occurs, the validation loop unintentionally runs against the expanded set rather than the original user input. As a result, explicitly invalid domains are silently ignored if"all"is also present in the flag (e.g.,--domain docs,invalid,all).Validate the original user input first, skipping
"all", and perform the expansion afterward.🐛 Proposed fix to reorder validation and expansion
- // Expand --domain all against the resolved legal domain set. - for _, d := range selectedDomains { - if strings.EqualFold(d, "all") { - selectedDomains = allLegalDomains - break - } - } - if len(selectedDomains) > 0 { // Validate explicitly-supplied domain names and suggest corrections. for _, d := range selectedDomains { + if strings.EqualFold(d, "all") { + continue + } if !legalDomains[d] { if suggestion := suggestDomain(d, legalDomains); suggestion != "" { return errs.NewValidationError(errs.SubtypeInvalidArgument, "unknown domain %q, did you mean %q?", d, suggestion).WithParam("--domain") } available := make([]string, 0, len(legalDomains)) for k := range legalDomains { available = append(available, k) } sort.Strings(available) return errs.NewValidationError(errs.SubtypeInvalidArgument, "unknown domain %q, available domains: %s", d, strings.Join(available, ", ")).WithParam("--domain") } } + + // Expand --domain all against the resolved legal domain set. + for _, d := range selectedDomains { + if strings.EqualFold(d, "all") { + selectedDomains = allLegalDomains + break + } + } } else {🤖 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/auth/login.go` around lines 174 - 196, In the domain-selection flow, validate the original selectedDomains user input before expanding any "all" entry, skipping validation for the literal "all" while preserving the existing suggestion and available-domain errors for other values. Move the allLegalDomains replacement and break logic to run only after this validation completes.
🧹 Nitpick comments (2)
cmd/auth/login_test.go (1)
1159-1159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a valid brand constant for robust test execution.
Passing an empty string
""forcore.LarkBrandbypasses realistic shortcut filtering sinceshortcuts.IsShortcutServiceAvailableexpects and checks against known brand constants. Use a valid constant likecore.BrandFeishuinstead to ensure test accuracy.
cmd/auth/login_test.go#L1159-L1159: replace""withcore.BrandFeishuin theallKnownDomainscall.cmd/auth/login_test.go#L1169-L1169: replace""withcore.BrandFeishuin thecollectScopesForDomainscall.🤖 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/auth/login_test.go` at line 1159, Use the valid core.BrandFeishu constant instead of an empty string for the brand argument in allKnownDomains and collectScopesForDomains in cmd/auth/login_test.go at lines 1159 and 1169.cmd/auth/login.go (1)
611-611: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid computing
allKnownDomainstwice on the fallback path.
sortedKnownDomains(brand)internally callsallKnownDomains(brand). Calling them both here iterates the project and shortcut registries twice. SincelegalDomainsForneeds both the map and the sorted slice, you can compute them both in a single pass.♻️ Proposed refactor
- return allKnownDomains(brand), sortedKnownDomains(brand) + m := allKnownDomains(brand) + sorted := make([]string, 0, len(m)) + for d := range m { + sorted = append(sorted, d) + } + sort.Strings(sorted) + return m, sorted🤖 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/auth/login.go` at line 611, Update the fallback return in legalDomainsFor to avoid invoking allKnownDomains through both return expressions: compute the known-domain map once, derive the sorted domain slice from that map, and return both values while preserving the existing ordering.
🤖 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.
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 174-196: In the domain-selection flow, validate the original
selectedDomains user input before expanding any "all" entry, skipping validation
for the literal "all" while preserving the existing suggestion and
available-domain errors for other values. Move the allLegalDomains replacement
and break logic to run only after this validation completes.
---
Nitpick comments:
In `@cmd/auth/login_test.go`:
- Line 1159: Use the valid core.BrandFeishu constant instead of an empty string
for the brand argument in allKnownDomains and collectScopesForDomains in
cmd/auth/login_test.go at lines 1159 and 1169.
In `@cmd/auth/login.go`:
- Line 611: Update the fallback return in legalDomainsFor to avoid invoking
allKnownDomains through both return expressions: compute the known-domain map
once, derive the sorted domain slice from that map, and return both values while
preserving the existing ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e4da44b-ac4d-4021-9075-46fe2dfec006
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
2782221 to
aece091
Compare
dc-bytedance
changed the title
3867fa0 to
00fa3e3
Compare
Switch auth login's recommended-permission source from the compiled-in local table to a scopes.json fetched from the platform at login time. - add internal/auth/remote_scopes.go: brand-addressed GET with a ~1s timeout and whole-file (binary) validation; scopes are used verbatim when the file is valid, and any fetch/parse/format failure falls back silently to the existing local computation - login.go: fetch remote scopes once per login and use them for domain validation, all-expansion, and per-domain scope selection; drop the terminal interactive page and the local auto-approve filter chain, so --recommend is now equivalent to --domain all - remove login_interactive.go and the service-description getters left orphaned by the interactive-page removal - keep the three entry flags (bare login / --recommend / --domain all) as an equivalent transitional surface
05d64e2 to
b2da915
Compare
2 participants
Summary
auth loginpreviously computed its recommended authorization scopes from a compiled-in local table, so different CLI versions could request different scope sets and drift from the platform's own scope computation. This PR (1) switches the scope source to a remotely published per-brandscopes.jsonfetched at login time (~1s timeout, whole-file validation, silent fallback to the local computation on any failure), (2) removes the terminal interactive domain-selection page and the local auto-approve filter chain so--recommendis equivalent to--domain all, and (3) passes the authorization response'sstatus_messagethrough to the login result, so the server-rendered outcome text replaces the locally synthesized scope-breakdown text.Changes
internal/auth/remote_scopes.go: brand-addressed GET with ~1s timeout and whole-file validation — a valid file is used verbatim (including domains/scopes unknown to this CLI build); any failure (non-2xx, empty body, bad JSON, missing/emptyscopes, a domain missinguser_scopes, or a malformed scope string: fewer than two:-separated segments, an empty segment, or characters outside[a-z0-9_.]) falls back silently to the local computationcmd/auth/login.go: fetch remote scopes once per login and use them for domain validation,--domain all/ bare-login /--recommendexpansion, and per-domain scope selection; the--scope-only path never touches the catalogcmd/auth/login_interactive.go, the auto-approve loader chain ininternal/registry/loader.go, and the service-description getters orphaned by the interactive-page removalinternal/auth/device_flow.go: readstatus_messagefrom the token success response intoDeviceFlowTokenDatacmd/auth/login_result.go+cmd/auth/login_messages.go: rework the login result into a granted / not-granted breakdown; a server-providedstatus_messageis rendered verbatim (no parsing, no truncation) in place of the locally synthesized not-granted text, and the JSONwarningpayload now carries it inhintauth loginin a non-TTY environment no longer fails fast — it initiates full-domain authorization and blocks up to ~10 minutes awaiting authorization (agent harnesses should use--no-wait);--recommendnow requests the full domain scope set rather than the former auto-approve subset (the authorization page still shows every requested scope); the JSONwarningpayload no longer includesmessage— breaking for consumers reading.warning.message(therequested/granted/missingscope lists are unchanged)Test Plan
make unit-testpassed (CI green at head 3867fa0, confirmed by a local full run)status_messagecommits — requires a live authorization response carryingstatus_message; covered by unit tests ininternal/auth/device_flow_test.goandcmd/auth/login_test.goscopes.jsonfor both brands; whole-file validation accepts them; per-domain diff against the local synthesis — 20/21 domains identicalRelated Issues
N/A