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}"), ); } _ => {