Skip to content

Improve parse errors around invalid is expressions #491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 12, 2023
Merged

Improve parse errors around invalid is expressions #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cab8207
Improve parse error
john-h-kastner-aws Dec 11, 2023
7456f73
tests
john-h-kastner-aws Dec 12, 2023
ed5912e
helps
john-h-kastner-aws Dec 12, 2023
5b101a8
comment
john-h-kastner-aws Dec 12, 2023
9c5e94b
changelog
john-h-kastner-aws Dec 12, 2023
b37534e
testcase
john-h-kastner-aws Dec 12, 2023
b082e95
comment
john-h-kastner-aws Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cedar-policy-core/src/parser/cst.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub struct VariableDef {
/// not used for anything other than error reporting.
pub unused_type_name: Option<Node<Name>>,
/// type of entity using current `var is type` syntax
pub entity_type: Option<Node<Name>>,
pub entity_type: Option<Node<Add>>,
/// hierarchy of entity
pub ineq: Option<(RelOp, Node<Expr>)>,
}
Expand Down Expand Up @@ -197,7 +197,7 @@ pub enum Relation {
/// element that may be an entity type and `in` an entity
target: Node<Add>,
/// entity type to check for
entity_type: Node<Name>,
entity_type: Node<Add>,
/// entity that the target may be `in`
in_entity: Option<Node<Add>>,
},
Expand Down
225 changes: 205 additions & 20 deletions cedar-policy-core/src/parser/cst_to_ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ impl ASTNode<Option<cst::VariableDef>> {
}
(cst::RelOp::In, None) => Some(PrincipalOrResourceConstraint::In(eref)),
(cst::RelOp::In, Some(entity_type)) => Some(PrincipalOrResourceConstraint::IsIn(
entity_type.to_name(errs)?,
entity_type.to_expr_or_special(errs)?.into_name(errs)?,
eref,
)),
(op, _) => {
Expand All @@ -571,7 +571,7 @@ impl ASTNode<Option<cst::VariableDef>> {
}
} else if let Some(entity_type) = &vardef.entity_type {
Some(PrincipalOrResourceConstraint::Is(
entity_type.to_name(errs)?,
entity_type.to_expr_or_special(errs)?.into_name(errs)?,
))
} else {
Some(PrincipalOrResourceConstraint::Any)
Expand Down Expand Up @@ -891,6 +891,24 @@ impl ExprOrSpecial<'_> {
}
}
}

fn into_name(self, errs: &mut ParseErrors) -> Option<ast::Name> {
match self {
Self::StrLit(s, _) => {
errs.push(self.to_ast_err(ToASTErrorKind::IsInvalidName(s.to_string())));
None
}
Self::Var(var, _) => {
errs.push(self.to_ast_err(ToASTErrorKind::IsInvalidName(var.to_string())));
None
}
Self::Name(name, _) => Some(name),
Self::Expr(ref e, _) => {
errs.push(self.to_ast_err(ToASTErrorKind::IsInvalidName(e.to_string())));
None
}
}
}
}

