From 6a5efcb1e7b7a269b54063c84991c71680260f81 Mon Sep 17 00:00:00 2001 From: Harshal Patil <12152047+harche@users.noreply.github.com> Date: Thu, 5 Mar 2026 11:26:26 -0500 Subject: [PATCH] Fix auth failures when accounts.json registry is missing Three related bugs caused all API calls to fail with "Access denied. No credentials provided" even after successful `gws auth login`: 1. resolve_account() rejected valid credentials.enc as "legacy" when accounts.json was absent, instead of falling through to use them. 2. main.rs silently swallowed all auth errors (Err(_) => None), masking real failures behind a generic "no credentials" message. 3. auth login didn't include openid/email scopes, so fetch_userinfo_email() couldn't identify the user, causing credentials to be saved without an accounts.json entry. {attribution.commit: ""} --- .changeset/fix-auth-legacy-credentials.md | 11 +++ src/auth.rs | 96 ++++++++++++++++++++--- src/auth_commands.rs | 31 +++++++- src/executor.rs | 38 +++++++++ src/main.rs | 14 +++- 5 files changed, 176 insertions(+), 14 deletions(-) create mode 100644 .changeset/fix-auth-legacy-credentials.md diff --git a/.changeset/fix-auth-legacy-credentials.md b/.changeset/fix-auth-legacy-credentials.md new file mode 100644 index 0000000..68d3c5b --- /dev/null +++ b/.changeset/fix-auth-legacy-credentials.md @@ -0,0 +1,11 @@ +--- +"@googleworkspace/cli": patch +--- + +Fix auth failures when accounts.json registry is missing + +Three related bugs caused all API calls to fail with "Access denied. No credentials provided" even after a successful `gws auth login`: + +1. `resolve_account()` rejected valid `credentials.enc` as "legacy" when `accounts.json` was absent, instead of using them. +2. `main.rs` silently swallowed all auth errors, masking real failures behind a generic message. +3. `auth login` didn't include `openid`/`email` scopes, so `fetch_userinfo_email()` couldn't identify the user, causing credentials to be saved without an `accounts.json` entry. diff --git a/src/auth.rs b/src/auth.rs index d895f26..3e473c3 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -131,18 +131,10 @@ fn resolve_account(account: Option<&str>) -> anyhow::Result> { ); } } - // No account, no registry — check for legacy credentials + // No account, no registry — use legacy credentials if they exist (None, None) => { - let legacy_path = credential_store::encrypted_credentials_path(); - if legacy_path.exists() { - anyhow::bail!( - "Legacy credentials found at {}. \ - gws now supports multiple accounts. \ - Please run 'gws auth login' to upgrade your credentials.", - legacy_path.display() - ); - } - // No registry, no legacy — fall through to standard credential loading + // Fall through to standard credential loading which will pick up + // the legacy credentials.enc file if it exists. Ok(None) } } @@ -425,6 +417,88 @@ mod tests { assert_eq!(result.unwrap(), "my-test-token"); } + #[test] + fn test_resolve_account_no_registry_no_account_returns_none() { + // When there is no accounts.json and no explicit account, + // resolve_account should return Ok(None) to allow legacy + // credentials.enc to be picked up by load_credentials_inner. + let result = resolve_account(None); + // This will return Ok(None) if accounts.json doesn't exist, + // or Ok(Some(...)) if it does with a default. Either way, it + // should NOT error for the no-registry case. + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_load_credentials_encrypted_file() { + // Simulate an encrypted credentials file + let json = r#"{ + "client_id": "enc_test_id", + "client_secret": "enc_test_secret", + "refresh_token": "enc_test_refresh", + "type": "authorized_user" + }"#; + + let dir = tempfile::tempdir().unwrap(); + let enc_path = dir.path().join("credentials.enc"); + + // Encrypt and write + let encrypted = crate::credential_store::encrypt(json.as_bytes()).unwrap(); + std::fs::write(&enc_path, &encrypted).unwrap(); + + let res = load_credentials_inner(None, &enc_path, &PathBuf::from("/does/not/exist")) + .await + .unwrap(); + + match res { + Credential::AuthorizedUser(secret) => { + assert_eq!(secret.client_id, "enc_test_id"); + assert_eq!(secret.client_secret, "enc_test_secret"); + assert_eq!(secret.refresh_token, "enc_test_refresh"); + } + _ => panic!("Expected AuthorizedUser from encrypted credentials"), + } + } + + #[tokio::test] + async fn test_load_credentials_encrypted_takes_priority_over_default() { + // Encrypted credentials should be loaded before the default plaintext path + let enc_json = r#"{ + "client_id": "encrypted_id", + "client_secret": "encrypted_secret", + "refresh_token": "encrypted_refresh", + "type": "authorized_user" + }"#; + let plain_json = r#"{ + "client_id": "plaintext_id", + "client_secret": "plaintext_secret", + "refresh_token": "plaintext_refresh", + "type": "authorized_user" + }"#; + + let dir = tempfile::tempdir().unwrap(); + let enc_path = dir.path().join("credentials.enc"); + let plain_path = dir.path().join("credentials.json"); + + let encrypted = crate::credential_store::encrypt(enc_json.as_bytes()).unwrap(); + std::fs::write(&enc_path, &encrypted).unwrap(); + std::fs::write(&plain_path, plain_json).unwrap(); + + let res = load_credentials_inner(None, &enc_path, &plain_path) + .await + .unwrap(); + + match res { + Credential::AuthorizedUser(secret) => { + assert_eq!( + secret.client_id, "encrypted_id", + "Encrypted credentials should take priority over plaintext" + ); + } + _ => panic!("Expected AuthorizedUser"), + } + } + #[tokio::test] #[serial_test::serial] async fn test_get_token_env_var_empty_falls_through() { diff --git a/src/auth_commands.rs b/src/auth_commands.rs index b5a5bb8..8f53df1 100644 --- a/src/auth_commands.rs +++ b/src/auth_commands.rs @@ -266,7 +266,7 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> { } // Determine scopes: explicit flags > interactive TUI > defaults - let scopes = resolve_scopes( + let mut scopes = resolve_scopes( &filtered_args, project_id.as_deref(), services_filter.as_ref(), @@ -311,6 +311,15 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> { .await .map_err(|e| GwsError::Auth(format!("Failed to build authenticator: {e}")))?; + // Ensure openid + email scopes are always present so we can identify the user + // via the userinfo endpoint after login. + let identity_scopes = ["openid", "https://www.googleapis.com/auth/userinfo.email"]; + for s in &identity_scopes { + if !scopes.iter().any(|existing| existing == s) { + scopes.push(s.to_string()); + } + } + // Request a token — this triggers the browser OAuth flow let scope_refs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect(); let token = auth @@ -2085,4 +2094,24 @@ mod tests { let result = filter_scopes_by_services(scopes.clone(), Some(&empty)); assert_eq!(result, scopes); } + + #[test] + fn mask_secret_long_string() { + let masked = mask_secret("GOCSPX-abcdefghijklmnopqrstuvwxyz"); + assert_eq!(masked, "GOCS...wxyz"); + } + + #[test] + fn mask_secret_short_string() { + // 8 chars or fewer should be fully masked + assert_eq!(mask_secret("12345678"), "***"); + assert_eq!(mask_secret("short"), "***"); + assert_eq!(mask_secret(""), "***"); + } + + #[test] + fn mask_secret_boundary() { + // Exactly 9 chars — first 4 + last 4 with "..." in between + assert_eq!(mask_secret("123456789"), "1234...6789"); + } } diff --git a/src/executor.rs b/src/executor.rs index 8bc4f02..bad03c4 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -1525,6 +1525,44 @@ mod tests { } } + #[test] + fn test_handle_error_response_401_with_oauth_does_not_mask_error() { + // When auth was attempted (OAuth) but the server still returns 401, + // the error should be an API error with the actual message, NOT + // the generic "Access denied. No credentials provided" message. + let json_err = json!({ + "error": { + "code": 401, + "message": "Request had invalid authentication credentials.", + "errors": [{ "reason": "authError" }] + } + }) + .to_string(); + + let err = handle_error_response::<()>( + reqwest::StatusCode::UNAUTHORIZED, + &json_err, + &AuthMethod::OAuth, + ) + .unwrap_err(); + match err { + GwsError::Api { + code, + message, + reason, + .. + } => { + assert_eq!(code, 401); + assert!(message.contains("invalid authentication credentials")); + assert_eq!(reason, "authError"); + } + GwsError::Auth(msg) => { + panic!("Should NOT get generic Auth error when OAuth was used, got: {msg}"); + } + other => panic!("Expected Api error, got: {other:?}"), + } + } + #[test] fn test_handle_error_response_api_error() { let json_err = json!({ diff --git a/src/main.rs b/src/main.rs index 4497731..e3373d3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -235,10 +235,20 @@ async fn run() -> Result<(), GwsError> { // Get scopes from the method let scopes: Vec<&str> = method.scopes.iter().map(|s| s.as_str()).collect(); - // Authenticate: try OAuth, otherwise proceed unauthenticated + // Authenticate: try OAuth, fail with error if credentials exist but are broken let (token, auth_method) = match auth::get_token(&scopes, account.as_deref()).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(e) => { + // If credentials were found but failed (e.g. decryption error, invalid token), + // propagate the error instead of silently falling back to unauthenticated. + // Only fall back to None if no credentials exist at all. + let err_msg = format!("{e:#}"); + if err_msg.starts_with("No credentials found") { + (None, executor::AuthMethod::None) + } else { + return Err(GwsError::Auth(format!("Authentication failed: {err_msg}"))); + } + } }; // Execute