Skip to content
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.

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

RFC 62 implementation #1327

Merged
merged 19 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
8 changes: 8 additions & 0 deletions cedar-policy-core/src/ast/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1861,6 +1861,14 @@ impl<T> Expr<T> {
}
}
}

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
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy-core/src/ast/name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
71 changes: 71 additions & 0 deletions cedar-policy-core/src/est.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
23 changes: 18 additions & 5 deletions cedar-policy-core/src/est/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -1170,11 +1171,23 @@ impl TryFrom<&Node<Option<cst::Relation>>> 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't be trivial since the types are all different, but I wonder if there's some nice way to make this code generic enough to be shared here and in construct_exprs_extended_has in the CST to AST conversion. I think we'd just need some sort of ExprBuilder<T> trait providing has_attr(T,SmolStr)->T, get_attr(T, SmolStr)->T and and(T, T) -> T. IDK if that'd be worthwhile, but maybe it's an idea that could help share code elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me like a good idea for a followup; this PR is large enough already and I think it would be good to review that trait change on its own

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a good idea but worth a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be very cool if cst->{ast,est} could share all of their code, but that sounds like a lot of work

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()?;
Expand Down
70 changes: 69 additions & 1 deletion cedar-policy-core/src/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5525,7 +5525,7 @@ pub mod test {
}

#[test]
fn parital_if_noerrors() {
fn partial_if_noerrors() {
let guard = Expr::get_attr(Expr::unknown(Unknown::new_untyped("a")), "field".into());
let cons = Expr::val(1);
let alt = Expr::val(2);
Expand Down Expand Up @@ -6144,4 +6144,72 @@ pub mod test {
);
assert_matches!(eval.partial_eval_expr(&e), Err(_));
}

#[test]
fn interpret_extended_has() {
let es = Entities::new();
let eval = Evaluator::new(empty_request(), &es, Extensions::none());
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has a.b.c
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(true));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has a.b
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(true));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has a
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(true));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has b.c
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(false));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has c
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(false));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has d
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(false));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has "🚫"
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(false));
});

assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has a.b.c && {a: {b: {c: 1}}}.a.b.c == 1
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(true));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has a.b && {a: {b: {c: 1}}}.a.b == {c: 1}
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(true));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has a && {a: {b: {c: 1}}}.a == {b: {c: 1}}
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(true));
});
assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {d: 1}}} has a.b.c && {a: {b: {d: 1}}}.a.b.c == 1
"#).unwrap()), Ok(v) => {
assert_eq!(v, Value::from(false));
});

assert_matches!(eval.interpret_inline_policy(&parse_expr(r#"
{a: {b: {c: 1}}} has a.b && {a: {b: {c: 1}}}.a.b.d == 1
"#).unwrap()), Err(EvaluationError::RecordAttrDoesNotExist(err)) => {
assert_eq!(err.attr, "d");
});
}
}
Loading