diff --git a/Cargo.lock b/Cargo.lock index a3b66d19576..f4978b96749 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4821,6 +4821,7 @@ dependencies = [ "ironclaw_conversations", "ironclaw_event_projections", "ironclaw_events", + "ironclaw_filesystem", "ironclaw_host_api", "ironclaw_host_runtime", "ironclaw_loop_support", diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index fc263178bf8..7f8e6308db6 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -12,7 +12,7 @@ use ironclaw_filesystem::{ use ironclaw_host_api::{ Action, ApprovalRequestId, CapabilityGrant, CapabilityGrantId, CapabilityId, GrantConstraints, HostApiError, PermissionMode, Principal, ProjectId, ResourceScope, ScopedPath, SystemServiceId, - TenantId, ThreadId, UserId, sha256_digest_token, + TenantId, UserId, sha256_digest_token, }; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -40,8 +40,6 @@ pub fn persistent_approval_grant_issuer() -> Principal { #[derive(Debug, Error)] pub enum PersistentApprovalPolicyError { - #[error("persistent approval scope must include project_id or thread_id")] - UnsupportedScope, #[error("unknown persistent approval policy")] UnknownPolicy, #[error("persistent approval policy changed concurrently")] @@ -95,28 +93,16 @@ pub struct PersistentApprovalScope { pub user_id: UserId, pub agent_id: Option, pub project_id: Option, - pub thread_id: Option, } impl PersistentApprovalScope { - pub fn from_resource_scope( - scope: &ResourceScope, - ) -> Result { - if scope.project_id.is_none() && scope.thread_id.is_none() { - return Err(PersistentApprovalPolicyError::UnsupportedScope); - } - let thread_id = if scope.project_id.is_some() { - None - } else { - scope.thread_id.clone() - }; - Ok(Self { + pub fn from_resource_scope(scope: &ResourceScope) -> Self { + Self { tenant_id: scope.tenant_id.clone(), user_id: scope.user_id.clone(), agent_id: scope.agent_id.clone(), project_id: scope.project_id.clone(), - thread_id, - }) + } } } @@ -134,13 +120,13 @@ impl PersistentApprovalPolicyKey { action: PersistentApprovalAction, capability_id: CapabilityId, grantee: Principal, - ) -> Result { - Ok(Self { - scope: PersistentApprovalScope::from_resource_scope(scope)?, + ) -> Self { + Self { + scope: PersistentApprovalScope::from_resource_scope(scope), action, capability_id, grantee, - }) + } } } @@ -243,7 +229,7 @@ impl PersistentApprovalPolicyStore for InMemoryPersistentApprovalPolicyStore { input.action, input.capability_id, input.grantee, - )?; + ); let mut policies = self .policies .write() @@ -363,7 +349,7 @@ where input.action, input.capability_id, input.grantee, - )?; + ); let path = self.cached_policy_path(&key)?; let lock = self.mutation_lock(&key); let _guard = lock.lock().await; @@ -480,7 +466,17 @@ where { let path = self.cached_policy_path(key)?; let scope = resource_scope_for_policy_key(key); - let Some(versioned) = self.filesystem.get(&scope, &path).await? else { + self.lookup_versioned_at(key, &scope, &path).await + } + + async fn lookup_versioned_at( + &self, + key: &PersistentApprovalPolicyKey, + scope: &ResourceScope, + path: &ScopedPath, + ) -> Result, PersistentApprovalPolicyError> + { + let Some(versioned) = self.filesystem.get(scope, path).await? else { return Ok(None); }; deserialize_versioned_policy(key, versioned) @@ -597,8 +593,6 @@ fn within_tenant_scope(scope: &PersistentApprovalScope) -> String { } if let Some(project_id) = &scope.project_id { segments.push(format!("projects/{project_id}")); - } else if let Some(thread_id) = &scope.thread_id { - segments.push(format!("threads/{thread_id}")); } if segments.is_empty() { "scope".to_string() @@ -626,7 +620,7 @@ fn resource_scope_for_policy_key(key: &PersistentApprovalPolicyKey) -> ResourceS agent_id: key.scope.agent_id.clone(), project_id: key.scope.project_id.clone(), mission_id: None, - thread_id: key.scope.thread_id.clone(), + thread_id: None, invocation_id: ironclaw_host_api::InvocationId::new(), } } @@ -660,7 +654,7 @@ mod tests { use ironclaw_filesystem::{InMemoryBackend, LocalFilesystem, ScopedFilesystem}; use ironclaw_host_api::{ AgentId, EffectKind, GrantConstraints, HostPath, InvocationId, MountAlias, MountGrant, - MountPermissions, MountView, NetworkPolicy, ProjectId, VirtualPath, + MountPermissions, MountView, NetworkPolicy, ProjectId, ThreadId, VirtualPath, }; use super::*; @@ -921,25 +915,113 @@ mod tests { ); } - #[tokio::test] - async fn policy_scope_prefers_project_over_thread() { + #[test] + fn policy_scope_prefers_project_over_thread() { + // Project is still part of the scope; differing thread ids under the same + // project produce identical keys. let scope_a = scope(Some("project-a"), Some("thread-a")); let scope_b = scope(Some("project-a"), Some("thread-b")); + let derived_a = PersistentApprovalScope::from_resource_scope(&scope_a); + let derived_b = PersistentApprovalScope::from_resource_scope(&scope_b); + + assert_eq!(derived_a, derived_b); + assert_eq!( + derived_a.project_id, + Some(ProjectId::new("project-a").unwrap()) + ); + } + + #[test] + fn project_scoped_policy_key_serialization_is_threadless() { + // Criterion 5 (#4825): persistent approvals intentionally ignore + // thread_id. There is no backward-compatibility read path for old local + // test records, so the key serialization should not retain thread_id. + let key = key_for(&scope(Some("project-a"), Some("thread-ignored"))); + let json = serde_json::to_string(&key).expect("serialize policy key"); + assert!( + !json.contains("thread_id"), + "persistent approval keys must not serialize thread_id; got {json}" + ); + + let digest_thread_one = + policy_digest(&key_for(&scope(Some("project-a"), Some("thread-1")))).expect("digest"); + let digest_thread_two = + policy_digest(&key_for(&scope(Some("project-a"), Some("thread-2")))).expect("digest"); + assert_eq!(digest_thread_one, digest_thread_two); assert_eq!( - PersistentApprovalScope::from_resource_scope(&scope_a).unwrap(), - PersistentApprovalScope::from_resource_scope(&scope_b).unwrap() + within_tenant_scope(&key.scope), + "agents/agent-a/projects/project-a" ); } #[tokio::test] - async fn policy_scope_uses_thread_without_project() { + async fn filesystem_project_scoped_policy_matches_in_new_thread_after_reload() { + // Criterion 5 (#4825): a project-scoped "always allow" applies across + // threads in the same project. A fresh store instance looks the policy + // up from a different thread and finds it through the canonical path. + let backend = Arc::new(InMemoryBackend::new()); + let scoped = scoped_fs(Arc::clone(&backend), "tenant-a", "alice"); + let store = FilesystemPersistentApprovalPolicyStore::new(Arc::clone(&scoped)); + + let granted = store + .allow(input(scope(Some("project-a"), Some("thread-1")))) + .await + .expect("allow project-scoped policy"); + + let new_thread_key = key_for(&scope(Some("project-a"), Some("thread-2"))); + let reloaded = FilesystemPersistentApprovalPolicyStore::new(scoped) + .lookup(&new_thread_key) + .await + .expect("lookup") + .expect("pre-existing project-scoped policy still matches in a new thread"); + + assert_eq!(reloaded, granted); + assert!(reloaded.active_grant().is_some()); + } + + #[test] + fn policy_scope_without_project_is_thread_agnostic() { let scope_a = scope(None, Some("thread-a")); let scope_b = scope(None, Some("thread-b")); + let derived_a = PersistentApprovalScope::from_resource_scope(&scope_a); + let derived_b = PersistentApprovalScope::from_resource_scope(&scope_b); + + assert_eq!(derived_a, derived_b); + } + + #[test] + fn policy_scope_without_project_isolates_users() { + let scope_a = ResourceScope { + user_id: UserId::new("alice").unwrap(), + ..scope(None, Some("thread-a")) + }; + let scope_b = ResourceScope { + user_id: UserId::new("bob").unwrap(), + ..scope(None, Some("thread-a")) + }; + + assert_ne!( + PersistentApprovalScope::from_resource_scope(&scope_a), + PersistentApprovalScope::from_resource_scope(&scope_b) + ); + } + + #[test] + fn policy_scope_without_project_isolates_agents() { + let scope_a = ResourceScope { + agent_id: Some(AgentId::new("agent-x").unwrap()), + ..scope(None, Some("thread-a")) + }; + let scope_b = ResourceScope { + agent_id: Some(AgentId::new("agent-y").unwrap()), + ..scope(None, Some("thread-a")) + }; + assert_ne!( - PersistentApprovalScope::from_resource_scope(&scope_a).unwrap(), - PersistentApprovalScope::from_resource_scope(&scope_b).unwrap() + PersistentApprovalScope::from_resource_scope(&scope_a), + PersistentApprovalScope::from_resource_scope(&scope_b) ); } @@ -987,16 +1069,6 @@ mod tests { assert_eq!(grant.issued_by, persistent_approval_grant_issuer()); } - #[tokio::test] - async fn from_resource_scope_errs_without_project_or_thread() { - let scope = scope(None, None); - - assert!(matches!( - PersistentApprovalScope::from_resource_scope(&scope), - Err(PersistentApprovalPolicyError::UnsupportedScope) - )); - } - fn input(scope: ResourceScope) -> PersistentApprovalPolicyInput { PersistentApprovalPolicyInput { scope, @@ -1024,7 +1096,6 @@ mod tests { CapabilityId::new("fixture.echo").unwrap(), Principal::User(UserId::new("alice").unwrap()), ) - .unwrap() } fn scope(project_id: Option<&str>, thread_id: Option<&str>) -> ResourceScope { diff --git a/crates/ironclaw_host_runtime/src/production.rs b/crates/ironclaw_host_runtime/src/production.rs index 93747d8fb22..ae31048d7a3 100644 --- a/crates/ironclaw_host_runtime/src/production.rs +++ b/crates/ironclaw_host_runtime/src/production.rs @@ -986,17 +986,7 @@ impl DefaultHostRuntime { ); return; } - let scope = match PersistentApprovalScope::from_resource_scope(&context.resource_scope) { - Ok(scope) => scope, - Err(error) => { - tracing::debug!( - capability_id = %capability_id, - error = %error, - "persistent approval lookup skipped for unsupported scope" - ); - return; - } - }; + let scope = PersistentApprovalScope::from_resource_scope(&context.resource_scope); let lookup_results = join_all(persistent_approval_grantees(context).into_iter().map( |grantee| { let policies = Arc::clone(policies); @@ -1557,9 +1547,11 @@ fn persistent_approval_grantees(context: &ironclaw_host_api::ExecutionContext) - if let Some(mission_id) = &context.mission_id { grantees.push(Principal::Mission(mission_id.clone())); } - if let Some(thread_id) = &context.thread_id { - grantees.push(Principal::Thread(thread_id.clone())); - } + // No `Principal::Thread` grantee: persistent approval policies are never + // written under a thread grantee (the grantee always comes from + // `ApprovalRequest.requested_by`, which is `Principal::User` or + // `Principal::Extension`), so looking one up could never match. Persistent + // approvals are deliberately thread-agnostic (see #4825). grantees } diff --git a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs index ce7f1df0758..68823918e21 100644 --- a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs +++ b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs @@ -7,12 +7,13 @@ use std::sync::{Arc, Mutex}; use async_trait::async_trait; use chrono::Utc; use ironclaw_approvals::{ - InMemoryPersistentApprovalPolicyStore, PersistentApprovalAction, PersistentApprovalPolicy, - PersistentApprovalPolicyError, PersistentApprovalPolicyInput, PersistentApprovalPolicyKey, - PersistentApprovalPolicyStore, + FilesystemPersistentApprovalPolicyStore, InMemoryPersistentApprovalPolicyStore, + PersistentApprovalAction, PersistentApprovalPolicy, PersistentApprovalPolicyError, + PersistentApprovalPolicyInput, PersistentApprovalPolicyKey, PersistentApprovalPolicyStore, }; use ironclaw_authorization::{GrantAuthorizer, TrustAwareCapabilityDispatchAuthorizer}; use ironclaw_extensions::{ExtensionManifest, ExtensionPackage, ExtensionRegistry, ManifestSource}; +use ironclaw_filesystem::{InMemoryBackend, ScopedFilesystem}; use ironclaw_host_api::*; use ironclaw_host_runtime::{ CapabilitySurfaceVersion, DefaultHostRuntime, HostRuntime, RuntimeCapabilityRequest, @@ -156,6 +157,82 @@ async fn default_runtime_uses_user_grantee_persistent_policy_as_dispatch_authori assert!(dispatcher.has_request()); } +#[tokio::test] +async fn default_runtime_uses_threadless_filesystem_policy_after_thread_change() { + let registry = Arc::new(registry_with_echo_capability()); + let dispatcher = Arc::new(RecordingDispatcher::default()); + let authorizer: Arc = Arc::new(GrantAuthorizer); + let run_state = Arc::new(InMemoryRunStateStore::new()); + let approval_requests = Arc::new(InMemoryApprovalRequestStore::new()); + let scoped = scoped_approval_fs(); + let policies = Arc::new(FilesystemPersistentApprovalPolicyStore::new(Arc::clone( + &scoped, + ))); + let mut context = execution_context_without_grants(); + let original_thread = ThreadId::new("thread-original").unwrap(); + let current_thread = ThreadId::new("thread-current").unwrap(); + context.project_id = None; + context.thread_id = Some(current_thread.clone()); + context.resource_scope.project_id = None; + context.resource_scope.thread_id = Some(current_thread); + + let mut original_scope = context.resource_scope.clone(); + original_scope.thread_id = Some(original_thread); + policies + .allow(PersistentApprovalPolicyInput { + scope: original_scope, + action: PersistentApprovalAction::Dispatch, + capability_id: capability_id(), + grantee: Principal::Extension(context.extension_id.clone()), + approved_by: Principal::User(context.user_id.clone()), + constraints: GrantConstraints { + allowed_effects: vec![EffectKind::DispatchCapability], + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets: Vec::new(), + resource_ceiling: None, + expires_at: None, + max_invocations: None, + }, + source_approval_request_id: None, + }) + .await + .expect("seed persistent policy"); + let policy_store: Arc = policies; + + let runtime = DefaultHostRuntime::new( + registry, + dispatcher.clone(), + authorizer, + CapabilitySurfaceVersion::new("surface-v1").unwrap(), + local_test_runtime_policy(), + ) + .with_trust_policy(Arc::new(local_manifest_trust_policy())) + .with_run_state(run_state) + .with_approval_requests(approval_requests) + .with_persistent_approval_policies(policy_store); + + let outcome = runtime + .invoke_capability(RuntimeCapabilityRequest::new( + context, + capability_id(), + ResourceEstimate::default(), + json!({"message": "hello"}), + trust_decision_with_dispatch_authority(), + )) + .await + .unwrap(); + + match outcome { + ironclaw_host_runtime::RuntimeCapabilityOutcome::Completed(completed) => { + assert_eq!(completed.capability_id, capability_id()); + assert_eq!(completed.output, json!({"ok": true})); + } + other => panic!("expected Completed outcome, got {:?}", other), + } + assert!(dispatcher.has_request()); +} + #[tokio::test] async fn default_runtime_does_not_replay_tenant_grantee_persistent_policy() { let registry = Arc::new(registry_with_echo_capability()); @@ -477,20 +554,44 @@ async fn default_runtime_skips_expired_persistent_policy() { } #[tokio::test] -async fn default_runtime_falls_back_gracefully_for_unsupported_persistent_scope() { +async fn default_runtime_uses_persistent_policy_for_no_project_no_thread_scope() { + // A fully unscoped context (no project, no thread) now yields a valid + // (tenant, user, agent) persistent approval scope: the lookup proceeds and a + // seeded "always allow" policy authorizes dispatch without a gate. let registry = Arc::new(registry_with_echo_capability()); let dispatcher = Arc::new(RecordingDispatcher::default()); let authorizer: Arc = Arc::new(GrantAuthorizer); let run_state = Arc::new(InMemoryRunStateStore::new()); let approval_requests = Arc::new(InMemoryApprovalRequestStore::new()); - let policies: Arc = - Arc::new(InMemoryPersistentApprovalPolicyStore::new()); + let policies = Arc::new(InMemoryPersistentApprovalPolicyStore::new()); let mut context = execution_context_without_grants(); context.project_id = None; context.thread_id = None; context.resource_scope.project_id = None; context.resource_scope.thread_id = None; + policies + .allow(PersistentApprovalPolicyInput { + scope: context.resource_scope.clone(), + action: PersistentApprovalAction::Dispatch, + capability_id: capability_id(), + grantee: Principal::Extension(context.extension_id.clone()), + approved_by: Principal::User(context.user_id.clone()), + constraints: GrantConstraints { + allowed_effects: vec![EffectKind::DispatchCapability], + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets: Vec::new(), + resource_ceiling: None, + expires_at: None, + max_invocations: None, + }, + source_approval_request_id: None, + }) + .await + .expect("seed persistent policy"); + let policy_store: Arc = policies; + let runtime = DefaultHostRuntime::new( registry, dispatcher.clone(), @@ -501,7 +602,7 @@ async fn default_runtime_falls_back_gracefully_for_unsupported_persistent_scope( .with_trust_policy(Arc::new(local_manifest_trust_policy())) .with_run_state(run_state) .with_approval_requests(approval_requests) - .with_persistent_approval_policies(policies); + .with_persistent_approval_policies(policy_store); let outcome = runtime .invoke_capability(RuntimeCapabilityRequest::new( @@ -515,13 +616,13 @@ async fn default_runtime_falls_back_gracefully_for_unsupported_persistent_scope( .unwrap(); match outcome { - ironclaw_host_runtime::RuntimeCapabilityOutcome::Failed(failure) => { - assert_eq!(failure.capability_id, capability_id()); - assert_eq!(failure.kind, RuntimeFailureKind::Authorization); + ironclaw_host_runtime::RuntimeCapabilityOutcome::Completed(completed) => { + assert_eq!(completed.capability_id, capability_id()); + assert_eq!(completed.output, json!({"ok": true})); } - other => panic!("expected authorization failure, got {:?}", other), + other => panic!("expected Completed outcome, got {:?}", other), } - assert!(!dispatcher.has_request()); + assert!(dispatcher.has_request()); } #[tokio::test] @@ -760,6 +861,25 @@ fn execution_context_without_grants() -> ExecutionContext { .unwrap() } +fn scoped_approval_fs() -> Arc> { + let mounts = MountView::new(vec![MountGrant::new( + MountAlias::new("/approvals").unwrap(), + VirtualPath::new("/approvals").unwrap(), + MountPermissions { + read: true, + write: true, + delete: false, + list: true, + execute: false, + }, + )]) + .expect("approval mount"); + Arc::new(ScopedFilesystem::with_fixed_view( + Arc::new(InMemoryBackend::new()), + mounts, + )) +} + fn local_manifest_trust_policy() -> HostTrustPolicy { local_manifest_trust_policy_with_effects(vec![EffectKind::DispatchCapability]) } diff --git a/crates/ironclaw_product_workflow/Cargo.toml b/crates/ironclaw_product_workflow/Cargo.toml index 163093ede2b..fb1e01deef8 100644 --- a/crates/ironclaw_product_workflow/Cargo.toml +++ b/crates/ironclaw_product_workflow/Cargo.toml @@ -44,6 +44,7 @@ url = "2" uuid = { version = "1", features = ["v4", "v5", "serde"] } [dev-dependencies] +ironclaw_filesystem = { path = "../ironclaw_filesystem" } ironclaw_host_runtime = { path = "../ironclaw_host_runtime" } ironclaw_loop_support = { path = "../ironclaw_loop_support" } ironclaw_event_projections = { path = "../ironclaw_event_projections" } diff --git a/crates/ironclaw_product_workflow/src/approval_interaction/service.rs b/crates/ironclaw_product_workflow/src/approval_interaction/service.rs index 127e66e92e2..003ef9d957e 100644 --- a/crates/ironclaw_product_workflow/src/approval_interaction/service.rs +++ b/crates/ironclaw_product_workflow/src/approval_interaction/service.rs @@ -268,17 +268,7 @@ impl DefaultApprovalInteractionService { input.action, input.capability_id.clone(), input.grantee.clone(), - ) - .map_err(|error| { - tracing::warn!( - error = %error, - approval_request_id = %gate.request().id, - "persistent approval policy preparation failed" - ); - ProductWorkflowError::Transient { - reason: "persistent approval policy unavailable".to_string(), - } - })?; + ); Ok(PreparedAllowPolicy { input, key }) } diff --git a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs index 4489e50a2bc..4268cf9077e 100644 --- a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs +++ b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs @@ -542,6 +542,75 @@ fn resource_scope(actor: &TurnActor) -> ResourceScope { } } +/// A no-project resource scope (WebChat shape) with explicit user/agent/thread. +/// Threads carry `project_id = None`, which is the case the persistent approval +/// scope fix targets: the scope key must be thread-agnostic. +fn no_project_scope(user: &str, agent: Option<&str>, thread: &str) -> ResourceScope { + ResourceScope { + tenant_id: TenantId::new("tenant-alpha").expect("tenant"), + user_id: UserId::new(user).expect("user"), + agent_id: agent.map(|id| ironclaw_host_api::AgentId::new(id).expect("agent")), + project_id: None, + mission_id: None, + thread_id: Some(ThreadId::new(thread).expect("thread")), + invocation_id: InvocationId::new(), + } +} + +/// In-memory-backed scoped filesystem matching the approvals store mount layout. +fn scoped_fs( + tenant: &str, + user: &str, +) -> Arc> { + use ironclaw_host_api::{MountAlias, MountGrant, MountPermissions, MountView, VirtualPath}; + let backend = Arc::new(ironclaw_filesystem::InMemoryBackend::new()); + let mounts = MountView::new(vec![MountGrant::new( + MountAlias::new("/approvals").expect("alias"), + VirtualPath::new(format!("/engine/tenants/{tenant}/users/{user}/approvals")) + .expect("virtual path"), + MountPermissions::read_write_list_delete(), + )]) + .expect("mount view"); + Arc::new(ironclaw_filesystem::ScopedFilesystem::with_fixed_view( + backend, mounts, + )) +} + +/// Builds a service fixture whose pending gate carries the supplied resource +/// scope and approval request, so tests can drive the real `resolve` path with +/// a chosen persisted scope and grantee. +fn service_fixture_with_scope( + request: ApprovalRequest, + gate_scope: ResourceScope, +) -> ( + DefaultApprovalInteractionService, + Arc, + Arc, + TurnRunId, + GateRef, +) { + let actor = actor(gate_scope.user_id.as_str()); + let gate_ref = approval_gate_ref(request.id).expect("gate ref"); + let run_id = TurnRunId::new(); + let gate = ApprovalGateRecord::with_status( + gate_scope, + run_id, + gate_ref.clone(), + request, + ApprovalStatus::Pending, + ) + .expect("approval gate"); + let resolver = Arc::new(RecordingApprovalResolver::default()); + let coordinator = Arc::new(FakeTurnCoordinator::blocked(actor, gate_ref.clone())); + let service = DefaultApprovalInteractionService::new( + Arc::new(FakeReadModel::with_gate(gate)), + Arc::new(FixedLeaseTermsProvider), + resolver.clone(), + coordinator.clone(), + ); + (service, resolver, coordinator, run_id, gate_ref) +} + fn approval_request(reason: &str) -> ApprovalRequest { ApprovalRequest { id: ApprovalRequestId::new(), @@ -557,6 +626,42 @@ fn approval_request(reason: &str) -> ApprovalRequest { } } +/// Dispatch approval request for `demo.echo` with an explicit grantee +/// (`requested_by`). Used by isolation tests that vary the policy grantee. +fn approval_request_by(reason: &str, requested_by: Principal) -> ApprovalRequest { + let mut request = approval_request(reason); + request.requested_by = requested_by; + request +} + +/// The two persistent-approval store backends every caller-level test exercises. +/// Filesystem scope paths are part of the fix, so both must pass. `prefix` +/// distinguishes the per-backend idempotency keys. +fn caller_level_store_pair(prefix: &str) -> [(Arc, String); 2] { + [ + ( + Arc::new(InMemoryPersistentApprovalPolicyStore::new()), + format!("{prefix}-in-memory"), + ), + ( + Arc::new( + ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( + "tenant-alpha", + "user-alpha", + )), + ), + format!("{prefix}-filesystem"), + ), + ] +} + +fn dispatch_capability(request: &ApprovalRequest) -> CapabilityId { + match request.action.as_ref() { + Action::Dispatch { capability, .. } => capability.clone(), + _ => panic!("test request should be dispatch"), + } +} + fn spawn_approval_request(reason: &str) -> ApprovalRequest { let mut request = approval_request(reason); request.action = Box::new(Action::SpawnCapability { @@ -686,8 +791,7 @@ async fn always_allow_resolves_gate_and_persists_reusable_policy() { PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let (service, resolver, coordinator, run_id, gate_ref) = service_fixture_for_request(request); let policies = Arc::new(InMemoryPersistentApprovalPolicyStore::new()); let policy_store: Arc = policies.clone(); @@ -725,6 +829,258 @@ async fn always_allow_resolves_gate_and_persists_reusable_policy() { assert!(policy.active_grant().is_some()); } +/// Drives the real `resolve(AlwaysAllow)` path: builds a service whose pending +/// gate carries `gate_scope` and `request`, wires `store`, and resolves with a +/// turn scope/actor derived from `gate_scope` (so the read-model gate lookup +/// matches). Asserts the resolution approved and resumed. The persisted policy +/// scope is `gate_scope` and the grantee is `request.requested_by`. +async fn drive_always_allow( + store: Arc, + request: ApprovalRequest, + gate_scope: ResourceScope, + idempotency: &str, +) { + let request_actor = TurnActor::new(gate_scope.user_id.clone()); + let request_scope = TurnScope::new( + gate_scope.tenant_id.clone(), + gate_scope.agent_id.clone(), + gate_scope.project_id.clone(), + gate_scope + .thread_id + .clone() + .expect("gate scope must carry a thread id"), + ); + let (service, resolver, coordinator, run_id, gate_ref) = + service_fixture_with_scope(request, gate_scope); + let service = service.with_persistent_policy_store(store); + + let response = service + .resolve(ResolveApprovalInteractionRequest { + scope: request_scope, + actor: request_actor, + run_id_hint: Some(run_id), + gate_ref, + decision: ApprovalInteractionDecision::AlwaysAllow, + idempotency_key: IdempotencyKey::new(idempotency).expect("idempotency"), + }) + .await + .expect("always allow"); + + assert!(matches!( + response, + ResolveApprovalInteractionResponse::Approved(_) + )); + assert_eq!(resolver.approval_count(), 1); + assert_eq!(coordinator.resumption_count(), 1); +} + +async fn drive_spawn_always_allow( + store: Arc, + request: ApprovalRequest, + gate_scope: ResourceScope, + idempotency: &str, +) { + let request_actor = TurnActor::new(gate_scope.user_id.clone()); + let request_scope = TurnScope::new( + gate_scope.tenant_id.clone(), + gate_scope.agent_id.clone(), + gate_scope.project_id.clone(), + gate_scope + .thread_id + .clone() + .expect("gate scope must carry a thread id"), + ); + let (service, resolver, coordinator, run_id, gate_ref) = + service_fixture_with_scope(request, gate_scope); + let service = service.with_persistent_policy_store(store); + + let response = service + .resolve(ResolveApprovalInteractionRequest { + scope: request_scope, + actor: request_actor, + run_id_hint: Some(run_id), + gate_ref, + decision: ApprovalInteractionDecision::AlwaysAllow, + idempotency_key: IdempotencyKey::new(idempotency).expect("idempotency"), + }) + .await + .expect("always allow"); + + assert!(matches!( + response, + ResolveApprovalInteractionResponse::Approved(_) + )); + assert_eq!(resolver.approval_count(), 0); + assert_eq!(resolver.spawn_approval_count(), 1); + assert_eq!(coordinator.resumption_count(), 1); +} + +/// Acceptance criterion 1: an "always allow" granted while resolving a gate in +/// thread 1 (no project) is reused for the same capability in thread 2 without a +/// gate. Covered against both InMemory and Filesystem stores because the +/// filesystem scope path is part of the fix. +#[tokio::test] +async fn always_allow_grants_reuse_in_new_thread_without_project() { + for (store, idempotency) in caller_level_store_pair("reuse") { + let request = approval_request("send the email"); + let capability = dispatch_capability(&request); + let thread_one = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, thread_one, &idempotency).await; + + // Look up from thread 2 (same user/agent, no project): different thread, + // same persistent scope key, so the grant is active. + let thread_two = no_project_scope("user-alpha", Some("agent-a"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &thread_two, + PersistentApprovalAction::Dispatch, + capability, + Principal::User(UserId::new("user-alpha").expect("user")), + ); + let policy = store + .lookup(&key) + .await + .expect("persistent policy lookup") + .expect("persistent policy active in new thread"); + assert!(policy.active_grant().is_some()); + } +} + +/// Acceptance criterion 2: a spawn-capability "always allow" is persisted as a +/// reusable policy and can be matched again from a later thread with the same +/// user/agent/project scope. +#[tokio::test] +async fn always_allow_spawn_grants_reuse_in_new_thread_without_project() { + for (store, idempotency) in caller_level_store_pair("spawn-reuse") { + let request = spawn_approval_request("start the worker"); + let capability = match request.action.as_ref() { + Action::SpawnCapability { capability, .. } => capability.clone(), + _ => panic!("test request should be spawn"), + }; + let thread_one = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_spawn_always_allow(Arc::clone(&store), request, thread_one, &idempotency).await; + + let thread_two = no_project_scope("user-alpha", Some("agent-a"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &thread_two, + PersistentApprovalAction::SpawnCapability, + capability, + Principal::User(UserId::new("user-alpha").expect("user")), + ); + let policy = store + .lookup(&key) + .await + .expect("persistent policy lookup") + .expect("persistent policy active in new thread"); + assert!(policy.active_grant().is_some()); + } +} + +/// Acceptance criterion 4 (isolation): an "always allow" granted by user A in +/// thread 1 must NOT authorize user B in thread 2 under the same tenant/agent. +#[tokio::test] +async fn always_allow_does_not_grant_other_user_in_new_thread() { + for (store, idempotency) in caller_level_store_pair("user-iso") { + let request = approval_request_by( + "send the email", + Principal::User(UserId::new("user-alpha").expect("user")), + ); + let capability = dispatch_capability(&request); + let user_a = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, user_a, &idempotency).await; + + let user_b = no_project_scope("user-beta", Some("agent-a"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &user_b, + PersistentApprovalAction::Dispatch, + capability, + Principal::User(UserId::new("user-beta").expect("user")), + ); + assert!( + store + .lookup(&key) + .await + .expect("persistent policy lookup") + .is_none(), + "another user must not inherit the grant" + ); + } +} + +/// Acceptance criterion 4 (isolation): an "always allow" granted under agent X +/// must NOT authorize agent Y under the same tenant/user. +#[tokio::test] +async fn always_allow_does_not_grant_other_agent_in_new_thread() { + for (store, idempotency) in caller_level_store_pair("agent-iso") { + let request = approval_request("send the email"); + let capability = dispatch_capability(&request); + let agent_x = no_project_scope("user-alpha", Some("agent-x"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, agent_x, &idempotency).await; + + let agent_y = no_project_scope("user-alpha", Some("agent-y"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &agent_y, + PersistentApprovalAction::Dispatch, + capability, + Principal::User(UserId::new("user-alpha").expect("user")), + ); + assert!( + store + .lookup(&key) + .await + .expect("persistent policy lookup") + .is_none(), + "another agent must not inherit the grant" + ); + } +} + +/// Acceptance criterion 4 (isolation): the policy key includes the grantee, so an +/// approval for extension X must not match a lookup for extension Y requesting +/// the same capability under the same scope. +#[tokio::test] +async fn always_allow_does_not_grant_other_extension_grantee() { + for (store, idempotency) in caller_level_store_pair("ext-iso") { + let extension_x = ironclaw_host_api::ExtensionId::new("extension-x").expect("extension"); + let request = + approval_request_by("send the email", Principal::Extension(extension_x.clone())); + let capability = dispatch_capability(&request); + let gate_scope = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, gate_scope, &idempotency).await; + + // Same scope, same capability, but a different extension grantee. + let lookup_scope = no_project_scope("user-alpha", Some("agent-a"), "thread-2"); + let extension_y = ironclaw_host_api::ExtensionId::new("extension-y").expect("extension"); + let key = PersistentApprovalPolicyKey::new( + &lookup_scope, + PersistentApprovalAction::Dispatch, + capability.clone(), + Principal::Extension(extension_y), + ); + assert!( + store + .lookup(&key) + .await + .expect("persistent policy lookup") + .is_none(), + "another extension grantee must not match" + ); + + // Sanity: the granting extension X DOES match (thread-agnostic reuse). + let key_x = PersistentApprovalPolicyKey::new( + &lookup_scope, + PersistentApprovalAction::Dispatch, + capability, + Principal::Extension(extension_x), + ); + let policy = store + .lookup(&key_x) + .await + .expect("persistent policy lookup") + .expect("granting extension active in new thread"); + assert!(policy.active_grant().is_some()); + } +} + #[tokio::test] async fn always_allow_without_policy_store_rejects_before_approval_side_effects() { let (service, resolver, coordinator, run_id, gate_ref) = service_fixture("send the email"); @@ -793,8 +1149,7 @@ async fn always_allow_disallowed_by_policy_rejects_without_persisting_or_approvi PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let actor = actor("user-alpha"); let gate_ref = approval_gate_ref(request.id).expect("gate ref"); let run_id = TurnRunId::new(); @@ -865,8 +1220,7 @@ async fn always_allow_does_not_persist_policy_when_resolution_fails() { PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let gate_ref = approval_gate_ref(request.id).expect("gate ref"); let run_id = TurnRunId::new(); let gate = ApprovalGateRecord::with_status( @@ -932,8 +1286,7 @@ async fn always_allow_resolution_failure_preserves_existing_policy() { PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let gate_ref = approval_gate_ref(request.id).expect("gate ref"); let run_id = TurnRunId::new(); let gate = ApprovalGateRecord::with_status( @@ -1147,8 +1500,7 @@ async fn already_approved_always_allow_replay_rejects_without_persisting_policy( PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let (service, resolver, coordinator, run_id, gate_ref) = service_fixture_for_request_status(request, ApprovalStatus::Approved); coordinator.set_status(TurnStatus::Queued); diff --git a/docs/reborn/contracts/approvals.md b/docs/reborn/contracts/approvals.md index c063937c94e..fcb981d6ded 100644 --- a/docs/reborn/contracts/approvals.md +++ b/docs/reborn/contracts/approvals.md @@ -256,8 +256,14 @@ This slice intentionally keeps approval resolution narrow: - no approval support for actions other than dispatch and one-shot spawn yet - persistent approval policies cover dispatch and `Action::SpawnCapability` approval interaction decisions at the current Reborn sandbox scope - (`tenant_id`, `user_id`, optional `agent_id`, and `project_id` when present, - otherwise `thread_id`) + (`tenant_id`, `user_id`, optional `agent_id`, and optional `project_id`); + `thread_id` never participates in the scope, so an "always allow" granted in + one thread applies to all of that user's threads with the same agent (and + project, when present) and is channel-agnostic — a WebUI grant is honored for + a Slack message resolving to the same `(user, agent)`. Existing local + thread-scoped approval-policy files from pre-DB testing are intentionally not + migrated or read through a compatibility fallback; wipe local approval state + when moving to this scope shape - persistent approval is fail-closed by manifest policy: the current default only allows durable reuse for capabilities whose manifest `default_permission` is `allow`; `ask` and `deny` remain one-shot approval