feat(auth): add --no-localhost flag to gws auth login#231
feat(auth): add --no-localhost flag to gws auth login#231vinay-google wants to merge 9 commits intomainfrom
--no-localhost flag to gws auth login#231Conversation
This commit introduces a `--no-localhost` flag to the `login` subcommand, which enables an out-of-band (OOB) OAuth flow. When this flag is provided, the CLI will output an authentication URL and prompt the user to paste the authorization code back into the terminal. This is particularly useful in environments where a local redirect server cannot be started or accessed, similar to `clasp login --no-localhost`. The flag adjusts the redirect URIs to prioritize `urn:ietf:wg:oauth:2.0:oob` and configures the `InstalledFlowAuthenticator` to use `InstalledFlowReturnMethod::Interactive` instead of `HTTPRedirect`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
🦋 Changeset detectedLatest commit: 210bee2 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 introduces a significant enhancement to the Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
googleworkspace-bot
added
the
cla: yes
This commit introduces a `--no-localhost` flag to the `login` subcommand, which enables an out-of-band (OOB) OAuth flow. When this flag is provided, the CLI will output an authentication URL and prompt the user to paste the authorization code back into the terminal. This is particularly useful in environments where a local redirect server cannot be started or accessed, similar to `clasp login --no-localhost`. The flag adjusts the redirect URIs to prioritize `urn:ietf:wg:oauth:2.0:oob` and configures the `InstalledFlowAuthenticator` to use `InstalledFlowReturnMethod::Interactive` instead of `HTTPRedirect`.
![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 adds a --no-localhost flag to the gws auth login command, enabling an out-of-band authentication flow for environments without a local web browser. The implementation correctly parses the new flag and configures yup_oauth2 to use the interactive flow when specified. The changes are logical and well-contained. I have one suggestion to simplify the logic for constructing the redirect_uris, which would improve clarity and correctness.
googleworkspace-bot
added
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the --no-localhost flag for gws auth login, enabling an out-of-band authentication flow for environments without a browser or localhost access. The implementation correctly uses yup_oauth2's Interactive flow when the flag is present. My review found one high-severity issue related to argument parsing. The existing manual parser can cause the new --no-localhost flag to be incorrectly consumed as a value for another flag like --account, leading to incorrect behavior. Please see the detailed comment.
| if args[i] == "--no-localhost" { | ||
| no_localhost = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The manual argument parsing logic in this function has a bug that affects this new flag. If a user provides an argument-taking flag like --account without a value, followed by --no-localhost, the --no-localhost flag will be incorrectly consumed as the value for --account.
For example: gws auth login --account --no-localhost
This will result in account_email being Some("--no-localhost") and this no_localhost flag being ignored, leading to confusing errors.
The root cause is that argument-taking flags don't check if the next token is another flag. While a full refactor to use clap for subcommand arguments is a larger change, the current parser should be made more robust to prevent this kind of incorrect behavior.
A potential fix for the --account flag (and similar flags) would be to check that the value is not a flag itself:
if args[i] == "--account" && i + 1 < args.len() && !args[i + 1].starts_with('-') {
// ...
}Since this change would be outside the diff, I'm pointing out the issue here on the new code that is impacted by it.
This commit introduces a `--no-localhost` flag to the `login` subcommand, which enables an out-of-band (OOB) OAuth flow. When this flag is provided, the CLI will output an authentication URL and prompt the user to paste the authorization code back into the terminal. This is particularly useful in environments where a local redirect server cannot be started or accessed, similar to `clasp login --no-localhost`. The flag adjusts the redirect URIs to prioritize `urn:ietf:wg:oauth:2.0:oob` and configures the `InstalledFlowAuthenticator` to use `InstalledFlowReturnMethod::Interactive` instead of `HTTPRedirect`. Additionally, it extracts the authorization code from a full redirect URL pasted by the user.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully adds the --no-localhost flag for out-of-band authentication, which is a great feature for headless environments. The implementation is mostly correct, but I've identified two high-severity issues. First, there's a blocking I/O call within an async function, which can freeze the application. Second, the manual argument parsing logic is fragile and can lead to incorrect behavior when flags are passed in a certain order. My review includes suggestions to fix both of these problems.
| let mut user_input = String::new(); | ||
| std::io::stdin() | ||
| .read_line(&mut user_input) | ||
| .map_err(|e| format!("Failed to read code: {e}"))?; |
There was a problem hiding this comment.
The use of std::io::stdin().read_line() here is a blocking I/O operation inside an async block. This can block the entire worker thread of the async runtime, leading to poor performance and unresponsiveness, especially if other async tasks are pending. It's crucial to avoid blocking calls in async code.
To fix this, you should move the blocking operation to a dedicated thread pool using tokio::task::spawn_blocking.
| let mut user_input = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut user_input) | |
| .map_err(|e| format!("Failed to read code: {e}"))?; | |
| let user_input = tokio::task::spawn_blocking(|| { | |
| let mut line = String::new(); | |
| std::io::stdin().read_line(&mut line).map(|_| line) | |
| }) | |
| .await | |
| .map_err(|e| format!("Failed to join blocking task: {e}"))? | |
| .map_err(|e| format!("Failed to read code: {e}"))?; |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
|
@jules address pr review comments |
There was a problem hiding this comment.
Code Review
This pull request introduces a --no-localhost flag for the gws auth login command, enabling an out-of-band authentication flow for headless environments. The implementation correctly uses yup_oauth2's interactive flow when the flag is present. However, there is a critical copy-paste error in src/auth_commands.rs that duplicates code and will cause a compilation failure. This needs to be fixed before merging.
| let redirect_uris = if no_localhost { | ||
| vec![ | ||
| "urn:ietf:wg:oauth:2.0:oob".to_string(), | ||
| "http://localhost".to_string(), | ||
| ] | ||
| } else { | ||
| let redirect_uris = if no_localhost { | ||
| vec!["urn:ietf:wg:oauth:2.0:oob".to_string()] | ||
| } else { | ||
| vec!["http://localhost".to_string()] | ||
| }; |
There was a problem hiding this comment.
This block of code appears to have a copy-paste error, resulting in duplicated logic and a syntax error that will prevent compilation. The if no_localhost block is defined twice, and the first definition incorrectly includes http://localhost in the redirect_uris for the --no-localhost case. The logic should be consolidated into a single, correct if/else block.
let redirect_uris = if no_localhost {
vec!["urn:ietf:wg:oauth:2.0:oob".to_string()]
} else {
vec!["http://localhost".to_string()]
};
googleworkspace-bot
added
the
gemini: reviewed
Resolved an unclosed delimiter compiler error that was caused by a formatting/merge conflict in `src/auth_commands.rs`. Tests pass, and `cargo fmt` has been run on all code.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
Resolved the unclosed delimiter in `src/auth_commands.rs` that broke CI. Verified by running `cargo fmt --all -- --check`, `cargo check`, `cargo build`, and `cargo test`.
There was a problem hiding this comment.
Code Review
This pull request introduces a --no-localhost flag for the gws auth login command, enabling an out-of-band authentication flow. This is a valuable addition for users in environments without a graphical browser or where localhost redirects are not feasible, such as on headless servers or in containers. The implementation correctly uses yup_oauth2's InstalledFlowReturnMethod::Interactive and handles user input for the authorization code. I've found one critical issue in the handling of pasted URLs which could lead to authentication failures, and I've provided a suggestion to address it. Otherwise, the changes look good.
src/auth_commands.rs
Outdated
| if let Ok(parsed_url) = reqwest::Url::parse(input) { | ||
| for (k, v) in parsed_url.query_pairs() { | ||
| if k == "code" { | ||
| return Ok(v.to_string()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, assume they pasted just the code | ||
| Ok(input.to_string()) |
There was a problem hiding this comment.
The current logic for handling a pasted URL is flawed. If a user pastes a string that is a valid URL but does not contain a code query parameter (e.g., https://google.com), the code will fall through and treat the entire URL string as the authorization code. This will lead to a cryptic authentication failure later on.
It's better to fail early with a clear error message if a URL is provided but it's not the expected redirect URL containing the authorization code.
| if let Ok(parsed_url) = reqwest::Url::parse(input) { | |
| for (k, v) in parsed_url.query_pairs() { | |
| if k == "code" { | |
| return Ok(v.to_string()); | |
| } | |
| } | |
| } | |
| // Otherwise, assume they pasted just the code | |
| Ok(input.to_string()) | |
| if let Ok(parsed_url) = reqwest::Url::parse(input) { | |
| for (k, v) in parsed_url.query_pairs() { | |
| if k == "code" { | |
| return Ok(v.to_string()); | |
| } | |
| } | |
| // If a URL was pasted but no 'code' was found, it's an error. | |
| return Err("Pasted URL does not contain an authorization code.".to_string()); | |
| } | |
| // Otherwise, assume they pasted just the code | |
| Ok(input.to_string()) |
googleworkspace-bot
added
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a --no-localhost flag for an out-of-band authentication flow, which is a great feature for environments without a local browser. The implementation is mostly solid, but I've found a potential bug in how pasted URLs are handled when retrieving the authorization code. My review includes a suggestion to make this logic more robust.
src/auth_commands.rs
Outdated
| // If they pasted a full URL (e.g. http://localhost/?code=4/0Aea...&scope=...) | ||
| if let Ok(parsed_url) = reqwest::Url::parse(input) { | ||
| for (k, v) in parsed_url.query_pairs() { | ||
| if k == "code" { | ||
| return Ok(v.to_string()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, assume they pasted just the code | ||
| Ok(input.to_string()) |
There was a problem hiding this comment.
The current logic for parsing the authorization code from a pasted URL can lead to silent failures. If a user pastes a valid URL that does not contain a code query parameter (e.g., the URL of the consent screen itself), the code will fall through and incorrectly treat the entire URL string as the authorization code. This will cause the authentication to fail downstream with a potentially confusing error.
A more robust approach is to check if the input looks like an HTTP URL. If it does, we should strictly expect to find a code parameter. If no code is found, we should return an error rather than falling through. This provides clearer feedback to the user if they paste the wrong URL.
| // If they pasted a full URL (e.g. http://localhost/?code=4/0Aea...&scope=...) | |
| if let Ok(parsed_url) = reqwest::Url::parse(input) { | |
| for (k, v) in parsed_url.query_pairs() { | |
| if k == "code" { | |
| return Ok(v.to_string()); | |
| } | |
| } | |
| } | |
| // Otherwise, assume they pasted just the code | |
| Ok(input.to_string()) | |
| // If they pasted a full URL (e.g. http://localhost/?code=4/0Aea...&scope=...) | |
| if input.starts_with("http") { | |
| if let Ok(parsed_url) = reqwest::Url::parse(input) { | |
| if let Some(code) = parsed_url.query_pairs().find_map(|(k, v)| if k == "code" { Some(v) } else { None }) { | |
| return Ok(code.into_owned()); | |
| } | |
| } | |
| // It looked like a URL, but we couldn't parse it or find a code. | |
| return Err("The pasted URL is invalid or does not contain a 'code' parameter.".to_string()); | |
| } | |
| // Otherwise, assume they pasted just the code | |
| Ok(input.to_string()) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
==========================================
+ Coverage 57.69% 58.03% +0.34%
==========================================
Files 38 38
Lines 14328 14418 +90
==========================================
+ Hits 8267 8368 +101
+ Misses 6061 6050 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Extracted the URL parsing logic for the out-of-band `--no-localhost` OAuth flow into a separate helper `extract_code_from_input` and added tests to prevent coverage drop.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --no-localhost flag for the gws auth login command, enabling an out-of-band authentication flow. This is a valuable feature for users in environments without a local browser, such as SSH sessions or containers. The implementation correctly uses yup_oauth2's interactive flow. However, I've found a critical issue where a blocking I/O call is made within an async context, which could cause the application to hang. I've provided a suggestion to resolve this by using tokio::task::spawn_blocking.
| let mut user_input = String::new(); | ||
| std::io::stdin() | ||
| .read_line(&mut user_input) | ||
| .map_err(|e| format!("Failed to read code: {e}"))?; |
There was a problem hiding this comment.
The use of std::io::stdin().read_line() within an async block is a blocking operation. This can block the Tokio runtime's worker thread, preventing other asynchronous tasks from running and potentially causing your application to hang, especially if the user takes a long time to provide input.
To fix this, you should perform the blocking I/O operation on a dedicated thread pool using tokio::task::spawn_blocking.
| let mut user_input = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut user_input) | |
| .map_err(|e| format!("Failed to read code: {e}"))?; | |
| let user_input = tokio::task::spawn_blocking(|| { | |
| let mut input = String::new(); | |
| std::io::stdin().read_line(&mut input).map(|_| input) | |
| }) | |
| .await | |
| .map_err(|e| format!("Task join error: {e}"))? | |
| .map_err(|e| format!("Failed to read code: {e}"))?; |
googleworkspace-bot
added
the
gemini: reviewed
Extracted the argument parsing logic from `handle_login` into a new `parse_login_args` helper and added unit tests to cover the extraction of `--account`, `--services`, and the new `--no-localhost` flag. This resolves the Codecov coverage regression.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --no-localhost flag for an out-of-band authentication flow, which is a valuable feature for headless environments. The implementation correctly leverages yup_oauth2's interactive flow and includes a refactoring of argument parsing for better code structure. The addition of tests for the new logic is also a good practice. I have identified one high-severity issue concerning a blocking I/O call within an async function and have provided a code suggestion to resolve it.
| let mut user_input = String::new(); | ||
| std::io::stdin() | ||
| .read_line(&mut user_input) | ||
| .map_err(|e| format!("Failed to read code: {e}"))?; |
There was a problem hiding this comment.
The use of std::io::stdin().read_line() within an async block is a blocking operation that can starve the Tokio runtime executor, preventing other asynchronous tasks from running. This can lead to performance degradation or even deadlocks in the application.
To fix this, you should perform the blocking I/O operation on a dedicated blocking thread using tokio::task::spawn_blocking.
| let mut user_input = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut user_input) | |
| .map_err(|e| format!("Failed to read code: {e}"))?; | |
| let user_input = tokio::task::spawn_blocking(|| { | |
| let mut input = String::new(); | |
| std::io::stdin().read_line(&mut input).map(|_| input) | |
| }) | |
| .await | |
| .map_err(|e| format!("Task join error: {e}"))? | |
| .map_err(|e| format!("Failed to read code: {e}"))?; |
googleworkspace-bot
added
the
gemini: reviewed
Resolved CI formatting checks by running `cargo fmt` after adding the new unit tests for `parse_login_args`.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --no-localhost flag for the gws auth login command, enabling an out-of-band authentication flow for environments without a local web server. The implementation correctly refactors argument parsing, adjusts the OAuth flow based on the new flag, and adds corresponding tests. My review identifies one high-severity issue related to using blocking I/O within an async context, which is against async programming best practices. I've provided details and suggested solutions in the specific comment.
| std::io::stdin() | ||
| .read_line(&mut user_input) | ||
| .map_err(|e| format!("Failed to read code: {e}"))?; |
There was a problem hiding this comment.
Using std::io::stdin() for I/O within an async block is a blocking operation. This can block the current thread and prevent other asynchronous tasks from running, potentially leading to performance issues or deadlocks in more complex applications.
While this might not cause a noticeable problem in the current context of a simple CLI prompt, it is a good practice to use non-blocking alternatives in async code.
Consider one of the following approaches:
- Wrap the blocking call in
tokio::task::spawn_blocking. This moves the operation to a dedicated thread pool. - Use
tokio::io::stdin()along withtokio::io::AsyncBufReadExtto perform the read asynchronously.
Both solutions would require adding the necessary use statements for tokio components.
googleworkspace-bot
added
the
gemini: reviewed
|
Consolidating this thread into the central discussion for the auth UX/user journey work: #245 |
4 participants
This PR adds a
--no-localhostflag to thegws auth logincommand.When working in environments like headless servers, containers, or SSH sessions where a localhost redirect callback cannot be reached from the browser, users need a way to authenticate using an out-of-band (OOB) flow. This feature works similarly to
clasp login --no-localhost.Changes:
--no-localhosttogws auth loginoptions.--no-localhostis specified,yup_oauth2is configured to useInstalledFlowReturnMethod::Interactiveand theurn:ietf:wg:oauth:2.0:oobredirect URI, prompting the user to paste the authorization code into the terminal after granting consent in their browser.PR created automatically by Jules for task 14183664837181903487