fix(executor): skip x-goog-user-project header for OAuth auth method#827
Conversation
🦋 Changeset detectedLatest commit: edc245f 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 the 'x-goog-user-project' header was being sent unconditionally, causing 403 permission errors for OAuth users who are not IAM members of the specified project. By restricting this header to non-OAuth authentication methods, the CLI now correctly handles project-level quota attribution without violating IAM constraints. 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 skip sending the x-goog-user-project header during OAuth authentication to prevent 403 errors for non-project-member users, and adds a unit test to verify this behavior. However, feedback indicates that both user OAuth and Service Account/ADC authentication paths utilize AuthMethod::OAuth to attach bearer tokens. Consequently, the check *auth_method != AuthMethod::OAuth will incorrectly prevent the header from being sent for ADC or service account authentication as well, breaking the intended behavior.
…ta header AuthMethod::OAuth covers both user OAuth and service-account credentials, so the previous check (*auth_method != AuthMethod::OAuth) would incorrectly suppress the x-goog-user-project header for service accounts (which do need it) while also setting it on unauthenticated (None) requests. Add AuthMethod::ServiceAccount and auth::CredentialKind so the call site in main.rs can tag the request with the right variant. The quota header is now only sent for ServiceAccount auth; user OAuth requests remain header-free to avoid 403 errors for users who are not IAM members of the project.
googleworkspace-bot
added
area: auth
area: core
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request prevents 403 errors for non-project-member users by skipping the x-goog-user-project header during OAuth authentication. It introduces a CredentialKind enum to track the credential type and updates the request builder to only attach the header for service-account credentials. The review feedback suggests a valuable refactoring to eliminate redundancy: instead of introducing CredentialKind, the existing AuthMethod enum should be moved to auth.rs, re-exported in executor.rs, and returned directly by get_token_with_kind. This change would simplify the codebase and remove unnecessary boilerplate mapping in main.rs.
Replace the separate CredentialKind enum with the existing AuthMethod enum (moved from executor.rs to auth.rs so authentication types live with authentication code). get_token_with_kind now returns AuthMethod directly, eliminating the mapping boilerplate in main.rs. Resolves Gemini r2 comment on PR googleworkspace#827.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the CLI to skip sending the "x-goog-user-project" header for OAuth authentication, preventing 403 errors for users who are not IAM members of the project, while retaining it for service-account authentication. To support this, an "AuthMethod" enum and a "get_token_with_kind" function are introduced. The review feedback highlights that completely omitting this header for OAuth requests breaks the documented behavior of the "GOOGLE_WORKSPACE_PROJECT_ID" environment variable, and suggests allowing explicit overrides via this variable.
…icitly set The previous fix completely omitted x-goog-user-project for OAuth, which broke the documented GOOGLE_WORKSPACE_PROJECT_ID env var behaviour. Users who explicitly set that variable expect the header to be forwarded. New behaviour: - ServiceAccount: always send the header (env var, config, or ADC) - OAuth: only send when GOOGLE_WORKSPACE_PROJECT_ID is explicitly set - None: never send Also serialise the two env-var-mutating tests with a static Mutex to avoid races when the test binary runs threads in parallel.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Google Workspace CLI to skip the x-goog-user-project quota header by default during OAuth authentication, preventing 403 errors for users who are not members of the project. It introduces an AuthMethod enum to track the authentication type and adds get_token_with_kind to retrieve both the token and its authentication method. The feedback suggests refactoring get_token to delegate to get_token_with_kind to eliminate code duplication. Additionally, it is recommended to replace the local mutex in the new tests with #[serial_test::serial] to avoid flaky test runs caused by concurrent environment variable modifications across different test files.
|
|
||
| /// Like [`get_token`] but also returns the [`AuthMethod`] so callers can | ||
| /// decide whether to include the `x-goog-user-project` quota header. | ||
| pub async fn get_token_with_kind(scopes: &[&str]) -> anyhow::Result<(String, AuthMethod)> { |
There was a problem hiding this comment.
The implementation of get_token_with_kind duplicates the entire token loading and fetching logic of get_token. To improve maintainability and avoid future divergence, get_token should be refactored to delegate to get_token_with_kind:
pub async fn get_token(scopes: &[&str]) -> anyhow::Result<String> {
get_token_with_kind(scopes).await.map(|(token, _)| token)
}| /// Mutex to serialise tests that mutate GOOGLE_WORKSPACE_PROJECT_ID so they | ||
| /// don't race with each other when the test binary runs its threads in parallel. | ||
| static QUOTA_PROJECT_ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); | ||
|
|
||
| #[tokio::test] | ||
| async fn test_oauth_auth_does_not_set_quota_project_header_by_default() { | ||
| let _guard = QUOTA_PROJECT_ENV_MUTEX.lock().unwrap(); |
There was a problem hiding this comment.
Using a local QUOTA_PROJECT_ENV_MUTEX only serializes tests within executor.rs. However, tests in auth.rs (such as test_get_quota_project_priority_env_var) also mutate and read the process-wide GOOGLE_WORKSPACE_PROJECT_ID environment variable and are marked with #[serial_test::serial]. Because serial_test only serializes tests marked with #[serial], these tests will run in parallel with the auth.rs tests, leading to flaky test failures due to concurrent environment variable modification.
To prevent this, remove the local mutex and use #[serial_test::serial] on both tests to ensure global serialization across all tests in the test suite.
#[tokio::test]
#[serial_test::serial]
async fn test_oauth_auth_does_not_set_quota_project_header_by_default() {| #[tokio::test] | ||
| async fn test_oauth_auth_sends_quota_project_when_env_var_explicitly_set() { | ||
| let _guard = QUOTA_PROJECT_ENV_MUTEX.lock().unwrap(); |
There was a problem hiding this comment.
Use #[serial_test::serial] to serialize this test globally and remove the local mutex guard.
| #[tokio::test] | |
| async fn test_oauth_auth_sends_quota_project_when_env_var_explicitly_set() { | |
| let _guard = QUOTA_PROJECT_ENV_MUTEX.lock().unwrap(); | |
| #[tokio::test] | |
| #[serial_test::serial] | |
| async fn test_oauth_auth_sends_quota_project_when_env_var_explicitly_set() { |
2 participants
Summary
The
x-goog-user-projectheader was sent unconditionally. Whenproject_idis present inclient_secret.json, this triggers IAM checks, causing 403 errors for OAuth users who are not IAM members of that project.Fix: only send the header when
auth_method != AuthMethod::OAuth.Fixes #729
Checklist
cargo fmt --allpassedcargo clippy -- -D warningspassedcargo testpassed