Skip to content

Commit 666c7a4

Browse files
john-h-kastner-awsjohn-h-kastner-aws
authored
Improve parse errors around invalid is expressions (#491)
1 parent bc0d5b6 commit 666c7a4

File tree

5 files changed

+221
-26
lines changed

5 files changed

+221
-26
lines changed

‎cedar-policy-core/src/parser/cst.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ pub struct VariableDef {
7070
/// not used for anything other than error reporting.
7171
pub unused_type_name: Option<Node<Name>>,
7272
/// type of entity using current `var is type` syntax
73-
pub entity_type: Option<Node<Name>>,
73+
pub entity_type: Option<Node<Add>>,
7474
/// hierarchy of entity
7575
pub ineq: Option<(RelOp, Node<Expr>)>,
7676
}
@@ -197,7 +197,7 @@ pub enum Relation {
197197
/// element that may be an entity type and `in` an entity
198198
target: Node<Add>,
199199
/// entity type to check for
200-
entity_type: Node<Name>,
200+
entity_type: Node<Add>,
201201
/// entity that the target may be `in`
202202
in_entity: Option<Node<Add>>,
203203
},

‎cedar-policy-core/src/parser/cst_to_ast.rs‎

Lines changed: 205 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,7 @@ impl ASTNode<Option<cst::VariableDef>> {
561561
}
562562
(cst::RelOp::In, None) => Some(PrincipalOrResourceConstraint::In(eref)),
563563
(cst::RelOp::In, Some(entity_type)) => Some(PrincipalOrResourceConstraint::IsIn(
564-
entity_type.to_name(errs)?,
564+
entity_type.to_expr_or_special(errs)?.into_name(errs)?,
565565
eref,
566566
)),
567567
(op, _) => {
@@ -571,7 +571,7 @@ impl ASTNode<Option<cst::VariableDef>> {
571571
}
572572
} else if let Some(entity_type) = &vardef.entity_type {
573573
Some(PrincipalOrResourceConstraint::Is(
574-
entity_type.to_name(errs)?,
574+
entity_type.to_expr_or_special(errs)?.into_name(errs)?,
575575
))
576576
} else {
577577
Some(PrincipalOrResourceConstraint::Any)
@@ -890,6 +890,24 @@ impl ExprOrSpecial<'_> {
890890
}
891891
}
892892
}
893+
894+
fn into_name(self, errs: &mut ParseErrors) -> Option<ast::Name> {
895+
match self {
896+
Self::StrLit(s, _) => {
897+
errs.push(self.to_ast_err(ToASTErrorKind::IsInvalidName(s.to_string())));
898+
None
899+
}
900+
Self::Var(var, _) => {
901+
errs.push(self.to_ast_err(ToASTErrorKind::IsInvalidName(var.to_string())));
902+
None
903+
}
904+
Self::Name(name, _) => Some(name),
905+
Self::Expr(ref e, _) => {
906+
errs.push(self.to_ast_err(ToASTErrorKind::IsInvalidName(e.to_string())));
907+
None
908+
}
909+
}
910+
}
893911
}
894912

