diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index ea495d5c04..485848611a 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -4962,17 +4962,39 @@ pub async fn provider_update(options: ProviderUpdateOptions<'_>) -> Result<()> { } let mut client = grpc_client(server, tls).await?; + + // Look up the stored provider so the update can carry its type and profile + // workspace. Policy interceptors evaluate the request before the gateway + // merges it with stored state, so an update that omits them cannot be + // authorized against the profile that owns the provider. + // + // The read is best-effort. A caller holding `provider:write` without + // `provider:read` must still be able to rotate credentials, so a denied + // read keeps the previous behavior of sending empty metadata rather than + // failing the update. `--from-existing` and `--from-oidc-token` need the + // stored type, so they surface the error instead. + let existing = match client + .get_provider(GetProviderRequest { + name: name.to_string(), + workspace: workspace.to_string(), + }) + .await + { + Ok(response) => response.into_inner().provider, + Err(status) + if status.code() == Code::PermissionDenied && !from_existing && !from_oidc_token => + { + None + } + Err(status) => return Err(status).into_diagnostic(), + }; + + if existing.is_none() && (from_existing || from_oidc_token) { + return Err(miette::miette!("provider '{name}' not found")); + } + let oidc_profile = if from_oidc_token { - let existing = client - .get_provider(GetProviderRequest { - name: name.to_string(), - workspace: workspace.to_string(), - }) - .await - .into_diagnostic()? - .into_inner() - .provider - .ok_or_else(|| miette::miette!("provider '{name}' not found"))?; + let existing = existing.as_ref().expect("checked above"); Some( fetch_provider_profile(&mut client, &existing.r#type, &existing.profile_workspace) .await?, @@ -4991,25 +5013,11 @@ pub async fn provider_update(options: ProviderUpdateOptions<'_>) -> Result<()> { credential_expires_at_ms.extend(oidc_credential_expires_at_ms); if from_existing { - // Fetch the existing provider to discover its type for credential lookup. - let existing = client - .get_provider(GetProviderRequest { - name: name.to_string(), - workspace: workspace.to_string(), - }) - .await - .into_diagnostic()? - .into_inner() - .provider - .ok_or_else(|| miette::miette!("provider '{name}' not found"))?; - - let provider_type = existing.r#type; - let discovered = discover_existing_provider_data( - &mut client, - &provider_type, - &existing.profile_workspace, - ) - .await?; + let stored = existing.as_ref().expect("checked above"); + let provider_type = stored.r#type.clone(); + let discovered = + discover_existing_provider_data(&mut client, &provider_type, &stored.profile_workspace) + .await?; let Some(discovered) = discovered else { return Err(miette::miette!( "no existing local credentials/config found for provider type '{provider_type}'" @@ -5037,11 +5045,17 @@ pub async fn provider_update(options: ProviderUpdateOptions<'_>) -> Result<()> { workspace: workspace.to_string(), deletion_timestamp_ms: 0, }), - r#type: String::new(), + r#type: existing + .as_ref() + .map(|provider| provider.r#type.clone()) + .unwrap_or_default(), credentials: credential_map, config: config_map, credential_expires_at_ms: HashMap::new(), - profile_workspace: String::new(), + profile_workspace: existing + .as_ref() + .map(|provider| provider.profile_workspace.clone()) + .unwrap_or_default(), credential_handles: HashMap::new(), }), credential_expires_at_ms, diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index d198c1e37a..3849df7a69 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -1202,6 +1202,59 @@ async fn install_test_profile(ts: &TestServer, id: &str, credential_key: &str) { ); } +/// A readable provider must carry its stored type and profile workspace into +/// the update request. Policy interceptors evaluate the request before the +/// gateway merges it with stored state, so an update that omits them cannot be +/// authorized against the profile that owns the provider. +/// +/// The stored `profile_workspace` is forwarded verbatim rather than recomputed +/// from the request workspace. The gateway treats it as immutable, so deriving +/// it here would look like a change and be rejected. +#[tokio::test] +async fn provider_update_preserves_stored_type_and_profile_workspace_when_readable() { + let ts = run_server().await; + + run::provider_create( + &ts.endpoint, + "my-claude", + "claude", + false, + &["API_KEY=abc".to_string()], + false, + &[], + "default", + &ts.tls, + ) + .await + .expect("provider create"); + + run::provider_update(run::ProviderUpdateOptions { + server: &ts.endpoint, + name: "my-claude", + from_existing: false, + from_oidc_token: false, + credentials: &["API_KEY=rotated".to_string()], + config: &[], + credential_expires_at: &[], + workspace: "default", + tls: &ts.tls, + }) + .await + .expect("provider update"); + + let requests = ts.state.provider_update_requests.lock().await; + let request = requests.last().expect("provider update request"); + // `claude` normalizes to the canonical `claude-code` at creation, so the + // update carries the stored type rather than the alias the caller typed. + assert_eq!(request.r#type, "claude-code"); + // Forwarded verbatim rather than recomputed. The gateway treats + // profile_workspace as immutable, so any substitution here would look like + // a change and be rejected. + let stored = ts.state.providers.lock().await; + let stored = stored.get("my-claude").expect("stored provider"); + assert_eq!(request.profile_workspace, stored.profile_workspace); +} + #[tokio::test] async fn provider_cli_run_functions_support_full_crud_flow() { let ts = run_server().await;