fix(executor): skip x-goog-user-project header for OAuth auth method#870
fix(executor): skip x-goog-user-project header for OAuth auth method#870mateusmaaia wants to merge 8 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.
- 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.
…w feedback Replaces the local QUOTA_PROJECT_ENV_MUTEX with the #[serial_test::serial] attribute already used elsewhere in this crate for serialising tests that mutate GOOGLE_WORKSPACE_PROJECT_ID, per gemini-code-assist's review comment on PR googleworkspace#827/googleworkspace#863. The two test fns are gated with #[cfg(test)] so plain `cargo clippy --workspace` (which doesn't set cfg(test)) doesn't try to link the serial_test dev-dependency.
🦋 Changeset detectedLatest commit: a8b854d 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 an issue where the CLI's unconditional inclusion of the 'x-goog-user-project' header caused authentication failures for OAuth users. By making this header optional for OAuth users—unless explicitly opted into via 'GOOGLE_WORKSPACE_PROJECT_ID'—the changes ensure better compatibility with shared OAuth clients while maintaining necessary quota attribution 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
|
![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 skips the x-goog-user-project header for OAuth authentication by default to prevent 403 errors for non-project-member users, while still forwarding it for service accounts or when explicitly requested via the GOOGLE_WORKSPACE_PROJECT_ID environment variable. The feedback suggests refactoring get_token to call the new get_token_with_kind function to eliminate code duplication, and using an RAII guard in the unit tests to prevent environment variable pollution in case of test panics.
…var test cleanup - get_token now delegates to get_token_with_kind and discards the AuthMethod, removing the near-duplicate credential-loading logic between the two functions (gemini-code-assist review on googleworkspace#870). - The two executor tests that mutate GOOGLE_WORKSPACE_PROJECT_ID now use an RAII EnvVarGuard (mirroring auth.rs's test helper) so the env var is restored even if an assertion panics mid-test.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to prevent 403 errors for non-project-member users by skipping the x-goog-user-project header during OAuth authentication, unless the GOOGLE_WORKSPACE_PROJECT_ID environment variable is explicitly set. It introduces an AuthMethod enum to track the authentication type and updates token retrieval and request building logic accordingly. The review feedback highlights a potential issue where service accounts might incorrectly use the OAuth client's project ID from client_secret.json as the quota project, which can also lead to 403 errors. The reviewer suggests introducing a helper function get_quota_project_for_method to properly resolve the quota project based on the authentication method.
…or service accounts get_quota_project() previously fell back to project_id from the OAuth client configuration (client_secret.json) for service-account requests. That project isn't necessarily one the service account is an IAM member of, so forwarding it as x-goog-user-project reintroduces the same class of 403 insufficientPermissions error this PR fixes for OAuth. Replaces get_quota_project() with get_quota_project_for_method(AuthMethod), which only consults GOOGLE_WORKSPACE_PROJECT_ID or ADC's quota_project_id for service accounts, and never the OAuth client config. executor.rs now calls this single helper instead of duplicating the OAuth/ServiceAccount branching inline (gemini-code-assist review on googleworkspace#870).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the CLI to conditionally set the x-goog-user-project header based on the authentication method used, preventing 403 errors for OAuth users who are not IAM members of the project. It introduces an AuthMethod enum, refactors token retrieval to return the authentication method, and updates the quota project resolution logic and request builder accordingly. Extensive unit tests have been added to verify the new behavior. There are no review comments to address, so I have no additional feedback to provide.
3 participants
Summary
Re-opens the work from #863 (which itself picked up #827, both auto-closed by the stale-bot after 72 hours of inactivity — not rejected). #863 couldn't be reopened via the GitHub API/UI, so this is a fresh PR from the same branch (
test-729-fix, now with one additional commit addressing review feedback).Root cause (from #729/#827/#861): 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 surfaces as403 insufficientPermissions: Request had insufficient authentication scopeseven though the token's actual scopes are correct. Confirmed to affect Drive,admin-reports, andcalendar.events.list.Fix
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 #863's commits, this adds:
QUOTA_PROJECT_ENV_MUTEXin the executor tests with#[serial_test::serial](the pattern already used elsewhere in this crate for tests that mutateGOOGLE_WORKSPACE_PROJECT_ID), pergemini-code-assist's outstanding review comment on fix(executor): skip x-goog-user-project header for OAuth auth method #863. The two test fns are gated with#[cfg(test)]so plaincargo clippy --workspace(no--tests) doesn't try to link theserial_testdev-dependency.Verification
cargo test -p google-workspace-cli: 701 passed, 2 failed (pre-existing ADC-path test failures unrelated to this change — reproduced independently on a cleanorigin/maincheckout).cargo fmt --all -- --check: clean.cargo clippy --workspace -- -D warnings: clean, except one pre-existing, unrelatedcollapsible_matchwarning inhelpers/script.rsthat also reproduces on a freshorigin/maincheckout without this branch.origin/main.Fixes #729
Fixes #861
Related to the
admin-reportscase reported in #729Supersedes #863
Co-authored-by: Varun Nuthalapati nuthalapativarun@gmail.com