feat(auth): validate explicit scopes locally before login network request#760
Closed
feat(auth): validate explicit scopes locally before login network request#760
Conversation
|
|
📝 WalkthroughWalkthroughThis PR adds explicit scope validation to the auth login command, prevents ChangesAuth Login Scope Validation
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Validator as Scope Validator
participant AuthSrv as Device/Auth Server
participant TokenStore as Token Store
CLI->>Validator: user passes --scope / --domain / --recommend
alt explicit --scope provided
Validator-->>CLI: validateExplicitScopes (normalize, dedupe, check knownScopes)
alt invalid scopes
Validator-->>CLI: return error (abort)
else valid scopes
CLI->>AuthSrv: request device authorization with finalScope
end
else domain/recommend path
CLI->>AuthSrv: resolve candidate scopes (domain/recommend) -> request device authorization
end
AuthSrv-->>CLI: device auth flow completes (granted scopes)
CLI->>TokenStore: save tokens and requested scope summary
CLI-->>User: print JSON/text summary (Requested, NewlyGranted, AlreadyGranted, Missing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #760 +/- ##
==========================================
+ Coverage 64.96% 65.24% +0.28%
==========================================
Files 502 504 +2
Lines 46224 46708 +484
==========================================
+ Hits 30030 30476 +446
- Misses 13583 13592 +9
- Partials 2611 2640 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6fe1939cbcb5efcf1c3dae7a07b65d1fe19e2a71🧩 Skill updatenpx skills add larksuite/cli#feat/auth-scope-validator -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/auth/login_test.go (1)
229-241: 💤 Low valueStray
t.Log(err)looks like leftover debug output.Line 231 unconditionally logs the error before any assertion. Likely accidental debug residue — safe to drop to keep test output clean.
🧹 Proposed change
err := validateExplicitScopes("base:app:create malformed:scope") - t.Log(err) if err == nil { t.Fatal("expected validation error for unknown scope") }🤖 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` around lines 229 - 241, In TestValidateExplicitScopes_RejectsUnknownScopes remove the stray debugging statement t.Log(err) so the test only asserts on the returned error; locate the test function TestValidateExplicitScopes_RejectsUnknownScopes and delete the t.Log(err) line (leaving the error checks that assert err != nil and the contains checks for "invalid scope(s): malformed:scope" and "auth scopes --format pretty").cmd/auth/login.go (1)
540-556: 💤 Low valueDead code:
resultslice is built but never used.
validateExplicitScopesonly returns an error; the deduplicatedresultslice is populated for valid scopes but never returned or otherwise consumed. The valid-side branch can be simplified (orresultremoved entirely), keepingseenonly for invalid-scope dedup.♻️ Proposed simplification
knownScopes := knownScopesForIdentity() invalid := make([]string, 0) - result := make([]string, 0, len(normalized)) - seen := make(map[string]bool, len(normalized)) + seen := make(map[string]bool, len(normalized)) for _, s := range normalized { - if !knownScopes[s] { - if !seen[s] { - seen[s] = true - invalid = append(invalid, s) - } - continue - } - if seen[s] { + if knownScopes[s] { continue } - seen[s] = true - result = append(result, s) + if seen[s] { + continue + } + seen[s] = true + invalid = append(invalid, s) }🤖 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 540 - 556, The code builds a deduplicated valid-scope slice `result` but never uses or returns it; remove `result` and simplify `validateExplicitScopes` by keeping only `seen := make(map[string]bool, len(normalized))` to dedupe entries, push unique invalid scopes into `invalid` when `!knownScopes[s]`, and skip duplicates for known scopes by checking `seen[s]` before marking seen; delete the `result` variable and the `result = append(...)` branch (or alternatively, if the intention was to return valid scopes, update the function signature to return that slice and return `result`), referencing the variables `normalized`, `knownScopes`, `invalid`, `seen`, and `result` to locate the code to change.
🤖 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 `@cmd/auth/login.go`:
- Around line 558-563: Replace the call to output.ErrValidation that embeds a
hint with a call to output.ErrWithHint using the correct signature: pass the
numeric code, the error type string, the main message (with formatted invalid
scopes via strings.Join(invalid, ", ")), and the hint text as the fourth
argument; update the invocation that currently references output.ErrValidation
and the invalid variable so it calls output.ErrWithHint(code, errType, msg,
hint) instead of providing five arguments or flipping message/hint order.
---
Nitpick comments:
In `@cmd/auth/login_test.go`:
- Around line 229-241: In TestValidateExplicitScopes_RejectsUnknownScopes remove
the stray debugging statement t.Log(err) so the test only asserts on the
returned error; locate the test function
TestValidateExplicitScopes_RejectsUnknownScopes and delete the t.Log(err) line
(leaving the error checks that assert err != nil and the contains checks for
"invalid scope(s): malformed:scope" and "auth scopes --format pretty").
In `@cmd/auth/login.go`:
- Around line 540-556: The code builds a deduplicated valid-scope slice `result`
but never uses or returns it; remove `result` and simplify
`validateExplicitScopes` by keeping only `seen := make(map[string]bool,
len(normalized))` to dedupe entries, push unique invalid scopes into `invalid`
when `!knownScopes[s]`, and skip duplicates for known scopes by checking
`seen[s]` before marking seen; delete the `result` variable and the `result =
append(...)` branch (or alternatively, if the intention was to return valid
scopes, update the function signature to return that slice and return `result`),
referencing the variables `normalized`, `knownScopes`, `invalid`, `seen`, and
`result` to locate the code to change.
🪄 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: 04619775-6d21-4709-953a-b06e13456c4c
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/auth/login.go (2)
552-557: ⚡ Quick winUse a more specific
errTypethan"validation".Per coding guidelines, error messages on
cmd/**/*.goshould be "specific for AI agent parsing". TheerrTypehere is generic"validation", while elsewhere in this file specific types like"strict_mode"and"internal"are used. Consider"invalid_scope"(or similar) so agents can branch on the failure reason without parsing message text.♻️ Proposed change
if len(invalid) > 0 { return output.ErrWithHint( - output.ExitValidation, "validation", + output.ExitValidation, "invalid_scope", fmt.Sprintf("invalid scope(s): %s", strings.Join(invalid, ", ")), "check the exact scope names with `lark-cli auth scopes --format pretty`, or use `lark-cli auth login --domain <domain> --recommend` to avoid manual scope typos", ) }As per coding guidelines: "Make error messages structured, actionable, and specific for AI agent parsing".
🤖 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 552 - 557, The error uses a generic errType "validation" in the output.ErrWithHint call when invalid scopes are detected; change that errType to a more specific token like "invalid_scope" (or "invalid_scopes") so AI agents can branch on it without parsing the message—update the output.ErrWithHint(...) invocation that checks len(invalid) > 0 (the call that currently passes "validation") to pass "invalid_scope" and leave the rest of the message/hint intact.
563-580: 💤 Low value
knownScopesForIdentityname implies a parameter it doesn't accept.The function is hardcoded to identity
"user"in three places, yet its name suggests it takes an identity argument. Auth login currently only supports"user"(set viacmdutil.SetSupportedIdentitieson line 64), so this is functionally correct — but the name is misleading. Either rename to e.g.knownUserScopesto match the implementation, or accept anidentity stringparameter for symmetry withcollectScopesForDomains(domains, identity)andshortcutSupportsIdentity(sc, identity).♻️ Option A: rename
-func knownScopesForIdentity() map[string]bool { +func knownUserScopes() map[string]bool {And update the call site in
validateExplicitScopes.♻️ Option B: parameterize
-func knownScopesForIdentity() map[string]bool { +func knownScopesForIdentity(identity string) map[string]bool { known := make(map[string]bool) for scope := range registry.LoadScopePriorities() { known[scope] = true } - for _, scope := range registry.CollectAllScopesFromMeta("user") { + for _, scope := range registry.CollectAllScopesFromMeta(identity) { known[scope] = true } for _, sc := range shortcuts.AllShortcuts() { - if shortcutSupportsIdentity(sc, "user") { - for _, scope := range sc.ScopesForIdentity("user") { + if shortcutSupportsIdentity(sc, identity) { + for _, scope := range sc.ScopesForIdentity(identity) { known[scope] = true } } } known["offline_access"] = true return known }And update
validateExplicitScopesto pass"user".🤖 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 563 - 580, knownScopesForIdentity is misleading because it hardcodes "user" in multiple places; change it to accept an identity string parameter (e.g., func knownScopesForIdentity(identity string) map[string]bool) and replace the three hardcoded "user" usages (registry.CollectAllScopesFromMeta, shortcutSupportsIdentity call, and sc.ScopesForIdentity) to use the identity parameter; then update validateExplicitScopes to call knownScopesForIdentity("user") (keeping other logic unchanged) so behavior remains the same but the function is symmetric with collectScopesForDomains and shortcutSupportsIdentity.
🤖 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 552-557: The error uses a generic errType "validation" in the
output.ErrWithHint call when invalid scopes are detected; change that errType to
a more specific token like "invalid_scope" (or "invalid_scopes") so AI agents
can branch on it without parsing the message—update the output.ErrWithHint(...)
invocation that checks len(invalid) > 0 (the call that currently passes
"validation") to pass "invalid_scope" and leave the rest of the message/hint
intact.
- Around line 563-580: knownScopesForIdentity is misleading because it hardcodes
"user" in multiple places; change it to accept an identity string parameter
(e.g., func knownScopesForIdentity(identity string) map[string]bool) and replace
the three hardcoded "user" usages (registry.CollectAllScopesFromMeta,
shortcutSupportsIdentity call, and sc.ScopesForIdentity) to use the identity
parameter; then update validateExplicitScopes to call
knownScopesForIdentity("user") (keeping other logic unchanged) so behavior
remains the same but the function is symmetric with collectScopesForDomains and
shortcutSupportsIdentity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2fad475-fc65-4446-89d1-414d4c13f5af
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
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
Add local scope validation during lark-cli auth login --scope to catch invalid or misspelled scopes before making any network requests, providing actionable error messages with hints for correct scope names.
Changes
Test Plan
Related Issues
Summary by CodeRabbit
New Features
Improvements
Tests