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..49eca42752 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 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..94e882ef76 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, } } } @@ -87,7 +101,7 @@ impl Validator { .flat_map(|p| self.validate_policy(p, mode)); let link_errs = policies .policies() - .filter_map(|p| self.validate_slots(p)) + .filter_map(|p| self.validate_slots(p, mode)) .flatten(); ValidationResult::new( template_and_static_policy_errs.chain(link_errs), @@ -103,15 +117,31 @@ impl Validator { p: &'a Template, mode: ValidationMode, ) -> impl Iterator + 'a { - self.validate_entity_types(p) - .chain(self.validate_action_ids(p)) - .chain(self.validate_action_application( - p.principal_constraint(), - p.action_constraint(), - p.resource_constraint(), - )) - .map(move |note| ValidationError::with_policy_id(p.id(), None, note)) - .chain(self.typecheck_policy(p, mode)) + 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 + // actions, so we can never claim that one doesn't exist. + None + } else { + Some( + self.validate_entity_types(p) + .chain(self.validate_action_ids(p)) + // We could usefully update this pass to apply to partial + // schema if it only failed when there is a known action + // applied to known principal/resource entity types that are + // not in its `appliesTo`. + .chain(self.validate_action_application( + p.principal_constraint(), + p.action_constraint(), + p.resource_constraint(), + )) + .map(move |note| ValidationError::with_policy_id(p.id(), None, note)), + ) + } + .into_iter() + .flatten() + .chain(self.typecheck_policy(p, mode)) } /// Run relevant validations against a single template-linked policy, @@ -119,11 +149,18 @@ impl Validator { fn validate_slots<'a>( &'a self, p: &'a Policy, + mode: ValidationMode, ) -> Option + 'a> { // Ignore static policies since they are already handled by `validate_policy` if p.is_static() { return None; } + // In partial validation, there may be arbitrary extra entity types and + // actions, so we can never claim that one doesn't exist or that the + // action application is invalid. + if mode.is_partial() { + return None; + } // For template-linked policies `Policy::principal_constraint()` and // `Policy::resource_constraint()` return a copy of the constraint with // the slot filled by the appropriate value. diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index 995c1cd906..0739d2fc04 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -340,7 +340,9 @@ impl Validator { // in [...] ActionConstraint::In(euids) => Box::new( self.schema - .get_actions_in_set(euids.iter().map(Arc::as_ref)), + .get_actions_in_set(euids.iter().map(Arc::as_ref)) + .unwrap_or_default() + .into_iter(), ), } } @@ -381,9 +383,12 @@ impl Validator { .into_iter(), ), // in - PrincipalOrResourceConstraint::In(EntityReference::EUID(euid)) => { - Box::new(self.schema.get_entity_types_in(euid.as_ref())) - } + PrincipalOrResourceConstraint::In(EntityReference::EUID(euid)) => Box::new( + self.schema + .get_entity_types_in(euid.as_ref()) + .unwrap_or_default() + .into_iter(), + ), PrincipalOrResourceConstraint::Eq(EntityReference::Slot) | PrincipalOrResourceConstraint::In(EntityReference::Slot) => { Box::new(self.schema.known_entity_types()) @@ -401,6 +406,8 @@ impl Validator { Box::new( self.schema .get_entity_types_in(in_entity.as_ref()) + .unwrap_or_default() + .into_iter() .filter(move |k| &entity_type == k), ) } @@ -1787,3 +1794,59 @@ mod test { ); } } + +#[cfg(test)] +#[cfg(feature = "partial-validate")] +mod partial_schema { + use cedar_policy_core::{ + ast::{StaticPolicy, Template}, + parser::parse_policy, + }; + + use crate::{NamespaceDefinition, Validator}; + + fn assert_validates_with_empty_schema(policy: StaticPolicy) { + let schema = serde_json::from_str::( + r#" + { + "entityTypes": { }, + "actions": {} + } + "#, + ) + .unwrap() + .try_into() + .unwrap(); + + let (template, _) = Template::link_static_policy(policy); + let validate = Validator::new(schema); + let errs = validate + .validate_policy(&template, crate::ValidationMode::Partial) + .collect::>(); + assert_eq!(errs, vec![], "Did not expect any validation errors."); + } + + #[test] + fn undeclared_entity_type_partial_schema() { + let policy = parse_policy( + Some("0".to_string()), + r#"permit(principal == User::"alice", action, resource);"#, + ) + .unwrap(); + assert_validates_with_empty_schema(policy); + + let policy = parse_policy( + Some("0".to_string()), + r#"permit(principal, action == Action::"view", resource);"#, + ) + .unwrap(); + assert_validates_with_empty_schema(policy); + + let policy = parse_policy( + Some("0".to_string()), + r#"permit(principal, action, resource == Photo::"party.jpg");"#, + ) + .unwrap(); + assert_validates_with_empty_schema(policy); + } +} diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 5d06f0f3c5..f865789d2d 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, } } @@ -493,19 +494,18 @@ impl ValidatorSchema { /// includes all entity types that are descendants of the type of `entity` /// according to the schema, and the type of `entity` itself because /// `entity in entity` evaluates to `true`. - pub(crate) fn get_entity_types_in<'a>( - &'a self, - entity: &'a EntityUID, - ) -> impl Iterator + 'a { + pub(crate) fn get_entity_types_in<'a>(&'a self, entity: &'a EntityUID) -> Option> { let ety = match entity.entity_type() { EntityType::Specified(ety) => Some(ety), EntityType::Unspecified => None, }; - ety.and_then(|ety| self.get_entity_type(ety)) - .into_iter() - .flat_map(|ety| ety.descendants.iter()) - .chain(ety.filter(|ety| self.is_known_entity_type(ety))) + ety.and_then(|ety| self.get_entity_type(ety)).map(|ety| { + ety.descendants + .iter() + .chain(std::iter::once(&ety.name)) + .collect() + }) } /// Get all entity types in the schema where an `{entity0} in {euids}` can @@ -514,10 +514,12 @@ impl ValidatorSchema { pub(crate) fn get_entity_types_in_set<'a>( &'a self, euids: impl IntoIterator + 'a, - ) -> impl Iterator + 'a { + ) -> Option> { euids .into_iter() - .flat_map(move |e| self.get_entity_types_in(e)) + .map(|e| self.get_entity_types_in(e)) + .collect::>>() + .map(|v| v.into_iter().flatten().collect::>()) } /// Get all action entities in the schema where `action in euids` evaluates @@ -526,17 +528,19 @@ impl ValidatorSchema { pub(crate) fn get_actions_in_set<'a>( &'a self, euids: impl IntoIterator + 'a, - ) -> impl Iterator + 'a { - euids.into_iter().flat_map(|e| { - self.get_action_id(e) - .into_iter() - .flat_map(|action| action.descendants.iter()) - .chain(if self.is_known_action_id(e) { - Some(e) - } else { - None + ) -> Option> { + euids + .into_iter() + .map(|e| { + self.get_action_id(e).map(|action| { + action + .descendants + .iter() + .chain(std::iter::once(&action.name)) }) - }) + }) + .collect::>>() + .map(|v| v.into_iter().flatten().collect::>()) } /// Get the `Type` of context expected for the given `action`. 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/type_error.rs b/cedar-policy-validator/src/type_error.rs index 9a263a8457..0a56e5c4bd 100644 --- a/cedar-policy-validator/src/type_error.rs +++ b/cedar-policy-validator/src/type_error.rs @@ -378,7 +378,10 @@ impl AttributeAccess { if let Some(Type::EntityOrRecord(EntityRecordKind::Entity(lub))) = expr.data() { return AttributeAccess::EntityLUB(lub.clone(), attrs); } else if let ExprKind::Var(Var::Context) = expr.expr_kind() { - return AttributeAccess::Context(req_env.action.clone(), attrs); + return match req_env.action_entity_uid() { + Some(action) => AttributeAccess::Context(action.clone(), attrs), + None => AttributeAccess::Other(attrs), + }; } else if let ExprKind::GetAttr { expr: sub_expr, attr, diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 3f90bf76fa..6e0c4b6ab2 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; @@ -398,24 +399,34 @@ impl<'a> Typechecker<'a> { // For every action compute the cross product of the principal and // resource applies_to sets. - all_actions.flat_map(move |action| { - action - .applies_to - .applicable_principal_types() - .flat_map(move |principal| { - action - .applies_to - .applicable_resource_types() - .map(move |resource| RequestEnv { - principal, - action: &action.name, - resource, - context: &action.context, - principal_slot: None, - resource_slot: None, - }) - }) - }) + all_actions + .flat_map(|action| { + action + .applies_to + .applicable_principal_types() + .flat_map(|principal| { + action + .applies_to + .applicable_resource_types() + .map(|resource| RequestEnv::DeclaredAction { + principal, + action: &action.name, + resource, + context: &action.context, + principal_slot: None, + resource_slot: None, + }) + }) + }) + .chain(if self.mode.is_partial() { + // A partial schema might not list all actions, and may not + // include all principal and resource types for the listed ones. + // So we typecheck with a fully unknown request to handle these + // missing cases. + Some(RequestEnv::UndeclaredAction) + } else { + None + }) } /// Given a request environment and a template, return new environments @@ -424,29 +435,40 @@ impl<'a> Typechecker<'a> { &'b self, env: RequestEnv<'b>, t: &'b Template, - ) -> impl Iterator + 'b { - self.possible_slot_instantiations( - t, - SlotId::principal(), - env.principal, - t.principal_constraint().as_inner(), - ) - .flat_map(move |p_slot| { - self.possible_slot_instantiations( - t, - SlotId::resource(), - env.resource, - t.resource_constraint().as_inner(), - ) - .map(move |r_slot| RequestEnv { - principal: env.principal, - action: env.action, - resource: env.resource, - context: env.context, - principal_slot: p_slot.clone(), - resource_slot: r_slot.clone(), - }) - }) + ) -> Box + 'b> { + match env { + RequestEnv::UndeclaredAction => Box::new(std::iter::once(RequestEnv::UndeclaredAction)), + RequestEnv::DeclaredAction { + principal, + action, + resource, + context, + .. + } => Box::new( + self.possible_slot_instantiations( + t, + SlotId::principal(), + principal, + t.principal_constraint().as_inner(), + ) + .flat_map(move |p_slot| { + self.possible_slot_instantiations( + t, + SlotId::resource(), + resource, + t.resource_constraint().as_inner(), + ) + .map(move |r_slot| RequestEnv::DeclaredAction { + principal, + action, + resource, + context, + principal_slot: p_slot.clone(), + resource_slot: r_slot.clone(), + }) + }), + ), + } } /// Get the entity types which could instantiate the slot given in this @@ -520,53 +542,41 @@ impl<'a> Typechecker<'a> { // Principal, resource, and context have types defined by // the request type. ExprKind::Var(Var::Principal) => TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::possibly_unspecified_entity_reference( - request_env.principal.clone(), - ))) - .with_same_source_info(e) - .var(Var::Principal), + ExprBuilder::with_data(Some(request_env.principal_type())) + .with_same_source_info(e) + .var(Var::Principal), ), // While the EntityUID for Action is held in the request context, // entity types do not consider the id of the entity (only the // entity type), so the type of Action is only the entity type name // taken from the euid. ExprKind::Var(Var::Action) => { - let ty = if matches!(request_env.action.entity_type(), EntityType::Unspecified) { - // The action entity may be unspecified. In this case it has - // type AnyEntity - Some(Type::any_entity_reference()) - } else { - // This returns `None` if the action entity is not defined - // in the schema which will cause a typecheck fail in the - // match below. - Type::euid_literal(request_env.action.clone(), self.schema) - }; - - match ty { + match request_env.action_type(self.schema) { Some(ty) => TypecheckAnswer::success( ExprBuilder::with_data(Some(ty)) .with_same_source_info(e) .var(Var::Action), ), + // `None` if the action entity is not defined in the schema. + // This will only show up if we're typechecking with a + // request environment that was not constructed from the + // schema cross product, which will not happen through our + // public entry points, but it can occur if calling + // `typecheck` directly which happens in our tests. None => TypecheckAnswer::fail( ExprBuilder::new().with_same_source_info(e).var(Var::Action), ), } } ExprKind::Var(Var::Resource) => TypecheckAnswer::success( - ExprBuilder::with_data(Some(Type::possibly_unspecified_entity_reference( - request_env.resource.clone(), - ))) - .with_same_source_info(e) - .var(Var::Resource), + ExprBuilder::with_data(Some(request_env.resource_type())) + .with_same_source_info(e) + .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_type())) + .with_same_source_info(e) + .var(Var::Context), ), ExprKind::Unknown(u) => { TypecheckAnswer::fail(ExprBuilder::with_data(None).unknown(u.clone())) @@ -575,13 +585,13 @@ impl<'a> Typechecker<'a> { ExprKind::Slot(slotid) => TypecheckAnswer::success( ExprBuilder::with_data(Some(if slotid.is_principal() { request_env - .principal_slot + .principal_slot() .clone() .map(Type::possibly_unspecified_entity_reference) .unwrap_or(Type::any_entity_reference()) } else if slotid.is_resource() { request_env - .resource_slot + .resource_slot() .clone() .map(Type::possibly_unspecified_entity_reference) .unwrap_or(Type::any_entity_reference()) @@ -618,6 +628,17 @@ impl<'a> Typechecker<'a> { // not generated here. We still return `TypecheckFail` so that // typechecking is not considered successful. match Type::euid_literal((**euid).clone(), self.schema) { + // The entity type is undeclared, but that's OK for a + // partial schema. The attributes record will be empty if we + // try to access it later, so all attributes will have the + // bottom type. + None if self.mode.is_partial() => TypecheckAnswer::success( + ExprBuilder::with_data(Some(Type::possibly_unspecified_entity_reference( + euid.entity_type().clone(), + ))) + .with_same_source_info(e) + .val(euid.clone()), + ), Some(ty) => TypecheckAnswer::success( ExprBuilder::with_data(Some(ty)) .with_same_source_info(e) @@ -969,6 +990,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::>(); @@ -1709,7 +1743,11 @@ impl<'a> Typechecker<'a> { ), // If none of the cases apply, then all we know is that `in` has - // type boolean. + // type boolean. Importantly for partial schema + // validation, this case captures an `in` between entity + // literals where the LHS is not an action defined in + // the schema and does not have an entity type defined + // in the schema. _ => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) .with_same_source_info(in_expr) @@ -1813,10 +1851,19 @@ impl<'a> Typechecker<'a> { fn is_unspecified_entity(query_env: &RequestEnv, expr: &Expr) -> bool { match expr.expr_kind() { - ExprKind::Var(Var::Principal) => matches!(query_env.principal, EntityType::Unspecified), - ExprKind::Var(Var::Resource) => matches!(query_env.resource, EntityType::Unspecified), + ExprKind::Var(Var::Principal) => matches!( + query_env.principal_entity_type(), + Some(EntityType::Unspecified) + ), + ExprKind::Var(Var::Resource) => matches!( + query_env.resource_entity_type(), + Some(EntityType::Unspecified) + ), ExprKind::Var(Var::Action) => { - matches!(query_env.action.entity_type(), EntityType::Unspecified) + matches!( + query_env.action_entity_uid().map(EntityUID::entity_type), + Some(EntityType::Unspecified) + ) } _ => false, } @@ -1835,22 +1882,39 @@ impl<'a> Typechecker<'a> { ) -> TypecheckAnswer<'c> { if let Some(rhs) = Typechecker::euids_from_euid_literals_or_action(request_env, rhs_elems) { let var_euid = if matches!(lhs_var, Var::Principal) { - request_env.principal + request_env.principal_entity_type() } else { - request_env.resource + request_env.resource_entity_type() }; let descendants = self.schema.get_entity_types_in_set(rhs.iter()); - match var_euid { - EntityType::Specified(var_name) => Typechecker::entity_in_descendants( - var_name, - descendants, - in_expr, - lhs_expr, - rhs_expr, - ), + match (var_euid, descendants) { + // We failed to lookup the descendants because the entity type + // is not declared in the schema, or we failed to get the + // principal/resource entity type because the request is + // unknown. We don't know if the euid would be in the + // descendants or not, so give it type boolean. + (_, None) | (None, _) => { + let in_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(in_expr) + .is_in(lhs_expr, rhs_expr); + if self.mode.is_partial() { + TypecheckAnswer::success(in_expr) + } else { + TypecheckAnswer::fail(in_expr) + } + } + (Some(EntityType::Specified(var_name)), Some(descendants)) => { + Typechecker::entity_in_descendants( + var_name, + descendants, + in_expr, + lhs_expr, + rhs_expr, + ) + } // Unspecified entities will be detected by a different part of the validator. // Still return `TypecheckFail` so that typechecking is not considered successful. - EntityType::Unspecified => TypecheckAnswer::fail( + (Some(EntityType::Unspecified), _) => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) .with_same_source_info(in_expr) .is_in(lhs_expr, rhs_expr), @@ -1881,8 +1945,8 @@ impl<'a> Typechecker<'a> { match lhs_euid.entity_type() { EntityType::Specified(name) => { // We don't want to apply the action hierarchy check to - // non-action entities. We have a set of entities, so We - // can apply the check as long as any are actions. The + // non-action entities, but now we have a set of entities. + // We can apply the check as long as any are actions. The // non-actions are omitted from the check, but they can // never be an ancestor of `Action`. let lhs_is_action = is_action_entity_type(name); @@ -1952,7 +2016,18 @@ impl<'a> Typechecker<'a> { rhs_expr: Expr>, ) -> TypecheckAnswer<'b> { let rhs_descendants = self.schema.get_actions_in_set(rhs); - Typechecker::entity_in_descendants(lhs, rhs_descendants, in_expr, lhs_expr, rhs_expr) + if let Some(rhs_descendants) = rhs_descendants { + Typechecker::entity_in_descendants(lhs, rhs_descendants, in_expr, lhs_expr, rhs_expr) + } else { + let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(in_expr) + .is_in(lhs_expr, rhs_expr); + if self.mode.is_partial() { + TypecheckAnswer::success(annotated_expr) + } else { + TypecheckAnswer::fail(annotated_expr) + } + } } // Get the type for `in` when it is applied to an non-action EUID literal @@ -1971,13 +2046,28 @@ impl<'a> Typechecker<'a> { match lhs.entity_type() { EntityType::Specified(lhs_ety) => { let rhs_descendants = self.schema.get_entity_types_in_set(rhs); - Typechecker::entity_in_descendants( - lhs_ety, - rhs_descendants, - in_expr, - lhs_expr, - rhs_expr, - ) + match rhs_descendants { + Some(rhs_descendants) if self.schema.is_known_entity_type(lhs_ety) => { + Typechecker::entity_in_descendants( + lhs_ety, + rhs_descendants, + in_expr, + lhs_expr, + rhs_expr, + ) + } + _ => { + let annotated_expr = + ExprBuilder::with_data(Some(Type::primitive_boolean())) + .with_same_source_info(in_expr) + .is_in(lhs_expr, rhs_expr); + if self.mode.is_partial() { + TypecheckAnswer::success(annotated_expr) + } else { + TypecheckAnswer::fail(annotated_expr) + } + } + } } EntityType::Unspecified => { // Unspecified entities will be detected by a different part of the validator. @@ -2190,7 +2280,10 @@ impl<'a> Typechecker<'a> { maybe_action_var: &'a Expr, ) -> Cow<'a, Expr> { match maybe_action_var.expr_kind() { - ExprKind::Var(Var::Action) => Cow::Owned(Expr::val(request_env.action.clone())), + ExprKind::Var(Var::Action) => match request_env.action_entity_uid() { + Some(action) => Cow::Owned(Expr::val(action.clone())), + None => Cow::Borrowed(maybe_action_var), + }, _ => Cow::Borrowed(maybe_action_var), } } 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..944ca10c00 --- /dev/null +++ b/cedar-policy-validator/src/typecheck/test_partial.rs @@ -0,0 +1,655 @@ +//! 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}; + +use super::test_utils::empty_schema_file; + +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."); +} + +pub(crate) fn assert_typechecks_empty_schema(policy: StaticPolicy) { + assert_partial_typecheck(empty_schema_file(), policy) +} + +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) +} + +mod passes_empty_schema { + + use super::*; + + #[test] + fn principal_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal.is_admin };"#, + ) + .unwrap(), + ); + } + + #[test] + fn action_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) unless { action.is_restricted };"#, + ) + .unwrap(), + ); + } + + #[test] + fn resource_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { resource.is_public };"#, + ) + .unwrap(), + ); + } + + #[test] + fn literal_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { User::"alice".is_admin };"#, + ) + .unwrap(), + ); + } + + #[test] + fn get_nested_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { resource.foo.bar.baz.buz };"#, + ) + .unwrap(), + ); + } + + #[test] + fn principal_has_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal has is_admin };"#, + ) + .unwrap(), + ); + } + + #[test] + fn action_has_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) unless { action has is_restricted };"#, + ) + .unwrap(), + ); + } + + #[test] + fn resource_has_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { resource has is_public };"#, + ) + .unwrap(), + ); + } + + #[test] + fn literal_has_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { User::"alice" has is_admin };"#, + ) + .unwrap(), + ); + } + + #[test] + fn has_nested_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { resource.foo has bar };"#, + ) + .unwrap(), + ); + } + + #[test] + fn principal_eq() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal == User::"alice", action, resource);"#, + ) + .unwrap(), + ); + } + + #[test] + fn action_eq() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action == Action::"view", resource);"#, + ) + .unwrap(), + ); + } + + #[test] + fn resource_eq() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource == Photo::"vacation.jpg");"#, + ) + .unwrap(), + ); + } + + #[test] + fn principal_in() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal in User::"alice", action, resource);"#, + ) + .unwrap(), + ); + } + + #[test] + fn action_in() { + // Test case with `in` are interesting because, for an undeclared + // entity type or action we don't know what should be `in` it, so the + // expression has type boolean (not known to be true or false). + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action in Action::"view", resource);"#, + ) + .unwrap(), + ); + } + + #[test] + fn resource_in() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource in Photo::"vacation.jpg");"#, + ) + .unwrap(), + ); + } + + #[test] + fn principal_in_set() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal in [User::"alice", Admin::"bob"] };"#, + ) + .unwrap(), + ); + } + + #[test] + fn action_in_set() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action in [Action::"view", Action::"edit"], resource);"#, + ) + .unwrap(), + ); + } + + #[test] + fn in_attr() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal in resource.owner };"#, + ) + .unwrap(), + ); + } + + #[test] + fn attr_in() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { resource.owner in principal };"#, + ) + .unwrap(), + ); + } + + #[test] + fn resource_in_set() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { resource in [Photo::"vacation.jpg", Album::"vacation_photos"] } ;"#, + ) + .unwrap(), + ); + } + + #[test] + fn if_both_branches_undefined() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { if principal.foo then action.bar else resource.bar };"# + ) + .unwrap(), + ); + } + + #[test] + fn if_one_branch_undefined() { + assert_typechecks_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { 0 < (if principal.foo then action.bar else 1) };"# + ) + .unwrap(), + ); + } + + #[test] + fn context_attr() { + let p = parse_policy( + Some("0".to_string()), + r#"permit(principal, action, resource) when { context.foo };"#, + ) + .expect("Policy should parse."); + assert_typechecks_partial_schema(p); + let p = parse_policy( + Some("0".to_string()), + r#"permit(principal, action, resource) when { context.foo.bar };"#, + ) + .expect("Policy should parse."); + assert_typechecks_partial_schema(p); + } +} + +mod fails_empty_schema { + use std::str::FromStr; + + use cedar_policy_core::ast::Expr; + + use crate::types::Type; + + use super::*; + + #[test] + fn operator_type_error() { + // We expect to see a type error for the incorrect literal argument to + // various operators. No error should be generated for missing + // attributes or the type of the attributes. + assert_typecheck_fails_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal.foo > "a" };"#, + ) + .unwrap(), + vec![TypeError::expected_type( + Expr::val("a"), + Type::primitive_long(), + Type::primitive_string(), + )], + ); + assert_typecheck_fails_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { 1.contains(principal.foo) };"#, + ) + .unwrap(), + vec![TypeError::expected_type( + Expr::val(1), + Type::any_set(), + Type::primitive_long(), + )], + ); + assert_typecheck_fails_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal.foo.containsAll(1) };"#, + ) + .unwrap(), + vec![TypeError::expected_type( + Expr::val(1), + Type::any_set(), + Type::primitive_long(), + )], + ); + } + + #[test] + fn top_level_type_error() { + assert_typecheck_fails_empty_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { principal.foo + 1 };"#, + ) + .unwrap(), + vec![TypeError::expected_type( + Expr::from_str("principal.foo + 1").unwrap(), + Type::primitive_boolean(), + Type::primitive_long(), + )], + ) + } + + #[test] + fn impossible_policy() { + let p = parse_policy( + None, + r#"permit(principal, action, resource) when { resource.bar && false };"#, + ) + .unwrap(); + assert_typecheck_fails_empty_schema( + p.clone(), + vec![TypeError::impossible_policy(p.condition())], + ) + } + + #[test] + fn record_lit_bad_attr() { + let p = parse_policy( + None, + r#"permit(principal, action, resource) when { {foo: 1}.bar };"#, + ) + .unwrap(); + assert_typecheck_fails_empty_schema( + p.clone(), + vec![TypeError::unsafe_attribute_access( + Expr::from_str("{foo: 1}.bar").unwrap(), + AttributeAccess::Other(vec!["bar".into()]), + Some("foo".into()), + false, + )], + ) + } +} + +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 undeclared_entity_type_in_declared_entity_type() { + assert_typechecks_partial_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { Admin::"alice" in User::"alice" };"#, + ) + .unwrap(), + ); + } + + #[test] + fn undeclared_action_in_declared_action() { + assert_typechecks_partial_schema( + parse_policy( + None, + r#"permit(principal, action, resource) when { Action::"undeclared" in Action::"view" };"#, + ) + .unwrap(), + ); + } + + #[test] + fn principal_wrong_for_known_actions() { + // `resource` can't be a `Group` for the defined actions, but there + // might be an undefined action where that's OK. + assert_typechecks_partial_schema( + parse_policy( + None, + r#"permit(principal, action, resource == Group::"owners");"#, + ) + .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..59b2685e5c 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, }; @@ -145,11 +145,11 @@ where { f( simple_schema_file(), - RequestEnv { + RequestEnv::DeclaredAction { 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..0c0e1b19ef 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, }; @@ -65,7 +65,7 @@ impl Typechecker<'_> { ) -> TypecheckAnswer<'a> { // Using bogus entity type names here for testing. They'll be treated as // having empty attribute records, so tests will behave as expected. - let request_env = RequestEnv { + let request_env = RequestEnv::DeclaredAction { principal: &EntityType::Specified( "Principal" .parse() @@ -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