From 32ae5d4c4f2e9baca20af809dc802db424eff12a Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 16 Jun 2026 20:27:26 -0400 Subject: [PATCH 1/4] feat(auth): add per-user provider ownership enforcement Extend the openshell.ai/owner label pattern (from sandbox ownership in PR #18) to providers, enabling OIDC-authenticated users to create and manage their own providers on a shared gateway without admin privileges. Changes: - Relax provider CRUD methods from admin to user role in rpc_auth annotations (ImportProviderProfiles/DeleteProviderProfile remain admin) - Stamp owner label on provider create via stamp_owner() - Add ownership checks (check_provider_owner) to get, update, delete, configure-refresh, rotate-credential, and delete-refresh handlers - Filter list_providers to return own + shared for regular users, all for admins - Make ownership.rs docstring entity-generic (shared with sandboxes) - Update authz tests to reflect relaxed role requirements - Add 12 ownership isolation unit tests covering create/get/list/update/ delete with owner, non-owner, admin, and shared provider scenarios Implements kagenti/kagenti#1995 (partial: Tasks 1-2, 5 complete; Tasks 3-4 deferred for follow-up). Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/authz.rs | 50 +- crates/openshell-server/src/auth/ownership.rs | 18 +- crates/openshell-server/src/grpc/mod.rs | 12 +- crates/openshell-server/src/grpc/provider.rs | 683 +++++++++++++++++- 4 files changed, 716 insertions(+), 47 deletions(-) diff --git a/crates/openshell-server/src/auth/authz.rs b/crates/openshell-server/src/auth/authz.rs index 1c04b0976..c64ec8fae 100644 --- a/crates/openshell-server/src/auth/authz.rs +++ b/crates/openshell-server/src/auth/authz.rs @@ -185,7 +185,7 @@ mod tests { let policy = default_policy(); assert!( policy - .check(&id, "/openshell.v1.OpenShell/CreateProvider") + .check(&id, "/openshell.v1.OpenShell/ImportProviderProfiles") .is_err() ); } @@ -408,7 +408,7 @@ mod tests { } #[test] - fn provider_refresh_methods_require_provider_scopes_and_admin_for_writes() { + fn provider_refresh_methods_require_provider_scopes_and_user_role() { let policy = scoped_policy(); let reader = identity_with_roles_and_scopes(&["openshell-user"], &["provider:read"]); assert!( @@ -417,37 +417,29 @@ mod tests { .is_ok() ); - let writer_without_admin = + // User with provider:write scope and user role can call write methods + // (ownership enforcement happens in the handler, not RBAC). + let user_writer = identity_with_roles_and_scopes(&["openshell-user"], &["provider:write"]); - let err = policy - .check( - &writer_without_admin, - "/openshell.v1.OpenShell/ConfigureProviderRefresh", - ) - .unwrap_err(); - assert_eq!(err.code(), tonic::Code::PermissionDenied); - assert!(err.message().contains("openshell-admin")); + for method in [ + "/openshell.v1.OpenShell/ConfigureProviderRefresh", + "/openshell.v1.OpenShell/RotateProviderCredential", + "/openshell.v1.OpenShell/DeleteProviderRefresh", + ] { + assert!(policy.check(&user_writer, method).is_ok(), "{method}"); + } - let admin_without_scope = - identity_with_roles_and_scopes(&["openshell-admin"], &["provider:read"]); + // Without the write scope, user is still denied. + let user_without_scope = + identity_with_roles_and_scopes(&["openshell-user"], &["provider:read"]); let err = policy .check( - &admin_without_scope, - "/openshell.v1.OpenShell/RotateProviderCredential", + &user_without_scope, + "/openshell.v1.OpenShell/ConfigureProviderRefresh", ) .unwrap_err(); assert_eq!(err.code(), tonic::Code::PermissionDenied); assert!(err.message().contains("provider:write")); - - let admin_writer = - identity_with_roles_and_scopes(&["openshell-admin"], &["provider:write"]); - for method in [ - "/openshell.v1.OpenShell/ConfigureProviderRefresh", - "/openshell.v1.OpenShell/RotateProviderCredential", - "/openshell.v1.OpenShell/DeleteProviderRefresh", - ] { - assert!(policy.check(&admin_writer, method).is_ok(), "{method}"); - } } #[test] @@ -493,10 +485,16 @@ mod tests { .check(&id, "/openshell.v1.OpenShell/GetProvider") .is_ok() ); - // admin methods still denied by role check + // User can now create providers (ownership enforced in handler). assert!( policy .check(&id, "/openshell.v1.OpenShell/CreateProvider") + .is_ok() + ); + // Admin methods still denied by role check. + assert!( + policy + .check(&id, "/openshell.v1.OpenShell/ImportProviderProfiles") .is_err() ); } diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index 6fb32a3c9..a4e1d1c06 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -1,11 +1,11 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -//! Per-user sandbox ownership enforcement. +//! Per-user object ownership enforcement. //! -//! Stamps an `openshell.ai/owner` label on sandboxes at creation time (from the -//! verified caller identity) and gates access on subsequent operations. Admin -//! callers bypass the ownership check. +//! Stamps an `openshell.ai/owner` label on objects (sandboxes, providers) at +//! creation time (from the verified caller identity) and gates access on +//! subsequent operations. Admin callers bypass the ownership check. #![allow(clippy::result_large_err)] @@ -18,7 +18,7 @@ use tonic::Status; use super::identity::Identity; use super::principal::Principal; -/// Reserved label key for sandbox ownership. Server-set, never client-controlled. +/// Reserved label key for object ownership. Server-set, never client-controlled. pub const OWNER_LABEL: &str = "openshell.ai/owner"; /// Reserved label key prefix. All keys starting with this are stripped from @@ -84,13 +84,13 @@ pub fn check_owner( admin_role: &str, ) -> Result<(), Status> { let Some(owner_value) = sandbox_labels.get(OWNER_LABEL) else { - // No owner label → legacy sandbox, allow access. + // No owner label → legacy/shared object, allow access. return Ok(()); }; let Some(identity) = principal_identity(principal) else { return Err(Status::permission_denied( - "sandbox is owned; authenticated identity required", + "object is owned; authenticated identity required", )); }; @@ -101,7 +101,7 @@ pub fn check_owner( let caller_value = sanitize_subject(&identity.subject)?; if caller_value != *owner_value { - return Err(Status::permission_denied("you do not own this sandbox")); + return Err(Status::permission_denied("you do not own this resource")); } Ok(()) @@ -140,7 +140,7 @@ fn require_identity(principal: Option<&Principal>) -> Result<&Identity, Status> match principal { Some(Principal::User(user)) => Ok(&user.identity), Some(Principal::Sandbox(_) | Principal::Anonymous) | None => Err(Status::unauthenticated( - "authenticated user identity required for sandbox operations", + "authenticated user identity required for ownership operations", )), } } diff --git a/crates/openshell-server/src/grpc/mod.rs b/crates/openshell-server/src/grpc/mod.rs index 2966152cf..7676d1745 100644 --- a/crates/openshell-server/src/grpc/mod.rs +++ b/crates/openshell-server/src/grpc/mod.rs @@ -356,7 +356,7 @@ impl OpenShell for OpenShellService { // --- Providers --- - #[rpc_auth(auth = "bearer", scope = "provider:write", role = "admin")] + #[rpc_auth(auth = "bearer", scope = "provider:write", role = "user")] async fn create_provider( &self, request: Request, @@ -412,7 +412,7 @@ impl OpenShell for OpenShellService { provider::handle_lint_provider_profiles(&self.state, request).await } - #[rpc_auth(auth = "bearer", scope = "provider:write", role = "admin")] + #[rpc_auth(auth = "bearer", scope = "provider:write", role = "user")] async fn update_provider( &self, request: Request, @@ -428,7 +428,7 @@ impl OpenShell for OpenShellService { provider::handle_get_provider_refresh_status(&self.state, request).await } - #[rpc_auth(auth = "bearer", scope = "provider:write", role = "admin")] + #[rpc_auth(auth = "bearer", scope = "provider:write", role = "user")] async fn configure_provider_refresh( &self, request: Request, @@ -436,7 +436,7 @@ impl OpenShell for OpenShellService { provider::handle_configure_provider_refresh(&self.state, request).await } - #[rpc_auth(auth = "bearer", scope = "provider:write", role = "admin")] + #[rpc_auth(auth = "bearer", scope = "provider:write", role = "user")] async fn rotate_provider_credential( &self, request: Request, @@ -444,7 +444,7 @@ impl OpenShell for OpenShellService { provider::handle_rotate_provider_credential(&self.state, request).await } - #[rpc_auth(auth = "bearer", scope = "provider:write", role = "admin")] + #[rpc_auth(auth = "bearer", scope = "provider:write", role = "user")] async fn delete_provider_refresh( &self, request: Request, @@ -452,7 +452,7 @@ impl OpenShell for OpenShellService { provider::handle_delete_provider_refresh(&self.state, request).await } - #[rpc_auth(auth = "bearer", scope = "provider:write", role = "admin")] + #[rpc_auth(auth = "bearer", scope = "provider:write", role = "user")] async fn delete_provider( &self, request: Request, diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 7591bdd6b..6ee630ba2 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -21,6 +21,33 @@ use super::{ MAX_MAP_KEY_LEN, MAX_MAP_VALUE_LEN, MAX_PAGE_SIZE, MAX_PROVIDER_CONFIG_ENTRIES, clamp_limit, }; +// --------------------------------------------------------------------------- +// Ownership helpers +// --------------------------------------------------------------------------- + +use crate::ServerState; + +fn admin_role_name(state: &ServerState) -> &str { + state + .config + .oidc + .as_ref() + .map_or("", |oidc| &oidc.admin_role) +} + +/// Verify the caller owns the provider (or is an admin). +/// Shared providers (no owner label) are accessible to all. +#[allow(clippy::result_large_err)] +fn check_provider_owner( + provider: &Provider, + principal: Option<&crate::auth::principal::Principal>, + state: &ServerState, +) -> Result<(), Status> { + let empty = std::collections::HashMap::new(); + let labels = provider.metadata.as_ref().map_or(&empty, |m| &m.labels); + crate::auth::ownership::check_owner(labels, principal, admin_role_name(state)) +} + // --------------------------------------------------------------------------- // CRUD helpers // --------------------------------------------------------------------------- @@ -111,6 +138,20 @@ pub(super) async fn create_provider_record( metadata.id.clone_from(&provider_id); } + // Serialize labels (includes ownership label if stamped) for storage index. + let labels_map = provider.object_labels(); + let labels_json = if labels_map + .as_ref() + .is_none_or(std::collections::HashMap::is_empty) + { + None + } else { + Some( + serde_json::to_string(&labels_map) + .map_err(|e| Status::internal(format!("serialize labels failed: {e}")))?, + ) + }; + // Create with MustCreate condition to prevent duplicate creation race let result = store .put_if( @@ -118,7 +159,7 @@ pub(super) async fn create_provider_record( &provider_id, provider.object_name(), &provider.encode_to_vec(), - None, + labels_json.as_deref(), WriteCondition::MustCreate, ) .await @@ -169,6 +210,46 @@ pub(super) async fn list_provider_records( .collect()) } +/// List providers visible to the given principal: own providers + shared (no owner). +/// Admins and anonymous principals see all providers (backward compat). +async fn list_provider_records_for_principal( + state: &Arc, + principal: Option<&crate::auth::principal::Principal>, + limit: u32, + offset: u32, +) -> Result, Status> { + use crate::auth::ownership::OWNER_LABEL; + + let admin_role = admin_role_name(state); + let caller_owner = match principal { + Some(crate::auth::principal::Principal::User(user)) => { + if !admin_role.is_empty() && user.identity.roles.iter().any(|r| r == admin_role) { + None // Admin sees all + } else { + Some(crate::auth::ownership::sanitize_subject(&user.identity.subject)?) + } + } + _ => None, // Anonymous/sandbox/none → see all (backward compat) + }; + + let all_providers = list_provider_records(state.store.as_ref(), limit, offset).await?; + + let Some(owner_value) = caller_owner else { + return Ok(all_providers); + }; + + Ok(all_providers + .into_iter() + .filter(|provider| { + let labels = provider.metadata.as_ref().map(|m| &m.labels); + match labels.and_then(|l| l.get(OWNER_LABEL)) { + None => true, // Shared provider (no owner) → visible + Some(v) => *v == owner_value, // Owned → visible only if caller's + } + }) + .collect()) +} + pub(super) async fn update_provider_record( store: &Store, provider: Provider, @@ -688,7 +769,6 @@ impl ObjectType for Provider { // Handler wrappers called from the trait impl in mod.rs // --------------------------------------------------------------------------- -use crate::ServerState; use openshell_core::proto::{ ConfigureProviderRefreshRequest, ConfigureProviderRefreshResponse, CreateProviderRequest, DeleteProviderProfileRequest, DeleteProviderProfileResponse, DeleteProviderRefreshRequest, @@ -713,8 +793,12 @@ pub(super) async fn handle_create_provider( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); - let Some(provider) = req.provider else { + let Some(mut provider) = req.provider else { emit_provider_lifecycle( "custom", LifecycleOperation::Create, @@ -722,6 +806,24 @@ pub(super) async fn handle_create_provider( ); return Err(Status::invalid_argument("provider is required")); }; + + // Stamp ownership on the provider's metadata labels (same pattern as sandbox). + // When OIDC is not configured (principal is None), skip ownership stamping + // to maintain backward compatibility with single-user local dev. + if principal.is_some() { + let labels = &mut provider + .metadata + .get_or_insert_with(|| openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: String::new(), + created_at_ms: 0, + labels: std::collections::HashMap::new(), + resource_version: 0, + }) + .labels; + crate::auth::ownership::stamp_owner(labels, principal.as_ref())?; + } + let provider_type = provider.r#type.clone(); let result = create_provider_record(state.store.as_ref(), provider).await; match result { @@ -750,8 +852,13 @@ pub(super) async fn handle_get_provider( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .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)?; Ok(Response::new(ProviderResponse { provider: Some(provider), @@ -762,9 +869,14 @@ pub(super) async fn handle_list_providers( state: &Arc, request: Request, ) -> Result, Status> { - let request = request.into_inner(); - let limit = clamp_limit(request.limit, 100, MAX_PAGE_SIZE); - let providers = list_provider_records(state.store.as_ref(), limit, request.offset).await?; + let principal = request + .extensions() + .get::() + .cloned(); + let req = request.into_inner(); + let limit = clamp_limit(req.limit, 100, MAX_PAGE_SIZE); + let providers = + list_provider_records_for_principal(state, principal.as_ref(), limit, req.offset).await?; Ok(Response::new(ListProvidersResponse { providers })) } @@ -1158,6 +1270,10 @@ pub(super) async fn handle_update_provider( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); let Some(mut provider) = req.provider else { emit_provider_lifecycle( @@ -1168,6 +1284,20 @@ pub(super) async fn handle_update_provider( return Err(Status::invalid_argument("provider is required")); }; 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) + .await + .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))?; + if let Some(ref existing) = existing { + check_provider_owner(existing, principal.as_ref(), state)?; + } + } + provider .credential_expires_at_ms .extend(req.credential_expires_at_ms); @@ -1198,6 +1328,10 @@ pub(super) async fn handle_get_provider_refresh_status( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .get::() + .cloned(); let request = request.into_inner(); if request.provider.trim().is_empty() { return Err(Status::invalid_argument("provider is required")); @@ -1208,6 +1342,7 @@ pub(super) async fn handle_get_provider_refresh_status( .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 states = if request.credential_key.trim().is_empty() { crate::provider_refresh::list_refresh_states_for_provider( @@ -1238,6 +1373,10 @@ pub(super) async fn handle_configure_provider_refresh( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .get::() + .cloned(); let request = request.into_inner(); let provider_name = request.provider.trim(); let credential_key = request.credential_key.trim(); @@ -1325,6 +1464,7 @@ pub(super) async fn handle_configure_provider_refresh( .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)?; validate_provider_credential_key_available_for_attached_sandboxes( state.store.as_ref(), &provider, @@ -1436,6 +1576,10 @@ pub(super) async fn handle_rotate_provider_credential( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .get::() + .cloned(); let request = request.into_inner(); let provider_name = request.provider.trim(); let credential_key = request.credential_key.trim(); @@ -1445,6 +1589,13 @@ 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 refresh_state = crate::provider_refresh::refresh_provider_credential( state.store.as_ref(), provider_name, @@ -1463,6 +1614,10 @@ pub(super) async fn handle_delete_provider_refresh( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .get::() + .cloned(); let request = request.into_inner(); let provider_name = request.provider.trim(); let credential_key = request.credential_key.trim(); @@ -1478,6 +1633,7 @@ pub(super) async fn handle_delete_provider_refresh( .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 existing_refresh_state = crate::provider_refresh::get_refresh_state( state.store.as_ref(), provider.object_id(), @@ -1529,7 +1685,22 @@ pub(super) async fn handle_delete_provider( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request + .extensions() + .get::() + .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) + .await + .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))?; + if let Some(ref existing) = existing { + check_provider_owner(existing, principal.as_ref(), state)?; + } + let provider_profile = provider_profile_for_name(state.store.as_ref(), &name).await; let result = delete_provider_record(state.store.as_ref(), &name).await; match result { @@ -4592,4 +4763,504 @@ mod tests { .count(); assert_eq!(new_keys_count, 1); } + + // ---- Per-user provider ownership isolation tests ---- + + mod ownership_tests { + use super::*; + use crate::auth::identity::{Identity, IdentityProvider}; + use crate::auth::ownership::OWNER_LABEL; + use crate::auth::principal::{Principal, UserPrincipal}; + use crate::compute::new_test_runtime; + use crate::persistence::Store; + use crate::sandbox_index::SandboxIndex; + use crate::sandbox_watch::SandboxWatchBus; + use crate::tracing_bus::TracingLogBus; + use openshell_core::OidcConfig; + + async fn oidc_test_state() -> std::sync::Arc { + let store = std::sync::Arc::new( + Store::connect("sqlite::memory:?cache=shared") + .await + .unwrap(), + ); + let compute = new_test_runtime(store.clone()).await; + let config = openshell_core::Config::new(None) + .with_database_url("sqlite::memory:?cache=shared") + .with_oidc(OidcConfig { + issuer: "http://test-issuer".to_string(), + audience: "test-audience".to_string(), + jwks_ttl_secs: 3600, + roles_claim: "realm_access.roles".to_string(), + admin_role: "openshell-admin".to_string(), + user_role: "openshell-user".to_string(), + scopes_claim: String::new(), + }); + std::sync::Arc::new(ServerState::new( + config, + store, + compute, + SandboxIndex::new(), + SandboxWatchBus::new(), + TracingLogBus::new(), + std::sync::Arc::new( + crate::supervisor_session::SupervisorSessionRegistry::new(), + ), + None, + None, + )) + } + + fn user_principal(subject: &str) -> Principal { + Principal::User(UserPrincipal { + identity: Identity { + subject: subject.to_string(), + display_name: None, + roles: vec!["openshell-user".to_string()], + scopes: vec![], + provider: IdentityProvider::Oidc, + }, + }) + } + + fn admin_principal() -> Principal { + Principal::User(UserPrincipal { + identity: Identity { + subject: "admin-uuid".to_string(), + display_name: None, + roles: vec![ + "openshell-admin".to_string(), + "openshell-user".to_string(), + ], + scopes: vec![], + provider: IdentityProvider::Oidc, + }, + }) + } + + fn create_request_with_principal( + provider: Provider, + principal: &Principal, + ) -> Request { + let mut req = Request::new(CreateProviderRequest { + provider: Some(provider), + }); + req.extensions_mut().insert(principal.clone()); + req + } + + fn get_request_with_principal( + name: &str, + principal: &Principal, + ) -> Request { + let mut req = Request::new(GetProviderRequest { + name: name.to_string(), + }); + req.extensions_mut().insert(principal.clone()); + req + } + + fn list_request_with_principal( + principal: &Principal, + ) -> Request { + let mut req = Request::new(ListProvidersRequest { + limit: 100, + offset: 0, + }); + req.extensions_mut().insert(principal.clone()); + req + } + + fn delete_request_with_principal( + name: &str, + principal: &Principal, + ) -> Request { + let mut req = Request::new(DeleteProviderRequest { + name: name.to_string(), + }); + req.extensions_mut().insert(principal.clone()); + req + } + + fn update_request_with_principal( + provider: Provider, + principal: &Principal, + ) -> Request { + let mut req = Request::new(UpdateProviderRequest { + provider: Some(provider), + credential_expires_at_ms: HashMap::new(), + }); + req.extensions_mut().insert(principal.clone()); + req + } + + #[tokio::test] + async fn create_stamps_owner_label() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let provider = provider_with_values("alice-provider", "generic"); + + let response = handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let created = response.into_inner().provider.unwrap(); + let labels = &created.metadata.as_ref().unwrap().labels; + assert_eq!(labels.get(OWNER_LABEL).unwrap(), "alice-uuid"); + } + + #[tokio::test] + async fn owner_can_get_own_provider() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let provider = provider_with_values("alice-provider", "generic"); + + handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let response = handle_get_provider( + &state, + get_request_with_principal("alice-provider", &alice), + ) + .await + .unwrap(); + assert_eq!(response.into_inner().provider.unwrap().object_name(), "alice-provider"); + } + + #[tokio::test] + async fn non_owner_denied_get() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let bob = user_principal("bob-uuid"); + let provider = provider_with_values("alice-provider", "generic"); + + handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let err = handle_get_provider( + &state, + get_request_with_principal("alice-provider", &bob), + ) + .await + .unwrap_err(); + assert_eq!(err.code(), Code::PermissionDenied); + } + + #[tokio::test] + async fn admin_can_get_any_provider() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let admin = admin_principal(); + let provider = provider_with_values("alice-provider", "generic"); + + handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let response = handle_get_provider( + &state, + get_request_with_principal("alice-provider", &admin), + ) + .await + .unwrap(); + assert_eq!(response.into_inner().provider.unwrap().object_name(), "alice-provider"); + } + + #[tokio::test] + async fn list_shows_own_and_shared_providers() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let bob = user_principal("bob-uuid"); + + // Alice creates a provider + let p1 = provider_with_values("alice-provider", "generic"); + handle_create_provider( + &state, + create_request_with_principal(p1, &alice), + ) + .await + .unwrap(); + + // Bob creates a provider + let p2 = provider_with_values("bob-provider", "generic"); + handle_create_provider( + &state, + create_request_with_principal(p2, &bob), + ) + .await + .unwrap(); + + // Create a shared provider (no principal → no owner label) + let shared = provider_with_values("shared-provider", "generic"); + handle_create_provider( + &state, + Request::new(CreateProviderRequest { + provider: Some(shared), + }), + ) + .await + .unwrap(); + + // Alice lists: sees her own + shared, not Bob's + let alice_list = handle_list_providers( + &state, + list_request_with_principal(&alice), + ) + .await + .unwrap() + .into_inner() + .providers; + + let alice_names: Vec<&str> = alice_list.iter().map(|p| p.object_name()).collect(); + assert!(alice_names.contains(&"alice-provider")); + assert!(alice_names.contains(&"shared-provider")); + assert!(!alice_names.contains(&"bob-provider")); + + // Bob lists: sees his own + shared, not Alice's + let bob_list = handle_list_providers( + &state, + list_request_with_principal(&bob), + ) + .await + .unwrap() + .into_inner() + .providers; + + let bob_names: Vec<&str> = bob_list.iter().map(|p| p.object_name()).collect(); + assert!(bob_names.contains(&"bob-provider")); + assert!(bob_names.contains(&"shared-provider")); + assert!(!bob_names.contains(&"alice-provider")); + } + + #[tokio::test] + async fn admin_list_sees_all_providers() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let admin = admin_principal(); + + let p1 = provider_with_values("alice-provider", "generic"); + handle_create_provider( + &state, + create_request_with_principal(p1, &alice), + ) + .await + .unwrap(); + + let shared = provider_with_values("shared-provider", "generic"); + handle_create_provider( + &state, + Request::new(CreateProviderRequest { + provider: Some(shared), + }), + ) + .await + .unwrap(); + + let admin_list = handle_list_providers( + &state, + list_request_with_principal(&admin), + ) + .await + .unwrap() + .into_inner() + .providers; + + let names: Vec<&str> = admin_list.iter().map(|p| p.object_name()).collect(); + assert!(names.contains(&"alice-provider")); + assert!(names.contains(&"shared-provider")); + } + + #[tokio::test] + async fn owner_can_update_own_provider() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let provider = provider_with_values("alice-provider", "generic"); + + handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let update = Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "alice-provider".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, + }), + r#type: String::new(), + credentials: [("NEW_KEY".to_string(), "new-val".to_string())] + .into_iter() + .collect(), + config: HashMap::new(), + credential_expires_at_ms: HashMap::new(), + }; + + let response = handle_update_provider( + &state, + update_request_with_principal(update, &alice), + ) + .await + .unwrap(); + + let updated = response.into_inner().provider.unwrap(); + assert!(updated.credentials.contains_key("NEW_KEY")); + } + + #[tokio::test] + async fn non_owner_denied_update() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let bob = user_principal("bob-uuid"); + let provider = provider_with_values("alice-provider", "generic"); + + handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let update = Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "alice-provider".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, + }), + r#type: String::new(), + credentials: [("STOLEN".to_string(), "val".to_string())] + .into_iter() + .collect(), + config: HashMap::new(), + credential_expires_at_ms: HashMap::new(), + }; + + let err = handle_update_provider( + &state, + update_request_with_principal(update, &bob), + ) + .await + .unwrap_err(); + assert_eq!(err.code(), Code::PermissionDenied); + } + + #[tokio::test] + async fn owner_can_delete_own_provider() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let provider = provider_with_values("alice-provider", "generic"); + + handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let response = handle_delete_provider( + &state, + delete_request_with_principal("alice-provider", &alice), + ) + .await + .unwrap(); + assert!(response.into_inner().deleted); + } + + #[tokio::test] + async fn non_owner_denied_delete() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + let bob = user_principal("bob-uuid"); + let provider = provider_with_values("alice-provider", "generic"); + + handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let err = handle_delete_provider( + &state, + delete_request_with_principal("alice-provider", &bob), + ) + .await + .unwrap_err(); + assert_eq!(err.code(), Code::PermissionDenied); + } + + #[tokio::test] + async fn shared_provider_accessible_by_any_user() { + let state = oidc_test_state().await; + let bob = user_principal("bob-uuid"); + + // Create provider without principal (shared/legacy) + let shared = provider_with_values("shared-provider", "generic"); + handle_create_provider( + &state, + Request::new(CreateProviderRequest { + provider: Some(shared), + }), + ) + .await + .unwrap(); + + // Any user can get it + let response = handle_get_provider( + &state, + get_request_with_principal("shared-provider", &bob), + ) + .await + .unwrap(); + assert_eq!(response.into_inner().provider.unwrap().object_name(), "shared-provider"); + } + + #[tokio::test] + async fn create_strips_spoofed_owner_label() { + let state = oidc_test_state().await; + let alice = user_principal("alice-uuid"); + + let mut provider = provider_with_values("test-provider", "generic"); + provider + .metadata + .as_mut() + .unwrap() + .labels + .insert(OWNER_LABEL.to_string(), "spoofed-uuid".to_string()); + + let response = handle_create_provider( + &state, + create_request_with_principal(provider, &alice), + ) + .await + .unwrap(); + + let created = response.into_inner().provider.unwrap(); + let labels = &created.metadata.as_ref().unwrap().labels; + assert_eq!( + labels.get(OWNER_LABEL).unwrap(), + "alice-uuid", + "spoofed owner label should be overwritten with actual caller" + ); + } + } } From dfef92d3b66ae6dc6ac6dc7fe265d3acd4477f81 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 16 Jun 2026 21:54:47 -0400 Subject: [PATCH 2/4] style: apply rustfmt to provider ownership code Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/authz.rs | 3 +- crates/openshell-server/src/grpc/provider.rs | 242 ++++++++----------- 2 files changed, 96 insertions(+), 149 deletions(-) diff --git a/crates/openshell-server/src/auth/authz.rs b/crates/openshell-server/src/auth/authz.rs index c64ec8fae..8727a3de3 100644 --- a/crates/openshell-server/src/auth/authz.rs +++ b/crates/openshell-server/src/auth/authz.rs @@ -419,8 +419,7 @@ mod tests { // User with provider:write scope and user role can call write methods // (ownership enforcement happens in the handler, not RBAC). - let user_writer = - identity_with_roles_and_scopes(&["openshell-user"], &["provider:write"]); + let user_writer = identity_with_roles_and_scopes(&["openshell-user"], &["provider:write"]); for method in [ "/openshell.v1.OpenShell/ConfigureProviderRefresh", "/openshell.v1.OpenShell/RotateProviderCredential", diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 6ee630ba2..b52a6afb4 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -226,7 +226,9 @@ async fn list_provider_records_for_principal( if !admin_role.is_empty() && user.identity.roles.iter().any(|r| r == admin_role) { None // Admin sees all } else { - Some(crate::auth::ownership::sanitize_subject(&user.identity.subject)?) + Some(crate::auth::ownership::sanitize_subject( + &user.identity.subject, + )?) } } _ => None, // Anonymous/sandbox/none → see all (backward compat) @@ -243,7 +245,7 @@ async fn list_provider_records_for_principal( .filter(|provider| { let labels = provider.metadata.as_ref().map(|m| &m.labels); match labels.and_then(|l| l.get(OWNER_LABEL)) { - None => true, // Shared provider (no owner) → visible + None => true, // Shared provider (no owner) → visible Some(v) => *v == owner_value, // Owned → visible only if caller's } }) @@ -4803,9 +4805,7 @@ mod tests { SandboxIndex::new(), SandboxWatchBus::new(), TracingLogBus::new(), - std::sync::Arc::new( - crate::supervisor_session::SupervisorSessionRegistry::new(), - ), + std::sync::Arc::new(crate::supervisor_session::SupervisorSessionRegistry::new()), None, None, )) @@ -4828,10 +4828,7 @@ mod tests { identity: Identity { subject: "admin-uuid".to_string(), display_name: None, - roles: vec![ - "openshell-admin".to_string(), - "openshell-user".to_string(), - ], + roles: vec!["openshell-admin".to_string(), "openshell-user".to_string()], scopes: vec![], provider: IdentityProvider::Oidc, }, @@ -4860,9 +4857,7 @@ mod tests { req } - fn list_request_with_principal( - principal: &Principal, - ) -> Request { + fn list_request_with_principal(principal: &Principal) -> Request { let mut req = Request::new(ListProvidersRequest { limit: 100, offset: 0, @@ -4900,12 +4895,10 @@ mod tests { let alice = user_principal("alice-uuid"); let provider = provider_with_values("alice-provider", "generic"); - let response = handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + let response = + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); let created = response.into_inner().provider.unwrap(); let labels = &created.metadata.as_ref().unwrap().labels; @@ -4918,20 +4911,18 @@ mod tests { let alice = user_principal("alice-uuid"); let provider = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); - let response = handle_get_provider( - &state, - get_request_with_principal("alice-provider", &alice), - ) - .await - .unwrap(); - assert_eq!(response.into_inner().provider.unwrap().object_name(), "alice-provider"); + let response = + handle_get_provider(&state, get_request_with_principal("alice-provider", &alice)) + .await + .unwrap(); + assert_eq!( + response.into_inner().provider.unwrap().object_name(), + "alice-provider" + ); } #[tokio::test] @@ -4941,19 +4932,14 @@ mod tests { let bob = user_principal("bob-uuid"); let provider = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); - let err = handle_get_provider( - &state, - get_request_with_principal("alice-provider", &bob), - ) - .await - .unwrap_err(); + let err = + handle_get_provider(&state, get_request_with_principal("alice-provider", &bob)) + .await + .unwrap_err(); assert_eq!(err.code(), Code::PermissionDenied); } @@ -4964,20 +4950,18 @@ mod tests { let admin = admin_principal(); let provider = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); - let response = handle_get_provider( - &state, - get_request_with_principal("alice-provider", &admin), - ) - .await - .unwrap(); - assert_eq!(response.into_inner().provider.unwrap().object_name(), "alice-provider"); + let response = + handle_get_provider(&state, get_request_with_principal("alice-provider", &admin)) + .await + .unwrap(); + assert_eq!( + response.into_inner().provider.unwrap().object_name(), + "alice-provider" + ); } #[tokio::test] @@ -4988,21 +4972,15 @@ mod tests { // Alice creates a provider let p1 = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(p1, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(p1, &alice)) + .await + .unwrap(); // Bob creates a provider let p2 = provider_with_values("bob-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(p2, &bob), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(p2, &bob)) + .await + .unwrap(); // Create a shared provider (no principal → no owner label) let shared = provider_with_values("shared-provider", "generic"); @@ -5016,14 +4994,11 @@ mod tests { .unwrap(); // Alice lists: sees her own + shared, not Bob's - let alice_list = handle_list_providers( - &state, - list_request_with_principal(&alice), - ) - .await - .unwrap() - .into_inner() - .providers; + let alice_list = handle_list_providers(&state, list_request_with_principal(&alice)) + .await + .unwrap() + .into_inner() + .providers; let alice_names: Vec<&str> = alice_list.iter().map(|p| p.object_name()).collect(); assert!(alice_names.contains(&"alice-provider")); @@ -5031,14 +5006,11 @@ mod tests { assert!(!alice_names.contains(&"bob-provider")); // Bob lists: sees his own + shared, not Alice's - let bob_list = handle_list_providers( - &state, - list_request_with_principal(&bob), - ) - .await - .unwrap() - .into_inner() - .providers; + let bob_list = handle_list_providers(&state, list_request_with_principal(&bob)) + .await + .unwrap() + .into_inner() + .providers; let bob_names: Vec<&str> = bob_list.iter().map(|p| p.object_name()).collect(); assert!(bob_names.contains(&"bob-provider")); @@ -5053,12 +5025,9 @@ mod tests { let admin = admin_principal(); let p1 = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(p1, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(p1, &alice)) + .await + .unwrap(); let shared = provider_with_values("shared-provider", "generic"); handle_create_provider( @@ -5070,14 +5039,11 @@ mod tests { .await .unwrap(); - let admin_list = handle_list_providers( - &state, - list_request_with_principal(&admin), - ) - .await - .unwrap() - .into_inner() - .providers; + let admin_list = handle_list_providers(&state, list_request_with_principal(&admin)) + .await + .unwrap() + .into_inner() + .providers; let names: Vec<&str> = admin_list.iter().map(|p| p.object_name()).collect(); assert!(names.contains(&"alice-provider")); @@ -5090,12 +5056,9 @@ mod tests { let alice = user_principal("alice-uuid"); let provider = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); let update = Provider { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { @@ -5113,12 +5076,10 @@ mod tests { credential_expires_at_ms: HashMap::new(), }; - let response = handle_update_provider( - &state, - update_request_with_principal(update, &alice), - ) - .await - .unwrap(); + let response = + handle_update_provider(&state, update_request_with_principal(update, &alice)) + .await + .unwrap(); let updated = response.into_inner().provider.unwrap(); assert!(updated.credentials.contains_key("NEW_KEY")); @@ -5131,12 +5092,9 @@ mod tests { let bob = user_principal("bob-uuid"); let provider = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); let update = Provider { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { @@ -5154,12 +5112,9 @@ mod tests { credential_expires_at_ms: HashMap::new(), }; - let err = handle_update_provider( - &state, - update_request_with_principal(update, &bob), - ) - .await - .unwrap_err(); + let err = handle_update_provider(&state, update_request_with_principal(update, &bob)) + .await + .unwrap_err(); assert_eq!(err.code(), Code::PermissionDenied); } @@ -5169,12 +5124,9 @@ mod tests { let alice = user_principal("alice-uuid"); let provider = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); let response = handle_delete_provider( &state, @@ -5192,12 +5144,9 @@ mod tests { let bob = user_principal("bob-uuid"); let provider = provider_with_values("alice-provider", "generic"); - handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); let err = handle_delete_provider( &state, @@ -5225,13 +5174,14 @@ mod tests { .unwrap(); // Any user can get it - let response = handle_get_provider( - &state, - get_request_with_principal("shared-provider", &bob), - ) - .await - .unwrap(); - assert_eq!(response.into_inner().provider.unwrap().object_name(), "shared-provider"); + let response = + handle_get_provider(&state, get_request_with_principal("shared-provider", &bob)) + .await + .unwrap(); + assert_eq!( + response.into_inner().provider.unwrap().object_name(), + "shared-provider" + ); } #[tokio::test] @@ -5247,12 +5197,10 @@ mod tests { .labels .insert(OWNER_LABEL.to_string(), "spoofed-uuid".to_string()); - let response = handle_create_provider( - &state, - create_request_with_principal(provider, &alice), - ) - .await - .unwrap(); + let response = + handle_create_provider(&state, create_request_with_principal(provider, &alice)) + .await + .unwrap(); let created = response.into_inner().provider.unwrap(); let labels = &created.metadata.as_ref().unwrap().labels; From 287bad1eb3b8ade26623fdd7ac757d6fa2bc1123 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 16 Jun 2026 22:19:39 -0400 Subject: [PATCH 3/4] feat(auth): validate provider-sandbox ownership alignment When attaching a provider to a sandbox, verify the provider is either owned by the caller or shared (no owner label). This prevents user A from referencing user B's private provider in their sandbox. Adds two tests: one for the rejection case and one confirming shared providers remain attachable by any user. Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/grpc/sandbox.rs | 114 +++++++++++++++++++- 1 file changed, 113 insertions(+), 1 deletion(-) diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 1114709ea..5c900c952 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -359,7 +359,7 @@ pub(super) async fn handle_attach_sandbox_provider( ))); } - get_provider_record(state.store.as_ref(), &request.provider_name) + let provider = get_provider_record(state.store.as_ref(), &request.provider_name) .await .map_err(|err| { if err.code() == tonic::Code::NotFound { @@ -372,6 +372,21 @@ pub(super) async fn handle_attach_sandbox_provider( } })?; + // 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, + principal.as_ref(), + admin_role_name(state), + ) + .map_err(|_| { + Status::permission_denied(format!( + "provider '{}' is owned by another user", + request.provider_name + )) + })?; + 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)?; @@ -2536,6 +2551,103 @@ mod tests { assert_eq!(err.code(), tonic::Code::FailedPrecondition); } + #[tokio::test] + async fn attach_sandbox_provider_rejects_other_users_provider() { + use crate::auth::identity::{Identity, IdentityProvider}; + use crate::auth::ownership::OWNER_LABEL; + use crate::auth::principal::{Principal, UserPrincipal}; + + let state = test_server_state().await; + + // Create a provider owned by alice + let mut provider = test_provider("alice-llm", "generic"); + provider + .metadata + .as_mut() + .unwrap() + .labels + .insert(OWNER_LABEL.to_string(), "alice-uuid".to_string()); + state.store.put_message(&provider).await.unwrap(); + + // Create a sandbox owned by bob + let mut sandbox = test_sandbox("bob-work", Vec::new()); + sandbox + .metadata + .as_mut() + .unwrap() + .labels + .insert(OWNER_LABEL.to_string(), "bob-uuid".to_string()); + state.store.put_message(&sandbox).await.unwrap(); + + // Bob tries to attach alice's provider + let bob = Principal::User(UserPrincipal { + identity: Identity { + subject: "bob-uuid".to_string(), + display_name: None, + roles: vec!["openshell-user".to_string()], + scopes: vec![], + provider: IdentityProvider::Oidc, + }, + }); + let mut req = Request::new(AttachSandboxProviderRequest { + sandbox_name: "bob-work".to_string(), + provider_name: "alice-llm".to_string(), + expected_resource_version: 0, + }); + req.extensions_mut().insert(bob); + + let err = handle_attach_sandbox_provider(&state, req) + .await + .unwrap_err(); + assert_eq!(err.code(), tonic::Code::PermissionDenied); + assert!(err.message().contains("owned by another user")); + } + + #[tokio::test] + async fn attach_sandbox_provider_allows_shared_provider() { + use crate::auth::identity::{Identity, IdentityProvider}; + use crate::auth::ownership::OWNER_LABEL; + use crate::auth::principal::{Principal, UserPrincipal}; + + let state = test_server_state().await; + + // Shared provider (no owner label) + state + .store + .put_message(&test_provider("shared-llm", "generic")) + .await + .unwrap(); + + // Sandbox owned by bob + let mut sandbox = test_sandbox("bob-work", Vec::new()); + sandbox + .metadata + .as_mut() + .unwrap() + .labels + .insert(OWNER_LABEL.to_string(), "bob-uuid".to_string()); + state.store.put_message(&sandbox).await.unwrap(); + + let bob = Principal::User(UserPrincipal { + identity: Identity { + subject: "bob-uuid".to_string(), + display_name: None, + roles: vec!["openshell-user".to_string()], + scopes: vec![], + provider: IdentityProvider::Oidc, + }, + }); + let mut req = Request::new(AttachSandboxProviderRequest { + sandbox_name: "bob-work".to_string(), + provider_name: "shared-llm".to_string(), + expected_resource_version: 0, + }); + req.extensions_mut().insert(bob); + + let response = handle_attach_sandbox_provider(&state, req).await.unwrap(); + assert!(response.into_inner().attached); + } + // ---- validate_interactive_exec_start ---- #[test] From 1ceb0f39d2015540069f85f6a5ce94f5d8f9797b Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 16 Jun 2026 23:02:14 -0400 Subject: [PATCH 4/4] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94=20?= =?UTF-8?q?clippy,=20pagination,=20not-found=20guards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix clippy warnings: unnecessary qualifications, redundant closures, iter_on_single_items, map_or→is_none_or - Fix list pagination for non-admins: loop in batches to guarantee `limit` visible results despite post-fetch filtering - Align update/delete ownership guards to return NOT_FOUND early when the provider doesn't exist, matching get/rotate behavior Signed-off-by: Paolo Dettori Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/grpc/provider.rs | 86 +++++++++++++------- 1 file changed, 56 insertions(+), 30 deletions(-) diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index b52a6afb4..1949d5eb5 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -212,6 +212,10 @@ pub(super) async fn list_provider_records( /// List providers visible to the given principal: own providers + shared (no owner). /// Admins and anonymous principals see all providers (backward compat). +/// +/// For non-admin users, filters happen post-fetch since the store selector +/// cannot express "owner=X OR no owner label". To maintain correct page sizes +/// we loop in batches until `limit` visible results are accumulated. async fn list_provider_records_for_principal( state: &Arc, principal: Option<&crate::auth::principal::Principal>, @@ -234,22 +238,42 @@ async fn list_provider_records_for_principal( _ => None, // Anonymous/sandbox/none → see all (backward compat) }; - let all_providers = list_provider_records(state.store.as_ref(), limit, offset).await?; - let Some(owner_value) = caller_owner else { - return Ok(all_providers); + return list_provider_records(state.store.as_ref(), limit, offset).await; }; - Ok(all_providers - .into_iter() - .filter(|provider| { + // Non-admin path: loop in batches to guarantee `limit` visible results + // despite post-fetch filtering. The store selector cannot express + // "owner=X OR no owner label", so we filter here. + let limit_usize = limit as usize; + let mut visible = Vec::new(); + let mut current_offset = offset; + let batch_size = limit.max(50); // fetch at least 50 to reduce round-trips + + loop { + let batch = list_provider_records(state.store.as_ref(), batch_size, current_offset).await?; + let exhausted = batch.len() < batch_size as usize; + + for provider in batch { let labels = provider.metadata.as_ref().map(|m| &m.labels); - match labels.and_then(|l| l.get(OWNER_LABEL)) { - None => true, // Shared provider (no owner) → visible - Some(v) => *v == owner_value, // Owned → visible only if caller's + if labels + .and_then(|l| l.get(OWNER_LABEL)) + .is_none_or(|v| *v == owner_value) + { + visible.push(provider); + if visible.len() >= limit_usize { + return Ok(visible); + } } - }) - .collect()) + } + + if exhausted { + break; + } + current_offset += batch_size; + } + + Ok(visible) } pub(super) async fn update_provider_record( @@ -1295,9 +1319,12 @@ pub(super) async fn handle_update_provider( .get_message_by_name::(provider_name) .await .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))?; - if let Some(ref existing) = existing { - check_provider_owner(existing, principal.as_ref(), state)?; - } + 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)?; } provider @@ -1699,9 +1726,10 @@ pub(super) async fn handle_delete_provider( .get_message_by_name::(&name) .await .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))?; - if let Some(ref existing) = existing { - check_provider_owner(existing, principal.as_ref(), state)?; - } + let Some(ref existing) = existing else { + return Err(Status::not_found(format!("provider '{name}' not found"))); + }; + check_provider_owner(existing, principal.as_ref(), state)?; let provider_profile = provider_profile_for_name(state.store.as_ref(), &name).await; let result = delete_provider_record(state.store.as_ref(), &name).await; @@ -4770,6 +4798,8 @@ mod tests { mod ownership_tests { use super::*; + use std::sync::Arc; + use crate::auth::identity::{Identity, IdentityProvider}; use crate::auth::ownership::OWNER_LABEL; use crate::auth::principal::{Principal, UserPrincipal}; @@ -4780,8 +4810,8 @@ mod tests { use crate::tracing_bus::TracingLogBus; use openshell_core::OidcConfig; - async fn oidc_test_state() -> std::sync::Arc { - let store = std::sync::Arc::new( + async fn oidc_test_state() -> Arc { + let store = Arc::new( Store::connect("sqlite::memory:?cache=shared") .await .unwrap(), @@ -4798,14 +4828,14 @@ mod tests { user_role: "openshell-user".to_string(), scopes_claim: String::new(), }); - std::sync::Arc::new(ServerState::new( + Arc::new(ServerState::new( config, store, compute, SandboxIndex::new(), SandboxWatchBus::new(), TracingLogBus::new(), - std::sync::Arc::new(crate::supervisor_session::SupervisorSessionRegistry::new()), + Arc::new(crate::supervisor_session::SupervisorSessionRegistry::new()), None, None, )) @@ -5000,7 +5030,7 @@ mod tests { .into_inner() .providers; - let alice_names: Vec<&str> = alice_list.iter().map(|p| p.object_name()).collect(); + let alice_names: Vec<&str> = alice_list.iter().map(Provider::object_name).collect(); assert!(alice_names.contains(&"alice-provider")); assert!(alice_names.contains(&"shared-provider")); assert!(!alice_names.contains(&"bob-provider")); @@ -5012,7 +5042,7 @@ mod tests { .into_inner() .providers; - let bob_names: Vec<&str> = bob_list.iter().map(|p| p.object_name()).collect(); + let bob_names: Vec<&str> = bob_list.iter().map(Provider::object_name).collect(); assert!(bob_names.contains(&"bob-provider")); assert!(bob_names.contains(&"shared-provider")); assert!(!bob_names.contains(&"alice-provider")); @@ -5045,7 +5075,7 @@ mod tests { .into_inner() .providers; - let names: Vec<&str> = admin_list.iter().map(|p| p.object_name()).collect(); + let names: Vec<&str> = admin_list.iter().map(Provider::object_name).collect(); assert!(names.contains(&"alice-provider")); assert!(names.contains(&"shared-provider")); } @@ -5069,9 +5099,7 @@ mod tests { resource_version: 0, }), r#type: String::new(), - credentials: [("NEW_KEY".to_string(), "new-val".to_string())] - .into_iter() - .collect(), + credentials: HashMap::from([("NEW_KEY".to_string(), "new-val".to_string())]), config: HashMap::new(), credential_expires_at_ms: HashMap::new(), }; @@ -5105,9 +5133,7 @@ mod tests { resource_version: 0, }), r#type: String::new(), - credentials: [("STOLEN".to_string(), "val".to_string())] - .into_iter() - .collect(), + credentials: HashMap::from([("STOLEN".to_string(), "val".to_string())]), config: HashMap::new(), credential_expires_at_ms: HashMap::new(), };