Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions crates/ironclaw_capability_policy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
//!
//! [nearai/ironclaw#4628]: https://github.com/nearai/ironclaw/issues/4628

use std::collections::HashMap;

use async_trait::async_trait;
use ironclaw_host_api::{CapabilityId, PermissionMode, ProjectId, TenantId, UserId};
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -230,6 +232,72 @@ pub trait PolicyResolver: Send + Sync {
) -> Result<EffectivePolicy, PolicyError>;
}

impl CapabilityDefaultPolicy {
/// A conservative global fallback for capabilities that declare no policy:
/// hidden, no credential, ask before running. Fail-closed on availability.
pub fn conservative_fallback() -> Self {
Self {
availability: Availability::Hidden,
identity: IdentityMode::None,
approval: PermissionMode::Ask,
config: Value::Null,
}
}
}

/// Source of per-capability default policy (architecture doc §7).
///
/// The capability manifest is the source of truth; this trait decouples lookup
/// from where manifests are registered, so a per-capability default can be
/// sourced without adding a field to the 49-construction-site
/// `CapabilityDescriptor`. Capabilities that declare no policy fall back to a
/// conservative global default.
pub trait CapabilityDefaultPolicySource: Send + Sync {
/// The default policy for `capability`, or the global fallback when the
/// capability declares none.
fn default_for(&self, capability: &CapabilityId) -> CapabilityDefaultPolicy;
}
Comment on lines +255 to +259

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To avoid unnecessary cloning of CapabilityDefaultPolicy (which contains a heap-allocated serde_json::Value in its config field), consider defining default_for to return a reference &CapabilityDefaultPolicy instead of an owned value. Since the lookup is performed against a read-only source, returning a reference avoids overhead on what may be a frequent capability dispatch path.

Suggested change
pub trait CapabilityDefaultPolicySource: Send + Sync {
/// The default policy for `capability`, or the global fallback when the
/// capability declares none.
fn default_for(&self, capability: &CapabilityId) -> CapabilityDefaultPolicy;
}
pub trait CapabilityDefaultPolicySource: Send + Sync {
/// The default policy for `capability`, or the global fallback when the
/// capability declares none.
fn default_for(&self, capability: &CapabilityId) -> &CapabilityDefaultPolicy;
}
References
  1. Prefer routing through centralized helper functions to maintain drift-resistance and avoid duplicating domain knowledge locally, even if it introduces minor performance overhead (like cloning) on non-hot paths. However, on hot paths (like frequent capability dispatch), avoiding cloning by returning references is preferred.


/// In-memory [`CapabilityDefaultPolicySource`]: explicit per-capability entries
/// over a global fallback. Seeded from manifest declarations at composition
/// time.
#[derive(Debug, Clone)]
pub struct StaticCapabilityDefaultPolicySource {
fallback: CapabilityDefaultPolicy,
entries: HashMap<CapabilityId, CapabilityDefaultPolicy>,
}

impl StaticCapabilityDefaultPolicySource {
/// Create a source with the given global fallback and no entries.
pub fn new(fallback: CapabilityDefaultPolicy) -> Self {
Self {
fallback,
entries: HashMap::new(),
}
}

/// Builder-style insert of a per-capability default.
#[must_use]
pub fn with_entry(mut self, capability: CapabilityId, policy: CapabilityDefaultPolicy) -> Self {
self.entries.insert(capability, policy);
self
}

/// Insert or replace a per-capability default.
pub fn insert(&mut self, capability: CapabilityId, policy: CapabilityDefaultPolicy) {
self.entries.insert(capability, policy);
}
}

impl CapabilityDefaultPolicySource for StaticCapabilityDefaultPolicySource {
fn default_for(&self, capability: &CapabilityId) -> CapabilityDefaultPolicy {
self.entries
.get(capability)
.cloned()
.unwrap_or_else(|| self.fallback.clone())
}
}
Comment on lines +292 to +299

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Update the implementation of CapabilityDefaultPolicySource to return a reference, matching the trait signature change and avoiding cloning the policy.

impl CapabilityDefaultPolicySource for StaticCapabilityDefaultPolicySource {
    fn default_for(&self, capability: &CapabilityId) -> &CapabilityDefaultPolicy {
        self.entries
            .get(capability)
            .unwrap_or(&self.fallback)
    }
}


