feat: use remote scopes.json for login scope recommendations#1799
Open
dc-bytedance wants to merge 8 commits into
Open
feat: use remote scopes.json for login scope recommendations#1799dc-bytedance wants to merge 8 commits into
dc-bytedance wants to merge 8 commits into
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 (2)
📝 WalkthroughWalkthroughThe PR changes auth login to resolve domains and scopes from remote ChangesAuth login scope resolution and output
Registry helper consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant authLoginRun
participant FetchRemoteScopes
participant resolveScopesForDomains
participant PollDeviceToken
participant authorizationCompletePayload
User->>authLoginRun: login options
authLoginRun->>FetchRemoteScopes: fetch scopes.json
FetchRemoteScopes-->>authLoginRun: remote scopes or local fallback
authLoginRun->>resolveScopesForDomains: selected domains
resolveScopesForDomains-->>authLoginRun: candidate scopes
PollDeviceToken-->>authLoginRun: token and status_message
authLoginRun->>authorizationCompletePayload: status_message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1799 +/- ##
==========================================
+ Coverage 74.66% 74.75% +0.09%
==========================================
Files 877 877
Lines 91731 91624 -107
==========================================
+ Hits 68494 68497 +3
+ Misses 17926 17816 -110
Partials 5311 5311 ☔ 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@05f4098a5a997ad60cf66ca942903c2ee3a543ed🧩 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
dc-bytedance
force-pushed
the
feat/optimize-auth-permission-strategy
branch
from
29b0fe6 to
e81d3f7
Compare
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.
Summary
auth loginpreviously computed its recommended authorization scopes from a compiled-in local table (API catalog + priority scoring + auto-approve filtering), so different CLI versions could request different scope sets and drift from the platform's own scope computation. This PR switches the scope source to a remotely-publishedscopes.jsonfetched per-brand at login time, with silent fallback to the local full set on any fetch/timeout/validation failure. It also removes the terminal interactive domain-selection page (--recommendbecomes equivalent to--domain all) and passes the authorization response'sstatus_messagethrough to the caller verbatim.Changes
internal/auth/remote_scopes.go: per-brand remotescopes.jsonfetch (1s timeout, reusestransport.NewHTTPClient+core.ResolveOpenBaseURL) with binary validation — whole file usable (JSON parses + non-emptyscopesobject + each domain has auser_scopesarray + every scope matchesservice:resource:action) or whole-file fallback to local; i18n / tenant_scopes / version are not validatedcmd/auth/login.go:authLoginRunresolves scopes remote-first / local-fallback via newresolveScopesForDomains+legalDomainsFor; remove the interactive-page branch; bareauth loginand--recommendboth request the full domain set (equivalent to--domain all)cmd/auth/login_interactive.goand theinternal/registryauto-approve filter chain (FilterAutoApproveScopesand friends) plus orphaned interactive message fieldsinternal/auth/device_flow.go+cmd/auth/login_result.go: read and pass throughstatus_messagefrom the success response, verbatim (no parse / classify / truncate)auth login/--recommend/--domain all) as behavior-equivalent transitional aliasesTest Plan
make unit-testpassed--excludeguard, bare-login authorization,--recommend≡--domain all)--excludedependency error,status_messagepassthrough)auth login --helpreflects the recommend/bare-login semantics;auth login --domain bogusxyzreturns a structured unknown-domain error with the available-domain listRelated Issues
N/A (tracked internally)
Summary by CodeRabbit
auth loginnow expands--recommendto cover all known business domains (same as--domain all).status_message.--domain alland explicit domain entries.