diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 191d66079e..0740725cb9 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -1384,6 +1384,19 @@ impl PrincipalConstraint { _ => self, } } + + /// If the principal constraint has a slot, return it + pub fn get_slot(self) -> Option { + match self.constraint { + PrincipalOrResourceConstraint::Eq(EntityReference::Slot(l)) + | PrincipalOrResourceConstraint::In(EntityReference::Slot(l)) + | PrincipalOrResourceConstraint::IsIn(_, EntityReference::Slot(l)) => Some(Slot { + id: SlotId::principal(), + loc: l, + }), + _ => None, + } + } } impl std::fmt::Display for PrincipalConstraint { @@ -1491,6 +1504,19 @@ impl ResourceConstraint { _ => self, } } + + /// If the resource constraint has a slot, return it + pub fn get_slot(self) -> Option { + match self.constraint { + PrincipalOrResourceConstraint::Eq(EntityReference::Slot(l)) + | PrincipalOrResourceConstraint::In(EntityReference::Slot(l)) + | PrincipalOrResourceConstraint::IsIn(_, EntityReference::Slot(l)) => Some(Slot { + id: SlotId::resource(), + loc: l, + }), + _ => None, + } + } } impl std::fmt::Display for ResourceConstraint { @@ -2365,7 +2391,7 @@ mod test { .exactly_one_underline("?principal") .build() ); - assert_eq!(e.len(), 2); + assert_eq!(e.len(), 1); }); } diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index fbfd55f52f..d7ce4dc759 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -138,9 +138,11 @@ impl Clause { /// Fill in any slots in the clause using the values in `vals`. Throws an /// error if `vals` doesn't contain a necessary mapping, but does not throw /// an error if `vals` contains unused mappings. - pub fn link(self, _vals: &HashMap) -> Result { - // currently, slots are not allowed in clauses - Ok(self) + pub fn link(self, vals: &HashMap) -> Result { + match self { + Clause::When(e) => Ok(Clause::When(e.link(vals)?)), + Clause::Unless(e) => Ok(Clause::Unless(e.link(vals)?)), + } } /// Substitute entity literals @@ -157,8 +159,10 @@ impl Clause { /// Returns true if this clause has a slot. pub fn has_slot(&self) -> bool { - // currently, slots are not allowed in clauses - false + match self { + Clause::When(e) => e.has_slot(), + Clause::Unless(e) => e.has_slot(), + } } } @@ -280,10 +284,13 @@ impl Policy { id: Option, ) -> Result { let id = id.unwrap_or_else(|| ast::PolicyID::from_string("JSON policy")); + let has_principal_slot = self.principal.has_slot(); + let has_resource_slot = self.resource.has_slot(); + let mut conditions_iter = self .conditions .into_iter() - .map(|cond| cond.try_into_ast(&id)); + .map(|cond| cond.try_into_ast(has_principal_slot, has_resource_slot, &id)); let conditions = match conditions_iter.next() { None => ast::Expr::val(true), Some(first) => ast::ExprBuilder::with_data(()) @@ -312,25 +319,50 @@ impl Policy { } impl Clause { - fn filter_slots(e: ast::Expr, is_when: bool) -> Result { - let first_slot = e.slots().next(); - if let Some(slot) = first_slot { - Err(parse_errors::SlotsInConditionClause { - slot, - clause_type: if is_when { "when" } else { "unless" }, + fn filter_slots( + e: ast::Expr, + has_principal_slot: bool, + has_resource_slot: bool, + is_when: bool, + ) -> Result { + for slot in e.slots() { + if (slot.id.is_principal() && !has_principal_slot) + || (slot.id.is_resource() && !has_resource_slot) + { + return Err(FromJsonError::SlotsNotInScopeInConditionClause( + parse_errors::SlotsNotInScopeInConditionClause { + slot, + clause_type: if is_when { "when" } else { "unless" }, + }, + )); } - .into()) - } else { - Ok(e) } + Ok(e) } + /// `id` is the ID of the policy the clause belongs to, used only for reporting errors - fn try_into_ast(self, id: &ast::PolicyID) -> Result { + /// has_principal_slot/has_resource_slot tells us whether there is a principal/resource slot in the scope + /// so we know when a slot is allowed to appear in the condition + /// an error is thrown otherwise if there is a slot not in the scope but in the condition + fn try_into_ast( + self, + has_principal_slot: bool, + has_resource_slot: bool, + id: &ast::PolicyID, + ) -> Result { match self { - Clause::When(expr) => Self::filter_slots(expr.try_into_ast(id)?, true), - Clause::Unless(expr) => { - Self::filter_slots(ast::Expr::not(expr.try_into_ast(id)?), false) - } + Clause::When(expr) => Self::filter_slots( + expr.try_into_ast(id)?, + has_principal_slot, + has_resource_slot, + true, + ), + Clause::Unless(expr) => Self::filter_slots( + ast::Expr::not(expr.try_into_ast(id)?), + has_principal_slot, + has_resource_slot, + false, + ), } } } @@ -420,10 +452,12 @@ impl std::fmt::Display for Clause { #[cfg(test)] mod test { use super::*; + use crate::ast::SlotId; use crate::parser::{self, parse_policy_or_template_to_est}; use crate::test_utils::*; use cool_asserts::assert_matches; use serde_json::json; + use std::collections::HashMap; /// helper function to just do EST data structure --> JSON --> EST data structure. /// This roundtrip should be lossless for all policies. @@ -500,6 +534,23 @@ mod test { cst.try_into().expect("Failed to convert to EST") } + #[track_caller] + fn cst_to_est_link_roundtrip( + policy_str: &str, + expected_policy_str: &str, + vals: &HashMap, + ) { + let cst = parser::text_to_cst::parse_policy(policy_str) + .unwrap() + .node + .unwrap(); + let est: Policy = cst.try_into().unwrap(); + + let new_policy = est.link(vals).unwrap(); + + assert_eq!(new_policy.to_string(), expected_policy_str); + } + #[test] fn empty_policy() { let policy = "permit(principal, action, resource);"; @@ -3320,6 +3371,237 @@ mod test { ); } + #[test] + fn link_with_slots_in_condition() { + let template = r#" + permit( + principal == ?principal, + action == Action::"view", + resource in ?resource + ) when { + ?principal in resource.owners + }; + "#; + let cst = parser::text_to_cst::parse_policy(template) + .unwrap() + .node + .unwrap(); + let est: Policy = cst.try_into().unwrap(); + + let linked_est = est + .link(&HashMap::from_iter([ + ( + ast::SlotId::principal(), + EntityUidJson::new("XYZCorp::User", "12UA45"), + ), + (ast::SlotId::resource(), EntityUidJson::new("Folder", "abc")), + ])) + .expect("did fill all the slots"); + + let old_est = linked_est.clone(); + let roundtripped = est_roundtrip(linked_est.clone()); + assert_eq!(&old_est, &roundtripped); + let est = text_roundtrip(&old_est); + assert_eq!(&old_est, &est); + + let expected_json = json!( + { + "effect": "permit", + "principal": { + "op": "==", + "entity": { "type": "XYZCorp::User", "id": "12UA45" }, + }, + "action": { + "op": "==", + "entity": { "type": "Action", "id": "view" }, + }, + "resource": { + "op": "in", + "entity": { "type": "Folder", "id": "abc" }, + }, + "conditions": [ + { + "kind": "when", + "body": { + "in": { + "left": { + "Value": { + "__entity": { "type": "XYZCorp::User", "id": "12UA45" } + } + }, + "right": { + ".": { + "left": { + "Var": "resource" + }, + "attr": "owners" + } + } + } + } + } + ], + } + ); + let linked_json = serde_json::to_value(linked_est).unwrap(); + assert_eq!( + linked_json, + expected_json, + "\nExpected:\n{}\n\nActual:\n{}\n\n", + serde_json::to_string_pretty(&expected_json).unwrap(), + serde_json::to_string_pretty(&linked_json).unwrap(), + ); + } + + #[test] + fn link_with_slots_in_condition_cst_to_est_roundtrip() { + let vals = &HashMap::from_iter([( + SlotId::principal(), + r#"User::"Bob""#.parse::().unwrap().into(), + )]); + + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal == User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" == User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { !?principal };"#, + r#"permit(principal == User::"Bob", action, resource) when { !User::"Bob" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { -(?principal) };"#, + r#"permit(principal == User::"Bob", action, resource) when { -(User::"Bob") };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal != User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" != User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal < User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" < User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal <= User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" <= User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal > User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" > User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal >= User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" >= User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal && User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" && User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal || User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" || User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal + User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" + User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal - User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" - User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal * User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" * User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.contains(User::"Alice") };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob".contains(User::"Alice") };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.containsAll(User::"Alice") };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob".containsAll(User::"Alice") };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.containsAny(User::"Alice") };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob".containsAny(User::"Alice") };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.isEmpty() };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob".isEmpty() };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.getTag(User::"Alice") };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob".getTag(User::"Alice") };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.hasTag(User::"Alice") };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob".hasTag(User::"Alice") };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.attr };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob"["attr"] };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal has attr };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" has "attr" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal like "*" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" like "*" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal is User };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" is User };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal is User in User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob" is User in User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { if ?principal then User::"Alice" else User::"Alice" };"#, + r#"permit(principal == User::"Bob", action, resource) when { if User::"Bob" then User::"Alice" else User::"Alice" };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { [?principal] };"#, + r#"permit(principal == User::"Bob", action, resource) when { [User::"Bob"] };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { {a: ?principal} };"#, + r#"permit(principal == User::"Bob", action, resource) when { {"a": User::"Bob"} };"#, + vals, + ); + cst_to_est_link_roundtrip( + r#"permit(principal == ?principal, action, resource) when { ?principal.lessThan(User::"Alice") };"#, + r#"permit(principal == User::"Bob", action, resource) when { User::"Bob".lessThan(User::"Alice") };"#, + vals, + ); + } + #[test] fn eid_with_nulls() { let policy = r#" @@ -3575,6 +3857,39 @@ mod test { ); } ); + + let template = json!({ + "effect": "permit", + "principal": { "op": "All" }, + "action": { "op": "All" }, + "resource": { "op": "All" }, + "conditions": [ + { + "kind": "when", + "body": { + "==": { + "left": { "Var": "principal" }, + "right": { "Slot": "?principal" } + } + } + } + ] + }); + + let est: Policy = serde_json::from_value(template).unwrap(); + let ast: Result = est.try_into_ast_policy(None); + assert_matches!( + ast, + Err(e) => { + expect_err( + "", + &miette::Report::new(e), + &ExpectedErrorMessageBuilder::error("found template slot ?principal in a `when` clause") + .help("?principal needs to appear in the scope to appear in the condition of the template") + .build(), + ); + } + ) } #[test] @@ -4689,6 +5004,46 @@ mod test { let est: Policy = cst.try_into().unwrap(); assert!(!est.is_template(), "Static policy marked as template"); } + + #[test] + fn template_condition_has_slot() { + let template: &'static str = r#" + permit( + principal == ?principal, + action == Action::"view", + resource + ) when { + ?principal in resource.owners && ?principal has owners + }; + "#; + let cst = parser::text_to_cst::parse_policy(template) + .unwrap() + .node + .unwrap(); + let est: Policy = cst.try_into().unwrap(); + let has_slot = est.conditions.iter().any(|c| c.has_slot()); + assert!(has_slot, "Policy condition not marked as having a slot"); + + let template: &'static str = r#" + permit( + principal == ?principal, + action == Action::"view", + resource + ) when { + principal == resource.owners + }; + "#; + let cst = parser::text_to_cst::parse_policy(template) + .unwrap() + .node + .unwrap(); + let est: Policy = cst.try_into().unwrap(); + let has_slot = est.conditions.iter().any(|c| c.has_slot()); + assert!( + !has_slot, + "Policy condition marked as having a slot when it does not have a slot" + ) + } } #[cfg(test)] diff --git a/cedar-policy-core/src/est/err.rs b/cedar-policy-core/src/est/err.rs index 2a6af4ff87..7562c1551f 100644 --- a/cedar-policy-core/src/est/err.rs +++ b/cedar-policy-core/src/est/err.rs @@ -15,7 +15,7 @@ */ use crate::ast; -use crate::entities::json::err::JsonDeserializationError; +use crate::entities::json::err::{JsonDeserializationError, JsonSerializationError}; use crate::parser::err::{parse_errors, ParseErrors}; use crate::parser::unescape; use miette::Diagnostic; @@ -53,7 +53,7 @@ pub enum FromJsonError { /// EST contained a template slot in policy condition #[error(transparent)] #[diagnostic(transparent)] - SlotsInConditionClause(#[from] parse_errors::SlotsInConditionClause), + SlotsNotInScopeInConditionClause(#[from] parse_errors::SlotsNotInScopeInConditionClause), /// EST contained the empty JSON object `{}` where a key (operator) was expected #[error("missing operator, found empty object")] MissingOperator, @@ -104,7 +104,7 @@ pub enum PolicySetFromJsonError { } /// Errors while linking a policy -#[derive(Debug, PartialEq, Eq, Diagnostic, Error)] +#[derive(Debug, Diagnostic, Error)] pub enum LinkingError { /// Template contains this slot, but a value wasn't provided for it #[error("failed to link template: no value provided for `{slot}`")] @@ -112,6 +112,16 @@ pub enum LinkingError { /// Slot which didn't have a value provided for it slot: ast::SlotId, }, + + /// Encounter JSON Serialization error when linking + #[error(transparent)] + #[diagnostic(transparent)] + UnableToSerializeJSONValueProvided(#[from] JsonSerializationError), + + /// Encountered JSON deserialization error when linking + #[error(transparent)] + #[diagnostic(transparent)] + InvalidJSON(#[from] JsonDeserializationError), } impl From for FromJsonError { diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs index 9dc89c3c30..848f9e42a9 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -22,8 +22,9 @@ use crate::ast::Infallible; use crate::ast::{self, BoundedDisplay, EntityUID}; use crate::entities::json::{ err::EscapeKind, err::JsonDeserializationError, err::JsonDeserializationErrorContext, - CedarValueJson, FnAndArgs, + CedarValueJson, EntityUidJson, FnAndArgs, }; +use crate::est::LinkingError; use crate::expr_builder::ExprBuilder; use crate::extensions::Extensions; use crate::jsonvalue::JsonValueWithNoDuplicateKeys; @@ -877,6 +878,174 @@ impl Expr { } } } + + /// Instantiate all the slots in an expression with their values + pub fn link(self, vals: &HashMap) -> Result { + match self { + Expr::ExprNoExt(e) => match e { + v @ ExprNoExt::Value(_) => Ok(Expr::ExprNoExt(v)), + v @ ExprNoExt::Var(_) => Ok(Expr::ExprNoExt(v)), + ExprNoExt::Slot(slot) => match vals.get(&slot) { + Some(e) => { + let literal = CedarValueJson::from_lit( + e.clone() + .into_euid(|| JsonDeserializationErrorContext::Unknown)? + .into(), + ); + Ok(Expr::ExprNoExt(ExprNoExt::Value(literal))) + } + None => Err(LinkingError::MissedSlot { slot }), + }, + ExprNoExt::Not { arg } => Ok(Expr::ExprNoExt(ExprNoExt::Not { + arg: Arc::new(Arc::unwrap_or_clone(arg).link(vals)?), + })), + ExprNoExt::Neg { arg } => Ok(Expr::ExprNoExt(ExprNoExt::Neg { + arg: Arc::new(Arc::unwrap_or_clone(arg).link(vals)?), + })), + ExprNoExt::Eq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Eq { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::NotEq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::NotEq { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::In { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::In { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::Less { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Less { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::LessEq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::LessEq { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::Greater { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Greater { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::GreaterEq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::GreaterEq { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::And { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::And { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::Or { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Or { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::Add { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Add { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::Sub { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Sub { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::Mul { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Mul { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::Contains { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Contains { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::ContainsAll { left, right } => { + Ok(Expr::ExprNoExt(ExprNoExt::ContainsAll { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })) + } + ExprNoExt::ContainsAny { left, right } => { + Ok(Expr::ExprNoExt(ExprNoExt::ContainsAny { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })) + } + ExprNoExt::IsEmpty { arg } => Ok(Expr::ExprNoExt(ExprNoExt::IsEmpty { + arg: Arc::new(Arc::unwrap_or_clone(arg).link(vals)?), + })), + ExprNoExt::GetTag { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::GetTag { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::HasTag { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::HasTag { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + right: Arc::new(Arc::unwrap_or_clone(right).link(vals)?), + })), + ExprNoExt::GetAttr { left, attr } => Ok(Expr::ExprNoExt(ExprNoExt::GetAttr { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + attr, + })), + ExprNoExt::HasAttr { left, attr } => Ok(Expr::ExprNoExt(ExprNoExt::HasAttr { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + attr, + })), + ExprNoExt::Like { left, pattern } => Ok(Expr::ExprNoExt(ExprNoExt::Like { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + pattern, + })), + ExprNoExt::Is { + left, + entity_type, + in_expr, + } => match in_expr { + Some(in_expr) => Ok(Expr::ExprNoExt(ExprNoExt::Is { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + entity_type, + in_expr: Some(Arc::new(Arc::unwrap_or_clone(in_expr).link(vals)?)), + })), + None => Ok(Expr::ExprNoExt(ExprNoExt::Is { + left: Arc::new(Arc::unwrap_or_clone(left).link(vals)?), + entity_type, + in_expr: None, + })), + }, + ExprNoExt::If { + cond_expr, + then_expr, + else_expr, + } => Ok(Expr::ExprNoExt(ExprNoExt::If { + cond_expr: Arc::new(Arc::unwrap_or_clone(cond_expr).link(vals)?), + then_expr: Arc::new(Arc::unwrap_or_clone(then_expr).link(vals)?), + else_expr: Arc::new(Arc::unwrap_or_clone(else_expr).link(vals)?), + })), + ExprNoExt::Set(v) => { + let mut new_v = vec![]; + for e in v { + new_v.push(e.link(vals)?); + } + Ok(Expr::ExprNoExt(ExprNoExt::Set(new_v))) + } + ExprNoExt::Record(m) => { + let mut new_m = BTreeMap::new(); + for (k, v) in m { + new_m.insert(k, v.link(vals)?); + } + Ok(Expr::ExprNoExt(ExprNoExt::Record(new_m))) + } + #[cfg(feature = "tolerant-ast")] + ExprNoExt::Error(_) => Err(LinkingError::InvalidJSON( + JsonDeserializationError::ASTErrorNode, + )), + }, + Expr::ExtFuncCall(e_fn_call) => { + let mut new_m = HashMap::new(); + for (k, v) in e_fn_call.call { + let mut new_v = vec![]; + for e in v { + new_v.push(e.link(vals)?); + } + new_m.insert(k, new_v); + } + Ok(Expr::ExtFuncCall(ExtFuncCall { call: new_m })) + } + } + } } impl Expr { @@ -1060,6 +1229,76 @@ impl Expr { Expr::ExprNoExt(ExprNoExt::Error(_)) => Err(FromJsonError::ASTErrorNode), } } + + /// Returns true if expr has a slot + pub fn has_slot(&self) -> bool { + match self { + Expr::ExprNoExt(ExprNoExt::Value(..)) => false, + Expr::ExprNoExt(ExprNoExt::Var(..)) => false, + Expr::ExprNoExt(ExprNoExt::Slot(..)) => true, + Expr::ExprNoExt(ExprNoExt::Not { arg }) => arg.has_slot(), + Expr::ExprNoExt(ExprNoExt::Neg { arg }) => arg.has_slot(), + Expr::ExprNoExt(ExprNoExt::Eq { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::NotEq { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::In { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::Less { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::LessEq { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::Greater { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::GreaterEq { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::And { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::Or { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::Add { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::Sub { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::Mul { left, right }) => left.has_slot() || right.has_slot(), + Expr::ExprNoExt(ExprNoExt::Contains { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::ContainsAll { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::ContainsAny { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::IsEmpty { arg }) => arg.has_slot(), + Expr::ExprNoExt(ExprNoExt::GetTag { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::HasTag { left, right }) => { + left.has_slot() || right.has_slot() + } + Expr::ExprNoExt(ExprNoExt::GetAttr { left, .. }) => left.has_slot(), + Expr::ExprNoExt(ExprNoExt::HasAttr { left, .. }) => left.has_slot(), + Expr::ExprNoExt(ExprNoExt::Like { left, .. }) => left.has_slot(), + Expr::ExprNoExt(ExprNoExt::Is { left, in_expr, .. }) => match in_expr { + Some(right) => left.has_slot() || right.has_slot(), + None => left.has_slot(), + }, + Expr::ExprNoExt(ExprNoExt::If { + cond_expr, + then_expr, + else_expr, + }) => cond_expr.has_slot() || then_expr.has_slot() || else_expr.has_slot(), + Expr::ExprNoExt(ExprNoExt::Set(elements)) => { + elements.iter().any(|expr| expr.has_slot()) + } + Expr::ExprNoExt(ExprNoExt::Record(map)) => map + .iter() + .fold(false, |init, (_, expr)| init || expr.has_slot()), + Expr::ExtFuncCall(ExtFuncCall { call }) => call.iter().fold(false, |init, (_, v)| { + init || (v.iter().any(|expr| expr.has_slot())) + }), + #[cfg(feature = "tolerant-ast")] + Expr::ExprNoExt(ExprNoExt::Error(_)) => false, + } + } } impl From for Expr { diff --git a/cedar-policy-core/src/parser.rs b/cedar-policy-core/src/parser.rs index 9a660057b6..e22d59382d 100644 --- a/cedar-policy-core/src/parser.rs +++ b/cedar-policy-core/src/parser.rs @@ -813,11 +813,12 @@ mod tests { resource == ?resource }; "#; - let slot_in_when_clause = - ExpectedErrorMessageBuilder::error("found template slot ?resource in a `when` clause") - .help("slots are currently unsupported in `when` clauses") - .exactly_one_underline("?resource") - .build(); + let slot_in_when_clause = ExpectedErrorMessageBuilder::error( + "found template slot ?resource in a `when` clause", + ) + .help("?resource needs to appear in the scope to appear in the condition of the template") + .exactly_one_underline("?resource") + .build(); let unexpected_template = ExpectedErrorMessageBuilder::error( "expected a static policy, got a template containing the slot ?resource", ) @@ -852,11 +853,12 @@ mod tests { resource == ?principal }; "#; - let slot_in_when_clause = - ExpectedErrorMessageBuilder::error("found template slot ?principal in a `when` clause") - .help("slots are currently unsupported in `when` clauses") - .exactly_one_underline("?principal") - .build(); + let slot_in_when_clause = ExpectedErrorMessageBuilder::error( + "found template slot ?principal in a `when` clause", + ) + .help("?principal needs to appear in the scope to appear in the condition of the template") + .exactly_one_underline("?principal") + .build(); let unexpected_template = ExpectedErrorMessageBuilder::error( "expected a static policy, got a template containing the slot ?principal", ) @@ -922,7 +924,7 @@ mod tests { let slot_in_unless_clause = ExpectedErrorMessageBuilder::error( "found template slot ?resource in a `unless` clause", ) - .help("slots are currently unsupported in `unless` clauses") + .help("?resource needs to appear in the scope to appear in the condition of the template") .exactly_one_underline("?resource") .build(); let unexpected_template = ExpectedErrorMessageBuilder::error( @@ -962,7 +964,7 @@ mod tests { let slot_in_unless_clause = ExpectedErrorMessageBuilder::error( "found template slot ?principal in a `unless` clause", ) - .help("slots are currently unsupported in `unless` clauses") + .help("?principal needs to appear in the scope to appear in the condition of the template") .exactly_one_underline("?principal") .build(); let unexpected_template = ExpectedErrorMessageBuilder::error( @@ -1029,15 +1031,16 @@ mod tests { resource == ?resource }; "#; - let slot_in_when_clause = - ExpectedErrorMessageBuilder::error("found template slot ?resource in a `when` clause") - .help("slots are currently unsupported in `when` clauses") - .exactly_one_underline("?resource") - .build(); + let slot_in_when_clause = ExpectedErrorMessageBuilder::error( + "found template slot ?resource in a `when` clause", + ) + .help("?resource needs to appear in the scope to appear in the condition of the template") + .exactly_one_underline("?resource") + .build(); let slot_in_unless_clause = ExpectedErrorMessageBuilder::error( "found template slot ?resource in a `unless` clause", ) - .help("slots are currently unsupported in `unless` clauses") + .help("?resource needs to appear in the scope to appear in the condition of the template") .exactly_one_underline("?resource") .build(); let unexpected_template = ExpectedErrorMessageBuilder::error( diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index dfb27b6edb..a0aba03672 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -270,16 +270,18 @@ impl Node> { ) .into()), // The source failed to parse completely. If the parse errors include - // `SlotsInConditionClause` also add an `ExpectedStaticPolicy` error. + // `SlotsNotInScopeInConditionClause` also add an `ExpectedStaticPolicy` error. Err(mut errs) => { let new_errs = errs .iter() .filter_map(|err| match err { ParseError::ToAST(err) => match err.kind() { - ToASTErrorKind::SlotsInConditionClause(inner) => Some(ToASTError::new( - ToASTErrorKind::expected_static_policy(inner.slot.clone()), - err.source_loc().into_maybe_loc(), - )), + ToASTErrorKind::SlotsNotInScopeInConditionClause(inner) => { + Some(ToASTError::new( + ToASTErrorKind::expected_static_policy(inner.slot.clone()), + err.source_loc().into_maybe_loc(), + )) + } _ => None, }, _ => None, @@ -320,24 +322,38 @@ impl Node> { let maybe_scope = policy.extract_scope(); // convert conditions - let maybe_conds = ParseErrors::transpose(policy.conds.iter().map(|c| { - let (e, is_when) = c.to_expr::>()?; + let maybe_conds = match policy.extract_scope() { + Ok((p, _, r)) => { + let slots_in_scope: HashSet = + HashSet::from_iter(vec![p.get_slot(), r.get_slot()].into_iter().flatten()); + + ParseErrors::transpose(policy.conds.iter().map(|c| { + let (e, is_when) = c.to_expr::>()?; + let slot_errs = + e.slots() + .filter(|slot| !slots_in_scope.contains(slot)) + .map(|slot| { + ToASTError::new( + ToASTErrorKind::slots_not_in_scope_in_condition_clause( + slot.clone(), + if is_when { "when" } else { "unless" }, + ), + slot.loc, + ) + .into() + }); - let slot_errs = e.slots().map(|slot| { - ToASTError::new( - ToASTErrorKind::slots_in_condition_clause( - slot.clone(), - if is_when { "when" } else { "unless" }, - ), - slot.loc.or_else(|| c.loc.clone()), - ) - .into() - }); - match ParseErrors::from_iter(slot_errs) { - Some(errs) => Err(errs), - None => Ok(e), + match ParseErrors::from_iter(slot_errs) { + Some(errs) => Err(errs), + None => Ok(e), + } + })) } - })); + Err(_) => ParseErrors::transpose(policy.conds.iter().map(|c| { + let (e, _) = c.to_expr::>()?; + Ok(e) + })), + }; let (effect, annotations, (principal, action, resource), conds) = flatten_tuple_4(maybe_effect, maybe_annotations, maybe_scope, maybe_conds)?; @@ -371,16 +387,18 @@ impl Node> { ) .into()), // The source failed to parse completely. If the parse errors include - // `SlotsInConditionClause` also add an `ExpectedStaticPolicy` error. + // `SlotsNotInScopeInConditionClause` also add an `ExpectedStaticPolicy` error. Err(mut errs) => { let new_errs = errs .iter() .filter_map(|err| match err { ParseError::ToAST(err) => match err.kind() { - ToASTErrorKind::SlotsInConditionClause(inner) => Some(ToASTError::new( - ToASTErrorKind::expected_static_policy(inner.slot.clone()), - err.source_loc().into_maybe_loc(), - )), + ToASTErrorKind::SlotsNotInScopeInConditionClause(inner) => { + Some(ToASTError::new( + ToASTErrorKind::expected_static_policy(inner.slot.clone()), + err.source_loc().into_maybe_loc(), + )) + } _ => None, }, _ => None, @@ -418,23 +436,37 @@ impl Node> { let maybe_scope = policy.extract_scope_tolerant_ast(); // convert conditions - let maybe_conds = ParseErrors::transpose(policy.conds.iter().map(|c| { - let (e, is_when) = c.to_expr::>()?; - let slot_errs = e.slots().map(|slot| { - ToASTError::new( - ToASTErrorKind::slots_in_condition_clause( - slot.clone(), - if is_when { "when" } else { "unless" }, - ), - slot.loc.or_else(|| c.loc.clone()), - ) - .into() - }); - match ParseErrors::from_iter(slot_errs) { - Some(errs) => Err(errs), - None => Ok(e), + let maybe_conds = match policy.extract_scope_tolerant_ast() { + Ok((p, _, r)) => { + let slots_in_scope: HashSet = + HashSet::from_iter(p.as_expr().slots().chain(r.as_expr().slots())); + ParseErrors::transpose(policy.conds.iter().map(|c| { + let (e, is_when) = c.to_expr::>()?; + let slot_errs = + e.slots() + .filter(|slot| !slots_in_scope.contains(slot)) + .map(|slot| { + ToASTError::new( + ToASTErrorKind::slots_not_in_scope_in_condition_clause( + slot.clone(), + if is_when { "when" } else { "unless" }, + ), + slot.loc, + ) + .into() + }); + + match ParseErrors::from_iter(slot_errs) { + Some(errs) => Err(errs), + None => Ok(e), + } + })) } - })); + Err(_) => ParseErrors::transpose(policy.conds.iter().map(|c| { + let (e, _) = c.to_expr::>()?; + Ok(e) + })), + }; let (effect, annotations, (principal, action, resource), conds) = flatten_tuple_4(maybe_effect, maybe_annotations, maybe_scope, maybe_conds)?; @@ -4805,6 +4837,94 @@ mod tests { } } + #[test] + fn template_slot_in_condition() { + let src = r#"permit(principal == ?principal, action == Action::"action", resource in ?resource) when {?principal.name == true && ?resource.valid == 5};"#; + text_to_cst::parse_policy(src) + .expect("parse_error") + .to_template(ast::PolicyID::from_string("i0")) + .unwrap_or_else(|errs| { + panic!( + "Failed to create a policy template: {:?}", + miette::Report::new(errs) + ); + }); + } + + #[test] + fn template_slot_not_in_scope_in_condition_1() { + let src = + r#"permit(principal, action == Action::"action", resource) when {?principal.valid};"#; + let errs = text_to_cst::parse_policy(src) + .expect("parse_error") + .to_template(ast::PolicyID::from_string("i0")) + .unwrap_err(); + + expect_n_errors(src, &errs, 1); + expect_some_error_matches( + src, + &errs, + &ExpectedErrorMessageBuilder::error("found template slot ?principal in a `when` clause") + .help("?principal needs to appear in the scope to appear in the condition of the template") + .exactly_one_underline("?principal") + .build(), + ); + } + + #[test] + fn template_slot_not_in_scope_in_condition_2() { + let src = r#"forbid(principal, action == Action::"action", resource) unless {?resource.storage == 5};"#; + let errs = text_to_cst::parse_policy(src) + .expect("parse_error") + .to_template(ast::PolicyID::from_string("i0")) + .unwrap_err(); + + expect_n_errors(src, &errs, 1); + expect_some_error_matches( + src, + &errs, + &ExpectedErrorMessageBuilder::error( + "found template slot ?resource in a `unless` clause", + ) + .help( + "?resource needs to appear in the scope to appear in the condition of the template", + ) + .exactly_one_underline("?resource") + .build(), + ); + } + + #[test] + fn template_slot_not_in_scope_in_condition_3() { + let src = r#"permit(principal, action == Action::"action", resource) unless {?principal.valid && ?resource.storage == 5};"#; + let errs = text_to_cst::parse_policy(src) + .expect("parse_error") + .to_template(ast::PolicyID::from_string("i0")) + .unwrap_err(); + + expect_n_errors(src, &errs, 2); + expect_some_error_matches( + src, + &errs, + &ExpectedErrorMessageBuilder::error("found template slot ?principal in a `unless` clause") + .help("?principal needs to appear in the scope to appear in the condition of the template") + .exactly_one_underline("?principal") + .build(), + ); + expect_some_error_matches( + src, + &errs, + &ExpectedErrorMessageBuilder::error( + "found template slot ?resource in a `unless` clause", + ) + .help( + "?resource needs to appear in the scope to appear in the condition of the template", + ) + .exactly_one_underline("?resource") + .build(), + ); + } + #[test] fn missing_scope_constraint() { let p_src = "permit();"; diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 00dd3a9a69..619e6f829e 100644 --- a/cedar-policy-core/src/parser/err.rs +++ b/cedar-policy-core/src/parser/err.rs @@ -152,7 +152,7 @@ pub enum ToASTErrorKind { /// This is not currently supported; see [RFC 3](https://github.com/cedar-policy/rfcs/pull/3). #[error(transparent)] #[diagnostic(transparent)] - SlotsInConditionClause(#[from] parse_errors::SlotsInConditionClause), + SlotsNotInScopeInConditionClause(#[from] parse_errors::SlotsNotInScopeInConditionClause), /// Returned when a policy is missing one of the three required scope elements /// (`principal`, `action`, and `resource`) #[error("this policy is missing the `{0}` variable in the scope")] @@ -455,9 +455,12 @@ impl ToASTErrorKind { } } - /// Constructor for the [`ToASTErrorKind::SlotsInConditionClause`] error - pub fn slots_in_condition_clause(slot: ast::Slot, clause_type: &'static str) -> Self { - parse_errors::SlotsInConditionClause { slot, clause_type }.into() + /// Constructor for the [`ToASTErrorKind::SlotsNotInScopeInConditionClause`] error + pub fn slots_not_in_scope_in_condition_clause( + slot: ast::Slot, + clause_type: &'static str, + ) -> Self { + parse_errors::SlotsNotInScopeInConditionClause { slot, clause_type }.into() } /// Constructor for the [`ToASTErrorKind::ExpectedStaticPolicy`] error @@ -546,11 +549,11 @@ pub mod parse_errors { } } - /// Details about a `SlotsInConditionClause` error. + /// Details about a `SlotsNotInScopeInConditionClause` error. #[derive(Debug, Clone, Diagnostic, Error, PartialEq, Eq)] #[error("found template slot {} in a `{clause_type}` clause", slot.id)] - #[diagnostic(help("slots are currently unsupported in `{clause_type}` clauses"))] - pub struct SlotsInConditionClause { + #[diagnostic(help("{} needs to appear in the scope to appear in the condition of the template", slot.id))] + pub struct SlotsNotInScopeInConditionClause { /// Slot that was found in a when/unless clause pub(crate) slot: ast::Slot, /// Clause type, e.g. "when" or "unless" diff --git a/cedar-policy-core/src/validator/typecheck/test/policy.rs b/cedar-policy-core/src/validator/typecheck/test/policy.rs index 8e95ea2a63..9dd7e5fe11 100644 --- a/cedar-policy-core/src/validator/typecheck/test/policy.rs +++ b/cedar-policy-core/src/validator/typecheck/test/policy.rs @@ -107,6 +107,118 @@ fn simple_schema_file() -> json_schema::NamespaceDefinition { .expect("Expected valid schema") } +fn simple_schema_file_1() -> json_schema::NamespaceDefinition { + serde_json::from_value(serde_json::json!( + { + "entityTypes": { + "Disk": { + "shape": { + "type": "Record", + "attributes": { + "name": { + "type": "EntityOrCommon", + "name": "String" + }, + "storage": { + "type": "EntityOrCommon", + "name": "Long" + } + } + } + }, + "Document": { + "memberOfTypes": [ + "Folder" + ], + "shape": { + "type": "Record", + "attributes": { + "lastEdited": { + "type": "EntityOrCommon", + "name": "datetime" + } + } + } + }, + "File": { + "memberOfTypes": [ + "Document" + ] + }, + "Folder": { + "memberOfTypes": [ + "Disk" + ], + "shape": { + "type": "Record", + "attributes": { + "lastEdited": { + "type": "EntityOrCommon", + "name": "datetime" + } + } + } + }, + "Person": { + "shape": { + "type": "Record", + "attributes": { + "age": { + "type": "EntityOrCommon", + "name": "Long" + } + } + } + } + }, + "actions": { + "Access": { + "appliesTo": { + "principalTypes": [ + "Person" + ], + "resourceTypes": [ + "Disk" + ] + } + }, + "Navigate": { + "appliesTo": { + "principalTypes": [ + "Person" + ], + "resourceTypes": [ + "Disk", + "Folder", + "Document" + ] + } + }, + "Write": { + "appliesTo": { + "principalTypes": [ + "Person" + ], + "resourceTypes": [ + "Folder", + "Document" + ], + "context": { + "type": "Record", + "attributes": { + "date": { + "type": "EntityOrCommon", + "name": "datetime" + } + } + } + } + } + } + })) + .expect("Expected valid schema") +} + #[track_caller] // report the caller's location as the location of the panic, not the location in this function fn assert_policy_typechecks_permissive_simple_schema(p: impl Into>) { assert_policy_typechecks_for_mode(simple_schema_file(), p, ValidationMode::Permissive) @@ -1326,4 +1438,36 @@ mod templates { ) ); } + + #[test] + fn slot_in_condition_should_typecheck() { + let s = simple_schema_file_1(); + + let templates = [ + r#"permit(principal == ?principal, action == Action::"Navigate", resource in ?resource) when { ?resource has storage && ?resource.storage == 5 };"#, + r#"permit(principal == ?principal, action == Action::"Navigate", resource in ?resource) when { ?principal.age == 25 };"#, + r#"permit(principal == ?principal, action == Action::"Write", resource == ?resource) when { ?resource.lastEdited > context.date };"#, + r#"permit(principal == ?principal, action == Action::"Access", resource in ?resource) when { ?resource.name == "SSD" };"#, + ]; + + for template in templates { + let t = parse_policy_or_template(None, template).unwrap(); + assert_policy_typechecks(s.clone(), t); + } + } + + #[test] + fn slot_in_condition_should_not_typecheck() { + let s = simple_schema_file_1(); + + let templates = [ + r#"permit(principal == ?principal, action == Action::"Navigate", resource in ?resource) when { ?resource.storage == 5 };"#, + r#"permit(principal == ?principal, action == Action::"Write", resource in ?resource) when { ?resource.random == true };"#, + ]; + + for template in templates { + let t = parse_policy_or_template(None, template).unwrap(); + assert_policy_typecheck_fails(s.clone(), t); + } + } } diff --git a/cedar-policy/src/test/test.rs b/cedar-policy/src/test/test.rs index 7f88c55f20..a6f829d1bf 100644 --- a/cedar-policy/src/test/test.rs +++ b/cedar-policy/src/test/test.rs @@ -5422,7 +5422,7 @@ mod issue_606 { &miette::Report::new(e), &ExpectedErrorMessageBuilder::error("error deserializing a policy/template from JSON") .source("found template slot ?principal in a `when` clause") - .help("slots are currently unsupported in `when` clauses") + .help("?principal needs to appear in the scope to appear in the condition of the template") .build(), ); }); @@ -7303,11 +7303,6 @@ mod policy_manipulation_functions_tests { r#"permit(principal, action, resource) when { User::"Bob".isEmpty() };"#, mapping.clone(), ); - assert_entity_sub( - r#"permit(principal, action, resource) when { User::"Alice".isEmpty() };"#, - r#"permit(principal, action, resource) when { User::"Bob".isEmpty() };"#, - mapping.clone(), - ); assert_entity_sub( r#"permit(principal, action, resource) when { User::"Alice".getTag(User::"Alice") };"#, r#"permit(principal, action, resource) when { User::"Bob".getTag(User::"Bob") };"#,