diff --git a/cedar-policy-core/src/ast/entity.rs b/cedar-policy-core/src/ast/entity.rs index c44f5b97d4..a20ad24249 100644 --- a/cedar-policy-core/src/ast/entity.rs +++ b/cedar-policy-core/src/ast/entity.rs @@ -280,10 +280,11 @@ impl Entity { &self.attrs } - /// Read-only access the internal `ancestors` hashset. - /// This function is available only inside Core. - pub(crate) fn ancestors_set(&self) -> &HashSet { - &self.ancestors + /// Test if two `Entity` objects are deep/structurally equal. + /// That is, not only do they have the same UID, but also the same + /// attributes, attribute values, and ancestors. + pub(crate) fn deep_eq(&self, other: &Self) -> bool { + self.uid == other.uid && self.attrs == other.attrs && self.ancestors == other.ancestors } /// Set the given attribute to the given value. diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 99f19436a7..51492734c9 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -27,6 +27,8 @@ use std::fmt::Write; use serde::{Deserialize, Serialize}; use serde_with::serde_as; +mod conformance; +pub use conformance::*; mod err; pub use err::*; mod json; @@ -113,14 +115,25 @@ impl Entities { /// Fails if the passed iterator contains any duplicate entities with this structure, /// or if any error is encountered in the transitive closure computation. /// + /// If `schema` is present, then the added entities will be validated + /// against the `schema`, returning an error if they do not conform to the + /// schema. + /// (This method will not add action entities from the `schema`.) + /// /// If you pass [`TCComputation::AssumeAlreadyComputed`], then the caller is /// responsible for ensuring that TC and DAG hold before calling this method. pub fn add_entities( mut self, collection: impl IntoIterator, - mode: TCComputation, + schema: Option<&impl Schema>, + tc_computation: TCComputation, + extensions: Extensions<'_>, ) -> Result { + let checker = schema.map(|schema| EntitySchemaConformanceChecker::new(schema, extensions)); for entity in collection.into_iter() { + if let Some(checker) = checker.as_ref() { + checker.validate_entity(&entity)?; + } match self.entities.entry(entity.uid()) { hash_map::Entry::Occupied(_) => return Err(EntitiesError::Duplicate(entity.uid())), hash_map::Entry::Vacant(vacant_entry) => { @@ -128,7 +141,7 @@ impl Entities { } } } - match mode { + match tc_computation { TCComputation::AssumeAlreadyComputed => (), TCComputation::EnforceAlreadyComputed => { enforce_tc_and_dag(&self.entities).map_err(Box::new)? @@ -141,13 +154,37 @@ impl Entities { /// Create an `Entities` object with the given entities. /// + /// If `schema` is present, then action entities from that schema will also + /// be added to the `Entities`. + /// Also, the entities in `entities` will be validated against the `schema`, + /// returning an error if they do not conform to the schema. + /// /// If you pass `TCComputation::AssumeAlreadyComputed`, then the caller is /// responsible for ensuring that TC and DAG hold before calling this method. pub fn from_entities( entities: impl IntoIterator, + schema: Option<&impl Schema>, tc_computation: TCComputation, + extensions: Extensions<'_>, ) -> Result { let mut entity_map = create_entity_map(entities.into_iter())?; + if let Some(schema) = schema { + // validate entities against schema. + // we do this before adding the actions, because we trust the + // actions were already validated as part of constructing the + // `Schema` + let checker = EntitySchemaConformanceChecker::new(schema, extensions); + for entity in entity_map.values() { + checker.validate_entity(entity)?; + } + // now add the action entities from the schema + entity_map.extend( + schema + .action_entities() + .into_iter() + .map(|e| (e.uid(), unwrap_or_clone(e))), + ); + } match tc_computation { TCComputation::AssumeAlreadyComputed => {} TCComputation::EnforceAlreadyComputed => { @@ -503,14 +540,14 @@ mod json_parsing_tests { } ] ); - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); parser.from_json_value(v).unwrap(); } #[test] fn enforces_tc_fail_cycle_almost() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ { @@ -532,31 +569,29 @@ mod json_parsing_tests { } ]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() - .unwrap(); - let es = simple_entities(&parser); - let es = es - .add_entities(stream, TCComputation::EnforceAlreadyComputed) - .err() - .unwrap(); + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let err = simple_entities(&parser).add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::EnforceAlreadyComputed, + Extensions::none(), + ); // Despite this being a cycle, alice doesn't have the appropriate edges to form the cycle, so we get this error let expected = TcError::MissingTcEdge { child: r#"Test::"janet""#.parse().unwrap(), parent: r#"Test::"george""#.parse().unwrap(), grandparent: r#"Test::"janet""#.parse().unwrap(), }; - match es { - EntitiesError::TransitiveClosureError(e) => assert_eq!(&expected, e.as_ref()), - e => panic!("Wrong error: {e}"), + match err { + Err(EntitiesError::TransitiveClosureError(e)) => assert_eq!(&expected, e.as_ref()), + Err(e) => panic!("Wrong error: {e}"), + Ok(_) => panic!("expected an error here due to TC not computed properly"), } } #[test] fn enforces_tc_fail_connecting() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ { @@ -574,30 +609,28 @@ mod json_parsing_tests { } ]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() - .unwrap(); - let es = simple_entities(&parser); - let es = es - .add_entities(stream, TCComputation::EnforceAlreadyComputed) - .err() - .unwrap(); + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let err = simple_entities(&parser).add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::EnforceAlreadyComputed, + Extensions::all_available(), + ); let expected = TcError::MissingTcEdge { child: r#"Test::"janet""#.parse().unwrap(), parent: r#"Test::"george""#.parse().unwrap(), grandparent: r#"Test::"henry""#.parse().unwrap(), }; - match es { - EntitiesError::TransitiveClosureError(e) => assert_eq!(&expected, e.as_ref()), - e => panic!("Wrong error: {e}"), + match err { + Err(EntitiesError::TransitiveClosureError(e)) => assert_eq!(&expected, e.as_ref()), + Err(e) => panic!("Wrong error: {e}"), + Ok(_) => panic!("expected an error here due to TC not computed properly"), } } #[test] fn enforces_tc_fail_missing_edge() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ { @@ -615,30 +648,28 @@ mod json_parsing_tests { } ]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() - .unwrap(); - let es = simple_entities(&parser); - let es = es - .add_entities(stream, TCComputation::EnforceAlreadyComputed) - .err() - .unwrap(); + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let err = simple_entities(&parser).add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::EnforceAlreadyComputed, + Extensions::all_available(), + ); let expected = TcError::MissingTcEdge { child: r#"Test::"jeff""#.parse().unwrap(), parent: r#"Test::"alice""#.parse().unwrap(), grandparent: r#"Test::"bob""#.parse().unwrap(), }; - match es { - EntitiesError::TransitiveClosureError(e) => assert_eq!(&expected, e.as_ref()), - e => panic!("Wrong error: {e}"), + match err { + Err(EntitiesError::TransitiveClosureError(e)) => assert_eq!(&expected, e.as_ref()), + Err(e) => panic!("Wrong error: {e}"), + Ok(_) => panic!("expected an error here due to TC not computed properly"), } } #[test] fn enforces_tc_success() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ { @@ -660,14 +691,14 @@ mod json_parsing_tests { } ]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() - .unwrap(); - let es = simple_entities(&parser); - let es = es - .add_entities(stream, TCComputation::EnforceAlreadyComputed) + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let es = simple_entities(&parser) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::EnforceAlreadyComputed, + Extensions::all_available(), + ) .unwrap(); let euid = r#"Test::"jeff""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); @@ -679,7 +710,7 @@ mod json_parsing_tests { #[test] fn adds_extends_tc_connecting() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ { @@ -697,13 +728,15 @@ mod json_parsing_tests { } ]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let es = simple_entities(&parser) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .unwrap(); - let es = simple_entities(&parser); - let es = es.add_entities(stream, TCComputation::ComputeNow).unwrap(); let euid = r#"Test::"george""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); assert!(jeff.is_descendant_of(&r#"Test::"henry""#.parse().unwrap())); @@ -714,7 +747,7 @@ mod json_parsing_tests { #[test] fn adds_extends_tc() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ { @@ -734,13 +767,15 @@ mod json_parsing_tests { } ]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let es = simple_entities(&parser) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .unwrap(); - let es = simple_entities(&parser); - let es = es.add_entities(stream, TCComputation::ComputeNow).unwrap(); let euid = r#"Test::"jeff""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); assert!(jeff.is_descendant_of(&r#"Test::"alice""#.parse().unwrap())); @@ -750,7 +785,7 @@ mod json_parsing_tests { #[test] fn adds_works() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ { @@ -770,13 +805,15 @@ mod json_parsing_tests { } ]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let es = simple_entities(&parser) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .unwrap(); - let es = simple_entities(&parser); - let es = es.add_entities(stream, TCComputation::ComputeNow).unwrap(); let euid = r#"Test::"jeff""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); let rexpr = jeff.get("foo").unwrap(); @@ -788,20 +825,20 @@ mod json_parsing_tests { #[test] fn add_duplicates_fail2() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ {"uid":{ "type" : "Test", "id" : "jeff" }, "attrs" : {}, "parents" : []}, {"uid":{ "type" : "Test", "id" : "jeff" }, "attrs" : {}, "parents" : []}]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() - .unwrap(); - let es = simple_entities(&parser); - let err = es - .add_entities(stream, TCComputation::ComputeNow) + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let err = simple_entities(&parser) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .err() .unwrap(); let expected = r#"Test::"jeff""#.parse().unwrap(); @@ -813,34 +850,32 @@ mod json_parsing_tests { #[test] fn add_duplicates_fail1() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); - let new = serde_json::json!([{"uid": { "type" : "Test", "id" : "alice"}, "attrs" : {}, "parents" : []}]); - let stream = parser - .iter_from_json_value(new) - .unwrap() - .collect::>>() - .unwrap(); - let es = simple_entities(&parser); - let err = es - .add_entities(stream, TCComputation::ComputeNow) - .err() - .unwrap(); + let new = serde_json::json!([{"uid":{ "type": "Test", "id": "alice" }, "attrs" : {}, "parents" : []}]); + let addl_entities = parser.iter_from_json_value(new).unwrap(); + let err = simple_entities(&parser).add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ); let expected = r#"Test::"alice""#.parse().unwrap(); match err { - EntitiesError::Duplicate(e) => assert_eq!(e, expected), - e => panic!("Wrong error: {e}"), + Err(EntitiesError::Duplicate(e)) => assert_eq!(e, expected), + Err(e) => panic!("Wrong error: {e}"), + Ok(_) => panic!("expected an error here due to TC not computed properly"), } } #[test] fn simple_entities_correct() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); simple_entities(&parser); } - fn simple_entities(parser: &EntityJsonParser<'_>) -> Entities { + fn simple_entities(parser: &EntityJsonParser<'_, '_>) -> Entities { let json = serde_json::json!( [ { @@ -933,7 +968,7 @@ mod json_parsing_tests { ] ); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let es = eparser .from_json_value(json) @@ -990,7 +1025,7 @@ mod json_parsing_tests { ] ); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let es = eparser.from_json_value(json).expect("JSON is correct"); @@ -1030,7 +1065,7 @@ mod json_parsing_tests { ] }, ]); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let error = eparser.from_json_value(json).err().unwrap().to_string(); assert!( @@ -1063,7 +1098,7 @@ mod json_parsing_tests { ] } ]); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let error = eparser.from_json_value(json).err().unwrap().to_string(); assert!( @@ -1096,7 +1131,7 @@ mod json_parsing_tests { ] } ]); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let error = eparser.from_json_value(json).err().unwrap().to_string(); assert!( @@ -1127,7 +1162,7 @@ mod json_parsing_tests { ] } ]); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let error = eparser.from_json_value(json).err().unwrap().to_string(); assert!( @@ -1158,7 +1193,7 @@ mod json_parsing_tests { ] } ]); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let error = eparser.from_json_value(json).err().unwrap().to_string(); assert!( @@ -1211,7 +1246,7 @@ mod json_parsing_tests { ] ); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let es = eparser.from_json_value(json).expect("JSON is correct"); @@ -1297,7 +1332,7 @@ mod json_parsing_tests { ] ); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let es = eparser.from_json_value(json).expect("JSON is correct"); @@ -1323,7 +1358,7 @@ mod json_parsing_tests { #[test] fn uid_failures() { // various JSON constructs that are invalid in `uid` and `parents` fields - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let json = serde_json::json!( @@ -1444,7 +1479,7 @@ mod json_parsing_tests { fn roundtrip(entities: &Entities) -> Result { let mut buf = Vec::new(); entities.write_to_json(&mut buf)?; - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); eparser.from_json_str(&String::from_utf8(buf).expect("should be valid UTF-8")) } @@ -1470,8 +1505,13 @@ mod json_parsing_tests { ); let (e0, e1, e2, e3) = test_entities(); - let entities = Entities::from_entities([e0, e1, e2, e3], TCComputation::ComputeNow) - .expect("Failed to construct entities"); + let entities = Entities::from_entities( + [e0, e1, e2, e3], + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::none(), + ) + .expect("Failed to construct entities"); assert_eq!( entities, roundtrip(&entities).expect("should roundtrip without errors") @@ -1529,7 +1569,9 @@ mod json_parsing_tests { Entity::with_uid(EntityUID::with_eid("parent1")), Entity::with_uid(EntityUID::with_eid("parent2")), ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::all_available(), ) .expect("Failed to construct entities"); assert_eq!( @@ -1559,7 +1601,9 @@ mod json_parsing_tests { Entity::with_uid(EntityUID::with_eid("parent1")), Entity::with_uid(EntityUID::with_eid("parent2")), ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::all_available(), ) .expect("Failed to construct entities"); assert!(matches!( @@ -1582,7 +1626,7 @@ mod json_parsing_tests { } ] ); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let err = eparser .from_json_value(json) @@ -1611,7 +1655,7 @@ mod json_parsing_tests { } ] ); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); assert_matches!(eparser.from_json_value(json), Ok(_)); } @@ -1637,7 +1681,7 @@ mod json_parsing_tests { } ] "#; - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let err = eparser .from_json_str(json) @@ -1679,8 +1723,13 @@ mod entities_tests { fn test_iter() { let (e0, e1, e2, e3) = test_entities(); let v = vec![e0.clone(), e1.clone(), e2.clone(), e3.clone()]; - let es = Entities::from_entities(v, TCComputation::ComputeNow) - .expect("Failed to construct entities"); + let es = Entities::from_entities( + v, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) + .expect("Failed to construct entities"); let es_v = es.iter().collect::>(); assert!(es_v.len() == 4, "All entities should be in the vec"); assert!(es_v.contains(&&e0)); @@ -1700,7 +1749,12 @@ mod entities_tests { e1.add_ancestor(EntityUID::with_eid("b")); e2.add_ancestor(EntityUID::with_eid("c")); - let es = Entities::from_entities(vec![e1, e2, e3], TCComputation::EnforceAlreadyComputed); + let es = Entities::from_entities( + vec![e1, e2, e3], + None::<&NoEntitiesSchema>, + TCComputation::EnforceAlreadyComputed, + Extensions::all_available(), + ); match es { Ok(_) => panic!("Was not transitively closed!"), Err(EntitiesError::TransitiveClosureError(_)) => (), @@ -1721,8 +1775,13 @@ mod entities_tests { e1.add_ancestor(EntityUID::with_eid("c")); e2.add_ancestor(EntityUID::with_eid("c")); - Entities::from_entities(vec![e1, e2, e3], TCComputation::EnforceAlreadyComputed) - .expect("Should have succeeded"); + Entities::from_entities( + vec![e1, e2, e3], + None::<&NoEntitiesSchema>, + TCComputation::EnforceAlreadyComputed, + Extensions::all_available(), + ) + .expect("Should have succeeded"); } } @@ -1743,6 +1802,7 @@ mod schema_based_parsing_tests { struct MockSchema; impl Schema for MockSchema { type EntityTypeDescription = MockEmployeeDescription; + type ActionEntityIterator = std::iter::Empty>; fn entity_type(&self, entity_type: &EntityType) -> Option { match entity_type.to_string().as_str() { "Employee" => Some(MockEmployeeDescription), @@ -1780,6 +1840,9 @@ mod schema_based_parsing_tests { _ => Box::new(std::iter::empty()), } } + fn action_entities(&self) -> Self::ActionEntityIterator { + std::iter::empty() + } } /// Mock schema impl for the `Employee` type used in these tests @@ -1867,7 +1930,7 @@ mod schema_based_parsing_tests { } } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// JSON that should parse differently with and without the above schema #[test] fn with_and_without_schema() { @@ -1901,7 +1964,7 @@ mod schema_based_parsing_tests { // without schema-based parsing, `home_ip` and `trust_score` are // strings, `manager` and `work_ip` are Records, `hr_contacts` contains // Records, and `json_blob.inner3.innerinner` is a Record - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let parsed = eparser .from_json_value(entitiesjson.clone()) @@ -1967,7 +2030,7 @@ mod schema_based_parsing_tests { // but with schema-based parsing, we get these other types let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2074,7 +2137,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// simple type mismatch with expected type #[test] fn type_mismatch_string_long() { @@ -2106,7 +2169,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2119,7 +2182,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// another simple type mismatch with expected type #[test] fn type_mismatch_entity_record() { @@ -2151,7 +2214,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2165,7 +2228,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// type mismatch where we expect a set and get just a single element #[test] fn type_mismatch_set_element() { @@ -2194,7 +2257,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2207,7 +2270,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// type mismatch where we just get the wrong entity type #[test] fn type_mismatch_entity_types() { @@ -2239,7 +2302,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2252,7 +2315,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// type mismatch where we're expecting an extension type and get a /// different extension type #[test] @@ -2285,7 +2348,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2298,7 +2361,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] #[test] fn missing_record_attr() { // missing a record attribute entirely @@ -2329,7 +2392,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2342,7 +2405,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// record attribute has the wrong type #[test] fn type_mismatch_in_record_attr() { @@ -2374,7 +2437,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2418,7 +2481,7 @@ mod schema_based_parsing_tests { .expect("this version with explicit __entity and __extn escapes should also pass"); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// unexpected record attribute #[test] fn unexpected_record_attr() { @@ -2451,7 +2514,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2464,6 +2527,7 @@ mod schema_based_parsing_tests { ); } + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// entity is missing a required attribute #[test] fn missing_required_attr() { @@ -2494,7 +2558,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2502,12 +2566,12 @@ mod schema_based_parsing_tests { .from_json_value(entitiesjson) .expect_err("should fail due to missing attribute \"numDirectReports\""); assert!( - err.to_string().contains(r#"expected entity `Employee::"12UA45"` to have an attribute `numDirectReports`, but it does not"#), + err.to_string().contains(r#"expected entity `Employee::"12UA45"` to have attribute `numDirectReports`, but it does not"#), "actual error message was {err}" ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// unexpected entity attribute #[test] fn unexpected_entity_attr() { @@ -2540,7 +2604,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2555,7 +2619,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// Test that involves parents of wrong types #[test] fn parents_wrong_type() { @@ -2589,7 +2653,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2598,7 +2662,7 @@ mod schema_based_parsing_tests { .expect_err("should fail due to incorrect parent type"); assert!( err.to_string().contains( - r#"`Employee::"12UA45"` is not allowed to have a parent of type `Employee` according to the schema"# + r#"`Employee::"12UA45"` is not allowed to have an ancestor of type `Employee` according to the schema"# ), "actual error message was {err}" ); @@ -2617,7 +2681,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2645,7 +2709,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2677,7 +2741,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2710,7 +2774,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2742,7 +2806,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2772,7 +2836,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2805,7 +2869,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2835,7 +2899,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2868,7 +2932,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2891,6 +2955,7 @@ mod schema_based_parsing_tests { struct MockSchema; impl Schema for MockSchema { type EntityTypeDescription = MockEmployeeDescription; + type ActionEntityIterator = std::iter::Empty>; fn entity_type(&self, entity_type: &EntityType) -> Option { if &entity_type.to_string() == "XYZCorp::Employee" { Some(MockEmployeeDescription) @@ -2912,6 +2977,9 @@ mod schema_based_parsing_tests { _ => Box::new(std::iter::empty()), } } + fn action_entities(&self) -> Self::ActionEntityIterator { + std::iter::empty() + } } struct MockEmployeeDescription; @@ -2958,7 +3026,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs new file mode 100644 index 0000000000..4d1d933348 --- /dev/null +++ b/cedar-policy-core/src/entities/conformance.rs @@ -0,0 +1,319 @@ +use super::{AttributeType, EntityTypeDescription, Schema, SchemaType}; +use crate::ast::{BorrowedRestrictedExpr, Entity, EntityType, EntityUID, ExprKind, Literal}; +use crate::extensions::{ExtensionFunctionLookupError, Extensions}; +use smol_str::SmolStr; +use std::collections::HashMap; +use thiserror::Error; + +/// Errors raised when entities do not conform to the schema +#[derive(Debug, Error)] +pub enum EntitySchemaConformanceError { + /// Encountered attribute that shouldn't exist on entities of this type + #[error("attribute `{attr}` on `{uid}` should not exist according to the schema")] + UnexpectedEntityAttr { + /// Entity that had the unexpected attribute + uid: EntityUID, + /// Name of the attribute that was unexpected + attr: SmolStr, + }, + /// Didn't encounter attribute that should exist + #[error("expected entity `{uid}` to have attribute `{attr}`, but it does not")] + MissingRequiredEntityAttr { + /// Entity that is missing a required attribute + uid: EntityUID, + /// Name of the attribute which was expected + attr: SmolStr, + }, + /// The given attribute on the given entity had a different type than the + /// schema indicated + #[error("in attribute `{attr}` on `{uid}`, type mismatch: attribute was expected to have type {expected}, but actually has type {actual}")] + TypeMismatch { + /// Entity where the type mismatch occurred + uid: EntityUID, + /// Name of the attribute where the type mismatch occurred + attr: SmolStr, + /// Type which was expected + expected: Box, + /// Type which was encountered instead + actual: Box, + }, + /// Found a set whose elements don't all have the same type. This doesn't match + /// any possible schema. + #[error("in attribute `{attr}` on `{uid}`, {err}")] + HeterogeneousSet { + /// Entity where the error occurred + uid: EntityUID, + /// Name of the attribute where the error occurred + attr: SmolStr, + /// Underlying error + err: HeterogeneousSetError, + }, + /// Found an ancestor of a type that's not allowed for that entity + #[error( + "`{uid}` is not allowed to have an ancestor of type `{ancestor_ty}` according to the schema" + )] + InvalidAncestorType { + /// Entity that has an invalid ancestor type + uid: EntityUID, + /// Ancestor type which was invalid + ancestor_ty: Box, // boxed to avoid this variant being very large (and thus all EntitySchemaConformanceErrors being large) + }, + /// Encountered an entity of a type which is not declared in the schema. + /// Note that this error is only used for non-Action entity types. + #[error("entity `{uid}` has type `{}` which is not declared in the schema{}", + &.uid.entity_type(), + match .suggested_types.as_slice() { + [] => String::new(), + [ty] => format!(". Did you mean `{ty}`?"), + tys => format!(". Did you mean one of {:?}?", tys.iter().map(ToString::to_string).collect::>()) + } + )] + UnexpectedEntityType { + /// Entity that had the unexpected type + uid: EntityUID, + /// Suggested similar entity types that actually are declared in the schema (if any) + suggested_types: Vec, + }, + /// Encountered an action which was not declared in the schema + #[error("found action entity `{uid}`, but it was not declared as an action in the schema")] + UndeclaredAction { + /// Action which was not declared in the schema + uid: EntityUID, + }, + /// Encountered an action whose definition doesn't precisely match the + /// schema's declaration of that action + #[error("definition of action `{uid}` does not match its schema declaration")] + ActionDeclarationMismatch { + /// Action whose definition mismatched between entity data and schema + uid: EntityUID, + }, + /// Error looking up an extension function. This error can occur when + /// checking entity conformance because that may require getting information + /// about any extension functions referenced in entity attribute values. + #[error("in attribute `{attr}` on `{uid}`, {err}")] + ExtensionFunctionLookup { + /// Entity where the error occurred + uid: EntityUID, + /// Name of the attribute where the error occurred + attr: SmolStr, + /// Underlying error + err: ExtensionFunctionLookupError, + }, +} + +/// Found a set whose elements don't all have the same type. This doesn't match +/// any possible schema. +#[derive(Debug, Error)] +#[error("set elements have different types: {ty1} and {ty2}")] +pub struct HeterogeneousSetError { + /// First element type which was found + ty1: Box, + /// Second element type which was found + ty2: Box, +} + +/// Struct used to check whether entities conform to a schema +#[derive(Debug, Clone)] +pub struct EntitySchemaConformanceChecker<'a, S: Schema> { + /// Schema to check conformance with + schema: &'a S, + /// Extensions which are active for the conformance checks + extensions: Extensions<'a>, +} + +impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { + /// Create a new checker + pub fn new(schema: &'a S, extensions: Extensions<'a>) -> Self { + Self { schema, extensions } + } + + /// Validate an entity against the schema, returning an + /// [`EntitySchemaConformanceError`] if it does not comply. + pub fn validate_entity(&self, entity: &Entity) -> Result<(), EntitySchemaConformanceError> { + let uid = entity.uid(); + let etype = uid.entity_type(); + if etype.is_action() { + let schema_action = self + .schema + .action(&uid) + .ok_or(EntitySchemaConformanceError::UndeclaredAction { uid: uid.clone() })?; + // check that the action exactly matches the schema's definition + if !entity.deep_eq(&schema_action) { + return Err(EntitySchemaConformanceError::ActionDeclarationMismatch { + uid: uid.clone(), + }); + } + } else { + let schema_etype = self.schema.entity_type(etype).ok_or_else(|| { + let suggested_types = match etype { + EntityType::Concrete(name) => self + .schema + .entity_types_with_basename(name.basename()) + .collect(), + EntityType::Unspecified => vec![], + }; + EntitySchemaConformanceError::UnexpectedEntityType { + uid: uid.clone(), + suggested_types, + } + })?; + // Ensure that all required attributes for `etype` are actually + // included in `entity` + for required_attr in schema_etype.required_attrs() { + if entity.get(&required_attr).is_none() { + return Err(EntitySchemaConformanceError::MissingRequiredEntityAttr { + uid: uid.clone(), + attr: required_attr, + }); + } + } + // For each attribute that actually appears in `entity`, ensure it + // complies with the schema + for (attr, val) in entity.attrs() { + match schema_etype.attr_type(attr) { + None => { + // `None` indicates the attribute shouldn't exist -- see + // docs on the `attr_type()` trait method + return Err(EntitySchemaConformanceError::UnexpectedEntityAttr { + uid: uid.clone(), + attr: attr.into(), + }); + } + Some(expected_ty) => { + // typecheck: ensure that the entity attribute value matches + // the expected type + match type_of_restricted_expr(val, self.extensions) { + Ok(actual_ty) => { + if actual_ty.is_consistent_with(&expected_ty) { + // typecheck passes + } else { + return Err(EntitySchemaConformanceError::TypeMismatch { + uid: uid.clone(), + attr: attr.into(), + expected: Box::new(expected_ty), + actual: Box::new(actual_ty), + }); + } + } + Err(TypeOfRestrictedExprError::HeterogeneousSet(err)) => { + return Err(EntitySchemaConformanceError::HeterogeneousSet { + uid: uid.clone(), + attr: attr.into(), + err, + }); + } + Err(TypeOfRestrictedExprError::ExtensionFunctionLookup(err)) => { + return Err( + EntitySchemaConformanceError::ExtensionFunctionLookup { + uid: uid.clone(), + attr: attr.into(), + err, + }, + ); + } + } + } + } + } + // For each ancestor that actually appears in `entity`, ensure the + // ancestor type is allowed by the schema + for ancestor_euid in entity.ancestors() { + let ancestor_type = ancestor_euid.entity_type(); + if schema_etype.allowed_parent_types().contains(ancestor_type) { + // note that `allowed_parent_types()` was transitively + // closed, so it's actually `allowed_ancestor_types()` + // + // thus, the check passes in this case + } else { + return Err(EntitySchemaConformanceError::InvalidAncestorType { + uid: uid.clone(), + ancestor_ty: Box::new(ancestor_type.clone()), + }); + } + } + } + Ok(()) + } +} + +/// Errors thrown by [`type_of_restricted_expr()`] +#[derive(Debug, Error)] +pub enum TypeOfRestrictedExprError { + /// Encountered a heterogeneous set + #[error(transparent)] + HeterogeneousSet(#[from] HeterogeneousSetError), + /// Error looking up an extension function + #[error(transparent)] + ExtensionFunctionLookup(#[from] ExtensionFunctionLookupError), +} + +/// Get the [`SchemaType`] of a restricted expression. +/// +/// This isn't possible for general `Expr`s (without a request, full schema, +/// etc), but is possible for restricted expressions, given the information in +/// `Extensions`. +/// +/// For records, we can't know whether the attributes in the given record are +/// required or optional. +/// This function, when given a record that has keys A, B, and C, will return a +/// `SchemaType` where A, B, and C are all marked as optional attributes, but no +/// other attributes are possible. +/// That is, this assumes that all existing attributes are optional, but that no +/// other optional attributes are possible. +/// Compared to marking A, B, and C as required, this allows the returned +/// `SchemaType` to `is_consistent_with()` more types. +pub fn type_of_restricted_expr( + rexpr: BorrowedRestrictedExpr<'_>, + extensions: Extensions<'_>, +) -> Result { + match rexpr.expr_kind() { + ExprKind::Lit(Literal::Bool(_)) => Ok(SchemaType::Bool), + ExprKind::Lit(Literal::Long(_)) => Ok(SchemaType::Long), + ExprKind::Lit(Literal::String(_)) => Ok(SchemaType::String), + ExprKind::Lit(Literal::EntityUID(uid)) => Ok(SchemaType::Entity { ty: uid.entity_type().clone() }), + ExprKind::Set(elements) => { + let mut element_types = elements.iter().map(|el| { + type_of_restricted_expr(BorrowedRestrictedExpr::new_unchecked(el), extensions) // assuming the invariant holds for the set as a whole, it will also hold for each element + }); + match element_types.next() { + None => Ok(SchemaType::EmptySet), + Some(Err(e)) => Err(e), + Some(Ok(element_ty)) => { + let matches_element_ty = |ty: &Result| matches!(ty, Ok(ty) if ty.is_consistent_with(&element_ty)); + let conflicting_ty = element_types.find(|ty| !matches_element_ty(ty)); + match conflicting_ty { + None => Ok(SchemaType::Set { element_ty: Box::new(element_ty) }), + Some(Ok(conflicting_ty)) => Err(HeterogeneousSetError { + ty1: Box::new(element_ty), + ty2: Box::new(conflicting_ty), + }.into()), + Some(Err(e)) => Err(e), + } + } + } + } + ExprKind::Record(map) => { + Ok(SchemaType::Record { attrs: { + map.iter().map(|(k, v)| { + let attr_type = type_of_restricted_expr( + BorrowedRestrictedExpr::new_unchecked(v), // assuming the invariant holds for the record as a whole, it will also hold for each attribute value + extensions, + )?; + // we can't know if the attribute is required or optional, + // but marking it optional is more flexible -- allows the + // attribute type to `is_consistent_with()` more types + Ok((k.clone(), AttributeType::optional(attr_type))) + }).collect::, TypeOfRestrictedExprError>>()? + }}) + } + ExprKind::ExtensionFunctionApp { fn_name, .. } => { + let efunc = extensions.func(fn_name)?; + Ok(efunc.return_type().cloned().ok_or_else(|| ExtensionFunctionLookupError::HasNoType { + name: efunc.name().clone() + })?) + } + // PANIC SAFETY. Unreachable by invariant on restricted expressions + #[allow(clippy::unreachable)] + expr => unreachable!("internal invariant violation: BorrowedRestrictedExpr somehow contained this expr case: {expr:?}"), + } +} diff --git a/cedar-policy-core/src/entities/err.rs b/cedar-policy-core/src/entities/err.rs index ea0e3614bb..2689def836 100644 --- a/cedar-policy-core/src/entities/err.rs +++ b/cedar-policy-core/src/entities/err.rs @@ -34,6 +34,9 @@ pub enum EntitiesError { /// entity hierarchy. #[error("transitive closure computation/enforcement error: {0}")] TransitiveClosureError(#[from] Box>), + /// Error because an entity doesn't conform to the schema + #[error("entity does not conform to the schema: {0}")] + InvalidEntity(#[from] crate::entities::EntitySchemaConformanceError), } /// Type alias for convenience diff --git a/cedar-policy-core/src/entities/json/context.rs b/cedar-policy-core/src/entities/json/context.rs index 826174ec4e..c212237bcb 100644 --- a/cedar-policy-core/src/entities/json/context.rs +++ b/cedar-policy-core/src/entities/json/context.rs @@ -75,9 +75,9 @@ impl<'e, 's, S: ContextSchema> ContextJsonParser<'e, 's, S> { &self, json: serde_json::Value, ) -> Result { - let vparser = ValueParser::new(self.extensions.clone()); + let vparser = ValueParser::new(self.extensions); let expected_ty = self.schema.map(|s| s.context_type()); - let rexpr = vparser.val_into_rexpr(json, expected_ty.as_ref(), || { + let rexpr = vparser.val_into_restricted_expr(json, expected_ty.as_ref(), || { JsonDeserializationErrorContext::Context })?; Context::from_expr(rexpr).map_err(|rexpr| { diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 476b39c3d0..51c7dd088e 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -20,7 +20,10 @@ use super::{ ValueParser, }; use crate::ast::{Entity, EntityType, EntityUID, RestrictedExpr}; -use crate::entities::{Entities, EntitiesError, TCComputation}; +use crate::entities::{ + type_of_restricted_expr, unwrap_or_clone, Entities, EntitiesError, + EntitySchemaConformanceError, TCComputation, TypeOfRestrictedExprError, +}; use crate::extensions::Extensions; use crate::jsonvalue::JsonValueWithNoDuplicateKeys; use serde::{Deserialize, Serialize}; @@ -50,14 +53,13 @@ pub struct EntityJson { /// Struct used to parse entities from JSON. #[derive(Debug, Clone)] -pub struct EntityJsonParser<'e, S: Schema = NoEntitiesSchema> { - /// If a `schema` is present, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit. - /// It will also ensure that the produced `Entities` fully conforms to the - /// `schema` -- for instance, it will error if attributes have the wrong - /// types (e.g., string instead of integer), or if required attributes are - /// missing or superfluous attributes are provided. - schema: Option, +pub struct EntityJsonParser<'e, 's, S: Schema = NoEntitiesSchema> { + /// See comments on [`EntityJsonParser::new()`] for the interpretation and + /// effects of this `schema` field. + /// + /// (Long doc comment on `EntityJsonParser::new()` is not repeated here, and + /// instead incorporated by reference, to avoid them becoming out of sync.) + schema: Option<&'s S>, /// Extensions which are active for the JSON parsing. extensions: Extensions<'e>, @@ -79,20 +81,28 @@ enum EntitySchemaInfo { NonAction(E), } -impl<'e, S: Schema> EntityJsonParser<'e, S> { +impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { /// Create a new `EntityJsonParser`. /// - /// If a `schema` is present, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit. - /// It will also ensure that the produced `Entities` fully conforms to the - /// `schema` -- for instance, it will error if attributes have the wrong - /// types (e.g., string instead of integer), or if required attributes are - /// missing or superfluous attributes are provided. + /// `schema` represents a source of `Action` entities, which will be added + /// to the entities parsed from JSON. + /// (If any `Action` entities are present in the JSON, and a `schema` is + /// also provided, each `Action` entity in the JSON must exactly match its + /// definition in the schema or an error is returned.) + /// + /// If a `schema` is present, this will also inform the parsing: for + /// instance, it will allow `__entity` and `__extn` escapes to be implicit. + /// + /// Finally, if a `schema` is present, the `EntityJsonParser` will ensure + /// that the produced entities fully conform to the `schema` -- for + /// instance, it will error if attributes have the wrong types (e.g., string + /// instead of integer), or if required attributes are missing or + /// superfluous attributes are provided. /// /// If you pass `TCComputation::AssumeAlreadyComputed`, then the caller is /// responsible for ensuring that TC holds before calling this method. pub fn new( - schema: Option, + schema: Option<&'s S>, extensions: Extensions<'e>, tc_computation: TCComputation, ) -> Self { @@ -103,145 +113,152 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { } } - /// Parse an entities JSON file (in [`&str`] form) into an [`Entities`] object + /// Parse an entities JSON file (in [`&str`] form) into an [`Entities`] object. + /// + /// If the `EntityJsonParser` has a `schema`, this also adds `Action` + /// entities declared in the `schema`. pub fn from_json_str(&self, json: &str) -> Result { let ejsons: Vec = serde_json::from_str(json).map_err(JsonDeserializationError::from)?; self.parse_ejsons(ejsons) } - /// Parse an entities JSON file (in [`serde_json::Value`] form) into an [`Entities`] object + /// Parse an entities JSON file (in [`serde_json::Value`] form) into an [`Entities`] object. + /// + /// If the `EntityJsonParser` has a `schema`, this also adds `Action` + /// entities declared in the `schema`. pub fn from_json_value(&self, json: serde_json::Value) -> Result { let ejsons: Vec = serde_json::from_value(json).map_err(JsonDeserializationError::from)?; self.parse_ejsons(ejsons) } - /// Parse an entities JSON file (in [`std::io::Read`] form) into an [`Entities`] object + /// Parse an entities JSON file (in [`std::io::Read`] form) into an [`Entities`] object. + /// + /// If the `EntityJsonParser` has a `schema`, this also adds `Action` + /// entities declared in the `schema`. pub fn from_json_file(&self, json: impl std::io::Read) -> Result { let ejsons: Vec = serde_json::from_reader(json).map_err(JsonDeserializationError::from)?; self.parse_ejsons(ejsons) } - /// Parse an entities JSON file (in [`&str`] form) into an iterator over [`Entity`]s + /// Parse an entities JSON file (in [`&str`] form) into an iterator over [`Entity`]s. + /// + /// If the `EntityJsonParser` has a `schema`, this also adds `Action` + /// entities declared in the `schema`. pub fn iter_from_json_str( &self, json: &str, - ) -> Result> + '_, EntitiesError> { + ) -> Result + '_, EntitiesError> { let ejsons: Vec = serde_json::from_str(json).map_err(JsonDeserializationError::from)?; - Ok(ejsons - .into_iter() - .map(|ejson| self.parse_ejson(ejson).map_err(EntitiesError::from))) + self.iter_ejson_to_iter_entity(ejsons) } - /// Parse an entities JSON file (in [`serde_json::Value`] form) into an iterator over [`Entity`]s + /// Parse an entities JSON file (in [`serde_json::Value`] form) into an iterator over [`Entity`]s. + /// + /// If the `EntityJsonParser` has a `schema`, this also adds `Action` + /// entities declared in the `schema`. pub fn iter_from_json_value( &self, json: serde_json::Value, - ) -> Result> + '_, EntitiesError> { + ) -> Result + '_, EntitiesError> { let ejsons: Vec = serde_json::from_value(json).map_err(JsonDeserializationError::from)?; - Ok(ejsons - .into_iter() - .map(|ejson| self.parse_ejson(ejson).map_err(EntitiesError::from))) + self.iter_ejson_to_iter_entity(ejsons) } - /// Parse an entities JSON file (in [`std::io::Read`] form) into an iterator over [`Entity`]s + /// Parse an entities JSON file (in [`std::io::Read`] form) into an iterator over [`Entity`]s. + /// + /// If the `EntityJsonParser` has a `schema`, this also adds `Action` + /// entities declared in the `schema`. pub fn iter_from_json_file( &self, json: impl std::io::Read, - ) -> Result> + '_, EntitiesError> { + ) -> Result + '_, EntitiesError> { let ejsons: Vec = serde_json::from_reader(json).map_err(JsonDeserializationError::from)?; - Ok(ejsons + self.iter_ejson_to_iter_entity(ejsons) + } + + /// Internal function that converts an iterator over [`EntityJson`] into an + /// iterator over [`Entity`] and also adds any `Action` entities declared in + /// `self.schema`. + fn iter_ejson_to_iter_entity( + &self, + ejsons: impl IntoIterator, + ) -> Result + '_, EntitiesError> { + let mut entities: Vec = ejsons .into_iter() - .map(|ejson| self.parse_ejson(ejson).map_err(EntitiesError::from))) + .map(|ejson| self.parse_ejson(ejson).map_err(EntitiesError::from)) + .collect::>()?; + if let Some(schema) = &self.schema { + entities.extend(schema.action_entities().into_iter().map(unwrap_or_clone)); + } + Ok(entities.into_iter()) } - /// internal function that creates an [`Entities`] from a stream of [`EntityJson`] + /// Internal function that creates an [`Entities`] from a stream of [`EntityJson`]. + /// + /// If the `EntityJsonParser` has a `schema`, this also adds `Action` + /// entities declared in the `schema`, and validates all the entities + /// against the schema. fn parse_ejsons( &self, ejsons: impl IntoIterator, ) -> Result { - let entities = ejsons + let entities: Vec = ejsons .into_iter() .map(|ejson| self.parse_ejson(ejson)) - .collect::, _>>()?; - Entities::from_entities(entities, self.tc_computation) + .collect::>()?; + Entities::from_entities(entities, self.schema, self.tc_computation, self.extensions) } - /// internal function that parses an `EntityJson` into an `Entity` + /// Internal function that parses an `EntityJson` into an `Entity`. + /// + /// This function is not responsible for fully validating the `Entity` + /// against the `schema`; that happens on construction of an `Entities` fn parse_ejson(&self, ejson: EntityJson) -> Result { let uid = ejson .uid .into_euid(|| JsonDeserializationErrorContext::EntityUid)?; let etype = uid.entity_type(); - let entity_schema_info = - match &self.schema { - None => EntitySchemaInfo::NoSchema, - Some(schema) => { - if etype.is_action() { - EntitySchemaInfo::Action(schema.action(&uid).ok_or( - JsonDeserializationError::UndeclaredAction { uid: uid.clone() }, - )?) - } else { - EntitySchemaInfo::NonAction(schema.entity_type(etype).ok_or_else(|| { - let basename = match etype { - EntityType::Concrete(name) => name.basename(), - // PANIC SAFETY: impossible to have the unspecified EntityType in JSON - #[allow(clippy::unreachable)] - EntityType::Unspecified => { - unreachable!("unspecified EntityType in JSON") - } - }; - JsonDeserializationError::UnexpectedEntityType { - uid: uid.clone(), - suggested_types: schema - .entity_types_with_basename(basename) - .collect(), + let entity_schema_info = match &self.schema { + None => EntitySchemaInfo::NoSchema, + Some(schema) => { + if etype.is_action() { + EntitySchemaInfo::Action(schema.action(&uid).ok_or( + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::UndeclaredAction { uid: uid.clone() }, + ), + )?) + } else { + EntitySchemaInfo::NonAction(schema.entity_type(etype).ok_or_else(|| { + let suggested_types = match etype { + EntityType::Concrete(name) => { + schema.entity_types_with_basename(name.basename()).collect() } - })?) - } - } - }; - match &entity_schema_info { - EntitySchemaInfo::NoSchema => {} // no checks to do - EntitySchemaInfo::Action(action) => { - // here, we ensure that all the attributes on the schema's copy of the - // action do exist in `ejson.attrs`. Later when consuming `ejson.attrs`, - // we'll do the rest of the checks for attribute agreement. - for schema_attr in action.attrs_map().keys() { - if !ejson.attrs.contains_key(schema_attr) { - return Err(JsonDeserializationError::ActionDeclarationMismatch { uid }); - } - } - } - EntitySchemaInfo::NonAction(etype_desc) => { - // here, we ensure that all required attributes for `etype` are actually - // included in `ejson.attrs`. Later when consuming `ejson.attrs` to build - // `attrs`, we'll check for unexpected attributes. - for required_attr in etype_desc.required_attrs() { - if ejson.attrs.contains_key(&required_attr) { - // all good - } else { - return Err(JsonDeserializationError::MissingRequiredEntityAttr { - uid, - attr: required_attr, - }); - } + EntityType::Unspecified => vec![], + }; + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::UnexpectedEntityType { + uid: uid.clone(), + suggested_types, + }, + ) + })?) } } - } - let vparser = ValueParser::new(self.extensions.clone()); + }; + let vparser = ValueParser::new(self.extensions); let attrs: HashMap = ejson .attrs .into_iter() .map(|(k, v)| match &entity_schema_info { EntitySchemaInfo::NoSchema => Ok(( k.clone(), - vparser.val_into_rexpr(v.into(), None, || { + vparser.val_into_restricted_expr(v.into(), None, || { JsonDeserializationErrorContext::EntityAttribute { uid: uid.clone(), attr: k.clone(), @@ -251,49 +268,27 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { EntitySchemaInfo::NonAction(desc) => { // Depending on the expected type, we may parse the contents // of the attribute differently. - let (rexpr, expected_ty) = match desc.attr_type(&k) { + let rexpr = match desc.attr_type(&k) { // `None` indicates the attribute shouldn't exist -- see // docs on the `attr_type()` trait method None => { - return Err(JsonDeserializationError::UnexpectedEntityAttr { - uid: uid.clone(), - attr: k, - }) - } - Some(expected_ty) => ( - vparser.val_into_rexpr(v.into(), Some(&expected_ty), || { - JsonDeserializationErrorContext::EntityAttribute { + return Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::UnexpectedEntityAttr { uid: uid.clone(), - attr: k.clone(), - } - })?, - expected_ty, - ), - }; - // typecheck: ensure that the final type of whatever we - // parsed actually does match the expected type. (For - // instance, this is where we check that we actually got the - // correct entity type when we expected an entity type, the - // correct extension type when we expected an extension - // type, or the correct type at all in other cases.) - let actual_ty = vparser.type_of_rexpr(rexpr.as_borrowed(), || { - JsonDeserializationErrorContext::EntityAttribute { - uid: uid.clone(), - attr: k.clone(), + attr: k, + }, + )) } - })?; - if actual_ty.is_consistent_with(&expected_ty) { - Ok((k, rexpr)) - } else { - Err(JsonDeserializationError::TypeMismatch { - ctx: Box::new(JsonDeserializationErrorContext::EntityAttribute { + Some(expected_ty) => vparser.val_into_restricted_expr( + v.into(), + Some(&expected_ty), + || JsonDeserializationErrorContext::EntityAttribute { uid: uid.clone(), - attr: k, - }), - expected: Box::new(expected_ty), - actual: Box::new(actual_ty), - }) - } + attr: k.clone(), + }, + )?, + }; + Ok((k.clone(), rexpr)) } EntitySchemaInfo::Action(action) => { // We'll do schema-based parsing assuming optimistically that @@ -304,21 +299,38 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { // `None` indicates the attribute isn't in the schema's // copy of the action entity None => { - return Err(JsonDeserializationError::ActionDeclarationMismatch { - uid: uid.clone(), - }) + return Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::ActionDeclarationMismatch { + uid: uid.clone(), + }, + )) } Some(rexpr) => rexpr, }; let expected_ty = - vparser.type_of_rexpr(expected_rexpr.as_borrowed(), || { - JsonDeserializationErrorContext::EntityAttribute { - uid: uid.clone(), - attr: k.clone(), - } - })?; + type_of_restricted_expr(expected_rexpr.as_borrowed(), self.extensions) + .map_err(|e| match e { + TypeOfRestrictedExprError::HeterogeneousSet(err) => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::HeterogeneousSet { + uid: uid.clone(), + attr: k.clone(), + err, + }, + ) + } + TypeOfRestrictedExprError::ExtensionFunctionLookup(err) => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::ExtensionFunctionLookup { + uid: uid.clone(), + attr: k.clone(), + err, + }, + ) + } + })?; let actual_rexpr = - vparser.val_into_rexpr(v.into(), Some(&expected_ty), || { + vparser.val_into_restricted_expr(v.into(), Some(&expected_ty), || { JsonDeserializationErrorContext::EntityAttribute { uid: uid.clone(), attr: k.clone(), @@ -327,53 +339,30 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { if actual_rexpr == *expected_rexpr { Ok((k, actual_rexpr)) } else { - Err(JsonDeserializationError::ActionDeclarationMismatch { - uid: uid.clone(), - }) + Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::ActionDeclarationMismatch { + uid: uid.clone(), + }, + )) } } }) .collect::>()?; let is_parent_allowed = |parent_euid: &EntityUID| { - match &entity_schema_info { - EntitySchemaInfo::NoSchema => { - if etype.is_action() { - if parent_euid.is_action() { - Ok(()) - } else { - Err(JsonDeserializationError::ActionParentIsNotAction { - uid: uid.clone(), - parent: parent_euid.clone(), - }) - } - } else { - Ok(()) // all parents are allowed - } - } - EntitySchemaInfo::Action(action) => { - // allowed iff the schema's copy also has this parent edge - if action.is_descendant_of(parent_euid) { - Ok(()) - } else { - Err(JsonDeserializationError::ActionDeclarationMismatch { - uid: uid.clone(), - }) - } - } - EntitySchemaInfo::NonAction(desc) => { - let parent_type = parent_euid.entity_type(); - if desc.allowed_parent_types().contains(parent_type) { - Ok(()) - } else { - Err(JsonDeserializationError::InvalidParentType { - ctx: Box::new(JsonDeserializationErrorContext::EntityParents { - uid: uid.clone(), - }), - uid: uid.clone(), - parent_ty: Box::new(parent_type.clone()), - }) - } + // full validation isn't done in this function (see doc comments on + // this function), but we do need to do the following check which + // happens even when there is no schema + if etype.is_action() { + if parent_euid.is_action() { + Ok(()) + } else { + Err(JsonDeserializationError::ActionParentIsNotAction { + uid: uid.clone(), + parent: parent_euid.clone(), + }) } + } else { + Ok(()) // all parents are allowed } }; let parents = ejson @@ -391,17 +380,6 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { }) }) .collect::>()?; - match &entity_schema_info { - EntitySchemaInfo::NoSchema => {} // no checks to do - EntitySchemaInfo::NonAction(_) => {} // no checks to do - EntitySchemaInfo::Action(action) => { - // check that the json entity and the schema declaration - // fully agree on parents - if parents != *action.ancestors_set() { - return Err(JsonDeserializationError::ActionDeclarationMismatch { uid }); - } - } - } Ok(Entity::new(uid, attrs, parents)) } } @@ -458,7 +436,7 @@ mod test { "parents": [] } ]); - let eparser: EntityJsonParser<'_, NoEntitiesSchema> = + let eparser: EntityJsonParser<'_, '_, NoEntitiesSchema> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let e = eparser.from_json_value(json); let bad_euid: EntityUID = r#"User::"alice""#.parse().unwrap(); diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index 67535bb2d4..3e72f06d48 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -17,9 +17,8 @@ use std::fmt::Display; use super::SchemaType; -use crate::ast::{ - EntityType, EntityUID, Expr, ExprKind, Name, PolicyID, RestrictedExpr, RestrictedExprError, -}; +use crate::ast::{EntityUID, Expr, ExprKind, Name, PolicyID, RestrictedExpr, RestrictedExprError}; +use crate::entities::conformance::{EntitySchemaConformanceError, HeterogeneousSetError}; use crate::extensions::ExtensionFunctionLookupError; use crate::parser::err::ParseErrors; use either::Either; @@ -64,9 +63,6 @@ pub enum JsonDeserializationError { /// Restricted expression error #[error(transparent)] RestrictedExpressionError(#[from] RestrictedExprError), - /// An error occurred when looking up an extension function - #[error(transparent)] - FailedExtensionFunctionLookup(#[from] ExtensionFunctionLookupError), /// A field that needs to be a literal entity reference, was some other JSON value #[error("{ctx}, expected a literal entity reference, but got `{}`", display_json_value(.got.as_ref()))] ExpectedLiteralEntityRef { @@ -108,45 +104,21 @@ pub enum JsonDeserializationError { /// argument type of the constructor we were looking for arg_type: Box, }, - /// During schema-based parsing, encountered an entity of a type which is - /// not declared in the schema. Note that this error is only used for non-Action entity types. - #[error("entity `{uid}` has type `{}` which is not declared in the schema{}", - &.uid.entity_type(), - match .suggested_types.as_slice() { - [] => String::new(), - [ty] => format!(". Did you mean `{ty}`?"), - tys => format!(". Did you mean one of {:?}?", tys.iter().map(ToString::to_string).collect::>()) - } - )] - UnexpectedEntityType { - /// Entity that had the unexpected type - uid: EntityUID, - /// Suggested similar entity types that actually are declared in the schema (if any) - suggested_types: Vec, - }, - /// During schema-based parsing, encountered an action which was not - /// declared in the schema - #[error("found action entity `{uid}`, but it was not declared as an action in the schema")] - UndeclaredAction { - /// Action which was not declared in the schema - uid: EntityUID, - }, - /// During schema-based parsing, encountered an action whose definition - /// doesn't precisely match the schema's declaration of that action - #[error("definition of action `{uid}` does not match its schema declaration")] - ActionDeclarationMismatch { - /// Action whose definition mismatched between entity data and schema - uid: EntityUID, - }, - /// During schema-based parsing, encountered this attribute on this entity, but that - /// attribute shouldn't exist on entities of this type - #[error("attribute `{attr}` on `{uid}` should not exist according to the schema")] - UnexpectedEntityAttr { - /// Entity that had the unexpected attribute - uid: EntityUID, - /// Name of the attribute that was unexpected - attr: SmolStr, + /// The same key appears two or more times in a single record literal + #[error("{ctx}, duplicate key `{key}` in record literal")] + DuplicateKeyInRecordLiteral { + /// Context of this error + ctx: Box, + /// The key that appeared two or more times + key: SmolStr, }, + /// During schema-based parsing, encountered an entity which does not + /// conform to the schema. + /// + /// This error contains the `Entity` analogues some of the other errors + /// listed below, among other things. + #[error(transparent)] + EntitySchemaConformance(EntitySchemaConformanceError), /// During schema-based parsing, encountered this attribute on a record, but /// that attribute shouldn't exist on that record #[error("{ctx}, record attribute `{record_attr}` should not exist according to the schema")] @@ -156,15 +128,6 @@ pub enum JsonDeserializationError { /// Name of the (Record) attribute which was unexpected record_attr: SmolStr, }, - /// During schema-based parsing, didn't encounter this attribute of an - /// entity, but that attribute should have existed - #[error("expected entity `{uid}` to have an attribute `{attr}`, but it does not")] - MissingRequiredEntityAttr { - /// Entity that is missing a required attribute - uid: EntityUID, - /// Name of the attribute which was expected - attr: SmolStr, - }, /// During schema-based parsing, didn't encounter this attribute of a /// record, but that attribute should have existed #[error("{ctx}, expected the record to have an attribute `{record_attr}`, but it does not")] @@ -174,48 +137,58 @@ pub enum JsonDeserializationError { /// Name of the (Record) attribute which was expected record_attr: SmolStr, }, - /// During schema-based parsing, the given attribute on the given entity had - /// a different type than the schema indicated to expect - #[error("{ctx}, type mismatch: attribute was expected to have type {expected}, but actually has type {actual}")] + /// During schema-based parsing, found a different type than the schema indicated. + /// + /// (This is used in all cases except inside entity attributes; type mismatches in + /// entity attributes are reported as `Self::EntitySchemaConformance`. As of + /// this writing, that means this should only be used for schema-based + /// parsing of the `Context`.) + #[error("{ctx}, type mismatch: expected type {expected}, but actually has type {actual}")] TypeMismatch { - /// Context of this error + /// Context of this error, which will be something other than `EntityAttribute`. + /// (Type mismatches in entity attributes are reported as + /// `Self::EntitySchemaConformance`.) ctx: Box, /// Type which was expected expected: Box, /// Type which was encountered instead actual: Box, }, - /// During schema-based parsing, found a set whose elements don't all have the - /// same type. This doesn't match any possible schema. - #[error("{ctx}, set elements have different types: {ty1} and {ty2}")] + /// During schema-based parsing, found a set whose elements don't all have + /// the same type. This doesn't match any possible schema. + /// + /// (This is used in all cases except inside entity attributes; + /// heterogeneous sets in entity attributes are reported as + /// `Self::EntitySchemaConformance`. As of this writing, that means this + /// should only be used for schema-based parsing of the `Context`. Note that + /// for non-schema-based parsing, heterogeneous sets are not an error.) + #[error("{ctx}, {err}")] HeterogeneousSet { - /// Context of this error - ctx: Box, - /// First element type which was found - ty1: Box, - /// Second element type which was found - ty2: Box, - }, - /// The same key appears two or more times in a single record literal - #[error("{ctx}, duplicate key `{key}` in record literal")] - DuplicateKeyInRecordLiteral { - /// Context of this error + /// Context of this error, which will be something other than `EntityAttribute`. + /// (Heterogeneous sets in entity attributes are reported as + /// `Self::EntitySchemaConformance`.) ctx: Box, - /// The key that appeared two or more times - key: SmolStr, + /// Underlying error + err: HeterogeneousSetError, }, - /// During schema-based parsing, found a parent of a type that's not allowed - /// for that entity - #[error( - "{ctx}, `{uid}` is not allowed to have a parent of type `{parent_ty}` according to the schema" - )] - InvalidParentType { - /// Context of this error + /// During schema-based parsing, error looking up an extension function. + /// This error can occur during schema-based parsing because that may + /// require getting information about any extension functions referenced in + /// the JSON. + /// + /// (This is used in all cases except inside entity attributes; extension + /// function lookup errors in entity attributes are reported as + /// `Self::EntitySchemaConformance`. As of this writing, that means this + /// should only be used for schema-based parsing of the `Context`.) + #[error("{ctx}, {err}")] + ExtensionFunctionLookup { + /// Context of this error, which will be something other than + /// `EntityAttribute`. + /// (Extension function lookup errors in entity attributes are reported + /// as `Self::EntitySchemaConformance`.) ctx: Box, - /// Entity that has an invalid parent type - uid: EntityUID, - /// Parent type which was invalid - parent_ty: Box, // boxed to avoid this variant being very large (and thus all JsonDeserializationErrors being large) + /// Underlying error + err: ExtensionFunctionLookupError, }, /// Raised when a JsonValue contains the no longer supported `__expr` escape #[error("{0}, invalid escape. The `__expr` escape is no longer supported")] diff --git a/cedar-policy-core/src/entities/json/schema.rs b/cedar-policy-core/src/entities/json/schema.rs index 0b70690443..36c8a6a37e 100644 --- a/cedar-policy-core/src/entities/json/schema.rs +++ b/cedar-policy-core/src/entities/json/schema.rs @@ -9,6 +9,9 @@ pub trait Schema { /// Type returned by `entity_type()`. Must implement the `EntityTypeDescription` trait type EntityTypeDescription: EntityTypeDescription; + /// Type returned by `action_entities()` + type ActionEntityIterator: IntoIterator>; + /// Get an `EntityTypeDescription` for the given entity type, or `None` if that /// entity type is not declared in the schema (in which case entities of that /// type should not appear in the JSON data). @@ -25,6 +28,9 @@ pub trait Schema { &'a self, basename: &'a Id, ) -> Box + 'a>; + + /// Get all the actions declared in the schema + fn action_entities(&self) -> Self::ActionEntityIterator; } /// Simple type that implements `Schema` by expecting no entities to exist at all @@ -32,6 +38,7 @@ pub trait Schema { pub struct NoEntitiesSchema; impl Schema for NoEntitiesSchema { type EntityTypeDescription = NullEntityTypeDescription; + type ActionEntityIterator = std::iter::Empty>; fn entity_type(&self, _entity_type: &EntityType) -> Option { None } @@ -44,15 +51,25 @@ impl Schema for NoEntitiesSchema { ) -> Box + 'a> { Box::new(std::iter::empty()) } + fn action_entities(&self) -> std::iter::Empty> { + std::iter::empty() + } } /// Simple type that implements `Schema` by allowing entities of all types to /// exist, and allowing all actions to exist, but expecting no attributes or -/// parents on any entity (action or otherwise) +/// parents on any entity (action or otherwise). +/// +/// This type returns an empty iterator for `action_entities()`, which is kind +/// of inconsistent with its behavior on `action()`. But it works out -- the +/// result is that, in `EntityJsonParser`, all actions encountered in JSON data +/// are allowed to exist without error, but no additional actions from the +/// schema are added. #[derive(Debug, Clone)] pub struct AllEntitiesNoAttrsSchema; impl Schema for AllEntitiesNoAttrsSchema { type EntityTypeDescription = NullEntityTypeDescription; + type ActionEntityIterator = std::iter::Empty>; fn entity_type(&self, entity_type: &EntityType) -> Option { Some(NullEntityTypeDescription { ty: entity_type.clone(), @@ -73,6 +90,9 @@ impl Schema for AllEntitiesNoAttrsSchema { Name::unqualified_name(basename.clone()), ))) } + fn action_entities(&self) -> std::iter::Empty> { + std::iter::empty() + } } /// Trait for a schema's description of an individual entity type diff --git a/cedar-policy-core/src/entities/json/value.rs b/cedar-policy-core/src/entities/json/value.rs index b4aa59465b..25f71d8248 100644 --- a/cedar-policy-core/src/entities/json/value.rs +++ b/cedar-policy-core/src/entities/json/value.rs @@ -15,21 +15,22 @@ */ use super::{ - AttributeType, JsonDeserializationError, JsonDeserializationErrorContext, - JsonSerializationError, SchemaType, + JsonDeserializationError, JsonDeserializationErrorContext, JsonSerializationError, SchemaType, }; use crate::ast::{ BorrowedRestrictedExpr, Eid, EntityUID, ExprConstructionError, ExprKind, Literal, Name, RestrictedExpr, }; -use crate::entities::EscapeKind; -use crate::extensions::{ExtensionFunctionLookupError, Extensions}; +use crate::entities::{ + type_of_restricted_expr, EntitySchemaConformanceError, EscapeKind, TypeOfRestrictedExprError, +}; +use crate::extensions::Extensions; use crate::FromNormalizedStr; use either::Either; use serde::{Deserialize, Serialize}; use serde_with::serde_as; use smol_str::SmolStr; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashSet}; /// The canonical JSON representation of a Cedar value. /// Many Cedar values have a natural one-to-one mapping to and from JSON values. @@ -357,8 +358,9 @@ impl<'e> ValueParser<'e> { /// internal function that converts a Cedar value (in JSON) into a /// `RestrictedExpr`. Performs schema-based parsing if `expected_ty` is - /// provided. - pub fn val_into_rexpr( + /// provided. This does not mean that this function fully validates the + /// value against `expected_ty` -- it does not. + pub fn val_into_restricted_expr( &self, val: serde_json::Value, expected_ty: Option<&SchemaType>, @@ -387,19 +389,42 @@ impl<'e> ValueParser<'e> { serde_json::Value::Array(elements) => Ok(RestrictedExpr::set( elements .into_iter() - .map(|element| self.val_into_rexpr(element, Some(element_ty), ctx.clone())) + .map(|element| { + self.val_into_restricted_expr(element, Some(element_ty), ctx.clone()) + }) .collect::, JsonDeserializationError>>()?, )), - _ => Err(JsonDeserializationError::TypeMismatch { - ctx: Box::new(ctx()), - expected: Box::new(expected_ty.clone()), - actual: { + _ => { + let expected = Box::new(expected_ty.clone()); + let actual = { let jvalue: CedarValueJson = serde_json::from_value(val)?; - Box::new( - self.type_of_rexpr(jvalue.into_expr(ctx.clone())?.as_borrowed(), ctx)?, + let ty = type_of_restricted_expr( + jvalue.into_expr(ctx.clone())?.as_borrowed(), + self.extensions, ) - }, - }), + .map_err(|e| { + type_of_restricted_expr_error_to_json_deserialization_error(e, ctx()) + })?; + Box::new(ty) + }; + match ctx() { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::TypeMismatch { + uid, + attr, + expected, + actual, + }, + )) + } + ctx => Err(JsonDeserializationError::TypeMismatch { + ctx: Box::new(ctx), + expected, + actual, + }), + } + } }, // The expected type is a record type. No special parsing rules // apply, but we need to parse the attribute values according to @@ -417,7 +442,7 @@ impl<'e> ValueParser<'e> { .filter_map(move |(k, expected_attr_ty)| { match mut_actual_attrs.remove(k.as_str()) { Some(actual_attr) => { - match self.val_into_rexpr(actual_attr, Some(expected_attr_ty.schema_type()), ctx.clone()) { + match self.val_into_restricted_expr(actual_attr, Some(expected_attr_ty.schema_type()), ctx.clone()) { Ok(actual_attr) => Some(Ok((k.clone(), actual_attr))), Err(e) => Some(Err(e)), } @@ -451,16 +476,37 @@ impl<'e> ValueParser<'e> { } }) } - _ => Err(JsonDeserializationError::TypeMismatch { - ctx: Box::new(ctx()), - expected: Box::new(expected_ty.clone()), - actual: { + _ => { + let expected = Box::new(expected_ty.clone()); + let actual = { let jvalue: CedarValueJson = serde_json::from_value(val)?; - Box::new( - self.type_of_rexpr(jvalue.into_expr(ctx.clone())?.as_borrowed(), ctx)?, + let ty = type_of_restricted_expr( + jvalue.into_expr(ctx.clone())?.as_borrowed(), + self.extensions, ) - }, - }), + .map_err(|e| { + type_of_restricted_expr_error_to_json_deserialization_error(e, ctx()) + })?; + Box::new(ty) + }; + match ctx() { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::TypeMismatch { + uid, + attr, + expected, + actual, + }, + )) + } + ctx => Err(JsonDeserializationError::TypeMismatch { + ctx: Box::new(ctx), + expected, + actual, + }), + } + } }, // The expected type is any other type, or we don't have an expected type. // No special parsing rules apply; we do ordinary, non-schema-based parsing. @@ -502,7 +548,10 @@ impl<'e> ValueParser<'e> { } ExtnValueJson::ImplicitConstructor(val) => { let arg = val.into_expr(ctx.clone())?; - let argty = self.type_of_rexpr(arg.as_borrowed(), ctx.clone())?; + let argty = + type_of_restricted_expr(arg.as_borrowed(), self.extensions).map_err(|e| { + type_of_restricted_expr_error_to_json_deserialization_error(e, ctx()) + })?; let func = self .extensions .lookup_single_arg_constructor( @@ -510,7 +559,11 @@ impl<'e> ValueParser<'e> { name: expected_typename.clone(), }, &argty, - )? + ) + .map_err(|err| JsonDeserializationError::ExtensionFunctionLookup { + ctx: Box::new(ctx()), + err, + })? .ok_or_else(|| JsonDeserializationError::MissingImpliedConstructor { ctx: Box::new(ctx()), return_type: Box::new(SchemaType::Extension { @@ -525,69 +578,35 @@ impl<'e> ValueParser<'e> { } } } +} - /// Get the `SchemaType` of a restricted expression. - /// - /// This isn't possible for general `Expr`s (without a Request, full schema, - /// etc), but is possible for restricted expressions, given the information - /// in `Extensions`. - pub fn type_of_rexpr( - &self, - rexpr: BorrowedRestrictedExpr<'_>, - ctx: impl Fn() -> JsonDeserializationErrorContext + Clone, - ) -> Result { - match rexpr.expr_kind() { - ExprKind::Lit(Literal::Bool(_)) => Ok(SchemaType::Bool), - ExprKind::Lit(Literal::Long(_)) => Ok(SchemaType::Long), - ExprKind::Lit(Literal::String(_)) => Ok(SchemaType::String), - ExprKind::Lit(Literal::EntityUID(uid)) => Ok(SchemaType::Entity { ty: uid.entity_type().clone() }), - ExprKind::Set(elements) => { - let mut element_types = elements.iter().map(|el| { - self.type_of_rexpr(BorrowedRestrictedExpr::new_unchecked(el), ctx.clone()) // assuming the invariant holds for the set as a whole, it will also hold for each element - }); - match element_types.next() { - None => Ok(SchemaType::EmptySet), - Some(Err(e)) => Err(e), - Some(Ok(element_ty)) => { - let matches_element_ty = |ty: &Result| matches!(ty, Ok(ty) if ty.is_consistent_with(&element_ty)); - let conflicting_ty = element_types.find(|ty| !matches_element_ty(ty)); - match conflicting_ty { - None => Ok(SchemaType::Set { element_ty: Box::new(element_ty) }), - Some(Ok(conflicting_ty)) => - Err(JsonDeserializationError::HeterogeneousSet { - ctx: Box::new(ctx()), - ty1: Box::new(element_ty), - ty2: Box::new(conflicting_ty), - }), - Some(Err(e)) => Err(e), - } - } - } - } - ExprKind::Record(map) => { - Ok(SchemaType::Record { attrs: { - map.iter().map(|(k, v)| { - let attr_type = self.type_of_rexpr( - BorrowedRestrictedExpr::new_unchecked(v), // assuming the invariant holds for the record as a whole, it will also hold for each attribute value - ctx.clone(), - )?; - // we can't know if the attribute is required or optional, - // but marking it optional is more flexible -- allows the - // attribute type to `is_consistent_with()` more types - Ok((k.clone(), AttributeType::optional(attr_type))) - }).collect::, JsonDeserializationError>>()? - }}) +fn type_of_restricted_expr_error_to_json_deserialization_error( + torerr: TypeOfRestrictedExprError, + ctx: JsonDeserializationErrorContext, +) -> JsonDeserializationError { + match torerr { + TypeOfRestrictedExprError::HeterogeneousSet(err) => match ctx { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::HeterogeneousSet { uid, attr, err }, + ) } - ExprKind::ExtensionFunctionApp { fn_name, .. } => { - let efunc = self.extensions.func(fn_name)?; - Ok(efunc.return_type().cloned().ok_or_else(|| ExtensionFunctionLookupError::HasNoType { - name: efunc.name().clone() - })?) + ctx => JsonDeserializationError::HeterogeneousSet { + ctx: Box::new(ctx), + err, + }, + }, + TypeOfRestrictedExprError::ExtensionFunctionLookup(err) => match ctx { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::ExtensionFunctionLookup { uid, attr, err }, + ) } - // PANIC SAFETY. Unreachable by invariant on restricted expressions - #[allow(clippy::unreachable)] - expr => unreachable!("internal invariant violation: BorrowedRestrictedExpr somehow contained this expr case: {expr:?}"), - } + ctx => JsonDeserializationError::ExtensionFunctionLookup { + ctx: Box::new(ctx), + err, + }, + }, } } diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index 37e32223fe..ed2a34cf7b 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -842,7 +842,7 @@ pub mod test { use super::*; use crate::{ - entities::{EntityJsonParser, TCComputation}, + entities::{EntityJsonParser, NoEntitiesSchema, TCComputation}, parser::{self, parse_policyset}, parser::{parse_expr, parse_policy_template}, }; @@ -879,7 +879,9 @@ pub mod test { Entity::with_uid(EntityUID::with_eid("test_action")), Entity::with_uid(EntityUID::with_eid("test_resource")), ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::none(), ) .expect("failed to create basic entities") } @@ -932,7 +934,9 @@ pub mod test { sibling, unrelated, ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::all_available(), ) .expect("Failed to create rich entities") } @@ -3043,8 +3047,13 @@ pub mod test { let mut alice = Entity::with_uid(EntityUID::with_eid("Alice")); let parent = Entity::with_uid(EntityUID::with_eid("Friends")); alice.add_ancestor(parent.uid()); - let entities = Entities::from_entities(vec![alice], TCComputation::AssumeAlreadyComputed) - .expect("failed to create basic entities"); + let entities = Entities::from_entities( + vec![alice], + None::<&NoEntitiesSchema>, + TCComputation::AssumeAlreadyComputed, + Extensions::all_available(), + ) + .expect("failed to create basic entities"); let exts = Extensions::none(); let eval = Evaluator::new(&request, &entities, &exts).expect("failed to create evaluator"); assert_eq!( @@ -3547,7 +3556,7 @@ pub mod test { fn eval_and_or() -> Result<()> { use crate::parser; let request = basic_request(); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::none(), TCComputation::ComputeNow); let entities = eparser.from_json_str("[]").expect("empty slice"); let exts = Extensions::none(); @@ -3660,7 +3669,7 @@ pub mod test { #[test] fn template_env_tests() { let request = basic_request(); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::none(), TCComputation::ComputeNow); let entities = eparser.from_json_str("[]").expect("empty slice"); let exts = Extensions::none(); @@ -3717,7 +3726,7 @@ pub mod test { EntityUID::with_eid("r"), Context::empty(), ); - let eparser: EntityJsonParser<'_> = + let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::none(), TCComputation::ComputeNow); let entities = eparser.from_json_str("[]").expect("empty slice"); let exts = Extensions::none(); diff --git a/cedar-policy-core/src/extensions.rs b/cedar-policy-core/src/extensions.rs index c42650927a..d51e8a4fc7 100644 --- a/cedar-policy-core/src/extensions.rs +++ b/cedar-policy-core/src/extensions.rs @@ -40,7 +40,7 @@ lazy_static::lazy_static! { /// Holds data on all the Extensions which are active for a given evaluation. /// /// Clone is cheap for this type. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Copy)] pub struct Extensions<'a> { /// the actual extensions extensions: &'a [Extension], diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 673bf9f79d..c2f817214b 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -26,6 +26,7 @@ use std::sync::Arc; use cedar_policy_core::{ ast::{Entity, EntityType, EntityUID, Id, Name}, entities::{Entities, TCComputation}, + extensions::Extensions, transitive_closure::compute_tc, }; use serde::{Deserialize, Serialize}; @@ -546,7 +547,8 @@ impl ValidatorSchema { }) } - /// Construct an `Entity` object for each action in the schema + /// Invert the action hierarchy to get the ancestor relation expected for + /// the `Entity` datatype instead of descendants as stored by the schema. fn action_entities_iter(&self) -> impl Iterator + '_ { // We could store the un-inverted `memberOf` relation for each action, // but I [john-h-kastner-aws] judge that the current implementation is @@ -571,12 +573,13 @@ impl ValidatorSchema { }) } - /// Invert the action hierarchy to get the ancestor relation expected for - /// the `Entity` datatype instead of descendant as stored by the schema. + /// Construct an `Entity` object for each action in the schema pub fn action_entities(&self) -> cedar_policy_core::entities::Result { Entities::from_entities( self.action_entities_iter(), + None::<&cedar_policy_core::entities::NoEntitiesSchema>, // we don't want to tell `Entities::from_entities()` to add the schema's action entities, that would infinitely recurse TCComputation::AssumeAlreadyComputed, + Extensions::all_available(), ) } } @@ -607,6 +610,7 @@ impl<'a> CoreSchema<'a> { impl<'a> cedar_policy_core::entities::Schema for CoreSchema<'a> { type EntityTypeDescription = EntityTypeDescription; + type ActionEntityIterator = Vec>; fn entity_type( &self, @@ -636,6 +640,10 @@ impl<'a> cedar_policy_core::entities::Schema for CoreSchema<'a> { } })) } + + fn action_entities(&self) -> Self::ActionEntityIterator { + self.actions.values().map(Arc::clone).collect() + } } /// Struct which carries enough information that it can impl Core's `EntityTypeDescription` diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 12743230f5..dc70befe03 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -22,7 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Removed `__expr` escape from Cedar JSON formats - Rename `cedar_policy_core::est::EstToAstError` to `cedar_policy_core::est::FromJsonError`. -- Rename `cedar_policy_core::entities::JsonDeserializationError::ExtensionsError` to `cedar_policy_core::entities::JsonDeserializationError::FailedExtensionsFunctionLookup`. +- Rename `cedar_policy_core::entities::JsonDeserializationError::ExtensionsError` to `cedar_policy_core::entities::JsonDeserializationError::ExtensionFunctionLookup`. - Rename variants in `cedar_policy::SchemaError`. - `Diagnostics::errors()` now returns an iterator over `AuthorizationError`s. - `Response::new()` now expects a `Vec` as its third argument. @@ -31,6 +31,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Implement [RFC 20](https://github.com/cedar-policy/rfcs/blob/main/text/0020-unique-record-keys.md), disallowing duplicate keys in record values (including record literals in policies, request `context`, and records in entity attributes). +- `Entities::from_*()` methods now automatically add action entities present in the `schema` + to the constructed `Entities`, if a `schema` is provided. +- `Entities::from_*()` methods now validate the entities against the `schema`, if a `schema` + is provided. +- `Entities::from_entities()` and `Entities::add_entities()` now take an optional schema argument. - Change the semantics of equality for IP ranges. For example, `ip("192.168.0.1/24") == ip("192.168.0.3/24")` was previously `true` and is now `false`. The behavior of equality on single IP addresses is unchanged, and so is diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index ca92174a13..038f5ecd2b 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -26,9 +26,9 @@ use cedar_policy_core::ast; use cedar_policy_core::ast::{ExprConstructionError, RestrictedExprError}; use cedar_policy_core::authorizer; pub use cedar_policy_core::authorizer::AuthorizationError; -use cedar_policy_core::entities; -use cedar_policy_core::entities::JsonDeserializationErrorContext; -use cedar_policy_core::entities::{ContextSchema, Dereference, JsonDeserializationError}; +use cedar_policy_core::entities::{ + self, ContextSchema, Dereference, JsonDeserializationError, JsonDeserializationErrorContext, +}; use cedar_policy_core::est; pub use cedar_policy_core::evaluator::{EvaluationError, EvaluationErrorKind}; use cedar_policy_core::evaluator::{Evaluator, RestrictedEvaluator}; @@ -246,111 +246,178 @@ impl Entities { } /// Create an `Entities` object with the given entities. - /// It will error if the entities cannot be read or if the entities hierarchy is cyclic + /// + /// `schema` represents a source of `Action` entities, which will be added + /// to the entities provided. + /// (If any `Action` entities are present in the provided entities, and a + /// `schema` is also provided, each `Action` entity in the provided entities + /// must exactly match its definition in the schema or an error is + /// returned.) + /// + /// If a `schema` is present, this function will also ensure that the + /// produced entities fully conform to the `schema` -- for instance, it will + /// error if attributes have the wrong types (e.g., string instead of + /// integer), or if required attributes are missing or superfluous + /// attributes are provided. pub fn from_entities( entities: impl IntoIterator, + schema: Option<&Schema>, ) -> Result { entities::Entities::from_entities( entities.into_iter().map(|e| e.0), + schema + .map(|s| cedar_policy_validator::CoreSchema::new(&s.0)) + .as_ref(), entities::TCComputation::ComputeNow, + Extensions::all_available(), ) .map(Entities) } - /// Add all of the [`Entity`]s in the collection to this [`Entities`] structure, re-computing the transitive closure - /// Re-computing the transitive closure can be expensive, so it is advised to not call this method in a loop + /// Add all of the [`Entity`]s in the collection to this [`Entities`] + /// structure, re-computing the transitive closure. + /// + /// If a `schema` is provided, this method will ensure that the added + /// entities fully conform to the schema -- for instance, it will error if + /// attributes have the wrong types (e.g., string instead of integer), or if + /// required attributes are missing or superfluous attributes are provided. + /// (This method will not add action entities from the `schema`.) + /// + /// Re-computing the transitive closure can be expensive, so it is advised + /// to not call this method in a loop. pub fn add_entities( self, entities: impl IntoIterator, + schema: Option<&Schema>, ) -> Result { - Ok(Self(self.0.add_entities( - entities.into_iter().map(|e| e.0), - entities::TCComputation::ComputeNow, - )?)) + Ok(Self( + self.0.add_entities( + entities.into_iter().map(|e| e.0), + schema + .map(|s| cedar_policy_validator::CoreSchema::new(&s.0)) + .as_ref(), + entities::TCComputation::ComputeNow, + Extensions::all_available(), + )?, + )) } - /// Parse an entities JSON file (in [&str] form) and add them into this [`Entities`] structure, re-computing the transitive closure + /// Parse an entities JSON file (in [&str] form) and add them into this + /// [`Entities`] structure, re-computing the transitive closure /// /// If a `schema` is provided, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit, and it will error - /// if attributes have the wrong types (e.g., string instead of integer). - /// Re-computing the transitive closure can be expensive, so it is advised to not call this method in a loop + /// will allow `__entity` and `__extn` escapes to be implicit. + /// This method will also ensure that the added entities fully conform to the + /// schema -- for instance, it will error if attributes have the wrong types + /// (e.g., string instead of integer), or if required attributes are missing + /// or superfluous attributes are provided. + /// (This method will not add action entities from the `schema`.) + /// + /// Re-computing the transitive closure can be expensive, so it is advised + /// to not call this method in a loop. pub fn add_entities_from_json_str( self, json: &str, schema: Option<&Schema>, ) -> Result { + let schema = schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)); let eparser = entities::EntityJsonParser::new( - schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)), + schema.as_ref(), Extensions::all_available(), entities::TCComputation::ComputeNow, ); - let new_entities = eparser - .iter_from_json_str(json)? - .collect::, _>>()?; + let new_entities = eparser.iter_from_json_str(json)?; Ok(Self(self.0.add_entities( new_entities, + schema.as_ref(), entities::TCComputation::ComputeNow, + Extensions::all_available(), )?)) } - /// Parse an entities JSON file (in [`serde_json::Value`] form) and add them into this [`Entities`] structure, re-computing the transitive closure + /// Parse an entities JSON file (in [`serde_json::Value`] form) and add them + /// into this [`Entities`] structure, re-computing the transitive closure /// /// If a `schema` is provided, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit, and it will error - /// if attributes have the wrong types (e.g., string instead of integer). + /// will allow `__entity` and `__extn` escapes to be implicit. + /// This method will also ensure that the added entities fully conform to the + /// schema -- for instance, it will error if attributes have the wrong types + /// (e.g., string instead of integer), or if required attributes are missing + /// or superfluous attributes are provided. + /// (This method will not add action entities from the `schema`.) /// - /// Re-computing the transitive closure can be expensive, so it is advised to not call this method in a loop + /// Re-computing the transitive closure can be expensive, so it is advised + /// to not call this method in a loop. pub fn add_entities_from_json_value( self, json: serde_json::Value, schema: Option<&Schema>, ) -> Result { + let schema = schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)); let eparser = entities::EntityJsonParser::new( - schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)), + schema.as_ref(), Extensions::all_available(), entities::TCComputation::ComputeNow, ); - let new_entities = eparser - .iter_from_json_value(json)? - .collect::, _>>()?; + let new_entities = eparser.iter_from_json_value(json)?; Ok(Self(self.0.add_entities( new_entities, + schema.as_ref(), entities::TCComputation::ComputeNow, + Extensions::all_available(), )?)) } - /// Parse an entities JSON file (in [`std::io::Read`] form) and add them into this [`Entities`] structure, re-computing the transitive closure + /// Parse an entities JSON file (in [`std::io::Read`] form) and add them + /// into this [`Entities`] structure, re-computing the transitive closure /// /// If a `schema` is provided, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit, and it will error - /// if attributes have the wrong types (e.g., string instead of integer). + /// will allow `__entity` and `__extn` escapes to be implicit. + /// This method will also ensure that the added entities fully conform to the + /// schema -- for instance, it will error if attributes have the wrong types + /// (e.g., string instead of integer), or if required attributes are missing + /// or superfluous attributes are provided. + /// (This method will not add action entities from the `schema`.) /// - /// Re-computing the transitive closure can be expensive, so it is advised to not call this method in a loop + /// Re-computing the transitive closure can be expensive, so it is advised + /// to not call this method in a loop. pub fn add_entities_from_json_file( self, json: impl std::io::Read, schema: Option<&Schema>, ) -> Result { + let schema = schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)); let eparser = entities::EntityJsonParser::new( - schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)), + schema.as_ref(), Extensions::all_available(), entities::TCComputation::ComputeNow, ); - let new_entities = eparser - .iter_from_json_file(json)? - .collect::, _>>()?; + let new_entities = eparser.iter_from_json_file(json)?; Ok(Self(self.0.add_entities( new_entities, + schema.as_ref(), entities::TCComputation::ComputeNow, + Extensions::all_available(), )?)) } /// Parse an entities JSON file (in `&str` form) into an `Entities` object /// - /// If a `schema` is provided, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit, and it will error - /// if attributes have the wrong types (e.g., string instead of integer). + /// `schema` represents a source of `Action` entities, which will be added + /// to the entities parsed from JSON. + /// (If any `Action` entities are present in the JSON, and a `schema` is + /// also provided, each `Action` entity in the JSON must exactly match its + /// definition in the schema or an error is returned.) + /// + /// If a `schema` is present, this will also inform the parsing: for + /// instance, it will allow `__entity` and `__extn` escapes to be implicit. + /// + /// Finally, if a `schema` is present, this function will ensure + /// that the produced entities fully conform to the `schema` -- for + /// instance, it will error if attributes have the wrong types (e.g., string + /// instead of integer), or if required attributes are missing or + /// superfluous attributes are provided. + /// /// ``` /// use std::collections::HashMap; /// use std::str::FromStr; @@ -385,8 +452,9 @@ impl Entities { json: &str, schema: Option<&Schema>, ) -> Result { + let schema = schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)); let eparser = entities::EntityJsonParser::new( - schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)), + schema.as_ref(), Extensions::all_available(), entities::TCComputation::ComputeNow, ); @@ -396,9 +464,21 @@ impl Entities { /// Parse an entities JSON file (in `serde_json::Value` form) into an /// `Entities` object /// - /// If a `schema` is provided, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit, and it will error - /// if attributes have the wrong types (e.g., string instead of integer). + /// `schema` represents a source of `Action` entities, which will be added + /// to the entities parsed from JSON. + /// (If any `Action` entities are present in the JSON, and a `schema` is + /// also provided, each `Action` entity in the JSON must exactly match its + /// definition in the schema or an error is returned.) + /// + /// If a `schema` is present, this will also inform the parsing: for + /// instance, it will allow `__entity` and `__extn` escapes to be implicit. + /// + /// Finally, if a `schema` is present, this function will ensure + /// that the produced entities fully conform to the `schema` -- for + /// instance, it will error if attributes have the wrong types (e.g., string + /// instead of integer), or if required attributes are missing or + /// superfluous attributes are provided. + /// /// ``` /// use std::collections::HashMap; /// use std::str::FromStr; @@ -426,8 +506,9 @@ impl Entities { json: serde_json::Value, schema: Option<&Schema>, ) -> Result { + let schema = schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)); let eparser = entities::EntityJsonParser::new( - schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)), + schema.as_ref(), Extensions::all_available(), entities::TCComputation::ComputeNow, ); @@ -437,15 +518,27 @@ impl Entities { /// Parse an entities JSON file (in `std::io::Read` form) into an `Entities` /// object /// - /// If a `schema` is provided, this will inform the parsing: for instance, it - /// will allow `__entity` and `__extn` escapes to be implicit, and it will error - /// if attributes have the wrong types (e.g., string instead of integer). + /// `schema` represents a source of `Action` entities, which will be added + /// to the entities parsed from JSON. + /// (If any `Action` entities are present in the JSON, and a `schema` is + /// also provided, each `Action` entity in the JSON must exactly match its + /// definition in the schema or an error is returned.) + /// + /// If a `schema` is present, this will also inform the parsing: for + /// instance, it will allow `__entity` and `__extn` escapes to be implicit. + /// + /// Finally, if a `schema` is present, this function will ensure + /// that the produced entities fully conform to the `schema` -- for + /// instance, it will error if attributes have the wrong types (e.g., string + /// instead of integer), or if required attributes are missing or + /// superfluous attributes are provided. pub fn from_json_file( json: impl std::io::Read, schema: Option<&Schema>, ) -> Result { + let schema = schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)); let eparser = entities::EntityJsonParser::new( - schema.map(|s| cedar_policy_validator::CoreSchema::new(&s.0)), + schema.as_ref(), Extensions::all_available(), entities::TCComputation::ComputeNow, ); @@ -4568,7 +4661,7 @@ mod ancestors_tests { HashMap::new(), std::iter::once(b_euid.clone()).collect(), ); - let es = Entities::from_entities([a, b, c]).unwrap(); + let es = Entities::from_entities([a, b, c], None).unwrap(); let ans = es.ancestors(&c_euid).unwrap().collect::>(); assert_eq!(ans.len(), 2); assert!(ans.contains(&b_euid)); @@ -4576,6 +4669,326 @@ mod ancestors_tests { } } +/// A few tests of validating entities. +/// Many other validation-related tests are in the separate module focusing on +/// schema-based parsing. +#[cfg(test)] +mod entity_validate_tests { + use super::*; + use serde_json::json; + + fn schema() -> Schema { + Schema::from_json_value(json!( + {"": { + "entityTypes": { + "Employee": { + "memberOfTypes": [], + "shape": { + "type": "Record", + "attributes": { + "isFullTime": { "type": "Boolean" }, + "numDirectReports": { "type": "Long" }, + "department": { "type": "String" }, + "manager": { "type": "Entity", "name": "Employee" }, + "hr_contacts": { "type": "Set", "element": { + "type": "Entity", "name": "HR" } }, + "json_blob": { "type": "Record", "attributes": { + "inner1": { "type": "Boolean" }, + "inner2": { "type": "String" }, + "inner3": { "type": "Record", "attributes": { + "innerinner": { "type": "Entity", "name": "Employee" } + }} + }}, + "home_ip": { "type": "Extension", "name": "ipaddr" }, + "work_ip": { "type": "Extension", "name": "ipaddr" }, + "trust_score": { "type": "Extension", "name": "decimal" }, + "tricky": { "type": "Record", "attributes": { + "type": { "type": "String" }, + "id": { "type": "String" } + }} + } + } + }, + "HR": { + "memberOfTypes": [] + } + }, + "actions": { + "view": { } + } + }} + )) + .expect("should be a valid schema") + } + + fn validate_entity(entity: Entity, schema: &Schema) -> Result<(), entities::EntitiesError> { + let _ = Entities::from_entities([entity], Some(schema))?; + Ok(()) + } + + #[test] + fn valid_entity() { + let entity = Entity::new( + EntityUid::from_strs("Employee", "123"), + HashMap::from_iter([ + ("isFullTime".into(), RestrictedExpression::new_bool(false)), + ("numDirectReports".into(), RestrictedExpression::new_long(3)), + ( + "department".into(), + RestrictedExpression::new_string("Sales".into()), + ), + ( + "manager".into(), + RestrictedExpression::from_str(r#"Employee::"456""#).unwrap(), + ), + ("hr_contacts".into(), RestrictedExpression::new_set([])), + ( + "json_blob".into(), + RestrictedExpression::new_record([ + ("inner1".into(), RestrictedExpression::new_bool(false)), + ( + "inner2".into(), + RestrictedExpression::new_string("foo".into()), + ), + ( + "inner3".into(), + RestrictedExpression::new_record([( + "innerinner".into(), + RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), + )]) + .unwrap(), + ), + ]) + .unwrap(), + ), + ( + "home_ip".into(), + RestrictedExpression::from_str(r#"ip("10.20.30.40")"#).unwrap(), + ), + ( + "work_ip".into(), + RestrictedExpression::from_str(r#"ip("10.50.60.70")"#).unwrap(), + ), + ( + "trust_score".into(), + RestrictedExpression::from_str(r#"decimal("36.53")"#).unwrap(), + ), + ( + "tricky".into(), + RestrictedExpression::from_str(r#"{ type: "foo", id: "bar" }"#).unwrap(), + ), + ]), + HashSet::new(), + ); + validate_entity(entity, &schema()).unwrap(); + } + + #[test] + fn invalid_entities() { + let schema = schema(); + let entity = Entity::new( + EntityUid::from_strs("Employee", "123"), + HashMap::from_iter([ + ("isFullTime".into(), RestrictedExpression::new_bool(false)), + ("numDirectReports".into(), RestrictedExpression::new_long(3)), + ( + "department".into(), + RestrictedExpression::new_string("Sales".into()), + ), + ( + "manager".into(), + RestrictedExpression::from_str(r#"Employee::"456""#).unwrap(), + ), + ("hr_contacts".into(), RestrictedExpression::new_set([])), + ( + "json_blob".into(), + RestrictedExpression::new_record([ + ("inner1".into(), RestrictedExpression::new_bool(false)), + ( + "inner2".into(), + RestrictedExpression::new_string("foo".into()), + ), + ( + "inner3".into(), + RestrictedExpression::new_record([( + "innerinner".into(), + RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), + )]) + .unwrap(), + ), + ]) + .unwrap(), + ), + ( + "home_ip".into(), + RestrictedExpression::from_str(r#"ip("10.20.30.40")"#).unwrap(), + ), + ( + "work_ip".into(), + RestrictedExpression::from_str(r#"ip("10.50.60.70")"#).unwrap(), + ), + ( + "trust_score".into(), + RestrictedExpression::from_str(r#"decimal("36.53")"#).unwrap(), + ), + ( + "tricky".into(), + RestrictedExpression::from_str(r#"{ type: "foo", id: "bar" }"#).unwrap(), + ), + ]), + HashSet::from_iter([EntityUid::from_strs("Manager", "jane")]), + ); + match validate_entity(entity, &schema) { + Ok(_) => panic!("expected an error due to extraneous parent"), + Err(e) => { + assert!( + e.to_string().contains(r#"`Employee::"123"` is not allowed to have an ancestor of type `Manager` according to the schema"#), + "actual error message was {e}", + ) + } + } + + let entity = Entity::new( + EntityUid::from_strs("Employee", "123"), + HashMap::from_iter([ + ("isFullTime".into(), RestrictedExpression::new_bool(false)), + ( + "department".into(), + RestrictedExpression::new_string("Sales".into()), + ), + ( + "manager".into(), + RestrictedExpression::from_str(r#"Employee::"456""#).unwrap(), + ), + ("hr_contacts".into(), RestrictedExpression::new_set([])), + ( + "json_blob".into(), + RestrictedExpression::new_record([ + ("inner1".into(), RestrictedExpression::new_bool(false)), + ( + "inner2".into(), + RestrictedExpression::new_string("foo".into()), + ), + ( + "inner3".into(), + RestrictedExpression::new_record([( + "innerinner".into(), + RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), + )]) + .unwrap(), + ), + ]) + .unwrap(), + ), + ( + "home_ip".into(), + RestrictedExpression::from_str(r#"ip("10.20.30.40")"#).unwrap(), + ), + ( + "work_ip".into(), + RestrictedExpression::from_str(r#"ip("10.50.60.70")"#).unwrap(), + ), + ( + "trust_score".into(), + RestrictedExpression::from_str(r#"decimal("36.53")"#).unwrap(), + ), + ( + "tricky".into(), + RestrictedExpression::from_str(r#"{ type: "foo", id: "bar" }"#).unwrap(), + ), + ]), + HashSet::new(), + ); + match validate_entity(entity, &schema) { + Ok(_) => panic!("expected an error due to missing attribute `numDirectReports`"), + Err(e) => { + assert!( + e.to_string().contains(r#"expected entity `Employee::"123"` to have attribute `numDirectReports`, but it does not"#), + "actual error message was {e}", + ) + } + } + + let entity = Entity::new( + EntityUid::from_strs("Employee", "123"), + HashMap::from_iter([ + ("isFullTime".into(), RestrictedExpression::new_bool(false)), + ("extra".into(), RestrictedExpression::new_bool(true)), + ("numDirectReports".into(), RestrictedExpression::new_long(3)), + ( + "department".into(), + RestrictedExpression::new_string("Sales".into()), + ), + ( + "manager".into(), + RestrictedExpression::from_str(r#"Employee::"456""#).unwrap(), + ), + ("hr_contacts".into(), RestrictedExpression::new_set([])), + ( + "json_blob".into(), + RestrictedExpression::new_record([ + ("inner1".into(), RestrictedExpression::new_bool(false)), + ( + "inner2".into(), + RestrictedExpression::new_string("foo".into()), + ), + ( + "inner3".into(), + RestrictedExpression::new_record([( + "innerinner".into(), + RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), + )]) + .unwrap(), + ), + ]) + .unwrap(), + ), + ( + "home_ip".into(), + RestrictedExpression::from_str(r#"ip("10.20.30.40")"#).unwrap(), + ), + ( + "work_ip".into(), + RestrictedExpression::from_str(r#"ip("10.50.60.70")"#).unwrap(), + ), + ( + "trust_score".into(), + RestrictedExpression::from_str(r#"decimal("36.53")"#).unwrap(), + ), + ( + "tricky".into(), + RestrictedExpression::from_str(r#"{ type: "foo", id: "bar" }"#).unwrap(), + ), + ]), + HashSet::new(), + ); + match validate_entity(entity, &schema) { + Ok(_) => panic!("expected an error due to extraneous attribute"), + Err(e) => { + assert!( + e.to_string().contains(r#"attribute `extra` on `Employee::"123"` should not exist according to the schema"#), + "actual error message was {e}", + ) + } + } + + let entity = Entity::new( + EntityUid::from_strs("Manager", "jane"), + HashMap::new(), + HashSet::new(), + ); + match validate_entity(entity, &schema) { + Ok(_) => panic!("expected an error due to unexpected entity type"), + Err(e) => { + assert!( + e.to_string().contains(r#"entity `Manager::"jane"` has type `Manager` which is not declared in the schema"#), + "actual error message was {e}", + ) + } + } + } +} + /// The main unit tests for schema-based parsing live here, as they require both /// the Validator and Core packages working together. /// @@ -4712,7 +5125,8 @@ mod schema_based_parsing_tests { // but with schema-based parsing, we get these other types let parsed = Entities::from_json_value(entitiesjson, Some(&schema)) .expect("Should parse without error"); - assert_eq!(parsed.iter().count(), 1); + assert_eq!(parsed.iter().count(), 2); // Employee::"12UA45" and the one action + assert_eq!(parsed.iter().filter(|e| e.0.uid().is_action()).count(), 1); let parsed = parsed .get(&EntityUid::from_strs("Employee", "12UA45")) .expect("that should be the employee id"); @@ -5084,7 +5498,8 @@ mod schema_based_parsing_tests { ); let parsed = Entities::from_json_value(entitiesjson, Some(&schema)) .expect("Should parse without error"); - assert_eq!(parsed.iter().count(), 1); + assert_eq!(parsed.iter().count(), 2); // XYZCorp::Employee::"12UA45" and one action + assert_eq!(parsed.iter().filter(|e| e.0.uid().is_action()).count(), 1); let parsed = parsed .get(&EntityUid::from_strs("XYZCorp::Employee", "12UA45")) .expect("that should be the employee type and id"); @@ -5165,7 +5580,8 @@ mod schema_based_parsing_tests { ); let parsed = Entities::from_json_value(entitiesjson, Some(&schema)) .expect("Should parse without error"); - assert_eq!(parsed.iter().count(), 1); + assert_eq!(parsed.iter().count(), 2); // Employee::"12UA45" and one action + assert_eq!(parsed.iter().filter(|e| e.0.uid().is_action()).count(), 1); // "department" shouldn't be required let entitiesjson = json!( @@ -5182,7 +5598,8 @@ mod schema_based_parsing_tests { ); let parsed = Entities::from_json_value(entitiesjson, Some(&schema)) .expect("Should parse without error"); - assert_eq!(parsed.iter().count(), 1); + assert_eq!(parsed.iter().count(), 2); // Employee::"12UA45" and the one action + assert_eq!(parsed.iter().filter(|e| e.0.uid().is_action()).count(), 1); } /// Test that involves open entities @@ -5495,29 +5912,32 @@ mod schema_based_parsing_tests { assert_eq!( action_entities, - Entities::from_entities([ - Entity::new(a_euid.clone(), HashMap::new(), HashSet::new()), - Entity::new( - b_euid.clone(), - HashMap::new(), - HashSet::from([a_euid.clone()]) - ), - Entity::new( - c_euid.clone(), - HashMap::new(), - HashSet::from([a_euid.clone()]) - ), - Entity::new( - d_euid.clone(), - HashMap::new(), - HashSet::from([a_euid.clone(), b_euid.clone(), c_euid.clone()]) - ), - Entity::new( - e_euid, - HashMap::new(), - HashSet::from([a_euid, b_euid, c_euid, d_euid]) - ), - ]) + Entities::from_entities( + [ + Entity::new(a_euid.clone(), HashMap::new(), HashSet::new()), + Entity::new( + b_euid.clone(), + HashMap::new(), + HashSet::from([a_euid.clone()]) + ), + Entity::new( + c_euid.clone(), + HashMap::new(), + HashSet::from([a_euid.clone()]) + ), + Entity::new( + d_euid.clone(), + HashMap::new(), + HashSet::from([a_euid.clone(), b_euid.clone(), c_euid.clone()]) + ), + Entity::new( + e_euid, + HashMap::new(), + HashSet::from([a_euid, b_euid, c_euid, d_euid]) + ), + ], + Some(&schema) + ) .unwrap() ); }