From e015e9bf8f3f250552e1eba6288a699cd8383e00 Mon Sep 17 00:00:00 2001 From: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com> Date: Fri, 8 Sep 2023 15:15:30 -0400 Subject: [PATCH] Simplified strict typechecking (#282) Implements cedar-policy/rfcs#19 --- cedar-policy-validator/src/type_error.rs | 51 +- cedar-policy-validator/src/typecheck.rs | 933 +++++------------- .../src/typecheck/test_expr.rs | 91 +- .../src/typecheck/test_optional_attributes.rs | 3 +- .../src/typecheck/test_policy.rs | 69 +- .../src/typecheck/test_strict.rs | 35 +- .../src/typecheck/test_utils.rs | 72 +- cedar-policy-validator/src/types.rs | 204 +++- cedar-policy/CHANGELOG.md | 1 + 9 files changed, 631 insertions(+), 828 deletions(-) diff --git a/cedar-policy-validator/src/type_error.rs b/cedar-policy-validator/src/type_error.rs index ae6879313..499bba479 100644 --- a/cedar-policy-validator/src/type_error.rs +++ b/cedar-policy-validator/src/type_error.rs @@ -19,7 +19,7 @@ use std::{collections::BTreeSet, fmt::Display}; use cedar_policy_core::{ - ast::{CallStyle, EntityUID, Expr, ExprKind, Var}, + ast::{CallStyle, EntityUID, Expr, ExprKind, Name, Var}, parser::SourceInfo, }; @@ -96,19 +96,6 @@ impl TypeError { } } - pub(crate) fn types_must_match( - on_expr: Expr, - types: impl IntoIterator, - ) -> Self { - Self { - on_expr: None, - source_location: on_expr.into_source_info(), - kind: TypeErrorKind::TypesMustMatch(TypesMustMatch { - types: types.into_iter().collect::>(), - }), - } - } - pub(crate) fn unsafe_attribute_access( on_expr: Expr, attribute_access: AttributeAccess, @@ -196,6 +183,18 @@ impl TypeError { kind: TypeErrorKind::NonLitExtConstructor, } } + + pub(crate) fn hierarchy_not_respected( + on_expr: Expr, + in_lhs: Option, + in_rhs: Option, + ) -> Self { + Self { + on_expr: None, + source_location: on_expr.into_source_info(), + kind: TypeErrorKind::HierarchyNotRespected(HierarchyNotRespected { in_lhs, in_rhs }), + } + } } impl Display for TypeError { @@ -264,12 +263,19 @@ pub enum TypeErrorKind { /// Error returned by custom extension function argument validation #[error("Error during extension function argument validation: {}", .0.msg)] FunctionArgumentValidationError(FunctionArgumentValidationError), - #[error("Types of operands in this expression are not equal: [{}]", .0.types.iter().join(","))] - TypesMustMatch(TypesMustMatch), #[error("empty set literals are forbidden in policies")] EmptySetForbidden, #[error("extension constructors may not be called with non-literal expressions")] NonLitExtConstructor, + /// To pass strict validation a policy cannot contain an `in` expression + /// where the entity type on the left might not be able to be a member of + /// the entity type on the right. + #[error("operands to `in` do not respect the entity hierarchy{}", + match (&.0.in_lhs, &.0.in_rhs) { + (Some(in_lhs), Some(in_rhs)) => format!(". `{}` is not a descendant of `{}`", in_lhs, in_rhs), + _ => "".to_string(), + })] + HierarchyNotRespected(HierarchyNotRespected), } /// Structure containing details about an unexpected type error. @@ -282,12 +288,6 @@ pub struct UnexpectedType { /// Structure containing details about an incompatible type error. #[derive(Debug, Hash, Eq, PartialEq)] pub struct IncompatibleTypes { - types: BTreeSet, -} - -/// Structure containing details about types must match error. -#[derive(Debug, Hash, Eq, PartialEq)] -pub struct TypesMustMatch { pub(crate) types: BTreeSet, } @@ -339,6 +339,13 @@ pub struct FunctionArgumentValidationError { msg: String, } +/// Structure containing details about a hierarchy not respected error +#[derive(Debug, Hash, Eq, PartialEq)] +pub struct HierarchyNotRespected { + in_lhs: Option, + in_rhs: Option, +} + /// Contains more detailed information about an attribute access when it occurs /// on an entity type expression or on the `context` variable. Track a `Vec` of /// attributes rather than a single attribute so that on `principal.foo.bar` can diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 39f821aef..3860cbc3c 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -41,9 +41,7 @@ use crate::{ schema::{ is_action_entity_type, ActionHeadVar, HeadVar, PrincipalOrResourceHeadVar, ValidatorSchema, }, - types::{ - AttributeType, Effect, EffectSet, EntityRecordKind, OpenTag, Primitive, RequestEnv, Type, - }, + types::{AttributeType, Effect, EffectSet, EntityRecordKind, OpenTag, RequestEnv, Type}, AttributeAccess, ValidationMode, }; @@ -118,14 +116,6 @@ impl<'a> TypecheckAnswer<'a> { == Some(ty) } - pub fn typed_expr(&self) -> Option<&Expr>> { - match self { - TypecheckAnswer::TypecheckSuccess { expr_type, .. } => Some(expr_type), - TypecheckAnswer::TypecheckFail { expr_recovery_type } => Some(expr_recovery_type), - TypecheckAnswer::RecursionLimit => None, - } - } - pub fn into_typed_expr(self) -> Option>> { match self { TypecheckAnswer::TypecheckSuccess { expr_type, .. } => Some(expr_type), @@ -277,11 +267,7 @@ impl<'a> Typechecker<'a> { /// added to the output list. Otherwise, the function returns false and the /// output list is populated with any errors encountered while typechecking. pub fn typecheck_policy(&self, t: &Template, type_errors: &mut HashSet) -> bool { - let typecheck_answers = if self.mode.is_strict() { - self.typecheck_by_request_env_strict(t) - } else { - self.typecheck_by_request_env(t) - }; + let typecheck_answers = self.typecheck_by_request_env(t); // consolidate the results from each query environment let (all_false, all_succ) = typecheck_answers.into_iter().fold( @@ -339,36 +325,6 @@ impl<'a> Typechecker<'a> { }) } - /// A strict variant of `typecheck_by_request_env` which is used by cedar policy - /// analysis. - pub fn typecheck_by_request_env_strict<'b>( - &'b self, - t: &'b Template, - ) -> Vec<(RequestEnv, PolicyCheck)> { - self.apply_typecheck_fn_by_request_env(t, move |request, expr| { - let mut type_errors = Vec::new(); - let ty = - self.typecheck_strict(request, expr, Type::primitive_boolean(), &mut type_errors); - - // Strict types don't include the True and False boolean singletons, - // so we can't check for for type False. The strict transformation - // includes a rewriting from any boolean singleton type expression - // to the corresponding boolean literal, so we instead look for the - // literal `false`. - match ty.typed_expr() { - Some(typed_expr) => { - let is_false = typed_expr.eq_shape(&Expr::val(false)); - match (is_false, ty.typechecked()) { - (false, false) => PolicyCheck::Fail(type_errors), - (false, true) => PolicyCheck::Success(typed_expr.clone()), - (true, _) => PolicyCheck::Irrelevant(type_errors), - } - } - None => PolicyCheck::Fail(type_errors), - } - }) - } - /// Utility abstracting the common logic for strict and regular typechecking /// by request environment. fn apply_typecheck_fn_by_request_env<'b, F, C>( @@ -396,15 +352,12 @@ impl<'a> Typechecker<'a> { result_checks } - /// Additional entry point for strict typechecking requests. This method takes a slice + /// Additional entry point for typechecking requests. This method takes a slice /// over policies and typechecks each under every schema-defined request environment. /// /// The result contains these environments in no particular order, but each list of /// policy checks will always match the original order. - /// - /// This function is currently only used in policy analysis where only the - /// strict variant is needed. - pub fn multi_typecheck_by_request_env_strict( + pub fn multi_typecheck_by_request_env( &self, policy_templates: &[&Template], ) -> Vec<(RequestEnv, Vec)> { @@ -412,34 +365,24 @@ impl<'a> Typechecker<'a> { for request in self.unlinked_request_envs() { let mut policy_checks = Vec::new(); for t in policy_templates.iter() { + let condition_expr = t.condition(); for linked_env in self.link_request_env(request.clone(), t) { let mut type_errors = Vec::new(); - let policy_condition = &t.condition(); - let ty = self.typecheck_strict( + let empty_prior_eff = EffectSet::new(); + let ty = self.expect_type( &linked_env, - policy_condition, + &empty_prior_eff, + &condition_expr, Type::primitive_boolean(), &mut type_errors, ); - // Again, look for the literal `false` instead of the type - // false. - match ty.typed_expr() { - Some(typed_expr) => { - let is_false = typed_expr.eq_shape(&Expr::val(false)); - match (is_false, ty.typechecked()) { - (false, false) => { - policy_checks.push(PolicyCheck::Fail(type_errors)) - } - (false, true) => { - policy_checks.push(PolicyCheck::Success(typed_expr.clone())); - } - (true, _) => { - policy_checks.push(PolicyCheck::Irrelevant(type_errors)) - } - } - } - None => policy_checks.push(PolicyCheck::Fail(type_errors)), + let is_false = ty.contains_type(&Type::singleton_boolean(false)); + match (is_false, ty.typechecked(), ty.into_typed_expr()) { + (false, true, None) => policy_checks.push(PolicyCheck::Fail(type_errors)), + (false, true, Some(e)) => policy_checks.push(PolicyCheck::Success(e)), + (false, false, _) => policy_checks.push(PolicyCheck::Fail(type_errors)), + (true, _, _) => policy_checks.push(PolicyCheck::Irrelevant(type_errors)), } } } @@ -551,557 +494,6 @@ impl<'a> Typechecker<'a> { } } - fn typecheck_strict<'b>( - &self, - request_env: &RequestEnv, - e: &'b Expr, - expected_type: Type, - type_errors: &mut Vec, - ) -> TypecheckAnswer<'b> { - let empty_prior_eff = EffectSet::new(); - let tc_ans = self.expect_type(request_env, &empty_prior_eff, e, expected_type, type_errors); - - tc_ans.then_typecheck(|annot_expr, _| self.strict_transform(&annot_expr, type_errors)) - } - - fn check_all_types_compat_acts(elems_types: &HashSet<&Option>) -> bool { - let mut action_entity_namespace: Option = None; - return elems_types.iter().all(|ot| match ot { - None => false, - Some(t) => match t { - Type::EntityOrRecord(EntityRecordKind::ActionEntity { .. }) => true, - Type::EntityOrRecord(EntityRecordKind::Entity(lub)) => { - match lub.get_single_entity() { - Some(n) => { - if is_action_entity_type(n) { - match &action_entity_namespace { - Some(ns) => n.namespace().eq(&ns.clone()), - None => { - action_entity_namespace = Some(n.namespace()); - true - } - } - } else { - false - } - } - None => false, - } - } - _ => false, - }, - }); - } - - fn strict_transform<'b>( - &self, - e: &Expr>, - type_errors: &mut Vec, - ) -> TypecheckAnswer<'b> { - #[cfg(not(target_arch = "wasm32"))] - if stacker::remaining_stack().unwrap_or(0) < REQUIRED_STACK_SPACE { - return TypecheckAnswer::RecursionLimit; - } - match e.data() { - // If the validator could conclude that an expression is always true - // or always false (encoded as the singleton boolean types - // `Type::True` and `Type::False`), then we return a constant - // `true` or `false` expression. The type of the constant expression - // is `Boolean` in either case because strictly typed expressions - // may not use the singleton boolean types. - Some(Type::True) => TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::primitive_boolean())).val(true), - ), - Some(Type::False) => TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::primitive_boolean())).val(false), - ), - _ => match e.expr_kind() { - // These are leaves of the AST, so no further work is done. - ExprKind::Lit(_) - | ExprKind::Var(_) - | ExprKind::Slot(_) - | ExprKind::Unknown { .. } => TypecheckAnswer::success(e.clone()), - - ExprKind::If { - test_expr, - then_expr, - else_expr, - } => { - match test_expr.data() { - // When the guard of an `if` expression always evaluates to - // the same constant boolean value, we can eliminate one of - // the branches without applying the strict transformation - // to it because it will never be evaluated. The guard is - // also eliminated. - Some(Type::True) => self.strict_transform(then_expr, type_errors), - Some(Type::False) => self.strict_transform(else_expr, type_errors), - - // The guard is not `True` or `False`. Assuming that `e` - // passed the validator, we know it's `Boolean`, so either - // the `then` or `else` branches may be evaluated. Strict - // validation requires that their strict types are - // equivalent. - _ => { - let test_ans = self.strict_transform(test_expr, type_errors); - let then_ans = self.strict_transform(then_expr, type_errors); - let else_ans = self.strict_transform(else_expr, type_errors); - then_ans.then_typecheck(|then_strict, _| { - else_ans.then_typecheck(|else_strict, _| { - test_ans.then_typecheck(|test_strict, _| { - let strict_expr = ExprBuilder::with_data(e.data().clone()) - .ite( - test_strict, - then_strict.clone(), - else_strict.clone(), - ); - // If either branch was not assigned a type, - // we do not conclude that they have different - // types. Note that we have already raised a - // type error whenever a type is `None`, so there - // are no soundness issues. - match (then_strict.data(), else_strict.data()) { - (Some(ty_then), Some(ty_else)) - if !Self::unify_strict_types(ty_then, ty_else) => - { - // Only generate a new type error when the - // types have a LUB. If the don't have a - // lub, the first typechecking pass already - // reported an error. - let has_lub = Type::least_upper_bound( - self.schema, - ty_then, - ty_else, - ) - .is_some(); - if has_lub { - type_errors.push(TypeError::types_must_match( - e.clone(), - [ty_then.clone(), ty_else.clone()], - )); - } - TypecheckAnswer::fail(strict_expr) - } - - _ => TypecheckAnswer::success(strict_expr), - } - }) - }) - }) - } - } - } - - ExprKind::BinaryApp { .. } => self.strict_transform_binary(e, type_errors), - - // The elements of a set must share a single type. We - // additionally require that they cannot be empty. This is not a - // hard requirement for strict validation, but the current - // validator can't assign a type to empty set literals, which is - // required by the analyzer. - ExprKind::Set(elems) => { - let elems_strict_answers = elems - .iter() - .map(|e| self.strict_transform(e, type_errors)) - .collect::>(); - TypecheckAnswer::sequence_all_then_typecheck( - elems_strict_answers, - |elems_strict_unwrapped| { - let elems_strict_exprs: Vec<_> = - elems_strict_unwrapped.into_iter().map(|(e, _)| e).collect(); - let strict_expr = ExprBuilder::with_data(e.data().clone()) - .set(elems_strict_exprs.clone()); - - let elems_types = elems_strict_exprs - .iter() - .map(|e| e.data()) - .collect::>(); - - let all_contained_types_are_compatable_actions = - Self::check_all_types_compat_acts(&elems_types); - - // Check that the type of all elements of the set - // can unify. This ensures that the all have the - // same type up to the limited subtyping allowed - // between True/False/Boolean. - let mut elems_types_iter = elems_types.iter(); - let representative_type = - elems_types_iter.next().and_then(|t| t.as_ref()); - let types_unify = representative_type - .map(|representative_type| { - elems_types_iter - .filter_map(|ty| ty.as_ref()) - .all(|ty| Self::unify_strict_types(representative_type, ty)) - }) - .unwrap_or(true); - - let contains_one_type = - types_unify || all_contained_types_are_compatable_actions; - let is_non_empty = elems.len() != 0; - - if !contains_one_type { - type_errors.push(TypeError::types_must_match( - e.clone(), - elems_types.into_iter().flatten().cloned(), - )); - } - if !is_non_empty { - type_errors.push(TypeError::empty_set_forbidden(e.clone())); - } - - if contains_one_type && is_non_empty { - TypecheckAnswer::success(strict_expr) - } else { - TypecheckAnswer::fail(strict_expr) - } - }, - ) - } - - // Extension type constructor functions requiring string - // parsing should only be callable with literals. The functions - // have a `check_arguments` function defined which is used by - // the standard validator to ensure that the string can be - // parsed. The standard validator, however, allows calling - // these functions with non-literal expression that might not - // parse, so we have a - ExprKind::ExtensionFunctionApp { fn_name, args } => { - let args_strict_answers = args - .iter() - .map(|e| self.strict_transform(e, type_errors)) - .collect::>(); - - TypecheckAnswer::sequence_all_then_typecheck( - args_strict_answers, - |args_strict_unwrapped| { - let strict_expr = ExprBuilder::with_data(e.data().clone()) - .call_extension_fn( - fn_name.clone(), - args_strict_unwrapped.into_iter().map(|a| a.0).collect(), - ); - let fn_has_arg_check = match self.lookup_extension_function(fn_name) { - Ok(f) => f.has_argument_check(), - // The function is not defined or is defined - // multiple times. An error was already raised by - // the standard typechecker. - Err(_) => false, - }; - let args_args_lit = args - .iter() - .all(|e| matches!(e.expr_kind(), ExprKind::Lit(_))); - if !fn_has_arg_check || args_args_lit { - TypecheckAnswer::success(strict_expr) - } else { - type_errors.push(TypeError::non_lit_ext_constructor(e.clone())); - TypecheckAnswer::fail(strict_expr) - } - }, - ) - } - - // All other expressions are also processed recursively. Any - // expressions that can have types `Type` and `False` are - // handled by the constant boolean expression rules. This - // applies to, for example, `And` and `Or` expressions which can - // short circuit to `true` or `false`. - ExprKind::And { left, right } => { - let left_strict = self.strict_transform(left, type_errors); - left_strict.then_typecheck(|left_strict, _| { - let right_strict = self.strict_transform(right, type_errors); - right_strict.then_typecheck(|right_strict, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()) - .and(left_strict, right_strict), - ) - }) - }) - } - ExprKind::Or { left, right } => { - let left_strict = self.strict_transform(left, type_errors); - left_strict.then_typecheck(|left_strict, _| { - let right_strict = self.strict_transform(right, type_errors); - right_strict.then_typecheck(|right_strict, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()) - .or(left_strict, right_strict), - ) - }) - }) - } - - ExprKind::UnaryApp { op, arg } => self - .strict_transform(arg, type_errors) - .then_typecheck(|strict_arg, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()).unary_app(*op, strict_arg), - ) - }), - ExprKind::MulByConst { arg, constant } => self - .strict_transform(arg, type_errors) - .then_typecheck(|strict_arg, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()).mul(strict_arg, *constant), - ) - }), - - ExprKind::GetAttr { expr, attr } => self - .strict_transform(expr, type_errors) - .then_typecheck(|strict_expr, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()) - .get_attr(strict_expr, attr.clone()), - ) - }), - ExprKind::HasAttr { expr, attr } => self - .strict_transform(expr, type_errors) - .then_typecheck(|strict_expr, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()) - .has_attr(strict_expr, attr.clone()), - ) - }), - ExprKind::Like { expr, pattern } => self - .strict_transform(expr, type_errors) - .then_typecheck(|strict_expr, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()) - .like(strict_expr, pattern.iter().cloned()), - ) - }), - ExprKind::Record { pairs } => { - let (attr_names, strict_attr_exprs): (Vec<_>, Vec<_>) = pairs - .iter() - .map(|(a, e)| (a.clone(), self.strict_transform(e, type_errors))) - .unzip(); - - TypecheckAnswer::sequence_all_then_typecheck( - strict_attr_exprs, - |strict_attr_exprs| { - TypecheckAnswer::success( - ExprBuilder::with_data(e.data().clone()).record( - attr_names - .into_iter() - .zip(strict_attr_exprs.into_iter().map(|(e, _)| e)), - ), - ) - }, - ) - } - }, - } - } - - fn strict_transform_binary<'b>( - &self, - bin_expr: &Expr>, - type_errors: &mut Vec, - ) -> TypecheckAnswer<'b> { - let ExprKind::BinaryApp { op, arg1, arg2 } = bin_expr.expr_kind() else { - panic!( - "`strict_transform_binary` called with an expression kind other than `BinaryApp`" - ); - }; - - // Binary operators `==`, `contains`, `containsAll`, and `containsAny` - // are restricted to operating on operands of the same type (or sets of - // that type as appropriate). - let arg1_strict = self.strict_transform(arg1, type_errors); - let arg2_strict = self.strict_transform(arg2, type_errors); - arg1_strict.then_typecheck(|arg1_strict, _| { - arg2_strict.then_typecheck(|arg2_strict, _| { - // If either operand was not assigned a type, then we should not - // conclude that they have different types, as this raises - // spurious errors. Note that we have already raised a type - // error whenever a type is `None`, so there are no soundness - // issues. - match (arg1_strict.data(), arg2_strict.data()) { - (None, _) | (_, None) => TypecheckAnswer::success( - ExprBuilder::with_data(bin_expr.data().clone()).binary_app( - *op, - arg1_strict, - arg2_strict, - ), - ), - (Some(ty1), Some(ty2)) => { - let operand_types_match = match (op, arg1_strict.data(), arg2_strict.data()) - { - (BinaryOp::Eq, Some(ty1), Some(ty2)) => { - Self::unify_strict_types(ty1, ty2) - } - // Assume that LHS is a set. This is checked by the - // standard typechecker. - ( - BinaryOp::Contains, - Some(Type::Set { - // This pattern causes us to assume that the set is not the empty set. - // The empty set was already rejected by the strict validator, but it - // can still show up here because we continue typechecking after - // failure. It's fine to fall through to the catch-all case at the - // bottom where no additional error will be raised. - element_type: Some(elem_ty), - }), - Some(ty2), - ) => Self::unify_strict_types(elem_ty.as_ref(), ty2), - // Both args must be sets, but this is checked by - // the typechecker. Their elements must then be the - // same type, but, they're both sets, so we can just - // check for equality directly. - ( - BinaryOp::ContainsAll | BinaryOp::ContainsAny, - Some(ty1), - Some(ty2), - ) => Self::unify_strict_types(ty1, ty2), - (BinaryOp::In, Some(ty1), Some(ty2)) => { - let ty2 = match ty2 { - // If `element_type` is None then the second operand to `in` was an empty - // set. An error was raised for this already, so it can fall through to - // the catch-all at the next `match`. - Type::Set { element_type } => { - element_type.as_ref().map(|t| t.as_ref()) - } - _ => Some(ty2), - }; - match (ty1, ty2) { - ( - Type::EntityOrRecord(EntityRecordKind::Entity(lub1)), - Some(Type::EntityOrRecord(EntityRecordKind::Entity(lub2))), - ) => { - match (lub1.get_single_entity(), lub2.get_single_entity()) { - (Some(entity_type_name1), Some(entity_type_name2)) => { - let entity_type2 = - self.schema.get_entity_type(entity_type_name2); - - // Strict validation does not permit an `in` between unrelated - // entity types. - let is_same_entity_type = - entity_type_name1 == entity_type_name2; - let is_descendant = match entity_type2 { - Some(entity2) => entity2 - .descendants - .contains(entity_type_name1), - // The entity type does not exist. Even though an error - // was raised for an unknown entity type, it still makes - // sense to raise an error noting that strict validation - // would fail even if the entity type was declared. - None => false, - }; - is_same_entity_type || is_descendant - } - // One of the operands has a non-singleton entity LUB type. This - // implies that it did not strictly typecheck. - _ => true, - } - } - // AnyEntity cannot appear on either side of the `in`. This would effect - // slots and unspecified entities, but an `in` with unspecified entities - // is always `False` after standard typechecking, and we explicitly permit - // slots below. - ( - Type::EntityOrRecord( - EntityRecordKind::AnyEntity - | EntityRecordKind::Entity(_), - ), - Some(Type::EntityOrRecord( - EntityRecordKind::AnyEntity - | EntityRecordKind::Entity(_) - | EntityRecordKind::ActionEntity { .. }, - )), - ) => false, - // `in` is applied to a type that was either not an entity/set-of-entity or - // was an empty set. The typechecker already raised an error for this, so - // it doesn't make sense to also complain that the types don't match. - _ => true, - } - } - // No extra checking is required for the remaining binary operators. - _ => true, - }; - - // Arg2 has type `AnyEntity` when it is a template slot. - // This would normally fail the strict check, but we - // want to let slots through so that templates are not - // always rejected by the strict validator. For every - // instantiation of the slots in a template, either that - // instantiation passes strict validation or we can - // conclude the instantiation evaluates to `false`. - let arg2_is_slot = matches!(arg2.expr_kind(), ExprKind::Slot(_)); - - if arg2_is_slot || operand_types_match { - TypecheckAnswer::success( - ExprBuilder::with_data(bin_expr.data().clone()).binary_app( - *op, - arg1_strict, - arg2_strict, - ), - ) - } else { - type_errors.push(TypeError::types_must_match( - bin_expr.clone(), - [ty1.clone(), ty2.clone()], - )); - TypecheckAnswer::fail( - ExprBuilder::with_data(bin_expr.data().clone()).binary_app( - *op, - arg1_strict, - arg2_strict, - ), - ) - } - } - } - }) - }) - } - - pub(crate) fn unify_strict_types(actual: &Type, expected: &Type) -> bool { - match (actual, expected) { - ( - Type::True - | Type::False - | Type::Primitive { - primitive_type: Primitive::Bool, - }, - Type::True - | Type::False - | Type::Primitive { - primitive_type: Primitive::Bool, - }, - ) => true, - ( - Type::Set { - element_type: Some(ety1), - }, - Type::Set { - element_type: Some(ety2), - }, - ) => Self::unify_strict_types(ety1, ety2), - ( - Type::EntityOrRecord(EntityRecordKind::Record { - attrs: attrs1, - open_attributes: open1, - }), - Type::EntityOrRecord(EntityRecordKind::Record { - attrs: attrs2, - open_attributes: open2, - }), - ) => { - let keys1 = attrs1.attrs.keys().collect::>(); - let keys2 = attrs2.attrs.keys().collect::>(); - open1 == open2 - && keys1 == keys2 - && attrs1.iter().all(|(k, attr1)| { - // PANIC SAFETY: The attribute keys sets are equal. `k` is a key in `attr1`, so it must be a key in `attrs2`. - #[allow(clippy::expect_used)] - let attr2 = attrs2 - .get_attr(k) - .expect("Guarded by `keys1` == `keys2`, and `k` is a key in `keys1`."); - attr2.is_required == attr1.is_required - && Self::unify_strict_types(&attr1.attr_type, &attr2.attr_type) - }) - } - _ => actual == expected, - } - } - /// This method handles the majority of the work. Given an expression, /// the type for the request, and the a prior effect context for the /// expression, return the result of typechecking the expression, and add @@ -1266,33 +658,14 @@ impl<'a> Typechecker<'a> { then_expr, type_errors, ); - // We have to build a type annotated `else` branch for - // strict typechecking, but we want to throw away the - // errors. This could instead ignore the `else` if we - // update our Dafny formalism to verify that this is - // correct. - let mut errs_else = Vec::new(); - let ans_else = - self.typecheck(request_env, prior_eff, else_expr, &mut errs_else); ans_then.then_typecheck(|typ_then, eff_then| { - match ans_else.into_typed_expr() { - Some(ety) => { - TypecheckAnswer::success_with_effect( - ExprBuilder::with_data(typ_then.data().clone()) - .with_same_source_info(e) - .ite(typ_test, typ_then, ety), - // The output effect of the whole `if` expression also - // needs to contain the effect of the condition. - eff_then.union(&eff_test), - ) - } - // We might have hit the recursion limit only on - // the `else` branch. Unfortunate since that - // shouldn't effect typechecking, but we still - // need to exit to avoid a crash. - None => TypecheckAnswer::RecursionLimit, - } + TypecheckAnswer::success_with_effect( + typ_then, + // The output effect of the whole `if` expression also + // needs to contain the effect of the condition. + eff_then.union(&eff_test), + ) }) } else if typ_test.data() == &Some(Type::singleton_boolean(false)) { // The `else` branch cannot use the `test` effect since @@ -1302,27 +675,8 @@ impl<'a> Typechecker<'a> { let ans_else = self.typecheck(request_env, prior_eff, else_expr, type_errors); - // Annotating types but ignoring errors in the `then` branch. - let mut errs_then = Vec::new(); - let ans_then = self.typecheck( - request_env, - &prior_eff.union(&eff_test), - then_expr, - &mut errs_then, - ); - ans_else.then_typecheck(|typ_else, eff_else| { - match ans_then.into_typed_expr() { - Some(ety) => TypecheckAnswer::success_with_effect( - ExprBuilder::with_data(typ_else.data().clone()) - .with_same_source_info(e) - .ite(typ_test, ety, typ_else), - eff_else, - ), - // We might have hit the recursion limit only on - // the `then` branch. - None => TypecheckAnswer::RecursionLimit, - } + TypecheckAnswer::success_with_effect(typ_else, eff_else) }) } else { // When we don't short circuit, the `then` and `else` @@ -1772,6 +1126,14 @@ impl<'a> Typechecker<'a> { type_errors, ); match elem_lub { + _ if self.mode.is_strict() && exprs.is_empty() => { + type_errors.push(TypeError::empty_set_forbidden(e.clone())); + TypecheckAnswer::fail( + ExprBuilder::new() + .with_same_source_info(e) + .set(elem_expr_types), + ) + } Some(elem_lub) => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::set(elem_lub))) .with_same_source_info(e) @@ -1862,20 +1224,34 @@ impl<'a> Typechecker<'a> { BinaryOp::Eq => { let lhs_ty = self.typecheck(request_env, prior_eff, arg1, type_errors); let rhs_ty = self.typecheck(request_env, prior_eff, arg2, type_errors); - lhs_ty.then_typecheck(|lhs_ty, _| { rhs_ty.then_typecheck(|rhs_ty, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(Some(self.type_of_equality( - request_env, - arg1, - lhs_ty.data().clone(), - arg2, - rhs_ty.data().clone(), - ))) - .with_same_source_info(bin_expr) - .binary_app(*op, lhs_ty, rhs_ty), - ) + let type_of_eq = self.type_of_equality( + request_env, + arg1, + lhs_ty.data().clone(), + arg2, + rhs_ty.data().clone(), + ); + + if self.mode.is_strict() { + let annotated_eq = ExprBuilder::with_data(Some(type_of_eq)) + .with_same_source_info(bin_expr) + .binary_app(*op, lhs_ty.clone(), rhs_ty.clone()); + self.enforce_strict_equality( + bin_expr, + annotated_eq, + lhs_ty.data(), + rhs_ty.data(), + type_errors, + ) + } else { + TypecheckAnswer::success( + ExprBuilder::with_data(Some(type_of_eq)) + .with_same_source_info(bin_expr) + .binary_app(*op, lhs_ty, rhs_ty), + ) + } }) }) } @@ -1943,11 +1319,34 @@ impl<'a> Typechecker<'a> { // The second argument may be any type. We do not care if the element type cannot be in the set. self.typecheck(request_env, prior_eff, arg2, type_errors) .then_typecheck(|expr_ty_arg2, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_info(bin_expr) - .binary_app(*op, expr_ty_arg1, expr_ty_arg2), - ) + if self.mode.is_strict() { + let annotated_expr = + ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(bin_expr) + .binary_app( + *op, + expr_ty_arg1.clone(), + expr_ty_arg2.clone(), + ); + self.enforce_strict_equality( + bin_expr, + annotated_expr, + &match expr_ty_arg1.data() { + Some(Type::Set { + element_type: Some(ty), + }) => Some(*ty.clone()), + _ => None, + }, + expr_ty_arg2.data(), + type_errors, + ) + } else { + TypecheckAnswer::success( + ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(bin_expr) + .binary_app(*op, expr_ty_arg1, expr_ty_arg2), + ) + } }) }) } @@ -1958,17 +1357,70 @@ impl<'a> Typechecker<'a> { .then_typecheck(|expr_ty_arg1, _| { self.expect_type(request_env, prior_eff, arg2, Type::any_set(), type_errors) .then_typecheck(|expr_ty_arg2, _| { - TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_info(bin_expr) - .binary_app(*op, expr_ty_arg1, expr_ty_arg2), - ) + if self.mode.is_strict() { + let annotated_expr = + ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(bin_expr) + .binary_app( + *op, + expr_ty_arg1.clone(), + expr_ty_arg2.clone(), + ); + self.enforce_strict_equality( + bin_expr, + annotated_expr, + expr_ty_arg1.data(), + expr_ty_arg2.data(), + type_errors, + ) + } else { + TypecheckAnswer::success( + ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(bin_expr) + .binary_app(*op, expr_ty_arg1, expr_ty_arg2), + ) + } }) }) } } } + fn enforce_strict_equality<'b>( + &self, + unannotated_expr: &'b Expr, + annotated_expr: Expr>, + lhs_ty: &Option, + rhs_ty: &Option, + type_errors: &mut Vec, + ) -> TypecheckAnswer<'b> { + match annotated_expr.data() { + Some(Type::False) => { + TypecheckAnswer::success(ExprBuilder::with_data(Some(Type::False)).val(false)) + } + Some(Type::True) => { + TypecheckAnswer::success(ExprBuilder::with_data(Some(Type::True)).val(true)) + } + _ => match (lhs_ty, rhs_ty) { + (Some(lhs_ty), Some(rhs_ty)) + if Type::least_upper_bound(self.schema, &lhs_ty, &rhs_ty, self.mode) + .is_none() => + { + type_errors.push(TypeError::incompatible_types( + unannotated_expr.clone(), + [lhs_ty.clone(), rhs_ty.clone()], + )); + TypecheckAnswer::fail(annotated_expr) + } + // Either we had `Some` type for lhs and rhs and these types + // were compatible, or we failed to a compute a type for either + // lhs or rhs, meaning we already failed typechecking for that + // expression. + _ => TypecheckAnswer::success(annotated_expr), + }, + } + } + /// Like `typecheck_binary()`, but for multiplication, which isn't /// technically a `BinaryOp` fn typecheck_mul<'b>( @@ -2092,6 +1544,8 @@ impl<'a> Typechecker<'a> { ty_lhs.then_typecheck(|lhs_expr, _lhs_effects| { ty_rhs.then_typecheck(|rhs_expr, _rhs_effects| { + let lhs_ty = lhs_expr.data().clone(); + let rhs_ty = rhs_expr.data().clone(); let left_is_unspecified = Typechecker::is_unspecified_entity(request_env, lhs); let right_is_specified = match rhs_expr.data() { Some(Type::Set { element_type }) => element_type.as_ref().map(|t| t.as_ref()), @@ -2188,10 +1642,75 @@ impl<'a> Typechecker<'a> { ), } } + .then_typecheck(|type_of_in, _| { + if !self.mode.is_strict() { + TypecheckAnswer::success(type_of_in) + } else if matches!(type_of_in.data(), Some(Type::False)) { + TypecheckAnswer::success( + ExprBuilder::with_data(Some(Type::False)).val(false), + ) + } else if matches!(type_of_in.data(), Some(Type::True)) { + TypecheckAnswer::success(ExprBuilder::with_data(Some(Type::True)).val(true)) + } else { + match (lhs_ty, rhs_ty) { + (Some(lhs_ty), Some(rhs_ty)) => { + match ( + Self::get_as_single_entity_type(lhs_ty), + Self::get_as_single_entity_type(rhs_ty), + ) { + (Some(lhs_name), Some(rhs_name)) => { + let lhs_ty_in_rhs_ty = self + .schema + .get_entity_type(&rhs_name) + .map(|ety| ety.descendants.contains(&lhs_name)) + .unwrap_or(false); + if lhs_name == rhs_name || lhs_ty_in_rhs_ty { + TypecheckAnswer::success(type_of_in) + } else { + // We could actually just return `Type::False`, but this is incurs a larger Dafny proof update. + type_errors.push(TypeError::hierarchy_not_respected( + in_expr.clone(), + Some(lhs_name), + Some(rhs_name), + )); + TypecheckAnswer::fail(type_of_in) + } + } + _ => { + type_errors.push(TypeError::hierarchy_not_respected( + in_expr.clone(), + None, + None, + )); + TypecheckAnswer::fail(type_of_in) + } + } + } + // An argument type is `None`, so one the arguments must have failed to typecheck already. + // There's no other interesting error to report in this case. + _ => TypecheckAnswer::fail(type_of_in), + } + } + }) }) }) } + fn get_as_single_entity_type(ty: Type) -> Option { + match ty { + Type::EntityOrRecord(EntityRecordKind::Entity(lub)) => lub.into_single_entity(), + Type::EntityOrRecord(EntityRecordKind::ActionEntity { name, .. }) => Some(name), + Type::Set { + element_type: Some(element_type), + } => match *element_type { + Type::EntityOrRecord(EntityRecordKind::Entity(lub)) => lub.into_single_entity(), + Type::EntityOrRecord(EntityRecordKind::ActionEntity { name, .. }) => Some(name), + _ => None, + }, + _ => None, + } + } + // Given an expression, if that expression is a literal or the `action` head // variable, return it as an EntityUID. Return `None` otherwise. fn euid_from_euid_literal_or_action(request_env: &RequestEnv, e: &Expr) -> Option { @@ -2485,10 +2004,22 @@ impl<'a> Typechecker<'a> { let actual = self.typecheck(request_env, prior_eff, expr, type_errors); actual.then_typecheck(|mut typ_actual, eff_actual| match typ_actual.data() { Some(actual_ty) => { - if !expected - .iter() - .any(|expected_ty| Type::is_subtype(self.schema, actual_ty, expected_ty)) - { + if !expected.iter().any(|expected_ty| { + // This check uses `ValidationMode::Permissive` even in + // strict typechecking because we use this function and + // `expect_type` to require that an operand is a record type + // or an entity type by calling this function with + // `AnyEntity` or `{}` as the expected type. In either case, + // we need to make the check using width subtyping to avoid + // reporting an error every time we see a `GetAttr` on a + // non-empty record. + Type::is_subtype( + self.schema, + actual_ty, + expected_ty, + ValidationMode::Permissive, + ) + }) { type_errors.push(TypeError::expected_one_of_types( expr.clone(), expected.to_vec(), @@ -2544,7 +2075,8 @@ impl<'a> Typechecker<'a> { // defined. .collect::>>() .and_then(|typechecked_types| { - let lub = Type::reduce_to_least_upper_bound(self.schema, &typechecked_types); + let lub = + Type::reduce_to_least_upper_bound(self.schema, &typechecked_types, self.mode); if lub.is_none() { // A type error is generated if we could not find a least // upper bound for the types. The computed least upper bound @@ -2640,6 +2172,17 @@ impl<'a> Typechecker<'a> { type_errors.push(TypeError::arg_validation_error(ext_expr.clone(), msg)); failed = true; } + + if self.mode.is_strict() + && efunc.has_argument_check() + && !args + .iter() + .all(|e| matches!(e.expr_kind(), ExprKind::Lit(_))) + { + type_errors.push(TypeError::non_lit_ext_constructor(ext_expr.clone())); + failed = true; + } + if failed { match typed_arg_exprs(type_errors) { Some(exprs) => TypecheckAnswer::fail( diff --git a/cedar-policy-validator/src/typecheck/test_expr.rs b/cedar-policy-validator/src/typecheck/test_expr.rs index 817927d15..cc3146123 100644 --- a/cedar-policy-validator/src/typecheck/test_expr.rs +++ b/cedar-policy-validator/src/typecheck/test_expr.rs @@ -27,12 +27,13 @@ use smol_str::SmolStr; use crate::{ type_error::TypeError, types::Type, AttributeAccess, AttributesOrContext, EntityType, - NamespaceDefinition, + NamespaceDefinition, ValidationMode, }; use super::test_utils::{ assert_typecheck_fails_empty_schema, assert_typecheck_fails_empty_schema_without_type, - assert_typechecks, assert_typechecks_empty_schema, + assert_typecheck_fails_for_mode, assert_typechecks, assert_typechecks_empty_schema, + assert_typechecks_empty_schema_permissive, assert_typechecks_for_mode, empty_schema_file, }; #[test] @@ -59,7 +60,7 @@ fn slot_in_typechecks() { shape: AttributesOrContext::default(), }; let schema = NamespaceDefinition::new([("typename".into(), etype)], []); - assert_typechecks( + assert_typechecks_for_mode( schema.clone(), Expr::binary_app( BinaryOp::In, @@ -67,8 +68,9 @@ fn slot_in_typechecks() { Expr::slot(SlotId::principal()), ), Type::primitive_boolean(), + ValidationMode::Permissive, ); - assert_typechecks( + assert_typechecks_for_mode( schema, Expr::binary_app( BinaryOp::In, @@ -76,6 +78,7 @@ fn slot_in_typechecks() { Expr::slot(SlotId::resource()), ), Type::primitive_boolean(), + ValidationMode::Permissive, ); } @@ -85,8 +88,12 @@ fn slot_equals_typechecks() { member_of_types: vec![], shape: AttributesOrContext::default(), }; + // These don't typecheck in strict mode because the test_util expression + // typechecker doesn't have access to a schema, so it can't instantiate + // the template slots with appropriate types. Similar policies that pass + // strict typechecking are in the test_policy file. let schema = NamespaceDefinition::new([("typename".into(), etype)], []); - assert_typechecks( + assert_typechecks_for_mode( schema.clone(), Expr::binary_app( BinaryOp::Eq, @@ -94,8 +101,9 @@ fn slot_equals_typechecks() { Expr::slot(SlotId::principal()), ), Type::primitive_boolean(), + ValidationMode::Permissive, ); - assert_typechecks( + assert_typechecks_for_mode( schema, Expr::binary_app( BinaryOp::Eq, @@ -103,6 +111,7 @@ fn slot_equals_typechecks() { Expr::slot(SlotId::resource()), ), Type::primitive_boolean(), + ValidationMode::Permissive, ); } @@ -416,7 +425,7 @@ fn set_eq_is_not_false() { }"#, ) .expect("Expected that schema would parse"); - assert_typechecks( + assert_typechecks_for_mode( schema, Expr::is_eq( Expr::get_attr( @@ -435,6 +444,7 @@ fn set_eq_is_not_false() { ), ), Type::primitive_boolean(), + ValidationMode::Permissive, ); } @@ -515,7 +525,7 @@ fn record_has_typechecks() { } #[test] -fn record_lub_has_typechecks() { +fn record_lub_has_typechecks_strict() { assert_typechecks_empty_schema( Expr::from_str("(if 1 > 0 then {a: 1} else {a: 2}) has a").unwrap(), Type::singleton_boolean(true), @@ -525,44 +535,48 @@ fn record_lub_has_typechecks() { Type::singleton_boolean(false), ); assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: true} else {a: false}) has b").unwrap(), + Type::singleton_boolean(false), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: true} else {a: false}) has a").unwrap(), + Type::singleton_boolean(true), + ); +} + +#[test] +fn record_lub_has_typechecks_permissive() { + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then {a: 1} else {a: 2, b: 3}) has a").unwrap(), Type::singleton_boolean(true), ); - assert_typechecks_empty_schema( + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then {a: 1, b: 2} else {a: 1, c: 2}) has a").unwrap(), Type::singleton_boolean(true), ); - assert_typechecks_empty_schema( + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then {a: 1} else {}) has a").unwrap(), Type::primitive_boolean(), ); - assert_typechecks_empty_schema( + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then {a: 1, b: 2} else {a: 1, c: 2}) has b").unwrap(), Type::primitive_boolean(), ); - assert_typechecks_empty_schema( + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then (if 1 > 0 then {a: 1} else {}) else {}) has a").unwrap(), Type::primitive_boolean(), ); - assert_typechecks_empty_schema( - Expr::from_str("(if 1 > 0 then {a: true} else {a: false}) has b").unwrap(), - Type::singleton_boolean(false), - ); - assert_typechecks_empty_schema( - Expr::from_str("(if 1 > 0 then {a: true} else {a: false}) has a").unwrap(), - Type::singleton_boolean(true), - ); // These cases are imprecise. - assert_typechecks_empty_schema( + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then {a: 1} else {}) has c").unwrap(), Type::primitive_boolean(), ); - assert_typechecks_empty_schema( + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then {a: 1} else {b: 2}) has c").unwrap(), Type::primitive_boolean(), ); - assert_typechecks_empty_schema( + assert_typechecks_empty_schema_permissive( Expr::from_str("(if 1 > 0 then {a: 1} else {a : false}) has a").unwrap(), Type::primitive_boolean(), ); @@ -598,14 +612,18 @@ fn record_get_attr_incompatible() { Expr::record([(attr.clone(), Expr::val(true))]), Expr::record([(attr.clone(), Expr::val(1))]), ); - assert_typecheck_fails_empty_schema_without_type( + + assert_typecheck_fails_for_mode( + empty_schema_file(), Expr::get_attr(if_expr.clone(), attr.clone()), + None, vec![TypeError::unsafe_attribute_access( Expr::get_attr(if_expr, attr.clone()), AttributeAccess::Other(vec![attr]), None, true, )], + crate::ValidationMode::Permissive, ); } @@ -678,16 +696,24 @@ fn record_get_attr_lub_does_not_exist() { } #[test] -fn in_typechecks() { - assert_typechecks_empty_schema( +fn in_typechecks_permissive() { + assert_typechecks_empty_schema_permissive( Expr::is_in(Expr::var(Var::Principal), Expr::var(Var::Resource)), Type::primitive_boolean(), ); } #[test] -fn in_set_typechecks() { +fn in_typechecks() { assert_typechecks_empty_schema( + Expr::is_in(Expr::var(Var::Principal), Expr::var(Var::Principal)), + Type::primitive_boolean(), + ); +} + +#[test] +fn in_set_typechecks_permissive() { + assert_typechecks_empty_schema_permissive( Expr::is_in( Expr::var(Var::Principal), Expr::set([Expr::var(Var::Resource)]), @@ -696,6 +722,17 @@ fn in_set_typechecks() { ); } +#[test] +fn in_set_typechecks_strict() { + assert_typechecks_empty_schema( + Expr::is_in( + Expr::var(Var::Principal), + Expr::set([Expr::var(Var::Principal)]), + ), + Type::primitive_boolean(), + ); +} + #[test] fn in_typecheck_fails() { assert_typecheck_fails_empty_schema( diff --git a/cedar-policy-validator/src/typecheck/test_optional_attributes.rs b/cedar-policy-validator/src/typecheck/test_optional_attributes.rs index 1b9b1fba6..729cf1cf7 100644 --- a/cedar-policy-validator/src/typecheck/test_optional_attributes.rs +++ b/cedar-policy-validator/src/typecheck/test_optional_attributes.rs @@ -640,7 +640,8 @@ fn action_attrs_passing() { "resourceTypes": ["User"] }, "attributes": { - "isReadOnly": true + "isReadOnly": true, + "canUndo": false } }, "edit": { diff --git a/cedar-policy-validator/src/typecheck/test_policy.rs b/cedar-policy-validator/src/typecheck/test_policy.rs index 239000117..9c0f102ca 100644 --- a/cedar-policy-validator/src/typecheck/test_policy.rs +++ b/cedar-policy-validator/src/typecheck/test_policy.rs @@ -29,7 +29,8 @@ use cedar_policy_core::{ use smol_str::SmolStr; use super::test_utils::{ - assert_policy_typecheck_fails, assert_policy_typechecks, assert_typechecks, + assert_policy_typecheck_fails, assert_policy_typecheck_fails_for_mode, + assert_policy_typechecks, assert_policy_typechecks_for_mode, assert_typechecks, with_typechecker_from_schema, }; use crate::{ @@ -37,7 +38,7 @@ use crate::{ typecheck::test_utils::static_to_template, typecheck::PolicyCheck, types::{EntityLUB, Type}, - AttributeAccess, NamespaceDefinition, + AttributeAccess, NamespaceDefinition, ValidationMode, }; fn simple_schema_file() -> NamespaceDefinition { @@ -115,6 +116,10 @@ fn assert_policy_typechecks_simple_schema(p: impl Into>) { assert_policy_typechecks(simple_schema_file(), p) } +fn assert_policy_typechecks_permissive_simple_schema(p: impl Into>) { + assert_policy_typechecks_for_mode(simple_schema_file(), p, ValidationMode::Permissive) +} + fn assert_policy_typecheck_fails_simple_schema( p: impl Into>, expected_type_errors: Vec, @@ -122,6 +127,18 @@ fn assert_policy_typecheck_fails_simple_schema( assert_policy_typecheck_fails(simple_schema_file(), p, expected_type_errors) } +fn assert_policy_typecheck_permissive_fails_simple_schema( + p: impl Into>, + expected_type_errors: Vec, +) { + assert_policy_typecheck_fails_for_mode( + simple_schema_file(), + p, + expected_type_errors, + ValidationMode::Permissive, + ) +} + #[test] fn entity_literal_typechecks() { assert_typechecks_simple_schema( @@ -330,7 +347,7 @@ fn policy_entity_type_action_in_set() { #[test] fn policy_entity_type_principal_in_set() { - assert_policy_typechecks_simple_schema(parse_policy( + assert_policy_typechecks_permissive_simple_schema(parse_policy( Some("0".to_string()), r#"permit(principal, action, resource) when { principal in [User::"admin", Group::"admin"] || true};"# ).expect("Policy should parse.")); @@ -422,7 +439,7 @@ fn policy_entity_has_then_get() { #[test] fn policy_entity_top_has() { - assert_policy_typechecks_simple_schema(parse_policy( + assert_policy_typechecks_permissive_simple_schema(parse_policy( Some("0".to_string()), r#"permit(principal, action, resource) when { (if principal.name == "foo" then principal else resource) has name || true };"#, ).expect("Policy should parse.")); @@ -430,17 +447,17 @@ fn policy_entity_top_has() { #[test] fn entity_lub_access_attribute() { - assert_policy_typechecks_simple_schema(parse_policy( + assert_policy_typechecks_permissive_simple_schema(parse_policy( Some("0".to_string()), - r#"permit(principal, action, resource) when { (if 1 > 0 then User::"alice" else Group::"alice_friends").name like "foo" || true };"# + r#"permit(principal, action, resource) when { (if 1 > 0 then User::"alice" else Group::"alice_friends").name like "foo"};"# ).expect("Policy should parse.")); } #[test] fn entity_lub_no_common_attributes_is_entity() { - assert_policy_typechecks_simple_schema(parse_policy( + assert_policy_typechecks_permissive_simple_schema(parse_policy( Some("0".to_string()), - r#"permit(principal, action, resource) when { principal in (if 1 > 0 then User::"alice" else Photo::"vacation.jpg") || true };"# + r#"permit(principal, action, resource) when { principal in (if 1 > 0 then User::"alice" else Photo::"vacation.jpg")};"# ).expect("Policy should parse.")); } @@ -451,29 +468,19 @@ fn entity_lub_cant_access_attribute_not_shared() { r#"permit(principal, action, resource == Group::"foo") when { (if 1 > 0 then User::"alice" else Photo::"vacation.jpg").name == "bob"};"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_permissive_fails_simple_schema( p, - vec![ - TypeError::unsafe_attribute_access( - Expr::from_str(r#"(if 1 > 0 then User::"alice" else Photo::"vacation.jpg").name"#) - .unwrap(), - AttributeAccess::EntityLUB( - EntityLUB::single_entity("User".parse().unwrap()) - .least_upper_bound(&EntityLUB::single_entity("Photo".parse().unwrap())), - vec!["name".into()], - ), - None, - true, - ), - TypeError::types_must_match( - Expr::from_str(r#"if 1 > 0 then User::"alice" else Photo::"vacation.jpg""#) - .unwrap(), - [ - Type::named_entity_reference_from_str("User"), - Type::named_entity_reference_from_str("Photo"), - ], + vec![TypeError::unsafe_attribute_access( + Expr::from_str(r#"(if 1 > 0 then User::"alice" else Photo::"vacation.jpg").name"#) + .unwrap(), + AttributeAccess::EntityLUB( + EntityLUB::single_entity("User".parse().unwrap()) + .least_upper_bound(&EntityLUB::single_entity("Photo".parse().unwrap())), + vec!["name".into()], ), - ], + None, + true, + )], ); } @@ -497,7 +504,7 @@ fn entity_attribute_recommendation() { #[test] fn entity_lub_no_common_attributes_might_have_declared_attribute() { - assert_policy_typechecks_simple_schema(parse_policy( + assert_policy_typechecks_permissive_simple_schema(parse_policy( Some("0".to_string()), r#"permit(principal, action, resource) when { (if 1 > 0 then User::"alice" else Photo::"vacation.jpg") has age || true };"# ).expect("Policy should parse.")); @@ -510,7 +517,7 @@ fn entity_lub_cant_have_undeclared_attribute() { r#"permit(principal, action, resource) when { (if 1 > 0 then User::"alice" else Photo::"vacation.jpg") has foo};"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_permissive_fails_simple_schema( p.clone(), vec![TypeError::impossible_policy(p.condition())], ); diff --git a/cedar-policy-validator/src/typecheck/test_strict.rs b/cedar-policy-validator/src/typecheck/test_strict.rs index d4914e8ea..34c6fd158 100644 --- a/cedar-policy-validator/src/typecheck/test_strict.rs +++ b/cedar-policy-validator/src/typecheck/test_strict.rs @@ -24,8 +24,8 @@ use std::str::FromStr; use cedar_policy_core::ast::{EntityType, EntityUID, Expr}; use crate::{ - types::{Attributes, RequestEnv, Type}, - SchemaFragment, TypeErrorKind, TypesMustMatch, + types::{Attributes, EffectSet, RequestEnv, Type}, + IncompatibleTypes, SchemaFragment, TypeErrorKind, ValidationMode, }; use super::test_utils::with_typechecker_from_schema; @@ -37,9 +37,11 @@ fn assert_typechecks_strict( e_strict: Expr, expected_type: Type, ) { - with_typechecker_from_schema(schema, |typechecker| { + with_typechecker_from_schema(schema, |mut typechecker| { + typechecker.mode = ValidationMode::Strict; let mut errs = Vec::new(); - let answer = typechecker.typecheck_strict(env, &e, expected_type, &mut errs); + let answer = typechecker.expect_type(env, &EffectSet::new(), &e, expected_type, &mut errs); + assert_eq!(errs, vec![], "Expression should not contain any errors."); match answer { crate::typecheck::TypecheckAnswer::TypecheckSuccess { expr_type, .. } => { @@ -63,9 +65,10 @@ fn assert_strict_type_error( expected_type: Type, expected_error: TypeErrorKind, ) { - with_typechecker_from_schema(schema, |typechecker| { + with_typechecker_from_schema(schema, |mut typechecker| { + typechecker.mode = ValidationMode::Strict; let mut errs = Vec::new(); - let answer = typechecker.typecheck_strict(env, &e, expected_type, &mut errs); + let answer = typechecker.expect_type(env, &EffectSet::new(), &e, expected_type, &mut errs); assert_eq!( errs.into_iter().map(|e| e.kind).collect::>(), @@ -100,7 +103,7 @@ fn assert_types_must_match( e, e_strict, expected_type, - TypeErrorKind::TypesMustMatch(TypesMustMatch { + TypeErrorKind::IncompatibleTypes(IncompatibleTypes { types: unequal_types.into_iter().collect(), }), ) @@ -152,10 +155,12 @@ where #[test] fn strict_typecheck_catches_regular_type_error() { with_simple_schema_and_request(|s, q| { - with_typechecker_from_schema(s, |typechecker| { + with_typechecker_from_schema(s, |mut typechecker| { let mut errs = Vec::new(); - typechecker.typecheck_strict( + typechecker.mode = ValidationMode::Strict; + typechecker.expect_type( &q, + &EffectSet::new(), &Expr::from_str("1 + false").unwrap(), Type::primitive_long(), &mut errs, @@ -232,7 +237,7 @@ fn contains_strict_types_mismatch() { Expr::from_str(r#"[1].contains("test")"#).unwrap(), Expr::from_str(r#"[1].contains("test")"#).unwrap(), Type::primitive_boolean(), - [Type::set(Type::primitive_long()), Type::primitive_string()], + [Type::primitive_long(), Type::primitive_string()], ) }) } @@ -516,7 +521,7 @@ fn test_mul() { Expr::from_str(r#"2*(if 1 == false then 3 else 4)"#).unwrap(), Expr::from_str(r#"2*(if 1 == false then 3 else 4)"#).unwrap(), Type::primitive_long(), - [Type::primitive_long(), Type::primitive_boolean()], + [Type::primitive_long(), Type::singleton_boolean(false)], ); }) } @@ -537,7 +542,7 @@ fn test_like() { Expr::from_str(r#"(if 1 == false then "foo" else "bar") like "bar""#).unwrap(), Expr::from_str(r#"(if 1 == false then "foo" else "bar") like "bar""#).unwrap(), Type::primitive_boolean(), - [Type::primitive_long(), Type::primitive_boolean()], + [Type::primitive_long(), Type::singleton_boolean(false)], ); }) } @@ -570,14 +575,14 @@ fn test_has_attr() { s.clone(), &q, Expr::from_str(r#"{name: "foo"} has bar"#).unwrap(), - Expr::from_str(r#"false"#).unwrap(), + Expr::from_str(r#"{name: "foo"} has bar"#).unwrap(), Type::primitive_boolean(), ); assert_typechecks_strict( s.clone(), &q, Expr::from_str(r#"{name: "foo"} has name"#).unwrap(), - Expr::from_str(r#"true"#).unwrap(), + Expr::from_str(r#"{name: "foo"} has name"#).unwrap(), Type::primitive_boolean(), ); assert_types_must_match( @@ -617,7 +622,7 @@ fn test_extension() { Expr::from_str(r#"ip("192.168.1.0/8").isInRange(if 1 == false then ip("127.0.0.1") else ip("192.168.1.1"))"#).unwrap(), Expr::from_str(r#"ip("192.168.1.0/8").isInRange(if 1 == false then ip("127.0.0.1") else ip("192.168.1.1"))"#).unwrap(), Type::primitive_boolean(), - [Type::primitive_long(), Type::primitive_boolean()] + [Type::primitive_long(), Type::singleton_boolean(false)] ); }) } diff --git a/cedar-policy-validator/src/typecheck/test_utils.rs b/cedar-policy-validator/src/typecheck/test_utils.rs index 995abf8ea..7417b44bf 100644 --- a/cedar-policy-validator/src/typecheck/test_utils.rs +++ b/cedar-policy-validator/src/typecheck/test_utils.rs @@ -105,10 +105,10 @@ pub(crate) fn with_typechecker_from_schema( /// improvement to the typechecker to return more specific types. pub(crate) fn assert_types_eq(schema: &ValidatorSchema, expected: &Type, actual: &Type) { assert!( - Type::is_subtype(schema, expected, actual), + Type::is_subtype(schema, expected, actual, ValidationMode::Permissive), "Type equality assertion failed: the expected type is not a subtype of the actual type.\nexpected: {:#?}\nactual: {:#?}", expected, actual); assert!( - Type::is_subtype(schema, actual, expected), + Type::is_subtype(schema, actual, expected, ValidationMode::Permissive), "Type equality assertion failed: the actual type is not a subtype of the expected type.\nexpected: {:#?}\nactual: {:#?}", expected, actual); } @@ -140,7 +140,16 @@ pub(crate) fn assert_policy_typechecks( schema: impl TryInto, policy: impl Into>, ) { - with_typechecker_from_schema(schema, |typechecker| { + assert_policy_typechecks_for_mode(schema, policy, ValidationMode::Strict) +} + +pub(crate) fn assert_policy_typechecks_for_mode( + schema: impl TryInto, + policy: impl Into>, + mode: ValidationMode, +) { + with_typechecker_from_schema(schema, |mut typechecker| { + typechecker.mode = mode; let mut type_errors: HashSet = HashSet::new(); let typechecked = typechecker.typecheck_policy(&policy.into(), &mut type_errors); assert_eq!(type_errors, HashSet::new(), "Did not expect any errors."); @@ -153,7 +162,22 @@ pub(crate) fn assert_policy_typecheck_fails( policy: impl Into>, expected_type_errors: Vec, ) { - with_typechecker_from_schema(schema, |typechecker| { + assert_policy_typecheck_fails_for_mode( + schema, + policy, + expected_type_errors, + ValidationMode::Strict, + ) +} + +pub(crate) fn assert_policy_typecheck_fails_for_mode( + schema: impl TryInto, + policy: impl Into>, + expected_type_errors: Vec, + mode: ValidationMode, +) { + with_typechecker_from_schema(schema, |mut typechecker| { + typechecker.mode = mode; let mut type_errors: HashSet = HashSet::new(); let typechecked = typechecker.typecheck_policy(&policy.into(), &mut type_errors); assert_expected_type_errors(&expected_type_errors, &type_errors); @@ -168,7 +192,17 @@ pub(crate) fn assert_typechecks( expr: Expr, expected: Type, ) { - with_typechecker_from_schema(schema, |typechecker| { + assert_typechecks_for_mode(schema, expr, expected, ValidationMode::Strict); +} + +pub(crate) fn assert_typechecks_for_mode( + schema: impl TryInto, + expr: Expr, + expected: Type, + mode: ValidationMode, +) { + with_typechecker_from_schema(schema, |mut typechecker| { + typechecker.mode = mode; let mut type_errors = HashSet::new(); let actual = typechecker.typecheck_expr(&expr, &mut type_errors); assert_types_eq( @@ -209,7 +243,24 @@ pub(crate) fn assert_typecheck_fails( expected_ty: Option, expected_type_errors: Vec, ) { - with_typechecker_from_schema(schema, |typechecker| { + assert_typecheck_fails_for_mode( + schema, + expr, + expected_ty, + expected_type_errors, + ValidationMode::Strict, + ) +} + +pub(crate) fn assert_typecheck_fails_for_mode( + schema: impl TryInto, + expr: Expr, + expected_ty: Option, + expected_type_errors: Vec, + mode: ValidationMode, +) { + with_typechecker_from_schema(schema, |mut typechecker| { + typechecker.mode = mode; let mut type_errors = HashSet::new(); let actual = typechecker.typecheck_expr(&expr, &mut type_errors); let actual_ty = match actual { @@ -243,6 +294,15 @@ pub(crate) fn assert_typechecks_empty_schema(expr: Expr, expected: Type) { assert_typechecks(empty_schema_file(), expr, expected) } +pub(crate) fn assert_typechecks_empty_schema_permissive(expr: Expr, expected: Type) { + assert_typechecks_for_mode( + empty_schema_file(), + expr, + expected, + ValidationMode::Permissive, + ) +} + pub(crate) fn assert_typecheck_fails_empty_schema( expr: Expr, expected: Type, diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index 03d05dc46..ad4221712 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -26,6 +26,8 @@ use std::{ use cedar_policy_core::ast::{EntityType, EntityUID, Expr, ExprShapeOnly, Name}; +use crate::ValidationMode; + use super::schema::{ is_action_entity_type, ValidatorActionId, ValidatorEntityType, ValidatorSchema, }; @@ -205,7 +207,12 @@ impl Type { /// `schema` so that the declared attributes for named entity types can be /// retrieved. This is used to determine subtyping between a named entity /// type and a record type. - pub(crate) fn is_subtype(schema: &ValidatorSchema, ty0: &Type, ty1: &Type) -> bool { + pub(crate) fn is_subtype( + schema: &ValidatorSchema, + ty0: &Type, + ty1: &Type, + mode: ValidationMode, + ) -> bool { match (ty0, ty1) { // Never is a subtype of every type. (Type::Never, _) => true, @@ -234,14 +241,14 @@ impl Type { element_type: e_ty1, }, ) => match (e_ty0, e_ty1) { - (Some(e_ty0), Some(e_ty1)) => Type::is_subtype(schema, e_ty0, e_ty1), + (Some(e_ty0), Some(e_ty1)) => Type::is_subtype(schema, e_ty0, e_ty1, mode), (Some(_), None) => true, (None, Some(_)) => false, (None, None) => true, }, (Type::EntityOrRecord(rk0), Type::EntityOrRecord(rk1)) => { - EntityRecordKind::is_subtype(schema, rk0, rk1) + EntityRecordKind::is_subtype(schema, rk0, rk1, mode) } // Subtypes between extension types only occurs when the extension @@ -259,10 +266,11 @@ impl Type { schema: &ValidatorSchema, ty0: &Type, ty1: &Type, + mode: ValidationMode, ) -> Option { match (ty0, ty1) { - _ if Type::is_subtype(schema, ty0, ty1) => Some(ty1.clone()), - _ if Type::is_subtype(schema, ty1, ty0) => Some(ty0.clone()), + _ if Type::is_subtype(schema, ty0, ty1, mode) => Some(ty1.clone()), + _ if Type::is_subtype(schema, ty1, ty0, mode) => Some(ty0.clone()), (Type::True | Type::False, Type::True | Type::False) => Some(Type::primitive_boolean()), @@ -284,10 +292,10 @@ impl Type { Type::Set { element_type: Some(te1), }, - ) => Some(Type::set(Type::least_upper_bound(schema, te0, te1)?)), + ) => Some(Type::set(Type::least_upper_bound(schema, te0, te1, mode)?)), (Type::EntityOrRecord(rk0), Type::EntityOrRecord(rk1)) => Some(Type::EntityOrRecord( - EntityRecordKind::least_upper_bound(schema, rk0, rk1)?, + EntityRecordKind::least_upper_bound(schema, rk0, rk1, mode)?, )), _ => None, @@ -324,9 +332,10 @@ impl Type { pub(crate) fn reduce_to_least_upper_bound( schema: &ValidatorSchema, tys: &[Type], + mode: ValidationMode, ) -> Option { tys.iter().try_fold(Type::Never, |lub, next| { - Type::least_upper_bound(schema, &lub, next) + Type::least_upper_bound(schema, &lub, next, mode) }) } @@ -759,7 +768,19 @@ impl EntityLUB { .expect("Invariant violated: EntityLUB set must be non-empty."), ); lub_element_attributes.fold(arbitrary_first, |acc, elem| { - Attributes::least_upper_bound(schema, &acc, &Attributes::with_attributes(elem)) + // Use the permissive version of least upper bound here for two + // reasons. First, when in permissive mode, the attributes least + // upper bound can never fail. We could call the main lub function + // with an unwrap, but this avoids a chance at a panic. Second, when + // in strict mode, an entity LUB will only ever have a single + // element, so that LUB can never fail, and the strict + // attributes lub is the same as permissive if there is only one + // attribute. + Attributes::permissive_least_upper_bound( + schema, + &acc, + &Attributes::with_attributes(elem), + ) }) } @@ -865,7 +886,12 @@ impl Attributes { self.attrs.get(attr) } - pub(crate) fn is_subtype(&self, schema: &ValidatorSchema, other: &Attributes) -> bool { + pub(crate) fn is_subtype( + &self, + schema: &ValidatorSchema, + other: &Attributes, + mode: ValidationMode, + ) -> bool { // For a one record type to subtype another, all the attributes of the // second must be present in the first, and each attribute types must // subtype the corresponding attribute type. If an attribute in the @@ -876,7 +902,7 @@ impl Attributes { .get(k) .map(|self_ty| { (self_ty.is_required || !other_ty.is_required) - && Type::is_subtype(schema, &self_ty.attr_type, &other_ty.attr_type) + && Type::is_subtype(schema, &self_ty.attr_type, &other_ty.attr_type, mode) }) .unwrap_or(false) }) @@ -889,23 +915,75 @@ impl Attributes { &self, schema: &ValidatorSchema, other: &Attributes, + mode: ValidationMode, ) -> bool { other.attrs.keys().collect::>() == self.attrs.keys().collect::>() - && self.is_subtype(schema, other) + && self.is_subtype(schema, other, mode) } pub(crate) fn least_upper_bound( schema: &ValidatorSchema, attrs0: &Attributes, attrs1: &Attributes, - ) -> Attributes { - Attributes::with_attributes(attrs0.attrs.iter().filter_map(move |(attr, ty0)| { + mode: ValidationMode, + ) -> Option { + if mode.is_strict() { + Self::strict_least_upper_bound(schema, attrs0, attrs1) + } else { + Some(Self::permissive_least_upper_bound(schema, attrs0, attrs1)) + } + } + + fn attributes_lub_iter<'a>( + schema: &'a ValidatorSchema, + attrs0: &'a Attributes, + attrs1: &'a Attributes, + mode: ValidationMode, + ) -> impl Iterator> + 'a { + attrs0.attrs.iter().map(move |(attr, ty0)| { let ty1 = attrs1.attrs.get(attr)?; - Type::least_upper_bound(schema, &ty0.attr_type, &ty1.attr_type).map(|lub| { + Type::least_upper_bound(schema, &ty0.attr_type, &ty1.attr_type, mode).map(|lub| { let is_lub_required = ty0.is_required && ty1.is_required; (attr.clone(), AttributeType::new(lub, is_lub_required)) }) - })) + }) + } + + pub(crate) fn strict_least_upper_bound( + schema: &ValidatorSchema, + attrs0: &Attributes, + attrs1: &Attributes, + ) -> Option { + if attrs0.keys().collect::>() != attrs1.keys().collect::>() { + return None; + } + + let mut any_err = false; + let attrs: Vec<_> = + Self::attributes_lub_iter(schema, attrs0, attrs1, ValidationMode::Strict) + .filter_map(|e| { + if e.is_none() { + any_err = true; + } + e + }) + .collect(); + + if any_err { + None + } else { + Some(Attributes::with_attributes(attrs)) + } + } + + pub(crate) fn permissive_least_upper_bound( + schema: &ValidatorSchema, + attrs0: &Attributes, + attrs1: &Attributes, + ) -> Attributes { + Attributes::with_attributes( + Self::attributes_lub_iter(schema, attrs0, attrs1, ValidationMode::Permissive).flatten(), + ) } } @@ -1035,6 +1113,7 @@ impl EntityRecordKind { schema: &ValidatorSchema, rk0: &EntityRecordKind, rk1: &EntityRecordKind, + mode: ValidationMode, ) -> Option { use EntityRecordKind::*; match (rk0, rk1) { @@ -1048,7 +1127,7 @@ impl EntityRecordKind { open_attributes: open1, }, ) => { - let attrs = Attributes::least_upper_bound(schema, attrs0, attrs1); + let attrs = Attributes::least_upper_bound(schema, attrs0, attrs1, mode)?; // Even though this function will never be called when the // records are in a subtype relation, it is still possible that @@ -1074,15 +1153,60 @@ impl EntityRecordKind { open_attributes, }) } - //We cannot take upper bounds of action entities because may_have_attr assumes the list of attrs it complete - (ActionEntity { .. }, ActionEntity { .. }) => Some(AnyEntity), - (Entity(lub0), Entity(lub1)) => Some(Entity(lub0.least_upper_bound(lub1))), + //We cannot, in general, have precise upper bounds between action + //entities because `may_have_attr` assumes the list of attrs is + //complete. + ( + ActionEntity { + name: action_type1, + attrs: attrs1, + }, + ActionEntity { + name: action_type2, + attrs: attrs2, + }, + ) => { + if action_type1 == action_type2 { + // Same action type. Ensure that the actions have the same + // attributes. Computing the LUB under strict mode disables + // means that the LUB does not exist if either record has as + // an attribute that does not exist in the other, so we know + // that list of attributes is complete, as is assumed by + // `may_have_attr`. As long as actions have empty an + // attribute records, the LUB no-ops, allowing for LUBs + // between actions with the same action entity type even in + // strict validation mode. + Attributes::least_upper_bound(schema, attrs1, attrs2, ValidationMode::Strict) + .map(|attrs| ActionEntity { + name: action_type1.clone(), + attrs, + }) + } else if mode.is_strict() { + None + } else { + Some(AnyEntity) + } + } + (Entity(lub0), Entity(lub1)) => { + if mode.is_strict() && lub0 != lub1 { + None + } else { + Some(Entity(lub0.least_upper_bound(lub1))) + } + } - (AnyEntity, AnyEntity) - | (AnyEntity, Entity(_)) + (AnyEntity, AnyEntity) => Some(AnyEntity), + + (AnyEntity, Entity(_)) | (Entity(_), AnyEntity) | (AnyEntity, ActionEntity { .. }) - | (ActionEntity { .. }, AnyEntity) => Some(AnyEntity), + | (ActionEntity { .. }, AnyEntity) => { + if mode.is_strict() { + None + } else { + Some(AnyEntity) + } + } // Entity and record types do not have a least upper bound to avoid // a non-terminating case. @@ -1092,7 +1216,13 @@ impl EntityRecordKind { //Likewise, we can't mix action entities and records (ActionEntity { .. }, Record { .. }) | (Record { .. }, ActionEntity { .. }) => None, //Action entities can be mixed with Entities. In this case, the LUB is AnyEntity - (ActionEntity { .. }, Entity(_)) | (Entity(_), ActionEntity { .. }) => Some(AnyEntity), + (ActionEntity { .. }, Entity(_)) | (Entity(_), ActionEntity { .. }) => { + if mode.is_strict() { + None + } else { + Some(AnyEntity) + } + } } } @@ -1102,6 +1232,7 @@ impl EntityRecordKind { schema: &ValidatorSchema, rk0: &EntityRecordKind, rk1: &EntityRecordKind, + mode: ValidationMode, ) -> bool { use EntityRecordKind::*; match (rk0, rk1) { @@ -1124,13 +1255,23 @@ impl EntityRecordKind { // there may be attributes in `rk0` that are not listed in // `rk1`. When `rk1` is closed, a subtype of `rk1` may not have // any attributes that are not listed in `rk1`, so we apply - // depth subtyping only. - && ((open1.is_open() && attrs0.is_subtype(schema, attrs1)) - || attrs0.is_subtype_depth_only(schema, attrs1)) + // depth subtyping only. We apply this same restriction in + // strict mode, i.e., strict mode applies depth subtyping but + // not width subtyping. + && ((open1.is_open() && !mode.is_strict() && attrs0.is_subtype(schema, attrs1, mode)) + || attrs0.is_subtype_depth_only(schema, attrs1, mode)) } (ActionEntity { .. }, ActionEntity { .. }) => false, - (Entity(lub0), Entity(lub1)) => lub0.is_subtype(lub1), - (Entity(_) | ActionEntity { .. } | AnyEntity, AnyEntity) => true, + (Entity(lub0), Entity(lub1)) => { + if mode.is_strict() { + lub0 == lub1 + } else { + lub0.is_subtype(lub1) + } + } + + (AnyEntity, AnyEntity) => true, + (Entity(_) | ActionEntity { .. }, AnyEntity) => !mode.is_strict(), // Entities cannot subtype records because their LUB is undefined to // avoid a non-terminating case. @@ -1278,7 +1419,7 @@ mod test { fn assert_least_upper_bound(schema: ValidatorSchema, lhs: Type, rhs: Type, lub: Option) { assert_eq!( - Type::least_upper_bound(&schema, &lhs, &rhs), + Type::least_upper_bound(&schema, &lhs, &rhs, ValidationMode::Permissive), lub, "assert_least_upper_bound({:?}, {:?}, {:?})", lhs, @@ -1294,7 +1435,7 @@ mod test { lub_names: &[&str], lub_attrs: &[(&str, Type)], ) { - let lub = Type::least_upper_bound(&schema, &lhs, &rhs); + let lub = Type::least_upper_bound(&schema, &lhs, &rhs, ValidationMode::Permissive); match lub { Some(Type::EntityOrRecord(EntityRecordKind::Entity(entity_lub))) => { assert_eq!( @@ -1974,6 +2115,7 @@ mod test { &ValidatorSchema::empty(), &Type::named_entity_reference_from_str("Foo"), &Type::named_entity_reference_from_str("Bar"), + ValidationMode::Permissive, ) .expect("Expected a least upper bound to exist."), r#"{"type":"Union","elements":[{"type":"Entity","name":"Bar"},{"type":"Entity","name":"Foo"}]}"#, diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 138c7fc37..ffdc0dcd3 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -37,6 +37,7 @@ - The `Response::new()` constructor now expects a `Vec` as its third argument. - Improved validation error messages for access to undeclared attributes and unsafe access to optional attributes to report the target of the access (fix #175). +- Implements RFC #19, making validation slightly more strict, but more explainable. ## 2.3.3