From 02f099c73eb2abb2b9fc097ac05fc68f735a550c Mon Sep 17 00:00:00 2001 From: John Kastner Date: Tue, 14 Nov 2023 10:51:04 -0500 Subject: [PATCH 1/5] Entities attributes and context with open attributes --- cedar-policy-cli/Cargo.toml | 5 + cedar-policy-cli/src/lib.rs | 19 +- cedar-policy-cli/tests/sample.rs | 1 + cedar-policy-core/src/entities.rs | 11 + cedar-policy-core/src/entities/conformance.rs | 10 +- .../src/entities/json/context.rs | 1 + .../src/entities/json/entities.rs | 21 +- cedar-policy-core/src/entities/json/schema.rs | 7 + .../src/entities/json/schema_types.rs | 41 +++- cedar-policy-core/src/entities/json/value.rs | 19 +- cedar-policy-validator/Cargo.toml | 3 + cedar-policy-validator/src/coreschema.rs | 4 + cedar-policy-validator/src/err.rs | 2 +- cedar-policy-validator/src/lib.rs | 14 ++ cedar-policy-validator/src/schema.rs | 51 ++-- cedar-policy-validator/src/schema/action.rs | 12 +- .../src/schema/entity_type.rs | 9 +- .../src/schema/namespace_def.rs | 13 +- .../src/schema_file_format.rs | 10 +- cedar-policy-validator/src/typecheck.rs | 23 +- .../src/typecheck/test_expr.rs | 2 +- .../src/typecheck/test_partial.rs | 230 ++++++++++++++++++ .../src/typecheck/test_policy.rs | 13 + .../src/typecheck/test_strict.rs | 4 +- .../src/typecheck/test_utils.rs | 22 +- cedar-policy-validator/src/types.rs | 75 +++--- cedar-policy/Cargo.toml | 3 +- cedar-policy/src/api.rs | 5 + cedar-policy/src/tests.rs | 155 ++++++------ 29 files changed, 602 insertions(+), 183 deletions(-) create mode 100644 cedar-policy-validator/src/typecheck/test_partial.rs diff --git a/cedar-policy-cli/Cargo.toml b/cedar-policy-cli/Cargo.toml index 8e7413a075..0cebea4bc9 100644 --- a/cedar-policy-cli/Cargo.toml +++ b/cedar-policy-cli/Cargo.toml @@ -19,6 +19,11 @@ serde_json = "1.0" miette = { version = "5.9.0", features = ["fancy"] } thiserror = "1.0" +[features] +default = [] +experimental = ["partial-validate"] +partial-validate = ["cedar-policy/partial-validate"] + [dev-dependencies] assert_cmd = "2.0" tempfile = "3" diff --git a/cedar-policy-cli/src/lib.rs b/cedar-policy-cli/src/lib.rs index ebd338cc5c..b22ae436c6 100644 --- a/cedar-policy-cli/src/lib.rs +++ b/cedar-policy-cli/src/lib.rs @@ -111,6 +111,11 @@ pub struct ValidateArgs { /// Report a validation failure for non-fatal warnings #[arg(long)] pub deny_warnings: bool, + /// Validate the policy using partial schema validation. This option is + /// experimental and will cause the CLI to exit if it was not built with the + /// experimental `partial-validate` feature enabled. + #[arg(long = "partial-validate")] + pub partial_validate: bool, } #[derive(Args, Debug)] @@ -432,6 +437,18 @@ pub fn check_parse(args: &CheckParseArgs) -> CedarExitCode { } pub fn validate(args: &ValidateArgs) -> CedarExitCode { + let mode = if args.partial_validate { + #[cfg(not(feature = "partial-validate"))] + { + println!("Error: arguments include the experimental option `--partial-validate`, but this executable was not built with `partial-validate` experimental feature enabled"); + return CedarExitCode::Failure; + } + #[cfg(feature = "partial-validate")] + ValidationMode::Partial + } else { + ValidationMode::default() + }; + let pset = match read_policy_set(Some(&args.policies_file)) { Ok(pset) => pset, Err(e) => { @@ -449,7 +466,7 @@ pub fn validate(args: &ValidateArgs) -> CedarExitCode { }; let validator = Validator::new(schema); - let result = validator.validate(&pset, ValidationMode::default()); + let result = validator.validate(&pset, mode); let exit_code = if !result.validation_passed() || (args.deny_warnings && !result.validation_passed_without_warnings()) diff --git a/cedar-policy-cli/tests/sample.rs b/cedar-policy-cli/tests/sample.rs index 2378ddfa94..77872569e9 100644 --- a/cedar-policy-cli/tests/sample.rs +++ b/cedar-policy-cli/tests/sample.rs @@ -446,6 +446,7 @@ fn run_validate_test(policies_file: &str, schema_file: &str, exit_code: CedarExi schema_file: schema_file.into(), policies_file: policies_file.into(), deny_warnings: false, + partial_validate: false, }; let output = validate(&cmd); assert_eq!(exit_code, output, "{:#?}", cmd); diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index fd98b01ddc..d09c66c74a 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -1767,11 +1767,13 @@ mod schema_based_parsing_tests { )] .into_iter() .collect(), + open_attrs: false, }), ), ] .into_iter() .collect(), + open_attrs: false, }), "home_ip" => Some(SchemaType::Extension { name: Name::parse_unqualified_name("ipaddr").expect("valid"), @@ -1789,6 +1791,7 @@ mod schema_based_parsing_tests { ] .into_iter() .collect(), + open_attrs: false, }), _ => None, } @@ -1815,6 +1818,10 @@ mod schema_based_parsing_tests { fn allowed_parent_types(&self) -> Arc> { Arc::new(HashSet::new()) } + + fn open_attributes(&self) -> bool { + false + } } #[cfg(all(feature = "decimal", feature = "ipaddr"))] @@ -2878,6 +2885,10 @@ mod schema_based_parsing_tests { fn allowed_parent_types(&self) -> Arc> { Arc::new(HashSet::new()) } + + fn open_attributes(&self) -> bool { + false + } } let entitiesjson = json!( diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index a47d577ad9..f154fa0c3b 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -167,10 +167,12 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { None => { // `None` indicates the attribute shouldn't exist -- see // docs on the `attr_type()` trait method - return Err(EntitySchemaConformanceError::UnexpectedEntityAttr { - uid: uid.clone(), - attr: attr.clone(), - }); + if !schema_etype.open_attributes() { + return Err(EntitySchemaConformanceError::UnexpectedEntityAttr { + uid: uid.clone(), + attr: attr.clone(), + }); + } } Some(expected_ty) => { // typecheck: ensure that the entity attribute value matches diff --git a/cedar-policy-core/src/entities/json/context.rs b/cedar-policy-core/src/entities/json/context.rs index 39dc4112fb..891b8ae6f4 100644 --- a/cedar-policy-core/src/entities/json/context.rs +++ b/cedar-policy-core/src/entities/json/context.rs @@ -33,6 +33,7 @@ impl ContextSchema for NullContextSchema { fn context_type(&self) -> SchemaType { SchemaType::Record { attrs: HashMap::new(), + open_attrs: false, } } } diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 819714205c..002b53de68 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -274,12 +274,21 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { // `None` indicates the attribute shouldn't exist -- see // docs on the `attr_type()` trait method None => { - return Err(JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::UnexpectedEntityAttr { - uid: uid.clone(), - attr: k, - }, - )) + if desc.open_attributes() { + vparser.val_into_restricted_expr(v.into(), None, || { + JsonDeserializationErrorContext::EntityAttribute { + uid: uid.clone(), + attr: k.clone(), + } + })? + } else { + return Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::UnexpectedEntityAttr { + uid: uid.clone(), + attr: k, + }, + )); + } } Some(expected_ty) => vparser.val_into_restricted_expr( v.into(), diff --git a/cedar-policy-core/src/entities/json/schema.rs b/cedar-policy-core/src/entities/json/schema.rs index b298260b71..4b46cb6747 100644 --- a/cedar-policy-core/src/entities/json/schema.rs +++ b/cedar-policy-core/src/entities/json/schema.rs @@ -110,6 +110,10 @@ pub trait EntityTypeDescription { /// Get the entity types which are allowed to be parents of this entity type. fn allowed_parent_types(&self) -> Arc>; + + /// May entities with this type have attributes other than those specified + /// in the schema + fn open_attributes(&self) -> bool; } /// Simple type that implements `EntityTypeDescription` by expecting no @@ -132,4 +136,7 @@ impl EntityTypeDescription for NullEntityTypeDescription { fn allowed_parent_types(&self) -> Arc> { Arc::new(HashSet::new()) } + fn open_attributes(&self) -> bool { + false + } } diff --git a/cedar-policy-core/src/entities/json/schema_types.rs b/cedar-policy-core/src/entities/json/schema_types.rs index a7fd18c643..840d6d335f 100644 --- a/cedar-policy-core/src/entities/json/schema_types.rs +++ b/cedar-policy-core/src/entities/json/schema_types.rs @@ -44,6 +44,8 @@ pub enum SchemaType { Record { /// Attributes and their types attrs: HashMap, + /// Can a record with this type have attributes other than those specified in `attrs` + open_attrs: bool, }, /// Entity Entity { @@ -119,7 +121,16 @@ impl SchemaType { (Set { element_ty: elty1 }, Set { element_ty: elty2 }) => { elty1.is_consistent_with(elty2) } - (Record { attrs: attrs1 }, Record { attrs: attrs2 }) => { + ( + Record { + attrs: attrs1, + open_attrs: open1, + }, + Record { + attrs: attrs2, + open_attrs: open2, + }, + ) => { attrs1.iter().all(|(k, v)| { match attrs2.get(k) { Some(ty) => { @@ -129,9 +140,9 @@ impl SchemaType { } None => { // attrs1 has the attribute, attrs2 does not. - // if required in attrs1, incompatible. - // otherwise fine - !v.required + // if required in attrs1 and and attrs2 is + // closed, incompatible. otherwise fine + !v.required || *open2 } } }) && attrs2.iter().all(|(k, v)| { @@ -143,9 +154,9 @@ impl SchemaType { } None => { // attrs2 has the attribute, attrs1 does not. - // if required in attrs2, incompatible. - // otherwise fine - !v.required + // if required in attrs2 and attrs1 is closed, + // incompatible. otherwise fine + !v.required || *open1 } } }) @@ -192,11 +203,17 @@ impl std::fmt::Display for SchemaType { Self::String => write!(f, "string"), Self::Set { element_ty } => write!(f, "(set of {})", &element_ty), Self::EmptySet => write!(f, "empty-set"), - Self::Record { attrs } => { - if attrs.is_empty() { + Self::Record { attrs, open_attrs } => { + if attrs.is_empty() && *open_attrs { + write!(f, "any record") + } else if attrs.is_empty() { write!(f, "empty record") } else { - write!(f, "record with attributes: {{")?; + if *open_attrs { + write!(f, "record with at least attributes: {{")?; + } else { + write!(f, "record with attributes: {{")?; + } // sorting attributes ensures that there is a single, deterministic // Display output for each `SchemaType`, which is important for // tests that check equality of error messages @@ -324,7 +341,8 @@ pub fn schematype_of_restricted_expr( // but marking it optional is more flexible -- allows the // attribute type to `is_consistent_with()` more types Ok((k.clone(), AttributeType::optional(attr_type))) - }).collect::, GetSchemaTypeError>>()? + }).collect::, GetSchemaTypeError>>()?, + open_attrs: false, }) } ExprKind::ExtensionFunctionApp { fn_name, .. } => { @@ -370,6 +388,7 @@ pub fn schematype_of_value(value: &Value) -> Result>()?, + open_attrs: false, }), Value::ExtensionValue(ev) => Ok(SchemaType::Extension { name: ev.typename(), diff --git a/cedar-policy-core/src/entities/json/value.rs b/cedar-policy-core/src/entities/json/value.rs index fc6893c761..860b86b870 100644 --- a/cedar-policy-core/src/entities/json/value.rs +++ b/cedar-policy-core/src/entities/json/value.rs @@ -514,6 +514,7 @@ impl<'e> ValueParser<'e> { Some( expected_ty @ SchemaType::Record { attrs: expected_attrs, + open_attrs, }, ) => match val { serde_json::Value::Object(mut actual_attrs) => { @@ -537,14 +538,18 @@ impl<'e> ValueParser<'e> { } }) .collect::, JsonDeserializationError>>()?; - // we've now checked that all expected attrs exist, and removed them from `actual_attrs`. - // we still need to verify that we didn't have any unexpected attrs. - if let Some((record_attr, _)) = actual_attrs.into_iter().next() { - return Err(JsonDeserializationError::UnexpectedRecordAttr { - ctx: Box::new(ctx2()), - record_attr: record_attr.into(), - }); + + if !open_attrs { + // we've now checked that all expected attrs exist, and removed them from `actual_attrs`. + // we still need to verify that we didn't have any unexpected attrs. + if let Some((record_attr, _)) = actual_attrs.into_iter().next() { + return Err(JsonDeserializationError::UnexpectedRecordAttr { + ctx: Box::new(ctx2()), + record_attr: record_attr.into(), + }); + } } + // having duplicate keys should be impossible here (because // neither `actual_attrs` nor `expected_attrs` can have // duplicate keys; they're both maps), but we can still throw diff --git a/cedar-policy-validator/Cargo.toml b/cedar-policy-validator/Cargo.toml index e785a4b677..2a3ba95b1e 100644 --- a/cedar-policy-validator/Cargo.toml +++ b/cedar-policy-validator/Cargo.toml @@ -32,5 +32,8 @@ decimal = ["cedar-policy-core/decimal"] # Enables `Arbitrary` implementations for several types in this crate arbitrary = ["dep:arbitrary"] +# Experimental features. +partial-validate = [] + [dev-dependencies] cool_asserts = "2.0" diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index faf45b43bd..a65c09b082 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -128,6 +128,10 @@ impl entities::EntityTypeDescription for EntityTypeDescription { fn allowed_parent_types(&self) -> Arc> { Arc::clone(&self.allowed_parent_types) } + + fn open_attributes(&self) -> bool { + self.validator_type.open_attributes.is_open() + } } impl ast::RequestSchema for ValidatorSchema { diff --git a/cedar-policy-validator/src/err.rs b/cedar-policy-validator/src/err.rs index 618da3c181..776e64f215 100644 --- a/cedar-policy-validator/src/err.rs +++ b/cedar-policy-validator/src/err.rs @@ -159,7 +159,7 @@ impl std::fmt::Display for UnsupportedFeature { match self { Self::OpenRecordsAndEntities => write!( f, - "records and entities with additional attributes are not yet implemented" + "records and entities with `additionalAttributes` are experimental, but the experimental `partial-validate` feature is not enabled" ), Self::ActionAttributes(attrs) => write!( f, diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index 661380f2b4..2c5692dc63 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -50,14 +50,28 @@ pub enum ValidationMode { #[default] Strict, Permissive, + #[cfg(feature = "partial-validate")] + Partial, } impl ValidationMode { + /// Does this mode use partial validation. We could conceivably have a + /// strict/partial validation mode. + fn is_partial(self) -> bool { + match self { + ValidationMode::Strict | ValidationMode::Permissive => false, + #[cfg(feature = "partial-validate")] + ValidationMode::Partial => true, + } + } + /// Does this mode apply strict validation rules. fn is_strict(self) -> bool { match self { ValidationMode::Strict => true, ValidationMode::Permissive => false, + #[cfg(feature = "partial-validate")] + ValidationMode::Partial => false, } } } diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 5d06f0f3c5..ab5a2081ba 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -34,7 +34,7 @@ use serde_with::serde_as; use super::NamespaceDefinition; use crate::{ err::*, - types::{Attributes, EntityRecordKind, Type}, + types::{Attributes, EntityRecordKind, OpenTag, Type}, SchemaFragment, }; @@ -254,17 +254,19 @@ impl ValidatorSchema { // error for any other undeclared entity types by // `check_for_undeclared`. let descendants = entity_children.remove(&name).unwrap_or_default(); + let (attributes, open_attributes) = Self::record_attributes_or_none( + entity_type.attributes.resolve_type_defs(&type_defs)?, + ) + .ok_or(SchemaError::ContextOrShapeNotRecord( + ContextOrShape::EntityTypeShape(name.clone()), + ))?; Ok(( name.clone(), ValidatorEntityType { - name: name.clone(), + name, descendants, - attributes: Self::record_attributes_or_none( - entity_type.attributes.resolve_type_defs(&type_defs)?, - ) - .ok_or(SchemaError::ContextOrShapeNotRecord( - ContextOrShape::EntityTypeShape(name), - ))?, + attributes, + open_attributes, }, )) }) @@ -283,19 +285,21 @@ impl ValidatorSchema { .into_iter() .map(|(name, action)| -> Result<_> { let descendants = action_children.remove(&name).unwrap_or_default(); - + let (context, open_context_attributes) = + Self::record_attributes_or_none(action.context.resolve_type_defs(&type_defs)?) + .ok_or(SchemaError::ContextOrShapeNotRecord( + ContextOrShape::ActionContext(name.clone()), + ))?; Ok(( name.clone(), ValidatorActionId { - name: name.clone(), + name, applies_to: action.applies_to, descendants, - context: Self::record_attributes_or_none( - action.context.resolve_type_defs(&type_defs)?, - ) - .ok_or(SchemaError::ContextOrShapeNotRecord( - ContextOrShape::ActionContext(name), - ))?, + context: Type::record_with_attributes( + context.attrs, + open_context_attributes, + ), attribute_types: action.attribute_types, attributes: action.attributes, }, @@ -371,13 +375,7 @@ impl ValidatorSchema { // types and `appliesTo` lists. See the `entity_types` loop for why the // `descendants` list is not checked. for action in action_ids.values() { - for (_, attr_typ) in action.context.iter() { - Self::check_undeclared_in_type( - &attr_typ.attr_type, - entity_types, - &mut undeclared_e, - ); - } + Self::check_undeclared_in_type(&action.context, entity_types, &mut undeclared_e); for p_entity in action.applies_to.applicable_principal_types() { match p_entity { @@ -411,9 +409,12 @@ impl ValidatorSchema { Ok(()) } - fn record_attributes_or_none(ty: Type) -> Option { + fn record_attributes_or_none(ty: Type) -> Option<(Attributes, OpenTag)> { match ty { - Type::EntityOrRecord(EntityRecordKind::Record { attrs, .. }) => Some(attrs), + Type::EntityOrRecord(EntityRecordKind::Record { + attrs, + open_attributes, + }) => Some((attrs, open_attributes)), _ => None, } } diff --git a/cedar-policy-validator/src/schema/action.rs b/cedar-policy-validator/src/schema/action.rs index ea0953c5a1..151e7c5547 100644 --- a/cedar-policy-validator/src/schema/action.rs +++ b/cedar-policy-validator/src/schema/action.rs @@ -8,7 +8,7 @@ use serde::Serialize; use smol_str::SmolStr; use std::collections::{HashMap, HashSet}; -use crate::types::{Attributes, OpenTag, Type}; +use crate::types::{Attributes, Type}; /// Contains information about actions used by the validator. The contents of /// the struct are the same as the schema entity type structure, but the @@ -28,9 +28,8 @@ pub struct ValidatorActionId { /// descendants before it is used in any validation. pub(crate) descendants: HashSet, - /// The context attributes associated with this action. Keys are the context - /// attribute identifiers while the values are the type of the attribute. - pub(crate) context: Attributes, + /// The type of the context record associated with this action. + pub(crate) context: Type, /// The attribute types for this action, used for typechecking. pub(crate) attribute_types: Attributes, @@ -49,10 +48,7 @@ impl ValidatorActionId { /// /// This always returns a closed record type. pub fn context_type(&self) -> Type { - Type::record_with_attributes( - self.context.iter().map(|(k, v)| (k.clone(), v.clone())), - OpenTag::ClosedAttributes, - ) + self.context.clone() } } diff --git a/cedar-policy-validator/src/schema/entity_type.rs b/cedar-policy-validator/src/schema/entity_type.rs index 89de684120..2ef5035b2c 100644 --- a/cedar-policy-validator/src/schema/entity_type.rs +++ b/cedar-policy-validator/src/schema/entity_type.rs @@ -9,7 +9,7 @@ use cedar_policy_core::{ transitive_closure::TCNode, }; -use crate::types::{AttributeType, Attributes}; +use crate::types::{AttributeType, Attributes, OpenTag}; /// Contains entity type information for use by the validator. The contents of /// the struct are the same as the schema entity type structure, but the @@ -28,6 +28,13 @@ pub struct ValidatorEntityType { /// The attributes associated with this entity. Keys are the attribute /// identifiers while the values are the type of the attribute. pub(crate) attributes: Attributes, + + /// Indicates that this entity type may have additional attributes + /// other than the declared attributes that may be accessed under partial + /// schema validation. We do not know if they are present, and do not know + /// their type when they are present. Attempting to access an undeclared + /// attribute under standard validation is an error regardless of this flag. + pub(crate) open_attributes: OpenTag, } impl ValidatorEntityType { diff --git a/cedar-policy-validator/src/schema/namespace_def.rs b/cedar-policy-validator/src/schema/namespace_def.rs index cce2f8cc90..aaf60a5a1f 100644 --- a/cedar-policy-validator/src/schema/namespace_def.rs +++ b/cedar-policy-validator/src/schema/namespace_def.rs @@ -680,14 +680,23 @@ impl ValidatorNamespaceDef { attributes, additional_attributes, }) => { - if additional_attributes { + if cfg!(not(feature = "partial-validate")) && additional_attributes { Err(SchemaError::UnsupportedFeature( UnsupportedFeature::OpenRecordsAndEntities, )) } else { Ok( Self::parse_record_attributes(default_namespace, attributes)?.map( - |attrs| Type::record_with_attributes(attrs, OpenTag::ClosedAttributes), + move |attrs| { + Type::record_with_attributes( + attrs, + if additional_attributes { + OpenTag::OpenAttributes + } else { + OpenTag::ClosedAttributes + }, + ) + }, ), ) } diff --git a/cedar-policy-validator/src/schema_file_format.rs b/cedar-policy-validator/src/schema_file_format.rs index 6911cfc613..e3ab086fdb 100644 --- a/cedar-policy-validator/src/schema_file_format.rs +++ b/cedar-policy-validator/src/schema_file_format.rs @@ -111,7 +111,7 @@ impl Default for AttributesOrContext { fn default() -> Self { Self(SchemaType::Type(SchemaTypeVariant::Record { attributes: BTreeMap::new(), - additional_attributes: false, + additional_attributes: partial_schema_default(), })) } } @@ -412,7 +412,7 @@ impl SchemaTypeVisitor { if let Some(attributes) = attributes { let additional_attributes = - additional_attributes.unwrap_or(Ok(additional_attributes_default())); + additional_attributes.unwrap_or(Ok(partial_schema_default())); Ok(SchemaType::Type(SchemaTypeVariant::Record { attributes: attributes?.0, additional_attributes: additional_attributes?, @@ -596,9 +596,9 @@ pub struct TypeOfAttribute { pub required: bool, } -/// Defines the default value for `additionalAttributes` on records and -/// entities -fn additional_attributes_default() -> bool { +/// By default schema properties which enable parts of partial schema validation +/// should be `false`. Defines the default value for `additionalAttributes`. +fn partial_schema_default() -> bool { false } diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 3f90bf76fa..196736a599 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -22,6 +22,7 @@ mod test_expr; mod test_extensions; mod test_namespace; mod test_optional_attributes; +mod test_partial; mod test_policy; mod test_strict; mod test_type_annotation; @@ -561,12 +562,9 @@ impl<'a> Typechecker<'a> { .var(Var::Resource), ), ExprKind::Var(Var::Context) => TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::record_with_attributes( - request_env.context.clone(), - OpenTag::ClosedAttributes, - ))) - .with_same_source_info(e) - .var(Var::Context), + ExprBuilder::with_data(Some(request_env.context.clone())) + .with_same_source_info(e) + .var(Var::Context), ), ExprKind::Unknown(u) => { TypecheckAnswer::fail(ExprBuilder::with_data(None).unknown(u.clone())) @@ -969,6 +967,19 @@ impl<'a> Typechecker<'a> { TypecheckAnswer::fail(annot_expr) } } + // In partial schema validation, if we can't find + // the attribute but there may be additional + // attributes, we do not fail and instead return the + // bottom type (`Never`). + None if self.mode.is_partial() + && Type::may_have_attr(self.schema, typ_actual, attr) => + { + TypecheckAnswer::success( + ExprBuilder::with_data(Some(Type::Never)) + .with_same_source_info(e) + .get_attr(typ_expr_actual, attr.clone()), + ) + } None => { let borrowed = all_attrs.iter().map(|s| s.as_str()).collect::>(); diff --git a/cedar-policy-validator/src/typecheck/test_expr.rs b/cedar-policy-validator/src/typecheck/test_expr.rs index f5710dc386..dd79a0dcad 100644 --- a/cedar-policy-validator/src/typecheck/test_expr.rs +++ b/cedar-policy-validator/src/typecheck/test_expr.rs @@ -505,7 +505,7 @@ fn eq_typecheck_entity_literals_false() { fn entity_has_typechecks() { assert_typechecks_empty_schema( Expr::has_attr(Expr::var(Var::Principal), "attr".into()), - Type::singleton_boolean(false), + Type::primitive_boolean(), ); } diff --git a/cedar-policy-validator/src/typecheck/test_partial.rs b/cedar-policy-validator/src/typecheck/test_partial.rs new file mode 100644 index 0000000000..fb4be48605 --- /dev/null +++ b/cedar-policy-validator/src/typecheck/test_partial.rs @@ -0,0 +1,230 @@ +//! Contains test for typechecking with partial schema files. +#![cfg(test)] +#![cfg(feature = "partial-validate")] +// GRCOV_STOP_COVERAGE + +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::Typechecker; +use crate::types::{EntityLUB, Type}; +use crate::{AttributeAccess, NamespaceDefinition, TypeError, ValidationMode, ValidatorSchema}; + +pub(crate) fn assert_partial_typecheck( + schema: impl TryInto, + policy: StaticPolicy, +) { + 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 typechecked = typechecker.typecheck_policy( + &Template::link_static_policy(policy.clone()).0, + &mut type_errors, + ); + assert_eq!(type_errors, HashSet::new(), "Did not expect any errors."); + assert!(typechecked, "Expected that policy would typecheck."); +} + +pub(crate) fn assert_partial_typecheck_fail( + schema: impl TryInto, + policy: StaticPolicy, + expected_type_errors: 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 typechecked = typechecker.typecheck_policy( + &Template::link_static_policy(policy.clone()).0, + &mut type_errors, + ); + assert_expected_type_errors(&expected_type_errors, &type_errors); + assert!(!typechecked, "Expected that policy would not typecheck."); +} + +fn partial_schema_file() -> NamespaceDefinition { + serde_json::from_value(serde_json::json!( + { + "entityTypes": { + "User": { + "memberOfTypes": [ "Group" ], + "shape": { + "type": "Record", + "additionalAttributes": true, + "attributes": { + "name": { "type": "String", "required": true}, + "age": { "type": "Long", "required": true}, + "favorite": { "type": "Entity", "name": "Photo", "required": true} + } + } + }, + "Group": { + "shape": { + "type": "Record", + "additionalAttributes": false, + "attributes": {} + } + }, + "Photo": { + "shape": { + "type": "Record", + "additionalAttributes": true, + "attributes": {} + } + } + }, + "actions": { + "view_photo": { + "appliesTo": { + "principalTypes": ["User", "Group"], + "resourceTypes": ["Photo"], + "context": { + "type": "Record", + "additionalAttributes": true, + "attributes": {} + } + } + } + } + } + )) + .expect("Expected valid schema") +} + +pub(crate) fn assert_typechecks_partial_schema(policy: StaticPolicy) { + assert_partial_typecheck(partial_schema_file(), policy) +} + +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) +} + +mod passes_partial_schema { + use super::*; + + #[test] + fn unknown_attr_on_declared_entity_type() { + assert_typechecks_partial_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { User::"alice".unknown };"#, + ) + .unwrap(), + ); + } + + #[test] + fn has_unknown_attr_on_declared_entity_type() { + assert_typechecks_partial_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { User::"alice" has unknown };"#, + ) + .unwrap(), + ); + } + + #[test] + fn policy_in_set_action_and_other() { + let p = parse_policy( + Some("0".to_string()), + r#"permit(principal, action, resource) when { User::"alice" in [action, User::"alice"] };"#, + ).expect("Policy should parse."); + assert_typechecks_partial_schema(p); + } + + #[test] + fn policy_action_in_set_action_and_other() { + let p = parse_policy( + Some("0".to_string()), + r#"permit(principal, action, resource) when { action in [action, User::"alice"] };"#, + ) + .expect("Policy should parse."); + assert_typechecks_partial_schema(p); + } + + #[test] + fn context_attr() { + let p = parse_policy( + Some("0".to_string()), + r#"permit(principal, action == Action::"view_photo", resource) when { context.foo };"#, + ) + .expect("Policy should parse."); + assert_typechecks_partial_schema(p); + } +} + +mod fail_partial_schema { + + use std::str::FromStr; + + use super::*; + + #[test] + fn error_on_declared_attr() { + // `name` is declared as a `String` in the partial schema, so we can + // error even though `principal.unknown` is not declared. + assert_typecheck_fails_partial_schema( + parse_policy( + None, + r#"permit(principal == User::"alice", action, resource) when { principal.name > principal.unknown };"#, + ) + .unwrap(), + vec![TypeError::expected_type( + Expr::get_attr(Expr::var(Var::Principal), "name".into()), + Type::primitive_long(), + Type::primitive_string(), + )], + ); + } + + #[test] + fn incompatible_attrs() { + assert_typecheck_fails_partial_schema( + // `age` and `name` are defined with incompatible types, while + // `unknown` is not defined. The conflict is noticed and an error is + // raised. + parse_policy( + None, + r#"permit(principal == User::"alice", action, resource) when { + (if resource.foo then + principal.age + else (if resource.bar then + principal.name + else + principal.unknown + )) == "alice"};"#, + ) + .unwrap(), + vec![TypeError::incompatible_types( + Expr::from_str("if resource.foo then principal.age else (if resource.bar then principal.name else principal.unknown)").unwrap(), + vec![Type::primitive_long(), Type::primitive_string()], + )], + ); + } + + #[test] + fn unknown_attr_on_closed_entity_type() { + assert_typecheck_fails_partial_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal.is_foo };"#, + ) + .unwrap(), + vec![TypeError::unsafe_attribute_access( + Expr::from_str("principal.is_foo").unwrap(), + AttributeAccess::EntityLUB( + EntityLUB::single_entity("Group".parse().unwrap()), + vec!["is_foo".into()], + ), + None, + false, + )], + ); + } +} diff --git a/cedar-policy-validator/src/typecheck/test_policy.rs b/cedar-policy-validator/src/typecheck/test_policy.rs index 7e3d92f36e..6685f34930 100644 --- a/cedar-policy-validator/src/typecheck/test_policy.rs +++ b/cedar-policy-validator/src/typecheck/test_policy.rs @@ -429,6 +429,19 @@ fn policy_in_action_impossible() { ); } +#[test] +fn policy_action_in_impossible() { + let p = parse_policy( + Some("0".to_string()), + r#"permit(principal, action, resource) when { action in [User::"alice"] };"#, + ) + .expect("Policy should parse."); + assert_policy_typecheck_fails_simple_schema( + p.clone(), + vec![TypeError::impossible_policy(p.condition())], + ); +} + #[test] fn policy_entity_has_then_get() { assert_policy_typechecks_simple_schema(parse_policy( diff --git a/cedar-policy-validator/src/typecheck/test_strict.rs b/cedar-policy-validator/src/typecheck/test_strict.rs index 182dbaa250..1a2971d933 100644 --- a/cedar-policy-validator/src/typecheck/test_strict.rs +++ b/cedar-policy-validator/src/typecheck/test_strict.rs @@ -28,7 +28,7 @@ use std::str::FromStr; use cedar_policy_core::ast::{EntityType, EntityUID, Expr}; use crate::{ - types::{Attributes, EffectSet, RequestEnv, Type}, + types::{EffectSet, OpenTag, RequestEnv, Type}, IncompatibleTypes, SchemaFragment, TypeErrorKind, ValidationMode, }; @@ -149,7 +149,7 @@ where principal: &EntityType::Specified("User".parse().unwrap()), action: &EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), resource: &EntityType::Specified("Photo".parse().unwrap()), - context: &Attributes::with_attributes(None), + context: &Type::record_with_attributes(None, OpenTag::ClosedAttributes), principal_slot: None, resource_slot: None, }, diff --git a/cedar-policy-validator/src/typecheck/test_utils.rs b/cedar-policy-validator/src/typecheck/test_utils.rs index 1b12c1593a..05e0039302 100644 --- a/cedar-policy-validator/src/typecheck/test_utils.rs +++ b/cedar-policy-validator/src/typecheck/test_utils.rs @@ -30,7 +30,7 @@ use cedar_policy_core::ast::{EntityType, EntityUID, Expr, ExprShapeOnly, StaticP use crate::{ schema::ACTION_ENTITY_TYPE, type_error::TypeError, - types::{Attributes, EffectSet, RequestEnv, Type}, + types::{EffectSet, OpenTag, RequestEnv, Type}, NamespaceDefinition, ValidationMode, ValidatorSchema, }; @@ -78,7 +78,7 @@ impl Typechecker<'_> { .parse() .expect("Placeholder type \"Resource\" failed to parse as valid type name."), ), - context: &Attributes::with_attributes(None), + context: &Type::record_with_attributes(None, OpenTag::ClosedAttributes), principal_slot: None, resource_slot: None, }; @@ -153,11 +153,27 @@ pub(crate) fn assert_policy_typechecks_for_mode( mode: ValidationMode, ) { with_typechecker_from_schema(schema, |mut typechecker| { + let policy: Arc