From a39689477815e4799ae45335f6056656adb732f6 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 16 Dec 2024 13:38:18 -0800 Subject: [PATCH 01/28] prototype Signed-off-by: Shaobo He --- .../src/cedar_schema/fmt.rs | 2 +- .../src/cedar_schema/test.rs | 4 +- .../src/cedar_schema/to_json_schema.rs | 4 +- cedar-policy-validator/src/json_schema.rs | 115 ++++++++++++------ cedar-policy-validator/src/lib.rs | 4 +- cedar-policy-validator/src/rbac.rs | 24 ++-- .../src/schema/entity_type.rs | 51 ++++---- .../src/schema/namespace_def.rs | 49 ++++---- .../src/typecheck/test/expr.rs | 4 +- 9 files changed, 150 insertions(+), 107 deletions(-) diff --git a/cedar-policy-validator/src/cedar_schema/fmt.rs b/cedar-policy-validator/src/cedar_schema/fmt.rs index 5f20f11e1..f54e45237 100644 --- a/cedar-policy-validator/src/cedar_schema/fmt.rs +++ b/cedar-policy-validator/src/cedar_schema/fmt.rs @@ -105,7 +105,7 @@ fn fmt_vec(f: &mut std::fmt::Formatter<'_>, ets: NonEmpty) -> std write!(f, "[{contents}]") } -impl Display for json_schema::EntityType { +impl Display for json_schema::CommonEntityType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { if let Some(non_empty) = non_empty_slice(&self.member_of_types) { write!(f, " in ")?; diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs index bf72a6abb..ac285cc69 100644 --- a/cedar-policy-validator/src/cedar_schema/test.rs +++ b/cedar-policy-validator/src/cedar_schema/test.rs @@ -434,7 +434,7 @@ namespace Baz {action "Foo" appliesTo { common_types: BTreeMap::new(), entity_types: BTreeMap::from([( "a".parse().unwrap(), - json_schema::EntityType:: { + json_schema::CommonEntityType:: { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -631,7 +631,7 @@ namespace Baz {action "Foo" appliesTo { } #[track_caller] - fn assert_empty_record(etyp: &json_schema::EntityType) { + fn assert_empty_record(etyp: &json_schema::CommonEntityType) { assert!(etyp.shape.is_empty_record()); } diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index f696c2f98..1b511becc 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -361,11 +361,11 @@ fn convert_id(node: Node) -> Result { fn convert_entity_decl( e: Annotated, ) -> Result< - impl Iterator)>, + impl Iterator)>, ToJsonSchemaErrors, > { // First build up the defined entity type - let etype = json_schema::EntityType { + let etype = json_schema::CommonEntityType { member_of_types: e .data .member_of_types diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index f51b2f6a7..f43bf5ef6 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -407,6 +407,34 @@ impl NamespaceDefinition { } } +/// ... +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(bound(deserialize = "N: Deserialize<'de> + From"))] +#[serde(untagged)] +pub enum EntityTypeKind { + /// ... + Common(CommonEntityType), + /// ... + Enum { + #[serde(rename = "Enum")] + /// ... + choices: Vec, + }, +} + +/// Represents the definition of a common type in the schema. +#[derive(Debug, Clone, Serialize, PartialEq, Eq, Deserialize)] +#[serde(bound(deserialize = "N: Deserialize<'de> + From"))] +pub struct EntityType { + /// The referred type + #[serde(flatten)] + pub kind: EntityTypeKind, + /// Annotations + #[serde(default)] + #[serde(skip_serializing_if = "Annotations::is_empty")] + pub annotations: Annotations, +} + /// Represents the full definition of an entity type in the schema. /// Entity types describe the relationships in the entity store, including what /// entities can be members of groups of what types, and what attributes @@ -421,7 +449,7 @@ impl NamespaceDefinition { #[serde(rename_all = "camelCase")] #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] #[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] -pub struct EntityType { +pub struct CommonEntityType { /// Entities of this [`EntityType`] are allowed to be members of entities of /// these types. #[serde(default)] @@ -435,10 +463,6 @@ pub struct EntityType { #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub tags: Option>, - /// Annotations - #[serde(default)] - #[serde(skip_serializing_if = "Annotations::is_empty")] - pub annotations: Annotations, } impl EntityType { @@ -447,18 +471,22 @@ impl EntityType { self, ns: Option<&InternalName>, ) -> EntityType { - EntityType { - member_of_types: self - .member_of_types - .into_iter() - .map(|rname| rname.conditionally_qualify_with(ns, ReferenceType::Entity)) // Only entity, not common, here for now; see #1064 - .collect(), - shape: self.shape.conditionally_qualify_type_references(ns), - tags: self - .tags - .map(|ty| ty.conditionally_qualify_type_references(ns)), - annotations: self.annotations, - } + let Self { kind, annotations } = self; + let kind = match kind { + EntityTypeKind::Common(et) => EntityTypeKind::Common(CommonEntityType { + member_of_types: et + .member_of_types + .into_iter() + .map(|rname| rname.conditionally_qualify_with(ns, ReferenceType::Entity)) // Only entity, not common, here for now; see #1064 + .collect(), + shape: et.shape.conditionally_qualify_type_references(ns), + tags: et + .tags + .map(|ty| ty.conditionally_qualify_type_references(ns)), + }), + EntityTypeKind::Enum { choices } => EntityTypeKind::Enum { choices }, + }; + EntityType { kind, annotations } } } @@ -473,19 +501,28 @@ impl EntityType { self, all_defs: &AllDefs, ) -> std::result::Result, TypeNotDefinedError> { - Ok(EntityType { - member_of_types: self - .member_of_types - .into_iter() - .map(|cname| cname.resolve(all_defs)) - .collect::>()?, - shape: self.shape.fully_qualify_type_references(all_defs)?, - tags: self - .tags - .map(|ty| ty.fully_qualify_type_references(all_defs)) - .transpose()?, - annotations: self.annotations, - }) + let Self { kind, annotations } = self; + match kind { + EntityTypeKind::Common(et) => Ok(EntityType { + kind: EntityTypeKind::Common(CommonEntityType { + member_of_types: et + .member_of_types + .into_iter() + .map(|cname| cname.resolve(all_defs)) + .collect::>()?, + shape: et.shape.fully_qualify_type_references(all_defs)?, + tags: et + .tags + .map(|ty| ty.fully_qualify_type_references(all_defs)) + .transpose()?, + }), + annotations: annotations, + }), + EntityTypeKind::Enum { choices } => Ok(EntityType { + kind: EntityTypeKind::Enum { choices }, + annotations, + }), + } } } @@ -1834,7 +1871,7 @@ mod test { "memberOfTypes" : ["UserGroup"] } "#; - let et = serde_json::from_str::>(user).expect("Parse Error"); + let et = serde_json::from_str::>(user).expect("Parse Error"); assert_eq!(et.member_of_types, vec!["UserGroup".parse().unwrap()]); assert_eq!( et.shape, @@ -1850,7 +1887,7 @@ mod test { let src = r#" { } "#; - let et = serde_json::from_str::>(src).expect("Parse Error"); + let et = serde_json::from_str::>(src).expect("Parse Error"); assert_eq!(et.member_of_types.len(), 0); assert_eq!( et.shape, @@ -2242,7 +2279,7 @@ mod strengthened_types { use cool_asserts::assert_matches; use super::{ - ActionEntityUID, ApplySpec, EntityType, Fragment, NamespaceDefinition, RawName, Type, + ActionEntityUID, ApplySpec, CommonEntityType, Fragment, NamespaceDefinition, RawName, Type, }; /// Assert that `result` is an `Err`, and the error message matches `msg` @@ -2385,28 +2422,28 @@ mod strengthened_types { { "memberOfTypes": [""] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name ``: unexpected end of input"); let src = serde_json::json!( { "memberOfTypes": ["*"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `*`: unexpected token `*`"); let src = serde_json::json!( { "memberOfTypes": ["A::"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `A::`: unexpected end of input"); let src = serde_json::json!( { "memberOfTypes": ["::A"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `::A`: unexpected token `::`"); } @@ -2802,7 +2839,7 @@ mod test_json_roundtrip { common_types: BTreeMap::new(), entity_types: BTreeMap::from([( "a".parse().unwrap(), - EntityType { + CommonEntityType { member_of_types: vec!["a".parse().unwrap()], shape: AttributesOrContext(Type::Type(TypeVariant::Record(RecordType { attributes: BTreeMap::new(), @@ -2848,7 +2885,7 @@ mod test_json_roundtrip { common_types: BTreeMap::new(), entity_types: BTreeMap::from([( "a".parse().unwrap(), - EntityType { + CommonEntityType { member_of_types: vec!["a".parse().unwrap()], shape: AttributesOrContext(Type::Type(TypeVariant::Record( RecordType { diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index 9a44d451d..006bdd6a7 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -289,7 +289,7 @@ mod test { [ ( foo_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -298,7 +298,7 @@ mod test { ), ( bar_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index 04ca08156..bd3083ee3 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -482,7 +482,7 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( foo_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -518,7 +518,7 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( "foo_type".parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -606,7 +606,7 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( p_name.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -632,7 +632,7 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( p_name.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -658,7 +658,7 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( p_name.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -1020,7 +1020,7 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( foo_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -1055,7 +1055,7 @@ mod test { [ ( principal_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -1064,7 +1064,7 @@ mod test { ), ( resource_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -1449,7 +1449,7 @@ mod test { [ ( principal_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -1458,7 +1458,7 @@ mod test { ), ( resource_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![resource_parent_type.parse().unwrap()], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -1467,7 +1467,7 @@ mod test { ), ( resource_parent_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![resource_grandparent_type.parse().unwrap()], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -1476,7 +1476,7 @@ mod test { ), ( resource_grandparent_type.parse().unwrap(), - json_schema::EntityType { + json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, diff --git a/cedar-policy-validator/src/schema/entity_type.rs b/cedar-policy-validator/src/schema/entity_type.rs index 5c15da2df..a3a092782 100644 --- a/cedar-policy-validator/src/schema/entity_type.rs +++ b/cedar-policy-validator/src/schema/entity_type.rs @@ -34,30 +34,33 @@ use cedar_policy_core::ast; /// the struct are the same as the schema entity type structure, but the /// `member_of` relation is reversed to instead be `descendants`. #[derive(Clone, Debug, Serialize)] -pub struct ValidatorEntityType { - /// The name of the entity type. - pub(crate) name: EntityType, - - /// The set of entity types that can be members of this entity type. When - /// this structure is initially constructed, the field will contain direct - /// children, but it will be updated to contain the closure of all - /// descendants before it is used in any validation. - pub descendants: HashSet, - - /// The attributes associated with this entity. - 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, - - /// Tag type for this entity type. `None` indicates that entities of this - /// type are not allowed to have tags. - #[serde(skip_serializing_if = "Option::is_none")] - pub(crate) tags: Option, +pub enum ValidatorEntityType { + Common { + /// The name of the entity type. + name: EntityType, + + /// The set of entity types that can be members of this entity type. When + /// this structure is initially constructed, the field will contain direct + /// children, but it will be updated to contain the closure of all + /// descendants before it is used in any validation. + descendants: HashSet, + + /// The attributes associated with this entity. + 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. + open_attributes: OpenTag, + + /// Tag type for this entity type. `None` indicates that entities of this + /// type are not allowed to have tags. + #[serde(skip_serializing_if = "Option::is_none")] + tags: Option, + }, + Enum(Vec), } impl ValidatorEntityType { diff --git a/cedar-policy-validator/src/schema/namespace_def.rs b/cedar-policy-validator/src/schema/namespace_def.rs index 5c98bf685..e7fa98d28 100644 --- a/cedar-policy-validator/src/schema/namespace_def.rs +++ b/cedar-policy-validator/src/schema/namespace_def.rs @@ -446,29 +446,32 @@ impl EntityTypesDef { /// references in `parents`, `attributes`, and `tags` may or may not be fully /// qualified yet, depending on `N`. #[derive(Debug, Clone)] -pub struct EntityTypeFragment { - /// Description of the attribute types for this entity type. - /// - /// This may contain references to common types which have not yet been - /// resolved/inlined (e.g., because they are not defined in this schema - /// fragment). - /// In the extreme case, this may itself be just a common type pointing to a - /// `Record` type defined in another fragment. - pub(super) attributes: json_schema::AttributesOrContext, - /// Direct parent entity types for this entity type. - /// These entity types may be declared in a different namespace or schema - /// fragment. - /// - /// We will check for undeclared parent types when combining fragments into - /// a [`crate::ValidatorSchema`]. - pub(super) parents: HashSet, - /// Tag type for this entity type. `None` means no tags are allowed on this - /// entity type. - /// - /// This may contain references to common types which have not yet been - /// resolved/inlined (e.g., because they are not defined in this schema - /// fragment). - pub(super) tags: Option>, +pub enum EntityTypeFragment { + Common { + /// Description of the attribute types for this entity type. + /// + /// This may contain references to common types which have not yet been + /// resolved/inlined (e.g., because they are not defined in this schema + /// fragment). + /// In the extreme case, this may itself be just a common type pointing to a + /// `Record` type defined in another fragment. + attributes: json_schema::AttributesOrContext, + /// Direct parent entity types for this entity type. + /// These entity types may be declared in a different namespace or schema + /// fragment. + /// + /// We will check for undeclared parent types when combining fragments into + /// a [`crate::ValidatorSchema`]. + parents: HashSet, + /// Tag type for this entity type. `None` means no tags are allowed on this + /// entity type. + /// + /// This may contain references to common types which have not yet been + /// resolved/inlined (e.g., because they are not defined in this schema + /// fragment). + tags: Option>, + }, + Enum(Vec), } impl EntityTypeFragment { diff --git a/cedar-policy-validator/src/typecheck/test/expr.rs b/cedar-policy-validator/src/typecheck/test/expr.rs index 5a80d8a08..5b0b94cca 100644 --- a/cedar-policy-validator/src/typecheck/test/expr.rs +++ b/cedar-policy-validator/src/typecheck/test/expr.rs @@ -64,7 +64,7 @@ fn slot_typechecks() { #[test] fn slot_in_typechecks() { - let etype = json_schema::EntityType { + let etype = json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, @@ -95,7 +95,7 @@ fn slot_in_typechecks() { #[test] fn slot_equals_typechecks() { - let etype = json_schema::EntityType { + let etype = json_schema::CommonEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, From 4a083f8093bca999a9945c9d3643245fa41511bb Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Sat, 4 Jan 2025 14:50:12 -0800 Subject: [PATCH 02/28] non-breaking version Signed-off-by: Shaobo He --- .../src/cedar_schema/fmt.rs | 11 +- .../src/cedar_schema/test.rs | 169 +++++++------- .../src/cedar_schema/to_json_schema.rs | 24 +- cedar-policy-validator/src/coreschema.rs | 5 +- .../src/entity_manifest/type_annotations.rs | 6 +- cedar-policy-validator/src/json_schema.rs | 188 +++++++++------ cedar-policy-validator/src/lib.rs | 14 +- cedar-policy-validator/src/rbac.rs | 84 +++---- cedar-policy-validator/src/schema.rs | 215 +++++++++++------- .../src/schema/entity_type.rs | 134 +++++++---- .../src/schema/namespace_def.rs | 134 ++++++----- .../src/typecheck/test/expr.rs | 15 +- cedar-policy-validator/src/types.rs | 10 +- 13 files changed, 596 insertions(+), 413 deletions(-) diff --git a/cedar-policy-validator/src/cedar_schema/fmt.rs b/cedar-policy-validator/src/cedar_schema/fmt.rs index 3e084c7e5..f73b0e3b4 100644 --- a/cedar-policy-validator/src/cedar_schema/fmt.rs +++ b/cedar-policy-validator/src/cedar_schema/fmt.rs @@ -106,7 +106,16 @@ fn fmt_non_empty_slice( write!(f, "]") } -impl Display for json_schema::CommonEntityType { +impl Display for json_schema::EntityType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self.kind { + json_schema::EntityTypeKind::Standard(ty) => ty.fmt(f), + _ => todo!(), + } + } +} + +impl Display for json_schema::StandardEntityType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { if let Some(non_empty) = self.member_of_types.split_first() { write!(f, " in ")?; diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs index 9963ca78c..d268e6572 100644 --- a/cedar-policy-validator/src/cedar_schema/test.rs +++ b/cedar-policy-validator/src/cedar_schema/test.rs @@ -40,7 +40,7 @@ mod demo_tests { ast::PR, err::{ToJsonSchemaError, NO_PR_HELP_MSG}, }, - json_schema, + json_schema::{self, EntityType, EntityTypeKind}, schema::test::utils::collect_warnings, CedarSchemaError, RawName, }; @@ -441,13 +441,12 @@ namespace Baz {action "Foo" appliesTo { common_types: BTreeMap::new(), entity_types: BTreeMap::from([( "a".parse().unwrap(), - json_schema::CommonEntityType:: { + json_schema::StandardEntityType:: { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), )]), actions: BTreeMap::from([( "j".to_smolstr(), @@ -547,30 +546,30 @@ namespace Baz {action "Foo" appliesTo { .get(&Some("GitHub".parse().unwrap())) .expect("`Github` name space did not exist"); // User - let user = github + assert_matches!(github .entity_types .get(&"User".parse().unwrap()) - .expect("No `User`"); + .expect("No `User`"), EntityType { kind: EntityTypeKind::Standard(user), ..} => { assert_empty_record(user); assert_eq!( &user.member_of_types, &vec!["UserGroup".parse().unwrap(), "Team".parse().unwrap()] - ); + );}); // UserGroup - let usergroup = &github + assert_matches!(github .entity_types .get(&"UserGroup".parse().unwrap()) - .expect("No `UserGroup`"); + .expect("No `UserGroup`"), EntityType { kind: EntityTypeKind::Standard(usergroup), ..} => { assert_empty_record(usergroup); assert_eq!( &usergroup.member_of_types, &vec!["UserGroup".parse().unwrap()] - ); + );}); // Repository - let repo = github + assert_matches!(github .entity_types .get(&"Repository".parse().unwrap()) - .expect("No `Repository`"); + .expect("No `Repository`"), EntityType {kind: EntityTypeKind::Standard(repo), ..} => { assert!(repo.member_of_types.is_empty()); let groups = ["readers", "writers", "triagers", "admins", "maintainers"]; for group in groups { @@ -585,11 +584,11 @@ namespace Baz {action "Foo" appliesTo { let attribute = attributes.get(group).expect("No attribute `{group}`"); assert_has_type(attribute, expected); }); - } - let issue = github + }}); + assert_matches!(github .entity_types .get(&"Issue".parse().unwrap()) - .expect("No `Issue`"); + .expect("No `Issue`"), EntityType {kind: EntityTypeKind::Standard(issue), .. } => { assert!(issue.member_of_types.is_empty()); assert_matches!(&issue.shape, json_schema::AttributesOrContext(json_schema::Type::Type(json_schema::TypeVariant::Record(json_schema::RecordType { attributes, @@ -609,11 +608,11 @@ namespace Baz {action "Foo" appliesTo { type_name: "User".parse().unwrap(), }), ); - }); - let org = github + });}); + assert_matches!(github .entity_types .get(&"Org".parse().unwrap()) - .expect("No `Org`"); + .expect("No `Org`"), EntityType { kind: EntityTypeKind::Standard(org), .. } => { assert!(org.member_of_types.is_empty()); let groups = ["members", "owners", "memberOfTypes"]; for group in groups { @@ -627,7 +626,7 @@ namespace Baz {action "Foo" appliesTo { let attribute = attributes.get(group).expect("No attribute `{group}`"); assert_has_type(attribute, expected); }); - } + }}); } #[track_caller] @@ -640,7 +639,7 @@ namespace Baz {action "Foo" appliesTo { } #[track_caller] - fn assert_empty_record(etyp: &json_schema::CommonEntityType) { + fn assert_empty_record(etyp: &json_schema::StandardEntityType) { assert!(etyp.shape.is_empty_record()); } @@ -675,10 +674,10 @@ namespace Baz {action "Foo" appliesTo { .0 .get(&Some("DocCloud".parse().unwrap())) .expect("No `DocCloud` namespace"); - let user = doccloud + assert_matches!(doccloud .entity_types .get(&"User".parse().unwrap()) - .expect("No `User`"); + .expect("No `User`"), EntityType {kind: EntityTypeKind::Standard(user), ..} => { assert_eq!(&user.member_of_types, &vec!["Group".parse().unwrap()]); assert_matches!(&user.shape, json_schema::AttributesOrContext(json_schema::Type::Type(json_schema::TypeVariant::Record(json_schema::RecordType { attributes, @@ -698,11 +697,11 @@ namespace Baz {action "Foo" appliesTo { })), }), ); - }); - let group = doccloud + });}); + assert_matches!(doccloud .entity_types .get(&"Group".parse().unwrap()) - .expect("No `Group`"); + .expect("No `Group`"), EntityType { kind: EntityTypeKind::Standard(group), .. } => { assert_eq!( &group.member_of_types, &vec!["DocumentShare".parse().unwrap()] @@ -717,11 +716,11 @@ namespace Baz {action "Foo" appliesTo { type_name: "User".parse().unwrap(), }), ); - }); - let document = doccloud + });}); + assert_matches!(doccloud .entity_types .get(&"Document".parse().unwrap()) - .expect("No `Group`"); + .expect("No `Group`"), EntityType { kind: EntityTypeKind::Standard(document), ..} => { assert!(document.member_of_types.is_empty()); assert_matches!(&document.shape, json_schema::AttributesOrContext(json_schema::Type::Type(json_schema::TypeVariant::Record(json_schema::RecordType { attributes, @@ -763,30 +762,33 @@ namespace Baz {action "Foo" appliesTo { type_name: "DocumentShare".parse().unwrap(), }), ); - }); - let document_share = doccloud + });}); + assert_matches!(doccloud .entity_types .get(&"DocumentShare".parse().unwrap()) - .expect("No `DocumentShare`"); + .expect("No `DocumentShare`"), EntityType { kind: EntityTypeKind::Standard(document_share), ..} => { assert!(document_share.member_of_types.is_empty()); assert_empty_record(document_share); + }); - let public = doccloud - .entity_types - .get(&"Public".parse().unwrap()) - .expect("No `Public`"); - assert_eq!( - &public.member_of_types, - &vec!["DocumentShare".parse().unwrap()] - ); - assert_empty_record(public); + assert_matches!(doccloud + .entity_types + .get(&"Public".parse().unwrap()) + .expect("No `Public`"), EntityType { kind: EntityTypeKind::Standard(public), ..} => { + assert_eq!( + &public.member_of_types, + &vec!["DocumentShare".parse().unwrap()] + ); + assert_empty_record(public); + }); - let drive = doccloud + assert_matches!(doccloud .entity_types .get(&"Drive".parse().unwrap()) - .expect("No `Drive`"); + .expect("No `Drive`"), EntityType { kind: EntityTypeKind::Standard(drive), ..} => { assert!(drive.member_of_types.is_empty()); assert_empty_record(drive); + }); } #[test] @@ -891,10 +893,10 @@ namespace Baz {action "Foo" appliesTo { json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); assert_eq!(warnings.collect::>(), vec![]); let service = fragment.0.get(&Some("Service".parse().unwrap())).unwrap(); - let resource = &service + assert_matches!(service .entity_types .get(&"Resource".parse().unwrap()) - .unwrap(); + .unwrap(), EntityType { kind: EntityTypeKind::Standard(resource), ..} => { assert_matches!(&resource.shape, json_schema::AttributesOrContext(json_schema::Type::Type(json_schema::TypeVariant::Record(json_schema::RecordType { attributes, additional_attributes: false, @@ -904,7 +906,7 @@ namespace Baz {action "Foo" appliesTo { assert_eq!(type_name, &"AWS::Tag".parse().unwrap()); }); }); - }); + });}); } #[test] @@ -1169,6 +1171,7 @@ mod translator_tests { use cedar_policy_core::FromNormalizedStr; use cool_asserts::assert_matches; + use crate::json_schema::{EntityType, EntityTypeKind}; use crate::{ cedar_schema::{ err::ToJsonSchemaError, parser::parse_schema, @@ -1426,7 +1429,7 @@ mod translator_tests { let (frag, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let demo = frag.0.get(&Some("Demo".parse().unwrap())).unwrap(); - let user = &demo.entity_types.get(&"User".parse().unwrap()).unwrap(); + assert_matches!(demo.entity_types.get(&"User".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(user), ..} => { assert_matches!(&user.shape, json_schema::AttributesOrContext(json_schema::Type::Type(json_schema::TypeVariant::Record(json_schema::RecordType { attributes, additional_attributes: false, @@ -1443,7 +1446,7 @@ mod translator_tests { }); assert_eq!(ty, &expected); }); - }); + });}); assert_matches!(ValidatorSchema::try_from(frag), Err(e) => { expect_err( src, @@ -1507,8 +1510,8 @@ mod translator_tests { validator_schema .get_entity_type(&"A::B".parse().unwrap()) .unwrap() - .attributes - .attrs["foo"] + .attr("foo") + .unwrap() .attr_type, Type::EntityOrRecord(EntityRecordKind::Entity(EntityLUB::single_entity( "X::Z".parse().unwrap() @@ -1526,8 +1529,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["namespace".parse().unwrap()]); + }); } #[test] @@ -1557,8 +1561,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["Set".parse().unwrap()]); + }); } #[test] @@ -1571,8 +1576,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["appliesTo".parse().unwrap()]); + }); } #[test] @@ -1585,8 +1591,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["principal".parse().unwrap()]); + }); } #[test] @@ -1599,8 +1606,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["resource".parse().unwrap()]); + }); } #[test] @@ -1613,8 +1621,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["action".parse().unwrap()]); + }); } #[test] @@ -1627,8 +1636,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); - assert_eq!(foo.member_of_types, vec!["context".parse().unwrap()]); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { + assert_eq!(foo.member_of_types, vec!["context".parse().unwrap()]); + }); } #[test] @@ -1641,8 +1651,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); - assert_eq!(foo.member_of_types, vec!["attributes".parse().unwrap()]); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { + assert_eq!(foo.member_of_types, vec!["attributes".parse().unwrap()]); + }); } #[test] @@ -1655,8 +1666,9 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); - assert_eq!(foo.member_of_types, vec!["Bool".parse().unwrap()]); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { + assert_eq!(foo.member_of_types, vec!["Bool".parse().unwrap()]); + }); } #[test] @@ -1669,8 +1681,8 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); - assert_eq!(foo.member_of_types, vec!["Long".parse().unwrap()]); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["Long".parse().unwrap()]); + }); } #[test] @@ -1683,8 +1695,8 @@ mod translator_tests { let (schema, _) = json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); - let foo = ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(); - assert_eq!(foo.member_of_types, vec!["String".parse().unwrap()]); + assert_matches!(ns.entity_types.get(&"Foo".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(foo), ..} => { assert_eq!(foo.member_of_types, vec!["String".parse().unwrap()]); + }); } #[test] @@ -2586,7 +2598,7 @@ mod common_type_references { /// Tests involving entity tags (RFC 82) mod entity_tags { - use crate::json_schema; + use crate::json_schema::{self, EntityType, EntityTypeKind}; use crate::schema::test::utils::collect_warnings; use cedar_policy_core::extensions::Extensions; use cool_asserts::assert_matches; @@ -2596,34 +2608,35 @@ mod entity_tags { let src = "entity E;"; assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => { assert!(warnings.is_empty()); - let entity_type = frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(entity_type), ..} => { assert_matches!(&entity_type.tags, None); + }); }); let src = "entity E tags String;"; assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => { assert!(warnings.is_empty()); - let entity_type = frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(entity_type), ..} => { assert_matches!(&entity_type.tags, Some(json_schema::Type::Type(json_schema::TypeVariant::EntityOrCommon { type_name })) => { assert_eq!(&format!("{type_name}"), "String"); }); - }); + });}); let src = "entity E tags Set;"; assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => { assert!(warnings.is_empty()); - let entity_type = frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(entity_type), ..} => { assert_matches!(&entity_type.tags, Some(json_schema::Type::Type(json_schema::TypeVariant::Set { element })) => { assert_matches!(&**element, json_schema::Type::Type(json_schema::TypeVariant::EntityOrCommon { type_name }) => { assert_eq!(&format!("{type_name}"), "String"); }); }); - }); + });}); let src = "entity E { foo: String } tags { foo: String };"; assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => { assert!(warnings.is_empty()); - let entity_type = frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(entity_type), ..} => { assert_matches!(&entity_type.tags, Some(json_schema::Type::Type(json_schema::TypeVariant::Record(rty))) => { assert_matches!(rty.attributes.get("foo"), Some(json_schema::TypeOfAttribute { ty, required, .. }) => { assert_matches!(ty, json_schema::Type::Type(json_schema::TypeVariant::EntityOrCommon { type_name }) => { @@ -2632,25 +2645,25 @@ mod entity_tags { assert!(*required); }); }); - }); + });}); let src = "type T = String; entity E tags T;"; assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => { assert!(warnings.is_empty()); - let entity_type = frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(entity_type), ..} => { assert_matches!(&entity_type.tags, Some(json_schema::Type::Type(json_schema::TypeVariant::EntityOrCommon { type_name })) => { assert_eq!(&format!("{type_name}"), "T"); }); - }); + });}); let src = "entity E tags E;"; assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => { assert!(warnings.is_empty()); - let entity_type = frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(entity_type), ..} => { assert_matches!(&entity_type.tags, Some(json_schema::Type::Type(json_schema::TypeVariant::EntityOrCommon { type_name })) => { assert_eq!(&format!("{type_name}"), "E"); }); - }); + });}); } } diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index 2706c6b18..0995cd168 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -360,20 +360,22 @@ fn convert_id(node: Node) -> Result { fn convert_entity_decl( e: Annotated>, ) -> Result< - impl Iterator)>, + impl Iterator)>, ToJsonSchemaErrors, > { // First build up the defined entity type - let etype = json_schema::CommonEntityType { - member_of_types: e - .data - .node - .member_of_types - .into_iter() - .map(RawName::from) - .collect(), - shape: convert_attr_decls(e.data.node.attrs), - tags: e.data.node.tags.map(cedar_type_to_json_type), + let etype = json_schema::EntityType { + kind: json_schema::EntityTypeKind::Standard(json_schema::StandardEntityType { + member_of_types: e + .data + .node + .member_of_types + .into_iter() + .map(RawName::from) + .collect(), + shape: convert_attr_decls(e.data.node.attrs), + tags: e.data.node.tags.map(cedar_type_to_json_type), + }), annotations: e.annotations.into(), loc: Some(e.data.loc), }; diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 18c4c18d4..3ebaa69a3 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -142,8 +142,7 @@ impl entities::EntityTypeDescription for EntityTypeDescription { fn required_attrs<'s>(&'s self) -> Box + 's> { Box::new( self.validator_type - .attributes - .iter() + .attributes() .filter(|(_, ty)| ty.is_required) .map(|(attr, _)| attr.clone()), ) @@ -154,7 +153,7 @@ impl entities::EntityTypeDescription for EntityTypeDescription { } fn open_attributes(&self) -> bool { - self.validator_type.open_attributes.is_open() + self.validator_type.open_attributes().is_open() } } diff --git a/cedar-policy-validator/src/entity_manifest/type_annotations.rs b/cedar-policy-validator/src/entity_manifest/type_annotations.rs index 0bf4b071e..84403a789 100644 --- a/cedar-policy-validator/src/entity_manifest/type_annotations.rs +++ b/cedar-policy-validator/src/entity_manifest/type_annotations.rs @@ -145,7 +145,11 @@ impl AccessTrie { .ok_or(MismatchedNotStrictSchemaError {})?, ) .ok_or(MismatchedNotStrictSchemaError {})?; - &entity_ty.attributes + &Attributes::with_required_attributes( + entity_ty + .attributes() + .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())), + ) } EntityRecordKind::ActionEntity { name: _, attrs } => attrs, }; diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index 0399f86e5..874ca8914 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -431,7 +431,7 @@ impl NamespaceDefinition { #[serde(untagged)] pub enum EntityTypeKind { /// ... - Common(CommonEntityType), + Standard(StandardEntityType), /// ... Enum { #[serde(rename = "Enum")] @@ -441,7 +441,8 @@ pub enum EntityTypeKind { } /// Represents the definition of a common type in the schema. -#[derive(Debug, Clone, Serialize, PartialEq, Eq, Deserialize)] +#[derive(Educe, Debug, Clone, Serialize, Deserialize)] +#[educe(PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] pub struct EntityType { /// The referred type @@ -451,6 +452,14 @@ pub struct EntityType { #[serde(default)] #[serde(skip_serializing_if = "Annotations::is_empty")] pub annotations: Annotations, + /// Source location + /// + /// (As of this writing, this is not populated when parsing from JSON. + /// It is only populated if constructing this structure from the + /// corresponding Cedar-syntax structure.) + #[serde(skip)] + #[educe(PartialEq(ignore))] + pub loc: Option, } /// Represents the full definition of an entity type in the schema. @@ -461,14 +470,13 @@ pub struct EntityType { /// The parameter `N` is the type of entity type names and common type names in /// this [`EntityType`], including recursively. /// See notes on [`Fragment`]. -#[derive(Educe, Debug, Clone, Serialize, Deserialize)] -#[educe(PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] #[serde(deny_unknown_fields)] #[serde(rename_all = "camelCase")] #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] #[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] -pub struct CommonEntityType { +pub struct StandardEntityType { /// Entities of this [`EntityType`] are allowed to be members of entities of /// these types. #[serde(default)] @@ -482,18 +490,17 @@ pub struct CommonEntityType { #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub tags: Option>, - /// Annotations - #[serde(default)] - #[serde(skip_serializing_if = "Annotations::is_empty")] - pub annotations: Annotations, - /// Source location - /// - /// (As of this writing, this is not populated when parsing from JSON. - /// It is only populated if constructing this structure from the - /// corresponding Cedar-syntax structure.) - #[serde(skip)] - #[educe(PartialEq(ignore))] - pub loc: Option, +} + +#[cfg(test)] +impl From> for EntityType { + fn from(value: StandardEntityType) -> Self { + Self { + kind: EntityTypeKind::Standard(value), + annotations: Annotations::new(), + loc: None, + } + } } impl EntityType { @@ -502,18 +509,32 @@ impl EntityType { self, ns: Option<&InternalName>, ) -> EntityType { - EntityType { - member_of_types: self - .member_of_types - .into_iter() - .map(|rname| rname.conditionally_qualify_with(ns, ReferenceType::Entity)) // Only entity, not common, here for now; see #1064 - .collect(), - shape: self.shape.conditionally_qualify_type_references(ns), - tags: self - .tags - .map(|ty| ty.conditionally_qualify_type_references(ns)), - annotations: self.annotations, - loc: self.loc, + let Self { + kind, + annotations, + loc, + } = self; + match kind { + EntityTypeKind::Enum { choices } => EntityType { + kind: EntityTypeKind::Enum { choices }, + annotations, + loc, + }, + EntityTypeKind::Standard(ty) => EntityType { + kind: EntityTypeKind::Standard(StandardEntityType { + member_of_types: ty + .member_of_types + .into_iter() + .map(|rname| rname.conditionally_qualify_with(ns, ReferenceType::Entity)) // Only entity, not common, here for now; see #1064 + .collect(), + shape: ty.shape.conditionally_qualify_type_references(ns), + tags: ty + .tags + .map(|ty| ty.conditionally_qualify_type_references(ns)), + }), + annotations, + loc, + }, } } } @@ -529,19 +550,33 @@ impl EntityType { self, all_defs: &AllDefs, ) -> std::result::Result, TypeNotDefinedError> { - Ok(EntityType { - member_of_types: self - .member_of_types - .into_iter() - .map(|cname| cname.resolve(all_defs)) - .collect::>()?, - shape: self.shape.fully_qualify_type_references(all_defs)?, - tags: self - .tags - .map(|ty| ty.fully_qualify_type_references(all_defs)) - .transpose()?, - annotations: self.annotations, - loc: self.loc, + let Self { + kind, + annotations, + loc, + } = self; + Ok(match kind { + EntityTypeKind::Enum { choices } => EntityType { + kind: EntityTypeKind::Enum { choices }, + annotations, + loc, + }, + EntityTypeKind::Standard(ty) => EntityType { + kind: EntityTypeKind::Standard(StandardEntityType { + member_of_types: ty + .member_of_types + .into_iter() + .map(|cname| cname.resolve(all_defs)) + .collect::>()?, + shape: ty.shape.fully_qualify_type_references(all_defs)?, + tags: ty + .tags + .map(|ty| ty.fully_qualify_type_references(all_defs)) + .transpose()?, + }), + annotations, + loc, + }, }) } } @@ -1903,7 +1938,7 @@ mod test { "memberOfTypes" : ["UserGroup"] } "#; - let et = serde_json::from_str::>(user).expect("Parse Error"); + let et = serde_json::from_str::>(user).expect("Parse Error"); assert_eq!(et.member_of_types, vec!["UserGroup".parse().unwrap()]); assert_eq!( et.shape, @@ -1919,7 +1954,7 @@ mod test { let src = r#" { } "#; - let et = serde_json::from_str::>(src).expect("Parse Error"); + let et = serde_json::from_str::>(src).expect("Parse Error"); assert_eq!(et.member_of_types.len(), 0); assert_eq!( et.shape, @@ -2311,7 +2346,8 @@ mod strengthened_types { use cool_asserts::assert_matches; use super::{ - ActionEntityUID, ApplySpec, CommonEntityType, Fragment, NamespaceDefinition, RawName, Type, + ActionEntityUID, ApplySpec, Fragment, NamespaceDefinition, RawName, StandardEntityType, + Type, }; /// Assert that `result` is an `Err`, and the error message matches `msg` @@ -2454,28 +2490,28 @@ mod strengthened_types { { "memberOfTypes": [""] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name ``: unexpected end of input"); let src = serde_json::json!( { "memberOfTypes": ["*"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `*`: unexpected token `*`"); let src = serde_json::json!( { "memberOfTypes": ["A::"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `A::`: unexpected end of input"); let src = serde_json::json!( { "memberOfTypes": ["::A"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `::A`: unexpected token `::`"); } @@ -2723,15 +2759,15 @@ mod entity_tags { fn basic() { let json = example_json_schema(); assert_matches!(Fragment::from_json_value(json), Ok(frag) => { - let user = &frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(user), ..} => { assert_matches!(&user.tags, Some(Type::Type(TypeVariant::Set { element })) => { assert_matches!(&**element, Type::Type(TypeVariant::String)); // TODO: why is this `TypeVariant::String` in this case but `EntityOrCommon { "String" }` in all the other cases in this test? Do we accept common types as the element type for sets? - }); - let doc = &frag.0.get(&None).unwrap().entity_types.get(&"Document".parse().unwrap()).unwrap(); + });}); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"Document".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(doc), ..} => { assert_matches!(&doc.tags, Some(Type::Type(TypeVariant::Set { element })) => { assert_matches!(&**element, Type::Type(TypeVariant::String)); // TODO: why is this `TypeVariant::String` in this case but `EntityOrCommon { "String" }` in all the other cases in this test? Do we accept common types as the element type for sets? }); - }) + })}) } /// In this schema, the tag type is a common type @@ -2757,11 +2793,11 @@ mod entity_tags { "actions": {} }}); assert_matches!(Fragment::from_json_value(json), Ok(frag) => { - let user = &frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap(), EntityType {kind: EntityTypeKind::Standard(user), ..} => { assert_matches!(&user.tags, Some(Type::CommonTypeRef { type_name }) => { assert_eq!(&format!("{type_name}"), "T"); }); - }) + })}); } /// In this schema, the tag type is an entity type @@ -2784,11 +2820,11 @@ mod entity_tags { "actions": {} }}); assert_matches!(Fragment::from_json_value(json), Ok(frag) => { - let user = &frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap(); + assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(user), ..} => { assert_matches!(&user.tags, Some(Type::Type(TypeVariant::Entity{ name })) => { assert_eq!(&format!("{name}"), "User"); }); - }) + })}); } /// This schema has `tags` inside `shape` instead of parallel to it @@ -2869,13 +2905,17 @@ mod test_json_roundtrip { common_types: BTreeMap::new(), entity_types: BTreeMap::from([( "a".parse().unwrap(), - CommonEntityType { - member_of_types: vec!["a".parse().unwrap()], - shape: AttributesOrContext(Type::Type(TypeVariant::Record(RecordType { - attributes: BTreeMap::new(), - additional_attributes: false, - }))), - tags: None, + EntityType { + kind: EntityTypeKind::Standard(StandardEntityType { + member_of_types: vec!["a".parse().unwrap()], + shape: AttributesOrContext(Type::Type(TypeVariant::Record( + RecordType { + attributes: BTreeMap::new(), + additional_attributes: false, + }, + ))), + tags: None, + }), annotations: Annotations::new(), loc: None, }, @@ -2914,15 +2954,17 @@ mod test_json_roundtrip { common_types: BTreeMap::new(), entity_types: BTreeMap::from([( "a".parse().unwrap(), - CommonEntityType { - member_of_types: vec!["a".parse().unwrap()], - shape: AttributesOrContext(Type::Type(TypeVariant::Record( - RecordType { - attributes: BTreeMap::new(), - additional_attributes: false, - }, - ))), - tags: None, + EntityType { + kind: EntityTypeKind::Standard(StandardEntityType { + member_of_types: vec!["a".parse().unwrap()], + shape: AttributesOrContext(Type::Type(TypeVariant::Record( + RecordType { + attributes: BTreeMap::new(), + additional_attributes: false, + }, + ))), + tags: None, + }), annotations: Annotations::new(), loc: None, }, diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index c9defa60a..3d291e5b4 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -289,23 +289,21 @@ mod test { [ ( foo_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ( bar_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ], [( diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index 1c4ce811c..c1633000b 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -482,13 +482,12 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( foo_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), )], [], ); @@ -519,13 +518,12 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( "foo_type".parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), )], [], ); @@ -609,13 +607,12 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( p_name.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), )], [], ); @@ -636,13 +633,12 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( p_name.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), )], [], ); @@ -663,13 +659,12 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( p_name.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), )], [], ); @@ -1031,13 +1026,12 @@ mod test { let schema_file = json_schema::NamespaceDefinition::new( [( foo_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), )], [], ); @@ -1067,23 +1061,21 @@ mod test { [ ( principal_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ( resource_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ], [( @@ -1464,43 +1456,39 @@ mod test { [ ( principal_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ( resource_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![resource_parent_type.parse().unwrap()], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ( resource_parent_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![resource_grandparent_type.parse().unwrap()], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ( resource_grandparent_type.parse().unwrap(), - json_schema::CommonEntityType { + json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }, + } + .into(), ), ], [ diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index c668b04d1..c2ef40d91 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -26,7 +26,9 @@ use cedar_policy_core::{ extensions::Extensions, transitive_closure::compute_tc, }; +use entity_type::{StandardValidatorEntityType, ValidatorEntityTypeKind}; use itertools::Itertools; +use namespace_def::EntityTypeFragment; use nonempty::NonEmpty; use serde::{Deserialize, Serialize}; use serde_with::serde_as; @@ -485,7 +487,7 @@ impl ValidatorSchema { // to get a `children` relation. let mut entity_children: HashMap> = HashMap::new(); for (name, entity_type) in entity_type_fragments.iter() { - for parent in entity_type.parents.iter() { + for parent in entity_type.parents() { entity_children .entry(internal_name_to_entity_type(parent.clone())?) .or_default() @@ -506,34 +508,53 @@ 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) = { - let unresolved = try_jsonschema_type_into_validator_type( - entity_type.attributes.0, - extensions, - )?; - Self::record_attributes_or_none( - unresolved.resolve_common_type_refs(&common_types)?, - ) - .ok_or_else(|| { - ContextOrShapeNotRecordError(ContextOrShape::EntityTypeShape(name.clone())) - })? - }; - let tags = entity_type - .tags - .map(|tags| try_jsonschema_type_into_validator_type(tags, extensions)) - .transpose()? - .map(|unresolved| unresolved.resolve_common_type_refs(&common_types)) - .transpose()?; - Ok(( - name.clone(), - ValidatorEntityType { - name, - descendants, + match entity_type { + EntityTypeFragment::Enum(choices) => Ok(( + name.clone(), + ValidatorEntityType { + name, + descendants, + kind: ValidatorEntityTypeKind::Enum(choices), + }, + )), + EntityTypeFragment::Standard { attributes, - open_attributes, + parents: _, tags, - }, - )) + } => { + let (attributes, open_attributes) = { + let unresolved = + try_jsonschema_type_into_validator_type(attributes.0, extensions)?; + Self::record_attributes_or_none( + unresolved.resolve_common_type_refs(&common_types)?, + ) + .ok_or_else(|| { + ContextOrShapeNotRecordError(ContextOrShape::EntityTypeShape( + name.clone(), + )) + })? + }; + let tags = tags + .map(|tags| try_jsonschema_type_into_validator_type(tags, extensions)) + .transpose()? + .map(|unresolved| unresolved.resolve_common_type_refs(&common_types)) + .transpose()?; + Ok(( + name.clone(), + ValidatorEntityType { + name, + descendants, + kind: ValidatorEntityTypeKind::Standard( + StandardValidatorEntityType { + attributes, + open_attributes, + tags, + }, + ), + }, + )) + } + } }) .collect::>>()?; @@ -2304,7 +2325,16 @@ pub(crate) mod test { .unwrap(); let schema: ValidatorSchema = fragment.try_into().unwrap(); assert_eq!( - schema.entity_types.iter().next().unwrap().1.attributes, + Attributes::with_required_attributes( + schema + .entity_types + .iter() + .next() + .unwrap() + .1 + .attributes() + .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) + ), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -2330,7 +2360,16 @@ pub(crate) mod test { .unwrap(); let schema: ValidatorSchema = fragment.try_into().unwrap(); assert_eq!( - schema.entity_types.iter().next().unwrap().1.attributes, + Attributes::with_required_attributes( + schema + .entity_types + .iter() + .next() + .unwrap() + .1 + .attributes() + .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) + ), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -2362,7 +2401,16 @@ pub(crate) mod test { .unwrap(); let schema: ValidatorSchema = fragment.try_into().unwrap(); assert_eq!( - schema.entity_types.iter().next().unwrap().1.attributes, + Attributes::with_required_attributes( + schema + .entity_types + .iter() + .next() + .unwrap() + .1 + .attributes() + .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) + ), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -2408,7 +2456,16 @@ pub(crate) mod test { .unwrap(); assert_eq!( - schema.entity_types.iter().next().unwrap().1.attributes, + Attributes::with_required_attributes( + schema + .entity_types + .iter() + .next() + .unwrap() + .1 + .attributes() + .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) + ), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -4155,29 +4212,29 @@ mod test_rfc70 { "; let schema = assert_valid_cedar_schema(src); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); // using the common type definition }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_string()); }); - assert_matches!(e.attributes.get_attr("c"), Some(atype) => { + assert_matches!(e.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(e.attributes.get_attr("d"), Some(atype) => { + assert_matches!(e.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); // using the common type definition }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_boolean()); }); - assert_matches!(f.attributes.get_attr("c"), Some(atype) => { + assert_matches!(f.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(f.attributes.get_attr("d"), Some(atype) => { + assert_matches!(f.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); @@ -4264,29 +4321,29 @@ mod test_rfc70 { }); let schema = assert_valid_json_schema(src_json); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_string()); }); - assert_matches!(e.attributes.get_attr("c"), Some(atype) => { + assert_matches!(e.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(e.attributes.get_attr("d"), Some(atype) => { + assert_matches!(e.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); // using the common type definition }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_boolean()); }); - assert_matches!(f.attributes.get_attr("c"), Some(atype) => { + assert_matches!(f.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(f.attributes.get_attr("d"), Some(atype) => { + assert_matches!(f.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); } @@ -4315,29 +4372,29 @@ mod test_rfc70 { "; let schema = assert_valid_cedar_schema(src); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); // using the common type definition }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("ipaddr".parse().unwrap())); }); - assert_matches!(e.attributes.get_attr("c"), Some(atype) => { + assert_matches!(e.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(e.attributes.get_attr("d"), Some(atype) => { + assert_matches!(e.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); // using the common type definition }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("decimal".parse().unwrap())); }); - assert_matches!(f.attributes.get_attr("c"), Some(atype) => { + assert_matches!(f.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(f.attributes.get_attr("d"), Some(atype) => { + assert_matches!(f.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); @@ -4383,29 +4440,29 @@ mod test_rfc70 { }); let schema = assert_valid_json_schema(src_json); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); // using the common type definition }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("ipaddr".parse().unwrap())); }); - assert_matches!(e.attributes.get_attr("c"), Some(atype) => { + assert_matches!(e.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(e.attributes.get_attr("d"), Some(atype) => { + assert_matches!(e.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); // using the common type definition }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("decimal".parse().unwrap())); }); - assert_matches!(f.attributes.get_attr("c"), Some(atype) => { + assert_matches!(f.attr("c"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); - assert_matches!(f.attributes.get_attr("d"), Some(atype) => { + assert_matches!(f.attr("d"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_long()); }); } @@ -4430,17 +4487,17 @@ mod test_rfc70 { "; let schema = assert_valid_cedar_schema(src); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("String")); }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_string()); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("NS::Bool")); // using the common type definition }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_boolean()); }); @@ -4478,17 +4535,17 @@ mod test_rfc70 { }); let schema = assert_valid_json_schema(src_json); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("String")); }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_string()); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("NS::Bool")); }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::primitive_boolean()); }); } @@ -4513,17 +4570,17 @@ mod test_rfc70 { "; let schema = assert_valid_cedar_schema(src); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("ipaddr")); }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("ipaddr".parse().unwrap())); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("NS::decimal")); }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("decimal".parse().unwrap())); }); @@ -4561,17 +4618,17 @@ mod test_rfc70 { }); let schema = assert_valid_json_schema(src_json); let e = assert_entity_type_exists(&schema, "E"); - assert_matches!(e.attributes.get_attr("a"), Some(atype) => { + assert_matches!(e.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("ipaddr")); }); - assert_matches!(e.attributes.get_attr("b"), Some(atype) => { + assert_matches!(e.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("ipaddr".parse().unwrap())); }); let f = assert_entity_type_exists(&schema, "NS::F"); - assert_matches!(f.attributes.get_attr("a"), Some(atype) => { + assert_matches!(f.attr("a"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::named_entity_reference_from_str("NS::decimal")); }); - assert_matches!(f.attributes.get_attr("b"), Some(atype) => { + assert_matches!(f.attr("b"), Some(atype) => { assert_eq!(&atype.attr_type, &Type::extension("decimal".parse().unwrap())); }); } diff --git a/cedar-policy-validator/src/schema/entity_type.rs b/cedar-policy-validator/src/schema/entity_type.rs index a3a092782..9ad883e9b 100644 --- a/cedar-policy-validator/src/schema/entity_type.rs +++ b/cedar-policy-validator/src/schema/entity_type.rs @@ -34,52 +34,89 @@ use cedar_policy_core::ast; /// the struct are the same as the schema entity type structure, but the /// `member_of` relation is reversed to instead be `descendants`. #[derive(Clone, Debug, Serialize)] -pub enum ValidatorEntityType { - Common { - /// The name of the entity type. - name: EntityType, - - /// The set of entity types that can be members of this entity type. When - /// this structure is initially constructed, the field will contain direct - /// children, but it will be updated to contain the closure of all - /// descendants before it is used in any validation. - descendants: HashSet, - - /// The attributes associated with this entity. - 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. - open_attributes: OpenTag, - - /// Tag type for this entity type. `None` indicates that entities of this - /// type are not allowed to have tags. - #[serde(skip_serializing_if = "Option::is_none")] - tags: Option, - }, +pub struct ValidatorEntityType { + /// The name of the entity type. + pub(crate) name: EntityType, + + /// The set of entity types that can be members of this entity type. When + /// this structure is initially constructed, the field will contain direct + /// children, but it will be updated to contain the closure of all + /// descendants before it is used in any validation. + pub(crate) descendants: HashSet, + pub(crate) kind: ValidatorEntityTypeKind, +} + +#[derive(Clone, Debug, Serialize)] +pub struct StandardValidatorEntityType { + /// The attributes associated with this entity. + 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, + + /// Tag type for this entity type. `None` indicates that entities of this + /// type are not allowed to have tags. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) tags: Option, +} + +#[derive(Clone, Debug, Serialize)] +pub enum ValidatorEntityTypeKind { + Standard(StandardValidatorEntityType), Enum(Vec), } impl ValidatorEntityType { + /// Return `true` if this entity type has an [`EntityType`] declared as a + /// possible descendant in the schema. + pub fn has_descendant_entity_type(&self, ety: &EntityType) -> bool { + self.descendants.contains(ety) + } + + /// An iterator over the attributes of this entity + pub fn attributes(&self) -> Box + '_> { + match &self.kind { + ValidatorEntityTypeKind::Enum(_) => Box::new(std::iter::empty()), + ValidatorEntityTypeKind::Standard(ty) => Box::new(ty.attributes()), + } + } + /// Get the type of the attribute with the given name, if it exists pub fn attr(&self, attr: &str) -> Option<&AttributeType> { - self.attributes.get_attr(attr) + match &self.kind { + ValidatorEntityTypeKind::Enum(_) => None, + ValidatorEntityTypeKind::Standard(ty) => ty.attributes.get_attr(attr), + } + } + + /// ... + pub fn open_attributes(&self) -> OpenTag { + match &self.kind { + ValidatorEntityTypeKind::Enum(_) => OpenTag::ClosedAttributes, + ValidatorEntityTypeKind::Standard(ty) => ty.open_attributes, + } + } + + /// Get the type of tags on this entity. `None` indicates that entities of + /// this type are not allowed to have tags. + pub fn tag_type(&self) -> Option<&Type> { + match &self.kind { + ValidatorEntityTypeKind::Enum(_) => None, + ValidatorEntityTypeKind::Standard(ty) => ty.tag_type(), + } } +} +impl StandardValidatorEntityType { /// An iterator over the attributes of this entity pub fn attributes(&self) -> impl Iterator { self.attributes.iter() } - /// Return `true` if this entity type has an [`EntityType`] declared as a - /// possible descendant in the schema. - pub fn has_descendant_entity_type(&self, ety: &EntityType) -> bool { - self.descendants.contains(ety) - } - /// Get the type of tags on this entity. `None` indicates that entities of /// this type are not allowed to have tags. pub fn tag_type(&self) -> Option<&Type> { @@ -108,7 +145,7 @@ impl TCNode for ValidatorEntityType { #[cfg(feature = "protobufs")] impl From<&ValidatorEntityType> for proto::ValidatorEntityType { fn from(v: &ValidatorEntityType) -> Self { - let tags = v.tags.as_ref().map(|tags| proto::Tag { + let tags = v.tag_type().map(|tags| proto::Tag { optional_type: Some(proto::Type::from(tags)), }); Self { @@ -118,8 +155,13 @@ impl From<&ValidatorEntityType> for proto::ValidatorEntityType { .iter() .map(ast::proto::EntityType::from) .collect(), - attributes: Some(proto::Attributes::from(&v.attributes)), - open_attributes: proto::OpenTag::from(&v.open_attributes).into(), + attributes: Some(proto::Attributes::from( + &Attributes::with_required_attributes( + v.attributes() + .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())), + ), + )), + open_attributes: proto::OpenTag::from(&v.open_attributes()).into(), tags, } } @@ -141,15 +183,17 @@ impl From<&proto::ValidatorEntityType> for ValidatorEntityType { .expect("`as_ref()` for field that should exist"), ), descendants: v.descendants.iter().map(ast::EntityType::from).collect(), - attributes: Attributes::from( - v.attributes - .as_ref() - .expect("`as_ref()` for field that should exist"), - ), - open_attributes: OpenTag::from( - &proto::OpenTag::try_from(v.open_attributes).expect("decode should succeed"), - ), - tags, + kind: ValidatorEntityTypeKind::Standard(StandardValidatorEntityType { + attributes: Attributes::from( + v.attributes + .as_ref() + .expect("`as_ref()` for field that should exist"), + ), + open_attributes: OpenTag::from( + &proto::OpenTag::try_from(v.open_attributes).expect("decode should succeed"), + ), + tags, + }), } } } diff --git a/cedar-policy-validator/src/schema/namespace_def.rs b/cedar-policy-validator/src/schema/namespace_def.rs index e7fa98d28..19cb4eb7a 100644 --- a/cedar-policy-validator/src/schema/namespace_def.rs +++ b/cedar-policy-validator/src/schema/namespace_def.rs @@ -36,7 +36,7 @@ use smol_str::{SmolStr, ToSmolStr}; use super::{internal_name_to_entity_type, AllDefs, ValidatorApplySpec}; use crate::{ err::{schema_errors::*, SchemaError}, - json_schema::{self, CommonTypeId}, + json_schema::{self, CommonTypeId, EntityTypeKind}, types::{AttributeType, Attributes, OpenTag, Type}, ActionBehavior, ConditionalName, RawName, ReferenceType, }; @@ -447,7 +447,7 @@ impl EntityTypesDef { /// qualified yet, depending on `N`. #[derive(Debug, Clone)] pub enum EntityTypeFragment { - Common { + Standard { /// Description of the attribute types for this entity type. /// /// This may contain references to common types which have not yet been @@ -474,6 +474,15 @@ pub enum EntityTypeFragment { Enum(Vec), } +impl EntityTypeFragment { + pub(crate) fn parents(&self) -> Box + '_> { + match self { + Self::Standard { parents, .. } => Box::new(parents.iter()), + Self::Enum(_) => Box::new(std::iter::empty()), + } + } +} + impl EntityTypeFragment { /// Construct a [`EntityTypeFragment`] by converting the /// structures used by the schema format to those used internally by the @@ -482,21 +491,24 @@ impl EntityTypeFragment { schema_file_type: json_schema::EntityType, schema_namespace: Option<&InternalName>, ) -> Self { - Self { - attributes: schema_file_type - .shape - .conditionally_qualify_type_references(schema_namespace), - parents: schema_file_type - .member_of_types - .into_iter() - .map(|raw_name| { - // Only entity, not common, here for now; see #1064 - raw_name.conditionally_qualify_with(schema_namespace, ReferenceType::Entity) - }) - .collect(), - tags: schema_file_type - .tags - .map(|tags| tags.conditionally_qualify_type_references(schema_namespace)), + match schema_file_type.kind { + EntityTypeKind::Enum { choices } => Self::Enum(choices), + EntityTypeKind::Standard(ty) => Self::Standard { + attributes: ty + .shape + .conditionally_qualify_type_references(schema_namespace), + parents: ty + .member_of_types + .into_iter() + .map(|raw_name| { + // Only entity, not common, here for now; see #1064 + raw_name.conditionally_qualify_with(schema_namespace, ReferenceType::Entity) + }) + .collect(), + tags: ty + .tags + .map(|tags| tags.conditionally_qualify_type_references(schema_namespace)), + }, } } @@ -510,47 +522,59 @@ impl EntityTypeFragment { self, all_defs: &AllDefs, ) -> Result, TypeNotDefinedError> { - // Fully qualify typenames appearing in `attributes` - let fully_qual_attributes = self.attributes.fully_qualify_type_references(all_defs); - // Fully qualify typenames appearing in `parents` - let parents: HashSet = self - .parents - .into_iter() - .map(|parent| parent.resolve(all_defs)) - .collect::>()?; - // Fully qualify typenames appearing in `tags` - let fully_qual_tags = self - .tags - .map(|tags| tags.fully_qualify_type_references(all_defs)) - .transpose(); - // Now is the time to check whether any parents are dangling, i.e., - // refer to entity types that are not declared in any fragment (since we - // now have the set of typenames that are declared in all fragments). - let undeclared_parents: Option> = NonEmpty::collect( - parents - .iter() - .filter(|ety| !all_defs.is_defined_as_entity(ety)) - .map(|ety| ConditionalName::unconditional(ety.clone(), ReferenceType::Entity)), - ); - match (fully_qual_attributes, fully_qual_tags, undeclared_parents) { - (Ok(attributes), Ok(tags), None) => Ok(EntityTypeFragment { + match self { + Self::Enum(choices) => Ok(EntityTypeFragment::Enum(choices)), + Self::Standard { attributes, parents, tags, - }), - (Ok(_), Ok(_), Some(undeclared_parents)) => { - Err(TypeNotDefinedError(undeclared_parents)) - } - (Err(e), Ok(_), None) | (Ok(_), Err(e), None) => Err(e), - (Err(e1), Err(e2), None) => Err(TypeNotDefinedError::join_nonempty(nonempty![e1, e2])), - (Err(e), Ok(_), Some(mut undeclared)) | (Ok(_), Err(e), Some(mut undeclared)) => { - undeclared.extend(e.0); - Err(TypeNotDefinedError(undeclared)) - } - (Err(e1), Err(e2), Some(mut undeclared)) => { - undeclared.extend(e1.0); - undeclared.extend(e2.0); - Err(TypeNotDefinedError(undeclared)) + } => { + // Fully qualify typenames appearing in `attributes` + let fully_qual_attributes = attributes.fully_qualify_type_references(all_defs); + // Fully qualify typenames appearing in `parents` + let parents: HashSet = parents + .into_iter() + .map(|parent| parent.resolve(all_defs)) + .collect::>()?; + // Fully qualify typenames appearing in `tags` + let fully_qual_tags = tags + .map(|tags| tags.fully_qualify_type_references(all_defs)) + .transpose(); + // Now is the time to check whether any parents are dangling, i.e., + // refer to entity types that are not declared in any fragment (since we + // now have the set of typenames that are declared in all fragments). + let undeclared_parents: Option> = NonEmpty::collect( + parents + .iter() + .filter(|ety| !all_defs.is_defined_as_entity(ety)) + .map(|ety| { + ConditionalName::unconditional(ety.clone(), ReferenceType::Entity) + }), + ); + match (fully_qual_attributes, fully_qual_tags, undeclared_parents) { + (Ok(attributes), Ok(tags), None) => Ok(EntityTypeFragment::Standard { + attributes, + parents, + tags, + }), + (Ok(_), Ok(_), Some(undeclared_parents)) => { + Err(TypeNotDefinedError(undeclared_parents)) + } + (Err(e), Ok(_), None) | (Ok(_), Err(e), None) => Err(e), + (Err(e1), Err(e2), None) => { + Err(TypeNotDefinedError::join_nonempty(nonempty![e1, e2])) + } + (Err(e), Ok(_), Some(mut undeclared)) + | (Ok(_), Err(e), Some(mut undeclared)) => { + undeclared.extend(e.0); + Err(TypeNotDefinedError(undeclared)) + } + (Err(e1), Err(e2), Some(mut undeclared)) => { + undeclared.extend(e1.0); + undeclared.extend(e2.0); + Err(TypeNotDefinedError(undeclared)) + } + } } } } diff --git a/cedar-policy-validator/src/typecheck/test/expr.rs b/cedar-policy-validator/src/typecheck/test/expr.rs index 118ccc47c..f4c87bdf2 100644 --- a/cedar-policy-validator/src/typecheck/test/expr.rs +++ b/cedar-policy-validator/src/typecheck/test/expr.rs @@ -22,7 +22,6 @@ use std::{str::FromStr, vec}; use cedar_policy_core::{ ast::{BinaryOp, EntityUID, Expr, Name, Pattern, PatternElem, SlotId, Value, Var}, - est::Annotations, extensions::Extensions, }; use itertools::Itertools; @@ -64,13 +63,12 @@ fn slot_typechecks() { #[test] fn slot_in_typechecks() { - let etype = json_schema::CommonEntityType { + let etype = json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }; + } + .into(); let schema = json_schema::NamespaceDefinition::new([("typename".parse().unwrap(), etype)], []); assert_typechecks_for_mode( schema.clone(), @@ -96,13 +94,12 @@ fn slot_in_typechecks() { #[test] fn slot_equals_typechecks() { - let etype = json_schema::CommonEntityType { + let etype = json_schema::StandardEntityType { member_of_types: vec![], shape: json_schema::AttributesOrContext::default(), tags: None, - annotations: Annotations::new(), - loc: None, - }; + } + .into(); // These don't typecheck in strict mode because the test_util expression // typechecker doesn't have access to a schema, so it can't link // the template slots with appropriate types. Similar policies that pass diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index d8e22c4a9..267e525a1 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -958,7 +958,13 @@ impl EntityLUB { let mut lub_element_attributes = self.lub_elements.iter().map(|name| { schema .get_entity_type(name) - .map(|entity_type| entity_type.attributes.clone()) + .map(|entity_type| { + Attributes::with_attributes( + entity_type + .attributes() + .map(|(attr, ty)| (attr.clone(), ty.clone())), + ) + }) .unwrap_or_else(|| Attributes::with_attributes(None)) }); @@ -1304,7 +1310,7 @@ impl EntityRecordKind { EntityRecordKind::Entity(lub) => lub.iter().any(|e_name| { schema .get_entity_type(e_name) - .map(|e_type| e_type.open_attributes) + .map(|e_type| e_type.open_attributes()) // The entity type was not found in the schema, so we know // nothing about it and must assume that it may have // additional attributes. From a7d98ba35f23b5cc3eedecffdba7bdbcfd2332d6 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 6 Jan 2025 13:08:55 -0800 Subject: [PATCH 03/28] updates Signed-off-by: Shaobo He --- cedar-policy-validator/src/json_schema.rs | 2 +- .../src/typecheck/test/policy.rs | 86 +++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index 6b8662020..14924362d 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -440,7 +440,7 @@ pub enum EntityTypeKind { Standard(StandardEntityType), /// ... Enum { - #[serde(rename = "Enum")] + #[serde(rename = "enum")] /// ... choices: Vec, }, diff --git a/cedar-policy-validator/src/typecheck/test/policy.rs b/cedar-policy-validator/src/typecheck/test/policy.rs index 92b7510ad..b6ef8140a 100644 --- a/cedar-policy-validator/src/typecheck/test/policy.rs +++ b/cedar-policy-validator/src/typecheck/test/policy.rs @@ -1312,3 +1312,89 @@ mod templates { ); } } + +mod enumerated_entity_types { + use cedar_policy_core::{ast::PolicyID, parser::parse_policy_or_template}; + + use crate::{ + json_schema, typecheck::test::test_utils::get_loc, types::EntityLUB, + validation_errors::AttributeAccess, RawName, ValidationError, + }; + + use super::{ + assert_exactly_one_diagnostic, assert_policy_typecheck_fails, assert_policy_typechecks, + }; + + #[track_caller] + fn schema() -> json_schema::NamespaceDefinition { + serde_json::from_value(serde_json::json!( + { + "entityTypes": { + "Foo": { + "enum": [ "foo" ], + }, + "Bar": { + "memberOfTypes": ["Foo"], + } + }, + "actions": { + "a": { + "appliesTo": { + "principalTypes": ["Foo"], + "resourceTypes": ["Bar"], + } + } + } + } + )) + .unwrap() + } + + #[test] + fn basic() { + let schema = schema(); + let template = parse_policy_or_template(None, r#"permit(principal, action == Action::"a", resource) when { principal == Foo::"foo" };"#).unwrap(); + assert_policy_typechecks(schema, template); + } + + #[test] + fn no_attrs_allowed() { + let schema = schema(); + let src = r#"permit(principal, action == Action::"a", resource) when { principal.foo == "foo" };"#; + let template = parse_policy_or_template(None, src).unwrap(); + let errs = assert_policy_typecheck_fails(schema, template); + let err = assert_exactly_one_diagnostic(errs); + assert_eq!( + err, + ValidationError::unsafe_attribute_access( + get_loc(src, "principal.foo"), + PolicyID::from_string("policy0"), + AttributeAccess::EntityLUB( + EntityLUB::single_entity("Foo".parse().unwrap()), + vec!["foo".into()], + ), + None, + false, + ) + ); + } + + #[test] + fn no_ancestors() { + let schema = schema(); + let src = + r#"permit(principal, action == Action::"a", resource) when { principal in resource };"#; + let template = parse_policy_or_template(None, src).unwrap(); + let errs = assert_policy_typecheck_fails(schema, template); + let err = assert_exactly_one_diagnostic(errs); + assert_eq!( + err, + ValidationError::hierarchy_not_respected( + get_loc(src, "principal in resource"), + PolicyID::from_string("policy0"), + Some("Foo".parse().unwrap()), + Some("Bar".parse().unwrap()), + ) + ); + } +} From 6b7d9b948736d3f08bc6965151bfe835b5c1bbb8 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 6 Jan 2025 15:12:04 -0800 Subject: [PATCH 04/28] add validation Signed-off-by: Shaobo He --- cedar-policy-validator/src/diagnostics.rs | 23 +++++++- .../src/diagnostics/validation_errors.rs | 23 ++++++++ cedar-policy-validator/src/lib.rs | 1 + cedar-policy-validator/src/rbac.rs | 53 ++++++++++++++++++- cedar-policy-validator/src/schema.rs | 4 +- .../src/schema/entity_type.rs | 5 ++ .../src/typecheck/test/policy.rs | 8 +++ cedar-policy/src/api/err.rs | 8 +++ cedar-policy/src/api/err/validation_errors.rs | 1 + 9 files changed, 121 insertions(+), 5 deletions(-) diff --git a/cedar-policy-validator/src/diagnostics.rs b/cedar-policy-validator/src/diagnostics.rs index 7d11f3ebe..dd9a59c54 100644 --- a/cedar-policy-validator/src/diagnostics.rs +++ b/cedar-policy-validator/src/diagnostics.rs @@ -18,12 +18,13 @@ //! returned by the validator. use miette::Diagnostic; +use smol_str::SmolStr; use thiserror::Error; use validation_errors::UnrecognizedActionIdHelp; use std::collections::BTreeSet; -use cedar_policy_core::ast::{EntityType, Expr, PolicyID}; +use cedar_policy_core::ast::{EntityType, EntityUID, Expr, PolicyID}; use cedar_policy_core::parser::Loc; use crate::types::{EntityLUB, Type}; @@ -166,6 +167,11 @@ pub enum ValidationError { #[error(transparent)] #[diagnostic(transparent)] InternalInvariantViolation(#[from] validation_errors::InternalInvariantViolation), + /// Returned when an entity literal is of an enumerated entity type but has + /// undeclared UID + #[error(transparent)] + #[diagnostic(transparent)] + InvalidEnumEntity(#[from] validation_errors::InvalidEnumEntity), #[cfg(feature = "level-validate")] /// If a entity dereference level was provided, the policies cannot deref /// more than `level` hops away from PARX @@ -398,6 +404,21 @@ impl ValidationError { } .into() } + + pub(crate) fn invalid_enum_entity( + source_loc: Option, + policy_id: PolicyID, + entity: EntityUID, + choices: impl IntoIterator, + ) -> Self { + validation_errors::InvalidEnumEntity { + source_loc, + policy_id, + entity, + choices: choices.into_iter().collect(), + } + .into() + } } /// Represents the different kinds of validation warnings and information diff --git a/cedar-policy-validator/src/diagnostics/validation_errors.rs b/cedar-policy-validator/src/diagnostics/validation_errors.rs index e696c9ae6..fdca37c7a 100644 --- a/cedar-policy-validator/src/diagnostics/validation_errors.rs +++ b/cedar-policy-validator/src/diagnostics/validation_errors.rs @@ -717,6 +717,29 @@ impl Display for AttributeAccess { } } +/// Returned when an entity literal is of an enumerated entity type but has +/// undeclared UID +#[derive(Debug, Clone, Error, Hash, Eq, PartialEq)] +#[error("for policy `{policy_id}`, entity `{entity}`'s UID is illegal")] +pub struct InvalidEnumEntity { + /// Source location + pub source_loc: Option, + /// Policy ID where the error occurred + pub policy_id: PolicyID, + /// UID of the problematic entity + pub entity: EntityUID, + /// UID choices of the enumerated entity type + pub choices: Vec, +} + +impl Diagnostic for InvalidEnumEntity { + impl_diagnostic_from_source_loc_opt_field!(source_loc); + + fn help<'a>(&'a self) -> Option> { + Some(Box::new(format!("allowed UID values: {:?}", self.choices))) + } +} + // These tests all assume that the typechecker found an error while checking the // outermost `GetAttr` in the expressions. If the attribute didn't exist at all, // only the primary message would included in the final error. If it was an diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index 3d291e5b4..b92cd0afb 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -202,6 +202,7 @@ impl Validator { } else { Some( self.validate_entity_types(p) + .chain(self.validate_enum_entity(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 diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index c1633000b..1c4d0d1d9 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -18,8 +18,8 @@ use cedar_policy_core::{ ast::{ - self, ActionConstraint, EntityReference, EntityUID, Policy, PolicyID, PrincipalConstraint, - PrincipalOrResourceConstraint, ResourceConstraint, SlotEnv, Template, + self, ActionConstraint, EntityReference, EntityUID, ExprKind, Literal, Policy, PolicyID, + PrincipalConstraint, PrincipalOrResourceConstraint, ResourceConstraint, SlotEnv, Template, }, fuzzy_match::fuzzy_search, parser::Loc, @@ -36,6 +36,55 @@ use crate::{ use super::{schema::*, Validator}; impl Validator { + /// Validate if a [`Template`] contains entities of enumerated entity types + /// but with invalid UIDs + pub(crate) fn validate_enum_entity<'a>( + &'a self, + template: &'a Template, + ) -> impl Iterator + 'a { + let non_action_entities = template + .principal_constraint() + .as_inner() + .get_euid() + .into_iter() + .map(|e| e.as_ref()) + .chain( + template + .resource_constraint() + .as_inner() + .get_euid() + .into_iter() + .map(|e| e.as_ref()), + ) + .chain( + template + .non_scope_constraints() + .subexpressions() + .filter_map(|e| match e.expr_kind() { + ExprKind::Lit(Literal::EntityUID(euid)) if !euid.is_action() => { + Some(euid.as_ref()) + } + _ => None, + }), + ); + non_action_entities.filter_map(|e: &EntityUID| { + if let Some(ValidatorEntityType { + kind: ValidatorEntityTypeKind::Enum(choices), + .. + }) = self.schema.get_entity_type(e.entity_type()) + { + if !choices.contains(e.eid().as_ref()) { + return Some(ValidationError::invalid_enum_entity( + e.loc().cloned(), + template.id().clone(), + e.clone(), + choices.into_iter().cloned(), + )); + } + } + None + }) + } /// Generate `UnrecognizedEntityType` error for every entity type in the /// expression that could not also be found in the schema. pub(crate) fn validate_entity_types<'a>( diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 801636bcd..5cf549074 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -27,7 +27,7 @@ use cedar_policy_core::{ parser::Loc, transitive_closure::compute_tc, }; -use entity_type::{StandardValidatorEntityType, ValidatorEntityTypeKind}; +use entity_type::StandardValidatorEntityType; use itertools::Itertools; use namespace_def::EntityTypeFragment; use nonempty::NonEmpty; @@ -54,7 +54,7 @@ mod action; pub use action::ValidatorActionId; pub(crate) use action::ValidatorApplySpec; mod entity_type; -pub use entity_type::ValidatorEntityType; +pub use entity_type::{ValidatorEntityType, ValidatorEntityTypeKind}; mod namespace_def; pub(crate) use namespace_def::try_jsonschema_type_into_validator_type; pub use namespace_def::ValidatorNamespaceDef; diff --git a/cedar-policy-validator/src/schema/entity_type.rs b/cedar-policy-validator/src/schema/entity_type.rs index 9ad883e9b..8f90f4f86 100644 --- a/cedar-policy-validator/src/schema/entity_type.rs +++ b/cedar-policy-validator/src/schema/entity_type.rs @@ -64,9 +64,14 @@ pub struct StandardValidatorEntityType { pub(crate) tags: Option, } +/// The kind of validator entity types +/// It can either be a standard (non-enum) entity type, or +/// an enumerated entity type #[derive(Clone, Debug, Serialize)] pub enum ValidatorEntityTypeKind { + /// Standard, aka non-enum Standard(StandardValidatorEntityType), + /// Enumerated Enum(Vec), } diff --git a/cedar-policy-validator/src/typecheck/test/policy.rs b/cedar-policy-validator/src/typecheck/test/policy.rs index b6ef8140a..b40fa6e70 100644 --- a/cedar-policy-validator/src/typecheck/test/policy.rs +++ b/cedar-policy-validator/src/typecheck/test/policy.rs @@ -1357,6 +1357,14 @@ mod enumerated_entity_types { assert_policy_typechecks(schema, template); } + //TODO: should type checker reject it as well? + #[test] + fn basic_invalid() { + let schema = schema(); + let template = parse_policy_or_template(None, r#"permit(principal, action == Action::"a", resource) when { principal == Foo::"fo" };"#).unwrap(); + assert_policy_typechecks(schema, template); + } + #[test] fn no_attrs_allowed() { let schema = schema(); diff --git a/cedar-policy/src/api/err.rs b/cedar-policy/src/api/err.rs index c02893e0c..7a751052d 100644 --- a/cedar-policy/src/api/err.rs +++ b/cedar-policy/src/api/err.rs @@ -434,6 +434,10 @@ pub enum ValidationError { #[error(transparent)] #[diagnostic(transparent)] EntityDerefLevelViolation(#[from] validation_errors::EntityDerefLevelViolation), + /// Returned when ... + #[error(transparent)] + #[diagnostic(transparent)] + InvalidEnumEntity(#[from] validation_errors::InvalidEnumEntity), } impl ValidationError { @@ -457,6 +461,7 @@ impl ValidationError { Self::HierarchyNotRespected(e) => e.policy_id(), Self::InternalInvariantViolation(e) => e.policy_id(), Self::EntityDerefLevelViolation(e) => e.policy_id(), + Self::InvalidEnumEntity(e) => e.policy_id(), } } } @@ -513,6 +518,9 @@ impl From for ValidationError { cedar_policy_validator::ValidationError::InternalInvariantViolation(e) => { Self::InternalInvariantViolation(e.into()) } + cedar_policy_validator::ValidationError::InvalidEnumEntity(e) => { + Self::InvalidEnumEntity(e.into()) + } #[cfg(feature = "level-validate")] cedar_policy_validator::ValidationError::EntityDerefLevelViolation(e) => { Self::EntityDerefLevelViolation(e.into()) diff --git a/cedar-policy/src/api/err/validation_errors.rs b/cedar-policy/src/api/err/validation_errors.rs index ca14df220..0faec02af 100644 --- a/cedar-policy/src/api/err/validation_errors.rs +++ b/cedar-policy/src/api/err/validation_errors.rs @@ -73,3 +73,4 @@ wrap_core_error!(EntityDerefLevelViolation); wrap_core_error!(EmptySetForbidden); wrap_core_error!(NonLitExtConstructor); wrap_core_error!(InternalInvariantViolation); +wrap_core_error!(InvalidEnumEntity); From 8af90e532253f3bf15d02bb7dfaae439ff220f9a Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 6 Jan 2025 16:12:31 -0800 Subject: [PATCH 05/28] cedar schema format Signed-off-by: Shaobo He --- .../src/cedar_schema/ast.rs | 24 ++++++++++- .../src/cedar_schema/grammar.lalrpop | 19 ++++++++- .../src/cedar_schema/to_json_schema.rs | 40 +++++++++++-------- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/cedar-policy-validator/src/cedar_schema/ast.rs b/cedar-policy-validator/src/cedar_schema/ast.rs index e93ddce1e..0fe24783b 100644 --- a/cedar-policy-validator/src/cedar_schema/ast.rs +++ b/cedar-policy-validator/src/cedar_schema/ast.rs @@ -249,9 +249,24 @@ impl Decl for TypeDecl { } } +#[derive(Debug, Clone)] +pub enum EntityDecl { + Standard(StandardEntityDecl), + Enum(EnumEntityDecl), +} + +impl EntityDecl { + pub fn names(&self) -> impl Iterator> + '_ { + match self { + Self::Enum(d) => d.names.iter().cloned(), + Self::Standard(d) => d.names.iter().cloned(), + } + } +} + /// Declaration of an entity type #[derive(Debug, Clone)] -pub struct EntityDecl { +pub struct StandardEntityDecl { /// Entity Type Names bound by this declaration. /// More than one name can be bound if they have the same definition, for convenience pub names: Vec>, @@ -263,6 +278,13 @@ pub struct EntityDecl { pub tags: Option>, } +/// Declaration of an entity type +#[derive(Debug, Clone)] +pub struct EnumEntityDecl { + pub names: Vec>, + pub choices: NonEmpty>, +} + /// Type definitions #[derive(Debug, Clone)] pub enum Type { diff --git a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop index 864836f5f..4f25d3af1 100644 --- a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop +++ b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop @@ -24,6 +24,8 @@ use smol_str::ToSmolStr; use crate::cedar_schema::ast::{ Path, EntityDecl, + StandardEntityDecl, + EnumEntityDecl, Declaration, Namespace, Schema as ASchema, @@ -76,6 +78,7 @@ match { "Long" => LONG, "String" => STRING, "Bool" => BOOL, + "enum" => ENUM, // data input r"[_a-zA-Z][_a-zA-Z0-9]*" => IDENTIFIER, @@ -128,12 +131,16 @@ Decl: Node = { // Entity := 'entity' Idents ['in' EntOrTypes] [['='] RecType] ';' Entity: Node = { ENTITY )?> "}")?> )?> ";" - => Node::with_source_loc(Declaration::Entity(EntityDecl { + => Node::with_source_loc(Declaration::Entity(EntityDecl::Standard(StandardEntityDecl { names: ets, member_of_types: ps.unwrap_or_default(), attrs: Node::with_source_loc(ds.map(|ds| ds.unwrap_or_default()).unwrap_or_default(), Loc::new(l2..r2, Arc::clone(src))), tags: ts, - }), Loc::new(l1..r1, Arc::clone(src))), + })), Loc::new(l1..r1, Arc::clone(src))), + ENTITY ENUM "[" "]" => Node::with_source_loc(Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { + names: ets, + choices, + })), Loc::new(l..r, Arc::clone(src))), } // Action := 'action' Names ['in' QualNameOrNames] @@ -262,6 +269,8 @@ AnyIdent: Node = { => Node::with_source_loc("type".parse().unwrap(), Loc::new(l..r, Arc::clone(src))), IN => Node::with_source_loc("in".parse().unwrap(), Loc::new(l..r, Arc::clone(src))), + ENUM + => Node::with_source_loc("enum".parse().unwrap(), Loc::new(l..r, Arc::clone(src))), => Node::with_source_loc(i.parse().unwrap(), Loc::new(l..r, Arc::clone(src))), } @@ -303,6 +312,12 @@ Idents: Vec> = { }, } +// STRs := STR {',' STR} +STRs: NonEmpty> = { + => NonEmpty::singleton(i), + ",")+> => NonEmpty::collect(is.into_iter().chain(std::iter::once(i))).unwrap(), +} + // Names := Name {',' Name} Names: NonEmpty> = { => NonEmpty::new(n), diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index 2eb343ff0..e30e80778 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -362,27 +362,33 @@ fn convert_entity_decl( impl Iterator)>, ToJsonSchemaErrors, > { - // First build up the defined entity type + let names: Vec> = e.data.node.clone().names().collect(); let etype = json_schema::EntityType { - kind: json_schema::EntityTypeKind::Standard(json_schema::StandardEntityType { - member_of_types: e - .data - .node - .member_of_types - .into_iter() - .map(RawName::from) - .collect(), - shape: convert_attr_decls(e.data.node.attrs), - tags: e.data.node.tags.map(cedar_type_to_json_type), - }), + kind: match e.data.node { + EntityDecl::Enum(d) => json_schema::EntityTypeKind::Enum { + choices: d.choices.into_iter().map(|n| n.node).collect(), + }, + EntityDecl::Standard(d) => { + // First build up the defined entity type + json_schema::EntityTypeKind::Standard(json_schema::StandardEntityType { + member_of_types: d.member_of_types.into_iter().map(RawName::from).collect(), + shape: convert_attr_decls(d.attrs), + tags: d.tags.map(cedar_type_to_json_type), + }) + } + }, annotations: e.annotations.into(), - loc: Some(e.data.loc), + loc: Some(e.data.loc.clone()), }; // Then map over all of the bound names - collect_all_errors(e.data.node.names.into_iter().map( - move |name| -> Result<_, ToJsonSchemaErrors> { Ok((convert_id(name)?, etype.clone())) }, - )) + collect_all_errors( + names + .into_iter() + .map(move |name| -> Result<_, ToJsonSchemaErrors> { + Ok((convert_id(name)?, etype.clone())) + }), + ) } /// Create a [`json_schema::AttributesOrContext`] from a series of `AttrDecl`s @@ -482,7 +488,7 @@ impl NamespaceRecord { let entities = collect_decls( entities .into_iter() - .flat_map(|decl| decl.names.clone()) + .flat_map(|decl| decl.names()) .map(extract_name), )?; // Ensure no duplicate actions From 9abf890b66659373e53bb96271a2b7003377c0c6 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 7 Jan 2025 13:26:33 -0800 Subject: [PATCH 06/28] more tests Signed-off-by: Shaobo He --- .../src/cedar_schema/grammar.lalrpop | 2 +- .../src/cedar_schema/test.rs | 89 ++++++++++++++++++- 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop index 4f25d3af1..4426f802a 100644 --- a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop +++ b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop @@ -137,7 +137,7 @@ Entity: Node = { attrs: Node::with_source_loc(ds.map(|ds| ds.unwrap_or_default()).unwrap_or_default(), Loc::new(l2..r2, Arc::clone(src))), tags: ts, })), Loc::new(l1..r1, Arc::clone(src))), - ENTITY ENUM "[" "]" => Node::with_source_loc(Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { + ENTITY ENUM "[" "]" ";" => Node::with_source_loc(Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { names: ets, choices, })), Loc::new(l..r, Arc::clone(src))), diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs index 3a8b958c8..ea9e6cd2f 100644 --- a/cedar-policy-validator/src/cedar_schema/test.rs +++ b/cedar-policy-validator/src/cedar_schema/test.rs @@ -920,7 +920,10 @@ namespace Baz {action "Foo" appliesTo { } mod parser_tests { - use crate::cedar_schema::parser::parse_schema; + use crate::cedar_schema::{ + ast::{Annotated, Declaration, EntityDecl, EnumEntityDecl, Namespace}, + parser::parse_schema, + }; use cool_asserts::assert_matches; #[test] @@ -1143,6 +1146,65 @@ mod parser_tests { ); assert_matches!(res, Ok(_)); } + + #[test] + fn enumerated_entity_types() { + let res = parse_schema( + r#" + entity Application enum [ "TinyTodo" ]; + entity User in [ Application ]; + "#, + ); + assert_matches!(res, Ok(ns) => { + assert_matches!(&ns, [Annotated {data: Namespace { decls, ..}, ..}, ..] => { + assert_matches!(decls, [Annotated { data, .. }] => { + assert_matches!(&data.node, Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { choices, ..})) => { + assert_eq!(choices.clone().map(|n| n.node), nonempty::NonEmpty::singleton("TinyTodo".into())); + }); + }); + }); + }); + let res = parse_schema( + r#" + entity Application enum [ "TinyTodo", "GitHub", "DocumentCloud" ]; + entity User in [ Application ]; + "#, + ); + assert_matches!(res, Ok(ns) => { + assert_matches!(&ns, [Annotated {data: Namespace { decls, ..}, ..}, ..] => { + assert_matches!(decls, [Annotated { data, .. }] => { + assert_matches!(&data.node, Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { choices, ..})) => { + assert_eq!(choices.clone().map(|n| n.node), nonempty::nonempty!["TinyTodo".into(), "GitHub".into(), "DocumentCloud".into()]); + }); + }); + }); + }); + let res = parse_schema( + r#" + entity enum enum [ "enum" ]; + "#, + ); + assert_matches!(res, Ok(ns) => { + assert_matches!(&ns, [Annotated {data: Namespace { decls, ..}, ..}] => { + assert_matches!(decls, [Annotated { data, .. }] => { + assert_matches!(&data.node, Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { choices, ..})) => { + assert_eq!(choices.clone().map(|n| n.node), nonempty::NonEmpty::singleton("enum".into())); + }); + }); + }); + }); + + let res = parse_schema( + r#" + entity Application enum [ ]; + entity User in [ Application ]; + "#, + ); + // Maybe we want a better error message here + assert_matches!(res, Err(errs) => { + assert_eq!(errs.to_string(), "unexpected token `]`"); + }); + } } mod translator_tests { @@ -2262,6 +2324,31 @@ mod translator_tests { }), ); } + + #[test] + fn enumerated_entity_types() { + let src = r#" + entity Fruits enum ["🍍", "🥭", "🥝"]; + "#; + + let (schema, _) = + json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); + let ns = schema.0.get(&None).unwrap(); + assert_matches!(ns.entity_types.get(&"Fruits".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Enum { choices }, ..} => { + assert_eq!(choices.as_slice(), ["🍍", "🥭", "🥝"]); + }); + + let src = r#" + entity enum enum ["enum"]; + "#; + + let (schema, _) = + json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); + let ns = schema.0.get(&None).unwrap(); + assert_matches!(ns.entity_types.get(&"enum".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Enum { choices }, ..} => { + assert_eq!(choices.as_slice(), ["enum"]); + }); + } } mod common_type_references { From ea628de74161f11dd2c707fb06dc228e0eef5607 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 7 Jan 2025 13:35:05 -0800 Subject: [PATCH 07/28] fix Signed-off-by: Shaobo He --- cedar-policy-validator/src/cedar_schema/err.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cedar-policy-validator/src/cedar_schema/err.rs b/cedar-policy-validator/src/cedar_schema/err.rs index d5b65b2bf..fbd283251 100644 --- a/cedar-policy-validator/src/cedar_schema/err.rs +++ b/cedar-policy-validator/src/cedar_schema/err.rs @@ -89,6 +89,7 @@ lazy_static! { ("SET", "`Set`"), ("IDENTIFIER", "identifier"), ("TAGS", "`tags`"), + ("ENUM", "`enum`"), ]), impossible_tokens: HashSet::new(), special_identifier_tokens: HashSet::from([ @@ -106,6 +107,7 @@ lazy_static! { "LONG", "STRING", "BOOL", + "ENUM", ]), identifier_sentinel: "IDENTIFIER", first_set_identifier_tokens: HashSet::from(["SET"]), From d538eb0550650b17aa99429a4ffbfced726a6ae0 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 7 Jan 2025 15:05:23 -0800 Subject: [PATCH 08/28] request validation Signed-off-by: Shaobo He --- cedar-policy-validator/src/coreschema.rs | 43 ++++++++++++++++++++++-- cedar-policy/src/api/err.rs | 17 ++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 879a6e791..1c7d86983 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -13,10 +13,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -use crate::{ValidatorEntityType, ValidatorSchema}; +use crate::{ValidatorEntityType, ValidatorEntityTypeKind, ValidatorSchema}; use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use cedar_policy_core::{ast, entities}; use miette::Diagnostic; +use request_validation_errors::InvalidEnumEntityError; use smol_str::SmolStr; use std::collections::hash_map::Values; use std::collections::HashSet; @@ -171,7 +172,21 @@ impl ast::RequestSchema for ValidatorSchema { euid: principal, .. } = request.principal() { - if self.get_entity_type(principal.entity_type()).is_none() { + if let Some(et) = self.get_entity_type(principal.entity_type()) { + if let ValidatorEntityType { + kind: ValidatorEntityTypeKind::Enum(choices), + .. + } = et + { + if choices.contains(principal.eid().as_ref()) { + return Err(request_validation_errors::InvalidEnumEntityError { + euid: principal.clone(), + choices: choices.clone(), + } + .into()); + } + } + } else { return Err(request_validation_errors::UndeclaredPrincipalTypeError { principal_ty: principal.entity_type().clone(), } @@ -298,6 +313,11 @@ pub enum RequestValidationError { #[error("context is not valid: {0}")] #[diagnostic(transparent)] TypeOfContext(ExtensionFunctionLookupError), + /// Error when a principal or resource entity is of an enumerated entity + /// type but has an invalid EID + #[error(transparent)] + #[diagnostic(transparent)] + InvalidEnumEntity(#[from] InvalidEnumEntityError), } /// Errors related to validation @@ -306,6 +326,7 @@ pub mod request_validation_errors { use cedar_policy_core::impl_diagnostic_from_method_on_field; use itertools::Itertools; use miette::Diagnostic; + use smol_str::SmolStr; use std::sync::Arc; use thiserror::Error; @@ -511,6 +532,24 @@ pub mod request_validation_errors { &self.action } } + + /// Request principal or resource is an invalid enumerated entity + #[derive(Debug, Error)] + #[error("entity `{euid}` is not declared as an enumerated entity but has invalid eid: `{}`", euid.eid().escaped())] + pub struct InvalidEnumEntityError { + /// problematic EUID + pub(super) euid: Arc, + /// valid entity EIDs + pub(super) choices: Vec, + } + + impl Diagnostic for InvalidEnumEntityError { + impl_diagnostic_from_method_on_field!(euid, loc); + + fn help<'a>(&'a self) -> Option> { + Some(Box::new(format!("valid entity eids: {:?}", self.choices))) + } + } } /// Struct which carries enough information that it can impl Core's diff --git a/cedar-policy/src/api/err.rs b/cedar-policy/src/api/err.rs index 7a751052d..30d45788b 100644 --- a/cedar-policy/src/api/err.rs +++ b/cedar-policy/src/api/err.rs @@ -1083,6 +1083,11 @@ pub enum RequestValidationError { #[error(transparent)] #[diagnostic(transparent)] TypeOfContext(#[from] request_validation_errors::TypeOfContextError), + /// Error when a principal or resource entity is of an enumerated entity + /// type but has an invalid EID + #[error(transparent)] + #[diagnostic(transparent)] + InvalidEnumEntity(#[from] request_validation_errors::InvalidEnumEntityError), } #[doc(hidden)] @@ -1110,6 +1115,9 @@ impl From for RequestValidationE cedar_policy_validator::RequestValidationError::TypeOfContext(e) => { Self::TypeOfContext(e.into()) } + cedar_policy_validator::RequestValidationError::InvalidEnumEntity(e) => { + Self::InvalidEnumEntity(e.into()) + } } } } @@ -1235,6 +1243,15 @@ pub mod request_validation_errors { #[error(transparent)] #[diagnostic(transparent)] pub struct TypeOfContextError(#[from] ExtensionFunctionLookupError); + + /// Error when a principal or resource entity is of an enumerated entity + /// type but has an invalid EID + #[derive(Debug, Diagnostic, Error)] + #[error(transparent)] + #[diagnostic(transparent)] + pub struct InvalidEnumEntityError( + #[from] cedar_policy_validator::request_validation_errors::InvalidEnumEntityError, + ); } /// An error generated by entity slicing. From 2b2c64db92f3011de943394f37c55e906e70d0a5 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 7 Jan 2025 20:19:43 -0800 Subject: [PATCH 09/28] an interesting idea to handle ugly error messages of `untagged` Signed-off-by: Shaobo He --- cedar-policy-core/src/entities.rs | 6 +++ cedar-policy-core/src/entities/json/schema.rs | 7 +++ cedar-policy-validator/src/coreschema.rs | 7 +++ cedar-policy-validator/src/json_schema.rs | 51 +++++++++++++++++-- 4 files changed, 68 insertions(+), 3 deletions(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 6ea78cef0..51d6b04bb 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -2149,6 +2149,9 @@ mod schema_based_parsing_tests { /// Mock schema impl for the `Employee` type used in most of these tests struct MockEmployeeDescription; impl EntityTypeDescription for MockEmployeeDescription { + fn choices(&self) -> Option> { + None + } fn entity_type(&self) -> EntityType { EntityType::from(Name::parse_unqualified_name("Employee").expect("valid")) } @@ -3398,6 +3401,9 @@ mod schema_based_parsing_tests { struct MockEmployeeDescription; impl EntityTypeDescription for MockEmployeeDescription { + fn choices(&self) -> Option> { + None + } fn entity_type(&self) -> EntityType { "XYZCorp::Employee".parse().expect("valid") } diff --git a/cedar-policy-core/src/entities/json/schema.rs b/cedar-policy-core/src/entities/json/schema.rs index 9dd8b7072..a834c2d35 100644 --- a/cedar-policy-core/src/entities/json/schema.rs +++ b/cedar-policy-core/src/entities/json/schema.rs @@ -136,6 +136,10 @@ pub trait EntityTypeDescription { /// May entities with this type have attributes other than those specified /// in the schema fn open_attributes(&self) -> bool; + + /// Return valid EID choices if the entity type is enumerated otherwise + /// return `None` + fn choices(&self) -> Option>; } /// Simple type that implements `EntityTypeDescription` by expecting no @@ -164,6 +168,9 @@ impl EntityTypeDescription for NullEntityTypeDescription { fn open_attributes(&self) -> bool { false } + fn choices(&self) -> Option> { + None + } } impl NullEntityTypeDescription { /// Create a new [`NullEntityTypeDescription`] for the given entity typename diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 1c7d86983..21faad9a6 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -106,6 +106,13 @@ impl EntityTypeDescription { } impl entities::EntityTypeDescription for EntityTypeDescription { + fn choices(&self) -> Option> { + match &self.validator_type.kind { + ValidatorEntityTypeKind::Enum(choices) => Some(choices.clone()), + _ => None, + } + } + fn entity_type(&self) -> ast::EntityType { self.core_type.clone() } diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index 14924362d..969884cec 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -447,7 +447,7 @@ pub enum EntityTypeKind { } /// Represents the definition of a common type in the schema. -#[derive(Educe, Debug, Clone, Serialize, Deserialize)] +#[derive(Educe, Debug, Clone, Serialize)] #[educe(PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] pub struct EntityType { @@ -468,6 +468,50 @@ pub struct EntityType { pub loc: Option, } +impl<'de, N: Deserialize<'de> + From> Deserialize<'de> for EntityType { + fn deserialize(deserializer: D) -> std::result::Result + where + D: serde::Deserializer<'de>, + { + #[derive(Deserialize)] + #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] + #[serde(deny_unknown_fields)] + #[serde(rename_all = "camelCase")] + struct Everything { + #[serde(default)] + member_of_types: Option>, + #[serde(default)] + shape: Option>, + #[serde(default)] + tags: Option>, + #[serde(default)] + #[serde(rename = "enum")] + choices: Option>, + #[serde(default)] + annotations: Option, + } + + let value: Everything = Everything::deserialize(deserializer)?; + if let Some(choices) = value.choices { + Ok(EntityType { + kind: EntityTypeKind::Enum { choices }, + annotations: value.annotations.unwrap_or_default(), + loc: None, + }) + } else { + Ok(EntityType { + kind: EntityTypeKind::Standard(StandardEntityType { + member_of_types: value.member_of_types.unwrap_or_default(), + shape: value.shape.unwrap_or_default(), + tags: value.tags, + }), + annotations: value.annotations.unwrap_or_default(), + loc: None, + }) + } + } +} + /// Represents the full definition of an entity type in the schema. /// Entity types describe the relationships in the entity store, including what /// entities can be members of groups of what types, and what attributes @@ -3458,7 +3502,8 @@ mod annotations { }); } - const ENTITY_TYPE_EXPECTED_ATTRIBUTES: &str = "`memberOfTypes`, `shape`, `tags`, `annotations`"; + const ENTITY_TYPE_EXPECTED_ATTRIBUTES: &str = + "`memberOfTypes`, `shape`, `tags`, `enum`, `annotations`"; const NAMESPACE_EXPECTED_ATTRIBUTES: &str = "`commonTypes`, `entityTypes`, `actions`, `annotations`"; const ATTRIBUTE_TYPE_EXPECTED_ATTRIBUTES: &str = @@ -3629,7 +3674,7 @@ mod annotations { "annotations": { "foo": "" }, - "bar": 1 + "bar": 1, } } } From 50734089f75809df8a43080948bca7156c11cf0e Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Thu, 9 Jan 2025 14:24:19 -0800 Subject: [PATCH 10/28] updates Signed-off-by: Shaobo He --- .../src/cedar_schema/to_json_schema.rs | 2 +- cedar-policy-validator/src/json_schema.rs | 141 ++++++++++++++++-- 2 files changed, 131 insertions(+), 12 deletions(-) diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index e30e80778..f525f358b 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -362,7 +362,7 @@ fn convert_entity_decl( impl Iterator)>, ToJsonSchemaErrors, > { - let names: Vec> = e.data.node.clone().names().collect(); + let names: Vec> = e.data.node.names().collect(); let etype = json_schema::EntityType { kind: match e.data.node { EntityDecl::Enum(d) => json_schema::EntityTypeKind::Enum { diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index 969884cec..cc2dcb197 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -25,6 +25,7 @@ use cedar_policy_core::{ FromNormalizedStr, }; use educe::Educe; +use itertools::Itertools; use nonempty::nonempty; use serde::{ de::{MapAccess, Visitor}, @@ -473,39 +474,90 @@ impl<'de, N: Deserialize<'de> + From> Deserialize<'de> for EntityType, { + // A "real" option that does not accept `null` during deserialization + // TODO: we should be able to use `serde_as` or `serde_with` instead + enum RealOption { + Some(T), + None, + } + impl<'de, T: Deserialize<'de>> Deserialize<'de> for RealOption { + fn deserialize(deserializer: D) -> std::result::Result + where + D: Deserializer<'de>, + { + T::deserialize(deserializer).map(Self::Some) + } + } + impl Default for RealOption { + fn default() -> Self { + Self::None + } + } + + impl From> for Option { + fn from(value: RealOption) -> Self { + match value { + RealOption::Some(v) => Self::Some(v), + RealOption::None => None, + } + } + } + + // A struct that contains all possible fields of entity type + // I tried to apply the same idea to `EntityTypeKind` but serde allows + // unknown fields #[derive(Deserialize)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] #[serde(deny_unknown_fields)] #[serde(rename_all = "camelCase")] struct Everything { #[serde(default)] - member_of_types: Option>, + member_of_types: RealOption>, #[serde(default)] - shape: Option>, + shape: RealOption>, #[serde(default)] - tags: Option>, + tags: RealOption>, #[serde(default)] #[serde(rename = "enum")] - choices: Option>, + choices: RealOption>, #[serde(default)] - annotations: Option, + annotations: Annotations, } let value: Everything = Everything::deserialize(deserializer)?; - if let Some(choices) = value.choices { + // We favor the "enum" key here. That is, when we observe this key, we + // assume the entity type is an enumerated one and hence reports fields + // of standard entity types as invalid. + if let Some(choices) = value.choices.into() { + let mut unexpected_fields: Vec<&str> = vec![]; + if Option::>::from(value.member_of_types).is_some() { + unexpected_fields.push("memberOfTypes"); + } + if Option::>::from(value.shape).is_some() { + unexpected_fields.push("shape"); + } + if Option::>::from(value.tags).is_some() { + unexpected_fields.push("tags"); + } + if !unexpected_fields.is_empty() { + return Err(serde::de::Error::custom(format!( + "unexpected field: {}", + unexpected_fields.into_iter().join(", ") + ))); + } Ok(EntityType { kind: EntityTypeKind::Enum { choices }, - annotations: value.annotations.unwrap_or_default(), + annotations: value.annotations, loc: None, }) } else { Ok(EntityType { kind: EntityTypeKind::Standard(StandardEntityType { - member_of_types: value.member_of_types.unwrap_or_default(), - shape: value.shape.unwrap_or_default(), - tags: value.tags, + member_of_types: Option::from(value.member_of_types).unwrap_or_default(), + shape: Option::from(value.shape).unwrap_or_default(), + tags: Option::from(value.tags), }), - annotations: value.annotations.unwrap_or_default(), + annotations: value.annotations, loc: None, }) } @@ -3726,3 +3778,70 @@ mod ord { }); } } + +#[cfg(test)] +mod enumerated_entity_types { + use cool_asserts::assert_matches; + + use crate::{ + json_schema::{EntityType, EntityTypeKind, Fragment}, + RawName, + }; + + #[test] + fn basic() { + let src = serde_json::json!({ + "": { + "entityTypes": { + "Foo": { + "enum": ["foo", "bar"], + "annotations": { + "a": "b", + } + }, + }, + "actions": {}, + } + }); + let schema: Result, _> = serde_json::from_value(src); + assert_matches!(schema, Ok(frag) => { + assert_matches!(&frag.0[&None].entity_types[&"Foo".parse().unwrap()], EntityType { + kind: EntityTypeKind::Enum {choices}, + .. + } => { + assert_eq!(choices.as_slice(), ["foo", "bar"]); + }); + }); + + let src = serde_json::json!({ + "": { + "entityTypes": { + "Foo": { + "enum": null, + }, + }, + "actions": {}, + } + }); + let schema: Result, _> = serde_json::from_value(src); + assert_matches!(schema, Err(errs) => { + assert_eq!(errs.to_string(), "invalid type: null, expected a sequence"); + }); + + let src = serde_json::json!({ + "": { + "entityTypes": { + "Foo": { + "enum": ["foo"], + "memberOfTypes": ["bar"], + }, + }, + "actions": {}, + } + }); + let schema: Result, _> = serde_json::from_value(src); + assert_matches!(schema, Err(errs) => { + assert_eq!(errs.to_string(), "unexpected field: memberOfTypes"); + }); + } +} From c05645669bef8134351b4d13d7dc086c0e23c405 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Thu, 9 Jan 2025 14:51:44 -0800 Subject: [PATCH 11/28] prototype Signed-off-by: Shaobo He --- .../src/entities/conformance/err.rs | 28 +++++++++++++++++++ .../src/entities/json/entities.rs | 12 ++++++++ 2 files changed, 40 insertions(+) diff --git a/cedar-policy-core/src/entities/conformance/err.rs b/cedar-policy-core/src/entities/conformance/err.rs index 666a611a3..8983f2512 100644 --- a/cedar-policy-core/src/entities/conformance/err.rs +++ b/cedar-policy-core/src/entities/conformance/err.rs @@ -70,6 +70,10 @@ pub enum EntitySchemaConformanceError { #[error(transparent)] #[diagnostic(transparent)] ExtensionFunctionLookup(ExtensionFunctionLookup), + /// Returned when an entity is of an enumerated entity type but has invalid EID + #[error(transparent)] + #[diagnostic(transparent)] + InvalidEnumEntity(#[from] InvalidEnumEntity), } impl EntitySchemaConformanceError { @@ -132,6 +136,16 @@ impl EntitySchemaConformanceError { err, }) } + + pub(crate) fn invalid_enum_entity( + uid: EntityUID, + choices: impl IntoIterator, + ) -> Self { + Self::InvalidEnumEntity(InvalidEnumEntity { + uid, + choices: choices.into_iter().collect(), + }) + } } /// Error looking up an extension function. This error can occur when @@ -277,3 +291,17 @@ impl Diagnostic for UnexpectedEntityTypeError { } } } + +/// Returned when an entity is of an enumerated entity type but has invalid EID +// +// CAUTION: this type is publicly exported in `cedar-policy`. +// Don't make fields `pub`, don't make breaking changes, and use caution +// when adding public methods. +#[derive(Debug, Error, Diagnostic)] +#[error("entity `{uid}` has invalid UID: `{}`", uid.eid().escaped())] +pub struct InvalidEnumEntity { + /// Entity where the error occurred + uid: EntityUID, + /// Name of the attribute where the error occurred + choices: Vec, +} diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index b555176b7..9a2e67870 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -297,6 +297,18 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { } } }; + match &entity_schema_info { + EntitySchemaInfo::NonAction(desc) => { + if let Some(choices) = desc.choices() { + if !choices.contains(uid.eid().as_ref()) { + return Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::invalid_enum_entity(uid.clone(), choices), + )); + } + } + } + _ => {} + }; let vparser = ValueParser::new(self.extensions); let attrs: HashMap = ejson .attrs From baba3abcf79e75226ea3b426aea12bf4e94b2bf4 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Fri, 10 Jan 2025 16:37:21 -0800 Subject: [PATCH 12/28] nonemptize Signed-off-by: Shaobo He --- Cargo.lock | 3 ++ cedar-policy-core/Cargo.toml | 2 +- cedar-policy-core/src/entities.rs | 5 ++-- .../src/entities/json/entities.rs | 2 +- cedar-policy-core/src/entities/json/schema.rs | 5 ++-- .../src/cedar_schema/test.rs | 4 +-- .../src/cedar_schema/to_json_schema.rs | 2 +- cedar-policy-validator/src/coreschema.rs | 6 ++-- cedar-policy-validator/src/json_schema.rs | 30 +++++++++++++++---- .../src/schema/entity_type.rs | 3 +- .../src/schema/namespace_def.rs | 2 +- 11 files changed, 45 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 755a9952a..8e6ae646d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1504,6 +1504,9 @@ name = "nonempty" version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "303e8749c804ccd6ca3b428de7fe0d86cb86bc7606bc15291f100fd487960bb8" +dependencies = [ + "serde", +] [[package]] name = "normalize-line-endings" diff --git a/cedar-policy-core/Cargo.toml b/cedar-policy-core/Cargo.toml index 0b25a5fff..46cbc3028 100644 --- a/cedar-policy-core/Cargo.toml +++ b/cedar-policy-core/Cargo.toml @@ -27,7 +27,7 @@ smol_str = { version = "0.3", features = ["serde"] } stacker = "0.1.15" arbitrary = { version = "1", features = ["derive"], optional = true } miette = { version = "7.4.0", features = ["serde"] } -nonempty = "0.10.0" +nonempty = { version = "0.10.0", features = ["serialize"] } educe = "0.6.0" # decimal extension requires regex diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 51d6b04bb..d90530357 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -2060,6 +2060,7 @@ mod schema_based_parsing_tests { use crate::extensions::Extensions; use crate::test_utils::*; use cool_asserts::assert_matches; + use nonempty::NonEmpty; use serde_json::json; use smol_str::SmolStr; use std::collections::HashSet; @@ -2149,7 +2150,7 @@ mod schema_based_parsing_tests { /// Mock schema impl for the `Employee` type used in most of these tests struct MockEmployeeDescription; impl EntityTypeDescription for MockEmployeeDescription { - fn choices(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { None } fn entity_type(&self) -> EntityType { @@ -3401,7 +3402,7 @@ mod schema_based_parsing_tests { struct MockEmployeeDescription; impl EntityTypeDescription for MockEmployeeDescription { - fn choices(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { None } fn entity_type(&self) -> EntityType { diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 9a2e67870..e9e2e1b18 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -299,7 +299,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { }; match &entity_schema_info { EntitySchemaInfo::NonAction(desc) => { - if let Some(choices) = desc.choices() { + if let Some(choices) = desc.enum_enity_eids() { if !choices.contains(uid.eid().as_ref()) { return Err(JsonDeserializationError::EntitySchemaConformance( EntitySchemaConformanceError::invalid_enum_entity(uid.clone(), choices), diff --git a/cedar-policy-core/src/entities/json/schema.rs b/cedar-policy-core/src/entities/json/schema.rs index a834c2d35..c9ccdd6c8 100644 --- a/cedar-policy-core/src/entities/json/schema.rs +++ b/cedar-policy-core/src/entities/json/schema.rs @@ -17,6 +17,7 @@ use super::SchemaType; use crate::ast::{Entity, EntityType, EntityUID}; use crate::entities::{Name, UnreservedId}; +use nonempty::NonEmpty; use smol_str::SmolStr; use std::collections::HashSet; use std::sync::Arc; @@ -139,7 +140,7 @@ pub trait EntityTypeDescription { /// Return valid EID choices if the entity type is enumerated otherwise /// return `None` - fn choices(&self) -> Option>; + fn enum_enity_eids(&self) -> Option>; } /// Simple type that implements `EntityTypeDescription` by expecting no @@ -168,7 +169,7 @@ impl EntityTypeDescription for NullEntityTypeDescription { fn open_attributes(&self) -> bool { false } - fn choices(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { None } } diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs index ea9e6cd2f..0efafcb51 100644 --- a/cedar-policy-validator/src/cedar_schema/test.rs +++ b/cedar-policy-validator/src/cedar_schema/test.rs @@ -2335,7 +2335,7 @@ mod translator_tests { json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); assert_matches!(ns.entity_types.get(&"Fruits".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Enum { choices }, ..} => { - assert_eq!(choices.as_slice(), ["🍍", "🥭", "🥝"]); + assert_eq!(Vec::from(choices.clone()), ["🍍", "🥭", "🥝"]); }); let src = r#" @@ -2346,7 +2346,7 @@ mod translator_tests { json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap(); let ns = schema.0.get(&None).unwrap(); assert_matches!(ns.entity_types.get(&"enum".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Enum { choices }, ..} => { - assert_eq!(choices.as_slice(), ["enum"]); + assert_eq!(Vec::from(choices.clone()), ["enum"]); }); } } diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index f525f358b..ba2fb77d7 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -366,7 +366,7 @@ fn convert_entity_decl( let etype = json_schema::EntityType { kind: match e.data.node { EntityDecl::Enum(d) => json_schema::EntityTypeKind::Enum { - choices: d.choices.into_iter().map(|n| n.node).collect(), + choices: d.choices.map(|n| n.node), }, EntityDecl::Standard(d) => { // First build up the defined entity type diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 0b17aabe3..261dca9fb 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -18,6 +18,7 @@ use cedar_policy_core::ast::{EntityType, EntityUID}; use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use cedar_policy_core::{ast, entities}; use miette::Diagnostic; +use nonempty::NonEmpty; use request_validation_errors::InvalidEnumEntityError; use smol_str::SmolStr; use std::collections::hash_map::Values; @@ -107,7 +108,7 @@ impl EntityTypeDescription { } impl entities::EntityTypeDescription for EntityTypeDescription { - fn choices(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { match &self.validator_type.kind { ValidatorEntityTypeKind::Enum(choices) => Some(choices.clone()), _ => None, @@ -360,6 +361,7 @@ pub mod request_validation_errors { use cedar_policy_core::impl_diagnostic_from_method_on_field; use itertools::Itertools; use miette::Diagnostic; + use nonempty::NonEmpty; use smol_str::SmolStr; use std::sync::Arc; use thiserror::Error; @@ -574,7 +576,7 @@ pub mod request_validation_errors { /// problematic EUID pub(super) euid: Arc, /// valid entity EIDs - pub(super) choices: Vec, + pub(super) choices: NonEmpty, } impl Diagnostic for InvalidEnumEntityError { diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index cc2dcb197..ae1eaa8d4 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -26,7 +26,7 @@ use cedar_policy_core::{ }; use educe::Educe; use itertools::Itertools; -use nonempty::nonempty; +use nonempty::{nonempty, NonEmpty}; use serde::{ de::{MapAccess, Visitor}, ser::SerializeMap, @@ -433,8 +433,7 @@ impl NamespaceDefinition { } /// ... -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(bound(deserialize = "N: Deserialize<'de> + From"))] +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[serde(untagged)] pub enum EntityTypeKind { /// ... @@ -443,7 +442,7 @@ pub enum EntityTypeKind { Enum { #[serde(rename = "enum")] /// ... - choices: Vec, + choices: NonEmpty, }, } @@ -519,7 +518,7 @@ impl<'de, N: Deserialize<'de> + From> Deserialize<'de> for EntityType>, #[serde(default)] #[serde(rename = "enum")] - choices: RealOption>, + choices: RealOption>, #[serde(default)] annotations: Annotations, } @@ -3809,10 +3808,29 @@ mod enumerated_entity_types { kind: EntityTypeKind::Enum {choices}, .. } => { - assert_eq!(choices.as_slice(), ["foo", "bar"]); + assert_eq!(Vec::from(choices.clone()), ["foo", "bar"]); }); }); + let src = serde_json::json!({ + "": { + "entityTypes": { + "Foo": { + "enum": [], + "annotations": { + "a": "b", + } + }, + }, + "actions": {}, + } + }); + let schema: Result, _> = serde_json::from_value(src); + assert_matches!(schema, Err(errs) => { + // TODO: write our own error messages if it's deemed to be too ugly. + assert_eq!(errs.to_string(), "the vector provided was empty, NonEmpty needs at least one element"); + }); + let src = serde_json::json!({ "": { "entityTypes": { diff --git a/cedar-policy-validator/src/schema/entity_type.rs b/cedar-policy-validator/src/schema/entity_type.rs index 8f90f4f86..47fc22a53 100644 --- a/cedar-policy-validator/src/schema/entity_type.rs +++ b/cedar-policy-validator/src/schema/entity_type.rs @@ -16,6 +16,7 @@ //! This module contains the definition of `ValidatorEntityType` +use nonempty::NonEmpty; use serde::Serialize; use smol_str::SmolStr; use std::collections::HashSet; @@ -72,7 +73,7 @@ pub enum ValidatorEntityTypeKind { /// Standard, aka non-enum Standard(StandardValidatorEntityType), /// Enumerated - Enum(Vec), + Enum(NonEmpty), } impl ValidatorEntityType { diff --git a/cedar-policy-validator/src/schema/namespace_def.rs b/cedar-policy-validator/src/schema/namespace_def.rs index dd08c1df1..e1c3334e4 100644 --- a/cedar-policy-validator/src/schema/namespace_def.rs +++ b/cedar-policy-validator/src/schema/namespace_def.rs @@ -474,7 +474,7 @@ pub enum EntityTypeFragment { /// fragment). tags: Option>, }, - Enum(Vec), + Enum(NonEmpty), } impl EntityTypeFragment { From b2518f04c21af5a7f43b551ef344ac544d884f36 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Fri, 10 Jan 2025 18:10:32 -0800 Subject: [PATCH 13/28] clean up Signed-off-by: Shaobo He --- cedar-policy-validator/src/json_schema.rs | 60 +++++++++++------------ 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index ae1eaa8d4..211f289b7 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -432,24 +432,36 @@ impl NamespaceDefinition { } } -/// ... +/// The kind of entity type. There are currently two kinds: The standard entity +/// type specified by [`StandardEntityType`] and the enumerated entity type +/// proposed by RFC 53 #[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[serde(untagged)] pub enum EntityTypeKind { - /// ... + /// The standard entity type specified by [`StandardEntityType`] Standard(StandardEntityType), - /// ... + /// The enumerated entity type: An entity type that can only have a + /// nonempty set of possible EIDs Enum { #[serde(rename = "enum")] - /// ... + /// The nonempty set of possible EIDs choices: NonEmpty, }, } -/// Represents the definition of a common type in the schema. +/// Represents the full definition of an entity type in the schema. +/// Entity types describe the relationships in the entity store, including what +/// entities can be members of groups of what types, and what attributes +/// can/should be included on entities of each type. +/// +/// The parameter `N` is the type of entity type names and common type names in +/// this [`EntityType`], including recursively. +/// See notes on [`Fragment`]. #[derive(Educe, Debug, Clone, Serialize)] #[educe(PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] +#[cfg_attr(feature = "wasm", derive(tsify::Tsify))] +#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] pub struct EntityType { /// The referred type #[serde(flatten)] @@ -563,32 +575,19 @@ impl<'de, N: Deserialize<'de> + From> Deserialize<'de> for EntityType { /// Entities of this [`EntityType`] are allowed to be members of entities of /// these types. - #[serde(default)] #[serde(skip_serializing_if = "Vec::is_empty")] pub member_of_types: Vec, /// Description of the attributes for entities of this [`EntityType`]. - #[serde(default)] #[serde(skip_serializing_if = "AttributesOrContext::is_empty_record")] pub shape: AttributesOrContext, /// Tag type for entities of this [`EntityType`]; `None` means entities of this [`EntityType`] do not have tags. - #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub tags: Option>, } @@ -2145,7 +2144,7 @@ mod test { "memberOfTypes" : ["UserGroup"] } "#; - let et = serde_json::from_str::>(user).expect("Parse Error"); + assert_matches!(serde_json::from_str::>(user), Ok(EntityType { kind: EntityTypeKind::Standard(et), .. }) => { assert_eq!(et.member_of_types, vec!["UserGroup".parse().unwrap()]); assert_eq!( et.shape, @@ -2156,7 +2155,7 @@ mod test { }), loc: None }), - ); + );}); } #[test] @@ -2164,7 +2163,7 @@ mod test { let src = r#" { } "#; - let et = serde_json::from_str::>(src).expect("Parse Error"); + assert_matches!(serde_json::from_str::>(src), Ok(EntityType { kind: EntityTypeKind::Standard(et), .. }) => { assert_eq!(et.member_of_types.len(), 0); assert_eq!( et.shape, @@ -2175,7 +2174,7 @@ mod test { }), loc: None }), - ); + );}); } #[test] @@ -2559,8 +2558,7 @@ mod strengthened_types { use cool_asserts::assert_matches; use super::{ - ActionEntityUID, ApplySpec, Fragment, NamespaceDefinition, RawName, StandardEntityType, - Type, + ActionEntityUID, ApplySpec, EntityType, Fragment, NamespaceDefinition, RawName, Type, }; /// Assert that `result` is an `Err`, and the error message matches `msg` @@ -2703,28 +2701,28 @@ mod strengthened_types { { "memberOfTypes": [""] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name ``: unexpected end of input"); let src = serde_json::json!( { "memberOfTypes": ["*"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `*`: unexpected token `*`"); let src = serde_json::json!( { "memberOfTypes": ["A::"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `A::`: unexpected end of input"); let src = serde_json::json!( { "memberOfTypes": ["::A"] }); - let schema: Result, _> = serde_json::from_value(src); + let schema: Result, _> = serde_json::from_value(src); assert_error_matches(schema, "invalid name `::A`: unexpected token `::`"); } From 10486da8319633843febb06200f923adc648f8fc Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 13 Jan 2025 10:10:02 -0800 Subject: [PATCH 14/28] fix wasm Signed-off-by: Shaobo He --- cedar-policy-validator/src/json_schema.rs | 6 ++++-- cedar-wasm/build-wasm.sh | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index 211f289b7..ca8f55dc3 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -437,6 +437,8 @@ impl NamespaceDefinition { /// proposed by RFC 53 #[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[serde(untagged)] +#[cfg_attr(feature = "wasm", derive(tsify::Tsify))] +#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] pub enum EntityTypeKind { /// The standard entity type specified by [`StandardEntityType`] Standard(StandardEntityType), @@ -460,8 +462,6 @@ pub enum EntityTypeKind { #[derive(Educe, Debug, Clone, Serialize)] #[educe(PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] -#[cfg_attr(feature = "wasm", derive(tsify::Tsify))] -#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] pub struct EntityType { /// The referred type #[serde(flatten)] @@ -579,6 +579,8 @@ impl<'de, N: Deserialize<'de> + From> Deserialize<'de> for EntityType { /// Entities of this [`EntityType`] are allowed to be members of entities of /// these types. diff --git a/cedar-wasm/build-wasm.sh b/cedar-wasm/build-wasm.sh index dde967091..022e40f24 100755 --- a/cedar-wasm/build-wasm.sh +++ b/cedar-wasm/build-wasm.sh @@ -97,6 +97,8 @@ process_types_file() { echo "type SmolStr = string;" >> "$types_file" echo "export type TypeOfAttribute = Type & { required?: boolean };" >> "$types_file" echo "export type CommonType = Type & { annotations?: Annotations };" >> "$types_file" + echo "export type EntityType = EntityTypeKind & { annotations?: Annotations; };" >> "$types_file" + echo "export type NonEmpty = Array;" >> "$types_file" } check_types_file() { From c90771eb2158a8c72b469e6a2d78634ada0b6bd0 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 13 Jan 2025 10:20:46 -0800 Subject: [PATCH 15/28] fix todo Signed-off-by: Shaobo He --- cedar-policy-validator/src/cedar_schema/fmt.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cedar-policy-validator/src/cedar_schema/fmt.rs b/cedar-policy-validator/src/cedar_schema/fmt.rs index f97dc00bd..8a108c70f 100644 --- a/cedar-policy-validator/src/cedar_schema/fmt.rs +++ b/cedar-policy-validator/src/cedar_schema/fmt.rs @@ -110,7 +110,14 @@ impl Display for json_schema::EntityType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.kind { json_schema::EntityTypeKind::Standard(ty) => ty.fmt(f), - _ => todo!(), + json_schema::EntityTypeKind::Enum { choices } => write!( + f, + "[{}]", + choices + .iter() + .map(|e| format!("\"{}\"", e.escape_debug())) + .join(", ") + ), } } } From f73d8382bf4ad71440cc8b2d1f21b293e3898d58 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 13 Jan 2025 11:03:07 -0800 Subject: [PATCH 16/28] review feedback Signed-off-by: Shaobo He --- cedar-policy-core/src/entities.rs | 4 +- .../src/entities/json/entities.rs | 11 +++- cedar-policy-core/src/entities/json/schema.rs | 6 +- .../src/cedar_schema/ast.rs | 6 +- .../src/cedar_schema/to_json_schema.rs | 4 +- cedar-policy-validator/src/coreschema.rs | 6 +- cedar-policy-validator/src/rbac.rs | 63 ++++++------------- 7 files changed, 41 insertions(+), 59 deletions(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index d90530357..e503520ab 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -2150,7 +2150,7 @@ mod schema_based_parsing_tests { /// Mock schema impl for the `Employee` type used in most of these tests struct MockEmployeeDescription; impl EntityTypeDescription for MockEmployeeDescription { - fn enum_enity_eids(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { None } fn entity_type(&self) -> EntityType { @@ -3402,7 +3402,7 @@ mod schema_based_parsing_tests { struct MockEmployeeDescription; impl EntityTypeDescription for MockEmployeeDescription { - fn enum_enity_eids(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { None } fn entity_type(&self) -> EntityType { diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index e9e2e1b18..b9d3d7d28 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -19,7 +19,7 @@ use super::{ CedarValueJson, EntityTypeDescription, EntityUidJson, NoEntitiesSchema, Schema, TypeAndId, ValueParser, }; -use crate::ast::{BorrowedRestrictedExpr, Entity, EntityUID, PartialValue, RestrictedExpr}; +use crate::ast::{BorrowedRestrictedExpr, Eid, Entity, EntityUID, PartialValue, RestrictedExpr}; use crate::entities::conformance::EntitySchemaConformanceChecker; use crate::entities::{ conformance::err::{EntitySchemaConformanceError, UnexpectedEntityTypeError}, @@ -300,9 +300,14 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { match &entity_schema_info { EntitySchemaInfo::NonAction(desc) => { if let Some(choices) = desc.enum_enity_eids() { - if !choices.contains(uid.eid().as_ref()) { + if !choices.contains(&uid.eid()) { return Err(JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::invalid_enum_entity(uid.clone(), choices), + EntitySchemaConformanceError::invalid_enum_entity( + uid.clone(), + choices + .iter() + .map(|e| >::as_ref(e).clone()), + ), )); } } diff --git a/cedar-policy-core/src/entities/json/schema.rs b/cedar-policy-core/src/entities/json/schema.rs index c9ccdd6c8..b6dedc572 100644 --- a/cedar-policy-core/src/entities/json/schema.rs +++ b/cedar-policy-core/src/entities/json/schema.rs @@ -15,7 +15,7 @@ */ use super::SchemaType; -use crate::ast::{Entity, EntityType, EntityUID}; +use crate::ast::{Eid, Entity, EntityType, EntityUID}; use crate::entities::{Name, UnreservedId}; use nonempty::NonEmpty; use smol_str::SmolStr; @@ -140,7 +140,7 @@ pub trait EntityTypeDescription { /// Return valid EID choices if the entity type is enumerated otherwise /// return `None` - fn enum_enity_eids(&self) -> Option>; + fn enum_enity_eids(&self) -> Option>; } /// Simple type that implements `EntityTypeDescription` by expecting no @@ -169,7 +169,7 @@ impl EntityTypeDescription for NullEntityTypeDescription { fn open_attributes(&self) -> bool { false } - fn enum_enity_eids(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { None } } diff --git a/cedar-policy-validator/src/cedar_schema/ast.rs b/cedar-policy-validator/src/cedar_schema/ast.rs index 72e56bb73..9262fa59f 100644 --- a/cedar-policy-validator/src/cedar_schema/ast.rs +++ b/cedar-policy-validator/src/cedar_schema/ast.rs @@ -256,10 +256,10 @@ pub enum EntityDecl { } impl EntityDecl { - pub fn names(&self) -> impl Iterator> + '_ { + pub fn names(&self) -> impl Iterator> + '_ { match self { - Self::Enum(d) => d.names.iter().cloned(), - Self::Standard(d) => d.names.iter().cloned(), + Self::Enum(d) => d.names.iter(), + Self::Standard(d) => d.names.iter(), } } } diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index ba2fb77d7..7b1468b8d 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -362,7 +362,7 @@ fn convert_entity_decl( impl Iterator)>, ToJsonSchemaErrors, > { - let names: Vec> = e.data.node.names().collect(); + let names: Vec> = e.data.node.names().cloned().collect(); let etype = json_schema::EntityType { kind: match e.data.node { EntityDecl::Enum(d) => json_schema::EntityTypeKind::Enum { @@ -488,7 +488,7 @@ impl NamespaceRecord { let entities = collect_decls( entities .into_iter() - .flat_map(|decl| decl.names()) + .flat_map(|decl| decl.names().cloned()) .map(extract_name), )?; // Ensure no duplicate actions diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 261dca9fb..90b7ad7a5 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -14,7 +14,7 @@ * limitations under the License. */ use crate::{ValidatorActionId, ValidatorEntityType, ValidatorEntityTypeKind, ValidatorSchema}; -use cedar_policy_core::ast::{EntityType, EntityUID}; +use cedar_policy_core::ast::{Eid, EntityType, EntityUID}; use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use cedar_policy_core::{ast, entities}; use miette::Diagnostic; @@ -108,9 +108,9 @@ impl EntityTypeDescription { } impl entities::EntityTypeDescription for EntityTypeDescription { - fn enum_enity_eids(&self) -> Option> { + fn enum_enity_eids(&self) -> Option> { match &self.validator_type.kind { - ValidatorEntityTypeKind::Enum(choices) => Some(choices.clone()), + ValidatorEntityTypeKind::Enum(choices) => Some(choices.clone().map(|s| Eid::new(s))), _ => None, } } diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index 1c4d0d1d9..aeb4c885d 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -18,8 +18,8 @@ use cedar_policy_core::{ ast::{ - self, ActionConstraint, EntityReference, EntityUID, ExprKind, Literal, Policy, PolicyID, - PrincipalConstraint, PrincipalOrResourceConstraint, ResourceConstraint, SlotEnv, Template, + self, ActionConstraint, EntityReference, EntityUID, Policy, PolicyID, PrincipalConstraint, + PrincipalOrResourceConstraint, ResourceConstraint, SlotEnv, Template, }, fuzzy_match::fuzzy_search, parser::Loc, @@ -42,48 +42,25 @@ impl Validator { &'a self, template: &'a Template, ) -> impl Iterator + 'a { - let non_action_entities = template - .principal_constraint() - .as_inner() - .get_euid() - .into_iter() - .map(|e| e.as_ref()) - .chain( - template - .resource_constraint() - .as_inner() - .get_euid() - .into_iter() - .map(|e| e.as_ref()), - ) - .chain( - template - .non_scope_constraints() - .subexpressions() - .filter_map(|e| match e.expr_kind() { - ExprKind::Lit(Literal::EntityUID(euid)) if !euid.is_action() => { - Some(euid.as_ref()) - } - _ => None, - }), - ); - non_action_entities.filter_map(|e: &EntityUID| { - if let Some(ValidatorEntityType { - kind: ValidatorEntityTypeKind::Enum(choices), - .. - }) = self.schema.get_entity_type(e.entity_type()) - { - if !choices.contains(e.eid().as_ref()) { - return Some(ValidationError::invalid_enum_entity( - e.loc().cloned(), - template.id().clone(), - e.clone(), - choices.into_iter().cloned(), - )); + policy_entity_uids(template) + .filter(|e| !e.is_action()) + .filter_map(|e: &EntityUID| { + if let Some(ValidatorEntityType { + kind: ValidatorEntityTypeKind::Enum(choices), + .. + }) = self.schema.get_entity_type(e.entity_type()) + { + if !choices.contains(e.eid().as_ref()) { + return Some(ValidationError::invalid_enum_entity( + e.loc().cloned(), + template.id().clone(), + e.clone(), + choices.into_iter().cloned(), + )); + } } - } - None - }) + None + }) } /// Generate `UnrecognizedEntityType` error for every entity type in the /// expression that could not also be found in the schema. From 8811f1a85f4f0c8f065a165e51fe2406e34f610b Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Mon, 13 Jan 2025 13:54:11 -0800 Subject: [PATCH 17/28] unified errors in various places Signed-off-by: Shaobo He --- .../src/entities/conformance/err.rs | 51 +++++++++++++++---- .../src/entities/json/entities.rs | 9 +--- cedar-policy-validator/src/coreschema.rs | 34 +++---------- cedar-policy-validator/src/diagnostics.rs | 9 ++-- .../src/diagnostics/validation_errors.rs | 11 ++-- cedar-policy/src/api/err.rs | 2 +- 6 files changed, 63 insertions(+), 53 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance/err.rs b/cedar-policy-core/src/entities/conformance/err.rs index 8983f2512..3216a5483 100644 --- a/cedar-policy-core/src/entities/conformance/err.rs +++ b/cedar-policy-core/src/entities/conformance/err.rs @@ -15,8 +15,10 @@ */ //! This module cotnains errors around entities not conforming to schemas use super::TypeMismatchError; -use crate::ast::{EntityType, EntityUID}; +use crate::ast::{Eid, EntityType, EntityUID}; use crate::extensions::ExtensionFunctionLookupError; +use crate::impl_diagnostic_from_method_on_field; +use itertools::Itertools; use miette::Diagnostic; use smol_str::SmolStr; use thiserror::Error; @@ -139,12 +141,15 @@ impl EntitySchemaConformanceError { pub(crate) fn invalid_enum_entity( uid: EntityUID, - choices: impl IntoIterator, + choices: impl IntoIterator, ) -> Self { - Self::InvalidEnumEntity(InvalidEnumEntity { - uid, - choices: choices.into_iter().collect(), - }) + Self::InvalidEnumEntity( + InvalidEnumEntityError { + uid, + choices: choices.into_iter().collect(), + } + .into(), + ) } } @@ -298,10 +303,38 @@ impl Diagnostic for UnexpectedEntityTypeError { // Don't make fields `pub`, don't make breaking changes, and use caution // when adding public methods. #[derive(Debug, Error, Diagnostic)] -#[error("entity `{uid}` has invalid UID: `{}`", uid.eid().escaped())] +#[error(transparent)] +#[diagnostic(transparent)] pub struct InvalidEnumEntity { + err: InvalidEnumEntityError, +} + +impl From for InvalidEnumEntity { + fn from(value: InvalidEnumEntityError) -> Self { + Self { err: value } + } +} + +/// Returned when an entity is of an enumerated entity type but has invalid EID +#[derive(Debug, Error, Clone, PartialEq, Eq, Hash)] +#[error("entity `{uid}` is of an enumerated entity type, but `\"{}\"` is not declared as a valid eid", uid.eid().escaped())] +pub struct InvalidEnumEntityError { /// Entity where the error occurred - uid: EntityUID, + pub uid: EntityUID, /// Name of the attribute where the error occurred - choices: Vec, + pub choices: Vec, +} + +impl Diagnostic for InvalidEnumEntityError { + impl_diagnostic_from_method_on_field!(uid, loc); + + fn help<'a>(&'a self) -> Option> { + Some(Box::new(format!( + "valid entity eids: {}", + self.choices + .iter() + .map(|e| format!("\"{}\"", e.escaped())) + .join(", ") + ))) + } } diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index b9d3d7d28..fe2bcc74b 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -19,7 +19,7 @@ use super::{ CedarValueJson, EntityTypeDescription, EntityUidJson, NoEntitiesSchema, Schema, TypeAndId, ValueParser, }; -use crate::ast::{BorrowedRestrictedExpr, Eid, Entity, EntityUID, PartialValue, RestrictedExpr}; +use crate::ast::{BorrowedRestrictedExpr, Entity, EntityUID, PartialValue, RestrictedExpr}; use crate::entities::conformance::EntitySchemaConformanceChecker; use crate::entities::{ conformance::err::{EntitySchemaConformanceError, UnexpectedEntityTypeError}, @@ -302,12 +302,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { if let Some(choices) = desc.enum_enity_eids() { if !choices.contains(&uid.eid()) { return Err(JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::invalid_enum_entity( - uid.clone(), - choices - .iter() - .map(|e| >::as_ref(e).clone()), - ), + EntitySchemaConformanceError::invalid_enum_entity(uid.clone(), choices), )); } } diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 90b7ad7a5..3c5919953 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -15,11 +15,11 @@ */ use crate::{ValidatorActionId, ValidatorEntityType, ValidatorEntityTypeKind, ValidatorSchema}; use cedar_policy_core::ast::{Eid, EntityType, EntityUID}; +use cedar_policy_core::entities::conformance::err::InvalidEnumEntityError; use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use cedar_policy_core::{ast, entities}; use miette::Diagnostic; use nonempty::NonEmpty; -use request_validation_errors::InvalidEnumEntityError; use smol_str::SmolStr; use std::collections::hash_map::Values; use std::collections::HashSet; @@ -186,9 +186,9 @@ impl ast::RequestSchema for ValidatorSchema { } = et { if !choices.contains(euid.eid().as_ref()) { - return Err(request_validation_errors::InvalidEnumEntityError { - euid: euid.clone().into(), - choices: choices.clone(), + return Err(InvalidEnumEntityError { + uid: euid.clone(), + choices: choices.into_iter().map(|s| Eid::new(s.clone())).collect(), } .into()); } @@ -210,9 +210,9 @@ impl ast::RequestSchema for ValidatorSchema { } = et { if !choices.contains(euid.eid().as_ref()) { - return Err(request_validation_errors::InvalidEnumEntityError { - euid: euid.clone().into(), - choices: choices.clone(), + return Err(InvalidEnumEntityError { + uid: euid.clone(), + choices: choices.into_iter().map(|s| Eid::new(s.clone())).collect(), } .into()); } @@ -361,8 +361,6 @@ pub mod request_validation_errors { use cedar_policy_core::impl_diagnostic_from_method_on_field; use itertools::Itertools; use miette::Diagnostic; - use nonempty::NonEmpty; - use smol_str::SmolStr; use std::sync::Arc; use thiserror::Error; @@ -568,24 +566,6 @@ pub mod request_validation_errors { &self.action } } - - /// Request principal or resource is an invalid enumerated entity - #[derive(Debug, Error)] - #[error("entity `{euid}` is not declared as an enumerated entity but has invalid eid: `{}`", euid.eid().escaped())] - pub struct InvalidEnumEntityError { - /// problematic EUID - pub(super) euid: Arc, - /// valid entity EIDs - pub(super) choices: NonEmpty, - } - - impl Diagnostic for InvalidEnumEntityError { - impl_diagnostic_from_method_on_field!(euid, loc); - - fn help<'a>(&'a self) -> Option> { - Some(Box::new(format!("valid entity eids: {:?}", self.choices))) - } - } } /// Struct which carries enough information that it can impl Core's diff --git a/cedar-policy-validator/src/diagnostics.rs b/cedar-policy-validator/src/diagnostics.rs index dd9a59c54..6035e6827 100644 --- a/cedar-policy-validator/src/diagnostics.rs +++ b/cedar-policy-validator/src/diagnostics.rs @@ -17,6 +17,7 @@ //! This module contains the diagnostics (i.e., errors and warnings) that are //! returned by the validator. +use cedar_policy_core::entities::conformance::err::InvalidEnumEntityError; use miette::Diagnostic; use smol_str::SmolStr; use thiserror::Error; @@ -24,7 +25,7 @@ use validation_errors::UnrecognizedActionIdHelp; use std::collections::BTreeSet; -use cedar_policy_core::ast::{EntityType, EntityUID, Expr, PolicyID}; +use cedar_policy_core::ast::{Eid, EntityType, EntityUID, Expr, PolicyID}; use cedar_policy_core::parser::Loc; use crate::types::{EntityLUB, Type}; @@ -414,8 +415,10 @@ impl ValidationError { validation_errors::InvalidEnumEntity { source_loc, policy_id, - entity, - choices: choices.into_iter().collect(), + err: InvalidEnumEntityError { + uid: entity, + choices: choices.into_iter().map(|s| Eid::new(s)).collect(), + }, } .into() } diff --git a/cedar-policy-validator/src/diagnostics/validation_errors.rs b/cedar-policy-validator/src/diagnostics/validation_errors.rs index fdca37c7a..d82b79a38 100644 --- a/cedar-policy-validator/src/diagnostics/validation_errors.rs +++ b/cedar-policy-validator/src/diagnostics/validation_errors.rs @@ -16,6 +16,7 @@ //! Defines errors returned by the validator. +use cedar_policy_core::entities::conformance::err::InvalidEnumEntityError; use miette::Diagnostic; use thiserror::Error; @@ -720,23 +721,21 @@ impl Display for AttributeAccess { /// Returned when an entity literal is of an enumerated entity type but has /// undeclared UID #[derive(Debug, Clone, Error, Hash, Eq, PartialEq)] -#[error("for policy `{policy_id}`, entity `{entity}`'s UID is illegal")] +#[error("for policy `{policy_id}`: {err}")] pub struct InvalidEnumEntity { /// Source location pub source_loc: Option, /// Policy ID where the error occurred pub policy_id: PolicyID, - /// UID of the problematic entity - pub entity: EntityUID, - /// UID choices of the enumerated entity type - pub choices: Vec, + /// The error + pub err: InvalidEnumEntityError, } impl Diagnostic for InvalidEnumEntity { impl_diagnostic_from_source_loc_opt_field!(source_loc); fn help<'a>(&'a self) -> Option> { - Some(Box::new(format!("allowed UID values: {:?}", self.choices))) + self.err.help() } } diff --git a/cedar-policy/src/api/err.rs b/cedar-policy/src/api/err.rs index 30d45788b..598845f05 100644 --- a/cedar-policy/src/api/err.rs +++ b/cedar-policy/src/api/err.rs @@ -1250,7 +1250,7 @@ pub mod request_validation_errors { #[error(transparent)] #[diagnostic(transparent)] pub struct InvalidEnumEntityError( - #[from] cedar_policy_validator::request_validation_errors::InvalidEnumEntityError, + #[from] cedar_policy_core::entities::conformance::err::InvalidEnumEntityError, ); } From beb2946c4fb8402a7575187dd35645fa797837b8 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 14 Jan 2025 13:32:04 -0800 Subject: [PATCH 18/28] tests for request validation Signed-off-by: Shaobo He --- cedar-policy-validator/src/coreschema.rs | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 3c5919953..86a06399d 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -609,10 +609,26 @@ pub fn context_schema_for_action( #[cfg(test)] mod test { use super::*; + use ast::Context; use cedar_policy_core::test_utils::{expect_err, ExpectedErrorMessageBuilder}; use cool_asserts::assert_matches; use serde_json::json; + #[track_caller] + fn schema_with_enums() -> ValidatorSchema { + let src = r#" + entity Fruit enum ["🍉", "🍓", "🍒"]; + entity People; + action "eat" appliesTo { + principal: [People], + resource: [Fruit], + }; + "#; + ValidatorSchema::from_cedarschema_str(src, Extensions::none()) + .expect("should be a valid schema") + .0 + } + fn schema() -> ValidatorSchema { let src = json!( { "": { @@ -1085,4 +1101,46 @@ mod test { } ); } + + #[test] + fn enumerated_entity_type() { + assert_matches!( + ast::Request::new( + ( + ast::EntityUID::with_eid_and_type("People", "😋").unwrap(), + None + ), + ( + ast::EntityUID::with_eid_and_type("Action", "eat").unwrap(), + None + ), + ( + ast::EntityUID::with_eid_and_type("Fruit", "🍉").unwrap(), + None + ), + Context::empty(), + Some(&schema_with_enums()), + Extensions::none(), + ), + Ok(_) + ); + assert_matches!( + ast::Request::new( + (ast::EntityUID::with_eid_and_type("People", "🤔").unwrap(), None), + (ast::EntityUID::with_eid_and_type("Action", "eat").unwrap(), None), + (ast::EntityUID::with_eid_and_type("Fruit", "🥝").unwrap(), None), + Context::empty(), + Some(&schema_with_enums()), + Extensions::none(), + ), + Err(e) => { + expect_err( + "", + &miette::Report::new(e), + &ExpectedErrorMessageBuilder::error(r#"entity `Fruit::"🥝"` is of an enumerated entity type, but `"🥝"` is not declared as a valid eid"#).help(r#"valid entity eids: "🍉", "🍓", "🍒""#) + .build(), + ); + } + ); + } } From 698b4b691633a9edb66dfce18cfc4e31edc04312 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Tue, 14 Jan 2025 16:57:44 -0800 Subject: [PATCH 19/28] added more tests Signed-off-by: Shaobo He --- cedar-policy-core/src/entities.rs | 103 +++++++++++++++++++++++ cedar-policy/src/tests.rs | 133 ++++++++++++++++++++++++++++++ 2 files changed, 236 insertions(+) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index e503520ab..09bd157a1 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -3533,6 +3533,109 @@ mod schema_based_parsing_tests { ); }); } + + #[test] + fn enumerated_entities() { + struct MockSchema; + struct StarTypeDescription; + impl EntityTypeDescription for StarTypeDescription { + fn entity_type(&self) -> EntityType { + "Star".parse().unwrap() + } + + fn attr_type(&self, _attr: &str) -> Option { + None + } + + fn tag_type(&self) -> Option { + None + } + + fn required_attrs<'s>(&'s self) -> Box + 's> { + Box::new(std::iter::empty()) + } + + fn allowed_parent_types(&self) -> Arc> { + Arc::new(HashSet::new()) + } + + fn open_attributes(&self) -> bool { + false + } + + fn enum_enity_eids(&self) -> Option> { + Some(nonempty::nonempty![Eid::new("🌎"), Eid::new("🌕"),]) + } + } + impl Schema for MockSchema { + type EntityTypeDescription = StarTypeDescription; + + type ActionEntityIterator = std::iter::Empty>; + + fn entity_type(&self, entity_type: &EntityType) -> Option { + if entity_type == &"Star".parse::().unwrap() { + Some(StarTypeDescription) + } else { + None + } + } + + fn action(&self, _action: &EntityUID) -> Option> { + None + } + + fn entity_types_with_basename<'a>( + &'a self, + basename: &'a UnreservedId, + ) -> Box + 'a> { + if basename == &"Star".parse::().unwrap() { + Box::new(std::iter::once("Star".parse::().unwrap())) + } else { + Box::new(std::iter::empty()) + } + } + + fn action_entities(&self) -> Self::ActionEntityIterator { + std::iter::empty() + } + } + + let eparser = EntityJsonParser::new( + Some(&MockSchema), + Extensions::none(), + TCComputation::ComputeNow, + ); + + assert_matches!( + eparser.from_json_value(serde_json::json!([ + { + "uid": { "type": "Star", "id": "🌎" }, + "attrs": {}, + "parents": [], + } + ])), + Ok(_) + ); + + let entitiesjson = serde_json::json!([ + { + "uid": { "type": "Star", "id": "🪐" }, + "attrs": {}, + "parents": [], + } + ]); + assert_matches!(eparser.from_json_value(entitiesjson.clone()), + Err(e) => { + expect_err( + &entitiesjson, + &miette::Report::new(e), + &ExpectedErrorMessageBuilder::error("error during entity deserialization") + .source(r#"entity `Star::"🪐"` is of an enumerated entity type, but `"🪐"` is not declared as a valid eid"#) + .help(r#"valid entity eids: "🌎", "🌕""#) + .build() + ); + }); + } } #[cfg(feature = "protobufs")] diff --git a/cedar-policy/src/tests.rs b/cedar-policy/src/tests.rs index e28920026..abf290a06 100644 --- a/cedar-policy/src/tests.rs +++ b/cedar-policy/src/tests.rs @@ -3825,6 +3825,139 @@ mod schema_based_parsing_tests { Err(EntitiesError::TransitiveClosureError(_)) )); } + + #[test] + fn enumerated_entity_types() { + let schema = Schema::from_str( + r#" + entity Fruit enum ["🍉", "🍓", "🍒"]; + entity People; + entity DeliciousFruit in Fruit; + action "eat" appliesTo { + principal: [People], + resource: [Fruit], + }; + "#, + ) + .expect("should be a valid schema"); + // invalid eid + let json = serde_json::json!([ + { + "uid" : { + "type" : "Fruit", + "id" : "🥝" + }, + "attrs" : {}, + "parents": [] + }, + { + "uid" : { + "type" : "People", + "id" : "😋" + }, + "attrs" : {}, + "parents": [] + } + ]); + assert_matches!(Entities::from_json_value(json.clone(), Some(&schema)), Err(EntitiesError::Deserialization(err)) => { + expect_err( + &json, + &Report::new(err), + &ExpectedErrorMessageBuilder::error( + r#"entity `Fruit::"🥝"` is of an enumerated entity type, but `"🥝"` is not declared as a valid eid"#, + ) + .help(r#"valid entity eids: "🍉", "🍓", "🍒""#) + .build(), + ); + }); + // no attributes are allowed + let json = serde_json::json!([ + { + "uid" : { + "type" : "Fruit", + "id" : "🍉" + }, + "attrs" : { + "sweetness": "high", + }, + "parents": [] + }, + { + "uid" : { + "type" : "People", + "id" : "😋" + }, + "attrs" : {}, + "parents": [] + } + ]); + assert_matches!(Entities::from_json_value(json.clone(), Some(&schema)), Err(EntitiesError::Deserialization(err)) => { + expect_err( + &json, + &Report::new(err), + &ExpectedErrorMessageBuilder::error( + r#"attribute `sweetness` on `Fruit::"🍉"` should not exist according to the schema"#, + ) + .build(), + ); + }); + // no parents are allowed + let json = serde_json::json!([ + { + "uid" : { + "type" : "Fruit", + "id" : "🍉" + }, + "attrs" : { + }, + "parents": [{"type": "Fruit", "id": "🍓"}] + }, + { + "uid" : { + "type" : "People", + "id" : "😋" + }, + "attrs" : {}, + "parents": [] + } + ]); + assert_matches!(Entities::from_json_value(json.clone(), Some(&schema)), Err(EntitiesError::InvalidEntity(err)) => { + expect_err( + &json, + &Report::new(err), + &ExpectedErrorMessageBuilder::error( + r#"`Fruit::"🍉"` is not allowed to have an ancestor of type `Fruit` according to the schema"#, + ) + .build(), + ); + }); + + // Reference to invalid eid + // TODO: fix this + let json = serde_json::json!([ + { + "uid" : { + "type" : "DeliciousFruit", + "id" : "🍉" + }, + "attrs" : { + }, + "parents": [{"type": "Fruit", "id": "🥝"}] + }, + { + "uid" : { + "type" : "People", + "id" : "😋" + }, + "attrs" : {}, + "parents": [] + } + ]); + assert_matches!( + Entities::from_json_value(json.clone(), Some(&schema)), + Ok(_) + ); + } } #[cfg(not(feature = "partial-validate"))] From bdef431586ce0148fc69557f57ceca70a83e74d8 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 13:38:21 -0800 Subject: [PATCH 20/28] better entity validation Signed-off-by: Shaobo He --- cedar-policy-core/src/entities/conformance.rs | 52 ++++++- .../src/entities/conformance/err.rs | 13 -- .../src/entities/json/entities.rs | 12 -- cedar-policy-validator/src/coreschema.rs | 6 + cedar-policy/src/tests.rs | 128 +++++++++++++++++- 5 files changed, 179 insertions(+), 32 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index 528390ca7..14a2ded7e 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -17,6 +17,7 @@ use std::collections::BTreeMap; use super::{json::err::TypeMismatchError, EntityTypeDescription, Schema, SchemaType}; +use super::{EntityUID, Literal}; use crate::ast::{ BorrowedRestrictedExpr, Entity, PartialValue, PartialValueToRestrictedExprError, RestrictedExpr, }; @@ -27,7 +28,7 @@ use smol_str::SmolStr; use thiserror::Error; pub mod err; -use err::{EntitySchemaConformanceError, UnexpectedEntityTypeError}; +use err::{EntitySchemaConformanceError, InvalidEnumEntityError, UnexpectedEntityTypeError}; /// Struct used to check whether entities conform to a schema #[derive(Debug, Clone)] @@ -61,6 +62,8 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { )); } } else { + validate_euid(self.schema, uid) + .map_err(|e| EntitySchemaConformanceError::InvalidEnumEntity(e.into()))?; let schema_etype = self.schema.entity_type(etype).ok_or_else(|| { let suggested_types = self .schema @@ -120,10 +123,14 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { } } } + validate_euids_in_partial_value(self.schema, val) + .map_err(|e| EntitySchemaConformanceError::InvalidEnumEntity(e.into()))?; } // For each ancestor that actually appears in `entity`, ensure the // ancestor type is allowed by the schema for ancestor_euid in entity.ancestors() { + validate_euid(self.schema, ancestor_euid) + .map_err(|e| EntitySchemaConformanceError::InvalidEnumEntity(e.into()))?; let ancestor_type = ancestor_euid.entity_type(); if schema_etype.allowed_parent_types().contains(ancestor_type) { // note that `allowed_parent_types()` was transitively @@ -137,11 +144,54 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { )); } } + + for (_, val) in entity.tags() { + validate_euids_in_partial_value(self.schema, val) + .map_err(|e| EntitySchemaConformanceError::InvalidEnumEntity(e.into()))?; + } } Ok(()) } } +/// Validate if `euid` is valid, provided that it's of an enumerated type +pub(crate) fn validate_euid( + schema: &impl Schema, + euid: &EntityUID, +) -> Result<(), InvalidEnumEntityError> { + if let Some(desc) = schema.entity_type(euid.entity_type()) { + if let Some(choices) = desc.enum_enity_eids() { + if !choices.contains(euid.eid()) { + return Err(InvalidEnumEntityError { + uid: euid.clone(), + choices: choices.into(), + }); + } + } + } + Ok(()) +} + +/// Validate if enumerated entities in `val` are valid +pub fn validate_euids_in_partial_value( + schema: &impl Schema, + val: &PartialValue, +) -> Result<(), InvalidEnumEntityError> { + match val { + PartialValue::Value(val) => { + for e in RestrictedExpr::from(val.clone()).subexpressions() { + match e.expr_kind() { + ExprKind::Lit(Literal::EntityUID(euid)) => validate_euid(schema, &euid)?, + _ => {} + } + } + } + _ => {} + }; + + Ok(()) +} + /// Check whether the given `PartialValue` typechecks with the given `SchemaType`. /// If the typecheck passes, return `Ok(())`. /// If the typecheck fails, return an appropriate `Err`. diff --git a/cedar-policy-core/src/entities/conformance/err.rs b/cedar-policy-core/src/entities/conformance/err.rs index 3216a5483..cea1eff5c 100644 --- a/cedar-policy-core/src/entities/conformance/err.rs +++ b/cedar-policy-core/src/entities/conformance/err.rs @@ -138,19 +138,6 @@ impl EntitySchemaConformanceError { err, }) } - - pub(crate) fn invalid_enum_entity( - uid: EntityUID, - choices: impl IntoIterator, - ) -> Self { - Self::InvalidEnumEntity( - InvalidEnumEntityError { - uid, - choices: choices.into_iter().collect(), - } - .into(), - ) - } } /// Error looking up an extension function. This error can occur when diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index fe2bcc74b..b555176b7 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -297,18 +297,6 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { } } }; - match &entity_schema_info { - EntitySchemaInfo::NonAction(desc) => { - if let Some(choices) = desc.enum_enity_eids() { - if !choices.contains(&uid.eid()) { - return Err(JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::invalid_enum_entity(uid.clone(), choices), - )); - } - } - } - _ => {} - }; let vparser = ValueParser::new(self.extensions); let attrs: HashMap = ejson .attrs diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 86a06399d..cd2a78e79 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -16,6 +16,7 @@ use crate::{ValidatorActionId, ValidatorEntityType, ValidatorEntityTypeKind, ValidatorSchema}; use cedar_policy_core::ast::{Eid, EntityType, EntityUID}; use cedar_policy_core::entities::conformance::err::InvalidEnumEntityError; +use cedar_policy_core::entities::conformance::validate_euids_in_partial_value; use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use cedar_policy_core::{ast, entities}; use miette::Diagnostic; @@ -241,6 +242,11 @@ impl ast::RequestSchema for ValidatorSchema { validator_action_id.check_resource_type(principal_type, action)?; } if let Some(context) = request.context() { + validate_euids_in_partial_value( + &CoreSchema::new(&self), + &context.clone().into(), + ) + .map_err(|err| RequestValidationError::InvalidEnumEntity(err))?; let expected_context_ty = validator_action_id.context_type(); if !expected_context_ty .typecheck_partial_value(&context.clone().into(), extensions) diff --git a/cedar-policy/src/tests.rs b/cedar-policy/src/tests.rs index abf290a06..fd2e49420 100644 --- a/cedar-policy/src/tests.rs +++ b/cedar-policy/src/tests.rs @@ -3831,8 +3831,11 @@ mod schema_based_parsing_tests { let schema = Schema::from_str( r#" entity Fruit enum ["🍉", "🍓", "🍒"]; - entity People; - entity DeliciousFruit in Fruit; + entity People { + fruit?: Fruit, + fruit_rec?: {name: Fruit}, + }; + entity DeliciousFruit in Fruit tags Fruit; action "eat" appliesTo { principal: [People], resource: [Fruit], @@ -3859,7 +3862,7 @@ mod schema_based_parsing_tests { "parents": [] } ]); - assert_matches!(Entities::from_json_value(json.clone(), Some(&schema)), Err(EntitiesError::Deserialization(err)) => { + assert_matches!(Entities::from_json_value(json.clone(), Some(&schema)), Err(EntitiesError::InvalidEntity(err)) => { expect_err( &json, &Report::new(err), @@ -3932,8 +3935,7 @@ mod schema_based_parsing_tests { ); }); - // Reference to invalid eid - // TODO: fix this + // Reference to invalid eid in the `parents` field let json = serde_json::json!([ { "uid" : { @@ -3955,7 +3957,121 @@ mod schema_based_parsing_tests { ]); assert_matches!( Entities::from_json_value(json.clone(), Some(&schema)), - Ok(_) + Err(EntitiesError::InvalidEntity(err)) => { + expect_err( + &json, + &Report::new(err), + &ExpectedErrorMessageBuilder::error( + r#"entity `Fruit::"🥝"` is of an enumerated entity type, but `"🥝"` is not declared as a valid eid"#, + ).help(r#"valid entity eids: "🍉", "🍓", "🍒""#) + .build(), + );} + ); + + // Reference to invalid eid in the `attrs` field + let json = serde_json::json!([ + { + "uid" : { + "type" : "DeliciousFruit", + "id" : "🍍" + }, + "attrs" : { + }, + "parents": [{"type": "Fruit", "id": "🍉"}] + }, + { + "uid" : { + "type" : "People", + "id" : "😋" + }, + "attrs" : { + "fruit": {"type": "Fruit", "id": "🍍"}, + }, + "parents": [] + } + ]); + assert_matches!( + Entities::from_json_value(json.clone(), Some(&schema)), + Err(EntitiesError::InvalidEntity(err)) => { + expect_err( + &json, + &Report::new(err), + &ExpectedErrorMessageBuilder::error( + r#"entity `Fruit::"🍍"` is of an enumerated entity type, but `"🍍"` is not declared as a valid eid"#, + ).help(r#"valid entity eids: "🍉", "🍓", "🍒""#) + .build(), + );} + ); + // Reference to invalid eid in the `attrs` field + let json = serde_json::json!([ + { + "uid" : { + "type" : "DeliciousFruit", + "id" : "🍍" + }, + "attrs" : { + }, + "parents": [{"type": "Fruit", "id": "🍉"}] + }, + { + "uid" : { + "type" : "People", + "id" : "😋" + }, + "attrs" : { + "fruit_rec": {"name": {"type": "Fruit", "id": "🥭"}}, + }, + "parents": [] + } + ]); + assert_matches!( + Entities::from_json_value(json.clone(), Some(&schema)), + Err(EntitiesError::InvalidEntity(err)) => { + expect_err( + &json, + &Report::new(err), + &ExpectedErrorMessageBuilder::error( + r#"entity `Fruit::"🥭"` is of an enumerated entity type, but `"🥭"` is not declared as a valid eid"#, + ).help(r#"valid entity eids: "🍉", "🍓", "🍒""#) + .build(), + );} + ); + // Reference to invalid eid in the `tags` field + let json = serde_json::json!([ + { + "uid" : { + "type" : "DeliciousFruit", + "id" : "🍍" + }, + "attrs" : { + }, + "parents": [{"type": "Fruit", "id": "🍉"}], + "tags": { + "mango": {"type": "Fruit", "id": "🥭"}, + } + }, + { + "uid" : { + "type" : "People", + "id" : "😋" + }, + "attrs" : { + "fruit_rec": {"name": {"type": "Fruit", "id": "🍉"}}, + }, + "parents": [] + } + ]); + assert_matches!( + Entities::from_json_value(json.clone(), Some(&schema)), + Err(EntitiesError::InvalidEntity(err)) => { + expect_err( + &json, + &Report::new(err), + &ExpectedErrorMessageBuilder::error( + r#"entity `Fruit::"🥭"` is of an enumerated entity type, but `"🥭"` is not declared as a valid eid"#, + ).help(r#"valid entity eids: "🍉", "🍓", "🍒""#) + .build(), + );} ); } } From f8b592df5143ea9429aae79b2b5871ba6f6e5513 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 13:48:11 -0800 Subject: [PATCH 21/28] updated tests Signed-off-by: Shaobo He --- cedar-policy-validator/src/coreschema.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index cd2a78e79..5981235a2 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -615,7 +615,7 @@ pub fn context_schema_for_action( #[cfg(test)] mod test { use super::*; - use ast::Context; + use ast::{Context, Value}; use cedar_policy_core::test_utils::{expect_err, ExpectedErrorMessageBuilder}; use cool_asserts::assert_matches; use serde_json::json; @@ -628,6 +628,9 @@ mod test { action "eat" appliesTo { principal: [People], resource: [Fruit], + context: { + fruit?: Fruit, + } }; "#; ValidatorSchema::from_cedarschema_str(src, Extensions::none()) @@ -1148,5 +1151,23 @@ mod test { ); } ); + assert_matches!( + ast::Request::new( + (ast::EntityUID::with_eid_and_type("People", "🤔").unwrap(), None), + (ast::EntityUID::with_eid_and_type("Action", "eat").unwrap(), None), + (ast::EntityUID::with_eid_and_type("Fruit", "🍉").unwrap(), None), + Context::from_pairs(std::iter::once(("fruit".into(), (Value::from(ast::EntityUID::with_eid_and_type("Fruit", "🥭").unwrap())).into())), Extensions::none()).expect("should be a valid context"), + Some(&schema_with_enums()), + Extensions::none(), + ), + Err(e) => { + expect_err( + "", + &miette::Report::new(e), + &ExpectedErrorMessageBuilder::error(r#"entity `Fruit::"🥭"` is of an enumerated entity type, but `"🥭"` is not declared as a valid eid"#).help(r#"valid entity eids: "🍉", "🍓", "🍒""#) + .build(), + ); + } + ); } } From 4c01686f461b4669cdd41afe8adc1d521720f67c Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 13:58:01 -0800 Subject: [PATCH 22/28] fix Signed-off-by: Shaobo He --- cedar-policy-core/src/entities.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 09bd157a1..708d91b6c 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -3629,7 +3629,7 @@ mod schema_based_parsing_tests { expect_err( &entitiesjson, &miette::Report::new(e), - &ExpectedErrorMessageBuilder::error("error during entity deserialization") + &ExpectedErrorMessageBuilder::error("entity does not conform to the schema") .source(r#"entity `Star::"🪐"` is of an enumerated entity type, but `"🪐"` is not declared as a valid eid"#) .help(r#"valid entity eids: "🌎", "🌕""#) .build() From 0c65e1eb8ce695e2ae32191d0766eb2c17b0745b Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 14:21:35 -0800 Subject: [PATCH 23/28] refactor Signed-off-by: Shaobo He --- cedar-policy-core/src/entities/conformance.rs | 24 +++++++++++++------ cedar-policy-validator/src/coreschema.rs | 22 ++++++----------- cedar-policy-validator/src/diagnostics.rs | 11 +++------ cedar-policy-validator/src/rbac.rs | 23 ++++++++++-------- 4 files changed, 40 insertions(+), 40 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index 14a2ded7e..f2aa3994d 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; use super::{json::err::TypeMismatchError, EntityTypeDescription, Schema, SchemaType}; -use super::{EntityUID, Literal}; +use super::{Eid, EntityUID, Literal}; use crate::ast::{ BorrowedRestrictedExpr, Entity, PartialValue, PartialValueToRestrictedExprError, RestrictedExpr, }; @@ -154,6 +154,21 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { } } +/// Return an [`InvalidEnumEntityError`] if `uid`'s eid is not among valid `choices` +pub fn is_valid_enumerated_entity( + choices: impl IntoIterator, + uid: &EntityUID, +) -> Result<(), InvalidEnumEntityError> { + let choices: Vec<_> = choices.into_iter().collect(); + if choices.contains(uid.eid()) { + Ok(()) + } else { + Err(InvalidEnumEntityError { + uid: uid.clone(), + choices, + }) + } +} /// Validate if `euid` is valid, provided that it's of an enumerated type pub(crate) fn validate_euid( schema: &impl Schema, @@ -161,12 +176,7 @@ pub(crate) fn validate_euid( ) -> Result<(), InvalidEnumEntityError> { if let Some(desc) = schema.entity_type(euid.entity_type()) { if let Some(choices) = desc.enum_enity_eids() { - if !choices.contains(euid.eid()) { - return Err(InvalidEnumEntityError { - uid: euid.clone(), - choices: choices.into(), - }); - } + is_valid_enumerated_entity(choices, euid)?; } } Ok(()) diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 5981235a2..693ed29ae 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -16,7 +16,9 @@ use crate::{ValidatorActionId, ValidatorEntityType, ValidatorEntityTypeKind, ValidatorSchema}; use cedar_policy_core::ast::{Eid, EntityType, EntityUID}; use cedar_policy_core::entities::conformance::err::InvalidEnumEntityError; -use cedar_policy_core::entities::conformance::validate_euids_in_partial_value; +use cedar_policy_core::entities::conformance::{ + is_valid_enumerated_entity, validate_euids_in_partial_value, +}; use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use cedar_policy_core::{ast, entities}; use miette::Diagnostic; @@ -186,13 +188,8 @@ impl ast::RequestSchema for ValidatorSchema { .. } = et { - if !choices.contains(euid.eid().as_ref()) { - return Err(InvalidEnumEntityError { - uid: euid.clone(), - choices: choices.into_iter().map(|s| Eid::new(s.clone())).collect(), - } - .into()); - } + is_valid_enumerated_entity(choices.clone().map(Eid::new), euid) + .map_err(Self::Error::from)?; } } } else { @@ -210,13 +207,8 @@ impl ast::RequestSchema for ValidatorSchema { .. } = et { - if !choices.contains(euid.eid().as_ref()) { - return Err(InvalidEnumEntityError { - uid: euid.clone(), - choices: choices.into_iter().map(|s| Eid::new(s.clone())).collect(), - } - .into()); - } + is_valid_enumerated_entity(choices.clone().map(Eid::new), euid) + .map_err(Self::Error::from)?; } } } else { diff --git a/cedar-policy-validator/src/diagnostics.rs b/cedar-policy-validator/src/diagnostics.rs index 6035e6827..21e8b09b3 100644 --- a/cedar-policy-validator/src/diagnostics.rs +++ b/cedar-policy-validator/src/diagnostics.rs @@ -19,13 +19,12 @@ use cedar_policy_core::entities::conformance::err::InvalidEnumEntityError; use miette::Diagnostic; -use smol_str::SmolStr; use thiserror::Error; use validation_errors::UnrecognizedActionIdHelp; use std::collections::BTreeSet; -use cedar_policy_core::ast::{Eid, EntityType, EntityUID, Expr, PolicyID}; +use cedar_policy_core::ast::{EntityType, Expr, PolicyID}; use cedar_policy_core::parser::Loc; use crate::types::{EntityLUB, Type}; @@ -409,16 +408,12 @@ impl ValidationError { pub(crate) fn invalid_enum_entity( source_loc: Option, policy_id: PolicyID, - entity: EntityUID, - choices: impl IntoIterator, + err: InvalidEnumEntityError, ) -> Self { validation_errors::InvalidEnumEntity { source_loc, policy_id, - err: InvalidEnumEntityError { - uid: entity, - choices: choices.into_iter().map(|s| Eid::new(s)).collect(), - }, + err, } .into() } diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index aeb4c885d..5e043044c 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -18,9 +18,10 @@ use cedar_policy_core::{ ast::{ - self, ActionConstraint, EntityReference, EntityUID, Policy, PolicyID, PrincipalConstraint, - PrincipalOrResourceConstraint, ResourceConstraint, SlotEnv, Template, + self, ActionConstraint, Eid, EntityReference, EntityUID, Policy, PolicyID, + PrincipalConstraint, PrincipalOrResourceConstraint, ResourceConstraint, SlotEnv, Template, }, + entities::conformance::is_valid_enumerated_entity, fuzzy_match::fuzzy_search, parser::Loc, }; @@ -50,14 +51,16 @@ impl Validator { .. }) = self.schema.get_entity_type(e.entity_type()) { - if !choices.contains(e.eid().as_ref()) { - return Some(ValidationError::invalid_enum_entity( - e.loc().cloned(), - template.id().clone(), - e.clone(), - choices.into_iter().cloned(), - )); - } + match is_valid_enumerated_entity(choices.clone().map(Eid::new), e) { + Ok(_) => {} + Err(err) => { + return Some(ValidationError::invalid_enum_entity( + e.loc().cloned(), + template.id().clone(), + err, + )); + } + }; } None }) From 97684b8595094afd25e1d19267d8809a52a037cf Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 16:33:45 -0800 Subject: [PATCH 24/28] move tests around Signed-off-by: Shaobo He --- cedar-policy-validator/src/lib.rs | 219 ++++++++++++++++++ .../src/typecheck/test/policy.rs | 94 -------- 2 files changed, 219 insertions(+), 94 deletions(-) diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index a3c2a1147..eae1a0a15 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -575,3 +575,222 @@ mod test { ); } } + +#[cfg(test)] +mod enumerated_entity_types { + use cedar_policy_core::{ + ast::{Eid, EntityUID, ExprBuilder, PolicyID}, + expr_builder::ExprBuilder as _, + extensions::Extensions, + parser::parse_policy_or_template, + }; + use cool_asserts::assert_matches; + use itertools::Itertools; + + use crate::{ + typecheck::test::test_utils::get_loc, + types::{EntityLUB, Type}, + validation_errors::AttributeAccess, + ValidationError, Validator, ValidatorSchema, + }; + + #[track_caller] + fn schema() -> ValidatorSchema { + ValidatorSchema::from_json_value( + serde_json::json!( + { + "": { "entityTypes": { + "Foo": { + "enum": [ "foo" ], + }, + "Bar": { + "memberOfTypes": ["Foo"], + } + }, + "actions": { + "a": { + "appliesTo": { + "principalTypes": ["Foo"], + "resourceTypes": ["Bar"], + } + } + } + } + } + ), + Extensions::none(), + ) + .unwrap() + } + + #[test] + fn basic() { + let schema = schema(); + let template = parse_policy_or_template(None, r#"permit(principal, action == Action::"a", resource) when { principal == Foo::"foo" };"#).unwrap(); + let validator = Validator::new(schema); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert!(errors.collect_vec().is_empty()); + } + + #[test] + fn basic_invalid() { + let schema = schema(); + let template = parse_policy_or_template(None, r#"permit(principal, action == Action::"a", resource) when { principal == Foo::"fo" };"#).unwrap(); + let validator = Validator::new(schema.clone()); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => { + assert_eq!(err.err.choices, vec![Eid::new("foo")]); + assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "fo").unwrap()); + }); + + let template = parse_policy_or_template( + None, + r#"permit(principal == Foo::"🏈", action == Action::"a", resource);"#, + ) + .unwrap(); + let validator = Validator::new(schema.clone()); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => { + assert_eq!(err.err.choices, vec![Eid::new("foo")]); + assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap()); + }); + + let template = parse_policy_or_template( + None, + r#"permit(principal in Foo::"🏈", action == Action::"a", resource);"#, + ) + .unwrap(); + let validator = Validator::new(schema.clone()); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => { + assert_eq!(err.err.choices, vec![Eid::new("foo")]); + assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap()); + }); + + let template = parse_policy_or_template( + None, + r#"permit(principal, action == Action::"a", resource) + when { {"🏈": Foo::"🏈"} has "🏈" }; + "#, + ) + .unwrap(); + let validator = Validator::new(schema.clone()); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => { + assert_eq!(err.err.choices, vec![Eid::new("foo")]); + assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap()); + }); + + let template = parse_policy_or_template( + None, + r#"permit(principal, action == Action::"a", resource) + when { [Foo::"🏈"].isEmpty() }; + "#, + ) + .unwrap(); + let validator = Validator::new(schema.clone()); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => { + assert_eq!(err.err.choices, vec![Eid::new("foo")]); + assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap()); + }); + + let template = parse_policy_or_template( + None, + r#"permit(principal, action == Action::"a", resource) + when { [{"🏈": Foo::"🏈"}].isEmpty() }; + "#, + ) + .unwrap(); + let validator = Validator::new(schema.clone()); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => { + assert_eq!(err.err.choices, vec![Eid::new("foo")]); + assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap()); + }); + } + + #[test] + fn no_attrs_allowed() { + let schema = schema(); + let src = r#"permit(principal, action == Action::"a", resource) when { principal.foo == "foo" };"#; + let template = parse_policy_or_template(None, src).unwrap(); + let validator = Validator::new(schema); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_eq!( + errors.collect_vec(), + [ValidationError::unsafe_attribute_access( + get_loc(src, "principal.foo"), + PolicyID::from_string("policy0"), + AttributeAccess::EntityLUB( + EntityLUB::single_entity("Foo".parse().unwrap()), + vec!["foo".into()], + ), + None, + false, + )] + ); + } + + #[test] + fn no_ancestors() { + let schema = schema(); + let src = + r#"permit(principal, action == Action::"a", resource) when { principal in resource };"#; + let template = parse_policy_or_template(None, src).unwrap(); + let validator = Validator::new(schema); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_eq!( + errors.collect_vec(), + [ValidationError::hierarchy_not_respected( + get_loc(src, "principal in resource"), + PolicyID::from_string("policy0"), + Some("Foo".parse().unwrap()), + Some("Bar".parse().unwrap()), + )] + ); + } + + #[test] + fn no_tags_allowed() { + let schema = schema(); + let src = r#"permit(principal, action == Action::"a", resource) when { principal.getTag("foo") == "foo" };"#; + let template = parse_policy_or_template(None, src).unwrap(); + let validator = Validator::new(schema); + let (errors, warnings) = + validator.validate_policy(&template, crate::ValidationMode::Strict); + assert!(warnings.collect_vec().is_empty()); + assert_eq!( + errors.collect_vec(), + [ValidationError::unsafe_tag_access( + get_loc(src, r#"principal.getTag("foo")"#), + PolicyID::from_string("policy0"), + Some(EntityLUB::single_entity("Foo".parse().unwrap()),), + { + let builder = ExprBuilder::new(); + let mut expr = builder.val("foo"); + expr.set_data(Some(Type::primitive_string())); + expr + }, + )] + ); + } +} diff --git a/cedar-policy-validator/src/typecheck/test/policy.rs b/cedar-policy-validator/src/typecheck/test/policy.rs index 5b2fd1290..3719be271 100644 --- a/cedar-policy-validator/src/typecheck/test/policy.rs +++ b/cedar-policy-validator/src/typecheck/test/policy.rs @@ -1304,97 +1304,3 @@ mod templates { ); } } - -mod enumerated_entity_types { - use cedar_policy_core::{ast::PolicyID, parser::parse_policy_or_template}; - - use crate::{ - json_schema, typecheck::test::test_utils::get_loc, types::EntityLUB, - validation_errors::AttributeAccess, RawName, ValidationError, - }; - - use super::{ - assert_exactly_one_diagnostic, assert_policy_typecheck_fails, assert_policy_typechecks, - }; - - #[track_caller] - fn schema() -> json_schema::NamespaceDefinition { - serde_json::from_value(serde_json::json!( - { - "entityTypes": { - "Foo": { - "enum": [ "foo" ], - }, - "Bar": { - "memberOfTypes": ["Foo"], - } - }, - "actions": { - "a": { - "appliesTo": { - "principalTypes": ["Foo"], - "resourceTypes": ["Bar"], - } - } - } - } - )) - .unwrap() - } - - #[test] - fn basic() { - let schema = schema(); - let template = parse_policy_or_template(None, r#"permit(principal, action == Action::"a", resource) when { principal == Foo::"foo" };"#).unwrap(); - assert_policy_typechecks(schema, template); - } - - //TODO: should type checker reject it as well? - #[test] - fn basic_invalid() { - let schema = schema(); - let template = parse_policy_or_template(None, r#"permit(principal, action == Action::"a", resource) when { principal == Foo::"fo" };"#).unwrap(); - assert_policy_typechecks(schema, template); - } - - #[test] - fn no_attrs_allowed() { - let schema = schema(); - let src = r#"permit(principal, action == Action::"a", resource) when { principal.foo == "foo" };"#; - let template = parse_policy_or_template(None, src).unwrap(); - let errs = assert_policy_typecheck_fails(schema, template); - let err = assert_exactly_one_diagnostic(errs); - assert_eq!( - err, - ValidationError::unsafe_attribute_access( - get_loc(src, "principal.foo"), - PolicyID::from_string("policy0"), - AttributeAccess::EntityLUB( - EntityLUB::single_entity("Foo".parse().unwrap()), - vec!["foo".into()], - ), - None, - false, - ) - ); - } - - #[test] - fn no_ancestors() { - let schema = schema(); - let src = - r#"permit(principal, action == Action::"a", resource) when { principal in resource };"#; - let template = parse_policy_or_template(None, src).unwrap(); - let errs = assert_policy_typecheck_fails(schema, template); - let err = assert_exactly_one_diagnostic(errs); - assert_eq!( - err, - ValidationError::hierarchy_not_respected( - get_loc(src, "principal in resource"), - PolicyID::from_string("policy0"), - Some("Foo".parse().unwrap()), - Some("Bar".parse().unwrap()), - ) - ); - } -} From 1666d0f491f4def262c9378dfb729fc68927d342 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 16:47:03 -0800 Subject: [PATCH 25/28] fix wasm? Signed-off-by: Shaobo He --- cedar-policy-validator/src/json_schema.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index ca8f55dc3..ff9d35e25 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -577,20 +577,24 @@ impl<'de, N: Deserialize<'de> + From> Deserialize<'de> for EntityType { - /// Entities of this [`EntityType`] are allowed to be members of entities of + /// Entities of this [`StandardEntityType`] are allowed to be members of entities of /// these types. #[serde(skip_serializing_if = "Vec::is_empty")] + #[serde(default)] pub member_of_types: Vec, - /// Description of the attributes for entities of this [`EntityType`]. + /// Description of the attributes for entities of this [`StandardEntityType`]. #[serde(skip_serializing_if = "AttributesOrContext::is_empty_record")] + #[serde(default)] pub shape: AttributesOrContext, - /// Tag type for entities of this [`EntityType`]; `None` means entities of this [`EntityType`] do not have tags. + /// Tag type for entities of this [`StandardEntityType`]; `None` means entities of this [`StandardEntityType`] do not have tags. #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] pub tags: Option>, } From 05574690d7df45d9af6ea3825252ecac8ad4f21d Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 17:02:29 -0800 Subject: [PATCH 26/28] clean up Signed-off-by: Shaobo He --- cedar-policy-validator/src/coreschema.rs | 1 + .../src/entity_manifest/type_annotations.rs | 1 + cedar-policy-validator/src/schema.rs | 48 +++---------------- .../src/schema/entity_type.rs | 14 ++++-- cedar-policy-validator/src/types.rs | 8 +--- 5 files changed, 20 insertions(+), 52 deletions(-) diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 693ed29ae..4a4ebdb28 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -156,6 +156,7 @@ impl entities::EntityTypeDescription for EntityTypeDescription { Box::new( self.validator_type .attributes() + .into_iter() .filter(|(_, ty)| ty.is_required) .map(|(attr, _)| attr.clone()), ) diff --git a/cedar-policy-validator/src/entity_manifest/type_annotations.rs b/cedar-policy-validator/src/entity_manifest/type_annotations.rs index 84403a789..18039ff1b 100644 --- a/cedar-policy-validator/src/entity_manifest/type_annotations.rs +++ b/cedar-policy-validator/src/entity_manifest/type_annotations.rs @@ -148,6 +148,7 @@ impl AccessTrie { &Attributes::with_required_attributes( entity_ty .attributes() + .into_iter() .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())), ) } diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index c1b0e4fc3..355472be2 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -2374,16 +2374,7 @@ pub(crate) mod test { .unwrap(); let schema: ValidatorSchema = fragment.try_into().unwrap(); assert_eq!( - Attributes::with_required_attributes( - schema - .entity_types - .iter() - .next() - .unwrap() - .1 - .attributes() - .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) - ), + schema.entity_types.iter().next().unwrap().1.attributes(), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -2409,16 +2400,7 @@ pub(crate) mod test { .unwrap(); let schema: ValidatorSchema = fragment.try_into().unwrap(); assert_eq!( - Attributes::with_required_attributes( - schema - .entity_types - .iter() - .next() - .unwrap() - .1 - .attributes() - .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) - ), + schema.entity_types.iter().next().unwrap().1.attributes(), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -2450,16 +2432,7 @@ pub(crate) mod test { .unwrap(); let schema: ValidatorSchema = fragment.try_into().unwrap(); assert_eq!( - Attributes::with_required_attributes( - schema - .entity_types - .iter() - .next() - .unwrap() - .1 - .attributes() - .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) - ), + schema.entity_types.iter().next().unwrap().1.attributes(), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -2505,16 +2478,7 @@ pub(crate) mod test { .unwrap(); assert_eq!( - Attributes::with_required_attributes( - schema - .entity_types - .iter() - .next() - .unwrap() - .1 - .attributes() - .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())) - ), + schema.entity_types.iter().next().unwrap().1.attributes(), Attributes::with_required_attributes([("a".into(), Type::primitive_long())]) ); } @@ -2896,7 +2860,9 @@ pub(crate) mod test { } ); let schema = ValidatorSchema::from_json_value(src, Extensions::all_available()).unwrap(); - let mut attributes = assert_entity_type_exists(&schema, "Demo::User").attributes(); + let mut attributes = assert_entity_type_exists(&schema, "Demo::User") + .attributes() + .into_iter(); let (attr_name, attr_ty) = attributes.next().unwrap(); assert_eq!(attr_name, "id"); assert_eq!(&attr_ty.attr_type, &Type::primitive_string()); diff --git a/cedar-policy-validator/src/schema/entity_type.rs b/cedar-policy-validator/src/schema/entity_type.rs index 47fc22a53..8f18066ac 100644 --- a/cedar-policy-validator/src/schema/entity_type.rs +++ b/cedar-policy-validator/src/schema/entity_type.rs @@ -19,7 +19,7 @@ use nonempty::NonEmpty; use serde::Serialize; use smol_str::SmolStr; -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; use cedar_policy_core::{ast::EntityType, transitive_closure::TCNode}; @@ -84,10 +84,15 @@ impl ValidatorEntityType { } /// An iterator over the attributes of this entity - pub fn attributes(&self) -> Box + '_> { + pub fn attributes(&self) -> Attributes { match &self.kind { - ValidatorEntityTypeKind::Enum(_) => Box::new(std::iter::empty()), - ValidatorEntityTypeKind::Standard(ty) => Box::new(ty.attributes()), + ValidatorEntityTypeKind::Enum(_) => Attributes { + attrs: BTreeMap::new(), + }, + ValidatorEntityTypeKind::Standard(ty) => Attributes::with_attributes( + ty.attributes() + .map(|(key, value)| (key.clone(), value.clone())), + ), } } @@ -164,6 +169,7 @@ impl From<&ValidatorEntityType> for proto::ValidatorEntityType { attributes: Some(proto::Attributes::from( &Attributes::with_required_attributes( v.attributes() + .into_iter() .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())), ), )), diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index 4c1509349..625e6b3a1 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -949,13 +949,7 @@ impl EntityLUB { let mut lub_element_attributes = self.lub_elements.iter().map(|name| { schema .get_entity_type(name) - .map(|entity_type| { - Attributes::with_attributes( - entity_type - .attributes() - .map(|(attr, ty)| (attr.clone(), ty.clone())), - ) - }) + .map(|entity_type| entity_type.attributes()) .unwrap_or_else(|| Attributes::with_attributes(None)) }); From d81d377e569f2b823b6ae47d76eba2564a111317 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 17:04:57 -0800 Subject: [PATCH 27/28] updated changelog Signed-off-by: Shaobo He --- cedar-policy/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 9abe40919..31ce01d39 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -25,6 +25,7 @@ Cedar Language Version: TBD - Implemented [RFC 62 (extended `has` operator)](https://github.com/cedar-policy/rfcs/blob/main/text/0062-extended-has.md) (#1327, resolving #1329) - Added a helper method to `PartialResponse` to accept substitutions from an iterator. - Added `unknown_*_with_type` methods to the RequestBuilder in partial-eval, allowing an unknown principal or resource to be constrained to a certain entity type. +- Implemented [RFC 53 (enumerated entity types)](https://github.com/cedar-policy/rfcs/blob/main/text/0053-enum-entities.md) (#1377) ### Changed From dc15e5c5483c81552b44802b23a3750c431f3c05 Mon Sep 17 00:00:00 2001 From: Shaobo He Date: Wed, 15 Jan 2025 17:17:33 -0800 Subject: [PATCH 28/28] clippy fixes Signed-off-by: Shaobo He --- cedar-policy-validator/src/coreschema.rs | 2 +- cedar-policy-validator/src/entity_manifest/type_annotations.rs | 2 +- cedar-policy-validator/src/schema/entity_type.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 4a4ebdb28..b780edcc9 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -158,7 +158,7 @@ impl entities::EntityTypeDescription for EntityTypeDescription { .attributes() .into_iter() .filter(|(_, ty)| ty.is_required) - .map(|(attr, _)| attr.clone()), + .map(|(attr, _)| attr), ) } diff --git a/cedar-policy-validator/src/entity_manifest/type_annotations.rs b/cedar-policy-validator/src/entity_manifest/type_annotations.rs index 18039ff1b..a08e8a569 100644 --- a/cedar-policy-validator/src/entity_manifest/type_annotations.rs +++ b/cedar-policy-validator/src/entity_manifest/type_annotations.rs @@ -149,7 +149,7 @@ impl AccessTrie { entity_ty .attributes() .into_iter() - .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())), + .map(|(attr, ty)| (attr, ty.attr_type)), ) } EntityRecordKind::ActionEntity { name: _, attrs } => attrs, diff --git a/cedar-policy-validator/src/schema/entity_type.rs b/cedar-policy-validator/src/schema/entity_type.rs index 8f18066ac..1278926f0 100644 --- a/cedar-policy-validator/src/schema/entity_type.rs +++ b/cedar-policy-validator/src/schema/entity_type.rs @@ -170,7 +170,7 @@ impl From<&ValidatorEntityType> for proto::ValidatorEntityType { &Attributes::with_required_attributes( v.attributes() .into_iter() - .map(|(attr, ty)| (attr.clone(), ty.attr_type.clone())), + .map(|(attr, ty)| (attr, ty.attr_type)), ), )), open_attributes: proto::OpenTag::from(&v.open_attributes()).into(),