impl ASTNode<Option<cst::Expr>> {
Expand Down Expand Up @@ -1253,7 +1271,10 @@ impl ASTNode<Option<cst::Relation>> {
target,
entity_type,
in_entity,
} => match (target.to_expr(errs), entity_type.to_name(errs)) {
} => match (
target.to_expr(errs),
entity_type.to_expr_or_special(errs)?.into_name(errs),
) {
(Some(t), Some(n)) => match in_entity {
Some(in_entity) => in_entity.to_expr(errs).map(|in_entity| {
ExprOrSpecial::Expr(
Expand Down Expand Up @@ -3891,6 +3912,26 @@ mod tests {
),
),
),
(
r#"principal is User && principal in Group::"friends""#,
Expr::and(
Expr::is_entity_type(Expr::var(ast::Var::Principal), "User".parse().unwrap()),
Expr::is_in(
Expr::var(ast::Var::Principal),
Expr::val(r#"Group::"friends""#.parse::<EntityUID>().unwrap()),
),
),
),
(
r#"principal is User || principal in Group::"friends""#,
Expr::or(
Expr::is_entity_type(Expr::var(ast::Var::Principal), "User".parse().unwrap()),
Expr::is_in(
Expr::var(ast::Var::Principal),
Expr::val(r#"Group::"friends""#.parse::<EntityUID>().unwrap()),
),
),
),
(
r#"true && principal is User in principal"#,
Expr::and(
Expand Down Expand Up @@ -3943,6 +3984,31 @@ mod tests {
),
),
),
(
r#"if principal is User then 1 else 2"#,
Expr::ite(
Expr::is_entity_type(Expr::var(ast::Var::Principal), "User".parse().unwrap()),
Expr::val(1),
Expr::val(2),
),
),
(
r#"if principal is User in Group::"friends" then 1 else 2"#,
Expr::ite(
Expr::and(
Expr::is_entity_type(
Expr::var(ast::Var::Principal),
"User".parse().unwrap(),
),
Expr::is_in(
Expr::var(ast::Var::Principal),
Expr::val(r#"Group::"friends""#.parse::<EntityUID>().unwrap()),
),
),
Expr::val(1),
Expr::val(2),
),
),
] {
let e = parse_expr(es).unwrap();
assert!(
Expand Down Expand Up @@ -4025,14 +4091,6 @@ mod tests {
#[test]
fn is_err() {
let invalid_is_policies = [
(
r#"permit(principal == ?resource, action, resource);"#,
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?resource instead of ?principal"),
),
(
r#"permit(principal, action, resource == ?principal);"#,
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?principal instead of ?resource"),
),
(
r#"permit(principal in Group::"friends" is User, action, resource);"#,
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found an `is` expression"),
Expand All @@ -4043,23 +4101,31 @@ mod tests {
),
(
r#"permit(principal is User == User::"Alice", action, resource);"#,
ExpectedErrorMessage::error(
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the scope at the same time as `==`",
"try moving `is` into a `when` condition"
),
),
(
r#"permit(principal, action, resource is Doc == Doc::"a");"#,
ExpectedErrorMessage::error(
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the scope at the same time as `==`",
"try moving `is` into a `when` condition"
),
),
(
r#"permit(principal is User::"alice", action, resource);"#,
ExpectedErrorMessage::error(r#"unexpected token `"alice"`"#),
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `User::"alice"`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource is File::"f");"#,
ExpectedErrorMessage::error(r#"unexpected token `"f"`"#),
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `File::"f"`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal is User in 1, action, resource);"#,
Expand All @@ -4079,27 +4145,88 @@ mod tests {
"expected an entity uid or matching template slot, found name `User`",
),
),
(
r#"permit(principal is User::"Alice" in Group::"f", action, resource);"#,
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `User::"Alice"`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource is File in File);"#,
ExpectedErrorMessage::error(
"expected an entity uid or matching template slot, found name `File`",
),
),
(
r#"permit(principal, action, resource is File::"file" in Folder::"folder");"#,
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `File::"file"`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal is 1, action, resource);"#,
ExpectedErrorMessage::error("unexpected token `1`"),
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `1`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource is 1);"#,
ExpectedErrorMessage::error("unexpected token `1`"),
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `1`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action is Action, resource);"#,
ExpectedErrorMessage::error("`is` cannot appear in the action scope"),
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the action scope",
"try moving `action is ..` into a `when` condition"
),
),
(
r#"permit(principal, action is Action::"a", resource);"#,
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the action scope",
"try moving `action is ..` into a `when` condition"
),
),
(
r#"permit(principal, action is Action in Action::"A", resource);"#,
ExpectedErrorMessage::error("`is` cannot appear in the action scope"),
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the action scope",
"try moving `action is ..` into a `when` condition"
),
),
(
r#"permit(principal, action is Action in Action, resource);"#,
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the action scope",
"try moving `action is ..` into a `when` condition"
),
),
(
r#"permit(principal, action is Action::"a" in Action::"b", resource);"#,
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the action scope",
"try moving `action is ..` into a `when` condition"
),
),
(
r#"permit(principal, action is Action in ?action, resource);"#,
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the action scope",
"try moving `action is ..` into a `when` condition"
),
),
(
r#"permit(principal, action is ?action, resource);"#,
ExpectedErrorMessage::error_and_help(
"`is` cannot appear in the action scope",
"try moving `action is ..` into a `when` condition"
),
),
(
r#"permit(principal is User in ?resource, action, resource);"#,
Expand All @@ -4109,9 +4236,59 @@ mod tests {
r#"permit(principal, action, resource is Folder in ?principal);"#,
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?principal instead of ?resource"),
),
(
r#"permit(principal is ?principal, action, resource);"#,
ExpectedErrorMessage::error_and_help(
"right hand side of an `is` expression must be an entity type name, but got `?principal`",
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource is ?resource);"#,
ExpectedErrorMessage::error_and_help(
"right hand side of an `is` expression must be an entity type name, but got `?resource`",
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource) when { principal is 1 };"#,
ExpectedErrorMessage::error("unexpected token `1`"),
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `1`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource) when { principal is User::"alice" in Group::"friends" };"#,
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `User::"alice"`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource) when { principal is ! User::"alice" in Group::"friends" };"#,
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `!User::"alice"`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource) when { principal is User::"alice" + User::"alice" in Group::"friends" };"#,
ExpectedErrorMessage::error_and_help(
r#"right hand side of an `is` expression must be an entity type name, but got `User::"alice" + User::"alice"`"#,
"try using `==` to test for equality"
),
),
(
r#"permit(principal, action, resource) when { principal is User in User::"alice" in Group::"friends" };"#,
ExpectedErrorMessage::error(
"unexpected token `in`"
),
),
(
r#"permit(principal, action, resource) when { principal is User == User::"alice" in Group::"friends" };"#,
ExpectedErrorMessage::error(
"unexpected token `==`"
),
),
];
for (p_src, expected) in invalid_is_policies {
Expand Down Expand Up @@ -4212,6 +4389,14 @@ mod tests {
#[test]
fn invalid_slot() {
let invalid_policies = [
(
r#"permit(principal == ?resource, action, resource);"#,
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?resource instead of ?principal"),
),
(
r#"permit(principal, action, resource == ?principal);"#,
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?principal instead of ?resource"),
),
(
r#"permit(principal, action, resource) when { principal == ?foo};"#,
ExpectedErrorMessage::error_and_help(
Expand Down
6 changes: 6 additions & 0 deletions cedar-policy-core/src/parser/err.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,10 @@ pub enum ToASTErrorKind {
/// Returned when the right hand side of a `like` expression is not a constant pattern literal
#[error("right hand side of a `like` expression must be a pattern literal, but got `{0}`")]
InvalidPattern(String),
/// Returned when the right hand side of a `is` expression is not an entity type name
#[error("right hand side of an `is` expression must be an entity type name, but got `{0}`")]
#[diagnostic(help("try using `==` to test for equality"))]
IsInvalidName(String),
/// Returned when an unexpected node is in the policy scope clause
#[error("expected {expected}, found {got}")]
WrongNode {
Expand Down Expand Up @@ -473,9 +477,11 @@ impl std::fmt::Display for Ref {
pub enum InvalidIsError {
/// The action scope may not contain an `is`
#[error("`is` cannot appear in the action scope")]
#[diagnostic(help("try moving `action is ..` into a `when` condition"))]
ActionScope,
/// An `is` cannot appear with this operator in the policy scope
#[error("`is` cannot appear in the scope at the same time as `{0}`")]
#[diagnostic(help("try moving `is` into a `when` condition"))]
WrongOp(cst::RelOp),
}

Expand Down
Loading