diff --git a/cedar-policy-core/src/tpe/evaluator.rs b/cedar-policy-core/src/tpe/evaluator.rs index 96b7fbd5c..288d13c88 100644 --- a/cedar-policy-core/src/tpe/evaluator.rs +++ b/cedar-policy-core/src/tpe/evaluator.rs @@ -136,6 +136,34 @@ impl Evaluator<'_> { }, .. } => left, + Residual::Concrete { + value: + Value { + value: ValueKind::Lit(ast::Literal::Bool(false)), + .. + }, + .. + } => { + if !left.can_error_assuming_well_formed() { + // simplify && false == false + Residual::Concrete { + value: false.into(), + ty, + } + } else { + // cannot simplify && false + Residual::Partial { + kind: ResidualKind::And { + left: Arc::new(left), + right: Arc::new(Residual::Concrete { + value: false.into(), + ty: ty.clone(), + }), + }, + ty, + } + } + } right => Residual::Partial { kind: ResidualKind::And { left: Arc::new(left), @@ -179,6 +207,34 @@ impl Evaluator<'_> { }, .. } => left, + Residual::Concrete { + value: + Value { + value: ValueKind::Lit(ast::Literal::Bool(true)), + .. + }, + .. + } => { + if !left.can_error_assuming_well_formed() { + // simplify || true == true + Residual::Concrete { + value: true.into(), + ty, + } + } else { + // cannot simplify || true + Residual::Partial { + kind: ResidualKind::Or { + left: Arc::new(left), + right: Arc::new(Residual::Concrete { + value: true.into(), + ty: ty.clone(), + }), + }, + ty, + } + } + } right => Residual::Partial { kind: ResidualKind::Or { left: Arc::new(left), @@ -843,6 +899,58 @@ mod tests { .. } ); + // && false => false + // principal.name like "lucas-*" && 41 == 42 => false + assert_matches!( + eval.interpret_expr( + &builder().and( + builder().like( + builder().get_attr(builder().var(Var::Principal), "name".parse().unwrap()), + vec![ + PatternElem::Char('l'), + PatternElem::Char('u'), + PatternElem::Char('c'), + PatternElem::Char('a'), + PatternElem::Char('s'), + PatternElem::Char('-'), + PatternElem::Wildcard + ] + .into() + ), + builder().is_eq(builder().val(41), builder().val(42)) + ) + ) + .unwrap(), + Residual::Concrete { + value: Value { + value: ValueKind::Lit(Literal::Bool(false)), + .. + }, + .. + }, + ); + // && false cannot be simplified, e.g. + // principal.foo + 1 == 100 && 41 == 42 + assert_matches!( + eval.interpret_expr(&builder().and( + builder().is_eq( + builder().add( + builder().get_attr(builder().var(Var::Principal), "foo".parse().unwrap()), + builder().val(1) + ), + builder().val(100) + ), + builder().is_eq(builder().val(41), builder().val(42)) + )) + .unwrap(), + // cannot match against the full residual, because of the Arc in the And enum variant, + // and due to Residual not implementing the Eq trait, but this shows that the evaluator + // kept the residual partial and with an And clause. + Residual::Partial { + kind: ResidualKind::And { .. }, + .. + } + ); } #[test] @@ -917,6 +1025,58 @@ mod tests { .. } ); + // || true => true + // principal.name like "lucas-*" || 42 == 42 => true + assert_matches!( + eval.interpret_expr( + &builder().or( + builder().like( + builder().get_attr(builder().var(Var::Principal), "name".parse().unwrap()), + vec![ + PatternElem::Char('l'), + PatternElem::Char('u'), + PatternElem::Char('c'), + PatternElem::Char('a'), + PatternElem::Char('s'), + PatternElem::Char('-'), + PatternElem::Wildcard + ] + .into() + ), + builder().is_eq(builder().val(42), builder().val(42)) + ) + ) + .unwrap(), + Residual::Concrete { + value: Value { + value: ValueKind::Lit(Literal::Bool(true)), + .. + }, + .. + }, + ); + // || true cannot be simplified, e.g. + // principal.foo + 1 == 100 || 42 == 42 + assert_matches!( + eval.interpret_expr(&builder().or( + builder().is_eq( + builder().add( + builder().get_attr(builder().var(Var::Principal), "foo".parse().unwrap()), + builder().val(1) + ), + builder().val(100) + ), + builder().is_eq(builder().val(42), builder().val(42)) + )) + .unwrap(), + // cannot match against the full residual, because of the Arc in the Or enum variant, + // and due to Residual not implementing the Eq trait, but this shows that the evaluator + // kept the residual partial and with an Or clause. + Residual::Partial { + kind: ResidualKind::Or { .. }, + .. + } + ); } #[test] diff --git a/cedar-policy-core/src/tpe/residual.rs b/cedar-policy-core/src/tpe/residual.rs index 4c1f2ca77..145ba608d 100644 --- a/cedar-policy-core/src/tpe/residual.rs +++ b/cedar-policy-core/src/tpe/residual.rs @@ -85,6 +85,78 @@ impl Residual { Residual::Error(ty) => ty, } } + + /// Whether this residual can result in a runtime error, assuming that self is well-formed, that is, has been validated against a schema. + pub fn can_error_assuming_well_formed(&self) -> bool { + match self { + Residual::Concrete { .. } => false, + Residual::Error(_) => true, + Residual::Partial { kind, .. } => match kind { + // Keep the same order of cases here as in tpe::Evaluator::interpret + ResidualKind::Var(_) => false, + // The general rule here is that an expression can only error if any child expression can error. + ResidualKind::And { left, right } => { + left.can_error_assuming_well_formed() || right.can_error_assuming_well_formed() + } + ResidualKind::Or { left, right } => { + left.can_error_assuming_well_formed() || right.can_error_assuming_well_formed() + } + ResidualKind::If { + test_expr, + then_expr, + else_expr, + } => { + test_expr.can_error_assuming_well_formed() + || then_expr.can_error_assuming_well_formed() + || else_expr.can_error_assuming_well_formed() + } + ResidualKind::Is { expr, .. } => expr.can_error_assuming_well_formed(), + ResidualKind::Like { expr, .. } => expr.can_error_assuming_well_formed(), + + ResidualKind::BinaryApp { op, arg1, arg2 } => match op { + // Arithmetic operations could error due to integer overflow + ast::BinaryOp::Add => true, + ast::BinaryOp::Mul => true, + ast::BinaryOp::Sub => true, + + // Other binary operations follow the general rule. They are all enumerated here for clarity, although + // a _ case could be used. + ast::BinaryOp::Contains + | ast::BinaryOp::ContainsAll + | ast::BinaryOp::ContainsAny + | ast::BinaryOp::Eq + | ast::BinaryOp::GetTag + | ast::BinaryOp::HasTag + | ast::BinaryOp::In + | ast::BinaryOp::Less + | ast::BinaryOp::LessEq => { + arg1.can_error_assuming_well_formed() + || arg2.can_error_assuming_well_formed() + } + }, + + // Extension function invocations can error at runtime. + ResidualKind::ExtensionFunctionApp { .. } => true, + + ResidualKind::GetAttr { expr, .. } => expr.can_error_assuming_well_formed(), + ResidualKind::HasAttr { expr, .. } => expr.can_error_assuming_well_formed(), + + ResidualKind::UnaryApp { op, arg } => match op { + // Integer negation can error due to integer overflow + ast::UnaryOp::Neg => true, + + // General rule for the rest of the unary operations. + ast::UnaryOp::IsEmpty | ast::UnaryOp::Not => { + arg.can_error_assuming_well_formed() + } + }, + ResidualKind::Set(items) => items.iter().any(Self::can_error_assuming_well_formed), + ResidualKind::Record(attrs) => attrs + .iter() + .any(|(_, e)| e.can_error_assuming_well_formed()), + }, + } + } } impl TryFrom for Value { @@ -456,42 +528,136 @@ mod test { use super::*; use crate::extensions::Extensions; use crate::parser::parse_expr; - use crate::validator::typecheck::{TypecheckAnswer, Typechecker}; + use crate::tpe::request::{PartialEntityUID, PartialRequest}; + use crate::validator::typecheck::{PolicyCheck, Typechecker}; use crate::validator::{ValidationMode, Validator, ValidatorSchema}; use similar_asserts::assert_eq; #[track_caller] fn parse_residual(expr_str: &str) -> Residual { let expr = parse_expr(expr_str).unwrap(); + let policy_id = crate::ast::PolicyID::from_string("test"); + let policy = Policy::from_when_clause(Effect::Permit, expr, policy_id, None); + let t = policy.template(); - let schema = ValidatorSchema::from_cedarschema_str( - "entity User { foo: Bool };", + let schema = ValidatorSchema::from_cedarschema_str(r#" + entity User { foo: Bool, str: String, num: Long, period: __cedar::duration, set: Set } tags String; + entity Document; + action get appliesTo { principal: [User], resource: [Document] };"#, &Extensions::all_available(), ) .unwrap() .0; - let validator = Validator::new(schema); - let typechecker = Typechecker::new(validator.schema(), ValidationMode::Strict); - let mut type_errors = std::collections::HashSet::new(); - let policy_id = crate::ast::PolicyID::from_string("test"); + let typechecker = Typechecker::new(&schema, ValidationMode::Strict); - let typecheck_result = typechecker.typecheck_expr(&expr, &policy_id, &mut type_errors); + let request = PartialRequest::new( + PartialEntityUID { + ty: "User".parse().unwrap(), + eid: None, + }, + r#"Action::"get""#.parse().unwrap(), + PartialEntityUID { + ty: "Document".parse().unwrap(), + eid: None, + }, + None, + &schema, + ) + .unwrap(); + let env = request.find_request_env(&schema).unwrap(); - match typecheck_result { - TypecheckAnswer::TypecheckSuccess { expr_type, .. } => { - Residual::try_from(&expr_type).unwrap() - } - _ => { - println!("got {} type errors", type_errors.len()); - for e in type_errors { + let errs: Vec<_> = Validator::validate_entity_types_and_literals(&schema, t).collect(); + if !errs.is_empty() { + panic!("unexpected type error in expression"); + } + match typechecker.typecheck_by_single_request_env(t, &env) { + PolicyCheck::Success(expr) => Residual::try_from(&expr).unwrap(), + PolicyCheck::Fail(errs) => { + println!("got {} type errors", errs.len()); + for e in errs { println!("{:?}", miette::Report::new(e)); } - panic!("unexpected type error in expression"); + panic!("unexpected type error in expression") + } + PolicyCheck::Irrelevant(errs, expr) => { + if errs.is_empty() { + Residual::try_from(&expr).unwrap() + } else { + println!("got {} type errors", errs.len()); + for e in errs { + println!("{:?}", miette::Report::new(e)); + } + panic!("unexpected type error in expression") + } } } } + #[test] + fn test_can_error_assuming_well_formed() { + assert_eq!( + parse_residual(r#"principal has foo && {a: true, b: false}["a"]"#) + .can_error_assuming_well_formed(), + false + ); + assert_eq!( + parse_residual(r#"principal is User || User::"jane".str like "jane-*""#) + .can_error_assuming_well_formed(), + false + ); + assert_eq!( + parse_residual( + r#"if principal.num > 0 then User::"jane".num >= 100 else User::"foo".num == 1"# + ) + .can_error_assuming_well_formed(), + false + ); + assert_eq!( + parse_residual(r#"principal.hasTag("foo") && principal.getTag("foo") == "bar""#) + .can_error_assuming_well_formed(), + false + ); + assert_eq!( + parse_residual(r#"!principal.set.isEmpty() && (principal.set.contains("foo") || principal.set.containsAll(["foo", "bar"]) || principal.set.containsAny(["foo", "bar"]))"#).can_error_assuming_well_formed(), + false + ); + assert_eq!( + parse_residual(r#"User::"jane" in [User::"foo", User::"jane"]"#) + .can_error_assuming_well_formed(), + false + ); + assert_eq!( + parse_residual(r#"principal.num + 1 == 100 || true"#).can_error_assuming_well_formed(), + true + ); + assert_eq!( + parse_residual(r#"if principal.foo then principal.num - 1 == 100 else true"#) + .can_error_assuming_well_formed(), + true + ); + assert_eq!( + parse_residual(r#"principal.foo && principal.num * 2 == 100"#) + .can_error_assuming_well_formed(), + true + ); + assert_eq!( + parse_residual(r#"principal.foo || -principal.num == 100"#) + .can_error_assuming_well_formed(), + true + ); + assert_eq!( + parse_residual(r#"principal.num == 1 && principal.period < (if principal.foo then duration("1d") else duration("2d"))"#).can_error_assuming_well_formed(), + true + ); + // in reality, this specific function could most likely never error + // in the future, we might want to be more precise about exactly what functions could produce errors + assert_eq!( + parse_residual(r#"principal.period.toDays() == 365"#).can_error_assuming_well_formed(), + true + ); + } + mod literal_uids { use similar_asserts::assert_eq; use std::collections::HashSet; @@ -501,7 +667,7 @@ mod test { #[test] fn var() { assert_eq!( - parse_residual("principal").all_literal_uids(), + parse_residual("principal.foo").all_literal_uids(), HashSet::new() ); } @@ -509,8 +675,10 @@ mod test { #[test] fn r#if() { assert_eq!( - parse_residual(r#"if User::"alice".foo then User::"bob" else User::"jane""#) - .all_literal_uids(), + parse_residual( + r#"if User::"alice".foo then User::"bob".foo else User::"jane".foo"# + ) + .all_literal_uids(), HashSet::from([ r#"User::"alice""#.parse().unwrap(), r#"User::"bob""#.parse().unwrap(), @@ -533,7 +701,7 @@ mod test { #[test] fn set() { assert_eq!( - parse_residual(r#"[User::"alice", User::"jane"]"#).all_literal_uids(), + parse_residual(r#"principal in [User::"alice", User::"jane"]"#).all_literal_uids(), HashSet::from([ r#"User::"alice""#.parse().unwrap(), r#"User::"jane""#.parse().unwrap(), @@ -544,7 +712,7 @@ mod test { #[test] fn record() { assert_eq!( - parse_residual(r#"{a: User::"alice", b: User::"jane"}"#).all_literal_uids(), + parse_residual(r#"(if principal.foo then {a: User::"alice", b: true} else {a: User::"jane", b: false}).a.foo"#).all_literal_uids(), HashSet::from([ r#"User::"alice""#.parse().unwrap(), r#"User::"jane""#.parse().unwrap(), @@ -554,9 +722,15 @@ mod test { } fn assert_eq_expr(expr_str: &str) { - let e: Expr = expr_str.parse().unwrap(); + // The unconstrained + let e: Expr = format!("true && (true && (true && ({})))", expr_str) + .parse() + .unwrap(); let residual = parse_residual(expr_str); - assert_eq!(Expr::from(residual), e); + let e2 = Expr::from(residual); + println!("e: {}", e); + println!("e2: {}", e2); + assert_eq!(e, e2); } #[test] @@ -565,9 +739,11 @@ mod test { assert_eq_expr(r#"User::"alice".foo || User::"jane".foo"#); assert_eq_expr(r#"[User::"jane".foo].contains(User::"jane".foo)"#); assert_eq_expr(r#"User::"alice" has foo"#); - assert_eq_expr(r#"if User::"alice".foo then User::"bob" else User::"jane""#); + assert_eq_expr(r#"(if User::"alice".foo then User::"bob" else User::"jane").foo"#); assert_eq_expr(r#""foo" like "bar""#); - assert_eq_expr(r#"[User::"alice", User::"jane"]"#); - assert_eq_expr(r#"{a: User::"alice", b: User::"jane"}"#); + assert_eq_expr(r#"principal in [User::"alice", User::"jane"]"#); + assert_eq_expr( + r#"(if principal.foo then {a: User::"alice", b: true} else {a: User::"jane", b: false}).a.foo"#, + ); } } diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index e15e960cb..64d3137b3 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -23,7 +23,7 @@ the `ffi` module (new impls on `ffi::EntityUid`, `ffi::Context`, `ffi::Entities` `ffi::Template`, and `ffi::StaticPolicySet`) (#2085) - `schema_to_json_with_resolved_types()` function, which takes in a Cedar schema and returns a json schema without any instances of EntityOrCommon; they're all either Entity or CommonType (#2058) - More derives (`PartialEq`, `Clone`, etc) for a number of types in the `ffi` module (#2083) - +- TPE: Simplify ` && false` to `false` and ` || true` to `true` when `` is error-free. (#2091) ## [4.8.2] - 2025-12-09