diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index 0739d2fc04..4c56f1046b 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -383,12 +383,9 @@ impl Validator { .into_iter(), ), // in - 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), ) diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index f865789d2d..e3264add6e 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/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> { - 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::>()) + .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 + 'a, - ) -> Option> { + ) -> impl Iterator { euids .into_iter() .map(|e| self.get_entity_types_in(e)) - .collect::>>() - .map(|v| v.into_iter().flatten().collect::>()) + .flatten() } /// Get all action entities in the schema where `action in euids` evaluates diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 6e0c4b6ab2..e0bef9a401 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/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 + 'a, + rhs: &Vec, in_expr: &Expr, lhs_expr: Expr>, rhs_expr: Expr>, ) -> 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) } } } diff --git a/cedar-policy-validator/src/typecheck/test_policy.rs b/cedar-policy-validator/src/typecheck/test_policy.rs index 6685f34930..e8aa268c2c 100644 --- a/cedar-policy-validator/src/typecheck/test_policy.rs +++ b/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]