From 3149180b9e2f8ca5f34802b850881c3bfe37c857 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 29 Jul 2025 09:59:01 -0400 Subject: [PATCH 01/39] slots in the condition of template Signed-off-by: Alan Wang --- cedar-policy-core/src/est.rs | 31 ++++++---- cedar-policy-core/src/est/err.rs | 2 +- cedar-policy-core/src/parser/cst_to_ast.rs | 71 ++++++++++++++-------- cedar-policy-core/src/parser/err.rs | 14 ++--- 4 files changed, 72 insertions(+), 46 deletions(-) diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index fbfd55f52f..82afc331b2 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -280,10 +280,13 @@ impl Policy { id: Option, ) -> Result { let id = id.unwrap_or_else(|| ast::PolicyID::from_string("JSON policy")); + let has_principal: bool = self.principal.has_slot(); + let has_resource = 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, has_resource, &id)); let conditions = match conditions_iter.next() { None => ast::Expr::val(true), Some(first) => ast::ExprBuilder::with_data(()) @@ -312,24 +315,26 @@ 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: bool, has_resource: bool, is_when: bool) -> Result { + for slot in e.slots() { + if (slot.id.is_principal() && !has_principal) || (slot.id.is_resource() && !has_resource) { + 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 { + fn try_into_ast(self, has_principal: bool, has_resource: bool, id: &ast::PolicyID) -> Result { match self { - Clause::When(expr) => Self::filter_slots(expr.try_into_ast(id)?, true), + Clause::When(expr) => Self::filter_slots(expr.try_into_ast(id)?, has_principal, has_resource, true), Clause::Unless(expr) => { - Self::filter_slots(ast::Expr::not(expr.try_into_ast(id)?), false) + Self::filter_slots(ast::Expr::not(expr.try_into_ast(id)?), has_principal, has_resource, false) } } } diff --git a/cedar-policy-core/src/est/err.rs b/cedar-policy-core/src/est/err.rs index 2a6af4ff87..fa8a72605a 100644 --- a/cedar-policy-core/src/est/err.rs +++ b/cedar-policy-core/src/est/err.rs @@ -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, diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index dfb27b6edb..ea7d6dad48 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -270,13 +270,13 @@ 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::SlotsNotInScopeInConditionClause(inner) => Some(ToASTError::new( ToASTErrorKind::expected_static_policy(inner.slot.clone()), err.source_loc().into_maybe_loc(), )), @@ -322,17 +322,27 @@ impl Node> { // convert conditions let maybe_conds = ParseErrors::transpose(policy.conds.iter().map(|c| { let (e, is_when) = c.to_expr::>()?; + let (p, _, r) = policy.extract_scope()?; + let slots_in_scope: HashSet = + HashSet::from_iter(p.as_expr().slots().chain(r.as_expr().slots())); + + // slots that are in the condition but not in the scope + let slot_errs = e + .slots() // Chore: change this with generalized 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.clone(), + ) + .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), @@ -371,13 +381,13 @@ 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::SlotsNotInScopeInConditionClause(inner) => Some(ToASTError::new( ToASTErrorKind::expected_static_policy(inner.slot.clone()), err.source_loc().into_maybe_loc(), )), @@ -419,17 +429,28 @@ impl Node> { // 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() - }); + let (e, is_when) = c.to_expr::>()?; + let (p, _, r) = policy.extract_scope()?; + let slots_in_scope: HashSet = + HashSet::from_iter(p.as_expr().slots().chain(r.as_expr().slots())); + + // slots that are in the condition but not in the scope + let slot_errs = e + .slots() // Chore: change this with generalized 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.clone(), + ) + .into() + }); + match ParseErrors::from_iter(slot_errs) { Some(errs) => Err(errs), None => Ok(e), diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 00dd3a9a69..6936239825 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,9 @@ 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 +546,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" From 4934e516578f3b9dde918a86040122bafade2c6e Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 29 Jul 2025 11:05:34 -0400 Subject: [PATCH 02/39] fixed + added testcases Signed-off-by: Alan Wang --- cedar-policy-core/src/ast/policy.rs | 2 +- cedar-policy-core/src/est.rs | 64 ++++- cedar-policy-core/src/parser.rs | 39 ++-- cedar-policy-core/src/parser/cst_to_ast.rs | 218 +++++++++++++----- cedar-policy-core/src/parser/err.rs | 5 +- .../src/validator/typecheck/test/policy.rs | 144 ++++++++++++ cedar-policy/src/test/test.rs | 2 +- 7 files changed, 386 insertions(+), 88 deletions(-) diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 191d66079e..ca88470700 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -2365,7 +2365,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 82afc331b2..5377ac310f 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -282,7 +282,7 @@ impl Policy { let id = id.unwrap_or_else(|| ast::PolicyID::from_string("JSON policy")); let has_principal: bool = self.principal.has_slot(); let has_resource = self.resource.has_slot(); - + let mut conditions_iter = self .conditions .into_iter() @@ -315,9 +315,16 @@ impl Policy { } impl Clause { - fn filter_slots(e: ast::Expr, has_principal: bool, has_resource: bool, is_when: bool) -> Result { + fn filter_slots( + e: ast::Expr, + has_principal: bool, + has_resource: bool, + is_when: bool, + ) -> Result { for slot in e.slots() { - if (slot.id.is_principal() && !has_principal) || (slot.id.is_resource() && !has_resource) { + if (slot.id.is_principal() && !has_principal) + || (slot.id.is_resource() && !has_resource) + { return Err(FromJsonError::SlotsNotInScopeInConditionClause( parse_errors::SlotsNotInScopeInConditionClause { slot, @@ -330,12 +337,22 @@ impl Clause { } /// `id` is the ID of the policy the clause belongs to, used only for reporting errors - fn try_into_ast(self, has_principal: bool, has_resource: bool, id: &ast::PolicyID) -> Result { + fn try_into_ast( + self, + has_principal: bool, + has_resource: bool, + id: &ast::PolicyID, + ) -> Result { match self { - Clause::When(expr) => Self::filter_slots(expr.try_into_ast(id)?, has_principal, has_resource, true), - Clause::Unless(expr) => { - Self::filter_slots(ast::Expr::not(expr.try_into_ast(id)?), has_principal, has_resource, false) + Clause::When(expr) => { + Self::filter_slots(expr.try_into_ast(id)?, has_principal, has_resource, true) } + Clause::Unless(expr) => Self::filter_slots( + ast::Expr::not(expr.try_into_ast(id)?), + has_principal, + has_resource, + false, + ), } } } @@ -3580,6 +3597,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] 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 ea7d6dad48..ba80027413 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -276,10 +276,12 @@ impl Node> { .iter() .filter_map(|err| match err { ParseError::ToAST(err) => match err.kind() { - ToASTErrorKind::SlotsNotInScopeInConditionClause(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,34 +322,37 @@ 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 (p, _, r) = policy.extract_scope()?; - let slots_in_scope: HashSet = - HashSet::from_iter(p.as_expr().slots().chain(r.as_expr().slots())); - - // slots that are in the condition but not in the scope - let slot_errs = e - .slots() // Chore: change this with generalized 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.clone(), - ) - .into() - }); + let maybe_conds = match policy.extract_scope() { + 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), + 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)?; @@ -387,10 +392,12 @@ impl Node> { .iter() .filter_map(|err| match err { ParseError::ToAST(err) => match err.kind() { - ToASTErrorKind::SlotsNotInScopeInConditionClause(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, @@ -428,34 +435,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 (p, _, r) = policy.extract_scope()?; - let slots_in_scope: HashSet = - HashSet::from_iter(p.as_expr().slots().chain(r.as_expr().slots())); - - // slots that are in the condition but not in the scope - let slot_errs = e - .slots() // Chore: change this with generalized 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.clone(), - ) - .into() - }); + 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), + 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)?; @@ -4826,6 +4836,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 6936239825..619e6f829e 100644 --- a/cedar-policy-core/src/parser/err.rs +++ b/cedar-policy-core/src/parser/err.rs @@ -456,7 +456,10 @@ impl ToASTErrorKind { } /// Constructor for the [`ToASTErrorKind::SlotsNotInScopeInConditionClause`] error - pub fn slots_not_in_scope_in_condition_clause(slot: ast::Slot, clause_type: &'static str) -> Self { + pub fn slots_not_in_scope_in_condition_clause( + slot: ast::Slot, + clause_type: &'static str, + ) -> Self { parse_errors::SlotsNotInScopeInConditionClause { slot, clause_type }.into() } 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..f50b29eda5 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(), ); }); From e3a96b561932e06663f9bdc85073a585f2f438de Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 29 Jul 2025 16:15:53 -0400 Subject: [PATCH 03/39] improved comments Signed-off-by: Alan Wang --- cedar-policy-core/src/est.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index 5377ac310f..b7c50e2ae6 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -280,7 +280,7 @@ impl Policy { id: Option, ) -> Result { let id = id.unwrap_or_else(|| ast::PolicyID::from_string("JSON policy")); - let has_principal: bool = self.principal.has_slot(); + let has_principal = self.principal.has_slot(); let has_resource = self.resource.has_slot(); let mut conditions_iter = self @@ -337,6 +337,9 @@ impl Clause { } /// `id` is the ID of the policy the clause belongs to, used only for reporting errors + /// has_principal/has_resource 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: bool, From b0e6683aa878a0c41359c6ba0f68026eea15a208 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 29 Jul 2025 17:19:37 -0400 Subject: [PATCH 04/39] fmt Signed-off-by: Alan Wang --- cedar-policy-core/src/est.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index b7c50e2ae6..a30934e54e 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -338,8 +338,8 @@ impl Clause { /// `id` is the ID of the policy the clause belongs to, used only for reporting errors /// has_principal/has_resource 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 + /// 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: bool, From 41804ab2f304ef343d5c9d581818f1f35ac8c24a Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 30 Jul 2025 12:53:51 -0400 Subject: [PATCH 05/39] added has_slot for est & refactored Signed-off-by: Alan Wang --- cedar-policy-core/src/ast/policy.rs | 24 +++++++ cedar-policy-core/src/est.rs | 79 +++++++++++++++++----- cedar-policy-core/src/est/expr.rs | 70 +++++++++++++++++++ cedar-policy-core/src/parser/cst_to_ast.rs | 3 +- 4 files changed, 158 insertions(+), 18 deletions(-) diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index ca88470700..24940e4a8d 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -1384,6 +1384,18 @@ 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)) => Some(Slot { + id: SlotId::principal(), + loc: l, + }), + _ => None, + } + } } impl std::fmt::Display for PrincipalConstraint { @@ -1491,6 +1503,18 @@ 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)) => Some(Slot { + id: SlotId::resource(), + loc: l, + }), + _ => None, + } + } } impl std::fmt::Display for ResourceConstraint { diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index a30934e54e..337eb79ae8 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -157,8 +157,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,13 +282,13 @@ impl Policy { id: Option, ) -> Result { let id = id.unwrap_or_else(|| ast::PolicyID::from_string("JSON policy")); - let has_principal = self.principal.has_slot(); - let has_resource = self.resource.has_slot(); + 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(has_principal, has_resource, &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(()) @@ -317,13 +319,13 @@ impl Policy { impl Clause { fn filter_slots( e: ast::Expr, - has_principal: bool, - has_resource: bool, + 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.id.is_resource() && !has_resource) + if (slot.id.is_principal() && !has_principal_slot) + || (slot.id.is_resource() && !has_resource_slot) { return Err(FromJsonError::SlotsNotInScopeInConditionClause( parse_errors::SlotsNotInScopeInConditionClause { @@ -337,23 +339,26 @@ impl Clause { } /// `id` is the ID of the policy the clause belongs to, used only for reporting errors - /// has_principal/has_resource tells us whether there is a principal/resource slot in the scope + /// 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: bool, - has_resource: bool, + 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)?, has_principal, has_resource, true) - } + 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, - has_resource, + has_principal_slot, + has_resource_slot, false, ), } @@ -4747,6 +4752,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/expr.rs b/cedar-policy-core/src/est/expr.rs index 9dc89c3c30..56d8950bca 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -1060,6 +1060,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/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index ba80027413..a0aba03672 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -325,7 +325,8 @@ impl Node> { let maybe_conds = match policy.extract_scope() { Ok((p, _, r)) => { let slots_in_scope: HashSet = - HashSet::from_iter(p.as_expr().slots().chain(r.as_expr().slots())); + 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 = From 4a7e2f5b156c2a1b086c82de71fef5184e7a668d Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 30 Jul 2025 14:02:43 -0400 Subject: [PATCH 06/39] added missing match arm Signed-off-by: Alan Wang --- cedar-policy-core/src/ast/policy.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 24940e4a8d..0740725cb9 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -1389,7 +1389,8 @@ impl PrincipalConstraint { pub fn get_slot(self) -> Option { match self.constraint { PrincipalOrResourceConstraint::Eq(EntityReference::Slot(l)) - | PrincipalOrResourceConstraint::In(EntityReference::Slot(l)) => Some(Slot { + | PrincipalOrResourceConstraint::In(EntityReference::Slot(l)) + | PrincipalOrResourceConstraint::IsIn(_, EntityReference::Slot(l)) => Some(Slot { id: SlotId::principal(), loc: l, }), @@ -1508,7 +1509,8 @@ impl ResourceConstraint { pub fn get_slot(self) -> Option { match self.constraint { PrincipalOrResourceConstraint::Eq(EntityReference::Slot(l)) - | PrincipalOrResourceConstraint::In(EntityReference::Slot(l)) => Some(Slot { + | PrincipalOrResourceConstraint::In(EntityReference::Slot(l)) + | PrincipalOrResourceConstraint::IsIn(_, EntityReference::Slot(l)) => Some(Slot { id: SlotId::resource(), loc: l, }), From d2967dedcf2b769ed2da5673e39b841a3d133235 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 30 Jul 2025 16:29:35 -0400 Subject: [PATCH 07/39] added linking with slots in the condition for the EST data structure Signed-off-by: Alan Wang --- cedar-policy-core/src/est.rs | 90 +++++++++++++++- cedar-policy-core/src/est/err.rs | 14 ++- cedar-policy-core/src/est/expr.rs | 171 +++++++++++++++++++++++++++++- 3 files changed, 269 insertions(+), 6 deletions(-) diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index 337eb79ae8..98de201e23 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 @@ -3350,6 +3352,88 @@ 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 eid_with_nulls() { let policy = r#" diff --git a/cedar-policy-core/src/est/err.rs b/cedar-policy-core/src/est/err.rs index fa8a72605a..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; @@ -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 56d8950bca..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 { From a0436e771178062c687f0fe8c1ad93c3efc2430d Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 30 Jul 2025 17:14:16 -0400 Subject: [PATCH 08/39] first step Signed-off-by: Alan Wang --- cedar-language-server/src/schema/fold.rs | 2 +- cedar-language-server/src/schema/symbols.rs | 2 +- cedar-policy-core/src/ast.rs | 2 + .../src/ast/generalized_slots_annotation.rs | 122 ++++++++++++++++++ .../src/validator/json_schema.rs | 8 +- cedar-policy-core/src/validator/schema.rs | 75 +++++++++-- 6 files changed, 197 insertions(+), 14 deletions(-) create mode 100644 cedar-policy-core/src/ast/generalized_slots_annotation.rs diff --git a/cedar-language-server/src/schema/fold.rs b/cedar-language-server/src/schema/fold.rs index 29bdde9f59..671ab8fb5d 100644 --- a/cedar-language-server/src/schema/fold.rs +++ b/cedar-language-server/src/schema/fold.rs @@ -80,7 +80,7 @@ pub(crate) fn fold_schema(schema_info: &SchemaInfo) -> Option> .filter_map(|et| et.loc.as_loc_ref()); let action_locs = validator.action_ids().filter_map(|a| a.loc()); let common_types = validator - .common_types() + .common_types_extended() .filter_map(|ct| ct.type_loc.as_loc_ref()); // Combine all locations and create folding ranges diff --git a/cedar-language-server/src/schema/symbols.rs b/cedar-language-server/src/schema/symbols.rs index b0be0e6cc2..602f533746 100644 --- a/cedar-language-server/src/schema/symbols.rs +++ b/cedar-language-server/src/schema/symbols.rs @@ -125,7 +125,7 @@ pub(crate) fn schema_symbols(schema_info: &SchemaInfo) -> Option = validator - .common_types() + .common_types_extended() .filter_map(|ct| { ct.name_loc .as_ref() diff --git a/cedar-policy-core/src/ast.rs b/cedar-policy-core/src/ast.rs index e8367a647d..020c32f31b 100644 --- a/cedar-policy-core/src/ast.rs +++ b/cedar-policy-core/src/ast.rs @@ -54,5 +54,7 @@ mod expr_iterator; pub use expr_iterator::*; mod annotation; pub use annotation::*; +mod generalized_slots_annotation; +pub use generalized_slots_annotation::*; mod expr_visitor; pub use expr_visitor::*; diff --git a/cedar-policy-core/src/ast/generalized_slots_annotation.rs b/cedar-policy-core/src/ast/generalized_slots_annotation.rs new file mode 100644 index 0000000000..1295052839 --- /dev/null +++ b/cedar-policy-core/src/ast/generalized_slots_annotation.rs @@ -0,0 +1,122 @@ +/* + * Copyright Cedar Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +use std::collections::BTreeMap; + +use crate::ast::SlotId; +use crate::extensions::Extensions; +use crate::validator::{ + json_schema::Type as JSONSchemaType, types::Type as ValidatorType, RawName, SchemaError, + ValidatorSchema, +}; +use serde::{Deserialize, Serialize}; + +/// Struct storing the pairs of SlotId's and their corresponding type +#[derive(Clone, Eq, PartialEq, PartialOrd, Ord, Debug, Hash, Serialize, Deserialize)] +pub struct GeneralizedSlotsAnnotation(BTreeMap>); + +impl GeneralizedSlotsAnnotation { + /// Create a new empty `GeneralizedSlotsAnnotation` (with no slots) + pub fn new() -> Self { + Self(BTreeMap::new()) + } + + /// Get the type of the slot by key + pub fn get(&self, key: &SlotId) -> Option<&JSONSchemaType> { + self.0.get(key) + } + + /// Iterate over all pairs of slots and their types + pub fn iter(&self) -> impl Iterator)> { + self.0.iter() + } + + /// Tell if it's empty + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + /// Converts the types of generalized slots annotation to + /// use validator types so that they can be used by the typechecker + pub fn into_validator_generalized_slots_annotation( + self, + schema: &ValidatorSchema, + ) -> Result { + let validator_generalized_slots_annotation: Result, SchemaError> = self + .0 + .into_iter() + .map(|(k, ty)| -> Result<_, SchemaError> { + Ok(( + k, + schema.json_schema_type_to_validator_type(ty, Extensions::all_available())?, + )) + }) + .collect(); + Ok(validator_generalized_slots_annotation?.into()) + } +} + +impl Default for GeneralizedSlotsAnnotation { + fn default() -> Self { + Self::new() + } +} + +impl FromIterator<(SlotId, JSONSchemaType)> for GeneralizedSlotsAnnotation { + fn from_iter)>>(iter: T) -> Self { + Self(BTreeMap::from_iter(iter)) + } +} + +impl From>> for GeneralizedSlotsAnnotation { + fn from(value: BTreeMap>) -> Self { + Self(value) + } +} + +/// Struct storing the pairs of SlotId's and their corresponding validator types +#[derive(Clone, Eq, PartialEq, PartialOrd, Ord, Debug, Hash)] +pub struct ValidatorGeneralizedSlotsAnnotation(BTreeMap); + +impl FromIterator<(SlotId, ValidatorType)> for ValidatorGeneralizedSlotsAnnotation { + fn from_iter>(iter: T) -> Self { + Self(BTreeMap::from_iter(iter)) + } +} + +impl From> for ValidatorGeneralizedSlotsAnnotation { + fn from(value: BTreeMap) -> Self { + Self(value) + } +} + +impl Default for ValidatorGeneralizedSlotsAnnotation { + fn default() -> Self { + Self::new() + } +} + +impl ValidatorGeneralizedSlotsAnnotation { + /// Create a new empty `ValidatorGeneralizedSlotsAnnotation` (with no slots) + pub fn new() -> Self { + Self(BTreeMap::new()) + } + + /// Get the validator type of the slot by key + pub fn get(&self, slot: &SlotId) -> Option<&ValidatorType> { + self.0.get(slot) + } +} diff --git a/cedar-policy-core/src/validator/json_schema.rs b/cedar-policy-core/src/validator/json_schema.rs index 5a2c34ab21..2a25e329dc 100644 --- a/cedar-policy-core/src/validator/json_schema.rs +++ b/cedar-policy-core/src/validator/json_schema.rs @@ -1207,7 +1207,7 @@ impl From for ActionEntityUID { /// this [`Type`], including recursively. /// See notes on [`Fragment`]. #[derive(Educe, Debug, Clone, Serialize)] -#[educe(PartialEq(bound(N: PartialEq)), Eq, PartialOrd, Ord(bound(N: Ord)))] +#[educe(PartialEq(bound(N: PartialEq)), Eq, PartialOrd, Ord(bound(N: Ord)), Hash(bound(N: Hash)))] // This enum is `untagged` with these variants as a workaround to a serde // limitation. It is not possible to have the known variants on one enum, and // then, have catch-all variant for any unrecognized tag in the same enum that @@ -1773,7 +1773,7 @@ impl From> for Type { /// this [`RecordType`], including recursively. /// See notes on [`Fragment`]. #[derive(Educe, Debug, Clone, Serialize, Deserialize)] -#[educe(PartialEq, Eq, PartialOrd, Ord)] +#[educe(PartialEq, Eq, PartialOrd, Ord, Hash(bound(N: Hash)))] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] #[serde(rename_all = "camelCase")] #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] @@ -1850,7 +1850,7 @@ impl RecordType { /// this [`TypeVariant`], including recursively. /// See notes on [`Fragment`]. #[derive(Educe, Debug, Clone, Serialize, Deserialize)] -#[educe(PartialEq(bound(N: PartialEq)), Eq, PartialOrd, Ord(bound(N: Ord)))] +#[educe(PartialEq(bound(N: PartialEq)), Eq, PartialOrd, Ord(bound(N: Ord)), Hash(bound(N: Hash)))] #[serde(tag = "type")] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] @@ -2130,7 +2130,7 @@ impl<'a> arbitrary::Arbitrary<'a> for Type { /// unknown fields for [`TypeOfAttribute`] should be passed to [`Type`] where /// they will be denied (``). #[derive(Educe, Debug, Clone, Serialize, Deserialize)] -#[educe(PartialEq, Eq, PartialOrd, Ord)] +#[educe(PartialEq, Eq, PartialOrd, Ord, Hash(bound(N: Hash)))] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] pub struct TypeOfAttribute { /// Underlying type of the attribute diff --git a/cedar-policy-core/src/validator/schema.rs b/cedar-policy-core/src/validator/schema.rs index a36f706f6f..981b7739d2 100644 --- a/cedar-policy-core/src/validator/schema.rs +++ b/cedar-policy-core/src/validator/schema.rs @@ -157,7 +157,8 @@ impl ValidatorSchemaFragment { #[derive(Clone, Debug, Educe)] #[educe(Eq, PartialEq)] pub struct ValidatorType { - ty: Type, + /// Type field + pub ty: Type, #[cfg(feature = "extended-schema")] loc: MaybeLoc, } @@ -235,6 +236,9 @@ pub struct ValidatorSchema { /// Map from action id names to the [`ValidatorActionId`] object. action_ids: HashMap, + /// Map from common types to the [`ValidatorType`] they represent. + common_types: HashMap, + /// For easy lookup, this is a map from action name to `Entity` object /// for each action in the schema. This information is contained elsewhere /// in the `ValidatorSchema`, but not efficient to extract -- getting the @@ -243,7 +247,7 @@ pub struct ValidatorSchema { pub(crate) actions: HashMap>, #[cfg(feature = "extended-schema")] - common_types: HashSet, + common_types_extended: HashSet, #[cfg(feature = "extended-schema")] namespaces: HashSet, } @@ -282,6 +286,7 @@ impl ValidatorSchema { pub fn new( entity_types: impl IntoIterator, action_ids: impl IntoIterator, + common_types: impl IntoIterator, ) -> Self { let entity_types = entity_types .into_iter() @@ -291,9 +296,11 @@ impl ValidatorSchema { .into_iter() .map(|id| (id.name().clone(), id)) .collect(); + let common_types = common_types.into_iter().collect(); Self::new_from_maps( entity_types, action_ids, + common_types, #[cfg(feature = "extended-schema")] HashSet::new(), #[cfg(feature = "extended-schema")] @@ -307,7 +314,8 @@ impl ValidatorSchema { fn new_from_maps( entity_types: HashMap, action_ids: HashMap, - #[cfg(feature = "extended-schema")] common_types: HashSet, + common_types: HashMap, + #[cfg(feature = "extended-schema")] common_types_extended: HashSet, #[cfg(feature = "extended-schema")] namespaces: HashSet, ) -> Self { let actions = Self::action_entities_iter(&action_ids) @@ -316,9 +324,10 @@ impl ValidatorSchema { Self { entity_types, action_ids, + common_types, actions, #[cfg(feature = "extended-schema")] - common_types, + common_types_extended, #[cfg(feature = "extended-schema")] namespaces, } @@ -326,8 +335,8 @@ impl ValidatorSchema { /// Returns an iter of common types in the schema #[cfg(feature = "extended-schema")] - pub fn common_types(&self) -> impl Iterator { - self.common_types.iter() + pub fn common_types_extended(&self) -> impl Iterator { + self.common_types_extended.iter() } /// Returns an iter of validator namespaces in the schema @@ -456,8 +465,9 @@ impl ValidatorSchema { entity_types: HashMap::new(), action_ids: HashMap::new(), actions: HashMap::new(), + common_types: HashMap::new(), #[cfg(feature = "extended-schema")] - common_types: HashSet::new(), + common_types_extended: HashSet::new(), #[cfg(feature = "extended-schema")] namespaces: HashSet::new(), } @@ -537,6 +547,39 @@ impl ValidatorSchema { ) } + /// Construct a [`Type`] from [`json_schema::Type`] + pub fn json_schema_type_to_validator_type( + &self, + ty: json_schema::Type, + extensions: &Extensions<'_>, + ) -> Result { + let ext_fragments = std::iter::once(cedar_fragment(extensions)).collect::>(); + + let mut all_defs = AllDefs::new(|| ext_fragments.iter()); + + for entity_type in self.entity_type_names() { + all_defs.mark_as_defined_as_entity_type(entity_type.name().qualify_with(None)); + } + + for common_type in self.common_type_internal_names() { + all_defs.mark_as_defined_as_common_type(common_type.clone()); + } + + let common_types = self + .common_types + .iter() + .map(|(name, ty)| (name, ty.clone())) + .collect(); + + let conditional_ty = ty.conditionally_qualify_type_references(None); + let internal_ty = conditional_ty.fully_qualify_type_references(&all_defs)?; + let unresolved = try_jsonschema_type_into_validator_type(internal_ty, extensions, None)?; + + unresolved + .resolve_common_type_refs(&common_types) + .map(|t| t.ty) + } + /// Construct a [`ValidatorSchema`] from some number of [`ValidatorSchemaFragment`]s. pub fn from_schema_fragments( fragments: impl IntoIterator>, @@ -820,6 +863,11 @@ impl ValidatorSchema { .map(|ct| ValidatorCommonType::new(ct.0, ct.1)) .collect(); + let common_types: HashMap = common_types + .into_iter() + .map(|ct| (ct.0.clone(), ct.1)) + .collect(); + // Return with an error if there is an undeclared entity or action // referenced in any fragment. `{entity,action}_children` are provided // for the `undeclared_parent_{entities,actions}` arguments because we @@ -832,11 +880,12 @@ impl ValidatorSchema { entity_children.into_keys(), &action_ids, action_children.into_keys(), - common_types.into_values(), + common_types.clone().into_values(), )?; Ok(ValidatorSchema::new_from_maps( entity_types, action_ids, + common_types, #[cfg(feature = "extended-schema")] common_type_validators, #[cfg(feature = "extended-schema")] @@ -985,6 +1034,16 @@ impl ValidatorSchema { self.action_ids.values() } + /// An iterator over the common type internal names in the schema. + pub fn common_type_internal_names(&self) -> impl Iterator { + self.common_types.keys() + } + + /// An iterator over the common types in the schema. + pub fn common_types(&self) -> impl Iterator { + self.common_types.iter() + } + /// An iterator over the entity type names in the schema. pub fn entity_type_names(&self) -> impl Iterator { self.entity_types.keys() From 85e8dd5cb6dfb787a1865a47644266761d228ad5 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Thu, 31 Jul 2025 11:45:29 -0400 Subject: [PATCH 09/39] adding a new field to Policy Signed-off-by: Alan Wang --- cedar-policy-core/src/ast/expr.rs | 4 +- cedar-policy-core/src/ast/expr_visitor.rs | 2 +- cedar-policy-core/src/ast/name.rs | 18 +- cedar-policy-core/src/ast/policy.rs | 393 +++++++++++++++++-- cedar-policy-core/src/ast/policy_set.rs | 92 ++++- cedar-policy-core/src/authorizer.rs | 4 +- cedar-policy-core/src/est.rs | 1 + cedar-policy-core/src/est/policy_set.rs | 9 +- cedar-policy-core/src/evaluator.rs | 7 +- cedar-policy-core/src/parser/cst_to_ast.rs | 7 +- cedar-policy-core/src/validator.rs | 12 + cedar-policy-core/src/validator/rbac.rs | 6 + cedar-policy-core/src/validator/typecheck.rs | 2 +- cedar-policy/src/api.rs | 12 +- 14 files changed, 509 insertions(+), 60 deletions(-) diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs index ee3c567ed8..e81c763c2b 100644 --- a/cedar-policy-core/src/ast/expr.rs +++ b/cedar-policy-core/src/ast/expr.rs @@ -293,7 +293,7 @@ impl Expr { self.subexpressions() .filter_map(|exp| match &exp.expr_kind { ExprKind::Slot(slotid) => Some(Slot { - id: *slotid, + id: slotid.clone(), loc: exp.source_loc().into_maybe_loc(), }), _ => None, @@ -1842,7 +1842,7 @@ mod test { let e = Expr::slot(SlotId::principal()); let p = SlotId::principal(); let r = SlotId::resource(); - let set: HashSet = HashSet::from_iter([p]); + let set: HashSet = HashSet::from_iter([p.clone()]); assert_eq!(set, e.slots().map(|slot| slot.id).collect::>()); let e = Expr::or( Expr::slot(SlotId::principal()), diff --git a/cedar-policy-core/src/ast/expr_visitor.rs b/cedar-policy-core/src/ast/expr_visitor.rs index ee6acd3f26..c5adb9b353 100644 --- a/cedar-policy-core/src/ast/expr_visitor.rs +++ b/cedar-policy-core/src/ast/expr_visitor.rs @@ -55,7 +55,7 @@ pub trait ExprVisitor { match expr.expr_kind() { ExprKind::Lit(lit) => self.visit_literal(lit, loc), ExprKind::Var(var) => self.visit_var(*var, loc), - ExprKind::Slot(slot) => self.visit_slot(*slot, loc), + ExprKind::Slot(slot) => self.visit_slot(slot.clone(), loc), ExprKind::Unknown(unknown) => self.visit_unknown(unknown, loc), ExprKind::If { test_expr, diff --git a/cedar-policy-core/src/ast/name.rs b/cedar-policy-core/src/ast/name.rs index 8482cf6e39..eb713995f9 100644 --- a/cedar-policy-core/src/ast/name.rs +++ b/cedar-policy-core/src/ast/name.rs @@ -283,7 +283,7 @@ impl<'de> Deserialize<'de> for InternalName { /// Clone is O(1). // This simply wraps a separate enum -- currently [`ValidSlotId`] -- in case we // want to generalize later -#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] #[serde(transparent)] pub struct SlotId(pub(crate) ValidSlotId); @@ -298,6 +298,11 @@ impl SlotId { Self(ValidSlotId::Resource) } + /// Create a `generalized slot` + pub fn generalized_slot(id: Id) -> Self { + Self(ValidSlotId::GeneralizedSlot(id)) + } + /// Check if a slot represents a principal pub fn is_principal(&self) -> bool { matches!(self, Self(ValidSlotId::Principal)) @@ -307,6 +312,11 @@ impl SlotId { pub fn is_resource(&self) -> bool { matches!(self, Self(ValidSlotId::Resource)) } + + /// Check if a slot represents a generalized slot + pub fn is_generalized_slot(&self) -> bool { + matches!(self, Self(ValidSlotId::GeneralizedSlot(_))) + } } impl From for SlotId { @@ -324,13 +334,14 @@ impl std::fmt::Display for SlotId { } } -/// Two possible variants for Slots -#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +/// Three possible variants for Slots +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub(crate) enum ValidSlotId { #[serde(rename = "?principal")] Principal, #[serde(rename = "?resource")] Resource, + GeneralizedSlot(Id), // Slots for generalized templates, for more info see [RFC 98](https://github.com/cedar-policy/rfcs/pull/98). } impl std::fmt::Display for ValidSlotId { @@ -338,6 +349,7 @@ impl std::fmt::Display for ValidSlotId { let s = match self { ValidSlotId::Principal => "principal", ValidSlotId::Resource => "resource", + ValidSlotId::GeneralizedSlot(id) => id.as_ref(), }; write!(f, "?{s}") } diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 0740725cb9..018101f17a 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -15,9 +15,16 @@ */ use crate::ast::*; +use crate::entities::{conformance::typecheck_restricted_expr_against_schematype, SchemaType}; +use crate::extensions::Extensions; use crate::parser::{AsLocRef, IntoMaybeLoc, Loc, MaybeLoc}; +use crate::validator::{ + err::SchemaError, json_schema::Type as JSONSchemaType, types::Type as ValidatorType, RawName, + ValidatorSchema, +}; use annotation::{Annotation, Annotations}; use educe::Educe; +use generalized_slots_annotation::GeneralizedSlotsAnnotation; use itertools::Itertools; use miette::Diagnostic; use nonempty::{nonempty, NonEmpty}; @@ -53,6 +60,9 @@ cfg_tolerant_ast! { static DEFAULT_ANNOTATIONS: std::sync::LazyLock> = std::sync::LazyLock::new(|| Arc::new(Annotations::default())); + static DEFAULT_GENERALIZED_SLOTS_ANNOTATION: std::sync::LazyLock> = + std::sync::LazyLock::new(|| Arc::new(GeneralizedSlotsAnnotation::default())); + static DEFAULT_PRINCIPAL_CONSTRAINT: std::sync::LazyLock = std::sync::LazyLock::new(PrincipalConstraint::any); @@ -120,6 +130,7 @@ impl Template { id: PolicyID, loc: MaybeLoc, annotations: Annotations, + generalized_slots_annotation: GeneralizedSlotsAnnotation, effect: Effect, principal_constraint: PrincipalConstraint, action_constraint: ActionConstraint, @@ -130,6 +141,7 @@ impl Template { id, loc, annotations, + generalized_slots_annotation, effect, principal_constraint, action_constraint, @@ -154,6 +166,7 @@ impl Template { id: PolicyID, loc: MaybeLoc, annotations: Arc, + generalized_slots_annotation: Arc, effect: Effect, principal_constraint: PrincipalConstraint, action_constraint: ActionConstraint, @@ -164,6 +177,7 @@ impl Template { id, loc, annotations, + generalized_slots_annotation, effect, principal_constraint, action_constraint, @@ -238,6 +252,18 @@ impl Template { self.body.annotations_arc() } + /// Get all generalized_slots_annotation data. + pub fn generalized_slots_annotation( + &self, + ) -> impl Iterator)> { + self.body.generalized_slots_annotation() + } + + /// Get [`Arc`] owning the generalized slots annotation data. + pub fn generalized_slots_annotation_arc(&self) -> &Arc { + self.body.generalized_slots_annotation_arc() + } + /// Get the condition expression of this template. /// /// This will be a conjunction of the template's scope constraints (on @@ -247,11 +273,18 @@ impl Template { self.body.condition() } - /// List of open slots in this template + /// List of open slots in this template including principal, resource, and generalized slots pub fn slots(&self) -> impl Iterator { self.slots.iter() } + /// List of principal and resource slots in this template + pub fn principal_resource_slots(&self) -> impl Iterator { + self.slots + .iter() + .filter(|slot| slot.id.is_principal() || slot.id.is_resource()) + } + /// Check if this template is a static policy /// /// Static policies can be linked without any slots, @@ -266,15 +299,33 @@ impl Template { pub fn check_binding( template: &Template, values: &HashMap, + generalized_values: &HashMap, ) -> Result<(), LinkingError> { // Verify all slots bound - let unbound = template + let unbound_values_and_generalized_values = template .slots .iter() - .filter(|slot| !values.contains_key(&slot.id)) + .filter(|slot| { + !values.contains_key(&slot.id) && !generalized_values.contains_key(&slot.id) + }) + .collect::>(); + + let extra_values = values + .iter() + .filter_map(|(slot, _)| { + if !template + .slots + .iter() + .any(|template_slot| template_slot.id == *slot) + { + Some(slot) + } else { + None + } + }) .collect::>(); - let extra = values + let extra_generalized_values = generalized_values .iter() .filter_map(|(slot, _)| { if !template @@ -289,16 +340,104 @@ impl Template { }) .collect::>(); - if unbound.is_empty() && extra.is_empty() { + let invalid_keys_in_values = values + .iter() + .filter_map(|(slot, _)| { + if *slot != (SlotId::principal()) && *slot != (SlotId::resource()) { + Some(slot) + } else { + None + } + }) + .collect::>(); + + let invalid_keys_in_generalized_values = generalized_values + .iter() + .filter_map(|(slot, _)| { + if *slot == (SlotId::principal()) || *slot == (SlotId::resource()) { + Some(slot) + } else { + None + } + }) + .collect::>(); + + if unbound_values_and_generalized_values.is_empty() + && extra_values.is_empty() + && extra_generalized_values.is_empty() + && invalid_keys_in_values.is_empty() + && invalid_keys_in_generalized_values.is_empty() + { Ok(()) + } else if !(invalid_keys_in_values.is_empty()) { + Err(LinkingError::from_invalid_env( + invalid_keys_in_values.into_iter().cloned(), + )) + } else if !(invalid_keys_in_generalized_values.is_empty()) { + Err(LinkingError::from_invalid_generalized_env( + invalid_keys_in_generalized_values.into_iter().cloned(), + )) } else { Err(LinkingError::from_unbound_and_extras( - unbound.into_iter().map(|slot| slot.id), - extra.into_iter().copied(), + unbound_values_and_generalized_values + .into_iter() + .map(|slot| slot.id.clone()), + extra_values + .into_iter() + .cloned() + .chain(extra_generalized_values.into_iter().cloned()), )) } } + /// Validates that the values provided for the generalized slots are of the types annotated + pub fn link_time_type_checking_with_schema( + template: &Template, + schema: &ValidatorSchema, + values: &HashMap, + generalized_values: &HashMap, + ) -> Result<(), LinkingError> { + let validator_generalized_slots_annotation = GeneralizedSlotsAnnotation::from_iter( + template + .generalized_slots_annotation() + .map(|(k, v)| (k.clone(), v.clone())), + ) + .into_validator_generalized_slots_annotation(schema)?; + + let values_restricted_expr: HashMap = values + .iter() + .map(|(slot, entity_uid)| (slot.clone(), RestrictedExpr::val(entity_uid.clone()))) + .collect(); + + for (slot, restricted_expr) in generalized_values + .iter() + .chain(values_restricted_expr.iter()) + { + let validator_type = validator_generalized_slots_annotation.get(slot).ok_or( + LinkingError::ArityError { + unbound_values: vec![slot.clone()], + extra_values: vec![], + }, + )?; + let borrowed_restricted_expr = restricted_expr.as_borrowed(); + #[allow(clippy::expect_used)] + let schema_ty = &SchemaType::try_from(validator_type.clone()) + .expect("This should never happen as expected_ty is a statically annotated type"); + let extensions = Extensions::all_available(); + typecheck_restricted_expr_against_schematype( + borrowed_restricted_expr, + schema_ty, + extensions, + ) + .map_err(|_| LinkingError::ValueProvidedForSlotIsNotOfTypeSpecified { + slot: slot.clone(), + value: restricted_expr.clone(), + ty: validator_type.clone(), + })? + } + Ok(()) + } + /// Attempt to create a template-linked policy from this template. /// This will fail if values for all open slots are not given. /// `new_instance_id` is the `PolicyId` for the created template-linked policy. @@ -306,10 +445,24 @@ impl Template { template: Arc