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 70bf8122..b5bb6df2 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -28743,7 +28743,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!({ @@ -28759,13 +28762,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 {