fix(auth): prevent --help on auth subcommands from triggering OAuth flow#835
fix(auth): prevent --help on auth subcommands from triggering OAuth flow#835nuthalapativarun wants to merge 1 commit into
Conversation
Adds an `args_request_help` helper and guards in `handle_auth_command` so that `gws auth login --help`, `gws auth status --help`, and `gws auth setup --help` print help text and exit cleanly instead of entering the OAuth or gcloud setup flow. The `setup` subcommand already delegated to `parse_setup_args` which handles `--help` internally (via its own clap parser without `disable_help_flag`). The `login` and `status` subcommands now have explicit guards that check for `--help`/`-h` in the raw args before dispatching to any network I/O. Closes googleworkspace#782
🦋 Changeset detectedLatest commit: 6cf4af6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where invoking help flags on specific authentication subcommands incorrectly triggered the full OAuth or setup flow. By implementing a pre-dispatch check for help flags, the CLI now correctly displays help documentation and exits cleanly, improving user experience and preventing unnecessary side effects. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to prevent --help or -h flags on auth subcommands (such as login and status) from triggering OAuth flows or network requests. It introduces a helper function args_request_help, adds guard match arms to handle help requests, and includes several unit tests. However, the reviewer points out that the new guard match arms for the login and status subcommands are redundant and unreachable. Because these subcommands do not disable the help flag, clap's built-in parser already intercepts the help flags and handles them before reaching the subcommand matching logic. The reviewer recommends removing these redundant match arms.
| Some(("login", _)) if args_request_help(args) => { | ||
| // Print login-specific help without starting any OAuth flow. | ||
| build_login_subcommand() | ||
| .print_help() | ||
| .map_err(|e| GwsError::Validation(format!("Failed to print help: {e}")))?; | ||
| Ok(()) | ||
| } | ||
| Some(("login", sub_m)) => { | ||
| let (scope_mode, services_filter) = parse_login_args(sub_m); | ||
|
|
||
| handle_login_inner(scope_mode, services_filter).await | ||
| } | ||
| Some(("setup", sub_m)) => { | ||
| // Collect remaining args and delegate to setup's own clap parser. | ||
| // setup uses disable_help_flag(true) + trailing_var_arg so --help/-h | ||
| // lands in the captured args; parse_setup_args handles it internally. | ||
| let setup_args: Vec<String> = sub_m | ||
| .get_many::<String>("args") | ||
| .map(|vals| vals.cloned().collect()) | ||
| .unwrap_or_default(); | ||
| crate::setup::run_setup(&setup_args).await | ||
| } | ||
| Some(("status", _)) if args_request_help(args) => { | ||
| // Print status-specific help without querying credentials or the network. | ||
| auth_command() | ||
| .find_subcommand_mut("status") | ||
| .unwrap() | ||
| .print_help() | ||
| .map_err(|e| GwsError::Validation(format!("Failed to print help: {e}")))?; | ||
| Ok(()) | ||
| } | ||
| Some(("status", _)) => handle_status().await, |
There was a problem hiding this comment.
Redundant and Unreachable Dead Code
The new guard match arms for login and status subcommands are completely redundant and unreachable dead code.
Why this is redundant:
- When a user runs
gws auth login --helporgws auth status --help,handle_auth_commandis called withargscontaining"--help"or"-h". auth_command().try_get_matches_from(...)is called with these arguments.- Since the
loginandstatussubcommands do not have.disable_help_flag(true)set,clap's built-in parser automatically intercepts the--help/-hflag and returns aclap::error::ErrorwithErrorKind::DisplayHelp. - This error is immediately caught by the
Err(e) if e.kind() == ErrorKind::DisplayHelpmatch arm at lines 447-455 (which prints the help message and returnsOk(())directly). - As a result, the execution never reaches the
match matches.subcommand()block for these help requests.
Recommendation:
Remove these redundant match arms. The existing integration tests added in this PR will still pass perfectly, proving that clap's built-in help handling is already fully sufficient.
Some(("login", sub_m)) => {
let (scope_mode, services_filter) = parse_login_args(sub_m);
handle_login_inner(scope_mode, services_filter).await
}
Some(("setup", sub_m)) => {
// Collect remaining args and delegate to setup's own clap parser.
// setup uses disable_help_flag(true) + trailing_var_arg so --help/-h
// lands in the captured args; parse_setup_args handles it internally.
let setup_args: Vec<String> = sub_m
.get_many::<String>("args")
.map(|vals| vals.cloned().collect())
.unwrap_or_default();
crate::setup::run_setup(&setup_args).await
}
Some(("status", _)) => handle_status().await,2 participants
Description
gws auth login --help,gws auth setup --help, andgws auth status --helpwere triggering the full OAuth/setup flow instead of printing help text. The top-levelgws auth --helpwas already handled correctly.The fix adds an
args_request_helphelper that checks for--help/-hin the raw args slice, plus guards at the subcommand dispatch level inhandle_auth_commandso help requests exit cleanly without any network I/O or credential resolution:login --help/-h: printsloginsubcommand help viabuild_login_subcommand().print_help()and returnsOk(())status --help/-h: printsstatussubcommand help and returnsOk(())without querying credentials or calling the networksetup --help/-h: already handled byparse_setup_args(which has its own clap parser withoutdisable_help_flag); the existing delegation torun_setupcorrectly passes--helpin the trailing argsCloses #782
Checklist:
google-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.