Allow principal resource slots in condition

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<Slot> {
+        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<Slot> {
+        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 {
@@ -2365,7 +2389,7 @@ mod test {
                 .exactly_one_underline("?principal")
                 .build()
             );
-            assert_eq!(e.len(), 2);
+            assert_eq!(e.len(), 1);
         });
     }
 

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,10 +282,13 @@ impl Policy {
         id: Option<ast::PolicyID>,
     ) -> Result<ast::Template, FromJsonError> {
         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 +317,50 @@ impl Policy {
 }
 
 impl Clause {
-    fn filter_slots(e: ast::Expr, is_when: bool) -> Result<ast::Expr, FromJsonError> {
-        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<ast::Expr, FromJsonError> {
+        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<ast::Expr, FromJsonError> {
+    /// 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<ast::Expr, FromJsonError> {
         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,
+            ),
         }
     }
 }
@@ -3575,6 +3605,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<ast::Policy, _> = 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 +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)]

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,

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<ast::Literal> for Expr {

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(