fix(executor): skip x-goog-user-project header for OAuth auth method#827
Conversation
🦋 Changeset detectedLatest commit: be15ebc 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.
| // Only send quota project for ADC/service-account auth; OAuth users are not | ||
| // necessarily IAM members of the project, so the header causes 403 errors. | ||
| if *auth_method != AuthMethod::OAuth { | ||
| if let Some(quota_project) = crate::auth::get_quota_project() { | ||
| request = request.header("x-goog-user-project", quota_project); | ||
| } | ||
| } |
There was a problem hiding this comment.
The check *auth_method != AuthMethod::OAuth will evaluate to false for both user OAuth and Service Account/ADC authentication, because AuthMethod only has two variants (OAuth and None), and both authentication paths use AuthMethod::OAuth to attach the bearer token. As a result, the x-goog-user-project header will never be sent for ADC or service account authentication, which contradicts the comment and breaks the intended behavior.
To fix this, we should refine the check locally within executor.rs rather than refactoring the broader auth.rs module, which would introduce changes outside the primary goal of this pull request and cause scope creep.
// Set quota project from ADC for billing/quota attribution
if let Some(quota_project) = crate::auth::get_quota_project() {
request = request.header("x-goog-user-project", quota_project);
}References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
There was a problem hiding this comment.
Good catch — the != AuthMethod::OAuth\ check was indeed inverted (it only matched \AuthMethod::None, the unauthenticated path, which is the opposite of what we wanted).
Rather than reverting to always sending the header, I've fixed this properly in 52cff70 by introducing a distinct \AuthMethod::ServiceAccount\ variant:
- **\�uth.rs**: Added a public \CredentialKind\ enum and \get_token_with_kind()\ that returns the token alongside whether it came from a user credential or a service-account key.
- **\executor.rs**: Added \AuthMethod::ServiceAccount; the bearer-auth block now covers both \OAuth\ and \ServiceAccount, and the quota header is sent only for \ServiceAccount.
- **\main.rs**: Updated the auth call to use \get_token_with_kind\ and map the credential kind to the right \AuthMethod\ variant.
This ensures service-account callers still get the \x-goog-user-project\ header while user OAuth callers do not, which is the correct behavior described in the PR.
There was a problem hiding this comment.
Fixed in be15ebc. The original check was inverted — AuthMethod::OAuth covered both user OAuth and service-account paths. The fix introduces a separate AuthMethod::ServiceAccount variant so the quota header is sent only for service-account credentials, not user OAuth.
…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.
| /// What kind of credential provided the token. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub enum CredentialKind { | ||
| /// Browser-based OAuth 2.0 user credential (from `gws auth login`) | ||
| UserOAuth, | ||
| /// Service-account key credential | ||
| ServiceAccount, | ||
| } |
There was a problem hiding this comment.
Defining a separate CredentialKind enum is redundant because we already have AuthMethod in executor.rs (which represents the exact same concepts: OAuth and ServiceAccount).
Instead of creating a new enum and mapping it in main.rs, we can define AuthMethod here in auth.rs (which is a more appropriate module for authentication-related types) and re-export it in executor.rs. This simplifies the codebase and removes unnecessary boilerplate.
| /// What kind of credential provided the token. | |
| #[derive(Debug, Clone, PartialEq)] | |
| pub enum CredentialKind { | |
| /// Browser-based OAuth 2.0 user credential (from `gws auth login`) | |
| UserOAuth, | |
| /// Service-account key credential | |
| ServiceAccount, | |
| } | |
| /// Tracks what authentication method was used for the request. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |
| pub enum AuthMethod { | |
| /// OAuth2 bearer token from a user credential (`gws auth login`) | |
| OAuth, | |
| /// Bearer token from a service-account key | |
| ServiceAccount, | |
| /// No authentication was provided | |
| None, | |
| } |
There was a problem hiding this comment.
Fixed in be15ebc. Replaced CredentialKind with AuthMethod directly in auth.rs (adding Copy and Eq as suggested). The AuthMethod enum now lives alongside the credential loading code where it belongs.
| /// Like [`get_token`] but also returns the [`CredentialKind`] 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, CredentialKind)> { | ||
| if let Ok(token) = std::env::var("GOOGLE_WORKSPACE_CLI_TOKEN") { | ||
| if !token.is_empty() { | ||
| return Ok((token, CredentialKind::UserOAuth)); | ||
| } | ||
| } | ||
|
|
||
| let creds_file = std::env::var("GOOGLE_WORKSPACE_CLI_CREDENTIALS_FILE").ok(); | ||
| let config_dir = crate::auth_commands::config_dir(); | ||
| let enc_path = credential_store::encrypted_credentials_path(); | ||
| let default_path = config_dir.join("credentials.json"); | ||
| let token_cache = config_dir.join("token_cache.json"); | ||
|
|
||
| let creds = load_credentials_inner(creds_file.as_deref(), &enc_path, &default_path).await?; | ||
| let kind = match &creds { | ||
| Credential::ServiceAccount(_) => CredentialKind::ServiceAccount, | ||
| Credential::AuthorizedUser(_) => CredentialKind::UserOAuth, | ||
| }; | ||
| let token = get_token_inner(scopes, creds, &token_cache).await?; | ||
| Ok((token, kind)) | ||
| } |
There was a problem hiding this comment.
Update get_token_with_kind to return the unified AuthMethod enum instead of CredentialKind.
/// 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)> {
if let Ok(token) = std::env::var("GOOGLE_WORKSPACE_CLI_TOKEN") {
if !token.is_empty() {
return Ok((token, AuthMethod::OAuth));
}
}
let creds_file = std::env::var("GOOGLE_WORKSPACE_CLI_CREDENTIALS_FILE").ok();
let config_dir = crate::auth_commands::config_dir();
let enc_path = credential_store::encrypted_credentials_path();
let default_path = config_dir.join("credentials.json");
let token_cache = config_dir.join("token_cache.json");
let creds = load_credentials_inner(creds_file.as_deref(), &enc_path, &default_path).await?;
let kind = match &creds {
Credential::ServiceAccount(_) => AuthMethod::ServiceAccount,
Credential::AuthorizedUser(_) => AuthMethod::OAuth,
};
let token = get_token_inner(scopes, creds, &token_cache).await?;
Ok((token, kind))
}There was a problem hiding this comment.
Fixed in be15ebc. get_token_with_kind now returns (String, AuthMethod) directly, mapping Credential::ServiceAccount to AuthMethod::ServiceAccount and Credential::AuthorizedUser to AuthMethod::OAuth.
| /// Tracks what authentication method was used for the request. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub enum AuthMethod { | ||
| /// OAuth2 bearer token from credentials file | ||
| /// OAuth2 bearer token from a user credential (`gws auth login`) | ||
| OAuth, | ||
| /// Bearer token from a service-account key | ||
| ServiceAccount, | ||
| /// No authentication was provided | ||
| None, | ||
| } |
There was a problem hiding this comment.
Re-export AuthMethod from crate::auth to avoid duplicating the enum definition and to maintain backward compatibility with existing imports of executor::AuthMethod.
| /// Tracks what authentication method was used for the request. | |
| #[derive(Debug, Clone, PartialEq)] | |
| pub enum AuthMethod { | |
| /// OAuth2 bearer token from credentials file | |
| /// OAuth2 bearer token from a user credential (`gws auth login`) | |
| OAuth, | |
| /// Bearer token from a service-account key | |
| ServiceAccount, | |
| /// No authentication was provided | |
| None, | |
| } | |
| pub use crate::auth::AuthMethod; |
There was a problem hiding this comment.
Fixed in be15ebc. The AuthMethod enum definition was removed from executor.rs and replaced with pub use crate::auth::AuthMethod, preserving backward compatibility for all existing imports.
| let (token, auth_method) = match auth::get_token_with_kind(&scopes).await { | ||
| Ok((t, kind)) => { | ||
| let method = match kind { | ||
| auth::CredentialKind::ServiceAccount => executor::AuthMethod::ServiceAccount, | ||
| auth::CredentialKind::UserOAuth => executor::AuthMethod::OAuth, | ||
| }; | ||
| (Some(t), method) | ||
| } |
There was a problem hiding this comment.
Since get_token_with_kind now directly returns AuthMethod, we can completely eliminate the nested match statement and the boilerplate mapping code.
let (token, auth_method) = match auth::get_token_with_kind(&scopes).await {
Ok((t, method)) => (Some(t), method),There was a problem hiding this comment.
Fixed in be15ebc. main.rs now does: let (token, auth_method) = match auth::get_token_with_kind(&scopes).await { Ok((t, method)) => (Some(t), method), ... } — the nested mapping boilerplate is gone.
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.
| // Only send quota project for service-account auth; OAuth users may not be | ||
| // IAM members of the project, so the header would trigger 403 errors. | ||
| if *auth_method == AuthMethod::ServiceAccount { | ||
| if let Some(quota_project) = crate::auth::get_quota_project() { | ||
| request = request.header("x-goog-user-project", quota_project); | ||
| } | ||
| } |
There was a problem hiding this comment.
By completely skipping the x-goog-user-project header for all OAuth requests, we break the documented behavior of the GOOGLE_WORKSPACE_PROJECT_ID environment variable. If a user explicitly sets GOOGLE_WORKSPACE_PROJECT_ID to override the quota/billing project, they expect it to be sent even when using OAuth.
We should only skip the automatic project ID from client_secret.json for OAuth users, but still allow explicit overrides via GOOGLE_WORKSPACE_PROJECT_ID.
// Only send quota project for service-account auth, or if explicitly overridden via env var for OAuth.
// OAuth users may not be IAM members of the project in client_secret.json, so sending it unconditionally triggers 403 errors.
let quota_project = if *auth_method == AuthMethod::ServiceAccount {
crate::auth::get_quota_project()
} else if *auth_method == AuthMethod::OAuth {
std::env::var("GOOGLE_WORKSPACE_PROJECT_ID").ok().filter(|s| !s.is_empty())
} else {
None
};
if let Some(quota_project) = quota_project {
request = request.header("x-goog-user-project", quota_project);
}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