-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(executor): skip x-goog-user-project header for OAuth auth method #827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
abcb2fb
52cff70
be15ebc
edc245f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@googleworkspace/cli": patch | ||
| --- | ||
|
|
||
| Skip x-goog-user-project header for OAuth auth to fix 403 errors for non-project-member users |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -31,14 +31,7 @@ use crate::discovery::{RestDescription, RestMethod}; | |||||||||||||
| use crate::error::GwsError; | ||||||||||||||
| use crate::output::sanitize_for_terminal; | ||||||||||||||
|
|
||||||||||||||
| /// Tracks what authentication method was used for the request. | ||||||||||||||
| #[derive(Debug, Clone, PartialEq)] | ||||||||||||||
| pub enum AuthMethod { | ||||||||||||||
| /// OAuth2 bearer token from credentials file | ||||||||||||||
| OAuth, | ||||||||||||||
| /// No authentication was provided | ||||||||||||||
| None, | ||||||||||||||
| } | ||||||||||||||
| pub use crate::auth::AuthMethod; | ||||||||||||||
|
|
||||||||||||||
| /// Source for media upload content. | ||||||||||||||
| /// | ||||||||||||||
|
|
@@ -182,13 +175,23 @@ async fn build_http_request( | |||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| if let Some(token) = token { | ||||||||||||||
| if *auth_method == AuthMethod::OAuth { | ||||||||||||||
| if matches!(*auth_method, AuthMethod::OAuth | AuthMethod::ServiceAccount) { | ||||||||||||||
| request = request.bearer_auth(token); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Set quota project from ADC for billing/quota attribution | ||||||||||||||
| if let Some(quota_project) = crate::auth::get_quota_project() { | ||||||||||||||
| // For service-account auth, always forward the quota project (env var, config, or ADC). | ||||||||||||||
| // For OAuth, only send when GOOGLE_WORKSPACE_PROJECT_ID is explicitly set — the user | ||||||||||||||
| // has opted in, so we honour it even though OAuth users may not be IAM members of every | ||||||||||||||
| // project. Omit the header entirely when neither condition is met to avoid 403 errors. | ||||||||||||||
| let quota_project = match auth_method { | ||||||||||||||
| AuthMethod::ServiceAccount => crate::auth::get_quota_project(), | ||||||||||||||
| AuthMethod::OAuth => std::env::var("GOOGLE_WORKSPACE_PROJECT_ID") | ||||||||||||||
| .ok() | ||||||||||||||
| .filter(|s| !s.is_empty()), | ||||||||||||||
| AuthMethod::None => None, | ||||||||||||||
| }; | ||||||||||||||
| if let Some(quota_project) = quota_project { | ||||||||||||||
| request = request.header("x-goog-user-project", quota_project); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -2399,3 +2402,90 @@ async fn test_get_does_not_set_content_length_zero() { | |||||||||||||
| "GET with no body should not have Content-Length header" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// 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(); | ||||||||||||||
|
Comment on lines
+2406
to
+2412
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a local To prevent this, remove the local mutex and use #[tokio::test]
#[serial_test::serial]
async fn test_oauth_auth_does_not_set_quota_project_header_by_default() { |
||||||||||||||
| // Without GOOGLE_WORKSPACE_PROJECT_ID set, OAuth requests must omit x-goog-user-project | ||||||||||||||
| // because OAuth users are not necessarily IAM members of the project. | ||||||||||||||
| std::env::remove_var("GOOGLE_WORKSPACE_PROJECT_ID"); | ||||||||||||||
| let client = reqwest::Client::new(); | ||||||||||||||
| let method = RestMethod { | ||||||||||||||
| http_method: "GET".to_string(), | ||||||||||||||
| path: "files".to_string(), | ||||||||||||||
| ..Default::default() | ||||||||||||||
| }; | ||||||||||||||
| let input = ExecutionInput { | ||||||||||||||
| full_url: "https://example.com/files".to_string(), | ||||||||||||||
| body: None, | ||||||||||||||
| params: Map::new(), | ||||||||||||||
| query_params: Vec::new(), | ||||||||||||||
| is_upload: false, | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| let request = build_http_request( | ||||||||||||||
| &client, | ||||||||||||||
| &method, | ||||||||||||||
| &input, | ||||||||||||||
| Some("fake-token"), | ||||||||||||||
| &AuthMethod::OAuth, | ||||||||||||||
| None, | ||||||||||||||
| 0, | ||||||||||||||
| &None, | ||||||||||||||
| ) | ||||||||||||||
| .await | ||||||||||||||
| .unwrap(); | ||||||||||||||
|
|
||||||||||||||
| let built = request.build().unwrap(); | ||||||||||||||
| assert!( | ||||||||||||||
| built.headers().get("x-goog-user-project").is_none(), | ||||||||||||||
| "OAuth requests must not include x-goog-user-project header when env var is not set" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| #[tokio::test] | ||||||||||||||
| async fn test_oauth_auth_sends_quota_project_when_env_var_explicitly_set() { | ||||||||||||||
| let _guard = QUOTA_PROJECT_ENV_MUTEX.lock().unwrap(); | ||||||||||||||
|
Comment on lines
+2450
to
+2452
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Suggested change
|
||||||||||||||
| // When GOOGLE_WORKSPACE_PROJECT_ID is explicitly set, OAuth requests should | ||||||||||||||
| // honour it and send x-goog-user-project (the user opted in). | ||||||||||||||
| std::env::set_var("GOOGLE_WORKSPACE_PROJECT_ID", "my-explicit-project"); | ||||||||||||||
| let client = reqwest::Client::new(); | ||||||||||||||
| let method = RestMethod { | ||||||||||||||
| http_method: "GET".to_string(), | ||||||||||||||
| path: "files".to_string(), | ||||||||||||||
| ..Default::default() | ||||||||||||||
| }; | ||||||||||||||
| let input = ExecutionInput { | ||||||||||||||
| full_url: "https://example.com/files".to_string(), | ||||||||||||||
| body: None, | ||||||||||||||
| params: Map::new(), | ||||||||||||||
| query_params: Vec::new(), | ||||||||||||||
| is_upload: false, | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| let request = build_http_request( | ||||||||||||||
| &client, | ||||||||||||||
| &method, | ||||||||||||||
| &input, | ||||||||||||||
| Some("fake-token"), | ||||||||||||||
| &AuthMethod::OAuth, | ||||||||||||||
| None, | ||||||||||||||
| 0, | ||||||||||||||
| &None, | ||||||||||||||
| ) | ||||||||||||||
| .await | ||||||||||||||
| .unwrap(); | ||||||||||||||
|
|
||||||||||||||
| std::env::remove_var("GOOGLE_WORKSPACE_PROJECT_ID"); | ||||||||||||||
|
|
||||||||||||||
| let built = request.build().unwrap(); | ||||||||||||||
| assert_eq!( | ||||||||||||||
| built.headers().get("x-goog-user-project").and_then(|v| v.to_str().ok()), | ||||||||||||||
| Some("my-explicit-project"), | ||||||||||||||
| "OAuth requests must include x-goog-user-project when GOOGLE_WORKSPACE_PROJECT_ID is set" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of
get_token_with_kindduplicates the entire token loading and fetching logic ofget_token. To improve maintainability and avoid future divergence,get_tokenshould be refactored to delegate toget_token_with_kind: