feat(auth): add configuration profiles for account switching#289
feat(auth): add configuration profiles for account switching#289joeVenner wants to merge 33 commits intogoogleworkspace:mainfrom
Conversation
|
googleworkspace-bot
added
area: auth
area: core
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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 significantly enhances the Highlights
Changelog
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
|
![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 introduces configuration profiles, a great feature for managing multiple accounts. The implementation correctly isolates configurations in different directories and updates the OS keyring service name. However, I've found a few critical issues related to code duplication and incorrect logic for resolving the base configuration directory. This duplicated logic is present in three different places and is buggy, which can lead to inconsistent behavior and break backward compatibility for users with legacy configurations. Additionally, there's a critical bug in the command-line argument parsing in main.rs that breaks the --api-version flag. My review includes detailed comments on these issues with suggestions for how to fix them by centralizing the logic and correcting the parsing.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles, a great feature for managing multiple accounts. The implementation is mostly solid, but I've found a few areas for improvement.
Specifically, there are two critical path traversal vulnerabilities related to unsanitized profile names that need to be addressed. I've also identified significant code duplication for profile resolution logic and a complex argument parsing implementation that could be simplified. My comments provide detailed suggestions for fixing these issues.
Allows users to seamlessly switch between multiple Google Workspace accounts using the '--profile <name>' global flag or the 'gws auth switch <name>' command. This isolates configurations, token caches, and OS keyring storage per profile.
Resolves code review feedback: - Extracted 'base_config_dir()' to a single location to prevent buggy fallback deduplication across 'auth_commands.rs' and 'credential_store.rs'. - Fixed 'main.rs' argument filtering logic which incorrectly skipped the values of the '--api-version' flag.
joeVenner
force-pushed
the
feat/configuration-profiles
branch
from
da8f10e to
ceb9579
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles, which is a great feature for managing multiple accounts. The implementation looks solid, but I've found a few areas for improvement. There's some duplicated code for resolving the active profile that should be consolidated into a single function. I also noticed a critical issue in the new argument parsing logic in main.rs where arguments are processed multiple times, which is inefficient and hard to maintain. Finally, there's a minor error handling issue where a file removal failure could be silently ignored. Addressing these points will make the new feature more robust and maintainable.
Resolves code review feedback: - Sanitized profile names during parsing and 'auth switch' to prevent path traversals using '../'. - Added 'get_active_profile()' to deduplicate the logic of loading the active profile environment variable or fallback file. - Optimized arguments iteration in 'main.rs'.
|
/gemini review |
Resolves code review feedback: - Correctly handles and surfacing errors when attempting to delete the active_profile file, while ignoring harmless 'NotFound' errors.
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles, a great feature for managing multiple accounts. The implementation is solid, with good attention to security aspects like preventing path traversal in profile names. My review focuses on improving maintainability by reducing code duplication and enhancing robustness by handling potential silent failures. Specifically, I've pointed out duplicated logic for profile name validation and active profile retrieval, and a potential bug in profile switching where an error could be silently ignored. Addressing these points will make the new feature more robust and easier to maintain.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles, a valuable feature for managing multiple accounts. The implementation correctly isolates configuration files and keyring entries based on the active profile. My review has identified two instances of code duplication that should be addressed to improve maintainability and reduce risk. One is the logic for determining the active profile, and the other is the validation of profile names, which is security-sensitive.
Resolves code review feedback: - Extracted 'validate_profile_name' to a reusable function in 'auth_commands.rs' and applied it consistently. - Updated 'credential_store.rs' to rely on the centralized 'get_active_profile' function instead of duplicating environmental checks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles, a valuable feature for managing multiple accounts. The implementation is largely well-executed, with good isolation of profile-specific configurations and keyring entries. However, I've identified a critical path traversal vulnerability where a manually edited profile name in the active_profile file is not validated, potentially allowing access to unintended directories. My review includes a code suggestion to address this vulnerability by validating the profile name upon reading it.
Resolves code review feedback: - Ensures profile names read from the local 'active_profile' file are validated against path traversal vectors (e.g. '../'). - Fallbacks to the default profile if the user-modified input on disk is flagged as invalid.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles, allowing users to switch between different accounts. The implementation correctly sandboxes configuration files and keyring entries based on the active profile. The argument parsing and new auth switch subcommand appear to be well-implemented. My main concern is a critical security issue where an invalid profile name in the configuration file causes a silent fallback to the default profile, which could lead to actions being performed on the wrong account. I've suggested a change to make this a hard failure to ensure user intent is always respected.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles for account switching, a great feature for managing multiple accounts. The implementation is mostly solid, with changes to configuration directory handling, keyring isolation, and argument parsing. I've found a critical issue in the new get_active_profile function that will prevent the code from compiling and could cause unexpected crashes. I've also suggested a refactoring in main.rs to simplify the argument parsing logic for better maintainability. Please see the detailed comments below.
Resolves code review feedback: - Fixed a compilation error regarding a missing 'else' return branch in 'get_active_profile', and handled the error without using 'process::exit'. - Simplified the argument iteration loop in 'main.rs' to strip redundant '--api-version' checks since it is handled later by the subcommand filter.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new feature: configuration profiles for managing multiple accounts. The implementation is well-structured, sandboxing configurations and credentials effectively. A significant part of the codebase has been refactored to be asynchronous using tokio, which is a substantial and well-executed effort. My review focuses on ensuring the new asynchronous file operations are handled in a cross-platform and safe manner.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for managing multiple accounts through configuration profiles. The implementation correctly isolates configurations, credentials, and keyring entries per profile. The refactoring to support this by making file system operations asynchronous is extensive. However, I've found a couple of critical issues where blocking I/O operations remain within async functions, which could lead to performance degradation or deadlocks. Please see the specific comments for details.
…fs in discovery and token storage
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles for account switching, a significant and well-implemented feature. The changes include adding a global --profile flag, a new gws auth switch command, and sandboxing for configurations and credentials. The transition to asynchronous file I/O across the codebase to support this is extensive and has been handled consistently. I've identified two critical issues that should be addressed before merging.
…ndardize status JSON output
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for managing multiple accounts through configuration profiles. The implementation of path and keyring sandboxing is a solid approach. The accompanying refactoring to asynchronous file operations is extensive and mostly well-executed. However, I've identified a critical security vulnerability related to path handling and a few high-severity issues concerning the use of blocking operations within async contexts, which could lead to performance degradation. Addressing these points will significantly improve the security and robustness of the new functionality.
…and prevent keyring IO lock contention
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for managing multiple accounts through configuration profiles. The implementation is comprehensive, touching many parts of the authentication and configuration logic, and includes a significant migration to asynchronous file system operations. The security considerations for path handling and profile name validation are well-thought-out.
My review focuses on a few areas where code duplication and complexity could impact maintainability and potentially introduce bugs in the future, particularly in security-sensitive code paths. I've provided suggestions to refactor duplicated logic and simplify complex expressions to improve code clarity and robustness.
…n blocks, and consolidate keyring cache write-locks
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration profiles, a great feature for managing multiple accounts. The implementation is well-structured, with changes spanning argument parsing, configuration handling, and authentication flows. The code is mostly solid, with good attention to security in profile name validation and path handling. However, I've identified a critical Time-of-check to time-of-use (TOCTOU) vulnerability in how the custom configuration directory is handled, which could allow an attacker to write files to sensitive locations. My review includes a comment with a suggested fix for this vulnerability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for managing multiple accounts through configuration profiles. The implementation is comprehensive, touching on configuration paths, credential storage, and OS keyring isolation to ensure proper namespacing. The large-scale refactoring to make the application more asynchronous is a significant undertaking and has been handled well for the most part.
My review focuses on ensuring the correctness and safety of these new asynchronous patterns. I've identified a potential data race issue related to setting environment variables in a multi-threaded async context. While I've also noticed some remaining synchronous I/O calls within async functions, I'm unable to comment on them due to line-based review constraints. I recommend a thorough audit for any remaining std::fs calls in async functions to prevent blocking the runtime.
…rofile argument propagation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for managing multiple accounts through configuration profiles. The implementation is thorough, including sandboxing for configuration, credentials, and even OS keyring entries. The extensive refactoring to adopt async for I/O operations across the codebase is a significant undertaking and is handled well for the most part.
My review focuses on a couple of areas where the async implementation can be simplified and made more idiomatic using standard patterns from the tokio ecosystem, particularly for one-time initialization and setting file permissions. These changes will improve code clarity and maintainability.
…eCell for credential key initialization
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature for managing multiple accounts through configuration profiles. The implementation is well-structured, especially the security enhancements around path handling and the use of OnceCell for the async key generation.
My review focuses on a recurring critical issue: several synchronous file system and subprocess calls remain within async functions. These blocking calls can degrade performance and potentially lead to deadlocks in the async runtime. I've provided specific suggestions to replace them with their tokio async equivalents. Addressing these will ensure the async implementation is robust and efficient.
…scopes and harden test isolation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed configuration profiles feature, allowing users to seamlessly switch between multiple accounts. The implementation is robust, with thoughtful security considerations for path handling and credential isolation. The extensive refactoring to adopt asynchronous I/O across the codebase is a significant improvement for the CLI's responsiveness and has been executed thoroughly and correctly. I've reviewed the changes and found no critical issues.
|
All automated code review feedback has been successfully addressed and resolved. The branch is completely green, and we are ready for a final review from the code owner! Here is a quick summary of the fixes implemented during this refactoring phase:
The test suite has been updated (including stricter isolation for OS environment config tests) and all 426 tests are passing locally. Let me know if you need anything else before merging! |
2 participants
Title : feat(auth): support configuration profiles (account switching)
Description
This pull request implements "Configuration Profiles" for gwcli, allowing users to securely manage and seamlessly switch between multiple Google Workspace accounts. By default, the CLI continues to work out-of-the-box using the default profile. Users can now easily isolate their configurations, credentials, and OS keyring caches into dedicated namespaced directories.
Background & Motivation
First off, a massive thank you to the team and especially JP for building this amazing tool! gwcli has been incredible for my workflow — moving away from disjointed MCPs to a single, unified CLI has saved me a ton of context switching and overhead.
However, as I was integrating it into my custom AI agent, I ran into a limitation: I have multiple, distinctly important Google Workspace profiles that my agent needs to manage dynamically. Initially, I threw together a quick local tweak to hack the directory switching, but the more I used it, the more I realized this could be an absolute game-changer for other AI agent developers working with gwcli.
Instead of keeping the workaround to myself, I decided to build it out properly and contribute it back to the repo. This feature allows agents (and human users!) to seamlessly sandbox and switch between as many Workspace environments as they need, all via a simple --profile flag or persistent auth switch defaults.
Changes Included
Global CLI Argument (--profile ): Added a cross-cutting --profile argument to main.rs that overrides the active configuration profile for a specific command execution.
Persistent Profile Switching: Added the new gws auth switch subcommand to globally set the active configuration profile (persisted in ~/.config/gws/active_profile).
Path Sandboxing: Updated config_dir() to dynamically resolve paths based on the active profile, naturally isolating credentials.json, token_cache.json, and client_secret.json into ~/.config/gws/profiles//.
OS Keyring Isolation: Modified the get_or_create_key() function to use a dynamic service name (gws-cli-[profile]), ensuring the encryption key stored in the OS keyring does not clash across profiles.
Enhanced Status Display: Updated gws auth status to always display the active profile name, removing any ambiguity over which account is currently contextualized.
Testing & Validation