From 39fa93e40f43f12cfd47eb4ba6423d9b7de35497 Mon Sep 17 00:00:00 2001 From: John Kastner Date: Thu, 30 Nov 2023 16:59:01 +0000 Subject: [PATCH 1/5] Fix typechecking for `in Action::"..."` The partial schema PR inadvertantly changed the behavior of, e.g., `princpal in Action::"view"` to be an error, claiming that the `Action` entity type is not defined. It should instead recognize that actions may only be `in` other actions and give the expression type `False`. Our unit tests check the `User::"alice" in Action::"view"` case, but did not test the case when the left side of the `in` was a var. --- cedar-policy-validator/src/rbac.rs | 10 +- cedar-policy-validator/src/schema.rs | 31 +++--- cedar-policy-validator/src/typecheck.rs | 102 +++++++++++------- .../src/typecheck/test_policy.rs | 60 +++++++++++ 4 files changed, 140 insertions(+), 63 deletions(-) diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index 0739d2fc0..4c56f1046 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 f865789d2..a5ecd42f4 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -471,7 +471,7 @@ 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) } /// An iterator over the action ids in the schema. @@ -494,18 +494,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 +514,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 6e0c4b6ab..2336ae53b 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -1886,35 +1886,57 @@ 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 if are doing partial 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 any_rhs_not_declared = rhs.iter().any(|e| match e.entity_type() { + EntityType::Specified(ety) => !self.schema.is_known_entity_type(ety), + EntityType::Unspecified => false, + }); + if any_rhs_not_declared && self.mode.is_partial() { + // In partial schema mode, undeclared entity types are + // expected. + TypecheckAnswer::success( + ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(in_expr) + .is_in(lhs_expr, rhs_expr), + ) + } else if any_rhs_not_declared { + TypecheckAnswer::fail( + ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(in_expr) + .is_in(lhs_expr, rhs_expr), + ) + } else { + let descendants = self.schema.get_entity_types_in_set(rhs.iter()); + Typechecker::entity_in_descendants( + var_name, + descendants, + in_expr, + lhs_expr, + rhs_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 +1988,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,35 +2060,35 @@ 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 rhs_descendants = self.schema.get_entity_types_in_set(rhs.iter()); + let any_rhs_not_declared = rhs.iter().any(|e| match e.entity_type() { + EntityType::Specified(ety) => !self.schema.is_known_entity_type(ety), + EntityType::Unspecified => false, + }); + if !self.schema.is_known_entity_type(lhs_ety) || any_rhs_not_declared { + 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) } + } else { + Typechecker::entity_in_descendants( + lhs_ety, + rhs_descendants, + in_expr, + lhs_expr, + rhs_expr, + ) } } EntityType::Unspecified => { diff --git a/cedar-policy-validator/src/typecheck/test_policy.rs b/cedar-policy-validator/src/typecheck/test_policy.rs index 6685f3493..e8aa268c2 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] From 81aa44f3e6c072d6c478b67def4ca337270e7d78 Mon Sep 17 00:00:00 2001 From: John Kastner Date: Fri, 1 Dec 2023 16:11:45 +0000 Subject: [PATCH 2/5] wip --- cedar-policy-validator/src/schema.rs | 8 ++++++++ cedar-policy-validator/src/typecheck.rs | 16 +++++----------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index a5ecd42f4..382f865d8 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -474,6 +474,14 @@ impl ValidatorSchema { 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. + 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. pub(crate) fn known_action_ids(&self) -> impl Iterator { self.action_ids.keys() diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 2336ae53b..9b8d1d888 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -1898,17 +1898,14 @@ impl<'a> Typechecker<'a> { if self.mode.is_partial() { TypecheckAnswer::success(in_expr) } else { - // This should only happen if are doing partial since we - // never construct the undeclared action request - // environment otherwise. + // 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)) => { - let any_rhs_not_declared = rhs.iter().any(|e| match e.entity_type() { - EntityType::Specified(ety) => !self.schema.is_known_entity_type(ety), - EntityType::Unspecified => false, - }); + let any_rhs_not_declared = rhs.iter().any(|e| !self.schema.euid_has_known_entity_type(e)); if any_rhs_not_declared && self.mode.is_partial() { // In partial schema mode, undeclared entity types are // expected. @@ -2068,10 +2065,7 @@ impl<'a> Typechecker<'a> { match lhs.entity_type() { EntityType::Specified(lhs_ety) => { let rhs_descendants = self.schema.get_entity_types_in_set(rhs.iter()); - let any_rhs_not_declared = rhs.iter().any(|e| match e.entity_type() { - EntityType::Specified(ety) => !self.schema.is_known_entity_type(ety), - EntityType::Unspecified => false, - }); + let any_rhs_not_declared = rhs.iter().any(|e| !self.schema.euid_has_known_entity_type(e)); if !self.schema.is_known_entity_type(lhs_ety) || any_rhs_not_declared { let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) .with_same_source_info(in_expr) From 4c6897c2591b512ac8ac0406540f82468ac34404 Mon Sep 17 00:00:00 2001 From: John Kastner Date: Fri, 1 Dec 2023 16:44:21 +0000 Subject: [PATCH 3/5] wip --- cedar-policy-validator/src/typecheck.rs | 58 +++++++++++++------------ 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 9b8d1d888..dbcd1bc97 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -1905,22 +1905,11 @@ impl<'a> Typechecker<'a> { } } Some(EntityType::Specified(var_name)) => { - let any_rhs_not_declared = rhs.iter().any(|e| !self.schema.euid_has_known_entity_type(e)); - if any_rhs_not_declared && self.mode.is_partial() { - // In partial schema mode, undeclared entity types are - // expected. - TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_info(in_expr) - .is_in(lhs_expr, rhs_expr), - ) - } else if any_rhs_not_declared { - TypecheckAnswer::fail( - ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_info(in_expr) - .is_in(lhs_expr, rhs_expr), - ) - } else { + if self.schema.is_known_entity_type(var_name) + && rhs + .iter() + .all(|e| self.schema.euid_has_known_entity_type(e)) + { let descendants = self.schema.get_entity_types_in_set(rhs.iter()); Typechecker::entity_in_descendants( var_name, @@ -1929,6 +1918,18 @@ impl<'a> Typechecker<'a> { 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. @@ -2064,9 +2065,20 @@ impl<'a> Typechecker<'a> { ) -> TypecheckAnswer<'b> { match lhs.entity_type() { EntityType::Specified(lhs_ety) => { - let rhs_descendants = self.schema.get_entity_types_in_set(rhs.iter()); - let any_rhs_not_declared = rhs.iter().any(|e| !self.schema.euid_has_known_entity_type(e)); - if !self.schema.is_known_entity_type(lhs_ety) || any_rhs_not_declared { + if self.schema.is_known_entity_type(lhs_ety) + && rhs + .iter() + .all(|e| self.schema.euid_has_known_entity_type(e)) + { + 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); @@ -2075,14 +2087,6 @@ impl<'a> Typechecker<'a> { } else { TypecheckAnswer::fail(annotated_expr) } - } else { - Typechecker::entity_in_descendants( - lhs_ety, - rhs_descendants, - in_expr, - lhs_expr, - rhs_expr, - ) } } EntityType::Unspecified => { From c9fa9b901c9e5d7d2495d247fd18b9a286ab611e Mon Sep 17 00:00:00 2001 From: John Kastner Date: Fri, 1 Dec 2023 16:48:31 +0000 Subject: [PATCH 4/5] wip --- cedar-policy-validator/src/typecheck.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index dbcd1bc97..e0bef9a40 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -1905,11 +1905,10 @@ impl<'a> Typechecker<'a> { } } Some(EntityType::Specified(var_name)) => { - if self.schema.is_known_entity_type(var_name) - && rhs - .iter() - .all(|e| self.schema.euid_has_known_entity_type(e)) - { + 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, @@ -2065,11 +2064,10 @@ impl<'a> Typechecker<'a> { ) -> TypecheckAnswer<'b> { match lhs.entity_type() { EntityType::Specified(lhs_ety) => { - if self.schema.is_known_entity_type(lhs_ety) - && rhs - .iter() - .all(|e| self.schema.euid_has_known_entity_type(e)) - { + 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, From 8144642322aece25d6685cea506e31549747117c Mon Sep 17 00:00:00 2001 From: John Kastner Date: Fri, 1 Dec 2023 18:22:29 +0000 Subject: [PATCH 5/5] comment --- cedar-policy-validator/src/schema.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 382f865d8..e3264add6 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -474,7 +474,10 @@ impl ValidatorSchema { 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. + /// 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),