From 19b729317a795f2a32ead56269e4c52c0436cd88 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 19 Nov 2024 15:57:28 -0800 Subject: [PATCH 01/15] prepare for the extension Signed-off-by: Shaobo He --- cedar-policy-core/src/ast/name.rs | 2 +- cedar-policy-core/src/parser/cst_to_ast.rs | 74 +++++++++++++++++++++- 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/cedar-policy-core/src/ast/name.rs b/cedar-policy-core/src/ast/name.rs index 5767bafa6..8576a1962 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/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index c59745f46..4827e994c 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; @@ -947,7 +948,10 @@ 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) => s, + Either::Right(ids) => ids.first().to_smolstr(), + }); let (target, field) = flatten_tuple_2(maybe_target, maybe_field)?; Ok(ExprOrSpecial::Expr { expr: construct_expr_has(target, field, self.loc.clone()), @@ -998,6 +1002,74 @@ impl Node> { fn to_expr(&self) -> Result { self.to_expr_or_special()?.into_expr() } + + // Peel the grammar onion until we see valid RHS + pub(crate) fn to_has_rhs(&self) -> Result>> { + let inner @ cst::Add { initial, extended } = self.try_as_inner()?; + let err = || { + ToASTError::new( + ToASTErrorKind::InvalidAttribute(inner.to_string().into()), + self.loc.clone(), + ) + .into() + }; + let construct_attrs = |first: UnreservedId, rest: &[Node>]| { + let mut acc = nonempty![first]; + rest.iter().try_for_each(|ma| { + let ma = ma.try_as_inner()?; + match ma { + cst::MemAccess::Field(id) => { + acc.push(id.to_unreserved_ident()?); + Ok(()) + } + _ => Err(err()), + } + })?; + Ok::, ParseErrors>(acc) + }; + if !extended.is_empty() { + return Err(err()); + } + let cst::Mult { initial, extended } = initial.try_as_inner()?; + if !extended.is_empty() { + return Err(err()); + } + if let cst::Unary { op: None, item } = initial.try_as_inner()? { + let cst::Member { item, access } = item.try_as_inner()?; + 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()) + } + } + _ => Err(err()), + } + } else { + Err(err()) + } + } pub(crate) fn to_expr_or_special(&self) -> Result> { let add = self.try_as_inner()?; From f3091d24010c8db7a7aebcd31b3f18e82c885f67 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 19 Nov 2024 17:29:36 -0800 Subject: [PATCH 02/15] prototype Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/cst_to_ast.rs | 46 ++++++++++++++++++++-- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 4827e994c..d0d830767 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -949,12 +949,12 @@ impl Node> { cst::Relation::Has { target, field } => { let maybe_target = target.to_expr(); let maybe_field = Ok(match field.to_has_rhs()? { - Either::Left(s) => s, - Either::Right(ids) => ids.first().to_smolstr(), + 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(), }) } @@ -1848,9 +1848,31 @@ 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()); + rest.into_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) } @@ -4791,4 +4813,20 @@ 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(p) => { + println!("{p}"); + }); + } } From 1cfb5d68f14aaa0155800378a2adfdacf7fa36c5 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 20 Nov 2024 10:35:02 -0800 Subject: [PATCH 03/15] updates Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/cst_to_ast.rs | 2 +- cedar-policy-core/src/parser/grammar.lalrpop | 8 ++- cedar-policy-core/src/parser/text_to_cst.rs | 52 ++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 23daa0296..2d4557d32 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -1859,7 +1859,7 @@ fn construct_exprs_extended_has(t: ast::Expr, attrs: NonEmpty, loc: Loc 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()); - rest.into_iter() + rest.iter() .fold((has_expr, get_expr), |(has_expr, get_expr), attr| { ( construct_expr_and( diff --git a/cedar-policy-core/src/parser/grammar.lalrpop b/cedar-policy-core/src/parser/grammar.lalrpop index 40c69e29c..d069842e8 100644 --- a/cedar-policy-core/src/parser/grammar.lalrpop +++ b/cedar-policy-core/src/parser/grammar.lalrpop @@ -207,12 +207,16 @@ 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 IF => { // 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 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 1db5fd6fe..fbd0c887e 100644 --- a/cedar-policy-core/src/parser/text_to_cst.rs +++ b/cedar-policy-core/src/parser/text_to_cst.rs @@ -1212,4 +1212,56 @@ 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 + }; + "#, + ); + } } From 654e41202a58df8d55cd2a50d515e6ef72748cfc Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 20 Nov 2024 10:42:10 -0800 Subject: [PATCH 04/15] updates Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/text_to_cst.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cedar-policy-core/src/parser/text_to_cst.rs b/cedar-policy-core/src/parser/text_to_cst.rs index fbd0c887e..684d97484 100644 --- a/cedar-policy-core/src/parser/text_to_cst.rs +++ b/cedar-policy-core/src/parser/text_to_cst.rs @@ -1263,5 +1263,13 @@ mod tests { }; "#, ); + assert_parse_succeeds( + parse_policy, + r#" + permit(principal, action, resource) when { + principal has if.then.else.in.like.has.is.__cedar + }; + "#, + ); } } From 98b55fb9fa870a8e27842d8aa33416f710616e17 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 20 Nov 2024 14:28:51 -0800 Subject: [PATCH 05/15] better error messages Signed-off-by: Shaobo He --- cedar-policy-core/src/ast/expr.rs | 8 ++ cedar-policy-core/src/parser/cst_to_ast.rs | 89 +++++++++++++++----- cedar-policy-core/src/parser/err.rs | 4 + cedar-policy-core/src/parser/grammar.lalrpop | 11 ++- cedar-policy/src/tests.rs | 9 +- 5 files changed, 94 insertions(+), 27 deletions(-) diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs index c50ae0ec4..f417f06db 100644 --- a/cedar-policy-core/src/ast/expr.rs +++ b/cedar-policy-core/src/ast/expr.rs @@ -1858,6 +1858,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/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 2d4557d32..32634a4a0 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -1006,36 +1006,36 @@ impl Node> { // Peel the grammar onion until we see valid RHS pub(crate) fn to_has_rhs(&self) -> Result>> { let inner @ cst::Add { initial, extended } = self.try_as_inner()?; - let err = || { - ToASTError::new( - ToASTErrorKind::InvalidAttribute(inner.to_string().into()), - self.loc.clone(), - ) - .into() + let err = |loc| { + ToASTError::new(ToASTErrorKind::InvalidHasRHS(inner.to_string().into()), loc).into() }; let construct_attrs = |first: UnreservedId, rest: &[Node>]| { let mut acc = nonempty![first]; - rest.iter().try_for_each(|ma| { - let ma = ma.try_as_inner()?; + 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()), + _ => Err(err(ma_node.loc.clone())), } })?; Ok::, ParseErrors>(acc) }; if !extended.is_empty() { - return Err(err()); + return Err(err(self.loc.clone())); } let cst::Mult { initial, extended } = initial.try_as_inner()?; if !extended.is_empty() { - return Err(err()); + return Err(err(self.loc.clone())); } - if let cst::Unary { op: None, item } = initial.try_as_inner()? { - let cst::Member { item, access } = item.try_as_inner()?; + if let cst::Unary { + op: None, + item: item_node, + } = initial.try_as_inner()? + { + let cst::Member { item, access } = item_node.try_as_inner()?; let item = item.to_expr_or_special()?; match (item, access.as_slice()) { (ExprOrSpecial::StrLit { lit, loc }, []) => Ok(Either::Left( @@ -1064,12 +1064,26 @@ impl Node> { .into()) } } - _ => Err(err()), + // 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()) + Err(err(self.loc.clone())) } } + pub(crate) fn to_expr_or_special(&self) -> Result> { let add = self.try_as_inner()?; @@ -2787,8 +2801,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(), ); @@ -3002,8 +3016,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(), ); @@ -4828,5 +4842,42 @@ mod tests { "#), Ok(p) => { println!("{p}"); }); + + 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()); + } + ); } } diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 0a017a012..ac52ea861 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 d069842e8..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> = { "{" "}" @@ -211,10 +217,9 @@ Relation: Node> = { // 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 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: 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))); diff --git a/cedar-policy/src/tests.rs b/cedar-policy/src/tests.rs index c1e0c56aa..7e9e33779 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}"), ); } _ => { From 777d074ac1dbf4c4aacc6978f41d3abfd77e8681 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 20 Nov 2024 16:15:19 -0800 Subject: [PATCH 06/15] added evaluator tests Signed-off-by: Shaobo He --- cedar-policy-core/src/evaluator.rs | 70 +++++++++++++++++++++- cedar-policy-core/src/parser/cst_to_ast.rs | 24 ++++++-- 2 files changed, 89 insertions(+), 5 deletions(-) diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index c521c8bfa..1be5ff0fc 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -5524,7 +5524,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); @@ -6140,4 +6140,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 32634a4a0..cf3534b6e 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -4830,7 +4830,10 @@ mod tests { #[test] fn extended_has() { - assert_matches!(parse_policy(None, r#" + assert_matches!( + parse_policy( + None, + r#" permit( principal is User, action == Action::"preview", @@ -4839,9 +4842,10 @@ mod tests { principal has contactInfo.address.zip && principal.contactInfo.address.zip == "90210" }; - "#), Ok(p) => { - println!("{p}"); - }); + "# + ), + Ok(_) + ); let policy = r#"permit(principal, action, resource) when { principal has a.if @@ -4879,5 +4883,17 @@ mod tests { ).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()); + } + ); } } From 91b21a257eee63c54c94509e7cdb4565e1354537 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 20 Nov 2024 16:29:17 -0800 Subject: [PATCH 07/15] add more tests Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/cst_to_ast.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index cf3534b6e..d5a25984f 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -4847,6 +4847,14 @@ mod tests { 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 };"#; From 9694d2ec4015154bda2e3e6ef9457435e443b618 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Thu, 21 Nov 2024 10:43:30 -0800 Subject: [PATCH 08/15] updates Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/cst_to_ast.rs | 27 +++++++++++----------- cedar-policy/CHANGELOG.md | 1 + 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index d5a25984f..1572ef619 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -1009,20 +1009,21 @@ impl Node> { let err = |loc| { ToASTError::new(ToASTErrorKind::InvalidHasRHS(inner.to_string().into()), loc).into() }; - let construct_attrs = |first: UnreservedId, rest: &[Node>]| { - 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(()) + 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())), } - _ => Err(err(ma_node.loc.clone())), - } - })?; - Ok::, ParseErrors>(acc) - }; + })?; + Ok(acc) + }; if !extended.is_empty() { return Err(err(self.loc.clone())); } diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 7637dc0c2..dc254d0a4 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -17,6 +17,7 @@ Cedar Language Version: TBD - Added protobuf schemas and (de)serialization code using on `prost` crate behind the experimental `protobufs` flag. - 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) ## [4.2.2] - 2024-11-11 Cedar Language version: 4.1 From 01ecc7e955a2836af0b8fdf4027a59018b0f0b42 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Thu, 21 Nov 2024 11:13:44 -0800 Subject: [PATCH 09/15] add comments Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/cst_to_ast.rs | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 1572ef619..fc8e3f0f8 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -1004,6 +1004,14 @@ impl Node> { } // 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| { @@ -1874,6 +1882,27 @@ fn construct_exprs_extended_has(t: ast::Expr, attrs: NonEmpty, loc: Loc 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| { ( From 16bed5eb2c3bcdd2668a744699005e547cffdd6c Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 25 Nov 2024 15:14:29 -0800 Subject: [PATCH 10/15] more tests Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/cst_to_ast.rs | 270 +++++++++++++++++--- cedar-policy-core/src/parser/text_to_cst.rs | 92 +++++++ 2 files changed, 322 insertions(+), 40 deletions(-) diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 977650c30..c46802d91 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -1045,48 +1045,66 @@ impl Node> { } = initial.try_as_inner()? { let cst::Member { item, access } = item_node.try_as_inner()?; - 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()) + //let item = item.to_expr_or_special()?; + // 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())), } } - // 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())) @@ -4931,5 +4949,177 @@ mod tests { ).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/text_to_cst.rs b/cedar-policy-core/src/parser/text_to_cst.rs index 3c3eed99f..e95964100 100644 --- a/cedar-policy-core/src/parser/text_to_cst.rs +++ b/cedar-policy-core/src/parser/text_to_cst.rs @@ -1263,5 +1263,97 @@ mod tests { }; "#, ); + 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 + }; + "#, + ); } } From ea69d811e69b0aba2cb0ba29b8847d362cd74e6e Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 25 Nov 2024 16:04:32 -0800 Subject: [PATCH 11/15] added a validator test Signed-off-by: Shaobo He --- .../src/typecheck/test/policy.rs | 116 +++++++++++++++++- 1 file changed, 115 insertions(+), 1 deletion(-) 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::*; From cd089f3dc73112f1241655efb85e91c0a0bd4502 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 25 Nov 2024 16:10:16 -0800 Subject: [PATCH 12/15] added a formatter test case Signed-off-by: Shaobo He --- .../tests/extended_has.cedar | 17 +++++++++++ ...ests__format_files@extended_has.cedar.snap | 28 +++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 cedar-policy-formatter/tests/extended_has.cedar create mode 100644 cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@extended_has.cedar.snap diff --git a/cedar-policy-formatter/tests/extended_has.cedar b/cedar-policy-formatter/tests/extended_has.cedar new file mode 100644 index 000000000..1d3d1e8f9 --- /dev/null +++ b/cedar-policy-formatter/tests/extended_has.cedar @@ -0,0 +1,17 @@ +// 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" +}; \ 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..983ac0fc6 --- /dev/null +++ b/cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@extended_has.cedar.snap @@ -0,0 +1,28 @@ +--- +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" +}; From 07ed080070a561056ab750119877a1e14881f92a Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 26 Nov 2024 16:21:02 -0800 Subject: [PATCH 13/15] added cst to est conversion Signed-off-by: Shaobo He --- cedar-policy-core/src/est.rs | 71 +++++++++++++++++++++++++++++++ cedar-policy-core/src/est/expr.rs | 23 +++++++--- 2 files changed, 89 insertions(+), 5 deletions(-) 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()?; From 323feee0c04751424e8dd1d5c7d6b7b59b0becb8 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 27 Nov 2024 09:31:04 -0800 Subject: [PATCH 14/15] review feedback Signed-off-by: Shaobo He --- cedar-policy-core/src/parser/cst_to_ast.rs | 1 - cedar-policy-formatter/tests/extended_has.cedar | 13 +++++++++++++ ...t__tests__format_files@extended_has.cedar.snap | 15 +++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 839463dcf..cebc8e024 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -1048,7 +1048,6 @@ impl Node> { } = initial.try_as_inner()? { let cst::Member { item, access } = item_node.try_as_inner()?; - //let item = item.to_expr_or_special()?; // Among successful conversion from `Primary` to `ExprOrSpecial`, // an `Ident` or `Str` becomes `ExprOrSpecial::StrLit`, // `ExprOrSpecial::Var`, and `ExprOrSpecial::Name`. Other diff --git a/cedar-policy-formatter/tests/extended_has.cedar b/cedar-policy-formatter/tests/extended_has.cedar index 1d3d1e8f9..1e332edbe 100644 --- a/cedar-policy-formatter/tests/extended_has.cedar +++ b/cedar-policy-formatter/tests/extended_has.cedar @@ -14,4 +14,17 @@ permit( 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 + 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 index 983ac0fc6..d5851f76b 100644 --- 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 @@ -26,3 +26,18 @@ when .address .zip == "90210" }; + +// Same example without comments +permit ( + principal is User, + action == Action::"preview", + resource == Movie::"Blockbuster" +) +when +{ + principal + has + contactInfooooooooooooooooooooooooooooooooooooooooooooooo.addressssssssssssssssssssss + .zipppppppppppp && + principal.contactInfo.address.zip == "90210" +}; From 1f2d7568ee39e0e835383a7b4989f036856d8be8 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 27 Nov 2024 09:34:47 -0800 Subject: [PATCH 15/15] more tests Signed-off-by: Shaobo He --- cedar-policy-formatter/tests/extended_has.cedar | 13 +++++++++++++ ...fmt__tests__format_files@extended_has.cedar.snap | 12 ++++++++++++ 2 files changed, 25 insertions(+) diff --git a/cedar-policy-formatter/tests/extended_has.cedar b/cedar-policy-formatter/tests/extended_has.cedar index 1e332edbe..b4da05036 100644 --- a/cedar-policy-formatter/tests/extended_has.cedar +++ b/cedar-policy-formatter/tests/extended_has.cedar @@ -17,6 +17,19 @@ permit( }; // 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", 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 index d5851f76b..e6ca386e3 100644 --- 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 @@ -34,6 +34,18 @@ permit ( 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