Fix typechecking ... in action (#462)

Author: john-h-kastner-aws

cedar-policy-validator/src/rbac.rs

@@ -383,12 +383,9 @@ impl Validator {
                 .into_iter(),
             ),
             // <var> in <literal euid>
-            PrincipalOrResourceConstraint::In(EntityReference::EUID(euid)) => Box::new(
-                self.schema
-                    .get_entity_types_in(euid.as_ref())
-                    .unwrap_or_default()
-                    .into_iter(),
-            ),
+            PrincipalOrResourceConstraint::In(EntityReference::EUID(euid)) => {
+                Box::new(self.schema.get_entity_types_in(euid.as_ref()).into_iter())
+            }
             PrincipalOrResourceConstraint::Eq(EntityReference::Slot)
             | PrincipalOrResourceConstraint::In(EntityReference::Slot) => {
                 Box::new(self.schema.known_entity_types())
@@ -406,7 +403,6 @@ impl Validator {
                 Box::new(
                     self.schema
                         .get_entity_types_in(in_entity.as_ref())
-                        .unwrap_or_default()
                         .into_iter()
                         .filter(move |k| &entity_type == k),
                 )

cedar-policy-validator/src/schema.rs

@@ -471,7 +471,18 @@ impl ValidatorSchema {
 
     /// Return true when the entity_type_id corresponds to a valid entity type.
     pub(crate) fn is_known_entity_type(&self, entity_type: &Name) -> bool {
-        self.entity_types.contains_key(entity_type)
+        is_action_entity_type(entity_type) || self.entity_types.contains_key(entity_type)
+    }
+
+    /// Return true when `euid` has an entity type declared by the schema. We
+    /// treat an Unspecified as "known" because it is always possible to declare
+    /// an action using an unspecified principal/resource type without first
+    /// declaring unspecified as an entity type in the entity types list.
+    pub(crate) fn euid_has_known_entity_type(&self, euid: &EntityUID) -> bool {
+        match euid.entity_type() {
+            EntityType::Specified(ety) => self.is_known_entity_type(ety),
+            EntityType::Unspecified => true,
+        }
     }
 
     /// An iterator over the action ids in the schema.
@@ -494,18 +505,18 @@ impl ValidatorSchema {
     /// includes all entity types that are descendants of the type of `entity`
     /// according  to the schema, and the type of `entity` itself because
     /// `entity in entity` evaluates to `true`.
-    pub(crate) fn get_entity_types_in<'a>(&'a self, entity: &'a EntityUID) -> Option<Vec<&Name>> {
-        let ety = match entity.entity_type() {
-            EntityType::Specified(ety) => Some(ety),
-            EntityType::Unspecified => None,
-        };
-
-        ety.and_then(|ety| self.get_entity_type(ety)).map(|ety| {
-            ety.descendants
-                .iter()
-                .chain(std::iter::once(&ety.name))
-                .collect()
-        })
+    pub(crate) fn get_entity_types_in<'a>(&'a self, entity: &'a EntityUID) -> Vec<&Name> {
+        match entity.entity_type() {
+            EntityType::Specified(ety) => {
+                let mut descendants = self
+                    .get_entity_type(ety)
+                    .map(|v_ety| v_ety.descendants.iter().collect::<Vec<_>>())
+                    .unwrap_or_default();
+                descendants.push(ety);
+                descendants
+            }
+            EntityType::Unspecified => Vec::new(),
+        }
     }
 
     /// Get all entity types in the schema where an `{entity0} in {euids}` can
@@ -514,12 +525,11 @@ impl ValidatorSchema {
     pub(crate) fn get_entity_types_in_set<'a>(
         &'a self,
         euids: impl IntoIterator<Item = &'a EntityUID> + 'a,
-    ) -> Option<Vec<&Name>> {
+    ) -> impl Iterator<Item = &Name> {
         euids
             .into_iter()
             .map(|e| self.get_entity_types_in(e))
-            .collect::<Option<Vec<_>>>()
-            .map(|v| v.into_iter().flatten().collect::<Vec<_>>())
+            .flatten()
     }
 
     /// Get all action entities in the schema where `action in euids` evaluates

cedar-policy-validator/src/typecheck.rs

@@ -1886,35 +1886,54 @@ impl<'a> Typechecker<'a> {
             } else {
                 request_env.resource_entity_type()
             };
-            let descendants = self.schema.get_entity_types_in_set(rhs.iter());
-            match (var_euid, descendants) {
-                // We failed to lookup the descendants because the entity type
-                // is not declared in the schema, or we failed to get the
-                // principal/resource entity type because the request is
-                // unknown.  We don't know if the euid would be in the
-                // descendants or not, so give it type boolean.
-                (_, None) | (None, _) => {
+            match var_euid {
+                // We failed to get the principal/resource entity type because
+                // we are typechecking a request for some action which isn't
+                // declared in the schema.  We don't know if the euid would be
+                // in the descendants or not, so give it type boolean.
+                None => {
                     let in_expr = ExprBuilder::with_data(Some(Type::primitive_boolean()))
                         .with_same_source_info(in_expr)
                         .is_in(lhs_expr, rhs_expr);
                     if self.mode.is_partial() {
                         TypecheckAnswer::success(in_expr)
                     } else {
+                        // This should only happen when doing partial validation
+                        // since we never construct the undeclared action
+                        // request environment otherwise.
                         TypecheckAnswer::fail(in_expr)
                     }
                 }
-                (Some(EntityType::Specified(var_name)), Some(descendants)) => {
-                    Typechecker::entity_in_descendants(
-                        var_name,
-                        descendants,
-                        in_expr,
-                        lhs_expr,
-                        rhs_expr,
-                    )
+                Some(EntityType::Specified(var_name)) => {
+                    let all_rhs_known = rhs
+                        .iter()
+                        .all(|e| self.schema.euid_has_known_entity_type(e));
+                    if self.schema.is_known_entity_type(var_name) && all_rhs_known {
+                        let descendants = self.schema.get_entity_types_in_set(rhs.iter());
+                        Typechecker::entity_in_descendants(
+                            var_name,
+                            descendants,
+                            in_expr,
+                            lhs_expr,
+                            rhs_expr,
+                        )
+                    } else {
+                        let annotated_expr =
+                            ExprBuilder::with_data(Some(Type::primitive_boolean()))
+                                .with_same_source_info(in_expr)
+                                .is_in(lhs_expr, rhs_expr);
+                        if self.mode.is_partial() {
+                            // In partial schema mode, undeclared entity types are
+                            // expected.
+                            TypecheckAnswer::success(annotated_expr)
+                        } else {
+                            TypecheckAnswer::fail(annotated_expr)
+                        }
+                    }
                 }
                 // Unspecified entities will be detected by a different part of the validator.
                 // Still return `TypecheckFail` so that typechecking is not considered successful.
-                (Some(EntityType::Unspecified), _) => TypecheckAnswer::fail(
+                Some(EntityType::Unspecified) => TypecheckAnswer::fail(
                     ExprBuilder::with_data(Some(Type::primitive_boolean()))
                         .with_same_source_info(in_expr)
                         .is_in(lhs_expr, rhs_expr),
@@ -1966,7 +1985,7 @@ impl<'a> Typechecker<'a> {
                     } else if !lhs_is_action && !non_actions.is_empty() {
                         self.type_of_non_action_in_entities(
                             lhs_euid,
-                            non_actions.iter(),
+                            &non_actions,
                             in_expr,
                             lhs_expr,
                             rhs_expr,
@@ -2038,34 +2057,33 @@ impl<'a> Typechecker<'a> {
     fn type_of_non_action_in_entities<'b>(
         &self,
         lhs: &EntityUID,
-        rhs: impl IntoIterator<Item = &'a EntityUID> + 'a,
+        rhs: &Vec<EntityUID>,
         in_expr: &Expr,
         lhs_expr: Expr<Option<Type>>,
         rhs_expr: Expr<Option<Type>>,
     ) -> TypecheckAnswer<'b> {
         match lhs.entity_type() {
             EntityType::Specified(lhs_ety) => {
-                let rhs_descendants = self.schema.get_entity_types_in_set(rhs);
-                match rhs_descendants {
-                    Some(rhs_descendants) if self.schema.is_known_entity_type(lhs_ety) => {
-                        Typechecker::entity_in_descendants(
-                            lhs_ety,
-                            rhs_descendants,
-                            in_expr,
-                            lhs_expr,
-                            rhs_expr,
-                        )
-                    }
-                    _ => {
-                        let annotated_expr =
-                            ExprBuilder::with_data(Some(Type::primitive_boolean()))
-                                .with_same_source_info(in_expr)
-                                .is_in(lhs_expr, rhs_expr);
-                        if self.mode.is_partial() {
-                            TypecheckAnswer::success(annotated_expr)
-                        } else {
-                            TypecheckAnswer::fail(annotated_expr)
-                        }
+                let all_rhs_known = rhs
+                    .iter()
+                    .all(|e| self.schema.euid_has_known_entity_type(e));
+                if self.schema.is_known_entity_type(lhs_ety) && all_rhs_known {
+                    let rhs_descendants = self.schema.get_entity_types_in_set(rhs.iter());
+                    Typechecker::entity_in_descendants(
+                        lhs_ety,
+                        rhs_descendants,
+                        in_expr,
+                        lhs_expr,
+                        rhs_expr,
+                    )
+                } else {
+                    let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean()))
+                        .with_same_source_info(in_expr)
+                        .is_in(lhs_expr, rhs_expr);
+                    if self.mode.is_partial() {
+                        TypecheckAnswer::success(annotated_expr)
+                    } else {
+                        TypecheckAnswer::fail(annotated_expr)
                     }
                 }
             }

cedar-policy-validator/src/typecheck/test_policy.rs

@@ -427,6 +427,66 @@ fn policy_in_action_impossible() {
         p.clone(),
         vec![TypeError::impossible_policy(p.condition())],
     );
+
+    let p = parse_policy(
+        Some("0".to_string()),
+        r#"permit(principal, action, resource) when { User::"alice" in [Action::"view_photo"] };"#,
+    )
+    .expect("Policy should parse.");
+    assert_policy_typecheck_fails_simple_schema(
+        p.clone(),
+        vec![TypeError::impossible_policy(p.condition())],
+    );
+
+    let p = parse_policy(
+        Some("0".to_string()),
+        r#"permit(principal, action, resource) when { principal in [action] };"#,
+    )
+    .expect("Policy should parse.");
+    assert_policy_typecheck_fails_simple_schema(
+        p.clone(),
+        vec![TypeError::impossible_policy(p.condition())],
+    );
+
+    let p = parse_policy(
+        Some("0".to_string()),
+        r#"permit(principal, action, resource) when { principal in action };"#,
+    )
+    .expect("Policy should parse.");
+    assert_policy_typecheck_fails_simple_schema(
+        p.clone(),
+        vec![TypeError::impossible_policy(p.condition())],
+    );
+
+    let p = parse_policy(
+        Some("0".to_string()),
+        r#"permit(principal, action, resource) when { principal in Action::"view_photo" };"#,
+    )
+    .expect("Policy should parse.");
+    assert_policy_typecheck_fails_simple_schema(
+        p.clone(),
+        vec![TypeError::impossible_policy(p.condition())],
+    );
+
+    let p = parse_policy(
+        Some("0".to_string()),
+        r#"permit(principal, action, resource) when { principal in [Action::"view_photo", Action::"delete_group"] };"#,
+    )
+    .expect("Policy should parse.");
+    assert_policy_typecheck_fails_simple_schema(
+        p.clone(),
+        vec![TypeError::impossible_policy(p.condition())],
+    );
+
+    let p = parse_policy(
+        Some("0".to_string()),
+        r#"permit(principal, action, resource) when { principal in [Action::"view_photo", Photo::"bar"] };"#,
+    )
+    .expect("Policy should parse.");
+    assert_policy_typecheck_permissive_fails_simple_schema(
+        p.clone(),
+        vec![TypeError::impossible_policy(p.condition())],
+    );
 }
 
 #[test]