From c16e9320bb0c7e1e9a2e32bfbf8ae9bdd70b204c Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Fri, 22 Mar 2024 10:12:32 -0400 Subject: [PATCH] Move `ImpossiblePolicy` from error to warning (#716) Signed-off-by: Kesha Hietala --- cedar-policy-validator/src/lib.rs | 55 ++-- cedar-policy-validator/src/rbac.rs | 281 ++++++++---------- cedar-policy-validator/src/str_checks.rs | 57 +--- cedar-policy-validator/src/type_error.rs | 12 +- cedar-policy-validator/src/typecheck.rs | 31 +- .../src/typecheck/test_namespace.rs | 14 +- .../src/typecheck/test_optional_attributes.rs | 33 +- .../src/typecheck/test_partial.rs | 51 +++- .../src/typecheck/test_policy.rs | 111 ++++--- .../src/typecheck/test_utils.rs | 72 ++++- .../src/validation_result.rs | 74 ++++- cedar-policy/CHANGELOG.md | 3 + cedar-policy/src/frontend/validate.rs | 4 +- 13 files changed, 463 insertions(+), 335 deletions(-) diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index a2cbefb68..a94e3747d 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -37,7 +37,7 @@ pub use schema::*; mod schema_file_format; pub use schema_file_format::*; mod str_checks; -pub use str_checks::{confusable_string_checks, ValidationWarning, ValidationWarningKind}; +pub use str_checks::confusable_string_checks; mod type_error; pub use type_error::*; pub mod human_schema; @@ -91,34 +91,41 @@ impl Validator { } /// Validate all templates, links, and static policies in a policy set. - /// Return an iterator of policy notes associated with each policy id. + /// Return a `ValidationResult`. pub fn validate<'a>( &'a self, policies: &'a PolicySet, mode: ValidationMode, ) -> ValidationResult<'a> { - let template_and_static_policy_errs = policies + let validate_policy_results: (Vec<_>, Vec<_>) = policies .all_templates() - .flat_map(|p| self.validate_policy(p, mode)); + .map(|p| self.validate_policy(p, mode)) + .unzip(); + let template_and_static_policy_errs = validate_policy_results.0.into_iter().flatten(); + let template_and_static_policy_warnings = validate_policy_results.1.into_iter().flatten(); let link_errs = policies .policies() .filter_map(|p| self.validate_slots(p, mode)) .flatten(); ValidationResult::new( template_and_static_policy_errs.chain(link_errs), - confusable_string_checks(policies.all_templates()), + template_and_static_policy_warnings + .chain(confusable_string_checks(policies.all_templates())), ) } /// Run all validations against a single static policy or template (note /// that Core `Template` includes static policies as well), gathering all - /// validation notes together in the returned iterator. + /// validation errors and warnings in the returned iterators. fn validate_policy<'a>( &'a self, p: &'a Template, mode: ValidationMode, - ) -> impl Iterator + 'a { - if mode.is_partial() { + ) -> ( + impl Iterator>, + impl Iterator>, + ) { + let validation_errors = if mode.is_partial() { // We skip `validate_entity_types`, `validate_action_ids`, and // `validate_action_application` passes for partial schema // validation because there may be arbitrary extra entity types and @@ -141,12 +148,13 @@ impl Validator { ) } .into_iter() - .flatten() - .chain(self.typecheck_policy(p, mode)) + .flatten(); + let (type_errors, warnings) = self.typecheck_policy(p, mode); + (validation_errors.chain(type_errors), warnings) } /// Run relevant validations against a single template-linked policy, - /// gathering all validation notes together in the returned iterator. + /// gathering all validation errors together in the returned iterator. fn validate_slots<'a>( &'a self, p: &'a Policy, @@ -185,14 +193,27 @@ impl Validator { &'a self, t: &'a Template, mode: ValidationMode, - ) -> impl Iterator + 'a { + ) -> ( + impl Iterator>, + impl Iterator>, + ) { let typecheck = Typechecker::new(&self.schema, mode); let mut type_errors = HashSet::new(); - typecheck.typecheck_policy(t, &mut type_errors); - type_errors.into_iter().map(|type_error| { - let (kind, location) = type_error.kind_and_location(); - ValidationError::with_policy_id(t.id(), location, ValidationErrorKind::type_error(kind)) - }) + let mut warnings = HashSet::new(); + typecheck.typecheck_policy(t, &mut type_errors, &mut warnings); + ( + type_errors.into_iter().map(|type_error| { + let (kind, location) = type_error.kind_and_location(); + ValidationError::with_policy_id( + t.id(), + location, + ValidationErrorKind::type_error(kind), + ) + }), + warnings + .into_iter() + .map(|kind| ValidationWarning::with_policy_id(t.id(), None, kind)), + ) } } diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index fb9473d40..cf5294d9d 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -30,7 +30,7 @@ use super::{ }; impl Validator { - /// Generate UnrecognizedEntityType notes for every entity type in the + /// Generate `UnrecognizedEntityType` error for every entity type in the /// expression that could not also be found in the schema. pub(crate) fn validate_entity_types<'a>( &'a self, @@ -72,7 +72,7 @@ impl Validator { })) } - /// Generate UnrecognizedActionId notes for every entity id with an action + /// Generate `UnrecognizedActionId` error for every entity id with an action /// entity type where the id could not be found in the actions list from the /// schema. pub(crate) fn validate_action_ids<'a>( @@ -109,8 +109,8 @@ impl Validator { }) } - /// Generate UnrecognizedEntityType or UnspecifiedEntityError notes for every - /// entity type in the slot environment that is either not in the schema, + /// Generate `UnrecognizedEntityType` or `UnspecifiedEntityError` error for + /// every entity type in the slot environment that is either not in the schema, /// or unspecified. pub(crate) fn validate_entity_types_in_slots<'a>( &'a self, @@ -429,9 +429,10 @@ mod test { use super::*; use crate::{ - err::*, schema_file_format::NamespaceDefinition, schema_file_format::*, TypeErrorKind, - UnrecognizedActionId, UnrecognizedEntityType, UnspecifiedEntityError, ValidationError, - ValidationMode, Validator, + err::*, + schema_file_format::{NamespaceDefinition, *}, + UnrecognizedActionId, UnrecognizedEntityType, UnspecifiedEntityError, ValidationMode, + ValidationWarningKind, Validator, }; use cool_asserts::assert_matches; @@ -582,7 +583,7 @@ mod test { let validate = Validator::new(singleton_schema); assert!( validate.validate_entity_types(&policy).next().is_none(), - "Did not expect any validation notes." + "Did not expect any validation errors." ); Ok(()) @@ -702,7 +703,7 @@ mod test { let validate = Validator::new(singleton_schema); assert!( validate.validate_action_ids(&policy).next().is_none(), - "Did not expect any validation notes." + "Did not expect any validation errors." ); Ok(()) @@ -1149,6 +1150,56 @@ mod test { (principal_euid, action_euid, resource_euid, schema) } + #[track_caller] // report the caller's location as the location of the panic, not the location in this function + fn assert_validate_policy_succeeds(validator: &Validator, policy: &Template) { + assert!( + validator + .validate_policy(&policy, ValidationMode::default()) + .0 + .next() + .is_none(), + "Did not expect any validation errors." + ); + assert!( + validator + .validate_policy(&policy, ValidationMode::default()) + .1 + .next() + .is_none(), + "Did not expect any validation warnings." + ); + } + + #[track_caller] // report the caller's location as the location of the panic, not the location in this function + fn assert_validate_policy_fails( + validator: &Validator, + policy: &Template, + expected: Vec, + ) { + assert_eq!( + validator + .validate_policy(policy, ValidationMode::default()) + .0 + .map(|e| { e.into_location_and_error_kind().1 }) + .collect::>(), + expected, + "Unexpected validation errors." + ); + } + + #[track_caller] // report the caller's location as the location of the panic, not the location in this function + fn assert_validate_policy_flags_impossible_policy(validator: &Validator, policy: &Template) { + assert_eq!( + validator + .validate_policy(policy, ValidationMode::default()) + .1 + .map(|w| { w.to_kind_and_location().1 }) + .collect::>(), + vec![ValidationWarningKind::ImpossiblePolicy], + "Unexpected validation warnings." + ); + } + #[test] fn validate_action_apply_correct() -> Result<()> { let (principal, action, resource, schema) = schema_with_single_principal_action_resource(); @@ -1163,15 +1214,8 @@ mod test { Expr::val(true), ); - let validate = Validator::new(schema); - assert!( - validate - .validate_policy(&policy, ValidationMode::default()) - .next() - .is_none(), - "Did not expect any validation notes." - ); - + let validator = Validator::new(schema); + assert_validate_policy_succeeds(&validator, &policy); Ok(()) } @@ -1263,15 +1307,8 @@ mod test { Expr::val(true), ); - let validate = Validator::new(schema); - assert!( - validate - .validate_policy(&policy, ValidationMode::default()) - .next() - .is_none(), - "Did not expect any validation notes." - ); - + let validator = Validator::new(schema); + assert_validate_policy_succeeds(&validator, &policy); Ok(()) } @@ -1308,14 +1345,8 @@ mod test { let policy = parse_policy_template(None, "permit(principal is bar, action, resource);").unwrap(); - let validate = Validator::new(schema.clone()); - assert!( - validate - .validate_policy(&policy, ValidationMode::default()) - .next() - .is_none(), - "Did not expect any validation notes." - ); + let validator = Validator::new(schema); + assert_validate_policy_succeeds(&validator, &policy); let policy = parse_policy_template( None, @@ -1323,14 +1354,7 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema); - assert!( - validate - .validate_policy(&policy, ValidationMode::default()) - .next() - .is_none(), - "Did not expect any validation notes." - ); + assert_validate_policy_succeeds(&validator, &policy); } #[test] @@ -1340,17 +1364,15 @@ mod test { let policy = parse_policy_template(None, "permit(principal is baz, action, resource);").unwrap(); - let validate = Validator::new(schema.clone()); - assert_eq!( - validate - .validate_policy(&policy, ValidationMode::default()) - .map(|e| { e.into_location_and_error_kind().1 }) - .collect::>(), - vec![ - ValidationErrorKind::invalid_action_application(false, false), - ValidationErrorKind::TypeError(TypeErrorKind::ImpossiblePolicy) - ], + let validator = Validator::new(schema); + assert_validate_policy_fails( + &validator, + &policy, + vec![ValidationErrorKind::invalid_action_application( + false, false, + )], ); + assert_validate_policy_flags_impossible_policy(&validator, &policy); let policy = parse_policy_template( None, @@ -1358,19 +1380,16 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema.clone()); - assert_eq!( - validate - .validate_policy(&policy, ValidationMode::default()) - .map(|e| { e.into_location_and_error_kind().1 }) - .collect::>(), + assert_validate_policy_fails( + &validator, + &policy, vec![ ValidationErrorKind::unrecognized_entity_type("faz".into(), Some("baz".into())), ValidationErrorKind::unrecognized_entity_type("biz".into(), Some("baz".into())), ValidationErrorKind::invalid_action_application(false, false), - ValidationErrorKind::TypeError(TypeErrorKind::ImpossiblePolicy) ], ); + assert_validate_policy_flags_impossible_policy(&validator, &policy); let policy = parse_policy_template( None, @@ -1378,17 +1397,14 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema.clone()); - assert_eq!( - validate - .validate_policy(&policy, ValidationMode::default()) - .map(|e| { e.into_location_and_error_kind().1 }) - .collect::>(), - vec![ - ValidationErrorKind::invalid_action_application(false, false), - ValidationErrorKind::TypeError(TypeErrorKind::ImpossiblePolicy) - ], + assert_validate_policy_fails( + &validator, + &policy, + vec![ValidationErrorKind::invalid_action_application( + false, false, + )], ); + assert_validate_policy_flags_impossible_policy(&validator, &policy); } #[test] @@ -1398,14 +1414,8 @@ mod test { let policy = parse_policy_template(None, "permit(principal, action, resource is baz);").unwrap(); - let validate = Validator::new(schema.clone()); - assert!( - validate - .validate_policy(&policy, ValidationMode::default()) - .next() - .is_none(), - "Did not expect any validation notes." - ); + let validator = Validator::new(schema); + assert_validate_policy_succeeds(&validator, &policy); let policy = parse_policy_template( None, @@ -1413,14 +1423,7 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema); - assert!( - validate - .validate_policy(&policy, ValidationMode::default()) - .next() - .is_none(), - "Did not expect any validation notes." - ); + assert_validate_policy_succeeds(&validator, &policy); } #[test] @@ -1430,17 +1433,15 @@ mod test { let policy = parse_policy_template(None, "permit(principal, action, resource is bar);").unwrap(); - let validate = Validator::new(schema.clone()); - assert_eq!( - validate - .validate_policy(&policy, ValidationMode::default()) - .map(|e| { e.into_location_and_error_kind().1 }) - .collect::>(), - vec![ - ValidationErrorKind::invalid_action_application(false, false), - ValidationErrorKind::TypeError(TypeErrorKind::ImpossiblePolicy) - ], + let validator = Validator::new(schema); + assert_validate_policy_fails( + &validator, + &policy, + vec![ValidationErrorKind::invalid_action_application( + false, false, + )], ); + assert_validate_policy_flags_impossible_policy(&validator, &policy); let policy = parse_policy_template( None, @@ -1448,17 +1449,14 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema.clone()); - assert_eq!( - validate - .validate_policy(&policy, ValidationMode::default()) - .map(|e| { e.into_location_and_error_kind().1 }) - .collect::>(), - vec![ - ValidationErrorKind::invalid_action_application(false, false), - ValidationErrorKind::TypeError(TypeErrorKind::ImpossiblePolicy) - ], + assert_validate_policy_fails( + &validator, + &policy, + vec![ValidationErrorKind::invalid_action_application( + false, false, + )], ); + assert_validate_policy_flags_impossible_policy(&validator, &policy); let policy = parse_policy_template( None, @@ -1466,19 +1464,16 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema.clone()); - assert_eq!( - validate - .validate_policy(&policy, ValidationMode::default()) - .map(|e| { e.into_location_and_error_kind().1 }) - .collect::>(), + assert_validate_policy_fails( + &validator, + &policy, vec![ ValidationErrorKind::unrecognized_entity_type("faz".into(), Some("baz".into())), ValidationErrorKind::unrecognized_entity_type("biz".into(), Some("baz".into())), ValidationErrorKind::invalid_action_application(false, false), - ValidationErrorKind::TypeError(TypeErrorKind::ImpossiblePolicy) ], ); + assert_validate_policy_flags_impossible_policy(&validator, &policy); } #[test] @@ -1490,17 +1485,16 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema.clone()); - assert_eq!( - validate - .validate_policy(&policy, ValidationMode::default()) - .map(|e| { e.into_location_and_error_kind().1 }) - .collect::>(), - vec![ - ValidationErrorKind::unrecognized_entity_type("biz".into(), Some("baz".into())), - ValidationErrorKind::TypeError(TypeErrorKind::ImpossiblePolicy) - ], + let validator = Validator::new(schema); + assert_validate_policy_fails( + &validator, + &policy, + vec![ValidationErrorKind::unrecognized_entity_type( + "biz".into(), + Some("baz".into()), + )], ); + assert_validate_policy_flags_impossible_policy(&validator, &policy); } #[test] @@ -1601,17 +1595,8 @@ mod test { Expr::val(true), ); - let validate = Validator::new(schema); - - let notes: Vec = validate - .validate_policy(&policy, ValidationMode::default()) - .collect(); - assert!( - notes.is_empty(), - "Expected empty validation notes, saw {:?}", - notes - ); - + let validator = Validator::new(schema); + assert_validate_policy_succeeds(&validator, &policy); Ok(()) } @@ -1715,13 +1700,9 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema); + let validator = Validator::new(schema); let (template, _) = Template::link_static_policy(policy); - let mut errs = validate.validate_policy(&template, ValidationMode::default()); - assert!( - errs.next().is_none(), - "Did not expect any validation errors." - ); + assert_validate_policy_succeeds(&validator, &template); } #[test] @@ -1750,13 +1731,9 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema); + let validator = Validator::new(schema); let (template, _) = Template::link_static_policy(policy); - let mut errs = validate.validate_policy(&template, ValidationMode::default()); - assert!( - errs.next().is_none(), - "Did not expect any validation errors." - ); + assert_validate_policy_succeeds(&validator, &template); } #[test] @@ -1780,16 +1757,9 @@ mod test { ) .unwrap(); - let validate = Validator::new(schema); + let validator = Validator::new(schema); let (template, _) = Template::link_static_policy(policy); - let errs = validate.validate_policy(&template, ValidationMode::default()); - assert_eq!( - errs.map(|e| e.into_location_and_error_kind().1) - .collect::>(), - vec![ValidationErrorKind::type_error( - TypeErrorKind::ImpossiblePolicy - )] - ); + assert_validate_policy_flags_impossible_policy(&validator, &template); } } @@ -1821,6 +1791,7 @@ mod partial_schema { let validate = Validator::new(schema); let errs = validate .validate_policy(&template, crate::ValidationMode::Partial) + .0 .collect::>(); assert_eq!(errs, vec![], "Did not expect any validation errors."); } diff --git a/cedar-policy-validator/src/str_checks.rs b/cedar-policy-validator/src/str_checks.rs index 3e1b87f3d..9aa17ba39 100644 --- a/cedar-policy-validator/src/str_checks.rs +++ b/cedar-policy-validator/src/str_checks.rs @@ -15,68 +15,13 @@ */ use cedar_policy_core::ast::{Pattern, Template}; -use miette::Diagnostic; -use thiserror::Error; use crate::expr_iterator::expr_text; use crate::expr_iterator::TextKind; -use crate::SourceLocation; +use crate::{SourceLocation, ValidationWarning, ValidationWarningKind}; use unicode_security::GeneralSecurityProfile; use unicode_security::MixedScript; -/// Returned by the standalone `confusable_string_checker` function, which checks a policy set for potentially confusing/obfuscating text. -#[derive(Debug, Clone)] -pub struct ValidationWarning<'a> { - location: SourceLocation<'a>, - kind: ValidationWarningKind, -} - -impl<'a> ValidationWarning<'a> { - pub fn location(&self) -> &SourceLocation<'a> { - &self.location - } - - pub fn kind(&self) -> &ValidationWarningKind { - &self.kind - } - - pub fn to_kind_and_location(self) -> (SourceLocation<'a>, ValidationWarningKind) { - (self.location, self.kind) - } -} - -impl std::fmt::Display for ValidationWarning<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!( - f, - "validation warning on policy `{}`: {}", - self.location.policy_id(), - self.kind - ) - } -} - -#[derive(Debug, Clone, PartialEq, Diagnostic, Error, Eq)] -#[non_exhaustive] -#[diagnostic(severity(Warning))] -pub enum ValidationWarningKind { - /// A string contains mixed scripts. Different scripts can contain visually similar characters which may be confused for each other. - #[error("string `\"{0}\"` contains mixed scripts")] - MixedScriptString(String), - /// A string contains BIDI control characters. These can be used to create crafted pieces of code that obfuscate true control flow. - #[error("string `\"{0}\"` contains BIDI control characters")] - BidiCharsInString(String), - /// An id contains BIDI control characters. These can be used to create crafted pieces of code that obfuscate true control flow. - #[error("identifier `{0}` contains BIDI control characters")] - BidiCharsInIdentifier(String), - /// An id contains mixed scripts. This can cause characters to be confused for each other. - #[error("identifier `{0}` contains mixed scripts")] - MixedScriptIdentifier(String), - /// An id contains characters that fall outside of the General Security Profile for Identifiers. We recommend adhering to this if possible. See Unicode® Technical Standard #39 for more info. - #[error("identifier `{0}` contains characters that fall outside of the General Security Profile for Identifiers")] - ConfusableIdentifier(String), -} - /// Perform identifier and string safety checks. pub fn confusable_string_checks<'a>( p: impl Iterator, diff --git a/cedar-policy-validator/src/type_error.rs b/cedar-policy-validator/src/type_error.rs index f2818e57f..6bb3d3796 100644 --- a/cedar-policy-validator/src/type_error.rs +++ b/cedar-policy-validator/src/type_error.rs @@ -167,14 +167,6 @@ impl TypeError { } } - pub(crate) fn impossible_policy(on_expr: Expr) -> Self { - Self { - on_expr: Some(on_expr), - source_loc: None, - kind: TypeErrorKind::ImpossiblePolicy, - } - } - pub(crate) fn undefined_extension(on_expr: Expr, name: String) -> Self { Self { on_expr: Some(on_expr), @@ -265,6 +257,10 @@ pub enum TypeErrorKind { #[error( "policy is impossible: the policy expression evaluates to false for all valid requests" )] + #[deprecated( + since = "3.2.0", + note = "`ImpossiblePolicy` is now a warning rather than an error" + )] ImpossiblePolicy, /// Undefined extension function. #[error("undefined extension function: {}", .0.name)] diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 9650764db..e52dc489b 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -43,7 +43,7 @@ use crate::{ types::{ AttributeType, Effect, EffectSet, EntityRecordKind, OpenTag, Primitive, RequestEnv, Type, }, - AttributeAccess, UnexpectedTypeHelp, ValidationMode, + AttributeAccess, UnexpectedTypeHelp, ValidationMode, ValidationWarningKind, }; use super::type_error::TypeError; @@ -261,13 +261,21 @@ impl<'a> Typechecker<'a> { } } - /// The main entry point for typechecking policies. This method takes a - /// policy and a mutable `Vec` used to output type errors. Typechecking - /// ensures that the policy expression has type boolean. If typechecking - /// succeeds, then the method will return true, and no items will be - /// 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 { + /// The main entry point for typechecking policies. Checks that the policy + /// expression has type boolean. If typechecking succeeds, then the method + /// will return true, and no items will be added to the output list. + /// Otherwise, the function returns false and the `type_errors` list is + /// populated with any errors encountered while typechecking. Note that it + /// is possible for _no_ errors to be added in this case because the + /// relevant error is expected to be added by a different pass. Finally, + /// warnings may be added to the `warnings` list, although these will not + /// impact the boolean return value. + pub fn typecheck_policy( + &self, + t: &Template, + type_errors: &mut HashSet, + warnings: &mut HashSet, + ) -> bool { let typecheck_answers = self.typecheck_by_request_env(t); // consolidate the results from each query environment @@ -290,11 +298,10 @@ impl<'a> Typechecker<'a> { // If every policy typechecked with type false, then the policy cannot // possibly apply to any request. if all_false { - type_errors.insert(TypeError::impossible_policy(t.condition())); - false - } else { - all_succ + warnings.insert(ValidationWarningKind::ImpossiblePolicy); } + + all_succ } /// Secondary entry point for typechecking requests. This method takes a policy and diff --git a/cedar-policy-validator/src/typecheck/test_namespace.rs b/cedar-policy-validator/src/typecheck/test_namespace.rs index a5b9b04b5..1b5fd8369 100644 --- a/cedar-policy-validator/src/typecheck/test_namespace.rs +++ b/cedar-policy-validator/src/typecheck/test_namespace.rs @@ -30,13 +30,13 @@ use cedar_policy_core::{ }; use super::test_utils::{ - assert_policy_typecheck_fails, assert_policy_typechecks, assert_typecheck_fails, - assert_typechecks, + assert_policy_typecheck_fails, assert_policy_typecheck_warns, assert_policy_typechecks, + assert_typecheck_fails, assert_typechecks, }; use crate::{ type_error::TypeError, types::{EntityLUB, Type}, - AttributeAccess, SchemaError, SchemaFragment, ValidatorSchema, + AttributeAccess, SchemaError, SchemaFragment, ValidationWarningKind, ValidatorSchema, }; fn namespaced_entity_type_schema() -> SchemaFragment { @@ -561,10 +561,10 @@ fn multi_namespace_action_eq() { r#"permit(principal, action, resource) when { NS1::Action::"Action" == NS2::Action::"Action" };"#, ) .unwrap(); - assert_policy_typecheck_fails( + assert_policy_typecheck_warns( schema.clone(), policy.clone(), - vec![TypeError::impossible_policy(policy.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -621,10 +621,10 @@ fn multi_namespace_action_in() { r#"permit(principal, action in NS4::Action::"Group", resource);"#, ) .unwrap(); - assert_policy_typecheck_fails( + assert_policy_typecheck_warns( schema.clone(), policy.clone(), - vec![TypeError::impossible_policy(policy.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } diff --git a/cedar-policy-validator/src/typecheck/test_optional_attributes.rs b/cedar-policy-validator/src/typecheck/test_optional_attributes.rs index 31e72a870..436a66fe9 100644 --- a/cedar-policy-validator/src/typecheck/test_optional_attributes.rs +++ b/cedar-policy-validator/src/typecheck/test_optional_attributes.rs @@ -20,17 +20,19 @@ // GRCOV_STOP_COVERAGE use cedar_policy_core::{ - ast::{BinaryOp, EntityUID, Expr, StaticPolicy, Var}, + ast::{Expr, StaticPolicy, Var}, parser::parse_policy, }; use smol_str::SmolStr; use crate::{ type_error::TypeError, types::EntityLUB, AttributeAccess, NamespaceDefinition, - NamespaceDefinitionWithActionAttributes, + NamespaceDefinitionWithActionAttributes, ValidationWarningKind, }; -use super::test_utils::{assert_policy_typecheck_fails, assert_policy_typechecks}; +use super::test_utils::{ + assert_policy_typecheck_fails, assert_policy_typecheck_warns, assert_policy_typechecks, +}; fn schema_with_optionals() -> NamespaceDefinition { serde_json::from_str::( @@ -776,32 +778,21 @@ fn action_attrs_failing() { )], ); - // Doesn't fail do to imprecision in ActionEntity LUB computation requiring `may_have_attr` to return true for ActionEntity types - + // No error is returned, but the typechecker identifies that `action has ""` + // is always false. let failing_policy = parse_policy( Some("0".to_string()), r#"permit(principal, action == Action::"view", resource) when { action has "" };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails( + assert_policy_typecheck_warns( schema.clone(), failing_policy, - vec![TypeError::impossible_policy(Expr::and( - Expr::and( - Expr::and( - Expr::val(true), - Expr::binary_app( - BinaryOp::Eq, - Expr::var(Var::Action), - Expr::val(EntityUID::with_eid_and_type("Action", "view").unwrap()), - ), - ), - Expr::val(true), - ), - Expr::has_attr(Expr::var(Var::Action), "".into()), - ))], + vec![ValidationWarningKind::ImpossiblePolicy], ); + // Fails because OtherNamespace::Action::"view" is not defined in the schema. + // However, this will be detected by a different pass, so no error is reported. let failing_policy = parse_policy( Some("0".to_string()), r#" @@ -816,5 +807,5 @@ fn action_attrs_failing() { "#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails(schema, failing_policy, vec![]); //fails because OtherNamespace::Action::"view" doesn't have defined attributes + assert_policy_typecheck_fails(schema, failing_policy, vec![]); } diff --git a/cedar-policy-validator/src/typecheck/test_partial.rs b/cedar-policy-validator/src/typecheck/test_partial.rs index 09afcb88c..c64c32e00 100644 --- a/cedar-policy-validator/src/typecheck/test_partial.rs +++ b/cedar-policy-validator/src/typecheck/test_partial.rs @@ -8,11 +8,14 @@ use std::collections::HashSet; use cedar_policy_core::ast::{Expr, Template, Var}; use cedar_policy_core::{ast::StaticPolicy, parser::parse_policy}; -use crate::typecheck::test_utils::assert_expected_type_errors; +use crate::typecheck::test_utils::{assert_expected_type_errors, assert_expected_warnings}; use crate::typecheck::Typechecker; use crate::types::{EntityLUB, Type}; use crate::UnexpectedTypeHelp; -use crate::{AttributeAccess, NamespaceDefinition, TypeError, ValidationMode, ValidatorSchema}; +use crate::{ + AttributeAccess, NamespaceDefinition, TypeError, ValidationMode, ValidationWarningKind, + ValidatorSchema, +}; use super::test_utils::empty_schema_file; @@ -24,16 +27,18 @@ pub(crate) fn assert_partial_typecheck( let schema = schema.try_into().expect("Failed to construct schema."); let typechecker = Typechecker::new(&schema, ValidationMode::Partial); let mut type_errors: HashSet = HashSet::new(); + let mut warnings: HashSet = HashSet::new(); let typechecked = typechecker.typecheck_policy( &Template::link_static_policy(policy.clone()).0, &mut type_errors, + &mut warnings, ); assert_eq!(type_errors, HashSet::new(), "Did not expect any errors."); assert!(typechecked, "Expected that policy would typecheck."); } #[track_caller] // report the caller's location as the location of the panic, not the location in this function -pub(crate) fn assert_partial_typecheck_fail( +pub(crate) fn assert_partial_typecheck_fails( schema: impl TryInto, policy: StaticPolicy, expected_type_errors: Vec, @@ -41,14 +46,38 @@ pub(crate) fn assert_partial_typecheck_fail( let schema = schema.try_into().expect("Failed to construct schema."); let typechecker = Typechecker::new(&schema, ValidationMode::Partial); let mut type_errors: HashSet = HashSet::new(); + let mut warnings: HashSet = HashSet::new(); let typechecked = typechecker.typecheck_policy( &Template::link_static_policy(policy.clone()).0, &mut type_errors, + &mut warnings, ); assert_expected_type_errors(&expected_type_errors, &type_errors); assert!(!typechecked, "Expected that policy would not typecheck."); } +#[track_caller] // report the caller's location as the location of the panic, not the location in this function +pub(crate) fn assert_partial_typecheck_warns( + schema: impl TryInto, + policy: StaticPolicy, + expected_warnings: Vec, +) { + let schema = schema.try_into().expect("Failed to construct schema."); + let typechecker = Typechecker::new(&schema, ValidationMode::Partial); + let mut type_errors: HashSet = HashSet::new(); + let mut warnings: HashSet = HashSet::new(); + let typechecked = typechecker.typecheck_policy( + &Template::link_static_policy(policy.clone()).0, + &mut type_errors, + &mut warnings, + ); + assert_expected_warnings(&expected_warnings, &warnings); + assert!( + typechecked, + "Expected that policy would typecheck (with warnings)." + ); +} + #[track_caller] // report the caller's location as the location of the panic, not the location in this function pub(crate) fn assert_typechecks_empty_schema(policy: StaticPolicy) { assert_partial_typecheck(empty_schema_file(), policy) @@ -59,7 +88,15 @@ pub(crate) fn assert_typecheck_fails_empty_schema( policy: StaticPolicy, expected_type_errors: Vec, ) { - assert_partial_typecheck_fail(empty_schema_file(), policy, expected_type_errors) + assert_partial_typecheck_fails(empty_schema_file(), policy, expected_type_errors) +} + +#[track_caller] // report the caller's location as the location of the panic, not the location in this function +pub(crate) fn assert_typecheck_warns_empty_schema( + policy: StaticPolicy, + expected_warnings: Vec, +) { + assert_partial_typecheck_warns(empty_schema_file(), policy, expected_warnings) } mod passes_empty_schema { @@ -418,9 +455,9 @@ mod fails_empty_schema { r#"permit(principal, action, resource) when { resource.bar && false };"#, ) .unwrap(); - assert_typecheck_fails_empty_schema( + assert_typecheck_warns_empty_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ) } @@ -502,7 +539,7 @@ pub(crate) fn assert_typecheck_fails_partial_schema( policy: StaticPolicy, expected_type_errors: Vec, ) { - assert_partial_typecheck_fail(partial_schema_file(), policy, expected_type_errors) + assert_partial_typecheck_fails(partial_schema_file(), policy, expected_type_errors) } mod passes_partial_schema { diff --git a/cedar-policy-validator/src/typecheck/test_policy.rs b/cedar-policy-validator/src/typecheck/test_policy.rs index aa0b58916..fb321f401 100644 --- a/cedar-policy-validator/src/typecheck/test_policy.rs +++ b/cedar-policy-validator/src/typecheck/test_policy.rs @@ -30,15 +30,15 @@ use smol_str::SmolStr; use super::test_utils::{ assert_policy_typecheck_fails, assert_policy_typecheck_fails_for_mode, + assert_policy_typecheck_warns, assert_policy_typecheck_warns_for_mode, assert_policy_typechecks, assert_policy_typechecks_for_mode, assert_typechecks, with_typechecker_from_schema, }; use crate::{ type_error::TypeError, - typecheck::test_utils::static_to_template, - typecheck::PolicyCheck, + typecheck::{test_utils::static_to_template, PolicyCheck}, types::{EntityLUB, Type}, - AttributeAccess, NamespaceDefinition, ValidationMode, + AttributeAccess, NamespaceDefinition, ValidationMode, ValidationWarningKind, }; fn simple_schema_file() -> NamespaceDefinition { @@ -131,6 +131,14 @@ fn assert_policy_typecheck_fails_simple_schema( assert_policy_typecheck_fails(simple_schema_file(), p, expected_type_errors) } +#[track_caller] // report the caller's location as the location of the panic, not the location in this function +fn assert_policy_typecheck_warns_simple_schema( + p: impl Into>, + expected_warnings: Vec, +) { + assert_policy_typecheck_warns(simple_schema_file(), p, expected_warnings) +} + #[track_caller] // report the caller's location as the location of the panic, not the location in this function fn assert_policy_typecheck_permissive_fails_simple_schema( p: impl Into>, @@ -144,6 +152,19 @@ fn assert_policy_typecheck_permissive_fails_simple_schema( ) } +#[track_caller] // report the caller's location as the location of the panic, not the location in this function +fn assert_policy_typecheck_permissive_warns_simple_schema( + p: impl Into>, + expected_warnings: Vec, +) { + assert_policy_typecheck_warns_for_mode( + simple_schema_file(), + p, + expected_warnings, + ValidationMode::Permissive, + ) +} + #[test] fn entity_literal_typechecks() { assert_typechecks_simple_schema( @@ -381,9 +402,9 @@ fn policy_impossible_head() { r#"permit(principal == Group::"foo", action == Action::"delete_group", resource);"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -394,9 +415,9 @@ fn policy_impossible_literal_euids() { r#"permit(principal, action, resource) when { Group::"foo" in User::"bar" };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -407,9 +428,9 @@ fn policy_impossible_not_has() { r#"permit(principal, action, resource) when { ! ({name: "alice"} has name)};"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -428,9 +449,9 @@ fn policy_in_action_impossible() { r#"permit(principal, action, resource) when { User::"alice" in [action] };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let p = parse_policy( @@ -438,9 +459,9 @@ fn policy_in_action_impossible() { r#"permit(principal, action, resource) when { User::"alice" in [Action::"view_photo"] };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let p = parse_policy( @@ -448,9 +469,9 @@ fn policy_in_action_impossible() { r#"permit(principal, action, resource) when { principal in [action] };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let p = parse_policy( @@ -458,9 +479,9 @@ fn policy_in_action_impossible() { r#"permit(principal, action, resource) when { principal in action };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let p = parse_policy( @@ -468,9 +489,9 @@ fn policy_in_action_impossible() { r#"permit(principal, action, resource) when { principal in Action::"view_photo" };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let p = parse_policy( @@ -478,9 +499,9 @@ fn policy_in_action_impossible() { r#"permit(principal, action, resource) when { principal in [Action::"view_photo", Action::"delete_group"] };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let p = parse_policy( @@ -488,9 +509,9 @@ fn policy_in_action_impossible() { r#"permit(principal, action, resource) when { principal in [Action::"view_photo", Photo::"bar"] };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_permissive_fails_simple_schema( + assert_policy_typecheck_permissive_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -501,9 +522,9 @@ fn policy_action_in_impossible() { r#"permit(principal, action, resource) when { action in [User::"alice"] };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -595,9 +616,9 @@ 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_permissive_fails_simple_schema( + assert_policy_typecheck_permissive_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -620,14 +641,14 @@ fn is_typechecks_singleton() { #[test] fn is_impossible() { let p = parse_policy(None, r#"permit(principal is Photo, action, resource);"#).unwrap(); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let p = parse_policy(None, r#"permit(principal, action, resource is User);"#).unwrap(); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -653,9 +674,9 @@ fn is_entity_lub() { "#, ) .unwrap(); - assert_policy_typecheck_permissive_fails_simple_schema( + assert_policy_typecheck_permissive_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -677,9 +698,9 @@ fn is_action() { "#, ) .unwrap(); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( p.clone(), - vec![TypeError::impossible_policy(p.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -829,10 +850,10 @@ fn action_groups() { r#"permit(principal, action, resource) when { action in Entity::"group" };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails( + assert_policy_typecheck_warns( schema.clone(), policy.clone(), - vec![TypeError::impossible_policy(policy.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let policy = parse_policy( @@ -840,10 +861,10 @@ fn action_groups() { r#"permit(principal, action, resource) when { action in Entity::"act" };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails( + assert_policy_typecheck_warns( schema.clone(), policy.clone(), - vec![TypeError::impossible_policy(policy.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let policy = parse_policy( @@ -851,10 +872,10 @@ fn action_groups() { r#"permit(principal, action, resource) when { Entity::"group" in action };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails( + assert_policy_typecheck_warns( schema.clone(), policy.clone(), - vec![TypeError::impossible_policy(policy.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); let policy = parse_policy( @@ -862,10 +883,10 @@ fn action_groups() { r#"permit(principal, action, resource) when { Entity::"act" in action };"#, ) .expect("Policy should parse."); - assert_policy_typecheck_fails( + assert_policy_typecheck_warns( schema, policy.clone(), - vec![TypeError::impossible_policy(policy.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } @@ -1086,9 +1107,9 @@ mod templates { r#"permit(principal == ?principal, action, resource) when { false };"#, ) .unwrap(); - assert_policy_typecheck_fails_simple_schema( + assert_policy_typecheck_warns_simple_schema( template.clone(), - vec![TypeError::impossible_policy(template.condition())], + vec![ValidationWarningKind::ImpossiblePolicy], ); } } diff --git a/cedar-policy-validator/src/typecheck/test_utils.rs b/cedar-policy-validator/src/typecheck/test_utils.rs index 2fa37d691..5ea2298e2 100644 --- a/cedar-policy-validator/src/typecheck/test_utils.rs +++ b/cedar-policy-validator/src/typecheck/test_utils.rs @@ -34,7 +34,8 @@ use crate::{ schema::ACTION_ENTITY_TYPE, type_error::TypeError, types::{EffectSet, OpenTag, RequestEnv, Type}, - NamespaceDefinition, UnexpectedTypeHelp, ValidationMode, ValidatorSchema, + NamespaceDefinition, UnexpectedTypeHelp, ValidationMode, ValidationWarningKind, + ValidatorSchema, }; impl TypeError { @@ -148,6 +149,31 @@ pub(crate) fn assert_expected_type_errors(expected: &Vec, actual: &Ha ); } +/// Assert that every `ValidationWarningKind` in the expected list of warnings +/// appears in the expected list of warnings, and that the expected number of +/// warnings were generated. +#[track_caller] // report the caller's location as the location of the panic, not the location in this function +pub(crate) fn assert_expected_warnings( + expected: &Vec, + actual: &HashSet, +) { + expected.iter().for_each(|expected| { + assert!( + actual.contains(expected), + "Expected generated warnings to contain {:#?}, but warning was not found. The following warnings were generated: {:#?}", + expected, + actual + ); + }); + assert_eq!( + expected.len(), + actual.len(), + "Unexpected warnings generated. Expected {:#?}, saw {:#?}.", + expected, + actual, + ); +} + #[track_caller] // report the caller's location as the location of the panic, not the location in this function pub(crate) fn assert_policy_typechecks( schema: impl TryInto, @@ -166,7 +192,8 @@ pub(crate) fn assert_policy_typechecks_for_mode( let policy: Arc