-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(executor): skip x-goog-user-project header for OAuth auth method #863
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
943f8ad
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,94 @@ 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. | ||||||||||||||||||||||||||
| #[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(); | ||||||||||||||||||||||||||
|
Comment on lines
+2406
to
+2413
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. The tests in #[tokio::test]
#[serial_test::serial]
async fn test_oauth_auth_does_not_set_quota_project_header_by_default() {
Comment on lines
+2411
to
+2413
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
|
||||||||||||||||||||||||||
| // 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
+2451
to
+2453
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. Replace the local mutex guard with
Suggested change
Comment on lines
+2451
to
+2453
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 local
QUOTA_PROJECT_ENV_MUTEXonly serializes tests withinexecutor.rs. However, tests inauth.rs(such astest_get_quota_project_priority_env_var) also mutate the sharedGOOGLE_WORKSPACE_PROJECT_IDenvironment variable. Because they do not share this mutex, they can still run in parallel and race, leading to flaky test failures.Since the
serial_testcrate is already a dependency and used inauth.rs, we should use#[serial_test::serial]on these tests instead and remove this local mutex entirely.