From 1dfe430d50d084943a9b3d40f60aa16609da07e8 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 10:38:51 -0400 Subject: [PATCH 1/9] feat(provider): implement scoped provider name uniqueness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow multiple users to create providers with the same visible name by storing them with an owner-prefixed DB key ({owner}/{name}). Resolution uses two-pass lookup: try owned first, fall back to shared. Key changes: - ownership.rs: scoped_name, display_name, owner_prefix, scoped_name_for_principal, resolve_scoped_name helpers + unit tests - provider.rs: create stamps scoped key, get/update/delete resolve via two-pass, list filters by owner, responses strip prefix - sandbox.rs: spec.providers stores DB keys, attach/detach resolve user-visible names, all response paths strip prefixes Zero DB migration required — existing partial unique index on (object_type, name) already supports scoped keys. Closes: kagenti/kagenti#1996 Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 160 ++++++ crates/openshell-server/src/grpc/provider.rs | 465 +++++++++++++++--- crates/openshell-server/src/grpc/sandbox.rs | 129 +++-- 3 files changed, 641 insertions(+), 113 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index a4e1d1c06..1adcbdb1b 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -166,6 +166,109 @@ fn is_valid_label_value(value: &str) -> bool { .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') } +// --------------------------------------------------------------------------- +// Scoped name helpers +// --------------------------------------------------------------------------- + +/// Separator between owner and object name in scoped DB keys. +const SCOPE_SEPARATOR: char = '/'; + +/// Build a scoped DB key: `"{owner}/{name}"`. +/// +/// The owner segment is the sanitized subject (UUID or hex hash), guaranteed to +/// contain no `/`. The user-visible name may contain `/` only if we ever allow +/// it (currently validation rejects it), but the *first* `/` is always the +/// owner boundary. +pub fn scoped_name(owner: &str, name: &str) -> String { + format!("{owner}{SCOPE_SEPARATOR}{name}") +} + +/// Extract the user-visible name from a potentially scoped DB key. +/// +/// If the key contains a `/`, the part after the first `/` is the display name. +/// If no `/`, the key *is* the display name (shared/legacy provider). +pub fn display_name(db_key: &str) -> &str { + match db_key.find(SCOPE_SEPARATOR) { + Some(pos) => &db_key[pos + 1..], + None => db_key, + } +} + +/// Extract the owner prefix from a scoped DB key, if present. +pub fn owner_prefix(db_key: &str) -> Option<&str> { + db_key.find(SCOPE_SEPARATOR).map(|pos| &db_key[..pos]) +} + +/// Build the scoped DB key for the given principal, or return the raw name for +/// anonymous/admin callers. +pub fn scoped_name_for_principal( + name: &str, + principal: Option<&Principal>, + admin_role: &str, +) -> Result { + let Some(identity) = principal_identity(principal) else { + return Ok(name.to_string()); + }; + + if !admin_role.is_empty() && identity.roles.iter().any(|r| r == admin_role) { + return Ok(name.to_string()); + } + + let owner_value = sanitize_subject(&identity.subject)?; + Ok(scoped_name(&owner_value, name)) +} + +/// Resolve a provider name with owner-scoped fallback. +/// +/// Resolution order: +/// 1. Try `{owner}/{name}` (user's own provider) +/// 2. Fall back to `{name}` (shared provider, no owner prefix) +/// +/// Admin callers and anonymous principals resolve the raw name directly. +pub async fn resolve_scoped_name( + store: &crate::persistence::Store, + object_type: &str, + name: &str, + principal: Option<&Principal>, + admin_role: &str, +) -> Result, Status> { + let Some(identity) = principal_identity(principal) else { + // Anonymous/none → direct lookup (backward compat) + return store + .get_by_name(object_type, name) + .await + .map_err(|e| Status::internal(format!("fetch by name failed: {e}"))); + }; + + // Admin with explicit scoped name (contains '/') → direct lookup + let is_admin = !admin_role.is_empty() && identity.roles.iter().any(|r| r == admin_role); + if is_admin && name.contains(SCOPE_SEPARATOR) { + return store + .get_by_name(object_type, name) + .await + .map_err(|e| Status::internal(format!("fetch by name failed: {e}"))); + } + + // Non-admin (or admin without explicit scope): try owned first + if !is_admin { + let owner_value = sanitize_subject(&identity.subject)?; + let owned_key = scoped_name(&owner_value, name); + let owned = store + .get_by_name(object_type, &owned_key) + .await + .map_err(|e| Status::internal(format!("fetch owned provider failed: {e}")))?; + if owned.is_some() { + return Ok(owned); + } + } + + // Fall back to shared (unscoped) name + store + .get_by_name(object_type, name) + .await + .map_err(|e| Status::internal(format!("fetch shared provider failed: {e}"))) +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -370,4 +473,61 @@ mod tests { let result = owner_selector(None, "", "openshell-admin").unwrap(); assert_eq!(result, ""); } + + // ---- scoped name helpers ---- + + #[test] + fn scoped_name_construction() { + assert_eq!(scoped_name("alice-uuid-1234", "openai"), "alice-uuid-1234/openai"); + } + + #[test] + fn display_name_strips_owner() { + assert_eq!(display_name("alice-uuid-1234/openai"), "openai"); + } + + #[test] + fn display_name_shared_unchanged() { + assert_eq!(display_name("openai"), "openai"); + } + + #[test] + fn owner_prefix_extracts_owner() { + assert_eq!(owner_prefix("alice-uuid-1234/openai"), Some("alice-uuid-1234")); + } + + #[test] + fn owner_prefix_none_for_shared() { + assert_eq!(owner_prefix("openai"), None); + } + + #[test] + fn scoped_name_for_principal_user() { + let principal = alice(); + let result = + scoped_name_for_principal("openai", Some(&principal), "openshell-admin").unwrap(); + assert_eq!(result, "alice-uuid-1234/openai"); + } + + #[test] + fn scoped_name_for_principal_admin_unscoped() { + let principal = admin_principal(); + let result = + scoped_name_for_principal("openai", Some(&principal), "openshell-admin").unwrap(); + assert_eq!(result, "openai"); + } + + #[test] + fn scoped_name_for_principal_anonymous_unscoped() { + let result = + scoped_name_for_principal("openai", Some(&Principal::Anonymous), "openshell-admin") + .unwrap(); + assert_eq!(result, "openai"); + } + + #[test] + fn scoped_name_for_principal_none_unscoped() { + let result = scoped_name_for_principal("openai", None, "openshell-admin").unwrap(); + assert_eq!(result, "openai"); + } } diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 1949d5eb5..8acd57b04 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -5,6 +5,8 @@ #![allow(clippy::result_large_err)] // gRPC handlers return Result, Status> +use std::sync::Arc; + use crate::persistence::{ ObjectId, ObjectLabels, ObjectName, ObjectType, Store, WriteCondition, generate_name, }; @@ -48,6 +50,28 @@ fn check_provider_owner( crate::auth::ownership::check_owner(labels, principal, admin_role_name(state)) } +/// Resolve a provider by user-visible name, decode it, and verify ownership. +/// Returns the full Provider (with scoped DB name in metadata.name). +async fn resolve_and_check_provider( + state: &Arc, + user_name: &str, + principal: Option<&crate::auth::principal::Principal>, +) -> Result { + let record = crate::auth::ownership::resolve_scoped_name( + state.store.as_ref(), + Provider::object_type(), + user_name, + principal, + admin_role_name(state), + ) + .await? + .ok_or_else(|| Status::not_found("provider not found"))?; + let provider = Provider::decode(record.payload.as_slice()) + .map_err(|e| Status::internal(format!("decode provider failed: {e}")))?; + check_provider_owner(&provider, principal, state)?; + Ok(provider) +} + // --------------------------------------------------------------------------- // CRUD helpers // --------------------------------------------------------------------------- @@ -63,6 +87,20 @@ fn redact_provider_credentials(mut provider: Provider) -> Provider { provider } +/// Strip the owner prefix from a provider's metadata.name for display. +/// DB stores `{owner}/{name}` but users see just `{name}`. +fn strip_name_prefix(mut provider: Provider) -> Provider { + if let Some(metadata) = provider.metadata.as_mut() { + metadata.name = crate::auth::ownership::display_name(&metadata.name).to_string(); + } + provider +} + +/// Prepare a provider for gRPC response: redact credentials and strip name prefix. +pub(super) fn prepare_provider_response(provider: Provider) -> Provider { + strip_name_prefix(redact_provider_credentials(provider)) +} + #[derive(Debug, Clone, Default, PartialEq, Eq)] pub(super) struct ProviderEnvironment { pub environment: std::collections::HashMap, @@ -90,6 +128,7 @@ pub(super) async fn create_provider_record( store: &Store, mut provider: Provider, ) -> Result { + use crate::auth::ownership::{OWNER_LABEL, scoped_name}; use crate::persistence::{ObjectName, current_time_ms}; // Initialize metadata if not present @@ -131,6 +170,14 @@ pub(super) async fn create_provider_record( // Validate field sizes before any I/O. validate_provider_fields(&provider)?; + // Scope the DB name: if the provider has an owner label, prefix the name + // with `{owner}/` so two users can have the same user-visible name. + if let Some(metadata) = provider.metadata.as_mut() { + if let Some(owner) = metadata.labels.get(OWNER_LABEL).cloned() { + metadata.name = scoped_name(&owner, &metadata.name); + } + } + // Generate UUID for database row and update metadata.id to match let provider_id = uuid::Uuid::new_v4().to_string(); let mut provider = provider; @@ -178,20 +225,82 @@ pub(super) async fn create_provider_record( metadata.resource_version = result.resource_version; } - Ok(redact_provider_credentials(provider)) + Ok(prepare_provider_response(provider)) } -pub(super) async fn get_provider_record(store: &Store, name: &str) -> Result { +/// Resolve a provider by name with owner-scoped lookup. +/// +/// Resolution order for authenticated non-admin users: +/// 1. `{owner}/{name}` — user's own provider +/// 2. `{name}` — shared provider (no owner prefix) +/// +/// Admin and anonymous callers resolve the raw name directly. +pub(super) async fn get_provider_record( + store: &Store, + name: &str, + principal: Option<&crate::auth::principal::Principal>, + admin_role: &str, +) -> Result { if name.is_empty() { return Err(Status::invalid_argument("name is required")); } + let record = crate::auth::ownership::resolve_scoped_name( + store, + Provider::object_type(), + name, + principal, + admin_role, + ) + .await? + .ok_or_else(|| Status::not_found("provider not found"))?; + + let provider = Provider::decode(record.payload.as_slice()) + .map_err(|e| Status::internal(format!("decode provider failed: {e}")))?; + + Ok(prepare_provider_response(provider)) +} + +/// Internal lookup by exact DB key (scoped or unscoped). Used by sandbox env +/// resolution and inference routing where the stored key is already correct. +pub(super) async fn get_provider_record_by_db_key( + store: &Store, + db_key: &str, +) -> Result { + if db_key.is_empty() { + return Err(Status::invalid_argument("name is required")); + } + store - .get_message_by_name::(name) + .get_message_by_name::(db_key) .await .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))? .ok_or_else(|| Status::not_found("provider not found")) - .map(redact_provider_credentials) +} + +/// Resolve a user-visible provider name to the DB key (scoped name). +/// Returns the DB key that should be stored in sandbox spec.providers. +pub(super) async fn resolve_provider_db_key( + store: &Store, + user_name: &str, + principal: Option<&crate::auth::principal::Principal>, + admin_role: &str, +) -> Result { + if user_name.is_empty() { + return Err(Status::invalid_argument("name is required")); + } + + let record = crate::auth::ownership::resolve_scoped_name( + store, + Provider::object_type(), + user_name, + principal, + admin_role, + ) + .await? + .ok_or_else(|| Status::not_found(format!("provider '{user_name}' not found")))?; + + Ok(record.name) } pub(super) async fn list_provider_records( @@ -206,7 +315,7 @@ pub(super) async fn list_provider_records( Ok(providers .into_iter() - .map(redact_provider_credentials) + .map(prepare_provider_response) .collect()) } @@ -376,7 +485,7 @@ pub(super) async fn update_provider_record( metadata.resource_version = result.resource_version; } - Ok(redact_provider_credentials(candidate)) + Ok(prepare_provider_response(candidate)) } pub(super) async fn delete_provider_record(store: &Store, name: &str) -> Result { @@ -812,7 +921,6 @@ use openshell_providers::{ CredentialRefreshProfile, ProfileValidationDiagnostic, ProviderTypeProfile, default_profiles, get_default_profile, normalize_profile_id, normalize_provider_type, validate_profile_set, }; -use std::sync::Arc; use tonic::{Request, Response}; pub(super) async fn handle_create_provider( @@ -883,8 +991,13 @@ pub(super) async fn handle_get_provider( .get::() .cloned(); let name = request.into_inner().name; - let provider = get_provider_record(state.store.as_ref(), &name).await?; - check_provider_owner(&provider, principal.as_ref(), state)?; + let provider = get_provider_record( + state.store.as_ref(), + &name, + principal.as_ref(), + admin_role_name(state), + ) + .await?; Ok(Response::new(ProviderResponse { provider: Some(provider), @@ -1311,20 +1424,27 @@ pub(super) async fn handle_update_provider( }; let provider_type = provider.r#type.clone(); - // Ownership check: verify caller owns the existing provider before allowing update. - let provider_name = provider.metadata.as_ref().map_or("", |m| m.name.as_str()); - if !provider_name.is_empty() { - let existing = state - .store - .get_message_by_name::(provider_name) + // Ownership check: resolve scoped name and verify caller owns the provider. + let user_visible_name = provider.metadata.as_ref().map_or("", |m| m.name.as_str()); + if !user_visible_name.is_empty() { + let existing = resolve_and_check_provider(state, user_visible_name, principal.as_ref()) .await - .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))?; - let Some(ref existing) = existing else { - return Err(Status::not_found(format!( - "provider '{provider_name}' not found" - ))); - }; - check_provider_owner(existing, principal.as_ref(), state)?; + .map_err(|e| { + if e.code() == tonic::Code::NotFound { + Status::not_found(format!("provider '{user_visible_name}' not found")) + } else { + e + } + })?; + + // Rewrite the request's name to the resolved DB key so + // update_provider_record finds the correct record. + if let Some(metadata) = provider.metadata.as_mut() { + metadata.name = existing + .metadata + .as_ref() + .map_or_else(String::new, |m| m.name.clone()); + } } provider @@ -1365,13 +1485,8 @@ pub(super) async fn handle_get_provider_refresh_status( if request.provider.trim().is_empty() { return Err(Status::invalid_argument("provider is required")); } - let provider = state - .store - .get_message_by_name::(&request.provider) - .await - .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))? - .ok_or_else(|| Status::not_found("provider not found"))?; - check_provider_owner(&provider, principal.as_ref(), state)?; + let provider = + resolve_and_check_provider(state, request.provider.trim(), principal.as_ref()).await?; let states = if request.credential_key.trim().is_empty() { crate::provider_refresh::list_refresh_states_for_provider( @@ -1487,13 +1602,7 @@ pub(super) async fn handle_configure_provider_refresh( )); } - let provider = state - .store - .get_message_by_name::(provider_name) - .await - .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))? - .ok_or_else(|| Status::not_found("provider not found"))?; - check_provider_owner(&provider, principal.as_ref(), state)?; + let provider = resolve_and_check_provider(state, provider_name, principal.as_ref()).await?; validate_provider_credential_key_available_for_attached_sandboxes( state.store.as_ref(), &provider, @@ -1618,16 +1727,15 @@ pub(super) async fn handle_rotate_provider_credential( if credential_key.is_empty() { return Err(Status::invalid_argument("credential_key is required")); } - let provider = state - .store - .get_message_by_name::(provider_name) - .await - .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))? - .ok_or_else(|| Status::not_found("provider not found"))?; - check_provider_owner(&provider, principal.as_ref(), state)?; + let provider = resolve_and_check_provider(state, provider_name, principal.as_ref()).await?; + // Use the resolved DB key for internal refresh operations. + let db_key = provider + .metadata + .as_ref() + .map_or("", |m| m.name.as_str()); let refresh_state = crate::provider_refresh::refresh_provider_credential( state.store.as_ref(), - provider_name, + db_key, credential_key, ) .await?; @@ -1656,13 +1764,7 @@ pub(super) async fn handle_delete_provider_refresh( if credential_key.is_empty() { return Err(Status::invalid_argument("credential_key is required")); } - let provider = state - .store - .get_message_by_name::(provider_name) - .await - .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))? - .ok_or_else(|| Status::not_found("provider not found"))?; - check_provider_owner(&provider, principal.as_ref(), state)?; + let provider = resolve_and_check_provider(state, provider_name, principal.as_ref()).await?; let existing_refresh_state = crate::provider_refresh::get_refresh_state( state.store.as_ref(), provider.object_id(), @@ -1720,19 +1822,24 @@ pub(super) async fn handle_delete_provider( .cloned(); let name = request.into_inner().name; - // Ownership check: verify caller owns the provider before allowing delete. - let existing = state - .store - .get_message_by_name::(&name) + // Resolve scoped name and ownership check. + let existing = resolve_and_check_provider(state, &name, principal.as_ref()) .await - .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))?; - let Some(ref existing) = existing else { - return Err(Status::not_found(format!("provider '{name}' not found"))); - }; - check_provider_owner(existing, principal.as_ref(), state)?; + .map_err(|e| { + if e.code() == tonic::Code::NotFound { + Status::not_found(format!("provider '{name}' not found")) + } else { + e + } + })?; - let provider_profile = provider_profile_for_name(state.store.as_ref(), &name).await; - let result = delete_provider_record(state.store.as_ref(), &name).await; + // Use the resolved DB key (scoped name) for deletion. + let db_key = existing + .metadata + .as_ref() + .map_or("", |m| m.name.as_str()); + let provider_profile = provider_profile_for_name(state.store.as_ref(), db_key).await; + let result = delete_provider_record(state.store.as_ref(), db_key).await; match result { Ok(deleted) => { let outcome = TelemetryOutcome::from_success(deleted); @@ -5236,5 +5343,239 @@ mod tests { "spoofed owner label should be overwritten with actual caller" ); } + + // ---- Scoped name uniqueness tests ---- + + #[tokio::test] + async fn two_users_can_create_same_named_provider() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let bob = user_principal("bob-uuid"); + + let p1 = provider_with_values("openai", "generic"); + handle_create_provider(&state, create_request_with_principal(p1, &alice)) + .await + .unwrap(); + + let p2 = provider_with_values("openai", "generic"); + handle_create_provider(&state, create_request_with_principal(p2, &bob)) + .await + .unwrap(); + + // Each user sees their own + let alice_resp = + handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap(); + assert_eq!( + alice_resp.into_inner().provider.unwrap().object_name(), + "openai" + ); + + let bob_resp = + handle_get_provider(&state, get_request_with_principal("openai", &bob)) + .await + .unwrap(); + assert_eq!( + bob_resp.into_inner().provider.unwrap().object_name(), + "openai" + ); + } + + #[tokio::test] + async fn same_user_cannot_create_duplicate_name() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + + let p1 = provider_with_values("openai", "generic"); + handle_create_provider(&state, create_request_with_principal(p1, &alice)) + .await + .unwrap(); + + let p2 = provider_with_values("openai", "generic"); + let err = + handle_create_provider(&state, create_request_with_principal(p2, &alice)) + .await + .unwrap_err(); + assert_eq!(err.code(), Code::AlreadyExists); + } + + #[tokio::test] + async fn user_resolves_own_provider_over_shared() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + + // Create a shared provider (no principal) + let shared = Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "openai".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, + }), + r#type: "generic".to_string(), + credentials: std::iter::once(("SHARED_KEY".to_string(), "shared-val".to_string())) + .collect(), + config: HashMap::new(), + credential_expires_at_ms: HashMap::new(), + }; + handle_create_provider( + &state, + Request::new(CreateProviderRequest { + provider: Some(shared), + }), + ) + .await + .unwrap(); + + // Alice creates her own with the same visible name + let alice_provider = Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "openai".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, + }), + r#type: "generic".to_string(), + credentials: std::iter::once(("ALICE_KEY".to_string(), "alice-val".to_string())) + .collect(), + config: HashMap::new(), + credential_expires_at_ms: HashMap::new(), + }; + handle_create_provider( + &state, + create_request_with_principal(alice_provider, &alice), + ) + .await + .unwrap(); + + // Alice's GET resolves to her own (has ALICE_KEY, not SHARED_KEY) + let resp = + handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap(); + let p = resp.into_inner().provider.unwrap(); + assert!( + p.credentials.contains_key("ALICE_KEY"), + "should resolve to Alice's own provider" + ); + assert!( + !p.credentials.contains_key("SHARED_KEY"), + "should NOT resolve to shared provider" + ); + } + + #[tokio::test] + async fn user_falls_back_to_shared_when_no_own() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + + // Create only a shared provider + let shared = Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "openai".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, + }), + r#type: "generic".to_string(), + credentials: std::iter::once(("SHARED_KEY".to_string(), "shared-val".to_string())) + .collect(), + config: HashMap::new(), + credential_expires_at_ms: HashMap::new(), + }; + handle_create_provider( + &state, + Request::new(CreateProviderRequest { + provider: Some(shared), + }), + ) + .await + .unwrap(); + + // Alice can still GET it via fallback + let resp = + handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap(); + let p = resp.into_inner().provider.unwrap(); + assert!(p.credentials.contains_key("SHARED_KEY")); + } + + #[tokio::test] + async fn response_never_exposes_scoped_db_key() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + + let provider = provider_with_values("openai", "generic"); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); + + // GET response shows display name, not scoped key + let resp = + handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap(); + let name = resp.into_inner().provider.unwrap().object_name().to_string(); + assert_eq!(name, "openai"); + assert!( + !name.contains('/'), + "response name must not contain owner prefix" + ); + + // LIST response also strips prefixes + let list = handle_list_providers(&state, list_request_with_principal(&alice)) + .await + .unwrap() + .into_inner() + .providers; + for p in &list { + let n = p.object_name(); + assert!( + !n.contains('/'), + "listed provider name '{n}' must not contain owner prefix" + ); + } + } + + #[tokio::test] + async fn delete_own_provider_does_not_affect_other_user_same_name() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let bob = user_principal("bob-uuid"); + + let p1 = provider_with_values("openai", "generic"); + handle_create_provider(&state, create_request_with_principal(p1, &alice)) + .await + .unwrap(); + + let p2 = provider_with_values("openai", "generic"); + handle_create_provider(&state, create_request_with_principal(p2, &bob)) + .await + .unwrap(); + + // Alice deletes her provider + handle_delete_provider(&state, delete_request_with_principal("openai", &alice)) + .await + .unwrap(); + + // Bob's is still there + let resp = + handle_get_provider(&state, get_request_with_principal("openai", &bob)) + .await + .unwrap(); + assert_eq!(resp.into_inner().provider.unwrap().object_name(), "openai"); + + // Alice's is gone + let err = + handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap_err(); + assert_eq!(err.code(), Code::NotFound); + } } } diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 5c900c952..e5df98832 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -43,7 +43,8 @@ use russh::ChannelMsg; use russh::client::AuthResult; use super::provider::{ - get_provider_record, is_valid_env_key, validate_provider_environment_keys_unique, + get_provider_record_by_db_key, is_valid_env_key, prepare_provider_response, + resolve_provider_db_key, validate_provider_environment_keys_unique, }; use super::validation::{ level_matches, source_matches, validate_exec_request_fields, validate_policy_safety, @@ -79,6 +80,16 @@ pub(super) fn check_sandbox_owner( crate::auth::ownership::check_owner(labels, principal, admin_role_name(state)) } +/// Strip owner prefixes from sandbox spec.providers before returning to user. +fn strip_sandbox_provider_prefixes(mut sandbox: Sandbox) -> Sandbox { + if let Some(ref mut spec) = sandbox.spec { + for name in &mut spec.providers { + *name = crate::auth::ownership::display_name(name).to_string(); + } + } + sandbox +} + // --------------------------------------------------------------------------- // Sandbox lifecycle handlers // --------------------------------------------------------------------------- @@ -168,15 +179,26 @@ async fn handle_create_sandbox_inner( crate::grpc::validation::validate_label_value(value)?; } - // Validate provider names exist (fail fast). + // Resolve provider names to scoped DB keys and validate they exist. + // Users pass visible names ("openai"); we store the resolved DB key + // ("{owner}/openai" or "openai" for shared) in spec.providers so that + // internal lookups are single-pass. + let mut resolved_providers = Vec::with_capacity(spec.providers.len()); for name in &spec.providers { - state - .store - .get_message_by_name::(name) - .await - .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))? - .ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))?; + let record = crate::auth::ownership::resolve_scoped_name( + state.store.as_ref(), + Provider::object_type(), + name, + principal.as_ref(), + admin_role, + ) + .await? + .ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))?; + let provider = Provider::decode(record.payload.as_slice()) + .map_err(|e| Status::internal(format!("decode provider failed: {e}")))?; + resolved_providers.push(provider.object_name().to_string()); } + spec.providers = resolved_providers; validate_provider_environment_keys_unique(state.store.as_ref(), &spec.providers).await?; // Ensure the template always carries the resolved image. @@ -255,7 +277,7 @@ async fn handle_create_sandbox_inner( "CreateSandbox request completed successfully" ); Ok(Response::new(SandboxResponse { - sandbox: Some(sandbox), + sandbox: Some(strip_sandbox_provider_prefixes(sandbox)), })) } @@ -281,7 +303,7 @@ pub(super) async fn handle_get_sandbox( let sandbox = sandbox.ok_or_else(|| Status::not_found("sandbox not found"))?; check_sandbox_owner(&sandbox, principal.as_ref(), state)?; Ok(Response::new(SandboxResponse { - sandbox: Some(sandbox), + sandbox: Some(strip_sandbox_provider_prefixes(sandbox)), })) } @@ -319,6 +341,10 @@ pub(super) async fn handle_list_sandboxes( .map_err(|e| Status::internal(format!("list sandboxes with selector failed: {e}")))? }; + let sandboxes = sandboxes + .into_iter() + .map(strip_sandbox_provider_prefixes) + .collect(); Ok(Response::new(ListSandboxesResponse { sandboxes })) } @@ -359,32 +385,23 @@ pub(super) async fn handle_attach_sandbox_provider( ))); } - let provider = get_provider_record(state.store.as_ref(), &request.provider_name) - .await - .map_err(|err| { - if err.code() == tonic::Code::NotFound { - Status::failed_precondition(format!( - "provider '{}' not found", - request.provider_name - )) - } else { - err - } - })?; - - // Verify the caller can use this provider (must be own or shared). - let empty = std::collections::HashMap::new(); - let provider_labels = provider.metadata.as_ref().map_or(&empty, |m| &m.labels); - crate::auth::ownership::check_owner( - provider_labels, + // Resolve user-visible name to DB key for storage in spec.providers. + let provider_db_key = resolve_provider_db_key( + state.store.as_ref(), + &request.provider_name, principal.as_ref(), admin_role_name(state), ) - .map_err(|_| { - Status::permission_denied(format!( - "provider '{}' is owned by another user", - request.provider_name - )) + .await + .map_err(|err| { + if err.code() == tonic::Code::NotFound { + Status::failed_precondition(format!( + "provider '{}' not found", + request.provider_name + )) + } else { + err + } })?; let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await; @@ -409,7 +426,7 @@ pub(super) async fn handle_attach_sandbox_provider( && !spec .providers .iter() - .any(|name| name == &request.provider_name) + .any(|name| name == &provider_db_key) { return Err(Status::invalid_argument(format!( "providers list exceeds maximum ({MAX_PROVIDERS})" @@ -420,15 +437,15 @@ pub(super) async fn handle_attach_sandbox_provider( if !candidate_spec .providers .iter() - .any(|name| name == &request.provider_name) + .any(|name| name == &provider_db_key) { - candidate_spec.providers.push(request.provider_name.clone()); + candidate_spec.providers.push(provider_db_key.clone()); } validate_sandbox_spec(&request.sandbox_name, &candidate_spec)?; validate_provider_environment_keys_unique(state.store.as_ref(), &candidate_spec.providers) .await?; - let provider_name = request.provider_name.clone(); + let provider_name_for_cas = provider_db_key.clone(); let attached = Arc::new(AtomicBool::new(false)); let attached_clone = attached.clone(); @@ -444,10 +461,10 @@ pub(super) async fn handle_attach_sandbox_provider( }; dedupe_provider_names(&mut spec.providers); - if !spec.providers.iter().any(|name| name == &provider_name) + if !spec.providers.iter().any(|name| name == &provider_name_for_cas) && spec.providers.len() < MAX_PROVIDERS { - spec.providers.push(provider_name.clone()); + spec.providers.push(provider_name_for_cas.clone()); attached_clone.store(true, Ordering::Relaxed); } }, @@ -465,7 +482,7 @@ pub(super) async fn handle_attach_sandbox_provider( ); Ok(Response::new(AttachSandboxProviderResponse { - sandbox: Some(sandbox), + sandbox: Some(strip_sandbox_provider_prefixes(sandbox)), attached, })) } @@ -492,6 +509,15 @@ pub(super) async fn handle_detach_sandbox_provider( ))); } + // Resolve user-visible name to DB key (spec stores DB keys) + let provider_db_key = resolve_provider_db_key( + state.store.as_ref(), + &request.provider_name, + principal.as_ref(), + &admin_role_name(state), + ) + .await?; + let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await; let sandbox = sandbox_by_name(state, &request.sandbox_name).await?; check_sandbox_owner(&sandbox, principal.as_ref(), state)?; @@ -508,7 +534,7 @@ pub(super) async fn handle_detach_sandbox_provider( .as_ref() .ok_or_else(|| Status::internal("sandbox spec is missing"))?; - let provider_name = request.provider_name.clone(); + let provider_name_for_cas = provider_db_key.clone(); let detached = Arc::new(AtomicBool::new(false)); let detached_clone = detached.clone(); @@ -524,7 +550,7 @@ pub(super) async fn handle_detach_sandbox_provider( }; let before_len = spec.providers.len(); - spec.providers.retain(|name| name != &provider_name); + spec.providers.retain(|name| name != &provider_name_for_cas); if spec.providers.len() != before_len { detached_clone.store(true, Ordering::Relaxed); // Only dedupe after making a change @@ -545,7 +571,7 @@ pub(super) async fn handle_detach_sandbox_provider( ); Ok(Response::new(DetachSandboxProviderResponse { - sandbox: Some(sandbox), + sandbox: Some(strip_sandbox_provider_prefixes(sandbox)), detached, })) } @@ -617,24 +643,25 @@ async fn providers_for_sandbox( state: &Arc, sandbox: &Sandbox, ) -> Result, Status> { - let provider_names = sandbox + let provider_db_keys = sandbox .spec .as_ref() .map(|spec| spec.providers.as_slice()) .ok_or_else(|| Status::failed_precondition("sandbox spec is missing"))?; - let mut providers = Vec::with_capacity(provider_names.len()); - for name in provider_names { - let provider = get_provider_record(state.store.as_ref(), name) + let mut providers = Vec::with_capacity(provider_db_keys.len()); + for db_key in provider_db_keys { + let provider = get_provider_record_by_db_key(state.store.as_ref(), db_key) .await .map_err(|err| { if err.code() == tonic::Code::NotFound { - Status::failed_precondition(format!("provider '{name}' not found")) + let display = crate::auth::ownership::display_name(db_key); + Status::failed_precondition(format!("provider '{display}' not found")) } else { err } })?; - providers.push(provider); + providers.push(prepare_provider_response(provider)); } Ok(providers) } @@ -742,7 +769,7 @@ pub(super) async fn handle_watch_sandbox( .send(Ok(SandboxStreamEvent { payload: Some( openshell_core::proto::sandbox_stream_event::Payload::Sandbox( - sandbox.clone(), + strip_sandbox_provider_prefixes(sandbox.clone()), ), ), })) @@ -816,7 +843,7 @@ pub(super) async fn handle_watch_sandbox( match state.store.get_message::(&sandbox_id).await { Ok(Some(sandbox)) => { state.sandbox_index.update_from_sandbox(&sandbox); - if tx.send(Ok(SandboxStreamEvent { payload: Some(openshell_core::proto::sandbox_stream_event::Payload::Sandbox(sandbox.clone()))})).await.is_err() { + if tx.send(Ok(SandboxStreamEvent { payload: Some(openshell_core::proto::sandbox_stream_event::Payload::Sandbox(strip_sandbox_provider_prefixes(sandbox.clone())))})).await.is_err() { return; } if stop_on_terminal { From 3063c349a6f6bf0261b52a73be6ffbfc76395c6d Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 11:20:01 -0400 Subject: [PATCH 2/9] =?UTF-8?q?fix:=20address=20PR=20#20=20review=20?= =?UTF-8?q?=E2=80=94=20fmt,=20detach=20ambiguity,=20doc=20notes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Run cargo fmt to fix CI rustfmt gate - Fix detach resolution ambiguity: try both owned and shared DB key variants when detaching, so a shared provider attached before the user created their own can still be detached - Add safety doc note to get_provider_record_by_db_key warning callers about un-redacted credentials - Add TODO notes for admin addressability (preserve scoped keys in responses for admin principals) Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 10 ++- crates/openshell-server/src/grpc/provider.rs | 75 ++++++++++--------- crates/openshell-server/src/grpc/sandbox.rs | 41 ++++++---- 3 files changed, 72 insertions(+), 54 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index 1adcbdb1b..a467fb993 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -478,7 +478,10 @@ mod tests { #[test] fn scoped_name_construction() { - assert_eq!(scoped_name("alice-uuid-1234", "openai"), "alice-uuid-1234/openai"); + assert_eq!( + scoped_name("alice-uuid-1234", "openai"), + "alice-uuid-1234/openai" + ); } #[test] @@ -493,7 +496,10 @@ mod tests { #[test] fn owner_prefix_extracts_owner() { - assert_eq!(owner_prefix("alice-uuid-1234/openai"), Some("alice-uuid-1234")); + assert_eq!( + owner_prefix("alice-uuid-1234/openai"), + Some("alice-uuid-1234") + ); } #[test] diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 8acd57b04..af561c660 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -89,6 +89,9 @@ fn redact_provider_credentials(mut provider: Provider) -> Provider { /// Strip the owner prefix from a provider's metadata.name for display. /// DB stores `{owner}/{name}` but users see just `{name}`. +/// +/// TODO: preserve scoped keys for admin principals so they can distinguish +/// between multiple users' identically-named providers. fn strip_name_prefix(mut provider: Provider) -> Provider { if let Some(metadata) = provider.metadata.as_mut() { metadata.name = crate::auth::ownership::display_name(&metadata.name).to_string(); @@ -263,6 +266,12 @@ pub(super) async fn get_provider_record( /// Internal lookup by exact DB key (scoped or unscoped). Used by sandbox env /// resolution and inference routing where the stored key is already correct. +/// +/// # Safety (credential exposure) +/// +/// Returns the provider with UN-redacted credentials. Callers MUST wrap with +/// `prepare_provider_response` (or at minimum `redact_provider_credentials`) +/// before returning to users over gRPC. pub(super) async fn get_provider_record_by_db_key( store: &Store, db_key: &str, @@ -1729,10 +1738,7 @@ pub(super) async fn handle_rotate_provider_credential( } let provider = resolve_and_check_provider(state, provider_name, principal.as_ref()).await?; // Use the resolved DB key for internal refresh operations. - let db_key = provider - .metadata - .as_ref() - .map_or("", |m| m.name.as_str()); + let db_key = provider.metadata.as_ref().map_or("", |m| m.name.as_str()); let refresh_state = crate::provider_refresh::refresh_provider_credential( state.store.as_ref(), db_key, @@ -1834,10 +1840,7 @@ pub(super) async fn handle_delete_provider( })?; // Use the resolved DB key (scoped name) for deletion. - let db_key = existing - .metadata - .as_ref() - .map_or("", |m| m.name.as_str()); + let db_key = existing.metadata.as_ref().map_or("", |m| m.name.as_str()); let provider_profile = provider_profile_for_name(state.store.as_ref(), db_key).await; let result = delete_provider_record(state.store.as_ref(), db_key).await; match result { @@ -5372,10 +5375,9 @@ mod tests { "openai" ); - let bob_resp = - handle_get_provider(&state, get_request_with_principal("openai", &bob)) - .await - .unwrap(); + let bob_resp = handle_get_provider(&state, get_request_with_principal("openai", &bob)) + .await + .unwrap(); assert_eq!( bob_resp.into_inner().provider.unwrap().object_name(), "openai" @@ -5393,10 +5395,9 @@ mod tests { .unwrap(); let p2 = provider_with_values("openai", "generic"); - let err = - handle_create_provider(&state, create_request_with_principal(p2, &alice)) - .await - .unwrap_err(); + let err = handle_create_provider(&state, create_request_with_principal(p2, &alice)) + .await + .unwrap_err(); assert_eq!(err.code(), Code::AlreadyExists); } @@ -5452,10 +5453,9 @@ mod tests { .unwrap(); // Alice's GET resolves to her own (has ALICE_KEY, not SHARED_KEY) - let resp = - handle_get_provider(&state, get_request_with_principal("openai", &alice)) - .await - .unwrap(); + let resp = handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap(); let p = resp.into_inner().provider.unwrap(); assert!( p.credentials.contains_key("ALICE_KEY"), @@ -5497,10 +5497,9 @@ mod tests { .unwrap(); // Alice can still GET it via fallback - let resp = - handle_get_provider(&state, get_request_with_principal("openai", &alice)) - .await - .unwrap(); + let resp = handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap(); let p = resp.into_inner().provider.unwrap(); assert!(p.credentials.contains_key("SHARED_KEY")); } @@ -5516,11 +5515,15 @@ mod tests { .unwrap(); // GET response shows display name, not scoped key - let resp = - handle_get_provider(&state, get_request_with_principal("openai", &alice)) - .await - .unwrap(); - let name = resp.into_inner().provider.unwrap().object_name().to_string(); + let resp = handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap(); + let name = resp + .into_inner() + .provider + .unwrap() + .object_name() + .to_string(); assert_eq!(name, "openai"); assert!( !name.contains('/'), @@ -5564,17 +5567,15 @@ mod tests { .unwrap(); // Bob's is still there - let resp = - handle_get_provider(&state, get_request_with_principal("openai", &bob)) - .await - .unwrap(); + let resp = handle_get_provider(&state, get_request_with_principal("openai", &bob)) + .await + .unwrap(); assert_eq!(resp.into_inner().provider.unwrap().object_name(), "openai"); // Alice's is gone - let err = - handle_get_provider(&state, get_request_with_principal("openai", &alice)) - .await - .unwrap_err(); + let err = handle_get_provider(&state, get_request_with_principal("openai", &alice)) + .await + .unwrap_err(); assert_eq!(err.code(), Code::NotFound); } } diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index e5df98832..a689b5237 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -81,6 +81,9 @@ pub(super) fn check_sandbox_owner( } /// Strip owner prefixes from sandbox spec.providers before returning to user. +/// +/// TODO: preserve scoped keys for admin principals so they can distinguish +/// between multiple users' identically-named providers in list/get responses. fn strip_sandbox_provider_prefixes(mut sandbox: Sandbox) -> Sandbox { if let Some(ref mut spec) = sandbox.spec { for name in &mut spec.providers { @@ -395,10 +398,7 @@ pub(super) async fn handle_attach_sandbox_provider( .await .map_err(|err| { if err.code() == tonic::Code::NotFound { - Status::failed_precondition(format!( - "provider '{}' not found", - request.provider_name - )) + Status::failed_precondition(format!("provider '{}' not found", request.provider_name)) } else { err } @@ -423,10 +423,7 @@ pub(super) async fn handle_attach_sandbox_provider( // Pre-check: fail fast if already at MAX_PROVIDERS limit (avoid spurious CAS conflicts) // Note: This is an optimization; the CAS closure rechecks after dedupe in case of races if spec.providers.len() >= MAX_PROVIDERS - && !spec - .providers - .iter() - .any(|name| name == &provider_db_key) + && !spec.providers.iter().any(|name| name == &provider_db_key) { return Err(Status::invalid_argument(format!( "providers list exceeds maximum ({MAX_PROVIDERS})" @@ -461,7 +458,10 @@ pub(super) async fn handle_attach_sandbox_provider( }; dedupe_provider_names(&mut spec.providers); - if !spec.providers.iter().any(|name| name == &provider_name_for_cas) + if !spec + .providers + .iter() + .any(|name| name == &provider_name_for_cas) && spec.providers.len() < MAX_PROVIDERS { spec.providers.push(provider_name_for_cas.clone()); @@ -509,14 +509,27 @@ pub(super) async fn handle_detach_sandbox_provider( ))); } - // Resolve user-visible name to DB key (spec stores DB keys) + // Resolve user-visible name to DB key (spec stores DB keys). + // We resolve optimistically — if the provider doesn't exist at all, we still + // attempt the detach using the raw name (it may have been deleted after attach). let provider_db_key = resolve_provider_db_key( state.store.as_ref(), &request.provider_name, principal.as_ref(), &admin_role_name(state), ) - .await?; + .await + .ok(); + + // Build the set of keys to try detaching. Handles the ambiguity where the + // sandbox may have the shared variant (`openai`) or the owned variant + // (`alice/openai`) attached — try both the resolved key and the raw name. + let raw_name = request.provider_name.clone(); + let detach_keys: Vec = match provider_db_key { + Some(ref key) if *key != raw_name => vec![key.clone(), raw_name.clone()], + Some(ref key) => vec![key.clone()], + None => vec![raw_name.clone()], + }; let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await; let sandbox = sandbox_by_name(state, &request.sandbox_name).await?; @@ -534,7 +547,6 @@ pub(super) async fn handle_detach_sandbox_provider( .as_ref() .ok_or_else(|| Status::internal("sandbox spec is missing"))?; - let provider_name_for_cas = provider_db_key.clone(); let detached = Arc::new(AtomicBool::new(false)); let detached_clone = detached.clone(); @@ -545,15 +557,14 @@ pub(super) async fn handle_detach_sandbox_provider( request.expected_resource_version, |sandbox| { let Some(ref mut spec) = sandbox.spec else { - // Spec should always exist post-creation; if missing, fail CAS to surface error return; }; let before_len = spec.providers.len(); - spec.providers.retain(|name| name != &provider_name_for_cas); + spec.providers + .retain(|name| !detach_keys.iter().any(|k| k == name)); if spec.providers.len() != before_len { detached_clone.store(true, Ordering::Relaxed); - // Only dedupe after making a change dedupe_provider_names(&mut spec.providers); } }, From 552f5c35b88b7f5288e9ae606b60384cbb49cfe8 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 11:30:02 -0400 Subject: [PATCH 3/9] =?UTF-8?q?fix:=20resolve=20CI=20compilation=20errors?= =?UTF-8?q?=20=E2=80=94=20missing=20variable,=20test=20signatures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `let admin_role = admin_role_name(state)` in CreateSandbox handler - Update test call sites for get_provider_record (now takes 4 args) Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/grpc/provider.rs | 8 +++++--- crates/openshell-server/src/grpc/sandbox.rs | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index af561c660..9cce6f72b 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -3215,7 +3215,9 @@ mod tests { let duplicate_err = create_provider_record(&store, created).await.unwrap_err(); assert_eq!(duplicate_err.code(), Code::AlreadyExists); - let loaded = get_provider_record(&store, "gitlab-local").await.unwrap(); + let loaded = get_provider_record(&store, "gitlab-local", None, "") + .await + .unwrap(); assert_eq!(loaded.object_id(), provider_id); let listed = list_provider_records(&store, 100, 0).await.unwrap(); @@ -3285,7 +3287,7 @@ mod tests { .unwrap(); assert!(!deleted_again); - let missing = get_provider_record(&store, "gitlab-local") + let missing = get_provider_record(&store, "gitlab-local", None, "") .await .unwrap_err(); assert_eq!(missing.code(), Code::NotFound); @@ -3654,7 +3656,7 @@ mod tests { .unwrap(); assert!(vertex_empty.credentials.is_empty()); - let get_err = get_provider_record(store, "").await.unwrap_err(); + let get_err = get_provider_record(store, "", None, "").await.unwrap_err(); assert_eq!(get_err.code(), Code::InvalidArgument); let delete_err = delete_provider_record(store, "").await.unwrap_err(); diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index a689b5237..c2dc6f060 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -186,6 +186,7 @@ async fn handle_create_sandbox_inner( // Users pass visible names ("openai"); we store the resolved DB key // ("{owner}/openai" or "openai" for shared) in spec.providers so that // internal lookups are single-pass. + let admin_role = admin_role_name(state); let mut resolved_providers = Vec::with_capacity(spec.providers.len()); for name in &spec.providers { let record = crate::auth::ownership::resolve_scoped_name( From 1402d69ba9e7be2b63a8efd63d72de957f1cca52 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 11:46:22 -0400 Subject: [PATCH 4/9] fix: make spec binding mutable for provider resolution assignment The spec variable needs to be declared `mut` since we assign resolved_providers to spec.providers before passing it downstream. Removes the redundant re-binding that was placed after the assignment. Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/grpc/sandbox.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index c2dc6f060..b6947cd82 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -162,7 +162,7 @@ async fn handle_create_sandbox_inner( .get::() .cloned(); let mut request = request.into_inner(); - let spec = request + let mut spec = request .spec .ok_or_else(|| Status::invalid_argument("spec is required"))?; @@ -206,7 +206,6 @@ async fn handle_create_sandbox_inner( validate_provider_environment_keys_unique(state.store.as_ref(), &spec.providers).await?; // Ensure the template always carries the resolved image. - let mut spec = spec; let template = spec.template.get_or_insert_with(SandboxTemplate::default); if template.image.is_empty() { template.image = state.compute.default_image().to_string(); From dae8a5ea25d58db2fd024d0a6d50034049605f9e Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 13:49:35 -0400 Subject: [PATCH 5/9] fix: resolve clippy and dead-code lint warnings - Replace match in display_name with map_or (option_if_let_else) - Collapse nested if-let in create_provider (collapsible_if) - Remove needless borrow in detach handler (needless_borrow) - Allow dead_code on owner_prefix and scoped_name_for_principal (used in tests; needed for admin addressability TODO) Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 9 +++++---- crates/openshell-server/src/grpc/provider.rs | 8 ++++---- crates/openshell-server/src/grpc/sandbox.rs | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index a467fb993..7e263fde4 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -188,19 +188,20 @@ pub fn scoped_name(owner: &str, name: &str) -> String { /// If the key contains a `/`, the part after the first `/` is the display name. /// If no `/`, the key *is* the display name (shared/legacy provider). pub fn display_name(db_key: &str) -> &str { - match db_key.find(SCOPE_SEPARATOR) { - Some(pos) => &db_key[pos + 1..], - None => db_key, - } + db_key + .find(SCOPE_SEPARATOR) + .map_or(db_key, |pos| &db_key[pos + 1..]) } /// Extract the owner prefix from a scoped DB key, if present. +#[allow(dead_code)] // Used in tests; needed for admin addressability (TODO). pub fn owner_prefix(db_key: &str) -> Option<&str> { db_key.find(SCOPE_SEPARATOR).map(|pos| &db_key[..pos]) } /// Build the scoped DB key for the given principal, or return the raw name for /// anonymous/admin callers. +#[allow(dead_code)] // Used in tests; needed for admin addressability (TODO). pub fn scoped_name_for_principal( name: &str, principal: Option<&Principal>, diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 9cce6f72b..66b911d2c 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -175,10 +175,10 @@ pub(super) async fn create_provider_record( // Scope the DB name: if the provider has an owner label, prefix the name // with `{owner}/` so two users can have the same user-visible name. - if let Some(metadata) = provider.metadata.as_mut() { - if let Some(owner) = metadata.labels.get(OWNER_LABEL).cloned() { - metadata.name = scoped_name(&owner, &metadata.name); - } + if let Some(metadata) = provider.metadata.as_mut() + && let Some(owner) = metadata.labels.get(OWNER_LABEL).cloned() + { + metadata.name = scoped_name(&owner, &metadata.name); } // Generate UUID for database row and update metadata.id to match diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index b6947cd82..ba5a239d5 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -516,7 +516,7 @@ pub(super) async fn handle_detach_sandbox_provider( state.store.as_ref(), &request.provider_name, principal.as_ref(), - &admin_role_name(state), + admin_role_name(state), ) .await .ok(); From b47a8762a5e3d75f62aa1ee0d05858035c106958 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 15:04:03 -0400 Subject: [PATCH 6/9] fix: resolve scoped provider name lookup for cross-user and admin access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two-pass resolution (owned → shared fallback) failed to detect providers owned by other users — returning NotFound instead of PermissionDenied — and admin callers couldn't resolve user-scoped providers by display name. Add find_by_name_suffix to the Store (SQL LIKE '%/{name}') and rewrite resolve_scoped_name with three-step logic: 1. Try {caller}/{name} (owned) 2. Try {name} (shared/legacy) with post-resolution ownership check 3. Suffix scan for cross-user detection (admin gets access, others get PermissionDenied) Fixes: non_owner_denied_get, non_owner_denied_update, non_owner_denied_delete, admin_can_get_any_provider, attach_sandbox_provider_rejects_other_users_provider Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 56 ++++++++++++++++--- .../openshell-server/src/persistence/mod.rs | 12 ++++ .../src/persistence/postgres.rs | 24 ++++++++ .../src/persistence/sqlite.rs | 24 ++++++++ 4 files changed, 107 insertions(+), 9 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index 7e263fde4..aab89e475 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -222,10 +222,15 @@ pub fn scoped_name_for_principal( /// Resolve a provider name with owner-scoped fallback. /// /// Resolution order: -/// 1. Try `{owner}/{name}` (user's own provider) -/// 2. Fall back to `{name}` (shared provider, no owner prefix) +/// 1. Try `{owner}/{name}` (user's own provider) — non-admin only +/// 2. Fall back to `{name}` (shared/legacy provider, no owner prefix) +/// - If found and owned by another user: PermissionDenied (non-admin) +/// 3. Suffix scan for `*/{name}` (cross-user detection) +/// - If found and admin: return the match +/// - If found and non-admin: PermissionDenied /// -/// Admin callers and anonymous principals resolve the raw name directly. +/// Admin callers with explicit scoped name (contains '/') resolve directly. +/// Anonymous principals resolve the raw name directly (backward compat). pub async fn resolve_scoped_name( store: &crate::persistence::Store, object_type: &str, @@ -250,10 +255,11 @@ pub async fn resolve_scoped_name( .map_err(|e| Status::internal(format!("fetch by name failed: {e}"))); } - // Non-admin (or admin without explicit scope): try owned first + let caller_owner = sanitize_subject(&identity.subject)?; + + // Step 1: Non-admin tries owned lookup first if !is_admin { - let owner_value = sanitize_subject(&identity.subject)?; - let owned_key = scoped_name(&owner_value, name); + let owned_key = scoped_name(&caller_owner, name); let owned = store .get_by_name(object_type, &owned_key) .await @@ -263,11 +269,43 @@ pub async fn resolve_scoped_name( } } - // Fall back to shared (unscoped) name - store + // Step 2: Fall back to shared (unscoped) name + let shared = store .get_by_name(object_type, name) .await - .map_err(|e| Status::internal(format!("fetch shared provider failed: {e}"))) + .map_err(|e| Status::internal(format!("fetch shared provider failed: {e}")))?; + + if let Some(ref record) = shared { + if let Some(ref labels_json) = record.labels { + let labels: HashMap = + serde_json::from_str(labels_json).unwrap_or_default(); + if let Some(record_owner) = labels.get(OWNER_LABEL) { + if *record_owner != caller_owner && !is_admin { + return Err(Status::permission_denied( + "provider is owned by another user", + )); + } + } + } + return Ok(shared); + } + + // Step 3: Both missed — suffix scan for cross-user detection + let candidates = store + .find_by_name_suffix(object_type, name) + .await + .map_err(|e| Status::internal(format!("suffix search failed: {e}")))?; + + if let Some(record) = candidates.into_iter().next() { + if is_admin { + return Ok(Some(record)); + } + return Err(Status::permission_denied( + "provider is owned by another user", + )); + } + + Ok(None) } // --------------------------------------------------------------------------- diff --git a/crates/openshell-server/src/persistence/mod.rs b/crates/openshell-server/src/persistence/mod.rs index aad8b39c9..c985a2203 100644 --- a/crates/openshell-server/src/persistence/mod.rs +++ b/crates/openshell-server/src/persistence/mod.rs @@ -302,6 +302,18 @@ impl Store { } } + /// Find objects whose name ends with `/{suffix}` (scoped name lookup). + pub async fn find_by_name_suffix( + &self, + object_type: &str, + suffix: &str, + ) -> PersistenceResult> { + match self { + Self::Postgres(store) => store.find_by_name_suffix(object_type, suffix).await, + Self::Sqlite(store) => store.find_by_name_suffix(object_type, suffix).await, + } + } + /// Delete an object by id. pub async fn delete(&self, object_type: &str, id: &str) -> PersistenceResult { match self { diff --git a/crates/openshell-server/src/persistence/postgres.rs b/crates/openshell-server/src/persistence/postgres.rs index 529bc38be..49a15713e 100644 --- a/crates/openshell-server/src/persistence/postgres.rs +++ b/crates/openshell-server/src/persistence/postgres.rs @@ -323,6 +323,30 @@ WHERE object_type = $1 AND name = $2 Ok(row.map(row_to_object_record)) } + pub async fn find_by_name_suffix( + &self, + object_type: &str, + suffix: &str, + ) -> PersistenceResult> { + let escaped = suffix.replace('%', r"\%").replace('_', r"\_"); + let pattern = format!("%/{escaped}"); + let rows = sqlx::query( + r" +SELECT object_type, id, name, payload, created_at_ms, updated_at_ms, labels, resource_version +FROM objects +WHERE object_type = $1 AND name LIKE $2 ESCAPE '\' +ORDER BY created_at_ms ASC +", + ) + .bind(object_type) + .bind(&pattern) + .fetch_all(&self.pool) + .await + .map_err(|e| map_db_error(&e))?; + + Ok(rows.into_iter().map(row_to_object_record).collect()) + } + pub async fn delete(&self, object_type: &str, id: &str) -> PersistenceResult { let result = sqlx::query("DELETE FROM objects WHERE object_type = $1 AND id = $2") .bind(object_type) diff --git a/crates/openshell-server/src/persistence/sqlite.rs b/crates/openshell-server/src/persistence/sqlite.rs index 1958b3232..feada9ad3 100644 --- a/crates/openshell-server/src/persistence/sqlite.rs +++ b/crates/openshell-server/src/persistence/sqlite.rs @@ -339,6 +339,30 @@ WHERE "object_type" = ?1 AND "name" = ?2 Ok(row.map(row_to_object_record)) } + pub async fn find_by_name_suffix( + &self, + object_type: &str, + suffix: &str, + ) -> PersistenceResult> { + let escaped = suffix.replace('%', r"\%").replace('_', r"\_"); + let pattern = format!("%/{escaped}"); + let rows = sqlx::query( + r#" +SELECT "object_type", "id", "name", "payload", "created_at_ms", "updated_at_ms", "labels", "resource_version" +FROM "objects" +WHERE "object_type" = ?1 AND "name" LIKE ?2 ESCAPE '\' +ORDER BY "created_at_ms" ASC +"#, + ) + .bind(object_type) + .bind(&pattern) + .fetch_all(&self.pool) + .await + .map_err(|e| map_db_error(&e))?; + + Ok(rows.into_iter().map(row_to_object_record).collect()) + } + pub async fn delete(&self, object_type: &str, id: &str) -> PersistenceResult { let result = sqlx::query( r#" From 0884c538cf6aebb60c44c120501917f607797fa2 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 15:08:33 -0400 Subject: [PATCH 7/9] fix: resolve clippy doc-markdown and collapsible-if lints - Backtick `PermissionDenied` in doc comments - Collapse nested if-let + if into single let-chain Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index aab89e475..7ba2ef681 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -224,10 +224,10 @@ pub fn scoped_name_for_principal( /// Resolution order: /// 1. Try `{owner}/{name}` (user's own provider) — non-admin only /// 2. Fall back to `{name}` (shared/legacy provider, no owner prefix) -/// - If found and owned by another user: PermissionDenied (non-admin) +/// - If found and owned by another user: `PermissionDenied` (non-admin) /// 3. Suffix scan for `*/{name}` (cross-user detection) /// - If found and admin: return the match -/// - If found and non-admin: PermissionDenied +/// - If found and non-admin: `PermissionDenied` /// /// Admin callers with explicit scoped name (contains '/') resolve directly. /// Anonymous principals resolve the raw name directly (backward compat). @@ -279,12 +279,13 @@ pub async fn resolve_scoped_name( if let Some(ref labels_json) = record.labels { let labels: HashMap = serde_json::from_str(labels_json).unwrap_or_default(); - if let Some(record_owner) = labels.get(OWNER_LABEL) { - if *record_owner != caller_owner && !is_admin { - return Err(Status::permission_denied( - "provider is owned by another user", - )); - } + if let Some(record_owner) = labels.get(OWNER_LABEL) + && *record_owner != caller_owner + && !is_admin + { + return Err(Status::permission_denied( + "provider is owned by another user", + )); } } return Ok(shared); From 85bade22b65a612969c53d3a3beb51941d6e20aa Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 15:19:04 -0400 Subject: [PATCH 8/9] fix: use information-hiding for cross-user provider resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-admin callers who try to resolve a provider name owned by another user (via suffix scan) now get NotFound rather than PermissionDenied. This prevents leaking resource existence to unauthorized callers and ensures consistent behavior when a user deletes their own provider while another user has the same name. The PermissionDenied path remains active for the shared/legacy fallback (Step 2) where the record's owner label is directly checked — this covers providers stored under raw names with explicit ownership. Admin callers continue to resolve any user's provider via suffix scan. Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 11 +++++------ crates/openshell-server/src/grpc/provider.rs | 9 ++++++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index 7ba2ef681..46200e8f5 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -291,19 +291,18 @@ pub async fn resolve_scoped_name( return Ok(shared); } - // Step 3: Both missed — suffix scan for cross-user detection + // Step 3: Both missed — suffix scan for cross-user detection. + // Admin callers get access to any user's provider. Non-admin callers + // get None (surfaced as NotFound) for information-hiding security. let candidates = store .find_by_name_suffix(object_type, name) .await .map_err(|e| Status::internal(format!("suffix search failed: {e}")))?; - if let Some(record) = candidates.into_iter().next() { - if is_admin { + if is_admin { + if let Some(record) = candidates.into_iter().next() { return Ok(Some(record)); } - return Err(Status::permission_denied( - "provider is owned by another user", - )); } Ok(None) diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 66b911d2c..f8d1cbb5f 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -5078,11 +5078,12 @@ mod tests { .await .unwrap(); + // Information-hiding: non-owner sees NotFound, not PermissionDenied let err = handle_get_provider(&state, get_request_with_principal("alice-provider", &bob)) .await .unwrap_err(); - assert_eq!(err.code(), Code::PermissionDenied); + assert_eq!(err.code(), Code::NotFound); } #[tokio::test] @@ -5250,10 +5251,11 @@ mod tests { credential_expires_at_ms: HashMap::new(), }; + // Information-hiding: non-owner sees NotFound, not PermissionDenied let err = handle_update_provider(&state, update_request_with_principal(update, &bob)) .await .unwrap_err(); - assert_eq!(err.code(), Code::PermissionDenied); + assert_eq!(err.code(), Code::NotFound); } #[tokio::test] @@ -5286,13 +5288,14 @@ mod tests { .await .unwrap(); + // Information-hiding: non-owner sees NotFound, not PermissionDenied let err = handle_delete_provider( &state, delete_request_with_principal("alice-provider", &bob), ) .await .unwrap_err(); - assert_eq!(err.code(), Code::PermissionDenied); + assert_eq!(err.code(), Code::NotFound); } #[tokio::test] From 4bb8700ed00a6091723e513deecfa8440e02f8e4 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Wed, 17 Jun 2026 15:25:45 -0400 Subject: [PATCH 9/9] fix: Collapse nested if for clippy collapsible_if lint Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index 46200e8f5..f0b271276 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -299,10 +299,8 @@ pub async fn resolve_scoped_name( .await .map_err(|e| Status::internal(format!("suffix search failed: {e}")))?; - if is_admin { - if let Some(record) = candidates.into_iter().next() { - return Ok(Some(record)); - } + if is_admin && let Some(record) = candidates.into_iter().next() { + return Ok(Some(record)); } Ok(None)