diff --git a/cedar-drt/fuzz/fuzz_targets/tc-repair-equivalence.rs b/cedar-drt/fuzz/fuzz_targets/tc-repair-equivalence.rs index f0437e343..637300e3e 100644 --- a/cedar-drt/fuzz/fuzz_targets/tc-repair-equivalence.rs +++ b/cedar-drt/fuzz/fuzz_targets/tc-repair-equivalence.rs @@ -193,7 +193,7 @@ fuzz_target!(|input: FuzzTargetInput| { } let touched = t.add_new_edges(&mut incremental_entities, &input.new_edges); - let repair_result = repair_tc(touched, &mut incremental_entities, true); + let repair_result = repair_tc(&touched, &mut incremental_entities, true); // Both must agree on whether the graph is a DAG. assert_eq!( @@ -224,7 +224,7 @@ fuzz_target!(|input: FuzzTargetInput| { let mut incremental_no_dag = t.build_entities(input.num_nodes, &input.pre_existing_edges); let _ = compute_tc(&mut incremental_no_dag, false); let touched = t.add_new_edges(&mut incremental_no_dag, &input.new_edges); - let _ = repair_tc(touched, &mut incremental_no_dag, false); + let _ = repair_tc(&touched, &mut incremental_no_dag, false); let expected = ancestor_sets(&expected_no_dag); let actual = ancestor_sets(&incremental_no_dag); diff --git a/cedar-drt/fuzz/fuzz_targets/tpe-pbt.rs b/cedar-drt/fuzz/fuzz_targets/tpe-pbt.rs index 730db225e..db311e512 100644 --- a/cedar-drt/fuzz/fuzz_targets/tpe-pbt.rs +++ b/cedar-drt/fuzz/fuzz_targets/tpe-pbt.rs @@ -65,7 +65,7 @@ fuzz_target!(|input: TpeFuzzTargetInput| { .tpe(&partial_request, &partial_entities, &schema) .expect("tpe failed"); assert!(test_weak_equiv( - &PolicySet::from_policies(response.residual_policies()).unwrap(), + &PolicySet::from_policies(response.policies()).unwrap(), &policyset, &request, &input.abac_input.entities diff --git a/cedar-drt/fuzz/src/tpe.rs b/cedar-drt/fuzz/src/tpe.rs index 9d7e5843c..18903fa5d 100644 --- a/cedar-drt/fuzz/src/tpe.rs +++ b/cedar-drt/fuzz/src/tpe.rs @@ -16,7 +16,7 @@ //! Test utilities for type-directed partial evaluation fuzz targets -use cedar_lean_ffi::CedarLeanFfi; +use cedar_lean_ffi::{CedarLeanFfi, FfiError}; use cedar_policy::pst::{Clause, Expr, UnaryOp}; use cedar_policy::{ Entity, EntityId, EntityUid, PartialEntities, PartialEntity, PartialEntityUid, PartialRequest, @@ -26,7 +26,7 @@ use cedar_policy_core::ast::{self, Value}; use cedar_policy_generators::abac::ABACRequest; use libfuzzer_sys::arbitrary::{self, Arbitrary, Unstructured}; use ref_cast::RefCast; -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::TryFrom; use std::sync::Arc; @@ -212,7 +212,6 @@ pub fn test_tpe_is_authorized_equiv( let (rust_resp, lean_resp) = match (maybe_rust_resp, maybe_lean_resp) { (Ok(r), Ok(l)) => (r, l), - (Err(_), Err(_)) => return, (Ok(r), Err(e)) => panic!( "Got Lean TPE error, but Rust returned response.\nRust: {:?}\n, Lean: {}", r, e @@ -221,96 +220,104 @@ pub fn test_tpe_is_authorized_equiv( "Got Rust TPE error, but Lean returned response.\nRust: {}\n, Lean: {:?}", e, l ), + // LeanBackendError is returned for expected error conditions like ill-typed policies + (Err(_), Err(FfiError::LeanBackendError(_))) => return, + // other FfiError variants indicate a bug in the FFI layer + (Err(_), Err(e)) => panic!("Unexpected FfiError: {e:?}"), }; - let rust_inner = rust_resp.as_ref(); - // Compare decisions assert_eq!( - rust_inner.decision(), + rust_resp.decision(), lean_resp.decision, "TPE decision mismatch" ); // Compare policy categorizations (comparing sets of policy IDs) - let to_set = - |iter: Box + '_>| { - iter.map(|p| PolicyId::new(p.get_policy_id().as_ref())) - .collect::>() - }; + fn to_set<'a>(iter: impl Iterator + 'a) -> HashSet { + iter.cloned().collect::>() + } // The satisfied forbids/permits match. assert_eq!( - to_set(Box::new(rust_inner.satisfied_permits())), + to_set(rust_resp.true_permits()), lean_resp.satisfied_permits, "satisfied_permits mismatch" ); assert_eq!( - to_set(Box::new(rust_inner.satisfied_forbids())), + to_set(rust_resp.true_forbids()), lean_resp.satisfied_forbids, "satisfied_forbids mismatch" ); - // Rust only returns false_permits, which should be the union of the - // false_permits and error_permits of the Lean side. + // False permits/forbids match + assert_eq!( + to_set(rust_resp.false_permits()), + lean_resp.false_permits, + "false_permits mismatch" + ); + assert_eq!( + to_set(rust_resp.false_forbids()), + lean_resp.false_forbids, + "false_forbids mismatch" + ); + // Error permits/forbids match assert_eq!( - to_set(Box::new(rust_inner.false_permits())), - &lean_resp.false_permits | &lean_resp.error_permits, - "rust false_permits mismatch with false permits union error permits" + to_set(rust_resp.error_permits()), + lean_resp.error_permits, + "error_permits mismatch" ); - // Same for the false_forbids and error_forbids. The Rust side puts the policy ID - // in false_forbids for all Effect::Forbid policies that result in false or error. assert_eq!( - to_set(Box::new(rust_inner.false_forbids())), - &lean_resp.false_forbids | &lean_resp.error_forbids, - "rust false_forbids mismatch with false forbids union error forbids" + to_set(rust_resp.error_forbids()), + lean_resp.error_forbids, + "error_forbids mismatch" ); // The policies with residuals match on both sides. assert_eq!( - to_set(Box::new(rust_inner.residual_permits())), + to_set(rust_resp.residual_permits()), lean_resp.residual_permits, "residual_permits mismatch" ); assert_eq!( - to_set(Box::new(rust_inner.residual_forbids())), + to_set(rust_resp.residual_forbids()), lean_resp.residual_forbids, "residual_forbids mismatch" ); // Compare residual expressions by policy ID via PST - let rust_residual_map: std::collections::HashMap = rust_resp - .residual_policies() + let rust_residual_map: HashMap = rust_resp + .policies() .map(|rp| { - let id: String = rp.id().to_string(); let pst = rp .to_pst() - .expect("policy->pst conversion should succeeed for residuals"); - // Residual should only have one clause + .expect("policy->pst conversion should succeed for residuals"); + // Residual should have exactly one clause let clause = match pst.body().clauses().as_slice() { [Clause::When(x)] => x.clone(), [Clause::Unless(x)] => Arc::new(Expr::UnaryOp { op: UnaryOp::Not, expr: x.clone(), }), - _ => panic!(""), + _ => panic!("zero or multiple when/unless clauses in residual policy"), }; - (id, Arc::unwrap_or_clone(clause)) + (rp.id().clone(), Arc::unwrap_or_clone(clause)) }) .collect(); for lean_rp in &lean_resp.residuals { let lean_pst = Expr::try_from(lean_rp.residual.clone()) .expect("lean residual->pst conversion should succeed"); - let id_str = lean_rp.id.to_string(); - let rust_pst = rust_residual_map.get(&id_str).unwrap_or_else(|| { + let rust_pst = rust_residual_map.get(&lean_rp.id).unwrap_or_else(|| { panic!( - "Lean returned residual for policy {id_str:?} but Rust did not.\n\ + "Lean returned residual for policy {:?} but Rust did not.\n\ Rust residual IDs: {:?}", + lean_rp.id, rust_residual_map.keys().collect::>() ) }); assert_eq!( rust_pst, &lean_pst, - "Residual expression mismatch for policy {id_str:?}\n\ - Rust PST: {rust_pst:?}\nLean PST: {lean_pst:?}" + "Residual expression mismatch for policy {:?}\n\ + Rust PST: {rust_pst:?}\nLean PST: {lean_pst:?}", + lean_rp.id, ); } } diff --git a/cedar-lean-ffi/src/lean_ffi/tpe.rs b/cedar-lean-ffi/src/lean_ffi/tpe.rs index 6a91cba87..2fa95fcf1 100644 --- a/cedar-lean-ffi/src/lean_ffi/tpe.rs +++ b/cedar-lean-ffi/src/lean_ffi/tpe.rs @@ -90,7 +90,7 @@ mod test { PartialRequest, Policy, PolicyId, PolicySet, RestrictedExpression, Schema, }; - use std::collections::{BTreeMap, HashSet}; + use std::collections::{BTreeMap, HashMap, HashSet}; use std::str::FromStr; use crate::CedarLeanFfi; @@ -108,79 +108,79 @@ mod test { lean: &crate::datatypes::TpeResponse, ) { let mut errors = Vec::new(); - let rust_inner = rust.as_ref(); - // Compare decisions - if lean.decision != rust_inner.decision() { + if lean.decision != rust.decision() { errors.push(format!( "decision mismatch: lean={:?}, rust={:?}", lean.decision, - rust_inner.decision() + rust.decision() )); } - // Compare policy categorizations - let policy_ids = |iter: Box< - dyn Iterator + '_, - >| - -> HashSet { - iter.map(|p| PolicyId::new(p.get_policy_id().as_ref())) - .collect() - }; - - macro_rules! check_set { - ($lean_field:expr, $rust_iter:expr, $name:expr) => { - let rust_set = policy_ids(Box::new($rust_iter)); - if $lean_field != rust_set { - errors.push(format!( - "{} mismatch:\n lean={:?}\n rust={:?}", - $name, $lean_field, rust_set - )); - } - }; + fn check_set<'a>( + lean_field: &HashSet, + rust_iter: impl Iterator, + name: &str, + errors: &mut Vec, + ) { + let rust_set: HashSet = rust_iter.cloned().collect(); + if lean_field != &rust_set { + errors.push(format!( + "{} mismatch:\n lean={:?}\n rust={:?}", + name, lean_field, rust_set + )); + } } - check_set!( - lean.satisfied_permits, - rust_inner.satisfied_permits(), - "satisfied_permits" + check_set( + &lean.satisfied_permits, + rust.true_permits(), + "satisfied_permits", + &mut errors, ); - check_set!( - lean.satisfied_forbids, - rust_inner.satisfied_forbids(), - "satisfied_forbids" + check_set( + &lean.satisfied_forbids, + rust.true_forbids(), + "satisfied_forbids", + &mut errors, ); - check_set!( - lean.false_permits, - rust_inner.false_permits(), - "false_permits" + check_set( + &lean.false_permits, + rust.false_permits(), + "false_permits", + &mut errors, ); - check_set!( - lean.false_forbids, - rust_inner.false_forbids(), - "false_forbids" + check_set( + &lean.false_forbids, + rust.false_forbids(), + "false_forbids", + &mut errors, ); - check_set!( - lean.residual_permits, - rust_inner.residual_permits(), - "residual_permits" + check_set( + &lean.residual_permits, + rust.residual_permits(), + "residual_permits", + &mut errors, ); - check_set!( - lean.residual_forbids, - rust_inner.residual_forbids(), - "residual_forbids" + check_set( + &lean.residual_forbids, + rust.residual_forbids(), + "residual_forbids", + &mut errors, ); // Compare residual expressions via PST use cedar_policy_core::pst; - let rust_residual_map: std::collections::HashMap = rust_inner - .residual_policies() + let rust_residual_map: HashMap = rust + .policies() .map(|rp| { - let id = rp.get_policy_id().to_string(); - let ast_expr = - cedar_policy_core::ast::Expr::from(rp.get_residual().as_ref().clone()); - let pst_expr = pst::Expr::try_from(ast_expr) + let id = rp.id().to_string(); + let pst_policy = rp + .to_pst() .expect("ast->pst conversion should succeed for residuals"); - (id, pst_expr) + let [pst::Clause::When(pst_expr)] = pst_policy.body().clauses().as_slice() else { + panic!("expected exactly one `when` clause in residual") + }; + (id, pst_expr.as_ref().clone()) }) .collect(); diff --git a/cedar-policy-generators/src/policy_set.rs b/cedar-policy-generators/src/policy_set.rs index e626fa1f6..382b687da 100644 --- a/cedar-policy-generators/src/policy_set.rs +++ b/cedar-policy-generators/src/policy_set.rs @@ -79,10 +79,9 @@ impl From for ast::PolicySet { } #[cfg(feature = "cedar-policy")] -impl TryFrom for cedar_policy::PolicySet { - type Error = cedar_policy::PolicySetError; - fn try_from(generated: GeneratedPolicySet) -> Result { - ast::PolicySet::from(generated).try_into() +impl From for cedar_policy::PolicySet { + fn from(generated: GeneratedPolicySet) -> Self { + ast::PolicySet::from(generated).into() } }