fix(executor): skip x-goog-user-project header for OAuth auth method#863
fix(executor): skip x-goog-user-project header for OAuth auth method#863mateusmaaia wants to merge 5 commits into
Conversation
…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.
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.
…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.
🦋 Changeset detectedLatest commit: 943f8ad 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 |
googleworkspace-bot
added
area: auth
area: http
area: core
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 a long-standing issue where the CLI would trigger 403 permission errors by unconditionally sending the 'x-goog-user-project' header during OAuth-authenticated requests. By defaulting to omitting this header for user-based OAuth and providing an opt-in mechanism for power users, the change resolves authentication failures across multiple Google Workspace services while preserving necessary functionality for service accounts. 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
|
|
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. |
![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 modifies the CLI to skip the x-goog-user-project header during OAuth authentication unless the GOOGLE_WORKSPACE_PROJECT_ID environment variable is explicitly set, preventing 403 errors for non-project-member users. The feedback highlights a potential race condition in the newly added tests; using a local mutex instead of the #[serial_test::serial] attribute can lead to flaky test failures when running concurrently with other environment-mutating tests in the workspace.
| /// 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. | ||
| #[cfg(test)] | ||
| 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.
The tests in auth.rs (which also mutate environment variables like GOOGLE_WORKSPACE_PROJECT_ID) are annotated with #[serial_test::serial]. Since serial_test uses its own global static lock to serialize tests, any test not annotated with #[serial] can run concurrently with them. By using a local QUOTA_PROJECT_ENV_MUTEX instead of #[serial_test::serial], these new tests in executor.rs can run in parallel with the auth.rs tests, leading to environment variable races and flaky test failures in CI. Using #[serial_test::serial] ensures all environment-mutating tests across the workspace are properly serialized.
#[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.
Replace the local mutex guard with #[serial_test::serial] to ensure this test is properly serialized with other environment-mutating tests in the workspace (such as those in auth.rs), preventing flaky test runs.
| #[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() { |
- QUOTA_PROJECT_ENV_MUTEX is only referenced from #[tokio::test] functions, which aren't compiled outside test builds, so plain `cargo clippy --workspace` (CI's lint job) flagged it as dead code. - cargo fmt line-wrap on the new test's assert_eq! chain.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the Google Workspace CLI to conditionally omit the x-goog-user-project header during OAuth authentication unless the GOOGLE_WORKSPACE_PROJECT_ID environment variable is explicitly set, resolving 403 errors for non-project-member users. It introduces an AuthMethod enum and a get_token_with_kind function to track the authentication type. The feedback recommends replacing the local QUOTA_PROJECT_ENV_MUTEX in the new tests with the #[serial_test::serial] attribute, as other test files also mutate the same environment variable and could cause flaky test runs due to parallel execution.
| /// 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. | ||
| #[cfg(test)] | ||
| static QUOTA_PROJECT_ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); |
There was a problem hiding this comment.
The 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 the shared GOOGLE_WORKSPACE_PROJECT_ID environment variable. Because they do not share this mutex, they can still run in parallel and race, leading to flaky test failures.
Since the serial_test crate is already a dependency and used in auth.rs, we should use #[serial_test::serial] on these tests instead and remove this local mutex entirely.
| #[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.
Use #[serial_test::serial] to serialize this test with other tests in the crate that mutate environment variables, and remove the local mutex guard.
| #[tokio::test] | |
| async fn test_oauth_auth_does_not_set_quota_project_header_by_default() { | |
| let _guard = QUOTA_PROJECT_ENV_MUTEX.lock().unwrap(); | |
| #[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 with other tests in the crate that mutate environment variables, 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() { |
3 participants
Summary
Picks up the fix from #827 (by @nuthalapativarun), which was auto-closed by the stale-bot after 72 hours of inactivity — not because it was rejected. The PR had already passed CI (fmt, clippy, tests) and been through several rounds of review addressing
gemini-code-assistfeedback.Root cause (from #827/#729): the CLI unconditionally sends the
x-goog-user-projectheader (fromproject_idinclient_secret.json) on every request. For OAuth desktop-app credentials, this triggers a GCP IAM check (serviceusage.services.use) that fails for any user who isn't an IAM member of the project backing the shared OAuth client — a common setup when an OAuth client is distributed org-wide. The API then returns a permission error that, depending on the service, surfaces as403 insufficientPermissions: Request had insufficient authentication scopeseven though the token's actual scopes are correct.New evidence this affects more than Drive:
admin-reports(comment from @snacsnoc, same error, confirmed fixed by removing the header).calendar.events.listwhile debugging aperf-tracker-style tool that shells out togws: every raw<service> <resource> <method>call tocalendar.events.listfailed with the exact same error, regardless ofcalendarIdor date range, immediately after a fresh interactive OAuth consent that included thecalendarscope — while the+agendahelper (which never sendsx-goog-user-project) succeeded on the identical account and data. Filed as Bug: raw calendar.events.list fails with insufficient scopes; +agenda helper succeeds with the same account #861 with the isolated repro.Fix (unchanged from #827, plus two small follow-ups)
ServiceAccount: always forward the quota project (env var, config, or ADC) — service accounts need it and are project members by construction.OAuth: only sendx-goog-user-projectwhenGOOGLE_WORKSPACE_PROJECT_IDis explicitly set (opt-in) — otherwise omit it entirely, since OAuth users may not be IAM members of the client's backing project.None: never send it.On top of #827's four commits, I added:
#[cfg(test)]onQUOTA_PROJECT_ENV_MUTEX— it's only referenced from#[tokio::test]functions, which aren't compiled outside test builds, so the plaincargo clippy --workspace(CI's lint job, no--tests/--all-targets) flagged it as dead code.cargo fmtpass on the new test file'sassert_eq!line wrap.Verification
cargo test -p google-workspace-cli: 703 passed, 0 failed (includes the two regression tests from fix(executor): skip x-goog-user-project header for OAuth auth method #827 covering both the default-omit and explicit-opt-in cases).cargo fmt --all -- --check: clean.cargo clippy --workspace -- -D warnings(CI's exact lint command): clean except for one pre-existing, unrelatedcollapsible_matchwarning inhelpers/script.rsthat I confirmed also fails on a freshorigin/maincheckout without this branch — not introduced by this change.origin/main(no rebase conflicts).calendar.events.listwith this exact error — it now succeeds consistently, including with the multi-weekmaxResults/orderByparams a real caller would use.Fixes #729
Fixes #861
Related to the
admin-reportscase reported in #729 (comment) (@snacsnoc)Co-authored-by: Varun Nuthalapati nuthalapativarun@gmail.com