#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -265,6 +333,50 @@ mod tests {
}
}

#[test]
fn default_source_returns_entry_then_fallback() {
let entry = CapabilityDefaultPolicy {
availability: Availability::Available,
identity: IdentityMode::None,
approval: PermissionMode::Allow,
config: json!({}),
};
let source = StaticCapabilityDefaultPolicySource::new(
CapabilityDefaultPolicy::conservative_fallback(),
)
.with_entry(cap(), entry.clone());
assert_eq!(source.default_for(&cap()), entry);

let unknown = CapabilityId::new("shell.exec").expect("valid capability id");
assert_eq!(
source.default_for(&unknown),
CapabilityDefaultPolicy::conservative_fallback()
);
}
Comment on lines +336 to +355

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Update the test to compare references instead of owned values, matching the updated default_for signature.

Suggested change
#[test]
fn default_source_returns_entry_then_fallback() {
let entry = CapabilityDefaultPolicy {
availability: Availability::Available,
identity: IdentityMode::None,
approval: PermissionMode::Allow,
config: json!({}),
};
let source = StaticCapabilityDefaultPolicySource::new(
CapabilityDefaultPolicy::conservative_fallback(),
)
.with_entry(cap(), entry.clone());
assert_eq!(source.default_for(&cap()), entry);
let unknown = CapabilityId::new("shell.exec").expect("valid capability id");
assert_eq!(
source.default_for(&unknown),
CapabilityDefaultPolicy::conservative_fallback()
);
}
#[test]
fn default_source_returns_entry_then_fallback() {
let entry = CapabilityDefaultPolicy {
availability: Availability::Available,
identity: IdentityMode::None,
approval: PermissionMode::Allow,
config: json!({}),
};
let source = StaticCapabilityDefaultPolicySource::new(
CapabilityDefaultPolicy::conservative_fallback(),
)
.with_entry(cap(), entry.clone());
assert_eq!(source.default_for(&cap()), &entry);
let unknown = CapabilityId::new("shell.exec").expect("valid capability id");
assert_eq!(
source.default_for(&unknown),
&CapabilityDefaultPolicy::conservative_fallback()
);
}
References
  1. In tests, when setting up a state that depends on environmental factors, prefer expect() to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks like unwrap_or() that could cause the test to silently check the wrong logic.


#[test]
fn conservative_fallback_is_hidden_and_ask() {
let fallback = CapabilityDefaultPolicy::conservative_fallback();
assert_eq!(fallback.availability, Availability::Hidden);
assert_eq!(fallback.approval, PermissionMode::Ask);
assert_eq!(fallback.identity, IdentityMode::None);
}

#[test]
fn source_default_feeds_the_fold() {
// An admin per-user grant flips the conservative hidden default to
// available for one user.
let source = StaticCapabilityDefaultPolicySource::new(
CapabilityDefaultPolicy::conservative_fallback(),
);
let mut user = delta(PolicyScope::User {
user_id: uid("bob"),
});
user.availability = Some(Availability::Available);
let eff = resolve_effective_policy(&source.default_for(&cap()), &[user]);
assert!(eff.available);
}
Comment on lines +365 to +378

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Update the test to pass the reference returned by default_for directly to resolve_effective_policy without borrowing it again.

Suggested change
#[test]
fn source_default_feeds_the_fold() {
// An admin per-user grant flips the conservative hidden default to
// available for one user.
let source = StaticCapabilityDefaultPolicySource::new(
CapabilityDefaultPolicy::conservative_fallback(),
);
let mut user = delta(PolicyScope::User {
user_id: uid("bob"),
});
user.availability = Some(Availability::Available);
let eff = resolve_effective_policy(&source.default_for(&cap()), &[user]);
assert!(eff.available);
}
#[test]
fn source_default_feeds_the_fold() {
// An admin per-user grant flips the conservative hidden default to
// available for one user.
let source = StaticCapabilityDefaultPolicySource::new(
CapabilityDefaultPolicy::conservative_fallback(),
);
let mut user = delta(PolicyScope::User {
user_id: uid("bob"),
});
user.availability = Some(Availability::Available);
let eff = resolve_effective_policy(source.default_for(&cap()), &[user]);
assert!(eff.available);
}


#[test]
fn default_only_resolves_to_default() {
let eff = resolve_effective_policy(&default_policy(), &[]);
Expand Down
Loading