feat(auth): support --exclude flag and combine --scope with --domain/…#844
Open
JackZhao10086 wants to merge 4 commits into
Open
feat(auth): support --exclude flag and combine --scope with --domain/…#844JackZhao10086 wants to merge 4 commits into
JackZhao10086 wants to merge 4 commits into
Conversation
|
|
|
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:
📝 WalkthroughWalkthroughAdds an ChangesAuth Login: scope merging and --exclude
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 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
![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)
226-240: 💤 Low valueConsider simplifying the empty-scope check.
Line 237 uses
strings.TrimSpace(finalScope)before the empty check, butjoinSortedScopeSet(called on line 236) already filters blank scopes, sofinalScopecan only be""or a non-blank string—never" ". TheTrimSpaceis defensive but unnecessary.Optional simplification
- if strings.TrimSpace(finalScope) == "" { + if finalScope == "" {🤖 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 226 - 240, The empty-scope check after applying excludes is overly defensive: since joinSortedScopeSet (used to produce finalScope) already filters blank entries, finalScope will be either "" or a non-blank string, so replace the strings.TrimSpace(finalScope) == "" check with a direct finalScope == "" comparison in the block that handles applyExcludeScopes/finalScope; update the conditional that returns ErrValidation("no scopes left after applying --exclude; nothing to authorize") to use finalScope == "" and remove the unnecessary TrimSpace call.
🤖 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 226-240: The empty-scope check after applying excludes is overly
defensive: since joinSortedScopeSet (used to produce finalScope) already filters
blank entries, finalScope will be either "" or a non-blank string, so replace
the strings.TrimSpace(finalScope) == "" check with a direct finalScope == ""
comparison in the block that handles applyExcludeScopes/finalScope; update the
conditional that returns ErrValidation("no scopes left after applying --exclude;
nothing to authorize") to use finalScope == "" and remove the unnecessary
TrimSpace call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 071ff8f1-7690-4a8d-890d-08b7a5fe3884
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #844 +/- ##
==========================================
+ Coverage 65.77% 65.90% +0.13%
==========================================
Files 516 517 +1
Lines 48625 48803 +178
==========================================
+ Hits 31985 32166 +181
+ Misses 13881 13872 -9
- Partials 2759 2765 +6 ☔ 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@03de1cc7c58fecc31f5589c2b2527e1041204fff🧩 Skill updatenpx skills add cqc-a11y/cli#feat/auth-login-exclude-and-combine-scopes -y -g |
当使用--exclude参数时，必须同时指定--scope、--domain或--recommend中的至少一个，避免非法参数调用
cqc-a11y
force-pushed
the
feat/auth-login-exclude-and-combine-scopes
branch
from
a0ad3d2 to
9a89aa0
Compare
1. 新增--exclude命令行标志用于排除指定的授权范围 2. 移除--scope与--domain/--recommend的互斥限制，改为叠加使用 3. 重构范围合并与排除逻辑，增加校验和辅助工具函数 4. 更新--scope参数的帮助文档说明叠加行为
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 161-164: The code references opts.Exclude in the validation but
LoginOptions doesn't define an Exclude field and the flag isn't registered; add
a new field Exclude []string to the LoginOptions struct and register the CLI
flag in NewCmdAuthLogin by calling cmd.Flags().StringSliceVar(&opts.Exclude,
"exclude", nil, "scopes to exclude (repeatable or comma-separated)"), ensuring
the flag name and option variable match the existing validation that checks
opts.Exclude.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 624-663: applyExcludeScopes currently treats every exclude entry
as a literal and flags patterns like "drive:*" as unknown; update it to
recognize wildcard/pattern excludes (e.g., "*" or "prefix:*" or other simple
glob-like patterns) by splitting excludes into literal entries and pattern
entries, using pattern matching (e.g., strings.HasPrefix for "prefix:*" or
path.Match/regexp for general globs) when computing unknown and when filtering
requestedSet; change the unknown-detection loop to consider a pattern as known
if it matches at least one requested scope, and change the final filtering to
remove any requested scope that matches any exclude pattern or literal (keep
joinSortedScopeSet, requestedSet, excludeSet, and the applyExcludeScopes
signature intact).
- Around line 162-163: The code treats opts.Exclude as a "present option" via
hasAnyOption, which allows `--exclude` alone to bypass the no-option path and
later causes unknown-exclude errors; change the logic so that hasAnyOption
ignores opts.Exclude (i.e., only consider opts.Scope, opts.Recommend, and
selectedDomains) and/or add an explicit validation: if len(opts.Exclude) > 0 and
none of opts.Scope, opts.Recommend, or selectedDomains are set, return an error
rejecting `--exclude` without a base selector. Update the checks that use
hasAnyOption and the validation around excludes (references: hasAnyOption,
opts.Exclude, opts.Scope, opts.Recommend, selectedDomains) so the
interactive/no-option path and exclude validation behave correctly.
- Around line 215-227: The merge currently adds tokens from the raw opts.Scope,
which can contain comma-normalized values and produce a single literal token;
instead merge from the normalized scope string produced earlier (not raw
opts.Scope). Update the loop that currently iterates over
strings.Fields(opts.Scope) to iterate over the normalized scope value (the
variable created by the earlier normalization step) so that merged
map[string]bool receives the correctly split scope tokens before calling
joinSortedScopeSet and assigning finalScope.
- Line 66: The help string for the "scope" flag is duplicated and malformed in
the StringVar call that sets opts.Scope (cmd.Flags().StringVar(&opts.Scope,
"scope", ...)); open that call and remove the repeated trailing text so the flag
description is a single well-formed string (e.g., "scopes to request (space- or
comma-separated). Combines additively with --domain/--recommend)"), ensuring the
call to StringVar uses a single quoted string for the help text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+624
to
+663
| // applyExcludeScopes removes the provided exclude entries from the requested | ||
| // scope string. Each --exclude flag value may itself contain comma- or | ||
| // whitespace-separated scopes. Returns the filtered scope string and any | ||
| // exclude entries that were not present in the requested set (callers can | ||
| // surface those as a validation error to catch typos like | ||
| // `--exclude drive:file:downlod`). | ||
| func applyExcludeScopes(requested string, excludes []string) (string, []string) { | ||
| requestedSet := make(map[string]bool) | ||
| for _, s := range strings.Fields(requested) { | ||
| requestedSet[s] = true | ||
| } | ||
|
|
||
| excludeSet := make(map[string]bool) | ||
| for _, raw := range excludes { | ||
| // --exclude already splits on commas (StringSliceVar), but also | ||
| // tolerate whitespace-separated entries inside a single value. | ||
| for _, s := range strings.Fields(strings.ReplaceAll(raw, ",", " ")) { | ||
| excludeSet[s] = true | ||
| } | ||
| } | ||
|
|
||
| var unknown []string | ||
| for s := range excludeSet { | ||
| if !requestedSet[s] { | ||
| unknown = append(unknown, s) | ||
| } | ||
| } | ||
| if len(unknown) > 0 { | ||
| sort.Strings(unknown) | ||
| return requested, unknown | ||
| } | ||
|
|
||
| kept := make(map[string]bool, len(requestedSet)) | ||
| for s := range requestedSet { | ||
| if !excludeSet[s] { | ||
| kept[s] = true | ||
| } | ||
| } | ||
| return joinSortedScopeSet(kept), nil | ||
| } |
There was a problem hiding this comment.
Wildcard/pattern excludes are not implemented yet.
The linked issue says --exclude should accept wildcard/pattern entries, but this helper only does exact set membership. Inputs like --exclude drive:* will always be returned in unknown, even when the requested set contains matching drive: scopes.
🧰 Tools
🪛 GitHub Check: security
[failure] 646-646:
expected declaration, found 'for'
🤖 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 624 - 663, applyExcludeScopes currently
treats every exclude entry as a literal and flags patterns like "drive:*" as
unknown; update it to recognize wildcard/pattern excludes (e.g., "*" or
"prefix:*" or other simple glob-like patterns) by splitting excludes into
literal entries and pattern entries, using pattern matching (e.g.,
strings.HasPrefix for "prefix:*" or path.Match/regexp for general globs) when
computing unknown and when filtering requestedSet; change the unknown-detection
loop to consider a pattern as known if it matches at least one requested scope,
and change the final filtering to remove any requested scope that matches any
exclude pattern or literal (keep joinSortedScopeSet, requestedSet, excludeSet,
and the applyExcludeScopes signature intact).
移除了重复的参数说明文本，整理冗余的注释内容，让帮助文档更清晰易读
cqc-a11y
force-pushed
the
feat/auth-login-exclude-and-combine-scopes
branch
from
78297c8 to
792f3eb
Compare
添加--exclude参数必须配合其他可选参数使用的校验，避免无效的exclude参数调用
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
3 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.
…--recommend in auth login
Fixes #766
Summary by CodeRabbit
New Features
auth loginadds an--excludeflag to omit specific scopes; scope sources (--scope,--domain,--recommend) now combine additively into the final scope set.--excludeis treated as a valid non-interactive option to avoid triggering prompts when it’s the only flag.Bug Fixes