From 91c00b58ed4dc0ff2ae03e3c9b590c013604c22e Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 1 Feb 2024 17:14:10 +0000 Subject: [PATCH 1/3] customizable error comparison in DRT --- cedar-drt/fuzz/src/lib.rs | 38 +++++++++++++++++++++----------- cedar-drt/src/cedar_test_impl.rs | 23 +++++++++++++++++++ cedar-drt/src/dafny_java_impl.rs | 4 ++++ cedar-drt/src/lean_impl.rs | 4 ++++ 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/cedar-drt/fuzz/src/lib.rs b/cedar-drt/fuzz/src/lib.rs index e4214cf46..f71231221 100644 --- a/cedar-drt/fuzz/src/lib.rs +++ b/cedar-drt/fuzz/src/lib.rs @@ -20,10 +20,12 @@ mod prt; pub use dump::*; pub use prt::*; -use cedar_drt::{time_function, CedarTestImplementation, RUST_AUTH_MSG, RUST_VALIDATION_MSG}; +use cedar_drt::{ + time_function, CedarTestImplementation, ErrorComparisonMode, RUST_AUTH_MSG, RUST_VALIDATION_MSG, +}; use cedar_policy::frontend::is_authorized::InterfaceResponse; use cedar_policy_core::ast; -use cedar_policy_core::authorizer::{Authorizer, Diagnostics, Response}; +use cedar_policy_core::authorizer::{Authorizer, AuthorizationError, Diagnostics, Response}; use cedar_policy_core::entities::{Entities, NoEntitiesSchema, TCComputation}; use cedar_policy_core::evaluator::{EvaluationErrorKind, Evaluator}; use cedar_policy_core::extensions::Extensions; @@ -119,15 +121,28 @@ pub fn run_auth_test( } } Ok(definitional_res) => { - // Otherwise, the definitional engine should return a result that matches `rust_res`. - let rust_res_for_comparison: cedar_policy::Response = Response { - diagnostics: Diagnostics { - errors: Vec::new(), - ..rust_res.clone().diagnostics + let rust_res_for_comparison: cedar_policy::Response = match custom_impl + .error_comparison_mode() + { + ErrorComparisonMode::Ignore => Response { + diagnostics: Diagnostics { + errors: Vec::new(), + reason: rust_res.diagnostics.reason.clone(), + }, + ..rust_res + } + .into(), + ErrorComparisonMode::PolicyIds => Response { + diagnostics: Diagnostics { + errors: rust_res.diagnostics.errors.map(|err| match err { + AuthorizationError::PolicyEvaluationError { id, .. } => format!("{id}"), + }), + reason: rust_res.diagnostics.reason.clone(), + }, + ..rust_res }, - ..rust_res - } - .into(); + ErrorComparisonMode::Full => rust_res.clone(), + }.into(); assert_eq!( InterfaceResponse::from(rust_res_for_comparison), definitional_res, @@ -136,9 +151,6 @@ pub fn run_auth_test( &entities ); rust_res - - // TODO(#69): Our current definitional engine does not return authorization - // errors, so those are not checked for equality. } } } diff --git a/cedar-drt/src/cedar_test_impl.rs b/cedar-drt/src/cedar_test_impl.rs index 04d92ac08..00c2b1d30 100644 --- a/cedar-drt/src/cedar_test_impl.rs +++ b/cedar-drt/src/cedar_test_impl.rs @@ -74,4 +74,27 @@ pub trait CedarTestImplementation { policies: &PolicySet, mode: ValidationMode, ) -> InterfaceResult; + + /// `ErrorComparisonMode` that should be used for this `CedarTestImplementation` + fn error_comparison_mode(&self) -> ErrorComparisonMode; +} + +/// Specifies how errors coming from a `CedarTestImplementation` should be +/// compared against errors coming from the Rust implementation. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum ErrorComparisonMode { + /// Don't compare errors at all; the `CedarTestImplementation` is not + /// expected to produce errors matching the Rust implementation's errors in + /// any way. + /// In fact, the `CedarTestImplementation` will be expected to never report + /// errors. + Ignore, + /// The `CedarTestImplementation` is expected to produce "error messages" that + /// are actually just the id of the erroring policy. This will be compared to + /// ensure that the `CedarTestImplementation` agrees with the Rust + /// implementation on which policies produce errors. + PolicyIds, + /// The `CedarTestImplementation` is expected to produce error messages that + /// exactly match the Rust implementation's error messages' `Display` text. + Full, } diff --git a/cedar-drt/src/dafny_java_impl.rs b/cedar-drt/src/dafny_java_impl.rs index 1fce4d155..9426d5208 100644 --- a/cedar-drt/src/dafny_java_impl.rs +++ b/cedar-drt/src/dafny_java_impl.rs @@ -414,6 +414,10 @@ impl<'j> CedarTestImplementation for JavaDefinitionalEngine<'j> { ) -> InterfaceResult { Ok(self.validate(schema, policies, mode)) } + + fn error_comparison_mode(&self) -> ErrorComparisonMode { + ErrorComparisonMode::Ignore + } } /// Implementation of the trait used for integration testing. diff --git a/cedar-drt/src/lean_impl.rs b/cedar-drt/src/lean_impl.rs index c791ace9b..01ea47fce 100644 --- a/cedar-drt/src/lean_impl.rs +++ b/cedar-drt/src/lean_impl.rs @@ -324,6 +324,10 @@ impl CedarTestImplementation for LeanDefinitionalEngine { ); self.validate(schema, policies) } + + fn error_comparison_mode(&self) -> ErrorComparisonMode { + ErrorComparisonMode::PolicyIds + } } /// Implementation of the trait used for integration testing. The integration From 448894a901bcded8689ef85d1f9ecbf1d142d739 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 1 Feb 2024 17:55:04 +0000 Subject: [PATCH 2/3] cargo fmt --- cedar-drt/fuzz/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cedar-drt/fuzz/src/lib.rs b/cedar-drt/fuzz/src/lib.rs index f71231221..9f78ed8d4 100644 --- a/cedar-drt/fuzz/src/lib.rs +++ b/cedar-drt/fuzz/src/lib.rs @@ -25,7 +25,7 @@ use cedar_drt::{ }; use cedar_policy::frontend::is_authorized::InterfaceResponse; use cedar_policy_core::ast; -use cedar_policy_core::authorizer::{Authorizer, AuthorizationError, Diagnostics, Response}; +use cedar_policy_core::authorizer::{AuthorizationError, Authorizer, Diagnostics, Response}; use cedar_policy_core::entities::{Entities, NoEntitiesSchema, TCComputation}; use cedar_policy_core::evaluator::{EvaluationErrorKind, Evaluator}; use cedar_policy_core::extensions::Extensions; @@ -142,7 +142,8 @@ pub fn run_auth_test( ..rust_res }, ErrorComparisonMode::Full => rust_res.clone(), - }.into(); + } + .into(); assert_eq!( InterfaceResponse::from(rust_res_for_comparison), definitional_res, From 4cf3d50fb13a211fc99ee4f534baf5c5a60fc14f Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 1 Feb 2024 18:53:20 +0000 Subject: [PATCH 3/3] fix? --- cedar-drt/fuzz/src/lib.rs | 65 ++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/cedar-drt/fuzz/src/lib.rs b/cedar-drt/fuzz/src/lib.rs index 9f78ed8d4..7a6859b6e 100644 --- a/cedar-drt/fuzz/src/lib.rs +++ b/cedar-drt/fuzz/src/lib.rs @@ -23,15 +23,16 @@ pub use prt::*; use cedar_drt::{ time_function, CedarTestImplementation, ErrorComparisonMode, RUST_AUTH_MSG, RUST_VALIDATION_MSG, }; -use cedar_policy::frontend::is_authorized::InterfaceResponse; +use cedar_policy::{frontend::is_authorized::InterfaceResponse, PolicyId}; use cedar_policy_core::ast; -use cedar_policy_core::authorizer::{AuthorizationError, Authorizer, Diagnostics, Response}; +use cedar_policy_core::authorizer::{AuthorizationError, Authorizer, Response}; use cedar_policy_core::entities::{Entities, NoEntitiesSchema, TCComputation}; use cedar_policy_core::evaluator::{EvaluationErrorKind, Evaluator}; use cedar_policy_core::extensions::Extensions; pub use cedar_policy_validator::{ValidationErrorKind, ValidationMode, Validator, ValidatorSchema}; use libfuzzer_sys::arbitrary::{self, Unstructured}; use log::info; +use std::collections::HashSet; /// Compare the behavior of the evaluator in `cedar-policy` against a custom Cedar /// implementation. Panics if the two do not agree. `expr` is the expression to @@ -121,35 +122,41 @@ pub fn run_auth_test( } } Ok(definitional_res) => { - let rust_res_for_comparison: cedar_policy::Response = match custom_impl - .error_comparison_mode() - { - ErrorComparisonMode::Ignore => Response { - diagnostics: Diagnostics { - errors: Vec::new(), - reason: rust_res.diagnostics.reason.clone(), - }, - ..rust_res - } - .into(), - ErrorComparisonMode::PolicyIds => Response { - diagnostics: Diagnostics { - errors: rust_res.diagnostics.errors.map(|err| match err { - AuthorizationError::PolicyEvaluationError { id, .. } => format!("{id}"), - }), - reason: rust_res.diagnostics.reason.clone(), - }, - ..rust_res - }, - ErrorComparisonMode::Full => rust_res.clone(), - } - .into(); + let rust_res_for_comparison: InterfaceResponse = { + let errors = match custom_impl.error_comparison_mode() { + ErrorComparisonMode::Ignore => HashSet::new(), + ErrorComparisonMode::PolicyIds => rust_res + .diagnostics + .errors + .iter() + .map(|err| match err { + AuthorizationError::PolicyEvaluationError { id, .. } => { + format!("{id}") + } + }) + .collect(), + ErrorComparisonMode::Full => rust_res + .diagnostics + .errors + .iter() + .map(ToString::to_string) + .collect(), + }; + InterfaceResponse::new( + rust_res.decision, + rust_res + .diagnostics + .reason + .iter() + .map(|id| PolicyId::new(id.clone())) + .collect(), + errors, + ) + }; assert_eq!( - InterfaceResponse::from(rust_res_for_comparison), - definitional_res, + rust_res_for_comparison, definitional_res, "Mismatch for {request}\nPolicies:\n{}\nEntities:\n{}", - &policies, - &entities + &policies, &entities ); rust_res }