diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs index 57e9d4f0d..f145b4230 100644 --- a/cedar-policy-core/src/ast/expr.rs +++ b/cedar-policy-core/src/ast/expr.rs @@ -1861,6 +1861,14 @@ impl Expr { } } } + + pub(crate) fn is_true(&self) -> bool { + matches!(self.expr_kind(), ExprKind::Lit(Literal::Bool(true))) + } + + pub(crate) fn is_false(&self) -> bool { + matches!(self.expr_kind(), ExprKind::Lit(Literal::Bool(false))) + } } /// AST variables diff --git a/cedar-policy-core/src/ast/name.rs b/cedar-policy-core/src/ast/name.rs index 349d37628..b5f0de7f1 100644 --- a/cedar-policy-core/src/ast/name.rs +++ b/cedar-policy-core/src/ast/name.rs @@ -523,7 +523,7 @@ impl Name { self.0.basename().clone().try_into().unwrap() } - /// Test if a [`Name`] is an [`Id`] + /// Test if a [`Name`] is an [`UnreservedId`] pub fn is_unqualified(&self) -> bool { self.0.is_unqualified() } diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index 3533ef863..a7bba01f6 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -4485,6 +4485,77 @@ mod test { ); } } + + #[test] + fn extended_has() { + let policy_text = r#" + permit(principal, action, resource) when + { principal has a.b.c };"#; + let cst = parser::text_to_cst::parse_policy(policy_text).unwrap(); + let est: Policy = cst.node.unwrap().try_into().unwrap(); + assert_eq!( + est, + serde_json::from_value(json!({ + "effect": "permit", + "principal": { "op": "All" }, + "action": { "op": "All" }, + "resource": { "op": "All" }, + "conditions": [ + { + "kind": "when", + "body": { + "&&": { + "left": { + "&&": { + "left": { + "has": { + "left": { + "Var": "principal", + }, + "attr": "a" + } + }, + "right": { + "has": { + "left": { + ".": { + "left": { + "Var": "principal", + }, + "attr": "a", + }, + }, + "attr": "b" + } + }, + } + }, + "right": { + "has": { + "left": { + ".": { + "left": { + ".": { + "left": { + "Var": "principal", + }, + "attr": "a" + } + }, + "attr": "b", + } + }, + "attr": "c", + } + }, + }, + }, + } + ] + })) + .unwrap() + ); + } } #[cfg(test)] diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs index 383cfe196..e87b6aea0 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -29,6 +29,7 @@ use crate::parser::util::flatten_tuple_2; use crate::parser::{Loc, Node}; use either::Either; use itertools::Itertools; +use nonempty::nonempty; use serde::{de::Visitor, Deserialize, Serialize}; use serde_with::serde_as; use smol_str::{SmolStr, ToSmolStr}; @@ -1170,11 +1171,23 @@ impl TryFrom<&Node>> for Expr { Ok(expr) } cst::Relation::Has { target, field } => { - let target_expr = target.try_into()?; - field - .to_expr_or_special()? - .into_valid_attr() - .map(|attr| Expr::has_attr(target_expr, attr)) + let target_expr: Expr = target.try_into()?; + let attrs = match field.to_has_rhs()? { + Either::Left(attr) => nonempty![attr], + Either::Right(ids) => ids.map(|id| id.to_smolstr()), + }; + let (first, rest) = attrs.split_first(); + let has_expr = Expr::has_attr(target_expr.clone(), first.clone()); + let get_expr = Expr::get_attr(target_expr, first.clone()); + Ok(rest + .iter() + .fold((has_expr, get_expr), |(has_expr, get_expr), attr| { + ( + Expr::and(has_expr, Expr::has_attr(get_expr.clone(), attr.to_owned())), + Expr::get_attr(get_expr, attr.to_owned()), + ) + }) + .0) } cst::Relation::Like { target, pattern } => { let target_expr = target.try_into()?; diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index 08ea58e67..4f245a70a 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -5525,7 +5525,7 @@ pub mod test { } #[test] - fn parital_if_noerrors() { + fn partial_if_noerrors() { let guard = Expr::get_attr(Expr::unknown(Unknown::new_untyped("a")), "field".into()); let cons = Expr::val(1); let alt = Expr::val(2); @@ -6144,4 +6144,72 @@ pub mod test { ); assert_matches!(eval.partial_eval_expr(&e), Err(_)); } + + #[test] + fn interpret_extended_has() { + let es = Entities::new(); + let eval = Evaluator::new(empty_request(), &es, Extensions::none()); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has a.b.c + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(true)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has a.b + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(true)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has a + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(true)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has b.c + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(false)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has c + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(false)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has d + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(false)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has "🚫" + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(false)); + }); + + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has a.b.c && {a: {b: {c: 1}}}.a.b.c == 1 + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(true)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has a.b && {a: {b: {c: 1}}}.a.b == {c: 1} + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(true)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has a && {a: {b: {c: 1}}}.a == {b: {c: 1}} + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(true)); + }); + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {d: 1}}} has a.b.c && {a: {b: {d: 1}}}.a.b.c == 1 + "#).unwrap()), Ok(v) => { + assert_eq!(v, Value::from(false)); + }); + + assert_matches!(eval.interpret_inline_policy(&parse_expr(r#" + {a: {b: {c: 1}}} has a.b && {a: {b: {c: 1}}}.a.b.d == 1 + "#).unwrap()), Err(EvaluationError::RecordAttrDoesNotExist(err)) => { + assert_eq!(err.attr, "d"); + }); + } } diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 94ad1d87c..cebc8e024 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -46,6 +46,7 @@ use crate::ast::{ use crate::est::extract_single_argument; use crate::fuzzy_match::fuzzy_search_limited; use itertools::Either; +use nonempty::nonempty; use nonempty::NonEmpty; use smol_str::{SmolStr, ToSmolStr}; use std::cmp::Ordering; @@ -950,10 +951,13 @@ impl Node> { } cst::Relation::Has { target, field } => { let maybe_target = target.to_expr(); - let maybe_field = field.to_expr_or_special()?.into_valid_attr(); + let maybe_field = Ok(match field.to_has_rhs()? { + Either::Left(s) => nonempty![s], + Either::Right(ids) => ids.map(|id| id.to_smolstr()), + }); let (target, field) = flatten_tuple_2(maybe_target, maybe_field)?; Ok(ExprOrSpecial::Expr { - expr: construct_expr_has(target, field, self.loc.clone()), + expr: construct_exprs_extended_has(target, field, self.loc.clone()), loc: self.loc.clone(), }) } @@ -1001,6 +1005,114 @@ impl Node> { fn to_expr(&self) -> Result { self.to_expr_or_special()?.into_expr() } + + // Peel the grammar onion until we see valid RHS + // This function is added to implement RFC 62 (extended `has` operator). + // We could modify existing code instead of having this function. However, + // the former requires adding a weird variant to `ExprOrSpecial` to + // accommodate a sequence of identifiers as RHS, which greatly complicates + // the conversion from CSTs to `ExprOrSpecial`. Hence, this function is + // added to directly tackle the CST to AST conversion for the has operator, + // This design choice should be noninvasive to existing CST to AST logic, + // despite producing deadcode. + pub(crate) fn to_has_rhs(&self) -> Result>> { + let inner @ cst::Add { initial, extended } = self.try_as_inner()?; + let err = |loc| { + ToASTError::new(ToASTErrorKind::InvalidHasRHS(inner.to_string().into()), loc).into() + }; + let construct_attrs = + |first, rest: &[Node>]| -> Result> { + let mut acc = nonempty![first]; + rest.iter().try_for_each(|ma_node| { + let ma = ma_node.try_as_inner()?; + match ma { + cst::MemAccess::Field(id) => { + acc.push(id.to_unreserved_ident()?); + Ok(()) + } + _ => Err(err(ma_node.loc.clone())), + } + })?; + Ok(acc) + }; + if !extended.is_empty() { + return Err(err(self.loc.clone())); + } + let cst::Mult { initial, extended } = initial.try_as_inner()?; + if !extended.is_empty() { + return Err(err(self.loc.clone())); + } + if let cst::Unary { + op: None, + item: item_node, + } = initial.try_as_inner()? + { + let cst::Member { item, access } = item_node.try_as_inner()?; + // Among successful conversion from `Primary` to `ExprOrSpecial`, + // an `Ident` or `Str` becomes `ExprOrSpecial::StrLit`, + // `ExprOrSpecial::Var`, and `ExprOrSpecial::Name`. Other + // syntactical variants become `ExprOrSpecial::Expr`. + match item.try_as_inner()? { + cst::Primary::EList(_) + | cst::Primary::Expr(_) + | cst::Primary::RInits(_) + | cst::Primary::Ref(_) + | cst::Primary::Slot(_) => Err(err(item.loc.clone())), + cst::Primary::Literal(_) | cst::Primary::Name(_) => { + let item = item.to_expr_or_special()?; + match (item, access.as_slice()) { + (ExprOrSpecial::StrLit { lit, loc }, []) => Ok(Either::Left( + to_unescaped_string(lit).map_err(|escape_errs| { + ParseErrors::new_from_nonempty(escape_errs.map(|e| { + ToASTError::new(ToASTErrorKind::Unescape(e), loc.clone()).into() + })) + })?, + )), + (ExprOrSpecial::Var { var, .. }, rest) => { + // PANIC SAFETY: any variable should be a valid identifier + #[allow(clippy::unwrap_used)] + let first = construct_string_from_var(var).parse().unwrap(); + Ok(Either::Right(construct_attrs(first, rest)?)) + } + (ExprOrSpecial::Name { name, loc }, rest) => { + if name.is_unqualified() { + let first = name.basename(); + + Ok(Either::Right(construct_attrs(first, rest)?)) + } else { + Err(ToASTError::new( + ToASTErrorKind::PathAsAttribute(inner.to_string()), + loc, + ) + .into()) + } + } + // Attempt to return a precise error message for RHS like `true.<...>` + (ExprOrSpecial::Expr { loc, expr }, _) if expr.is_true() => { + Err(ToASTError::new( + ToASTErrorKind::ReservedIdentifier(cst::Ident::True), + loc, + ) + .into()) + } + // Attempt to return a precise error message for RHS like `false.<...>` + (ExprOrSpecial::Expr { loc, expr }, _) if expr.is_false() => { + Err(ToASTError::new( + ToASTErrorKind::ReservedIdentifier(cst::Ident::False), + loc, + ) + .into()) + } + (ExprOrSpecial::Expr { loc, .. }, _) => Err(err(loc)), + _ => Err(err(self.loc.clone())), + } + } + } + } else { + Err(err(self.loc.clone())) + } + } + pub(crate) fn to_expr_or_special(&self) -> Result> { let add = self.try_as_inner()?; @@ -1775,9 +1887,52 @@ fn construct_expr_mul( } expr } -fn construct_expr_has(t: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { + +fn construct_expr_has_attr(t: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { ast::ExprBuilder::new().with_source_loc(loc).has_attr(t, s) } +fn construct_expr_get_attr(t: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).get_attr(t, s) +} +fn construct_exprs_extended_has(t: ast::Expr, attrs: NonEmpty, loc: Loc) -> ast::Expr { + let (first, rest) = attrs.split_first(); + let has_expr = construct_expr_has_attr(t.clone(), first.to_owned(), loc.clone()); + let get_expr = construct_expr_get_attr(t, first.to_owned(), loc.clone()); + // Foldl on the attribute list + // It produces the following for `principal has contactInfo.address.zip` + // Expr.and + // (Expr.and + // (Expr.hasAttr (Expr.var .principal) "contactInfo") + // (Expr.hasAttr + // (Expr.getAttr (Expr.var .principal) "contactInfo") + // "address")) + // (Expr.hasAttr + // (Expr.getAttr + // (Expr.getAttr (Expr.var .principal) "contactInfo") + // "address") + // "zip") + // This is sound. However, the evaluator has to recur multiple times to the + // left-most node to evaluate the existence of the first attribute. The + // desugared expression should be the following to avoid the issue above, + // Expr.and + // Expr.hasAttr (Expr.var .principal) "contactInfo" + // (Expr.and + // (Expr.hasAttr (Expr.getAttr (Expr.var .principal) "contactInfo")"address") + // (Expr.hasAttr ..., "zip")) + rest.iter() + .fold((has_expr, get_expr), |(has_expr, get_expr), attr| { + ( + construct_expr_and( + has_expr, + construct_expr_has_attr(get_expr.clone(), attr.to_owned(), loc.clone()), + std::iter::empty(), + &loc, + ), + construct_expr_get_attr(get_expr, attr.to_owned(), loc.clone()), + ) + }) + .0 +} fn construct_expr_attr(e: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { ast::ExprBuilder::new().with_source_loc(loc).get_attr(e, s) } @@ -2694,8 +2849,8 @@ mod tests { expect_some_error_matches( src, &errs, - &ExpectedErrorMessageBuilder::error("invalid attribute name: 1") - .help("attribute names can either be identifiers or string literals") + &ExpectedErrorMessageBuilder::error("invalid RHS of a `has` operation: 1") + .help("valid RHS of a `has` operation is either a sequence of identifiers separated by `.` or a string literal") .exactly_one_underline("1") .build(), ); @@ -2909,8 +3064,8 @@ mod tests { expect_some_error_matches( src, &errs, - &ExpectedErrorMessageBuilder::error("invalid attribute name: 1") - .help("attribute names can either be identifiers or string literals") + &ExpectedErrorMessageBuilder::error("invalid RHS of a `has` operation: 1") + .help("valid RHS of a `has` operation is either a sequence of identifiers separated by `.` or a string literal") .exactly_one_underline("1") .build(), ); @@ -4720,4 +4875,253 @@ mod tests { ); }); } + + #[test] + fn extended_has() { + assert_matches!( + parse_policy( + None, + r#" + permit( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) when { + principal has contactInfo.address.zip && + principal.contactInfo.address.zip == "90210" +}; + "# + ), + Ok(_) + ); + + assert_matches!(parse_expr(r#"context has a.b"#), Ok(e) => { + assert!(e.eq_shape(&parse_expr(r#"(context has a) && (context.a has b)"#).unwrap())); + }); + + assert_matches!(parse_expr(r#"context has a.b.c"#), Ok(e) => { + assert!(e.eq_shape(&parse_expr(r#"((context has a) && (context.a has b)) && (context.a.b has c)"#).unwrap())); + }); + + let policy = r#"permit(principal, action, resource) when { + principal has a.if + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "this identifier is reserved and cannot be used: if", + ).exactly_one_underline(r#"if"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has if.a + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "this identifier is reserved and cannot be used: if", + ).exactly_one_underline(r#"if"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has true.if + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "this identifier is reserved and cannot be used: true", + ).exactly_one_underline(r#"true"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has a.__cedar + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "The name `__cedar` contains `__cedar`, which is reserved", + ).exactly_one_underline(r#"__cedar"#).build()); + } + ); + + let help_msg = "valid RHS of a `has` operation is either a sequence of identifiers separated by `.` or a string literal"; + + let policy = r#"permit(principal, action, resource) when { + principal has 1 + 1 + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: 1 + 1", + ).help(help_msg). + exactly_one_underline(r#"1 + 1"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has a - 1 + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: a - 1", + ).help(help_msg).exactly_one_underline(r#"a - 1"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has a*3 + 1 + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: a * 3 + 1", + ).help(help_msg).exactly_one_underline(r#"a*3 + 1"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has 3*a + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: 3 * a", + ).help(help_msg).exactly_one_underline(r#"3*a"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has -a.b + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: -a.b", + ).help(help_msg).exactly_one_underline(r#"-a.b"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has !a.b + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: !a.b", + ).help(help_msg).exactly_one_underline(r#"!a.b"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has a::b.c + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "`a::b.c` cannot be used as an attribute as it contains a namespace", + ).exactly_one_underline(r#"a::b"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has A::"" + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: A::\"\"", + ).help(help_msg).exactly_one_underline(r#"A::"""#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has A::"".a + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: A::\"\".a", + ).help(help_msg).exactly_one_underline(r#"A::"""#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has ?principal + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: ?principal", + ).help(help_msg).exactly_one_underline(r#"?principal"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has ?principal.a + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: ?principal.a", + ).help(help_msg).exactly_one_underline(r#"?principal"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has (b).a + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: (b).a", + ).help(help_msg).exactly_one_underline(r#"(b)"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has [b].a + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: [b].a", + ).help(help_msg).exactly_one_underline(r#"[b]"#).build()); + } + ); + let policy = r#"permit(principal, action, resource) when { + principal has {b:1}.a + };"#; + assert_matches!( + parse_policy(None, policy), + Err(e) => { + expect_n_errors(policy, &e, 1); + expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error( + "invalid RHS of a `has` operation: {b: 1}.a", + ).help(help_msg).exactly_one_underline(r#"{b:1}"#).build()); + } + ); + } } diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 188c3f655..8e8254b9a 100644 --- a/cedar-policy-core/src/parser/err.rs +++ b/cedar-policy-core/src/parser/err.rs @@ -237,6 +237,10 @@ pub enum ToASTErrorKind { #[error("invalid attribute name: {0}")] #[diagnostic(help("attribute names can either be identifiers or string literals"))] InvalidAttribute(SmolStr), + /// Returned when the RHS of a `has` operation is invalid + #[error("invalid RHS of a `has` operation: {0}")] + #[diagnostic(help("valid RHS of a `has` operation is either a sequence of identifiers separated by `.` or a string literal"))] + InvalidHasRHS(SmolStr), /// Returned when attempting to use an attribute with a namespace #[error("`{0}` cannot be used as an attribute as it contains a namespace")] PathAsAttribute(String), diff --git a/cedar-policy-core/src/parser/grammar.lalrpop b/cedar-policy-core/src/parser/grammar.lalrpop index 40c69e29c..12a09e018 100644 --- a/cedar-policy-core/src/parser/grammar.lalrpop +++ b/cedar-policy-core/src/parser/grammar.lalrpop @@ -172,6 +172,12 @@ AnyIdent: Node> = { } pub Ident: Node> = AnyIdent; +#[inline] +IfIdent: Node> = { + IF + => Node::with_source_loc(Some(cst::Ident::If), Loc::new(l..r, Arc::clone(src))), +} + // Cond := ('when' | 'unless') '{' Expr '}' Cond: Node> = { "{" "}" @@ -207,12 +213,15 @@ Relation: Node> = { => Node::with_source_loc(Some(cst::Relation::Common{initial: i, extended: e}), Loc::new(l..r, Arc::clone(src))), HAS => Node::with_source_loc(Some(cst::Relation::Has{target: t, field: f}), Loc::new(l..r, Arc::clone(src))), - HAS IF => { + // The following rule exists allegedly for the sake of better error + // reporting. RFC 62 (extended has operator) allows a sequence of + // identifiers separated by . as RHS. Hence, we need to extend this rule to + // `HAS IF { MemAccess }`, as opposed to the original `HAS IF`. + HAS => { // Create an add expression from this identifier - let id0 = Node::with_source_loc(Some(cst::Ident::If), Loc::new(l..r, Arc::clone(src))); - let id1 = Node::with_source_loc(Some(cst::Name{path: vec![], name: id0}), Loc::new(l..r, Arc::clone(src))); + let id1 = Node::with_source_loc(Some(cst::Name{path: vec![], name: ii}), Loc::new(l..r, Arc::clone(src))); let id2 = Node::with_source_loc(Some(cst::Primary::Name(id1)), Loc::new(l..r, Arc::clone(src))); - let id3 = Node::with_source_loc(Some(cst::Member{ item: id2, access: vec![] }), Loc::new(l..r, Arc::clone(src))); + let id3 = Node::with_source_loc(Some(cst::Member{ item: id2, access: a }), Loc::new(l..r, Arc::clone(src))); let id4 = Node::with_source_loc(Some(cst::Unary{op: None, item:id3}), Loc::new(l..r, Arc::clone(src))); let id5 = Node::with_source_loc(Some(cst::Mult{initial: id4, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); let id6 = Node::with_source_loc(Some(cst::Add{initial:id5, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); diff --git a/cedar-policy-core/src/parser/text_to_cst.rs b/cedar-policy-core/src/parser/text_to_cst.rs index cc6ab4e1b..e95964100 100644 --- a/cedar-policy-core/src/parser/text_to_cst.rs +++ b/cedar-policy-core/src/parser/text_to_cst.rs @@ -1204,4 +1204,156 @@ mod tests { .build(), ); } + + #[test] + fn extended_has() { + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has a.b + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has a.if + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has if.a + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has if.if + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has true.if + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has if.true + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has if.then.else.in.like.has.is.__cedar + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has 1+1 + }; + "#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has a - 1 + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has a*3 + 1 + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has 3*a + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has -a.b + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has !a.b + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has a::b.c + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has A::"" + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has A::"".a + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has ?principal + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#"permit(principal, action, resource) when { + principal has ?principal.a + };"#, + ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has (b).a + }; + "#, + ); + assert_parse_fails( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has a.(b) + }; + "#, + ); + assert_parse_fails( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has a.1 + }; + "#, + ); + } } diff --git a/cedar-policy-formatter/tests/extended_has.cedar b/cedar-policy-formatter/tests/extended_has.cedar new file mode 100644 index 000000000..b4da05036 --- /dev/null +++ b/cedar-policy-formatter/tests/extended_has.cedar @@ -0,0 +1,43 @@ +// An example from RFC +permit( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) when { + // extended has + principal has + // contactInfo + contactInfo. + // address + address. + // zip + zip && + // we are safe to access all attributes + principal.contactInfo.address.zip == "90210" +}; + +// Same example without comments +permit( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) when { + principal has + contactInfo. + address. + zip && + principal.contactInfo.address.zip == "90210" +}; + +// Same example with long attributes +permit( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) when { + principal has + contactInfooooooooooooooooooooooooooooooooooooooooooooooo. + addressssssssssssssssssssss. + zipppppppppppp && + principal.contactInfo.address.zip == "90210" +}; \ No newline at end of file diff --git a/cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@extended_has.cedar.snap b/cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@extended_has.cedar.snap new file mode 100644 index 000000000..e6ca386e3 --- /dev/null +++ b/cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@extended_has.cedar.snap @@ -0,0 +1,55 @@ +--- +source: cedar-policy-formatter/src/pprint/fmt.rs +expression: formatted +input_file: cedar-policy-formatter/tests/extended_has.cedar +--- +// An example from RFC +permit ( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) +when +{ + // extended has + principal + has + // contactInfo + contactInfo. + // address + address + . + // zip + zip && + // we are safe to access all attributes + principal.contactInfo + .address + .zip == "90210" +}; + +// Same example without comments +permit ( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) +when +{ + principal has contactInfo.address.zip && + principal.contactInfo.address.zip == "90210" +}; + +// Same example with long attributes +permit ( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) +when +{ + principal + has + contactInfooooooooooooooooooooooooooooooooooooooooooooooo.addressssssssssssssssssssss + .zipppppppppppp && + principal.contactInfo.address.zip == "90210" +}; diff --git a/cedar-policy-validator/src/typecheck/test/policy.rs b/cedar-policy-validator/src/typecheck/test/policy.rs index 9ee214167..57693bee9 100644 --- a/cedar-policy-validator/src/typecheck/test/policy.rs +++ b/cedar-policy-validator/src/typecheck/test/policy.rs @@ -22,6 +22,7 @@ use std::sync::Arc; use cedar_policy_core::{ ast::{EntityUID, Expr, PolicyID, Template}, + extensions::Extensions, parser::{parse_policy, parse_policy_or_template}, }; @@ -37,7 +38,7 @@ use crate::{ typecheck::{PolicyCheck, Typechecker}, types::{EntityLUB, Type}, validation_errors::{AttributeAccess, LubContext, LubHelp}, - RawName, ValidationMode, ValidationWarning, + RawName, ValidationMode, ValidationWarning, ValidatorSchema, }; fn simple_schema_file() -> json_schema::NamespaceDefinition { @@ -1041,6 +1042,119 @@ fn validate_policy_with_common_type_schema() { ); } +#[test] +fn extended_has() { + let schema_src = r#" + entity A { + x?: { + y?: { + z?: Long, + } + } + }; + + action "action" appliesTo { + principal: A, + resource: A, + }; + "#; + let (schema, _) = + ValidatorSchema::from_cedarschema_str(schema_src, Extensions::none()).unwrap(); + + let policy = parse_policy( + None, + r#" + permit(principal, action == Action::"action", resource) when { + principal has x.y.z && principal.x.y.z > 1 + }; + "#, + ) + .unwrap(); + assert_policy_typechecks(schema.clone(), policy); + let policy = parse_policy( + None, + r#" + permit(principal, action == Action::"action", resource) when { + principal has x.y && principal.x.y has z + }; + "#, + ) + .unwrap(); + assert_policy_typechecks(schema.clone(), policy); + let policy = parse_policy( + None, + r#" + permit(principal, action == Action::"action", resource) when { + principal has x && principal.x has y.z + }; + "#, + ) + .unwrap(); + assert_policy_typechecks(schema.clone(), policy); + + let src = r#" + permit(principal, action == Action::"action", resource) when { + principal has x.y && principal.x.y.z > 1 + }; + "#; + let policy = parse_policy(None, src).unwrap(); + let errors = assert_policy_typecheck_fails(schema.clone(), policy); + let error = assert_exactly_one_diagnostic(errors); + assert_eq!( + error, + ValidationError::unsafe_optional_attribute_access( + get_loc(src, "principal.x.y.z"), + PolicyID::from_string("policy0"), + AttributeAccess::EntityLUB( + EntityLUB::single_entity("A".parse().unwrap()), + vec!["z".into(), "y".into(), "x".into()], + ) + ) + ); + + let src = r#" + permit(principal, action == Action::"action", resource) when { + principal has x && principal.x.y has z + }; + "#; + let policy = parse_policy(None, src).unwrap(); + let errors = assert_policy_typecheck_fails(schema.clone(), policy); + let error = assert_exactly_one_diagnostic(errors); + assert_eq!( + error, + ValidationError::unsafe_optional_attribute_access( + get_loc(src, "principal.x.y"), + PolicyID::from_string("policy0"), + AttributeAccess::EntityLUB( + EntityLUB::single_entity("A".parse().unwrap()), + vec!["y".into(), "x".into()], + ) + ) + ); + + let src = r#" + permit(principal, action == Action::"action", resource) when { + principal has x.y.z && principal.x.y.a > 1 + }; + "#; + let policy = parse_policy(None, src).unwrap(); + let errors = assert_policy_typecheck_fails(schema.clone(), policy); + let error = assert_exactly_one_diagnostic(errors); + assert_eq!( + error, + ValidationError::unsafe_attribute_access( + get_loc(src, "principal.x.y.a"), + PolicyID::from_string("policy0"), + AttributeAccess::EntityLUB( + EntityLUB::single_entity("A".parse().unwrap()), + vec!["a".into(), "y".into(), "x".into(),], + ), + Some("z".into()), + false + ) + ); +} + mod templates { use super::*; diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 018c6cba6..6faee2ab9 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -18,6 +18,7 @@ Cedar Language Version: TBD - Added protobuf schemas and (de)serialization code using on `prost` crate behind the experimental `protobufs` flag. - Added protobuf and JSON generation code to `cedar-policy-cli`. - Added a new get helper method to Context that allows easy extraction of generic values from the context by key. This method simplifies the common use case of retrieving values from Context objects. +- Implemented [RFC 62 (extended `has` operator)](https://github.com/cedar-policy/rfcs/blob/main/text/0062-extended-has.md) (#1327, resolving #1329) ### Changed diff --git a/cedar-policy/src/tests.rs b/cedar-policy/src/tests.rs index 893bf5d1a..17dc0ef47 100644 --- a/cedar-policy/src/tests.rs +++ b/cedar-policy/src/tests.rs @@ -6314,11 +6314,10 @@ mod reserved_keywords_in_policies { id.into(), "attribute names can either be identifiers or string literals".into(), ); - assert_invalid_expression_with_help( + assert_invalid_expression( format!("principal has {id}"), - format!("invalid attribute name: {id}"), - id.into(), - "attribute names can either be identifiers or string literals".into(), + RESERVED_IDENT_MSG(id), + format!("{id}"), ); } "if" => { @@ -6330,7 +6329,7 @@ mod reserved_keywords_in_policies { assert_invalid_expression( format!("principal has {id}"), RESERVED_IDENT_MSG(id), - format!("principal has {id}"), + format!("{id}"), ); } _ => {