895913
impl ASTNode<Option<cst::Expr>> {
@@ -1252,7 +1270,10 @@ impl ASTNode<Option<cst::Relation>> {
12521270
target,
12531271
entity_type,
12541272
in_entity,
1255-
} => match (target.to_expr(errs), entity_type.to_name(errs)) {
1273+
} => match (
1274+
target.to_expr(errs),
1275+
entity_type.to_expr_or_special(errs)?.into_name(errs),
1276+
) {
12561277
(Some(t), Some(n)) => match in_entity {
12571278
Some(in_entity) => in_entity.to_expr(errs).map(|in_entity| {
12581279
ExprOrSpecial::Expr(
@@ -3878,6 +3899,26 @@ mod tests {
38783899
),
38793900
),
38803901
),
3902+
(
3903+
r#"principal is User && principal in Group::"friends""#,
3904+
Expr::and(
3905+
Expr::is_entity_type(Expr::var(ast::Var::Principal), "User".parse().unwrap()),
3906+
Expr::is_in(
3907+
Expr::var(ast::Var::Principal),
3908+
Expr::val(r#"Group::"friends""#.parse::<EntityUID>().unwrap()),
3909+
),
3910+
),
3911+
),
3912+
(
3913+
r#"principal is User || principal in Group::"friends""#,
3914+
Expr::or(
3915+
Expr::is_entity_type(Expr::var(ast::Var::Principal), "User".parse().unwrap()),
3916+
Expr::is_in(
3917+
Expr::var(ast::Var::Principal),
3918+
Expr::val(r#"Group::"friends""#.parse::<EntityUID>().unwrap()),
3919+
),
3920+
),
3921+
),
38813922
(
38823923
r#"true && principal is User in principal"#,
38833924
Expr::and(
@@ -3930,6 +3971,31 @@ mod tests {
39303971
),
39313972
),
39323973
),
3974+
(
3975+
r#"if principal is User then 1 else 2"#,
3976+
Expr::ite(
3977+
Expr::is_entity_type(Expr::var(ast::Var::Principal), "User".parse().unwrap()),
3978+
Expr::val(1),
3979+
Expr::val(2),
3980+
),
3981+
),
3982+
(
3983+
r#"if principal is User in Group::"friends" then 1 else 2"#,
3984+
Expr::ite(
3985+
Expr::and(
3986+
Expr::is_entity_type(
3987+
Expr::var(ast::Var::Principal),
3988+
"User".parse().unwrap(),
3989+
),
3990+
Expr::is_in(
3991+
Expr::var(ast::Var::Principal),
3992+
Expr::val(r#"Group::"friends""#.parse::<EntityUID>().unwrap()),
3993+
),
3994+
),
3995+
Expr::val(1),
3996+
Expr::val(2),
3997+
),
3998+
),
39333999
] {
39344000
let e = parse_expr(es).unwrap();
39354001
assert!(
@@ -4012,14 +4078,6 @@ mod tests {
40124078
#[test]
40134079
fn is_err() {
40144080
let invalid_is_policies = [
4015-
(
4016-
r#"permit(principal == ?resource, action, resource);"#,
4017-
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?resource instead of ?principal"),
4018-
),
4019-
(
4020-
r#"permit(principal, action, resource == ?principal);"#,
4021-
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?principal instead of ?resource"),
4022-
),
40234081
(
40244082
r#"permit(principal in Group::"friends" is User, action, resource);"#,
40254083
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found an `is` expression"),
@@ -4030,23 +4088,31 @@ mod tests {
40304088
),
40314089
(
40324090
r#"permit(principal is User == User::"Alice", action, resource);"#,
4033-
ExpectedErrorMessage::error(
4091+
ExpectedErrorMessage::error_and_help(
40344092
"`is` cannot appear in the scope at the same time as `==`",
4093+
"try moving `is` into a `when` condition"
40354094
),
40364095
),
40374096
(
40384097
r#"permit(principal, action, resource is Doc == Doc::"a");"#,
4039-
ExpectedErrorMessage::error(
4098+
ExpectedErrorMessage::error_and_help(
40404099
"`is` cannot appear in the scope at the same time as `==`",
4100+
"try moving `is` into a `when` condition"
40414101
),
40424102
),
40434103
(
40444104
r#"permit(principal is User::"alice", action, resource);"#,
4045-
ExpectedErrorMessage::error(r#"unexpected token `"alice"`"#),
4105+
ExpectedErrorMessage::error_and_help(
4106+
r#"right hand side of an `is` expression must be an entity type name, but got `User::"alice"`"#,
4107+
"try using `==` to test for equality"
4108+
),
40464109
),
40474110
(
40484111
r#"permit(principal, action, resource is File::"f");"#,
4049-
ExpectedErrorMessage::error(r#"unexpected token `"f"`"#),
4112+
ExpectedErrorMessage::error_and_help(
4113+
r#"right hand side of an `is` expression must be an entity type name, but got `File::"f"`"#,
4114+
"try using `==` to test for equality"
4115+
),
40504116
),
40514117
(
40524118
r#"permit(principal is User in 1, action, resource);"#,
@@ -4066,27 +4132,88 @@ mod tests {
40664132
"expected an entity uid or matching template slot, found name `User`",
40674133
),
40684134
),
4135+
(
4136+
r#"permit(principal is User::"Alice" in Group::"f", action, resource);"#,
4137+
ExpectedErrorMessage::error_and_help(
4138+
r#"right hand side of an `is` expression must be an entity type name, but got `User::"Alice"`"#,
4139+
"try using `==` to test for equality"
4140+
),
4141+
),
40694142
(
40704143
r#"permit(principal, action, resource is File in File);"#,
40714144
ExpectedErrorMessage::error(
40724145
"expected an entity uid or matching template slot, found name `File`",
40734146
),
40744147
),
4148+
(
4149+
r#"permit(principal, action, resource is File::"file" in Folder::"folder");"#,
4150+
ExpectedErrorMessage::error_and_help(
4151+
r#"right hand side of an `is` expression must be an entity type name, but got `File::"file"`"#,
4152+
"try using `==` to test for equality"
4153+
),
4154+
),
40754155
(
40764156
r#"permit(principal is 1, action, resource);"#,
4077-
ExpectedErrorMessage::error("unexpected token `1`"),
4157+
ExpectedErrorMessage::error_and_help(
4158+
r#"right hand side of an `is` expression must be an entity type name, but got `1`"#,
4159+
"try using `==` to test for equality"
4160+
),
40784161
),
40794162
(
40804163
r#"permit(principal, action, resource is 1);"#,
4081-
ExpectedErrorMessage::error("unexpected token `1`"),
4164+
ExpectedErrorMessage::error_and_help(
4165+
r#"right hand side of an `is` expression must be an entity type name, but got `1`"#,
4166+
"try using `==` to test for equality"
4167+
),
40824168
),
40834169
(
40844170
r#"permit(principal, action is Action, resource);"#,
4085-
ExpectedErrorMessage::error("`is` cannot appear in the action scope"),
4171+
ExpectedErrorMessage::error_and_help(
4172+
"`is` cannot appear in the action scope",
4173+
"try moving `action is ..` into a `when` condition"
4174+
),
4175+
),
4176+
(
4177+
r#"permit(principal, action is Action::"a", resource);"#,
4178+
ExpectedErrorMessage::error_and_help(
4179+
"`is` cannot appear in the action scope",
4180+
"try moving `action is ..` into a `when` condition"
4181+
),
40864182
),
40874183
(
40884184
r#"permit(principal, action is Action in Action::"A", resource);"#,
4089-
ExpectedErrorMessage::error("`is` cannot appear in the action scope"),
4185+
ExpectedErrorMessage::error_and_help(
4186+
"`is` cannot appear in the action scope",
4187+
"try moving `action is ..` into a `when` condition"
4188+
),
4189+
),
4190+
(
4191+
r#"permit(principal, action is Action in Action, resource);"#,
4192+
ExpectedErrorMessage::error_and_help(
4193+
"`is` cannot appear in the action scope",
4194+
"try moving `action is ..` into a `when` condition"
4195+
),
4196+
),
4197+
(
4198+
r#"permit(principal, action is Action::"a" in Action::"b", resource);"#,
4199+
ExpectedErrorMessage::error_and_help(
4200+
"`is` cannot appear in the action scope",
4201+
"try moving `action is ..` into a `when` condition"
4202+
),
4203+
),
4204+
(
4205+
r#"permit(principal, action is Action in ?action, resource);"#,
4206+
ExpectedErrorMessage::error_and_help(
4207+
"`is` cannot appear in the action scope",
4208+
"try moving `action is ..` into a `when` condition"
4209+
),
4210+
),
4211+
(
4212+
r#"permit(principal, action is ?action, resource);"#,
4213+
ExpectedErrorMessage::error_and_help(
4214+
"`is` cannot appear in the action scope",
4215+
"try moving `action is ..` into a `when` condition"
4216+
),
40904217
),
40914218
(
40924219
r#"permit(principal is User in ?resource, action, resource);"#,
@@ -4096,9 +4223,59 @@ mod tests {
40964223
r#"permit(principal, action, resource is Folder in ?principal);"#,
40974224
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?principal instead of ?resource"),
40984225
),
4226+
(
4227+
r#"permit(principal is ?principal, action, resource);"#,
4228+
ExpectedErrorMessage::error_and_help(
4229+
"right hand side of an `is` expression must be an entity type name, but got `?principal`",
4230+
"try using `==` to test for equality"
4231+
),
4232+
),
4233+
(
4234+
r#"permit(principal, action, resource is ?resource);"#,
4235+
ExpectedErrorMessage::error_and_help(
4236+
"right hand side of an `is` expression must be an entity type name, but got `?resource`",
4237+
"try using `==` to test for equality"
4238+
),
4239+
),
40994240
(
41004241
r#"permit(principal, action, resource) when { principal is 1 };"#,
4101-
ExpectedErrorMessage::error("unexpected token `1`"),
4242+
ExpectedErrorMessage::error_and_help(
4243+
r#"right hand side of an `is` expression must be an entity type name, but got `1`"#,
4244+
"try using `==` to test for equality"
4245+
),
4246+
),
4247+
(
4248+
r#"permit(principal, action, resource) when { principal is User::"alice" in Group::"friends" };"#,
4249+
ExpectedErrorMessage::error_and_help(
4250+
r#"right hand side of an `is` expression must be an entity type name, but got `User::"alice"`"#,
4251+
"try using `==` to test for equality"
4252+
),
4253+
),
4254+
(
4255+
r#"permit(principal, action, resource) when { principal is ! User::"alice" in Group::"friends" };"#,
4256+
ExpectedErrorMessage::error_and_help(
4257+
r#"right hand side of an `is` expression must be an entity type name, but got `!User::"alice"`"#,
4258+
"try using `==` to test for equality"
4259+
),
4260+
),
4261+
(
4262+
r#"permit(principal, action, resource) when { principal is User::"alice" + User::"alice" in Group::"friends" };"#,
4263+
ExpectedErrorMessage::error_and_help(
4264+
r#"right hand side of an `is` expression must be an entity type name, but got `User::"alice" + User::"alice"`"#,
4265+
"try using `==` to test for equality"
4266+
),
4267+
),
4268+
(
4269+
r#"permit(principal, action, resource) when { principal is User in User::"alice" in Group::"friends" };"#,
4270+
ExpectedErrorMessage::error(
4271+
"unexpected token `in`"
4272+
),
4273+
),
4274+
(
4275+
r#"permit(principal, action, resource) when { principal is User == User::"alice" in Group::"friends" };"#,
4276+
ExpectedErrorMessage::error(
4277+
"unexpected token `==`"
4278+
),
41024279
),
41034280
];
41044281
for (p_src, expected) in invalid_is_policies {
@@ -4199,6 +4376,14 @@ mod tests {
41994376
#[test]
42004377
fn invalid_slot() {
42014378
let invalid_policies = [
4379+
(
4380+
r#"permit(principal == ?resource, action, resource);"#,
4381+
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?resource instead of ?principal"),
4382+
),
4383+
(
4384+
r#"permit(principal, action, resource == ?principal);"#,
4385+
ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?principal instead of ?resource"),
4386+
),
42024387
(
42034388
r#"permit(principal, action, resource) when { principal == ?foo};"#,
42044389
ExpectedErrorMessage::error_and_help(

‎cedar-policy-core/src/parser/err.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,10 @@ pub enum ToASTErrorKind {
267267
/// Returned when the right hand side of a `like` expression is not a constant pattern literal
268268
#[error("right hand side of a `like` expression must be a pattern literal, but got `{0}`")]
269269
InvalidPattern(String),
270+
/// Returned when the right hand side of a `is` expression is not an entity type name
271+
#[error("right hand side of an `is` expression must be an entity type name, but got `{0}`")]
272+
#[diagnostic(help("try using `==` to test for equality"))]
273+
IsInvalidName(String),
270274
/// Returned when an unexpected node is in the policy scope clause
271275
#[error("expected {expected}, found {got}")]
272276
WrongNode {
@@ -473,9 +477,11 @@ impl std::fmt::Display for Ref {
473477
pub enum InvalidIsError {
474478
/// The action scope may not contain an `is`
475479
#[error("`is` cannot appear in the action scope")]
480+
#[diagnostic(help("try moving `action is ..` into a `when` condition"))]
476481
ActionScope,
477482
/// An `is` cannot appear with this operator in the policy scope
478483
#[error("`is` cannot appear in the scope at the same time as `{0}`")]
484+
#[diagnostic(help("try moving `is` into a `when` condition"))]
479485
WrongOp(cst::RelOp),
480486
}
481487

0 commit comments

Comments
 (0)