From d272bd71a7f6bfa07cac0ecdbf6fc55498c296f0 Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Tue, 13 Oct 2026 12:08:46 +0200 Subject: [PATCH 1/9] feat(auth): allow OAuth device login against custom API targets Resolve the authorization server and client id from the API host being targeted: the built-in production and staging/dev hosts carry their own configuration, and any other host is configured at runtime via CLICKHOUSECTL_AUTH_URL and CLICKHOUSECTL_CLIENT_ID (both required, no fallback). This lets the `cloud auth login` device flow authenticate against additional targets without rebuilding the CLI. --- crates/clickhousectl/src/cloud/auth.rs | 96 +++++++++++++------ crates/clickhousectl/src/cloud/mod.rs | 18 ++-- .../tests/cli_request_shape_test.rs | 62 +++++++++++- 3 files changed, 136 insertions(+), 40 deletions(-) diff --git a/crates/clickhousectl/src/cloud/auth.rs b/crates/clickhousectl/src/cloud/auth.rs index 1993d378..fffee2ae 100644 --- a/crates/clickhousectl/src/cloud/auth.rs +++ b/crates/clickhousectl/src/cloud/auth.rs @@ -633,43 +633,63 @@ fn prompt_api_credentials() -> std::result::Result<(String, String), Box Option<&'static AuthConfig> { +fn auth_config_for_url(api_url: &str) -> Option { let parsed = url::Url::parse(api_url).ok()?; let host = parsed.host_str()?; - KNOWN_CONFIGS + + if let Some(config) = KNOWN_CONFIGS .iter() - .find(|(known_host, _)| host == *known_host) - .map(|(_, config)| config) + .find(|(known_host, _, _)| host == *known_host) + .map(|(_, auth_url, client_id)| AuthConfig { + auth_url: (*auth_url).to_string(), + client_id: (*client_id).to_string(), + }) + { + return Some(config); + } + + // Unknown host: require both env vars, with no built-in fallback. + let env = |key: &str| std::env::var(key).ok().filter(|v| !v.is_empty()); + match ( + env("CLICKHOUSECTL_AUTH_URL"), + env("CLICKHOUSECTL_CLIENT_ID"), + ) { + (Some(auth_url), Some(client_id)) => Some(AuthConfig { + // Trim trailing slashes so joining `/oauth/...` can't produce a + // rejected double slash (common when copying an issuer URL). + auth_url: auth_url.trim_end_matches('/').to_string(), + client_id, + }), + _ => None, + } } /// Normalize a user-provided URL into the API base URL we store and use. @@ -817,11 +837,12 @@ pub async fn device_auth_login(api_url: &str) -> Result>() .join(", ") ) @@ -832,7 +853,7 @@ pub async fn device_auth_login(api_url: &str) -> Result Result Result Result> { - let config = auth_config_for_url(&tokens.api_url) - .ok_or_else(|| format!("Cannot refresh: unknown API host in '{}'", tokens.api_url))?; + let config = auth_config_for_url(&tokens.api_url).ok_or_else(|| { + format!( + "Cannot refresh: unknown API host in '{}'. \ + For a non-standard host, set CLICKHOUSECTL_AUTH_URL and CLICKHOUSECTL_CLIENT_ID.", + tokens.api_url + ) + })?; let client = crate::http::client_builder().build()?; let form_body = format!( "grant_type={}&client_id={}&refresh_token={}", urlencoding::encode("refresh_token"), - urlencoding::encode(config.client_id), + urlencoding::encode(&config.client_id), urlencoding::encode(&tokens.refresh_token), ); let resp = client @@ -1002,8 +1028,17 @@ pub async fn ensure_fresh_tokens(json: bool) -> Result<(), Box { save_tokens(&new_tokens)?; } - Err(_) => { - // Refresh failed — clear stale tokens so we fall back to API keys + Err(err) => { + // If the auth config for this host can't even be resolved (e.g. a + // custom host whose CLICKHOUSECTL_AUTH_URL / CLICKHOUSECTL_CLIENT_ID + // are unset), the refresh token is still valid: don't destroy it and + // don't silently fall back to another host. Surface the actionable, + // fail-fast error instead. + if auth_config_for_url(&tokens.api_url).is_none() { + return Err(err); + } + // Genuine refresh rejection (revoked/expired): clear stale tokens so + // we fall back to API keys. clear_tokens(); if !json { eprint_line("Warning: OAuth token refresh failed. Tokens cleared."); @@ -1383,7 +1418,8 @@ mod tests { assert_ne!(prod.client_id, dev.client_id); assert_ne!(staging.client_id, dev.client_id); - // Must not match on substring — these are hostile URLs that embed a known host + // Must not match on substring — these are hostile URLs that embed a known + // host. With no env vars set they fall through to the unknown-host branch. assert!(auth_config_for_url("https://api.clickhouse.cloud.evil.com/v1").is_none()); assert!(auth_config_for_url("https://evil-api.clickhouse.cloud.attacker.com/v1").is_none()); assert!(auth_config_for_url("https://not-api.clickhouse-staging.com.bad.com/v1").is_none()); diff --git a/crates/clickhousectl/src/cloud/mod.rs b/crates/clickhousectl/src/cloud/mod.rs index dedb3e8e..620450aa 100644 --- a/crates/clickhousectl/src/cloud/mod.rs +++ b/crates/clickhousectl/src/cloud/mod.rs @@ -74,12 +74,18 @@ pub async fn run(args: CloudArgs, json: bool) -> Result<()> { command => command, }; - // Refresh OAuth tokens if needed. Errors here are filesystem failures - // (refresh-rpc failures are swallowed and tokens cleared), so this stays - // a generic error rather than `AuthRequired`. - auth::ensure_fresh_tokens(json) - .await - .map_err(|e| Error::Cloud(e.to_string()))?; + // Refresh OAuth tokens if needed. A refresh failure (or unresolvable + // custom-host config) is only fatal when OAuth is the sole auth source: + // API keys from flags, the credentials file, or env outrank OAuth, so if + // any are present we proceed on those and leave the stale tokens alone. + auth::ensure_fresh_tokens(json).await.or_else(|e| { + if resolve_active_auth_source(args.api_key.as_deref(), args.api_secret.as_deref()).is_none() + { + Err(Error::Cloud(e.to_string())) + } else { + Ok(()) + } + })?; let client = CloudClient::new( args.api_key.as_deref(), diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 0aaf0086..7a4a3e78 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -29623,7 +29623,10 @@ async fn cloud_runtime_errors_keep_credential_notices_out_of_json() { let mut command = cloud_runtime_error_command(dir.path(), &mock.uri(), mode); let token_dir = dir.path().join("home/.clickhouse"); std::fs::create_dir_all(&token_dir).unwrap(); - // An unknown auth host fails refresh locally, then clears stale tokens. + // An unknown auth host whose CLICKHOUSECTL_AUTH_URL/CLIENT_ID are unset + // fails refresh locally. With no API keys to fall back on, the + // actionable fail-fast error is surfaced (generic, exit 1) and the + // stale tokens are preserved rather than silently wiped. std::fs::write( token_dir.join("tokens.json"), serde_json::to_vec(&serde_json::json!({ @@ -29639,13 +29642,64 @@ async fn cloud_runtime_errors_keep_credential_notices_out_of_json() { .args(["service", "list", "--org-id", "org-1"]) .output() .unwrap(); - assert_eq!(output.status.code(), Some(4)); + assert_eq!(output.status.code(), Some(1)); assert!(output.stdout.is_empty()); - assert_eq!(cloud_runtime_error(&output)["code"], "auth_required"); - assert!(!token_dir.join("tokens.json").exists()); + let error = cloud_runtime_error(&output); + assert_eq!(error["code"], "other"); + assert!( + error["message"] + .as_str() + .unwrap() + .contains("CLICKHOUSECTL_AUTH_URL") + ); + assert!(token_dir.join("tokens.json").exists()); } } +// Credential precedence outranks a stale OAuth refresh failure: expired +// custom-host tokens that cannot be refreshed must not block a command when +// higher-precedence API keys are available (regression for #1082). +#[tokio::test] +async fn expired_custom_host_tokens_do_not_block_api_key_auth() { + let mock = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1/services")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ "result": [] }))) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + // API keys supplied via env (EnvVars source) outrank OAuth tokens. + let mut command = cloud_runtime_error_command(dir.path(), &mock.uri(), "explicit"); + let token_dir = dir.path().join("home/.clickhouse"); + std::fs::create_dir_all(&token_dir).unwrap(); + std::fs::write( + token_dir.join("tokens.json"), + serde_json::to_vec(&serde_json::json!({ + "access_token": "expired", "refresh_token": "refresh", "expires_at": 1, + "api_url": mock.uri() + })) + .unwrap(), + ) + .unwrap(); + let output = command + .args(["service", "list", "--org-id", "org-1"]) + .output() + .unwrap(); + + assert_eq!( + output.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + // The command reached the API using the API keys instead of failing fast. + assert_eq!(mock.received_requests().await.unwrap().len(), 1); + // The unrefreshable OAuth tokens were left untouched. + assert!(token_dir.join("tokens.json").exists()); +} + // A pipe with no reader before spawn makes failure deterministic, even if the // scheduler never runs the reader until after the CLI has produced its output. fn closed_stdout() -> std::io::PipeWriter { From 7c1c82482a585da2d06519c848ce5b8076ed6dfd Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 12:53:32 +0200 Subject: [PATCH 2/9] fixup! feat(auth): allow OAuth device login against custom API targets store auth config --- crates/clickhousectl/src/cloud/auth.rs | 154 ++++++++++--- crates/clickhousectl/src/cloud/mod.rs | 16 +- .../tests/cli_request_shape_test.rs | 206 ++++++++++++++++-- crates/clickhousectl/tests/telemetry_test.rs | 1 + 4 files changed, 316 insertions(+), 61 deletions(-) diff --git a/crates/clickhousectl/src/cloud/auth.rs b/crates/clickhousectl/src/cloud/auth.rs index fffee2ae..b72e7fa4 100644 --- a/crates/clickhousectl/src/cloud/auth.rs +++ b/crates/clickhousectl/src/cloud/auth.rs @@ -203,7 +203,7 @@ pub async fn run( })?; login_with_api_key(&key, &secret, api_url, json).await } else { - let url = api_url.unwrap_or("https://api.clickhouse.cloud"); + let url = api_url.unwrap_or(DEFAULT_API_URL); let tokens = device_auth_login(url) .await .map_err(|error| Error::Cloud(error.to_string()))?; @@ -633,7 +633,7 @@ fn prompt_api_credentials() -> std::result::Result<(String, String), Box Option { +/// Look up bundled configuration without consulting the environment. +fn known_auth_config_for_url(api_url: &str) -> Option { let parsed = url::Url::parse(api_url).ok()?; let host = parsed.host_str()?; - if let Some(config) = KNOWN_CONFIGS + KNOWN_CONFIGS .iter() .find(|(known_host, _, _)| host == *known_host) .map(|(_, auth_url, client_id)| AuthConfig { auth_url: (*auth_url).to_string(), client_id: (*client_id).to_string(), }) - { +} + +/// Resolve configuration for a new login, never for an existing session. +fn auth_config_for_url(api_url: &str) -> Option { + // Preserve rejection of malformed or hostless API URLs before reading env. + url::Url::parse(api_url).ok()?.host_str()?; + if let Some(config) = known_auth_config_for_url(api_url) { return Some(config); } @@ -709,12 +716,9 @@ pub struct TokenStore { pub refresh_token: String, pub expires_at: i64, /// The API base URL these tokens were issued for (e.g. "https://api.clickhouse.cloud/v1"). - #[serde(default = "default_api_url")] pub api_url: String, -} - -fn default_api_url() -> String { - DEFAULT_API_URL.to_string() + /// Complete authorization configuration resolved at login, never at refresh. + auth_config: AuthConfig, } #[derive(Debug, Deserialize)] @@ -933,6 +937,7 @@ pub async fn device_auth_login(api_url: &str) -> Result Result Result> { - let config = auth_config_for_url(&tokens.api_url).ok_or_else(|| { - format!( - "Cannot refresh: unknown API host in '{}'. \ - For a non-standard host, set CLICKHOUSECTL_AUTH_URL and CLICKHOUSECTL_CLIENT_ID.", - tokens.api_url - ) - })?; + let config = &tokens.auth_config; let client = crate::http::client_builder().build()?; @@ -1005,6 +1004,7 @@ pub async fn refresh_access_token( .unwrap_or_else(|| tokens.refresh_token.clone()), expires_at: now + token_resp.expires_in, api_url: tokens.api_url.clone(), + auth_config: config.clone(), }) } @@ -1028,17 +1028,8 @@ pub async fn ensure_fresh_tokens(json: bool) -> Result<(), Box { save_tokens(&new_tokens)?; } - Err(err) => { - // If the auth config for this host can't even be resolved (e.g. a - // custom host whose CLICKHOUSECTL_AUTH_URL / CLICKHOUSECTL_CLIENT_ID - // are unset), the refresh token is still valid: don't destroy it and - // don't silently fall back to another host. Surface the actionable, - // fail-fast error instead. - if auth_config_for_url(&tokens.api_url).is_none() { - return Err(err); - } - // Genuine refresh rejection (revoked/expired): clear stale tokens so - // we fall back to API keys. + Err(_) => { + // Failed refresh requires a new login. clear_tokens(); if !json { eprint_line("Warning: OAuth token refresh failed. Tokens cleared."); @@ -1222,6 +1213,7 @@ mod tests { refresh_token: "refresh456".to_string(), expires_at: 1700000000, api_url: "https://api.clickhouse.cloud/v1".into(), + auth_config: known_auth_config_for_url(DEFAULT_API_URL).unwrap(), }; let json = serde_json::to_string(&tokens).unwrap(); let parsed: TokenStore = serde_json::from_str(&json).unwrap(); @@ -1229,6 +1221,7 @@ mod tests { assert_eq!(parsed.refresh_token, "refresh456"); assert_eq!(parsed.expires_at, 1700000000); assert_eq!(parsed.api_url, "https://api.clickhouse.cloud/v1"); + assert_eq!(parsed.auth_config, tokens.auth_config); } #[test] @@ -1238,6 +1231,7 @@ mod tests { refresh_token: "r".into(), expires_at: 1700000000, api_url: "https://api.control-plane.clickhouse-staging.com/v1".into(), + auth_config: sample_auth_config(), }; let json = serde_json::to_string(&tokens).unwrap(); assert!(json.contains("clickhouse-staging.com")); @@ -1249,10 +1243,95 @@ mod tests { } #[test] - fn test_token_deserialization_defaults_to_production() { - let json = r#"{"access_token":"a","refresh_token":"r","expires_at":1700000000}"#; - let parsed: TokenStore = serde_json::from_str(json).unwrap(); - assert_eq!(parsed.api_url, "https://api.clickhouse.cloud/v1"); + fn token_deserialization_requires_complete_configuration() { + let complete = serde_json::to_value(sample_tokens()).unwrap(); + for missing in ["api_url", "auth_config"] { + let mut record = complete.clone(); + record.as_object_mut().unwrap().remove(missing); + assert!(serde_json::from_value::(record).is_err()); + } + for missing in ["auth_url", "client_id"] { + let mut record = complete.clone(); + record["auth_config"] + .as_object_mut() + .unwrap() + .remove(missing); + assert!(serde_json::from_value::(record).is_err()); + } + let mut record = complete; + record["auth_config"] = serde_json::Value::Null; + assert!(serde_json::from_value::(record).is_err()); + } + + #[test] + fn load_tokens_rejects_old_records_for_known_and_custom_hosts() { + let tmp = tempfile::tempdir().unwrap(); + let global = tmp.path().join("tokens.json"); + let legacy = tmp.path().join("legacy.json"); + for api_url in [DEFAULT_API_URL, "https://custom.example.com/v1"] { + let old = serde_json::json!({ + "access_token": "a", "refresh_token": "r", + "expires_at": 4102444800_i64, "api_url": api_url + }); + std::fs::write(&global, serde_json::to_vec(&old).unwrap()).unwrap(); + std::fs::write(&legacy, serde_json::to_vec(&old).unwrap()).unwrap(); + assert!(load_tokens_from(&global, &legacy).is_none()); + } + } + + #[tokio::test] + async fn refresh_uses_saved_config_even_for_a_known_api_host() { + use wiremock::matchers::{body_string_contains, method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let issuer = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .and(body_string_contains("client_id=saved-client")) + .and(body_string_contains("refresh_token=r")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "refreshed", "expires_in": 3600, "token_type": "Bearer" + }))) + .expect(1) + .mount(&issuer) + .await; + let mut tokens = sample_tokens(); + tokens.auth_config = AuthConfig { + auth_url: issuer.uri(), + client_id: "saved-client".into(), + }; + let refreshed = refresh_access_token(&tokens).await.unwrap(); + assert_eq!(refreshed.api_url, DEFAULT_API_URL); + assert_eq!(refreshed.auth_config, tokens.auth_config); + assert_eq!(refreshed.access_token, "refreshed"); + } + + #[test] + fn saved_auth_config_survives_legacy_path_migration_and_login_replacement() { + let tmp = tempfile::tempdir().unwrap(); + let global = tmp.path().join("global/tokens.json"); + let legacy = tmp.path().join("legacy/tokens.json"); + let mut tokens = sample_tokens(); + tokens.auth_config = AuthConfig { + auth_url: "https://auth-a.example.com".into(), + client_id: "client-a".into(), + }; + save_tokens_to(&legacy, &tokens).unwrap(); + let migrated = load_tokens_from(&global, &legacy).unwrap(); + assert_eq!(migrated.auth_config, tokens.auth_config); + assert!(!legacy.exists()); + + tokens.access_token = "new-access".into(); + tokens.refresh_token = "new-refresh".into(); + tokens.auth_config = AuthConfig { + auth_url: "https://auth-b.example.com".into(), + client_id: "client-b".into(), + }; + save_tokens_to(&global, &tokens).unwrap(); + let replaced = load_tokens_from(&global, &legacy).unwrap(); + assert_eq!(replaced.access_token, tokens.access_token); + assert_eq!(replaced.refresh_token, tokens.refresh_token); + assert_eq!(replaced.auth_config, tokens.auth_config); } #[test] @@ -1264,6 +1343,7 @@ mod tests { refresh_token: "r".into(), expires_at: now + 3600, api_url: DEFAULT_API_URL.into(), + auth_config: sample_auth_config(), }; assert!(is_token_valid(&valid)); @@ -1272,6 +1352,7 @@ mod tests { refresh_token: "r".into(), expires_at: now - 10, api_url: DEFAULT_API_URL.into(), + auth_config: sample_auth_config(), }; assert!(!is_token_valid(&expired)); @@ -1280,6 +1361,7 @@ mod tests { refresh_token: "r".into(), expires_at: now + 30, // within 60s buffer api_url: DEFAULT_API_URL.into(), + auth_config: sample_auth_config(), }; assert!(!is_token_valid(&near_expiry)); } @@ -1310,6 +1392,14 @@ mod tests { refresh_token: "r".into(), expires_at: chrono::Utc::now().timestamp() + 3600, api_url: DEFAULT_API_URL.into(), + auth_config: sample_auth_config(), + } + } + + fn sample_auth_config() -> AuthConfig { + AuthConfig { + auth_url: "https://auth.example.com".into(), + client_id: "test-client".into(), } } diff --git a/crates/clickhousectl/src/cloud/mod.rs b/crates/clickhousectl/src/cloud/mod.rs index 620450aa..cc2e1b18 100644 --- a/crates/clickhousectl/src/cloud/mod.rs +++ b/crates/clickhousectl/src/cloud/mod.rs @@ -74,18 +74,10 @@ pub async fn run(args: CloudArgs, json: bool) -> Result<()> { command => command, }; - // Refresh OAuth tokens if needed. A refresh failure (or unresolvable - // custom-host config) is only fatal when OAuth is the sole auth source: - // API keys from flags, the credentials file, or env outrank OAuth, so if - // any are present we proceed on those and leave the stale tokens alone. - auth::ensure_fresh_tokens(json).await.or_else(|e| { - if resolve_active_auth_source(args.api_key.as_deref(), args.api_secret.as_deref()).is_none() - { - Err(Error::Cloud(e.to_string())) - } else { - Ok(()) - } - })?; + // Refresh failures clear unusable tokens; returned errors are storage failures. + auth::ensure_fresh_tokens(json) + .await + .map_err(|e| Error::Cloud(e.to_string()))?; let client = CloudClient::new( args.api_key.as_deref(), diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 7a4a3e78..b0ed6cf5 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -11674,6 +11674,7 @@ async fn service_query_with_oauth_sends_bearer_and_never_provisions() { "refresh_token": "unused", "expires_at": 4102444800u64, // 2100-01-01: never expires in tests "api_url": format!("{}/v1", control.uri()), + "auth_config": {"auth_url": control.uri(), "client_id": "test-client"}, }); std::fs::write( ch_dir.join("tokens.json"), @@ -15398,6 +15399,7 @@ fn write_oauth_tokens(ch_dir: &std::path::Path, control_uri: &str) { "refresh_token": "unused", "expires_at": 4102444800u64, // 2100-01-01: never expires in tests "api_url": format!("{control_uri}/v1"), + "auth_config": {"auth_url": control_uri, "client_id": "test-client"}, }); std::fs::write( ch_dir.join("tokens.json"), @@ -29623,10 +29625,8 @@ async fn cloud_runtime_errors_keep_credential_notices_out_of_json() { let mut command = cloud_runtime_error_command(dir.path(), &mock.uri(), mode); let token_dir = dir.path().join("home/.clickhouse"); std::fs::create_dir_all(&token_dir).unwrap(); - // An unknown auth host whose CLICKHOUSECTL_AUTH_URL/CLIENT_ID are unset - // fails refresh locally. With no API keys to fall back on, the - // actionable fail-fast error is surfaced (generic, exit 1) and the - // stale tokens are preserved rather than silently wiped. + // Old records without configuration are not usable credentials. + // Require a new login rather than guessing from the environment. std::fs::write( token_dir.join("tokens.json"), serde_json::to_vec(&serde_json::json!({ @@ -29639,26 +29639,22 @@ async fn cloud_runtime_errors_keep_credential_notices_out_of_json() { let output = command .env_remove("CLICKHOUSE_CLOUD_API_KEY") .env_remove("CLICKHOUSE_CLOUD_API_SECRET") + // Even configured env values must not repair an unknown legacy issuer. + .env("CLICKHOUSECTL_AUTH_URL", mock.uri()) + .env("CLICKHOUSECTL_CLIENT_ID", "must-not-be-used") .args(["service", "list", "--org-id", "org-1"]) .output() .unwrap(); - assert_eq!(output.status.code(), Some(1)); + assert_eq!(output.status.code(), Some(4)); assert!(output.stdout.is_empty()); - let error = cloud_runtime_error(&output); - assert_eq!(error["code"], "other"); - assert!( - error["message"] - .as_str() - .unwrap() - .contains("CLICKHOUSECTL_AUTH_URL") - ); + assert_eq!(cloud_runtime_error(&output)["code"], "auth_required"); + // Loading rejects the old file without modifying it; login replaces it. assert!(token_dir.join("tokens.json").exists()); } } -// Credential precedence outranks a stale OAuth refresh failure: expired -// custom-host tokens that cannot be refreshed must not block a command when -// higher-precedence API keys are available (regression for #1082). +// Unrecoverable legacy OAuth tokens must not block higher-precedence API keys. +// Skipping refresh entirely when keys win is a separate precedence concern. #[tokio::test] async fn expired_custom_host_tokens_do_not_block_api_key_auth() { let mock = MockServer::start().await; @@ -29696,10 +29692,186 @@ async fn expired_custom_host_tokens_do_not_block_api_key_auth() { ); // The command reached the API using the API keys instead of failing fast. assert_eq!(mock.received_requests().await.unwrap().len(), 1); - // The unrefreshable OAuth tokens were left untouched. + // Old files are ignored, not refreshed or modified; API-key auth succeeds. assert!(token_dir.join("tokens.json").exists()); } +fn custom_oauth_command(root: &Path, api_url: &str) -> Command { + let mut command = cloud_runtime_error_command(root, api_url, "explicit"); + command + .env_remove("CLICKHOUSE_CLOUD_API_KEY") + .env_remove("CLICKHOUSE_CLOUD_API_SECRET"); + command +} + +#[tokio::test] +async fn old_oauth_records_require_login_even_for_known_hosts_and_fresh_tokens() { + let mock = MockServer::start().await; + let dir = tempfile::tempdir().unwrap(); + let token_dir = dir.path().join("home/.clickhouse"); + std::fs::create_dir_all(&token_dir).unwrap(); + for api_url in ["https://api.clickhouse.cloud/v1".to_string(), mock.uri()] { + for expires_at in [1_i64, 4102444800_i64] { + let old = serde_json::json!({ + "access_token": "old-access", "refresh_token": "old-refresh", + "expires_at": expires_at, "api_url": api_url + }); + std::fs::write( + token_dir.join("tokens.json"), + serde_json::to_vec(&old).unwrap(), + ) + .unwrap(); + let output = custom_oauth_command(dir.path(), &mock.uri()) + .env("CLICKHOUSECTL_AUTH_URL", mock.uri()) + .env("CLICKHOUSECTL_CLIENT_ID", "must-not-be-used") + .args(["auth", "whoami"]) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(4), "{output:?}"); + assert_eq!(cloud_runtime_error(&output)["code"], "auth_required"); + } + } + assert!(mock.received_requests().await.unwrap().is_empty()); +} + +#[tokio::test] +async fn custom_oauth_login_pins_config_through_refresh_and_relogin() { + let api = MockServer::start().await; + let issuer_a = MockServer::start().await; + let issuer_b = MockServer::start().await; + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + Mock::given(method("GET")) + .and(path("/v1/whoami")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": {"actorType": "user", "email": "test@example.com", "organizations": []} + }))) + .expect(4) + .mount(&api) + .await; + + // An absent local path makes the best-effort opener fail without launching + // a real browser. This test drives the token response, not interactive login. + let verification_uri = dir.path().join("no-browser-target"); + for (issuer, client_id, access, refresh) in [ + (&issuer_a, "client-a", "access-a", "refresh-a"), + (&issuer_b, "client-b", "access-b", "refresh-b"), + ] { + Mock::given(method("POST")) + .and(path("/oauth/device/code")) + .and(wiremock::matchers::body_string_contains(format!( + "client_id={client_id}" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "device_code": "test-device", "user_code": "TEST", + "verification_uri": verification_uri.to_string_lossy(), + "expires_in": 60, "interval": 0 + }))) + .expect(1) + .mount(issuer) + .await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .and(wiremock::matchers::body_string_contains( + "device_code=test-device", + )) + .and(wiremock::matchers::body_string_contains(format!( + "client_id={client_id}" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": access, "refresh_token": refresh, + "expires_in": 3600, "token_type": "Bearer" + }))) + .expect(1) + .mount(issuer) + .await; + } + + let output = custom_oauth_command(dir.path(), &api.uri()) + .env("CLICKHOUSECTL_AUTH_URL", format!("{}/", issuer_a.uri())) + .env("CLICKHOUSECTL_CLIENT_ID", "client-a") + .args(["auth", "login"]) + .output() + .unwrap(); + assert_success(&output); + let mut saved: Value = serde_json::from_slice(&std::fs::read(&token_path).unwrap()).unwrap(); + let config_a = serde_json::json!({"auth_url": issuer_a.uri(), "client_id": "client-a"}); + assert_eq!(saved["auth_config"], config_a); + assert_eq!(saved["api_url"], format!("{}/v1", api.uri())); + assert_eq!(saved["refresh_token"], "refresh-a"); + + // First refresh removes the variables and rotates the token. The second + // changes both variables to B and omits rotation: both must still use A. + for (changed_env, old_refresh, response) in [ + ( + false, + "refresh-a", + serde_json::json!({ + "access_token": "refreshed-a", "refresh_token": "rotated-a", + "expires_in": 3600, "token_type": "Bearer" + }), + ), + ( + true, + "rotated-a", + serde_json::json!({ + "access_token": "refreshed-a-again", "expires_in": 3600, "token_type": "Bearer" + }), + ), + ] { + Mock::given(method("POST")) + .and(path("/oauth/token")) + .and(wiremock::matchers::body_string_contains( + "grant_type=refresh_token", + )) + .and(wiremock::matchers::body_string_contains( + "client_id=client-a", + )) + .and(wiremock::matchers::body_string_contains(format!( + "refresh_token={old_refresh}" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(response)) + .expect(1) + .mount(&issuer_a) + .await; + saved["expires_at"] = serde_json::json!(1); + std::fs::write(&token_path, serde_json::to_vec(&saved).unwrap()).unwrap(); + let mut command = custom_oauth_command(dir.path(), &api.uri()); + if changed_env { + command + .env("CLICKHOUSECTL_AUTH_URL", issuer_b.uri()) + .env("CLICKHOUSECTL_CLIENT_ID", "client-b"); + } + let output = command.args(["auth", "whoami"]).output().unwrap(); + assert_success(&output); + saved = serde_json::from_slice(&std::fs::read(&token_path).unwrap()).unwrap(); + assert_eq!(saved["auth_config"], config_a); + assert_eq!(saved["refresh_token"], "rotated-a"); + assert!(saved["expires_at"].as_i64().unwrap() > chrono::Utc::now().timestamp()); + assert!(issuer_b.received_requests().await.unwrap().is_empty()); + } + + // Only a deliberate new login changes the saved issuer/token pairing. + let output = custom_oauth_command(dir.path(), &api.uri()) + .env("CLICKHOUSECTL_AUTH_URL", issuer_b.uri()) + .env("CLICKHOUSECTL_CLIENT_ID", "client-b") + .args(["auth", "login"]) + .output() + .unwrap(); + assert_success(&output); + saved = serde_json::from_slice(&std::fs::read(&token_path).unwrap()).unwrap(); + assert_eq!( + saved["auth_config"], + serde_json::json!({ + "auth_url": issuer_b.uri(), "client_id": "client-b" + }) + ); + assert_eq!(saved["access_token"], "access-b"); + assert_eq!(saved["refresh_token"], "refresh-b"); + assert_eq!(issuer_a.received_requests().await.unwrap().len(), 4); + assert_eq!(issuer_b.received_requests().await.unwrap().len(), 2); +} + // A pipe with no reader before spawn makes failure deterministic, even if the // scheduler never runs the reader until after the CLI has produced its output. fn closed_stdout() -> std::io::PipeWriter { diff --git a/crates/clickhousectl/tests/telemetry_test.rs b/crates/clickhousectl/tests/telemetry_test.rs index 6a272531..87a38ba7 100644 --- a/crates/clickhousectl/tests/telemetry_test.rs +++ b/crates/clickhousectl/tests/telemetry_test.rs @@ -1709,6 +1709,7 @@ fn write_oauth_tokens(sandbox: &Sandbox, control_uri: &str) { "refresh_token": "unused", "expires_at": 4102444800u64, "api_url": format!("{control_uri}/v1"), + "auth_config": {"auth_url": control_uri, "client_id": "test-client"}, })) .unwrap(), ) From 2b869ca2fd01d0d1e08fd061a599b6039d71b6c3 Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 13:12:21 +0200 Subject: [PATCH 3/9] fixup! feat(auth): allow OAuth device login against custom API targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer feedback (point 2): https://github.com/ClickHouse/clickhousectl/pull/1082#pullrequestreview-5523226916 **2. Route the env read through an injectable lookup** `auth_config_for_url` calls `std::env::var` directly. Two consequences: the env path has no unit test (it would mean mutating process env), and the existing hostile-URL `is_none()` assertions fail on any machine that has these two vars exported. Please follow the `resolve_auth` / `resolve_auth_with_sources` + `EnvLookup` pattern in `cloud/client.rs` (a thin wrapper around an `_with` fn that takes the lookup), and add unit tests: built-in host ignores env, only one var set → `None`, empty value counts as unset, trailing slash trimmed. How this addresses the feedback: - Keep auth_config_for_url as the production wrapper and inject an EnvLookup into auth_config_for_url_with, following the existing client resolver pattern. - Run lookup and hostile-host tests against a controlled environment instead of reading or mutating process variables. - Cover built-in hosts bypassing environment reads, missing or empty values, custom configuration and trailing-slash trimming, and malformed/hostless API URLs rejecting input before consulting the environment. - Verify all 24 auth tests pass with both overrides absent and exported. - Include the pending development note for point 1: login persists complete configuration, refresh uses only stored values, and older incomplete token files require a new login, including for built-in hosts. This reflects the explicitly agreed compatibility policy rather than the review's proposed legacy built-in fallback. Validation: - cargo fmt --all - cargo clippy -p clickhousectl -- -D warnings - cargo check -p clickhousectl --no-default-features - cargo clippy -p clickhousectl --all-targets --no-default-features -- -D warnings - cargo test -p clickhousectl --no-fail-fast: all Cloud and auth tests pass; the sole failure is the unrelated local Postgres readiness test failed_fresh_start_preserves_postgres_identity_without_polluting_clickhouse_selection, which reports that the Docker socket was not found. --- crates/clickhousectl/src/cloud/auth.rs | 102 ++++++++++++++++++++----- docs/development.md | 2 + 2 files changed, 86 insertions(+), 18 deletions(-) diff --git a/crates/clickhousectl/src/cloud/auth.rs b/crates/clickhousectl/src/cloud/auth.rs index b72e7fa4..def28c71 100644 --- a/crates/clickhousectl/src/cloud/auth.rs +++ b/crates/clickhousectl/src/cloud/auth.rs @@ -675,8 +675,15 @@ fn known_auth_config_for_url(api_url: &str) -> Option { }) } +/// Injectable environment lookup keeps login tests independent of the shell. +type EnvLookup<'a> = &'a dyn Fn(&str) -> Option; + /// Resolve configuration for a new login, never for an existing session. fn auth_config_for_url(api_url: &str) -> Option { + auth_config_for_url_with(api_url, &|key| std::env::var(key).ok()) +} + +fn auth_config_for_url_with(api_url: &str, env_lookup: EnvLookup<'_>) -> Option { // Preserve rejection of malformed or hostless API URLs before reading env. url::Url::parse(api_url).ok()?.host_str()?; if let Some(config) = known_auth_config_for_url(api_url) { @@ -684,7 +691,7 @@ fn auth_config_for_url(api_url: &str) -> Option { } // Unknown host: require both env vars, with no built-in fallback. - let env = |key: &str| std::env::var(key).ok().filter(|v| !v.is_empty()); + let env = |key: &str| env_lookup(key).filter(|v| !v.is_empty()); match ( env("CLICKHOUSECTL_AUTH_URL"), env("CLICKHOUSECTL_CLIENT_ID"), @@ -1492,27 +1499,86 @@ mod tests { #[test] fn test_auth_config_lookup() { - assert!(auth_config_for_url("https://api.clickhouse.cloud/v1").is_some()); - assert!( - auth_config_for_url("https://api.control-plane.clickhouse-staging.com/v1").is_some() - ); - assert!(auth_config_for_url("https://api.control-plane.clickhouse-dev.com/v1").is_some()); - assert!(auth_config_for_url("https://api.unknown.com/v1").is_none()); - - // Verify distinct configs - let prod = auth_config_for_url("https://api.clickhouse.cloud/v1").unwrap(); - let staging = - auth_config_for_url("https://api.control-plane.clickhouse-staging.com/v1").unwrap(); - let dev = auth_config_for_url("https://api.control-plane.clickhouse-dev.com/v1").unwrap(); + let lookup = |url: &str| auth_config_for_url_with(url, &|_| None); + assert!(lookup("https://api.clickhouse.cloud/v1").is_some()); + assert!(lookup("https://api.control-plane.clickhouse-staging.com/v1").is_some()); + assert!(lookup("https://api.control-plane.clickhouse-dev.com/v1").is_some()); + assert!(lookup("https://api.unknown.com/v1").is_none()); + + // Verify distinct configs without reading the process environment. + let prod = lookup("https://api.clickhouse.cloud/v1").unwrap(); + let staging = lookup("https://api.control-plane.clickhouse-staging.com/v1").unwrap(); + let dev = lookup("https://api.control-plane.clickhouse-dev.com/v1").unwrap(); assert_ne!(prod.client_id, staging.client_id); assert_ne!(prod.client_id, dev.client_id); assert_ne!(staging.client_id, dev.client_id); - // Must not match on substring — these are hostile URLs that embed a known - // host. With no env vars set they fall through to the unknown-host branch. - assert!(auth_config_for_url("https://api.clickhouse.cloud.evil.com/v1").is_none()); - assert!(auth_config_for_url("https://evil-api.clickhouse.cloud.attacker.com/v1").is_none()); - assert!(auth_config_for_url("https://not-api.clickhouse-staging.com.bad.com/v1").is_none()); + // Must not match known hosts by substring, regardless of the real env. + assert!(lookup("https://api.clickhouse.cloud.evil.com/v1").is_none()); + assert!(lookup("https://evil-api.clickhouse.cloud.attacker.com/v1").is_none()); + assert!(lookup("https://not-api.clickhouse-staging.com.bad.com/v1").is_none()); + } + + #[test] + fn auth_config_known_hosts_ignore_environment() { + for (host, auth_url, client_id) in KNOWN_CONFIGS { + let api_url = format!("https://{host}/v1"); + let config = auth_config_for_url_with(&api_url, &|_| { + panic!("built-in host must not read environment overrides") + }) + .unwrap(); + assert_eq!(config.auth_url, *auth_url); + assert_eq!(config.client_id, *client_id); + } + } + + #[test] + fn auth_config_custom_host_requires_both_nonempty_values() { + for (auth_url, client_id) in [ + (None, None), + (Some("https://auth.example.com"), None), + (None, Some("client")), + (Some(""), Some("client")), + (Some("https://auth.example.com"), Some("")), + (Some(""), Some("")), + ] { + let config = auth_config_for_url_with("https://api.example.com/v1", &|key| match key { + "CLICKHOUSECTL_AUTH_URL" => auth_url.map(str::to_owned), + "CLICKHOUSECTL_CLIENT_ID" => client_id.map(str::to_owned), + _ => panic!("unexpected environment lookup: {key}"), + }); + assert!(config.is_none(), "{auth_url:?}, {client_id:?}"); + } + } + + #[test] + fn auth_config_custom_host_uses_environment_and_trims_trailing_slashes() { + for auth_url in [ + "https://auth.example.com", + "https://auth.example.com/", + "https://auth.example.com///", + ] { + let config = auth_config_for_url_with("https://api.example.com/v1", &|key| match key { + "CLICKHOUSECTL_AUTH_URL" => Some(auth_url.to_owned()), + "CLICKHOUSECTL_CLIENT_ID" => Some("custom-client".into()), + _ => panic!("unexpected environment lookup: {key}"), + }) + .unwrap(); + assert_eq!(config.auth_url, "https://auth.example.com"); + assert_eq!(config.client_id, "custom-client"); + } + } + + #[test] + fn auth_config_invalid_api_url_never_reads_environment() { + for api_url in ["not a URL", "file:///tmp/api", "mailto:user@example.com"] { + assert!( + auth_config_for_url_with(api_url, &|_| { + panic!("invalid API URL must not read environment overrides") + }) + .is_none() + ); + } } #[test] diff --git a/docs/development.md b/docs/development.md index 7f72b8c1..0d2572ec 100644 --- a/docs/development.md +++ b/docs/development.md @@ -65,6 +65,8 @@ Failures fail the run by default. `CONTINUE_ON_NON_BLOCKING_FAILURES=1` collects Cloud `--debug` prints the selected credential source and API URL. For a non-production control plane, `--url` overrides the Cloud API base URL; release builds accept it but hide it from help. `CLICKHOUSE_CLOUD_QUERY_HOST` separately overrides the Query API host. +For OAuth login to an API host outside the built-in mappings, set `CLICKHOUSECTL_AUTH_URL` and `CLICKHOUSECTL_CLIENT_ID` when running `cloud auth login --url `; login saves the resolved configuration with the tokens, and refresh uses that saved configuration rather than the current environment. Built-in hosts ignore these overrides. Token files must include the complete configuration; older files without it require a new login, including for built-in hosts. Refresh never consults the built-in mappings or environment. + Keep tests isolated from real credentials and telemetry. Use the existing temporary-project and local-mock patterns in subprocess tests and set `DO_NOT_TRACK=1` for development checks. ## Telemetry changes From 9a0932a01d6b9831714c5ccefc605dcb950db7c0 Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 14:23:44 +0200 Subject: [PATCH 4/9] fixup! feat(auth): allow OAuth device login against custom API targets Reviewer feedback (point 3): https://github.com/ClickHouse/clickhousectl/pull/1082#pullrequestreview-5523226916 **3. Keep named fields in the built-in table** Flattening `KNOWN_CONFIGS` to `(&str, &str, &str)` drops the field names: swapping an auth URL and a client id still compiles. Suggest a separate `KnownAuthConfig { host, auth_url, client_id }` with `&'static str` fields for the const table, converted into the owned `AuthConfig` (which needs `String` anyway once it's stored in `TokenStore`). How this addresses the feedback: - Replace positional tuples with KnownAuthConfig entries naming host, auth_url, and client_id, retaining static string fields in the const table. - Add From<&KnownAuthConfig> for AuthConfig to explicitly copy the named authorization fields into the owned runtime and persisted configuration. - Update exact-host lookup and the known-host diagnostic to access host by name rather than destructuring tuple positions. - Add a conversion test and update the table-wide precedence test to verify named values are preserved and built-in hosts never read env overrides. - Preserve all bundled values, login precedence, token serialization, and refresh behavior. This improves reviewability, not string type safety. Validation: - cargo fmt --all - cargo clippy -p clickhousectl -- -D warnings - cargo check -p clickhousectl --no-default-features - cargo clippy -p clickhousectl --all-targets --no-default-features -- -D warnings - cargo test -p clickhousectl --no-fail-fast: unit and Cloud tests pass; the sole failure is the unrelated local Postgres readiness test failed_fresh_start_preserves_postgres_identity_without_polluting_clickhouse_selection, which reports that the Docker socket was not found. --- crates/clickhousectl/src/cloud/auth.rs | 82 +++++++++++++++++--------- 1 file changed, 55 insertions(+), 27 deletions(-) diff --git a/crates/clickhousectl/src/cloud/auth.rs b/crates/clickhousectl/src/cloud/auth.rs index def28c71..d93fde57 100644 --- a/crates/clickhousectl/src/cloud/auth.rs +++ b/crates/clickhousectl/src/cloud/auth.rs @@ -639,26 +639,41 @@ struct AuthConfig { client_id: String, } -/// Known API host → (auth URL, client id) mappings. +struct KnownAuthConfig { + host: &'static str, + auth_url: &'static str, + client_id: &'static str, +} + +impl From<&KnownAuthConfig> for AuthConfig { + fn from(config: &KnownAuthConfig) -> Self { + Self { + auth_url: config.auth_url.to_owned(), + client_id: config.client_id.to_owned(), + } + } +} + +/// Known API host → authorization configuration mappings. /// /// Any host not listed here requires `CLICKHOUSECTL_AUTH_URL` and /// `CLICKHOUSECTL_CLIENT_ID` to be set; there is no built-in fallback. -const KNOWN_CONFIGS: &[(&str, &str, &str)] = &[ - ( - "api.clickhouse.cloud", - "https://auth.clickhouse.cloud", - "9q6XAueAs47R4X5d1d6FbjbJqjsrA2ZJ", - ), - ( - "api.control-plane.clickhouse-staging.com", - "https://auth.control-plane.clickhouse-staging.com", - "ZC8AupPshQt2UNO2hEDutnKitx4PhizY", - ), - ( - "api.control-plane.clickhouse-dev.com", - "https://auth.control-plane.clickhouse-dev.com", - "bVVcrqNw1t5dya9WFzfnM7PSsAgmfzwY", - ), +const KNOWN_CONFIGS: &[KnownAuthConfig] = &[ + KnownAuthConfig { + host: "api.clickhouse.cloud", + auth_url: "https://auth.clickhouse.cloud", + client_id: "9q6XAueAs47R4X5d1d6FbjbJqjsrA2ZJ", + }, + KnownAuthConfig { + host: "api.control-plane.clickhouse-staging.com", + auth_url: "https://auth.control-plane.clickhouse-staging.com", + client_id: "ZC8AupPshQt2UNO2hEDutnKitx4PhizY", + }, + KnownAuthConfig { + host: "api.control-plane.clickhouse-dev.com", + auth_url: "https://auth.control-plane.clickhouse-dev.com", + client_id: "bVVcrqNw1t5dya9WFzfnM7PSsAgmfzwY", + }, ]; /// Look up bundled configuration without consulting the environment. @@ -668,11 +683,8 @@ fn known_auth_config_for_url(api_url: &str) -> Option { KNOWN_CONFIGS .iter() - .find(|(known_host, _, _)| host == *known_host) - .map(|(_, auth_url, client_id)| AuthConfig { - auth_url: (*auth_url).to_string(), - client_id: (*client_id).to_string(), - }) + .find(|config| host == config.host) + .map(AuthConfig::from) } /// Injectable environment lookup keeps login tests independent of the shell. @@ -853,7 +865,7 @@ pub async fn device_auth_login(api_url: &str) -> Result>() .join(", ") ) @@ -1519,16 +1531,32 @@ mod tests { assert!(lookup("https://not-api.clickhouse-staging.com.bad.com/v1").is_none()); } + #[test] + fn known_auth_config_converts_named_fields_to_owned_values() { + let known = KnownAuthConfig { + host: "api.example.com", + auth_url: "https://auth.example.com", + client_id: "example-client", + }; + assert_eq!( + AuthConfig::from(&known), + AuthConfig { + auth_url: "https://auth.example.com".into(), + client_id: "example-client".into(), + } + ); + } + #[test] fn auth_config_known_hosts_ignore_environment() { - for (host, auth_url, client_id) in KNOWN_CONFIGS { - let api_url = format!("https://{host}/v1"); + for known in KNOWN_CONFIGS { + let api_url = format!("https://{}/v1", known.host); let config = auth_config_for_url_with(&api_url, &|_| { panic!("built-in host must not read environment overrides") }) .unwrap(); - assert_eq!(config.auth_url, *auth_url); - assert_eq!(config.client_id, *client_id); + assert_eq!(config.auth_url, known.auth_url); + assert_eq!(config.client_id, known.client_id); } } From 0d7cc4a8f7856de2546dcd009866c7087bf047ba Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 14:38:39 +0200 Subject: [PATCH 5/9] fixup! feat(auth): allow OAuth device login against custom API targets Reviewer feedback (point 4): https://github.com/ClickHouse/clickhousectl/pull/1082#pullrequestreview-5523226916 **4. Check credential precedence before refreshing, not after** `cloud::run` currently runs the refresh and then, if it failed, asks whether API keys would have won. Flip it: only call `ensure_fresh_tokens` when OAuth is the source that will actually be used. That skips a pointless refresh request when keys outrank OAuth, and the `or_else` can go (along with its comment, which claims the stale tokens are left alone, though a genuine rejection has already cleared them by then). How this addresses the feedback: - Check credential precedence before calling ensure_fresh_tokens in cloud::run. - Skip OAuth refresh whenever explicit credential flags are supplied, including incomplete pairs that must fail validation without touching saved tokens. - Reuse resolve_active_auth_source for saved credentials, env, and .env rather than duplicating the credential precedence ladder. - Allow both OAuthTokens and None through the refresh guard: the resolver excludes expired access tokens, so None can mean tokens need refreshing. With no saved tokens, ensure_fresh_tokens remains a no-op. - The obsolete or_else and its misleading comment were already removed with point 1; this change prevents refresh from running when API keys win. - Add subprocess tests proving zero issuer requests and byte-for-byte unchanged token files for flags, credentials files, env, .env, mixed env/.env, and incomplete CLI flags. Complementary tests prove absent keys, a partial env pair, or an empty credentials file still permit OAuth refresh and use the refreshed bearer token. - Document the refresh/precedence behavior in the authentication reference. Validation: - cargo fmt --all - cargo clippy -p clickhousectl -- -D warnings - cargo check -p clickhousectl --no-default-features - cargo clippy -p clickhousectl --all-targets --no-default-features -- -D warnings - cargo test -p clickhousectl --no-fail-fast: all 632 active Cloud tests pass; the sole failure is the unrelated local Postgres readiness test failed_fresh_start_preserves_postgres_identity_without_polluting_clickhouse_selection, which reports that the Docker socket was not found. --- crates/clickhousectl/src/cloud/mod.rs | 20 ++- .../tests/cli_request_shape_test.rs | 148 +++++++++++++++++- docs/reference/authentication.md | 2 +- 3 files changed, 163 insertions(+), 7 deletions(-) diff --git a/crates/clickhousectl/src/cloud/mod.rs b/crates/clickhousectl/src/cloud/mod.rs index cc2e1b18..acda62e5 100644 --- a/crates/clickhousectl/src/cloud/mod.rs +++ b/crates/clickhousectl/src/cloud/mod.rs @@ -74,10 +74,22 @@ pub async fn run(args: CloudArgs, json: bool) -> Result<()> { command => command, }; - // Refresh failures clear unusable tokens; returned errors are storage failures. - auth::ensure_fresh_tokens(json) - .await - .map_err(|e| Error::Cloud(e.to_string()))?; + // Explicit key flags select API-key auth (even an incomplete pair must fail + // validation without touching OAuth). Otherwise refresh only when no higher + // tier wins. Expired OAuth tokens resolve to None, so include that case; + // ensure_fresh_tokens is a no-op when no tokens exist or they are still fresh. + if args.api_key.is_none() + && args.api_secret.is_none() + && matches!( + resolve_active_auth_source(None, None), + None | Some(AuthSource::OAuthTokens) + ) + { + // Refresh failures clear unusable tokens; returned errors are storage failures. + auth::ensure_fresh_tokens(json) + .await + .map_err(|e| Error::Cloud(e.to_string()))?; + } let client = CloudClient::new( args.api_key.as_deref(), diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index b0ed6cf5..3b53ef08 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -29653,8 +29653,7 @@ async fn cloud_runtime_errors_keep_credential_notices_out_of_json() { } } -// Unrecoverable legacy OAuth tokens must not block higher-precedence API keys. -// Skipping refresh entirely when keys win is a separate precedence concern. +// Old OAuth records must not block higher-precedence API keys. #[tokio::test] async fn expired_custom_host_tokens_do_not_block_api_key_auth() { let mock = MockServer::start().await; @@ -29696,6 +29695,151 @@ async fn expired_custom_host_tokens_do_not_block_api_key_auth() { assert!(token_dir.join("tokens.json").exists()); } +#[tokio::test] +async fn api_key_precedence_skips_oauth_refresh_and_preserves_tokens() { + for source in ["flags", "file", "env", "dotenv", "mixed", "partial-flags"] { + let api = MockServer::start().await; + let issuer = MockServer::start().await; + // If refresh is accidentally attempted it would reject and clear tokens. + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(401)) + .expect(0) + .mount(&issuer) + .await; + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1/services")) + .and(wiremock::matchers::basic_auth( + "winning-key", + "winning-secret", + )) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!({"result": []})), + ) + .expect(if source == "partial-flags" { 0 } else { 1 }) + .mount(&api) + .await; + let dir = tempfile::tempdir().unwrap(); + let mut command = custom_oauth_command(dir.path(), &api.uri()); + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + let original = serde_json::to_vec(&serde_json::json!({ + "access_token": "expired", "refresh_token": "preserve-refresh", "expires_at": 1, + "api_url": api.uri(), + "auth_config": {"auth_url": issuer.uri(), "client_id": "saved-client"} + })) + .unwrap(); + std::fs::write(&token_path, &original).unwrap(); + match source { + "flags" => { + // Populate lower tiers too, proving the same precedence as the client. + write_project_api_credentials(dir.path(), "ignored-file", "ignored-secret"); + command + .env("CLICKHOUSE_CLOUD_API_KEY", "ignored-env") + .env("CLICKHOUSE_CLOUD_API_SECRET", "ignored-secret") + .args(["--api-key", "winning-key", "--api-secret", "winning-secret"]); + } + "file" => { + write_project_api_credentials(dir.path(), "winning-key", "winning-secret"); + command + .env("CLICKHOUSE_CLOUD_API_KEY", "ignored-env") + .env("CLICKHOUSE_CLOUD_API_SECRET", "ignored-secret"); + } + "env" => { + command + .env("CLICKHOUSE_CLOUD_API_KEY", "winning-key") + .env("CLICKHOUSE_CLOUD_API_SECRET", "winning-secret"); + } + "dotenv" => { + std::fs::write(dir.path().join(".env"), "CLICKHOUSE_CLOUD_API_KEY=winning-key\nCLICKHOUSE_CLOUD_API_SECRET=winning-secret\n").unwrap(); + } + "mixed" => { + command.env("CLICKHOUSE_CLOUD_API_KEY", "winning-key"); + std::fs::write( + dir.path().join(".env"), + "CLICKHOUSE_CLOUD_API_SECRET=winning-secret\n", + ) + .unwrap(); + } + "partial-flags" => { + command.arg("--api-key").arg("winning-key"); + } + _ => unreachable!(), + } + let output = command + .args(["service", "list", "--org-id", "org-1"]) + .output() + .unwrap(); + assert_eq!( + output.status.code(), + Some(if source == "partial-flags" { 4 } else { 0 }), + "{source}: {output:?}" + ); + assert_eq!(std::fs::read(&token_path).unwrap(), original, "{source}"); + assert!( + issuer.received_requests().await.unwrap().is_empty(), + "{source}" + ); + } +} + +#[tokio::test] +async fn incomplete_lower_priority_keys_do_not_prevent_oauth_refresh() { + for source in ["none", "partial-env", "empty-file"] { + let api = MockServer::start().await; + let issuer = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "refreshed", "expires_in": 3600, "token_type": "Bearer" + }))) + .expect(1) + .mount(&issuer) + .await; + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1/services")) + .and(wiremock::matchers::header( + "authorization", + "Bearer refreshed", + )) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!({"result": []})), + ) + .expect(1) + .mount(&api) + .await; + let dir = tempfile::tempdir().unwrap(); + let mut command = custom_oauth_command(dir.path(), &api.uri()); + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + std::fs::write( + &token_path, + serde_json::to_vec(&serde_json::json!({ + "access_token": "expired", "refresh_token": "refresh", "expires_at": 1, + "api_url": api.uri(), + "auth_config": {"auth_url": issuer.uri(), "client_id": "saved-client"} + })) + .unwrap(), + ) + .unwrap(); + match source { + "partial-env" => { + command.env("CLICKHOUSE_CLOUD_API_KEY", "incomplete-key"); + } + "empty-file" => write_project_api_credentials(dir.path(), "", ""), + "none" => {} + _ => unreachable!(), + } + let output = command + .args(["service", "list", "--org-id", "org-1"]) + .output() + .unwrap(); + assert_success(&output); + let saved: Value = serde_json::from_slice(&std::fs::read(&token_path).unwrap()).unwrap(); + assert_eq!(saved["access_token"], "refreshed", "{source}"); + } +} + fn custom_oauth_command(root: &Path, api_url: &str) -> Command { let mut command = cloud_runtime_error_command(root, api_url, "explicit"); command diff --git a/docs/reference/authentication.md b/docs/reference/authentication.md index ab103700..ad1838cc 100644 --- a/docs/reference/authentication.md +++ b/docs/reference/authentication.md @@ -33,7 +33,7 @@ The first applicable source wins: 4. Variables loaded from the current directory's `.env`. 5. Global OAuth tokens. -Supplying only one credential flag blocks fallback. `auth status` identifies inactive sources; `--debug` on a Cloud command prints the selected credential source and API URL to stderr. +Supplying only one credential flag blocks fallback. Cloud commands skip OAuth token refresh when a higher-priority API key source wins, leaving saved OAuth tokens unchanged. When OAuth is selected, expired or near-expiry tokens are refreshed before the API request. `auth status` identifies inactive sources; `--debug` on a Cloud command prints the selected credential source and API URL to stderr. `cloud postgres query` accepts **OAuth only**, unlike `cloud service query`. Remove higher-priority API key sources before using that route. Clearing a saved key file alone is insufficient if exported variables or `.env` still supply a key. From 93c7b0e6bf1fd0d096c61a599d0c5cc5d6758fe9 Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 14:55:17 +0200 Subject: [PATCH 6/9] fixup! feat(auth): allow OAuth device login against custom API targets Reviewer feedback (point 6, authorization URL validation): https://github.com/ClickHouse/clickhousectl/pull/1082#pullrequestreview-5523226916 - Validate `CLICKHOUSECTL_AUTH_URL` with `url::Url` and require `https` (allow `http` only for a loopback host), so device codes and refresh tokens never go over plaintext. How this addresses the feedback: - Parse authorization URLs with url::Url and require HTTPS, allowing HTTP only for exact localhost or literal loopback IPs (127.0.0.0/8 and ::1). - Validate environment-provided configuration at login and saved authorization URLs during deserialization. Refresh continues using only stored config, without environment reads or built-in configuration resolution. - Reject userinfo, query strings, and fragments to keep endpoint construction unambiguous; normalize accepted URLs and trim trailing slashes. - Disable redirects on the OAuth HTTP client so device-flow and refresh form bodies cannot be forwarded to a different endpoint or downgraded to HTTP. - Add unit coverage for HTTPS, loopback HTTP, unsafe schemes/hosts, malformed URLs, and saved-config validation. Add subprocess tests for rejection before requests and zero redirect-target requests across 301/302/303/307/308. - Document the URL restrictions and no-redirect behavior in development docs. Validation: - cargo fmt --all - cargo clippy -p clickhousectl -- -D warnings - cargo check -p clickhousectl --no-default-features - cargo clippy -p clickhousectl --all-targets --no-default-features -- -D warnings - cargo test -p clickhousectl --no-fail-fast: all 634 active Cloud tests pass; the sole failure is the unrelated local Postgres readiness test failed_fresh_start_preserves_postgres_identity_without_polluting_clickhouse_selection, which reports that the Docker socket was not found. - Plannotator review completed with no changes requested. --- crates/clickhousectl/src/cloud/auth.rs | 110 ++++++++++++++++-- .../tests/cli_request_shape_test.rs | 95 +++++++++++++++ docs/development.md | 2 + 3 files changed, 200 insertions(+), 7 deletions(-) diff --git a/crates/clickhousectl/src/cloud/auth.rs b/crates/clickhousectl/src/cloud/auth.rs index d93fde57..09f74f5e 100644 --- a/crates/clickhousectl/src/cloud/auth.rs +++ b/crates/clickhousectl/src/cloud/auth.rs @@ -635,10 +635,48 @@ fn prompt_api_credentials() -> std::result::Result<(String, String), Box Result { + let parsed = url::Url::parse(value).map_err(|_| INVALID_AUTH_URL)?; + let loopback = match parsed.host() { + Some(url::Host::Domain(host)) => host.eq_ignore_ascii_case("localhost"), + Some(url::Host::Ipv4(ip)) => ip.is_loopback(), + Some(url::Host::Ipv6(ip)) => ip.is_loopback(), + None => return Err(INVALID_AUTH_URL), + }; + if !(parsed.scheme() == "https" || (parsed.scheme() == "http" && loopback)) + || !parsed.username().is_empty() + || parsed.password().is_some() + || parsed.query().is_some() + || parsed.fragment().is_some() + { + return Err(INVALID_AUTH_URL); + } + Ok(parsed.as_str().trim_end_matches('/').to_owned()) +} + +fn deserialize_auth_url<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let value = String::deserialize(deserializer)?; + validated_auth_url(&value).map_err(serde::de::Error::custom) +} + +/// OAuth form bodies contain credentials; never forward them via redirects. +fn oauth_client() -> Result { + crate::http::client_builder() + .redirect(reqwest::redirect::Policy::none()) + .build() +} + struct KnownAuthConfig { host: &'static str, auth_url: &'static str, @@ -709,9 +747,7 @@ fn auth_config_for_url_with(api_url: &str, env_lookup: EnvLookup<'_>) -> Option< env("CLICKHOUSECTL_CLIENT_ID"), ) { (Some(auth_url), Some(client_id)) => Some(AuthConfig { - // Trim trailing slashes so joining `/oauth/...` can't produce a - // rejected double slash (common when copying an issuer URL). - auth_url: auth_url.trim_end_matches('/').to_string(), + auth_url: validated_auth_url(&auth_url).ok()?, client_id, }), _ => None, @@ -860,8 +896,9 @@ pub async fn device_auth_login(api_url: &str) -> Result Result Result> { let config = &tokens.auth_config; - let client = crate::http::client_builder().build()?; + let client = oauth_client()?; let form_body = format!( "grant_type={}&client_id={}&refresh_token={}", @@ -1597,6 +1634,65 @@ mod tests { } } + #[test] + fn authorization_url_requires_https_except_for_loopback() { + for value in [ + "https://auth.example.com", + "https://auth.example.com/tenant", + "https://127.0.0.1:8443", + "http://localhost:8080", + "http://127.0.0.1:8080", + "http://127.255.255.254", + "http://[::1]:8080", + ] { + assert!(validated_auth_url(value).is_ok(), "{value}"); + } + for value in [ + "", + "not a URL", + "/relative", + "//auth.example.com", + "file:///tmp/auth", + "ftp://localhost", + "http://auth.example.com", + "http://0.0.0.0", + "http://192.168.1.1", + "http://[::]", + "http://[2001:db8::1]", + "http://localhost.evil.example", + "http://127.0.0.1.evil.example", + "http://localhost@evil.example", + "https://user:password@auth.example.com", + "https://auth.example.com?redirect=http://evil.example", + "https://auth.example.com#fragment", + ] { + assert!(validated_auth_url(value).is_err(), "{value}"); + assert!( + auth_config_for_url_with("https://api.example.com", &|key| { + Some( + if key == "CLICKHOUSECTL_AUTH_URL" { + value + } else { + "client" + } + .into(), + ) + }) + .is_none(), + "{value}" + ); + let mut record = serde_json::to_value(sample_tokens()).unwrap(); + record["auth_config"]["auth_url"] = serde_json::json!(value); + assert!( + serde_json::from_value::(record).is_err(), + "{value}" + ); + } + for known in KNOWN_CONFIGS { + assert!(validated_auth_url(known.auth_url).is_ok()); + } + } + #[test] fn auth_config_invalid_api_url_never_reads_environment() { for api_url in ["not a URL", "file:///tmp/api", "mailto:user@example.com"] { diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 3b53ef08..43bfcf4e 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -29878,6 +29878,101 @@ async fn old_oauth_records_require_login_even_for_known_hosts_and_fresh_tokens() assert!(mock.received_requests().await.unwrap().is_empty()); } +#[tokio::test] +async fn oauth_login_and_refresh_never_follow_redirects() { + let api = MockServer::start().await; + let destination = MockServer::start().await; + for status in [301, 302, 303, 307, 308] { + for login in [true, false] { + let issuer = MockServer::start().await; + let endpoint = if login { + "/oauth/device/code" + } else { + "/oauth/token" + }; + Mock::given(method("POST")) + .and(path(endpoint)) + .respond_with( + ResponseTemplate::new(status) + .insert_header("Location", format!("{}/stolen", destination.uri())), + ) + .expect(1) + .mount(&issuer) + .await; + let dir = tempfile::tempdir().unwrap(); + let mut command = custom_oauth_command(dir.path(), &api.uri()); + if login { + command + .env("CLICKHOUSECTL_AUTH_URL", issuer.uri()) + .env("CLICKHOUSECTL_CLIENT_ID", "client") + .args(["auth", "login"]); + } else { + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + std::fs::write(&token_path, serde_json::to_vec(&serde_json::json!({ + "access_token": "expired", "refresh_token": "secret-refresh", "expires_at": 1, + "api_url": api.uri(), + "auth_config": {"auth_url": issuer.uri(), "client_id": "client"} + })).unwrap()).unwrap(); + command.args(["auth", "whoami"]); + } + let output = command.output().unwrap(); + assert_eq!( + output.status.code(), + Some(if login { 1 } else { 4 }), + "{status}: {output:?}" + ); + assert!(destination.received_requests().await.unwrap().is_empty()); + } + } + assert!(api.received_requests().await.unwrap().is_empty()); +} + +#[tokio::test] +async fn oauth_rejects_insecure_authorization_config_before_requests() { + let api = MockServer::start().await; + for auth_url in [ + "http://auth.example.invalid", + "ftp://localhost", + "not-a-url", + ] { + let dir = tempfile::tempdir().unwrap(); + let output = custom_oauth_command(dir.path(), &api.uri()) + .env("CLICKHOUSECTL_AUTH_URL", auth_url) + .env("CLICKHOUSECTL_CLIENT_ID", "client") + .args(["auth", "login"]) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(1)); + assert!( + cloud_runtime_error(&output)["message"] + .as_str() + .unwrap() + .contains("HTTPS") + ); + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + assert!(!token_path.exists()); + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + std::fs::write( + &token_path, + serde_json::to_vec(&serde_json::json!({ + "access_token": "expired", "refresh_token": "secret-refresh", "expires_at": 1, + "api_url": api.uri(), + "auth_config": {"auth_url": auth_url, "client_id": "client"} + })) + .unwrap(), + ) + .unwrap(); + let output = custom_oauth_command(dir.path(), &api.uri()) + .args(["auth", "whoami"]) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(4)); + assert_eq!(cloud_runtime_error(&output)["code"], "auth_required"); + } + assert!(api.received_requests().await.unwrap().is_empty()); +} + #[tokio::test] async fn custom_oauth_login_pins_config_through_refresh_and_relogin() { let api = MockServer::start().await; diff --git a/docs/development.md b/docs/development.md index 0d2572ec..33910242 100644 --- a/docs/development.md +++ b/docs/development.md @@ -67,6 +67,8 @@ Cloud `--debug` prints the selected credential source and API URL. For a non-pro For OAuth login to an API host outside the built-in mappings, set `CLICKHOUSECTL_AUTH_URL` and `CLICKHOUSECTL_CLIENT_ID` when running `cloud auth login --url `; login saves the resolved configuration with the tokens, and refresh uses that saved configuration rather than the current environment. Built-in hosts ignore these overrides. Token files must include the complete configuration; older files without it require a new login, including for built-in hosts. Refresh never consults the built-in mappings or environment. +The authorization URL must use HTTPS; HTTP is allowed only for exact `localhost` or a loopback IP address (`127.0.0.0/8` or `::1`). Userinfo, query strings, and fragments are rejected. OAuth requests do not follow redirects, so configure the final authorization server URL. Saved authorization URLs are validated when loading tokens as well. + Keep tests isolated from real credentials and telemetry. Use the existing temporary-project and local-mock patterns in subprocess tests and set `DO_NOT_TRACK=1` for development checks. ## Telemetry changes From ef4d43cffacbf44a59fa4f566f3229227ccb400b Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 17:12:06 +0200 Subject: [PATCH 7/9] test(auth): require login for expired legacy production tokens Cover legacy production records with and without api_url in human, JSON, and agent output modes. Assert exit 4, auth_required in structured output, login guidance, unchanged token files, and no requests to the mock endpoint. Preserve the strict saved-configuration requirement rather than restoring built-in issuer fallback. --- .../tests/cli_request_shape_test.rs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 43bfcf4e..2d17f513 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -29878,6 +29878,53 @@ async fn old_oauth_records_require_login_even_for_known_hosts_and_fresh_tokens() assert!(mock.received_requests().await.unwrap().is_empty()); } +// Legacy production records cannot be refreshed: issuer configuration must be +// saved by a new login, not reconstructed from the built-in table or current env. +#[tokio::test] +async fn expired_legacy_production_tokens_require_login_instead_of_refresh() { + let mock = MockServer::start().await; + for mode in ["human", "explicit", "agent"] { + // The oldest production format also omitted api_url. + for includes_api_url in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let mut command = cloud_runtime_error_command(dir.path(), &mock.uri(), mode); + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + let mut legacy = serde_json::json!({ + "access_token": "expired-production-access", + "refresh_token": "legacy-production-refresh", + "expires_at": 1 + }); + if includes_api_url { + legacy["api_url"] = serde_json::json!("https://api.clickhouse.cloud/v1"); + } + let original = serde_json::to_vec(&legacy).unwrap(); + std::fs::write(&token_path, &original).unwrap(); + let output = command + .env_remove("CLICKHOUSE_CLOUD_API_KEY") + .env_remove("CLICKHOUSE_CLOUD_API_SECRET") + // No environment or built-in configuration may repair the record. + .env("CLICKHOUSECTL_AUTH_URL", mock.uri()) + .env("CLICKHOUSECTL_CLIENT_ID", "must-not-be-used") + .args(["auth", "whoami"]) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(4), "{mode}: {output:?}"); + assert!(output.stdout.is_empty()); + let message = if mode == "human" { + String::from_utf8_lossy(&output.stderr).into_owned() + } else { + let error = cloud_runtime_error(&output); + assert_eq!(error["code"], "auth_required"); + error["message"].as_str().unwrap().to_owned() + }; + assert!(message.contains("cloud auth login"), "{message}"); + assert_eq!(std::fs::read(&token_path).unwrap(), original); + } + } + assert!(mock.received_requests().await.unwrap().is_empty()); +} + #[tokio::test] async fn oauth_login_and_refresh_never_follow_redirects() { let api = MockServer::start().await; From b1bae8cff849ef72b4359a9cf228f8f6ad12b89f Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 17:25:04 +0200 Subject: [PATCH 8/9] fixup! feat(auth): allow OAuth device login against custom API targets Separate login configuration errors: auth_config_for_url_with now returns AuthConfigError so device_auth_login can distinguish env vars not set from an invalid CLICKHOUSECTL_AUTH_URL, carrying the validated_auth_url reason instead of discarding it, and naming the missing variable when only one is unset. --- crates/clickhousectl/src/cloud/auth.rs | 150 +++++++++++++++++++------ 1 file changed, 114 insertions(+), 36 deletions(-) diff --git a/crates/clickhousectl/src/cloud/auth.rs b/crates/clickhousectl/src/cloud/auth.rs index 09f74f5e..0b0744eb 100644 --- a/crates/clickhousectl/src/cloud/auth.rs +++ b/crates/clickhousectl/src/cloud/auth.rs @@ -728,16 +728,54 @@ fn known_auth_config_for_url(api_url: &str) -> Option { /// Injectable environment lookup keeps login tests independent of the shell. type EnvLookup<'a> = &'a dyn Fn(&str) -> Option; +/// Why a login could not resolve OAuth configuration for an API URL. +#[derive(Debug, Clone, PartialEq, Eq)] +enum AuthConfigError { + /// The API URL is malformed or has no host. + InvalidApiUrl, + /// Unknown host: the environment override is unset or empty. The payload + /// names the missing variable(s). + MissingEnvVars(&'static str), + /// `CLICKHOUSECTL_AUTH_URL` was set but rejected by `validated_auth_url`; + /// the payload is the validation reason. + InvalidAuthUrl(&'static str), +} + +impl std::fmt::Display for AuthConfigError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::InvalidApiUrl => write!(f, "the URL is malformed or has no host"), + Self::MissingEnvVars(missing) => { + let known = KNOWN_CONFIGS + .iter() + .map(|config| config.host) + .collect::>() + .join(", "); + write!(f, "known hosts: {known}; for any other host, set {missing}") + } + Self::InvalidAuthUrl(reason) => { + write!(f, "CLICKHOUSECTL_AUTH_URL is invalid: {reason}") + } + } + } +} + /// Resolve configuration for a new login, never for an existing session. -fn auth_config_for_url(api_url: &str) -> Option { +fn auth_config_for_url(api_url: &str) -> Result { auth_config_for_url_with(api_url, &|key| std::env::var(key).ok()) } -fn auth_config_for_url_with(api_url: &str, env_lookup: EnvLookup<'_>) -> Option { +fn auth_config_for_url_with( + api_url: &str, + env_lookup: EnvLookup<'_>, +) -> Result { // Preserve rejection of malformed or hostless API URLs before reading env. - url::Url::parse(api_url).ok()?.host_str()?; + let parsed = url::Url::parse(api_url).map_err(|_| AuthConfigError::InvalidApiUrl)?; + if parsed.host_str().is_none() { + return Err(AuthConfigError::InvalidApiUrl); + } if let Some(config) = known_auth_config_for_url(api_url) { - return Some(config); + return Ok(config); } // Unknown host: require both env vars, with no built-in fallback. @@ -746,11 +784,19 @@ fn auth_config_for_url_with(api_url: &str, env_lookup: EnvLookup<'_>) -> Option< env("CLICKHOUSECTL_AUTH_URL"), env("CLICKHOUSECTL_CLIENT_ID"), ) { - (Some(auth_url), Some(client_id)) => Some(AuthConfig { - auth_url: validated_auth_url(&auth_url).ok()?, - client_id, - }), - _ => None, + (Some(auth_url), Some(client_id)) => { + let auth_url = + validated_auth_url(&auth_url).map_err(AuthConfigError::InvalidAuthUrl)?; + Ok(AuthConfig { + auth_url, + client_id, + }) + } + (None, Some(_)) => Err(AuthConfigError::MissingEnvVars("CLICKHOUSECTL_AUTH_URL")), + (Some(_), None) => Err(AuthConfigError::MissingEnvVars("CLICKHOUSECTL_CLIENT_ID")), + (None, None) => Err(AuthConfigError::MissingEnvVars( + "CLICKHOUSECTL_AUTH_URL and CLICKHOUSECTL_CLIENT_ID", + )), } } @@ -894,19 +940,8 @@ pub fn is_token_valid(tokens: &TokenStore) -> bool { pub async fn device_auth_login(api_url: &str) -> Result> { let api_url = normalize_api_url(api_url); - let config = auth_config_for_url(&api_url).ok_or_else(|| { - format!( - "Cannot resolve OAuth configuration for API URL '{}'. Known hosts: {}. \ - For any other host, set CLICKHOUSECTL_AUTH_URL and CLICKHOUSECTL_CLIENT_ID. \ - {INVALID_AUTH_URL}.", - api_url, - KNOWN_CONFIGS - .iter() - .map(|config| config.host) - .collect::>() - .join(", ") - ) - })?; + let config = auth_config_for_url(&api_url) + .map_err(|error| format!("Cannot resolve OAuth configuration for '{api_url}': {error}"))?; let client = oauth_client()?; @@ -1549,10 +1584,15 @@ mod tests { #[test] fn test_auth_config_lookup() { let lookup = |url: &str| auth_config_for_url_with(url, &|_| None); - assert!(lookup("https://api.clickhouse.cloud/v1").is_some()); - assert!(lookup("https://api.control-plane.clickhouse-staging.com/v1").is_some()); - assert!(lookup("https://api.control-plane.clickhouse-dev.com/v1").is_some()); - assert!(lookup("https://api.unknown.com/v1").is_none()); + assert!(lookup("https://api.clickhouse.cloud/v1").is_ok()); + assert!(lookup("https://api.control-plane.clickhouse-staging.com/v1").is_ok()); + assert!(lookup("https://api.control-plane.clickhouse-dev.com/v1").is_ok()); + assert_eq!( + lookup("https://api.unknown.com/v1"), + Err(AuthConfigError::MissingEnvVars( + "CLICKHOUSECTL_AUTH_URL and CLICKHOUSECTL_CLIENT_ID" + )) + ); // Verify distinct configs without reading the process environment. let prod = lookup("https://api.clickhouse.cloud/v1").unwrap(); @@ -1563,9 +1603,33 @@ mod tests { assert_ne!(staging.client_id, dev.client_id); // Must not match known hosts by substring, regardless of the real env. - assert!(lookup("https://api.clickhouse.cloud.evil.com/v1").is_none()); - assert!(lookup("https://evil-api.clickhouse.cloud.attacker.com/v1").is_none()); - assert!(lookup("https://not-api.clickhouse-staging.com.bad.com/v1").is_none()); + assert!(lookup("https://api.clickhouse.cloud.evil.com/v1").is_err()); + assert!(lookup("https://evil-api.clickhouse.cloud.attacker.com/v1").is_err()); + assert!(lookup("https://not-api.clickhouse-staging.com.bad.com/v1").is_err()); + } + + #[test] + fn auth_config_unset_and_invalid_auth_url_are_distinct_errors() { + let api_url = "https://api.example.com/v1"; + + // Env vars simply not set: the error names the variables and never + // carries the URL-format advice. + let error = auth_config_for_url_with(api_url, &|_| None).unwrap_err(); + assert_eq!( + error, + AuthConfigError::MissingEnvVars("CLICKHOUSECTL_AUTH_URL and CLICKHOUSECTL_CLIENT_ID") + ); + assert!(!error.to_string().contains(INVALID_AUTH_URL), "{error}"); + + // Set but invalid: the error carries the validation reason. + let error = auth_config_for_url_with(api_url, &|key| match key { + "CLICKHOUSECTL_AUTH_URL" => Some("http://auth.example.com".to_owned()), + "CLICKHOUSECTL_CLIENT_ID" => Some("custom-client".to_owned()), + _ => panic!("unexpected environment lookup: {key}"), + }) + .unwrap_err(); + assert_eq!(error, AuthConfigError::InvalidAuthUrl(INVALID_AUTH_URL)); + assert!(error.to_string().contains(INVALID_AUTH_URL), "{error}"); } #[test] @@ -1612,7 +1676,10 @@ mod tests { "CLICKHOUSECTL_CLIENT_ID" => client_id.map(str::to_owned), _ => panic!("unexpected environment lookup: {key}"), }); - assert!(config.is_none(), "{auth_url:?}, {client_id:?}"); + assert!( + matches!(config, Err(AuthConfigError::MissingEnvVars(_))), + "{auth_url:?}, {client_id:?}" + ); } } @@ -1667,7 +1734,14 @@ mod tests { "https://auth.example.com#fragment", ] { assert!(validated_auth_url(value).is_err(), "{value}"); - assert!( + // An empty value is filtered out as unset before validation, so + // it reports the missing variable rather than an invalid URL. + let expected = if value.is_empty() { + AuthConfigError::MissingEnvVars("CLICKHOUSECTL_AUTH_URL") + } else { + AuthConfigError::InvalidAuthUrl(INVALID_AUTH_URL) + }; + assert_eq!( auth_config_for_url_with("https://api.example.com", &|key| { Some( if key == "CLICKHOUSECTL_AUTH_URL" { @@ -1678,7 +1752,8 @@ mod tests { .into(), ) }) - .is_none(), + .unwrap_err(), + expected, "{value}" ); let mut record = serde_json::to_value(sample_tokens()).unwrap(); @@ -1697,10 +1772,13 @@ mod tests { fn auth_config_invalid_api_url_never_reads_environment() { for api_url in ["not a URL", "file:///tmp/api", "mailto:user@example.com"] { assert!( - auth_config_for_url_with(api_url, &|_| { - panic!("invalid API URL must not read environment overrides") - }) - .is_none() + matches!( + auth_config_for_url_with(api_url, &|_| { + panic!("invalid API URL must not read environment overrides") + }), + Err(AuthConfigError::InvalidApiUrl) + ), + "{api_url}" ); } } From 6326128a404b204c3a734b31fdd84ce178b46e65 Mon Sep 17 00:00:00 2001 From: Emanuele De Cupis Date: Thu, 22 Oct 2026 23:27:49 +0200 Subject: [PATCH 9/9] fix(auth): require re-login for unusable saved OAuth tokens --- crates/clickhousectl/src/cloud/auth.rs | 139 +++++-- crates/clickhousectl/src/cloud/client.rs | 16 +- .../tests/cli_request_shape_test.rs | 364 ++++++++++++++---- docs/reference/authentication.md | 2 + 4 files changed, 402 insertions(+), 119 deletions(-) diff --git a/crates/clickhousectl/src/cloud/auth.rs b/crates/clickhousectl/src/cloud/auth.rs index 0b0744eb..9dfb65c8 100644 --- a/crates/clickhousectl/src/cloud/auth.rs +++ b/crates/clickhousectl/src/cloud/auth.rs @@ -301,8 +301,9 @@ pub async fn run( active: mark(AuthSource::CliFlags), }); + let mut oauth_warning = None; match load_tokens() { - Some(tokens) if is_token_valid(&tokens) => { + Ok(Some(tokens)) if is_token_valid(&tokens) => { rows.push(AuthRow { auth_type: "OAuth".into(), status: configured_status(AuthSource::OAuthTokens), @@ -310,7 +311,7 @@ pub async fn run( active: mark(AuthSource::OAuthTokens), }); } - Some(_) => { + Ok(Some(_)) => { rows.push(AuthRow { auth_type: "OAuth".into(), status: "Expired".into(), @@ -318,7 +319,16 @@ pub async fn run( active: "-".into(), }); } - None => { + Err(UnusableTokens) => { + oauth_warning = Some(OAUTH_LOGIN_REQUIRED); + rows.push(AuthRow { + auth_type: "OAuth".into(), + status: "Login required".into(), + scope: "-".into(), + active: "-".into(), + }); + } + Ok(None) => { rows.push(AuthRow { auth_type: "OAuth".into(), status: "Not configured".into(), @@ -409,6 +419,9 @@ pub async fn run( "[debug] auth source: none (incomplete CLI flags: missing {missing})" )); } + None if oauth_warning.is_some() => { + eprint_line("[debug] auth source: none (saved OAuth login unusable)"); + } None => eprint_line("[debug] auth source: none (no credentials configured)"), } } @@ -430,16 +443,23 @@ pub async fn run( #[derive(Serialize)] struct StatusOutput<'a> { sources: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + oauth_warning: Option<&'a str>, #[serde(flatten)] identity: &'a IdentityCheck, } let output = StatusOutput { sources: rows, + oauth_warning, identity: &identity, }; println!("{}", serde_json::to_string_pretty(&output)?); } else { println!("{}", Table::new(rows).with(Style::markdown())); + if let Some(warning) = oauth_warning { + println!(); + println!("{warning}"); + } match (&identity.identity, &identity.warning) { (Some(whoami), _) => { println!(); @@ -867,38 +887,51 @@ fn legacy_tokens_path() -> PathBuf { crate::init::local_dir().join("tokens.json") } -pub fn load_tokens() -> Option { - let global = tokens_path().ok()?; +/// A saved login could not be read or decoded. Never reconstruct its issuer +/// from the environment or built-in configuration; a new login is required. +#[derive(Debug)] +pub struct UnusableTokens; + +pub(super) const OAUTH_LOGIN_REQUIRED: &str = + "Saved OAuth login can't be used. Run `clickhousectl cloud auth login` to log in again."; + +pub fn load_tokens() -> Result, UnusableTokens> { + let global = tokens_path().map_err(|_| UnusableTokens)?; let legacy = legacy_tokens_path(); load_tokens_from(&global, &legacy) } -/// Path-injected core of [`load_tokens`]. Reads the global token file first; -/// if absent/unreadable, attempts a one-time migration from `legacy_path` -/// (write the global copy, best-effort delete the legacy file). +fn read_tokens(path: &std::path::Path) -> Result, UnusableTokens> { + match std::fs::read_to_string(path) { + Ok(data) => serde_json::from_str(&data) + .map(Some) + .map_err(|_| UnusableTokens), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(_) => Err(UnusableTokens), + } +} + +/// Path-injected core of [`load_tokens`]. Only an absent global file permits +/// migration from `legacy_path`. An unusable global file must not be silently +/// replaced by a different saved login. fn load_tokens_from( global_path: &std::path::Path, legacy_path: &std::path::Path, -) -> Option { - if let Ok(data) = std::fs::read_to_string(global_path) - && let Ok(tokens) = serde_json::from_str::(&data) - { - return Some(tokens); +) -> Result, UnusableTokens> { + if let Some(tokens) = read_tokens(global_path)? { + return Ok(Some(tokens)); } - // Global file missing or unreadable — try a one-time migration from the - // legacy cwd-based path. Best-effort: write the global copy, then delete - // the legacy file so it can't re-migrate on the next command. - if let Ok(data) = std::fs::read_to_string(legacy_path) - && let Ok(tokens) = serde_json::from_str::(&data) - { + // Best-effort migration: write the global copy, then delete the legacy + // file so it can't re-migrate on the next command. + if let Some(tokens) = read_tokens(legacy_path)? { if save_tokens_to(global_path, &tokens).is_ok() { let _ = std::fs::remove_file(legacy_path); } - return Some(tokens); + return Ok(Some(tokens)); } - None + Ok(None) } pub fn save_tokens(tokens: &TokenStore) -> Result<(), Box> { @@ -1102,7 +1135,9 @@ pub async fn refresh_access_token( /// If tokens exist and are near-expiry, refresh them. Returns Ok(()) even if /// no tokens are present (the user may be using API keys instead). pub async fn ensure_fresh_tokens(json: bool) -> Result<(), Box> { - let Some(tokens) = load_tokens() else { + // Leave unusable files intact. The credential resolver reports the re-login + // error only if no higher-precedence credential source wins. + let Ok(Some(tokens)) = load_tokens() else { return Ok(()); }; @@ -1366,7 +1401,7 @@ mod tests { }); std::fs::write(&global, serde_json::to_vec(&old).unwrap()).unwrap(); std::fs::write(&legacy, serde_json::to_vec(&old).unwrap()).unwrap(); - assert!(load_tokens_from(&global, &legacy).is_none()); + assert!(load_tokens_from(&global, &legacy).is_err()); } } @@ -1408,7 +1443,7 @@ mod tests { client_id: "client-a".into(), }; save_tokens_to(&legacy, &tokens).unwrap(); - let migrated = load_tokens_from(&global, &legacy).unwrap(); + let migrated = load_tokens_from(&global, &legacy).unwrap().unwrap(); assert_eq!(migrated.auth_config, tokens.auth_config); assert!(!legacy.exists()); @@ -1419,7 +1454,7 @@ mod tests { client_id: "client-b".into(), }; save_tokens_to(&global, &tokens).unwrap(); - let replaced = load_tokens_from(&global, &legacy).unwrap(); + let replaced = load_tokens_from(&global, &legacy).unwrap().unwrap(); assert_eq!(replaced.access_token, tokens.access_token); assert_eq!(replaced.refresh_token, tokens.refresh_token); assert_eq!(replaced.auth_config, tokens.auth_config); @@ -1504,7 +1539,9 @@ mod tests { let tokens = sample_tokens(); std::fs::write(&legacy, serde_json::to_string_pretty(&tokens).unwrap()).unwrap(); - let loaded = load_tokens_from(&global, &legacy).expect("legacy file should migrate"); + let loaded = load_tokens_from(&global, &legacy) + .expect("legacy file should migrate") + .unwrap(); assert_eq!(loaded.access_token, "a"); // Migration writes the global copy and deletes the legacy file. @@ -1542,7 +1579,9 @@ mod tests { ) .unwrap(); - let loaded = load_tokens_from(&global, &legacy).expect("global file should load"); + let loaded = load_tokens_from(&global, &legacy) + .expect("global file should load") + .unwrap(); assert_eq!(loaded.access_token, "global-wins"); assert!(legacy.exists(), "legacy file must not be touched"); } @@ -1552,19 +1591,49 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let global = tmp.path().join("global/.clickhouse/tokens.json"); let legacy = tmp.path().join("legacy/.clickhouse/tokens.json"); - assert!(load_tokens_from(&global, &legacy).is_none()); + assert!(load_tokens_from(&global, &legacy).unwrap().is_none()); } #[test] - fn load_tokens_returns_none_when_both_unparseable() { + fn load_tokens_reports_unusable_files_without_modifying_them() { + for location in ["global", "legacy"] { + for data in [b"not json".as_slice(), b"{}", b"\xff"] { + let tmp = tempfile::tempdir().unwrap(); + let global = tmp.path().join("global.json"); + let legacy = tmp.path().join("legacy.json"); + let path = if location == "global" { + &global + } else { + &legacy + }; + std::fs::write(path, data).unwrap(); + assert!(load_tokens_from(&global, &legacy).is_err()); + assert_eq!(std::fs::read(path).unwrap(), data); + } + } + } + + #[test] + fn unusable_global_tokens_do_not_fall_back_to_valid_legacy_tokens() { let tmp = tempfile::tempdir().unwrap(); - let global = tmp.path().join("global/.clickhouse/tokens.json"); - let legacy = tmp.path().join("legacy/.clickhouse/tokens.json"); - std::fs::create_dir_all(global.parent().unwrap()).unwrap(); - std::fs::create_dir_all(legacy.parent().unwrap()).unwrap(); + let global = tmp.path().join("global.json"); + let legacy = tmp.path().join("legacy.json"); std::fs::write(&global, b"not json").unwrap(); - std::fs::write(&legacy, b"also not json").unwrap(); - assert!(load_tokens_from(&global, &legacy).is_none()); + save_tokens_to(&legacy, &sample_tokens()).unwrap(); + let original_legacy = std::fs::read(&legacy).unwrap(); + assert!(load_tokens_from(&global, &legacy).is_err()); + assert_eq!(std::fs::read(&global).unwrap(), b"not json"); + assert_eq!(std::fs::read(&legacy).unwrap(), original_legacy); + } + + #[test] + fn unreadable_token_path_requires_login() { + let tmp = tempfile::tempdir().unwrap(); + let global = tmp.path().join("global.json"); + let legacy = tmp.path().join("legacy.json"); + // A directory reliably fails read_to_string even when tests run as root. + std::fs::create_dir(&global).unwrap(); + assert!(load_tokens_from(&global, &legacy).is_err()); } #[test] diff --git a/crates/clickhousectl/src/cloud/client.rs b/crates/clickhousectl/src/cloud/client.rs index 7a54a5d2..84bdeaff 100644 --- a/crates/clickhousectl/src/cloud/client.rs +++ b/crates/clickhousectl/src/cloud/client.rs @@ -172,7 +172,10 @@ type CredentialsLookup<'a> = &'a dyn Fn() -> Option = &'a dyn Fn() -> Option; +type TokensLookup<'a> = &'a dyn Fn() -> std::result::Result< + Option, + crate::cloud::auth::UnusableTokens, +>; /// Treat empty as absent. An exported-but-empty variable (`CLICKHOUSE_..=`) /// or a bare `KEY=` line in `.env` yields `Some("")`; collapsing it to `None` @@ -266,7 +269,9 @@ fn resolve_auth_with_sources( }); } - if let Some(tokens) = load_tokens() + let tokens = + load_tokens().map_err(|_| CloudError::auth(crate::cloud::auth::OAUTH_LOGIN_REQUIRED))?; + if let Some(tokens) = tokens && crate::cloud::auth::is_token_valid(&tokens) { let base_url = url_override @@ -1569,8 +1574,11 @@ mod tests { None } - fn no_tokens() -> Option { - None + fn no_tokens() -> std::result::Result< + Option, + crate::cloud::auth::UnusableTokens, + > { + Ok(None) } // A populated credentials file, for asserting the file tier wins over env. diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 2d17f513..58eab68c 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -29697,89 +29697,101 @@ async fn expired_custom_host_tokens_do_not_block_api_key_auth() { #[tokio::test] async fn api_key_precedence_skips_oauth_refresh_and_preserves_tokens() { - for source in ["flags", "file", "env", "dotenv", "mixed", "partial-flags"] { - let api = MockServer::start().await; - let issuer = MockServer::start().await; - // If refresh is accidentally attempted it would reject and clear tokens. - Mock::given(method("POST")) - .and(path("/oauth/token")) - .respond_with(ResponseTemplate::new(401)) - .expect(0) - .mount(&issuer) - .await; - Mock::given(method("GET")) - .and(path("/v1/organizations/org-1/services")) - .and(wiremock::matchers::basic_auth( - "winning-key", - "winning-secret", - )) - .respond_with( - ResponseTemplate::new(200).set_body_json(serde_json::json!({"result": []})), - ) - .expect(if source == "partial-flags" { 0 } else { 1 }) - .mount(&api) - .await; - let dir = tempfile::tempdir().unwrap(); - let mut command = custom_oauth_command(dir.path(), &api.uri()); - let token_path = dir.path().join("home/.clickhouse/tokens.json"); - std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); - let original = serde_json::to_vec(&serde_json::json!({ - "access_token": "expired", "refresh_token": "preserve-refresh", "expires_at": 1, - "api_url": api.uri(), - "auth_config": {"auth_url": issuer.uri(), "client_id": "saved-client"} - })) - .unwrap(); - std::fs::write(&token_path, &original).unwrap(); - match source { - "flags" => { - // Populate lower tiers too, proving the same precedence as the client. - write_project_api_credentials(dir.path(), "ignored-file", "ignored-secret"); - command - .env("CLICKHOUSE_CLOUD_API_KEY", "ignored-env") - .env("CLICKHOUSE_CLOUD_API_SECRET", "ignored-secret") - .args(["--api-key", "winning-key", "--api-secret", "winning-secret"]); - } - "file" => { - write_project_api_credentials(dir.path(), "winning-key", "winning-secret"); - command - .env("CLICKHOUSE_CLOUD_API_KEY", "ignored-env") - .env("CLICKHOUSE_CLOUD_API_SECRET", "ignored-secret"); - } - "env" => { - command - .env("CLICKHOUSE_CLOUD_API_KEY", "winning-key") - .env("CLICKHOUSE_CLOUD_API_SECRET", "winning-secret"); - } - "dotenv" => { - std::fs::write(dir.path().join(".env"), "CLICKHOUSE_CLOUD_API_KEY=winning-key\nCLICKHOUSE_CLOUD_API_SECRET=winning-secret\n").unwrap(); - } - "mixed" => { - command.env("CLICKHOUSE_CLOUD_API_KEY", "winning-key"); - std::fs::write( - dir.path().join(".env"), - "CLICKHOUSE_CLOUD_API_SECRET=winning-secret\n", + for format in ["current", "old", "malformed"] { + for source in ["flags", "file", "env", "dotenv", "mixed", "partial-flags"] { + let api = MockServer::start().await; + let issuer = MockServer::start().await; + // If refresh is accidentally attempted it would reject and clear tokens. + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(401)) + .expect(0) + .mount(&issuer) + .await; + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1/services")) + .and(wiremock::matchers::basic_auth( + "winning-key", + "winning-secret", + )) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!({"result": []})), ) - .unwrap(); + .expect(if source == "partial-flags" { 0 } else { 1 }) + .mount(&api) + .await; + let dir = tempfile::tempdir().unwrap(); + let mut command = custom_oauth_command(dir.path(), &api.uri()); + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + let mut record = serde_json::json!({ + "access_token": "expired", "refresh_token": "preserve-refresh", "expires_at": 1, + "api_url": api.uri(), + "auth_config": {"auth_url": issuer.uri(), "client_id": "saved-client"} + }); + if format == "old" { + record.as_object_mut().unwrap().remove("auth_config"); } - "partial-flags" => { - command.arg("--api-key").arg("winning-key"); + let original = if format == "malformed" { + b"not json".to_vec() + } else { + serde_json::to_vec(&record).unwrap() + }; + std::fs::write(&token_path, &original).unwrap(); + match source { + "flags" => { + // Populate lower tiers too, proving the same precedence as the client. + write_project_api_credentials(dir.path(), "ignored-file", "ignored-secret"); + command + .env("CLICKHOUSE_CLOUD_API_KEY", "ignored-env") + .env("CLICKHOUSE_CLOUD_API_SECRET", "ignored-secret") + .args(["--api-key", "winning-key", "--api-secret", "winning-secret"]); + } + "file" => { + write_project_api_credentials(dir.path(), "winning-key", "winning-secret"); + command + .env("CLICKHOUSE_CLOUD_API_KEY", "ignored-env") + .env("CLICKHOUSE_CLOUD_API_SECRET", "ignored-secret"); + } + "env" => { + command + .env("CLICKHOUSE_CLOUD_API_KEY", "winning-key") + .env("CLICKHOUSE_CLOUD_API_SECRET", "winning-secret"); + } + "dotenv" => { + std::fs::write(dir.path().join(".env"), "CLICKHOUSE_CLOUD_API_KEY=winning-key\nCLICKHOUSE_CLOUD_API_SECRET=winning-secret\n").unwrap(); + } + "mixed" => { + command.env("CLICKHOUSE_CLOUD_API_KEY", "winning-key"); + std::fs::write( + dir.path().join(".env"), + "CLICKHOUSE_CLOUD_API_SECRET=winning-secret\n", + ) + .unwrap(); + } + "partial-flags" => { + command.arg("--api-key").arg("winning-key"); + } + _ => unreachable!(), } - _ => unreachable!(), + let output = command + .args(["service", "list", "--org-id", "org-1"]) + .output() + .unwrap(); + assert_eq!( + output.status.code(), + Some(if source == "partial-flags" { 4 } else { 0 }), + "{source}: {output:?}" + ); + assert_eq!(std::fs::read(&token_path).unwrap(), original, "{source}"); + assert!( + issuer.received_requests().await.unwrap().is_empty(), + "{source}" + ); + // Replenish Tokio's cooperative budget before wiremock's synchronous + // drop verification acquires its async lock on the next iteration. + tokio::task::yield_now().await; } - let output = command - .args(["service", "list", "--org-id", "org-1"]) - .output() - .unwrap(); - assert_eq!( - output.status.code(), - Some(if source == "partial-flags" { 4 } else { 0 }), - "{source}: {output:?}" - ); - assert_eq!(std::fs::read(&token_path).unwrap(), original, "{source}"); - assert!( - issuer.received_requests().await.unwrap().is_empty(), - "{source}" - ); } } @@ -29918,13 +29930,205 @@ async fn expired_legacy_production_tokens_require_login_instead_of_refresh() { assert_eq!(error["code"], "auth_required"); error["message"].as_str().unwrap().to_owned() }; - assert!(message.contains("cloud auth login"), "{message}"); + assert!( + message.contains("Saved OAuth login can't be used"), + "{message}" + ); + assert!( + message.contains("`clickhousectl cloud auth login` to log in again"), + "{message}" + ); + assert!(!message.contains("No credentials found"), "{message}"); + assert!(!message.contains("--api-key"), "{message}"); assert_eq!(std::fs::read(&token_path).unwrap(), original); } } assert!(mock.received_requests().await.unwrap().is_empty()); } +#[tokio::test] +async fn unusable_oauth_login_is_distinct_from_missing_credentials() { + let mock = MockServer::start().await; + for mode in ["human", "explicit", "agent"] { + for format in ["absent", "old", "incomplete", "malformed"] { + for location in ["global", "legacy"] { + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join(if location == "global" { + "home/.clickhouse/tokens.json" + } else { + ".clickhouse/tokens.json" + }); + let original = match format { + "absent" => None, + "old" => Some( + serde_json::to_vec(&serde_json::json!({ + "access_token": "private-access", "refresh_token": "private-refresh", + "expires_at": 4102444800_i64, "api_url": mock.uri() + })) + .unwrap(), + ), + "incomplete" => Some( + serde_json::to_vec(&serde_json::json!({ + "access_token": "private-access", "refresh_token": "private-refresh", + "expires_at": 4102444800_i64, "api_url": mock.uri(), + "auth_config": {"auth_url": mock.uri()} + })) + .unwrap(), + ), + "malformed" => Some(b"not json: private-access".to_vec()), + _ => unreachable!(), + }; + if let Some(data) = &original { + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + std::fs::write(&token_path, data).unwrap(); + } + let command = || { + let mut cmd = cloud_runtime_error_command(dir.path(), &mock.uri(), mode); + cmd.env_remove("CLICKHOUSE_CLOUD_API_KEY") + .env_remove("CLICKHOUSE_CLOUD_API_SECRET") + .env("CLICKHOUSECTL_AUTH_URL", mock.uri()) + .env("CLICKHOUSECTL_CLIENT_ID", "must-not-be-used"); + cmd + }; + let output = command().args(["auth", "status"]).output().unwrap(); + assert_success(&output); + let stdout = String::from_utf8(output.stdout).unwrap(); + let expected = if format == "absent" { + "Not configured" + } else { + "Login required" + }; + if mode == "human" { + let row = stdout + .lines() + .find(|line| line.contains("| OAuth")) + .unwrap(); + let cells: Vec<_> = row.trim_matches('|').split('|').map(str::trim).collect(); + assert_eq!(cells, ["OAuth", expected, "-", "-"]); + } else { + let status: Value = serde_json::from_str(&stdout).unwrap(); + let row = status["sources"] + .as_array() + .unwrap() + .iter() + .find(|row| row["type"] == "OAuth") + .unwrap(); + assert_eq!(row["status"], expected); + assert_eq!(row["scope"], "-"); + assert_eq!(row["active"], "-"); + assert_eq!(status["verification"], "skipped"); + assert!(status.get("warning").is_none()); + if format == "absent" { + assert!(status.get("oauth_warning").is_none()); + } else { + let warning = status["oauth_warning"].as_str().unwrap(); + assert!(warning.contains("Saved OAuth login can't be used")); + assert!( + warning.contains("`clickhousectl cloud auth login` to log in again") + ); + } + } + if format == "absent" { + assert!(!stdout.contains("log in again")); + } else { + assert!(stdout.contains("Saved OAuth login can't be used")); + assert!(stdout.contains("`clickhousectl cloud auth login` to log in again")); + } + let output = command() + .args(["service", "list", "--org-id", "org-1"]) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(4), "{output:?}"); + assert!(output.stdout.is_empty()); + let message = if mode == "human" { + String::from_utf8_lossy(&output.stderr).into_owned() + } else { + let error = cloud_runtime_error(&output); + assert_eq!(error["code"], "auth_required"); + error["message"].as_str().unwrap().to_owned() + }; + if format == "absent" { + assert!(message.contains("No credentials found"), "{message}"); + assert!(!message.contains("log in again"), "{message}"); + } else { + assert!( + message.contains("Saved OAuth login can't be used"), + "{message}" + ); + assert!( + message.contains("`clickhousectl cloud auth login` to log in again"), + "{message}" + ); + assert!(!message.contains("No credentials found"), "{message}"); + assert!(!message.contains("--api-key"), "{message}"); + } + for secret in ["private-access", "private-refresh"] { + assert!(!stdout.contains(secret)); + assert!(!String::from_utf8_lossy(&output.stderr).contains(secret)); + } + if let Some(data) = original { + assert_eq!(std::fs::read(&token_path).unwrap(), data); + if location == "legacy" { + assert!(!dir.path().join("home/.clickhouse/tokens.json").exists()); + } + } + } + } + } + assert!(mock.received_requests().await.unwrap().is_empty()); +} + +#[tokio::test] +async fn auth_status_reports_unusable_oauth_alongside_api_key_identity() { + for mode in ["human", "explicit", "agent"] { + let mock = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/v1/whoami")) + .and(wiremock::matchers::basic_auth("test-key", "test-secret")) + .respond_with(ResponseTemplate::new(401).set_body_json(serde_json::json!({ + "status": 401, "error": "key rejected" + }))) + .expect(1) + .mount(&mock) + .await; + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join("home/.clickhouse/tokens.json"); + std::fs::create_dir_all(token_path.parent().unwrap()).unwrap(); + std::fs::write(&token_path, b"not json").unwrap(); + let output = cloud_runtime_error_command(dir.path(), &mock.uri(), mode) + .args(["auth", "status", "--debug"]) + .output() + .unwrap(); + assert_success(&output); + let stdout = String::from_utf8(output.stdout).unwrap(); + assert!(stdout.contains("Saved OAuth login can't be used")); + assert!(stdout.contains("`clickhousectl cloud auth login` to log in again")); + if mode == "human" { + assert!(stdout.contains("Identity: rejected")); + } else { + let status: Value = serde_json::from_str(&stdout).unwrap(); + assert_eq!(status["verification"], "rejected"); + assert!(status["warning"].as_str().unwrap().contains("key rejected")); + assert!( + status["oauth_warning"] + .as_str() + .unwrap() + .contains("log in again") + ); + let sources = status["sources"].as_array().unwrap(); + let env = sources + .iter() + .find(|row| row["type"] == "Env vars") + .unwrap(); + assert_eq!(env["active"], "yes"); + let oauth = sources.iter().find(|row| row["type"] == "OAuth").unwrap(); + assert_eq!(oauth["status"], "Login required"); + assert_eq!(oauth["active"], "-"); + } + assert_eq!(std::fs::read(&token_path).unwrap(), b"not json"); + } +} + #[tokio::test] async fn oauth_login_and_refresh_never_follow_redirects() { let api = MockServer::start().await; diff --git a/docs/reference/authentication.md b/docs/reference/authentication.md index ad1838cc..bfc02e4f 100644 --- a/docs/reference/authentication.md +++ b/docs/reference/authentication.md @@ -21,6 +21,8 @@ clickhousectl cloud auth status `auth status` shows configured sources, the active source, and identity verification. It can exit 0 with no active credentials, rejected credentials, or an unavailable identity check; use `whoami` or the intended command to test access. Its remote check times out after five seconds. +If a saved OAuth login cannot be read or uses an older format without complete authorization configuration, `auth status` shows OAuth as `Login required`, not `Not configured`, and explains the problem below the table with the command to log in again. JSON output includes this explanation in `oauth_warning`, separate from any identity-verification `warning`; it is shown even when an API key is active. Run `clickhousectl cloud auth login` to log in again. Without a higher-priority API key source, Cloud commands exit 4 (`auth_required` in JSON) with an explicit re-login message. The unusable file is left unchanged; the CLI does not reconstruct its authorization configuration from built-in defaults or environment variables. + For automation, inject `CLICKHOUSE_CLOUD_API_KEY` and `CLICKHOUSE_CLOUD_API_SECRET` through a secret manager. The CLI does not save environment credentials. For development it also reads `.env` from the exact current directory; keep that file out of version control and restrict access. Flags are available, but command-line secrets can appear in shell history and process listings. ## Credential precedence