From 2dca4c33d415681d3a0c866c8957f9aedb9c8e9c Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 25 Sep 2023 13:11:02 +0000 Subject: [PATCH 01/39] for entities parsed from json, add action entities from schema --- cedar-policy-core/src/entities.rs | 134 +++++++----------- .../src/entities/json/entities.rs | 117 ++++++++++----- cedar-policy-core/src/entities/json/schema.rs | 22 ++- cedar-policy-validator/src/schema.rs | 5 + cedar-policy/src/api.rs | 24 ++-- 5 files changed, 170 insertions(+), 132 deletions(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 1341aadf26..5dc8dee376 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -481,25 +481,19 @@ mod json_parsing_tests { let new = serde_json::json!([ {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"george\"", "Test::\"janet\""]}]); - 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, TCComputation::EnforceAlreadyComputed); // 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"), } } @@ -510,24 +504,18 @@ mod json_parsing_tests { let new = serde_json::json!([ {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"henry\""]}]); - 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, TCComputation::EnforceAlreadyComputed); 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"), } } @@ -538,24 +526,18 @@ mod json_parsing_tests { let new = serde_json::json!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"alice\""]}]); - 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, TCComputation::EnforceAlreadyComputed); 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"), } } @@ -566,14 +548,9 @@ mod json_parsing_tests { let new = serde_json::json!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"alice\"", "Test::\"bob\""]}]); - 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, TCComputation::EnforceAlreadyComputed) .unwrap(); let euid = r#"Test::"jeff""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); @@ -590,13 +567,10 @@ mod json_parsing_tests { let new = serde_json::json!([ {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"henry\""] }]); - 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, TCComputation::ComputeNow) .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())); @@ -612,13 +586,10 @@ mod json_parsing_tests { let new = serde_json::json!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"alice\""]}]); - 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, TCComputation::ComputeNow) .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())); @@ -633,13 +604,10 @@ mod json_parsing_tests { let new = serde_json::json!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"susan\""]}]); - 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, TCComputation::ComputeNow) .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(); @@ -657,14 +625,9 @@ mod json_parsing_tests { {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : {}, "parents" : []}, {"uid":{"__expr":"Test::\"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, TCComputation::ComputeNow) .err() .unwrap(); let expected = r#"Test::"jeff""#.parse().unwrap(); @@ -680,20 +643,13 @@ mod json_parsing_tests { EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([{"uid":{"__expr":"Test::\"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 addl_entities = parser.iter_from_json_value(new).unwrap(); + let err = simple_entities(&parser).add_entities(addl_entities, TCComputation::ComputeNow); 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"), } } @@ -1323,6 +1279,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), @@ -1360,6 +1317,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 @@ -2490,6 +2450,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) @@ -2511,6 +2472,9 @@ mod schema_based_parsing_tests { _ => Box::new(std::iter::empty()), } } + fn action_entities(&self) -> Self::ActionEntityIterator { + std::iter::empty() + } } struct MockEmployeeDescription; diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 4770c0b603..edb7f49cd1 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -20,7 +20,7 @@ use super::{ ValueParser, }; use crate::ast::{Entity, EntityType, EntityUID, RestrictedExpr}; -use crate::entities::{Entities, EntitiesError, TCComputation}; +use crate::entities::{unwrap_or_clone, Entities, EntitiesError, TCComputation}; use crate::extensions::Extensions; use serde::{Deserialize, Serialize}; use smol_str::SmolStr; @@ -44,12 +44,20 @@ 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` 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, those entities' definitions must match exactly 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. schema: Option, /// Extensions which are active for the JSON parsing. @@ -75,12 +83,20 @@ enum EntitySchemaInfo { impl<'e, S: Schema> EntityJsonParser<'e, 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, those entities' definitions must match exactly 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. @@ -96,72 +112,107 @@ 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`. fn parse_ejsons( &self, ejsons: impl IntoIterator, ) -> Result { - let entities = ejsons + let mut entities: Vec = ejsons .into_iter() .map(|ejson| self.parse_ejson(ejson)) - .collect::, _>>()?; + .collect::>()?; + if let Some(schema) = &self.schema { + entities.extend(schema.action_entities().into_iter().map(unwrap_or_clone)); + } Entities::from_entities(entities, self.tc_computation) } 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-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 4726ff5ef1..9e63ed984a 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -1277,6 +1277,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, @@ -1306,6 +1307,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/src/api.rs b/cedar-policy/src/api.rs index 4b4c6db31f..fd0c6cd2aa 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -283,9 +283,7 @@ impl Entities { 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, entities::TCComputation::ComputeNow, @@ -309,9 +307,7 @@ impl Entities { 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, entities::TCComputation::ComputeNow, @@ -335,9 +331,7 @@ impl Entities { 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, entities::TCComputation::ComputeNow, @@ -4651,7 +4645,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"); @@ -5023,7 +5018,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"); @@ -5104,7 +5100,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!( @@ -5121,7 +5118,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 From e439d1736943f12f18fee6b17fab728c90a80223 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 25 Sep 2023 15:09:30 +0000 Subject: [PATCH 02/39] Entities::from_entities adds actions from schema --- cedar-policy-cli/src/lib.rs | 33 ----- cedar-policy-core/src/entities.rs | 42 +++++-- .../src/entities/json/entities.rs | 7 +- cedar-policy-core/src/evaluator.rs | 12 +- cedar-policy-validator/src/schema.rs | 7 +- cedar-policy/src/api.rs | 118 +++++++++++++----- 6 files changed, 134 insertions(+), 85 deletions(-) diff --git a/cedar-policy-cli/src/lib.rs b/cedar-policy-cli/src/lib.rs index 66c9c30b87..015a9636e0 100644 --- a/cedar-policy-cli/src/lib.rs +++ b/cedar-policy-cli/src/lib.rs @@ -463,13 +463,6 @@ pub fn evaluate(args: &EvaluateArgs) -> (CedarExitCode, EvalResult) { } }, }; - let entities = match load_actions_from_schema(entities, &schema) { - Ok(entities) => entities, - Err(e) => { - println!("Error: {e:?}"); - return (CedarExitCode::Failure, EvalResult::Bool(false)); - } - }; match eval_expression(&request, &entities, &expr) .into_diagnostic() .wrap_err("failed to evaluate the expression") @@ -935,25 +928,6 @@ fn read_schema_file(filename: impl AsRef + std::marker::Copy) -> Result) -> Result { - match schema { - Some(schema) => match schema.action_entities() { - Ok(action_entities) => Entities::from_entities( - entities - .iter() - .cloned() - .chain(action_entities.iter().cloned()), - ) - .into_diagnostic() - .wrap_err("failed to merge action entities with entity file"), - Err(e) => Err(e) - .into_diagnostic() - .wrap_err("failed to construct action entities"), - }, - None => Ok(entities), - } -} - /// This uses the Cedar API to call the authorization engine. fn execute_request( request: &RequestArgs, @@ -986,13 +960,6 @@ fn execute_request( Entities::empty() } }; - let entities = match load_actions_from_schema(entities, &schema) { - Ok(entities) => entities, - Err(e) => { - errs.push(e); - Entities::empty() - } - }; match request.get_request(schema.as_ref()) { Ok(request) if errs.is_empty() => { let authorizer = Authorizer::new(); diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 5dc8dee376..0692907707 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -141,13 +141,27 @@ 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`. + /// TODO: validate the entities against 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, ) -> Result { - let mut entity_map = entities.into_iter().map(|e| (e.uid(), e)).collect(); + let mut entity_map: HashMap = + entities.into_iter().map(|e| (e.uid(), e)).collect(); + if let Some(schema) = schema { + entity_map.extend( + schema + .action_entities() + .into_iter() + .map(|e| (e.uid(), unwrap_or_clone(e))), + ); + } match tc_computation { TCComputation::AssumeAlreadyComputed => {} TCComputation::EnforceAlreadyComputed => { @@ -1064,8 +1078,12 @@ 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, + ) + .expect("Failed to construct entities"); assert_eq!( entities, roundtrip(&entities).expect("should roundtrip without errors") @@ -1122,6 +1140,7 @@ mod json_parsing_tests { Entity::with_uid(EntityUID::with_eid("parent1")), Entity::with_uid(EntityUID::with_eid("parent2")), ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, ) .expect("Failed to construct entities"); @@ -1152,6 +1171,7 @@ mod json_parsing_tests { Entity::with_uid(EntityUID::with_eid("parent1")), Entity::with_uid(EntityUID::with_eid("parent2")), ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, ) .expect("Failed to construct entities"); @@ -1217,7 +1237,7 @@ 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) + let es = Entities::from_entities(v, None::<&NoEntitiesSchema>, TCComputation::ComputeNow) .expect("Failed to construct entities"); let es_v = es.iter().collect::>(); assert!(es_v.len() == 4, "All entities should be in the vec"); @@ -1238,7 +1258,11 @@ 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, + ); match es { Ok(_) => panic!("Was not transitively closed!"), Err(EntitiesError::TransitiveClosureError(_)) => (), @@ -1259,8 +1283,12 @@ 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, + ) + .expect("Should have succeeded"); } } diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index edb7f49cd1..f5dff18a9b 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -206,14 +206,11 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { &self, ejsons: impl IntoIterator, ) -> Result { - let mut entities: Vec = ejsons + let entities: Vec = ejsons .into_iter() .map(|ejson| self.parse_ejson(ejson)) .collect::>()?; - if let Some(schema) = &self.schema { - entities.extend(schema.action_entities().into_iter().map(unwrap_or_clone)); - } - Entities::from_entities(entities, self.tc_computation) + Entities::from_entities(entities, self.schema.as_ref(), self.tc_computation) } /// internal function that parses an `EntityJSON` into an `Entity` diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index 2703a42801..65294cce49 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -827,7 +827,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}, }; @@ -862,6 +862,7 @@ pub mod test { Entity::with_uid(EntityUID::with_eid("test_action")), Entity::with_uid(EntityUID::with_eid("test_resource")), ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, ) .expect("failed to create basic entities") @@ -914,6 +915,7 @@ pub mod test { sibling, unrelated, ], + None::<&NoEntitiesSchema>, TCComputation::ComputeNow, ) .expect("Failed to create rich entities") @@ -2997,8 +2999,12 @@ 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, + ) + .expect("failed to create basic entities"); let exts = Extensions::none(); let eval = Evaluator::new(&request, &entities, &exts).expect("failed to create evaluator"); assert_eq!( diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 9e63ed984a..2a1145f1ca 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -1216,7 +1216,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 descendant 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 @@ -1241,11 +1242,11 @@ 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, ) } diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index fd0c6cd2aa..f4cfce6921 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -244,12 +244,23 @@ 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 + /// + /// If `schema` is present, then action entities from that schema will also + /// be added to the `Entities`. + /// (In this version of Cedar, the entities in `entities` are not actually + /// validated against the `schema`.) + /// + /// Returns an error if the entities cannot be read or if the entities + /// hierarchy is cyclic. 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, ) .map(Entities) @@ -272,6 +283,7 @@ impl Entities { /// 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 pub fn add_entities_from_json_str( self, @@ -340,9 +352,21 @@ impl Entities { /// 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, those entities' definitions must match exactly 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; @@ -388,9 +412,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, those entities' definitions must match exactly 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; @@ -429,9 +465,20 @@ 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, those entities' definitions must match exactly 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>, @@ -4501,7 +4548,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)); @@ -5432,29 +5479,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() ); } From 0b083d8eb1a0d95a8d2d576b25ff484ba1b5c68c Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 10 Oct 2023 12:58:02 +0000 Subject: [PATCH 03/39] refactor entity-schema-conformance errors --- cedar-policy-core/src/entities.rs | 2 + cedar-policy-core/src/entities/conformance.rs | 66 ++++++++++++++++ cedar-policy-core/src/entities/err.rs | 2 +- .../src/entities/json/entities.rs | 47 +++++++----- cedar-policy-core/src/entities/json/err.rs | 63 +++++---------- .../src/entities/json/jsonvalue.rs | 76 ++++++++++++++----- 6 files changed, 169 insertions(+), 87 deletions(-) create mode 100644 cedar-policy-core/src/entities/conformance.rs diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 0692907707..b9a582a569 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::EntitySchemaConformanceError; mod err; pub use err::*; mod json; diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs new file mode 100644 index 0000000000..a155e09d35 --- /dev/null +++ b/cedar-policy-core/src/entities/conformance.rs @@ -0,0 +1,66 @@ +use super::SchemaType; +use crate::ast::{EntityType, EntityUID}; +use smol_str::SmolStr; +use thiserror::Error; + +/// Errors raised when (non-action) entities do not conform to the schema. +#[derive(Debug, Error)] +pub enum EntitySchemaConformanceError { + /// 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, + }, + /// 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, + }, + /// The given attribute on the given entity had a different type than the + /// schema indicated to expect + #[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, + }, + /// During schema-based parsing, 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}`, set elements have different types: {ty1} and {ty2}" + )] + HeterogeneousSet { + /// Entity where the error occurred + uid: EntityUID, + /// Name of the attribute where the error occurred + attr: SmolStr, + /// First element type which was found + ty1: Box, + /// Second element type which was found + ty2: Box, + }, + /// During schema-based parsing, found a parent of a type that's not allowed + /// for that entity + #[error( + "`{uid}` is not allowed to have a parent of type `{parent_ty}` according to the schema" + )] + InvalidParentType { + /// 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 EntitySchemaConformanceErrors being large) + }, +} diff --git a/cedar-policy-core/src/entities/err.rs b/cedar-policy-core/src/entities/err.rs index ea0e3614bb..7b97ab927e 100644 --- a/cedar-policy-core/src/entities/err.rs +++ b/cedar-policy-core/src/entities/err.rs @@ -14,7 +14,7 @@ * limitations under the License. */ -use super::EntityUID; +use super::{EntitySchemaConformanceError, EntityUID}; use crate::transitive_closure; use thiserror::Error; diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index f5dff18a9b..39e09b6375 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -20,7 +20,9 @@ use super::{ ValueParser, }; use crate::ast::{Entity, EntityType, EntityUID, RestrictedExpr}; -use crate::entities::{unwrap_or_clone, Entities, EntitiesError, TCComputation}; +use crate::entities::{ + unwrap_or_clone, Entities, EntitiesError, EntitySchemaConformanceError, TCComputation, +}; use crate::extensions::Extensions; use serde::{Deserialize, Serialize}; use smol_str::SmolStr; @@ -267,10 +269,12 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { if ejson.attrs.contains_key(&required_attr) { // all good } else { - return Err(JsonDeserializationError::MissingRequiredEntityAttr { - uid, - attr: required_attr, - }); + return Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::MissingRequiredEntityAttr { + uid, + attr: required_attr, + }, + )); } } } @@ -296,10 +300,12 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { // `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, - }) + return Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::UnexpectedEntityAttr { + uid: uid.clone(), + attr: k, + }, + )) } Some(expected_ty) => ( vparser.val_into_rexpr(v, Some(&expected_ty), || { @@ -326,14 +332,14 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { if actual_ty.is_consistent_with(&expected_ty) { Ok((k, rexpr)) } else { - Err(JsonDeserializationError::TypeMismatch { - ctx: Box::new(JsonDeserializationErrorContext::EntityAttribute { + Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::TypeMismatch { uid: uid.clone(), attr: k, - }), - expected: Box::new(expected_ty), - actual: Box::new(actual_ty), - }) + expected: Box::new(expected_ty), + actual: Box::new(actual_ty), + }, + )) } } EntitySchemaInfo::Action(action) => { @@ -405,13 +411,12 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { if desc.allowed_parent_types().contains(parent_type) { Ok(()) } else { - Err(JsonDeserializationError::InvalidParentType { - ctx: Box::new(JsonDeserializationErrorContext::EntityParents { + Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::InvalidParentType { uid: uid.clone(), - }), - uid: uid.clone(), - parent_ty: Box::new(parent_type.clone()), - }) + parent_ty: Box::new(parent_type.clone()), + }, + )) } } } diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index a357a81a47..7f464bc8ca 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -16,7 +16,7 @@ use std::fmt::Display; -use super::SchemaType; +use super::{super::EntitySchemaConformanceError, SchemaType}; use crate::ast::{ EntityType, EntityUID, Expr, ExprKind, Name, RestrictedExpr, RestrictedExprError, }; @@ -143,15 +143,13 @@ pub enum JsonDeserializationError { /// 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, - }, + /// During schema-based parsing, encountered a (non-action) entity which + /// does not conform to the schema. + /// + /// This error contains the Entity analogues of the Record error cases + /// 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")] @@ -161,15 +159,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")] @@ -179,41 +168,27 @@ 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}")] - TypeMismatch { - /// Context of this error - ctx: Box, + /// During schema-based parsing of the Context, found a different type than + /// the schema indicated to expect + /// + /// (type mismatches in entity attributes will be + /// `EntitySchemaConformanceError`) + #[error("while parsing context, type mismatch: expected type {expected}, but actually has type {actual}")] + ContextTypeMismatch { /// 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}")] - HeterogeneousSet { - /// Context of this error - ctx: Box, + /// During schema-based parsing of the Context, found a set whose elements + /// don't all have the same type. This doesn't match any possible schema. + #[error("while parsing context, found set elements with different types: {ty1} and {ty2}")] + ContextHeterogeneousSet { /// First element type which was found ty1: Box, /// Second element type which was found ty2: Box, }, - /// 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 - 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) - }, } /// Errors thrown during serialization to JSON diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index 4769f1c20a..1266288276 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -21,7 +21,7 @@ use super::{ use crate::ast::{ BorrowedRestrictedExpr, Eid, EntityUID, Expr, ExprKind, Literal, Name, RestrictedExpr, }; -use crate::entities::EscapeKind; +use crate::entities::{EntitySchemaConformanceError, EscapeKind}; use crate::extensions::{ExtensionFunctionLookupError, Extensions}; use crate::FromNormalizedStr; use serde::{Deserialize, Serialize}; @@ -348,14 +348,31 @@ impl<'e> ValueParser<'e> { .map(|element| self.val_into_rexpr(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: JSONValue = serde_json::from_value(val)?; - Box::new(self.type_of_rexpr(jvalue.into_expr()?.as_borrowed(), ctx)?) - }, - }), + Box::new( + self.type_of_rexpr(jvalue.into_expr()?.as_borrowed(), ctx.clone())?, + ) + }; + match ctx() { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::TypeMismatch { + uid, + attr, + expected, + actual, + }, + )) + } + JsonDeserializationErrorContext::Context => { + Err(JsonDeserializationError::ContextTypeMismatch { expected, actual }) + } + ctx => panic!("type mismatches can only occur in entity attributes or in context, but somehow found one {ctx}"), + } + } }, // The expected type is a record type. No special parsing rules // apply, but we need to parse the attribute values according to @@ -396,14 +413,24 @@ impl<'e> ValueParser<'e> { } Ok(RestrictedExpr::record(rexpr_pairs)) } - _ => 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: JSONValue = serde_json::from_value(val)?; - Box::new(self.type_of_rexpr(jvalue.into_expr()?.as_borrowed(), ctx)?) - }, - }), + Box::new( + self.type_of_rexpr(jvalue.into_expr()?.as_borrowed(), ctx.clone())?, + ) + }; + match ctx() { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + Err(JsonDeserializationError::EntitySchemaConformance(EntitySchemaConformanceError::TypeMismatch { uid, attr, expected, actual })) + } + JsonDeserializationErrorContext::Context => { + Err(JsonDeserializationError::ContextTypeMismatch { expected, actual }) + } + ctx => panic!("type mismatches can only occur in entity attributes or in context, but somehow found one {ctx}") + } + } }, // The expected type is any other type. No special parsing rules apply, // and we treat this exactly as the non-schema-based-parsing case. @@ -503,12 +530,19 @@ impl<'e> ValueParser<'e> { 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(Ok(conflicting_ty)) => { + let ty1 = Box::new(element_ty); + let ty2 = Box::new(conflicting_ty); + match ctx() { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + Err(JsonDeserializationError::EntitySchemaConformance(EntitySchemaConformanceError::HeterogeneousSet { uid, attr, ty1, ty2 })) + } + JsonDeserializationErrorContext::Context => { + Err(JsonDeserializationError::ContextHeterogeneousSet { ty1, ty2 }) + } + ctx => panic!("heterogeneous sets can only occur in entity attributes or in context, but somehow found one {ctx}") + } + } Some(Err(e)) => Err(e), } } From a741f9ef6045e0256d856c48d461da87f01ccaa2 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 13 Oct 2023 14:08:06 +0000 Subject: [PATCH 04/39] validate entities against schema --- cedar-policy-core/src/ast/entity.rs | 9 +- cedar-policy-core/src/entities.rs | 113 +++++-- cedar-policy-core/src/entities/conformance.rs | 281 ++++++++++++++++-- cedar-policy-core/src/entities/err.rs | 5 +- .../src/entities/json/entities.rs | 222 +++++--------- cedar-policy-core/src/entities/json/err.rs | 53 +--- .../src/entities/json/jsonvalue.rs | 124 +++----- cedar-policy-core/src/evaluator.rs | 3 + cedar-policy-core/src/extensions.rs | 4 +- cedar-policy-validator/src/schema.rs | 2 + cedar-policy/src/api.rs | 1 + 11 files changed, 505 insertions(+), 312 deletions(-) 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 b9a582a569..6b9a665dff 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -28,7 +28,7 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; mod conformance; -pub use conformance::EntitySchemaConformanceError; +pub use conformance::*; mod err; pub use err::*; mod json; @@ -115,14 +115,26 @@ 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 they are + /// not already present.) + /// /// 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) => { @@ -130,7 +142,7 @@ impl Entities { } } } - match mode { + match tc_computation { TCComputation::AssumeAlreadyComputed => (), TCComputation::EnforceAlreadyComputed => { enforce_tc_and_dag(&self.entities).map_err(Box::new)? @@ -145,7 +157,8 @@ impl Entities { /// /// If `schema` is present, then action entities from that schema will also /// be added to the `Entities`. - /// TODO: validate the entities against the schema + /// 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. @@ -153,10 +166,20 @@ impl Entities { entities: impl IntoIterator, schema: Option<&impl Schema>, tc_computation: TCComputation, + extensions: Extensions<'_>, ) -> Result { let mut entity_map: HashMap = entities.into_iter().map(|e| (e.uid(), e)).collect(); 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() @@ -498,8 +521,12 @@ mod json_parsing_tests { {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"george\"", "Test::\"janet\""]}]); let addl_entities = parser.iter_from_json_value(new).unwrap(); - let err = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::EnforceAlreadyComputed); + 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(), @@ -521,8 +548,12 @@ mod json_parsing_tests { {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"henry\""]}]); let addl_entities = parser.iter_from_json_value(new).unwrap(); - let err = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::EnforceAlreadyComputed); + 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(), @@ -543,8 +574,12 @@ mod json_parsing_tests { {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"alice\""]}]); let addl_entities = parser.iter_from_json_value(new).unwrap(); - let err = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::EnforceAlreadyComputed); + 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(), @@ -566,7 +601,12 @@ mod json_parsing_tests { let addl_entities = parser.iter_from_json_value(new).unwrap(); let es = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::EnforceAlreadyComputed) + .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(); @@ -585,7 +625,12 @@ mod json_parsing_tests { let addl_entities = parser.iter_from_json_value(new).unwrap(); let es = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::ComputeNow) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .unwrap(); let euid = r#"Test::"george""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); @@ -604,7 +649,12 @@ mod json_parsing_tests { let addl_entities = parser.iter_from_json_value(new).unwrap(); let es = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::ComputeNow) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .unwrap(); let euid = r#"Test::"jeff""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); @@ -622,7 +672,12 @@ mod json_parsing_tests { let addl_entities = parser.iter_from_json_value(new).unwrap(); let es = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::ComputeNow) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .unwrap(); let euid = r#"Test::"jeff""#.parse().unwrap(); let jeff = es.entity(&euid).unwrap(); @@ -643,7 +698,12 @@ mod json_parsing_tests { let addl_entities = parser.iter_from_json_value(new).unwrap(); let err = simple_entities(&parser) - .add_entities(addl_entities, TCComputation::ComputeNow) + .add_entities( + addl_entities, + None::<&NoEntitiesSchema>, + TCComputation::ComputeNow, + Extensions::all_available(), + ) .err() .unwrap(); let expected = r#"Test::"jeff""#.parse().unwrap(); @@ -660,7 +720,12 @@ mod json_parsing_tests { let new = serde_json::json!([{"uid":{"__expr":"Test::\"alice\""}, "attrs" : {}, "parents" : []}]); let addl_entities = parser.iter_from_json_value(new).unwrap(); - let err = simple_entities(&parser).add_entities(addl_entities, TCComputation::ComputeNow); + 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 { Err(EntitiesError::Duplicate(e)) => assert_eq!(e, expected), @@ -1084,6 +1149,7 @@ mod json_parsing_tests { [e0, e1, e2, e3], None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::none(), ) .expect("Failed to construct entities"); assert_eq!( @@ -1144,6 +1210,7 @@ mod json_parsing_tests { ], None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::all_available(), ) .expect("Failed to construct entities"); assert_eq!( @@ -1175,6 +1242,7 @@ mod json_parsing_tests { ], None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::all_available(), ) .expect("Failed to construct entities"); assert!(matches!( @@ -1239,8 +1307,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, None::<&NoEntitiesSchema>, 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)); @@ -1264,6 +1337,7 @@ mod entities_tests { vec![e1, e2, e3], None::<&NoEntitiesSchema>, TCComputation::EnforceAlreadyComputed, + Extensions::all_available(), ); match es { Ok(_) => panic!("Was not transitively closed!"), @@ -1289,6 +1363,7 @@ mod entities_tests { vec![e1, e2, e3], None::<&NoEntitiesSchema>, TCComputation::EnforceAlreadyComputed, + Extensions::all_available(), ) .expect("Should have succeeded"); } @@ -2178,7 +2253,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 diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index a155e09d35..567bc5e292 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -1,9 +1,11 @@ -use super::SchemaType; -use crate::ast::{EntityType, EntityUID}; +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 (non-action) entities do not conform to the schema. +/// Errors raised when entities do not conform to the schema #[derive(Debug, Error)] pub enum EntitySchemaConformanceError { /// Encountered this attribute on this entity, but that attribute shouldn't @@ -37,30 +39,271 @@ pub enum EntitySchemaConformanceError { /// 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( - "in attribute `{attr}` on `{uid}`, set elements have different types: {ty1} and {ty2}" - )] + /// 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, - /// First element type which was found - ty1: Box, - /// Second element type which was found - ty2: Box, + /// Underlying error + err: HeterogeneousSetError, }, - /// During schema-based parsing, found a parent of a type that's not allowed - /// for that entity + /// Found an ancestor of a type that's not allowed for that entity #[error( - "`{uid}` is not allowed to have a parent of type `{parent_ty}` according to the schema" + "`{uid}` is not allowed to have an ancestor of type `{ancestor_ty}` according to the schema" )] - InvalidParentType { - /// Entity that has an invalid parent type + 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, - /// Parent type which was invalid - parent_ty: Box, // boxed to avoid this variant being very large (and thus all EntitySchemaConformanceErrors being large) }, + /// 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}")] + Extension { + /// 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_rexpr(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(TypeOfRexprError::HeterogeneousSet(err)) => { + return Err(EntitySchemaConformanceError::HeterogeneousSet { + uid: uid.clone(), + attr: attr.into(), + err, + }); + } + Err(TypeOfRexprError::Extension(err)) => { + return Err(EntitySchemaConformanceError::Extension { + 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_rexpr()`] +#[derive(Debug, Error)] +pub enum TypeOfRexprError { + /// Encountered a heterogeneous set + #[error(transparent)] + HeterogeneousSet(#[from] HeterogeneousSetError), + /// Error looking up an extension function + #[error(transparent)] + Extension(#[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`. +pub fn type_of_rexpr( + 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_rexpr(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 { pairs } => { + Ok(SchemaType::Record { attrs: { + pairs.iter().map(|(k, v)| { + let attr_type = 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 + 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::, TypeOfRexprError>>()? + }}) + } + 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 7b97ab927e..2689def836 100644 --- a/cedar-policy-core/src/entities/err.rs +++ b/cedar-policy-core/src/entities/err.rs @@ -14,7 +14,7 @@ * limitations under the License. */ -use super::{EntitySchemaConformanceError, EntityUID}; +use super::EntityUID; use crate::transitive_closure; use thiserror::Error; @@ -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/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 39e09b6375..0568aa28c3 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -21,7 +21,8 @@ use super::{ }; use crate::ast::{Entity, EntityType, EntityUID, RestrictedExpr}; use crate::entities::{ - unwrap_or_clone, Entities, EntitiesError, EntitySchemaConformanceError, TCComputation, + type_of_rexpr, unwrap_or_clone, Entities, EntitiesError, EntitySchemaConformanceError, + TCComputation, TypeOfRexprError, }; use crate::extensions::Extensions; use serde::{Deserialize, Serialize}; @@ -203,7 +204,8 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { /// 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`. + /// entities declared in the `schema`, and validates all the entities + /// against the schema. fn parse_ejsons( &self, ejsons: impl IntoIterator, @@ -212,73 +214,50 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { .into_iter() .map(|ejson| self.parse_ejson(ejson)) .collect::>()?; - Entities::from_entities(entities, self.schema.as_ref(), self.tc_computation) + Entities::from_entities( + entities, + self.schema.as_ref(), + self.tc_computation, + self.extensions, + ) } /// 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::EntitySchemaConformance( - EntitySchemaConformanceError::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 attrs: HashMap = ejson .attrs @@ -296,7 +275,7 @@ 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 => { @@ -307,40 +286,16 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { }, )) } - Some(expected_ty) => ( + Some(expected_ty) => { vparser.val_into_rexpr(v, Some(&expected_ty), || { JsonDeserializationErrorContext::EntityAttribute { 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(), + })? } - })?; - if actual_ty.is_consistent_with(&expected_ty) { - Ok((k, rexpr)) - } else { - Err(JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::TypeMismatch { - uid: uid.clone(), - attr: k, - expected: Box::new(expected_ty), - actual: Box::new(actual_ty), - }, - )) - } + }; + Ok((k.clone(), rexpr)) } EntitySchemaInfo::Action(action) => { // We'll do schema-based parsing assuming optimistically that @@ -351,17 +306,27 @@ 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(), + let expected_ty = type_of_rexpr(expected_rexpr.as_borrowed(), self.extensions) + .map_err(|e| match e { + TypeOfRexprError::HeterogeneousSet(err) => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::HeterogeneousSet { + uid: uid.clone(), + attr: k.clone(), + err, + }, + ) + } + TypeOfRexprError::Extension(err) => { + JsonDeserializationError::FailedExtensionFunctionLookup(err) } })?; let actual_rexpr = vparser.val_into_rexpr(v, Some(&expected_ty), || { @@ -372,53 +337,31 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { })?; if actual_rexpr == *expected_rexpr { Ok((k, actual_rexpr)) - } else { - Err(JsonDeserializationError::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::EntitySchemaConformance( - EntitySchemaConformanceError::InvalidParentType { + EntitySchemaConformanceError::ActionDeclarationMismatch { uid: uid.clone(), - parent_ty: Box::new(parent_type.clone()), }, )) } } + }) + .collect::>()?; + let is_parent_allowed = |parent_euid: &EntityUID| { + // 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 @@ -436,17 +379,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)) } } diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index 7f464bc8ca..10348ae5c8 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::{super::EntitySchemaConformanceError, SchemaType}; -use crate::ast::{ - EntityType, EntityUID, Expr, ExprKind, Name, RestrictedExpr, RestrictedExprError, -}; +use crate::ast::{EntityUID, Expr, ExprKind, Name, RestrictedExpr, RestrictedExprError}; +use crate::entities::conformance::HeterogeneousSetError; use crate::extensions::ExtensionFunctionLookupError; use crate::parser::err::ParseErrors; use smol_str::SmolStr; @@ -113,38 +112,8 @@ 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 a (non-action) entity which - /// does not conform to the schema. + /// During schema-based parsing, encountered an entity which does not + /// conform to the schema. /// /// This error contains the Entity analogues of the Record error cases /// listed below, among other things. @@ -182,13 +151,13 @@ pub enum JsonDeserializationError { }, /// During schema-based parsing of the Context, found a set whose elements /// don't all have the same type. This doesn't match any possible schema. - #[error("while parsing context, found set elements with different types: {ty1} and {ty2}")] - ContextHeterogeneousSet { - /// First element type which was found - ty1: Box, - /// Second element type which was found - ty2: Box, - }, + #[error("while parsing context, {0}")] + ContextHeterogeneousSet(HeterogeneousSetError), + /// During schema-based parsing of the Context, error looking up an extension function. + /// This error can occur when parsing the Context because that may require + /// getting information about any extension functions referenced in Context values. + #[error("while parsing context, {0}")] + ContextExtension(ExtensionFunctionLookupError), } /// Errors thrown during serialization to JSON diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index 1266288276..d7b0b9be39 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -15,14 +15,13 @@ */ use super::{ - AttributeType, JsonDeserializationError, JsonDeserializationErrorContext, - JsonSerializationError, SchemaType, + JsonDeserializationError, JsonDeserializationErrorContext, JsonSerializationError, SchemaType, }; use crate::ast::{ BorrowedRestrictedExpr, Eid, EntityUID, Expr, ExprKind, Literal, Name, RestrictedExpr, }; -use crate::entities::{EntitySchemaConformanceError, EscapeKind}; -use crate::extensions::{ExtensionFunctionLookupError, Extensions}; +use crate::entities::{type_of_rexpr, EntitySchemaConformanceError, EscapeKind, TypeOfRexprError}; +use crate::extensions::Extensions; use crate::FromNormalizedStr; use serde::{Deserialize, Serialize}; use smol_str::SmolStr; @@ -309,7 +308,8 @@ 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. + /// provided. This does not mean that this function fully validates the + /// value against `expected_ty` -- it does not. pub fn val_into_rexpr( &self, val: serde_json::Value, @@ -352,9 +352,11 @@ impl<'e> ValueParser<'e> { let expected = Box::new(expected_ty.clone()); let actual = { let jvalue: JSONValue = serde_json::from_value(val)?; - Box::new( - self.type_of_rexpr(jvalue.into_expr()?.as_borrowed(), ctx.clone())?, - ) + let ty = type_of_rexpr(jvalue.into_expr()?.as_borrowed(), self.extensions) + .map_err(|e| { + type_of_rexpr_error_to_json_deserialization_error(e, ctx()) + })?; + Box::new(ty) }; match ctx() { JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { @@ -417,9 +419,11 @@ impl<'e> ValueParser<'e> { let expected = Box::new(expected_ty.clone()); let actual = { let jvalue: JSONValue = serde_json::from_value(val)?; - Box::new( - self.type_of_rexpr(jvalue.into_expr()?.as_borrowed(), ctx.clone())?, - ) + let ty = type_of_rexpr(jvalue.into_expr()?.as_borrowed(), self.extensions) + .map_err(|e| { + type_of_rexpr_error_to_json_deserialization_error(e, ctx()) + })?; + Box::new(ty) }; match ctx() { JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { @@ -479,7 +483,8 @@ impl<'e> ValueParser<'e> { } ExtnValueJSON::ImplicitConstructor(val) => { let arg = val.into_expr()?; - let argty = self.type_of_rexpr(arg.as_borrowed(), ctx.clone())?; + let argty = type_of_rexpr(arg.as_borrowed(), self.extensions) + .map_err(|e| type_of_rexpr_error_to_json_deserialization_error(e, ctx()))?; let func = self .extensions .lookup_single_arg_constructor( @@ -502,77 +507,36 @@ 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)) => { - let ty1 = Box::new(element_ty); - let ty2 = Box::new(conflicting_ty); - match ctx() { - JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { - Err(JsonDeserializationError::EntitySchemaConformance(EntitySchemaConformanceError::HeterogeneousSet { uid, attr, ty1, ty2 })) - } - JsonDeserializationErrorContext::Context => { - Err(JsonDeserializationError::ContextHeterogeneousSet { ty1, ty2 }) - } - ctx => panic!("heterogeneous sets can only occur in entity attributes or in context, but somehow found one {ctx}") - } - } - Some(Err(e)) => Err(e), - } - } - } - } - ExprKind::Record { pairs } => { - Ok(SchemaType::Record { attrs: { - pairs.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>>()? - }}) - } - 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() - })?) - } - // PANIC SAFETY. Unreachable by invariant on restricted expressions - #[allow(clippy::unreachable)] - expr => unreachable!("internal invariant violation: BorrowedRestrictedExpr somehow contained this expr case: {expr:?}"), +fn type_of_rexpr_error_to_json_deserialization_error( + torerr: TypeOfRexprError, + ctx: JsonDeserializationErrorContext, +) -> JsonDeserializationError { + match torerr { + TypeOfRexprError::HeterogeneousSet(err) => match ctx { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::HeterogeneousSet { uid, attr, err } + ) + } + JsonDeserializationErrorContext::Context => { + JsonDeserializationError::ContextHeterogeneousSet(err) } + ctx => panic!("heterogenous sets can only occur in entity attributes or in context, but somehow found one {ctx}"), } + TypeOfRexprError::Extension(err) => match ctx { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::Extension { uid, attr, err } + ) + } + JsonDeserializationErrorContext::Context => { + JsonDeserializationError::ContextExtension(err) + } + ctx => panic!("failed extension function lookups can only occur in entity attributes or in context, but somehow found one {ctx}"), + } +} } /// Serde JSON format for Cedar values where we know we're expecting an entity diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index 65294cce49..ccaaf63d21 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -864,6 +864,7 @@ pub mod test { ], None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::none(), ) .expect("failed to create basic entities") } @@ -917,6 +918,7 @@ pub mod test { ], None::<&NoEntitiesSchema>, TCComputation::ComputeNow, + Extensions::all_available(), ) .expect("Failed to create rich entities") } @@ -3003,6 +3005,7 @@ pub mod test { vec![alice], None::<&NoEntitiesSchema>, TCComputation::AssumeAlreadyComputed, + Extensions::all_available(), ) .expect("failed to create basic entities"); let exts = Extensions::none(); diff --git a/cedar-policy-core/src/extensions.rs b/cedar-policy-core/src/extensions.rs index c42650927a..1c30a24860 100644 --- a/cedar-policy-core/src/extensions.rs +++ b/cedar-policy-core/src/extensions.rs @@ -39,8 +39,8 @@ 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)] +/// Clone is cheap for this type, and in fact we commit to this by marking it Copy +#[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 2a1145f1ca..8a124a9c5d 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::{Eid, Entity, EntityType, EntityUID, Id, Name, RestrictedExpr}, entities::{Entities, JSONValue, TCComputation}, + extensions::Extensions, parser::err::ParseErrors, transitive_closure::{compute_tc, TCNode}, FromNormalizedStr, @@ -1248,6 +1249,7 @@ impl ValidatorSchema { 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(), ) } } diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index f4cfce6921..00d79cedc2 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -262,6 +262,7 @@ impl Entities { .map(|s| cedar_policy_validator::CoreSchema::new(&s.0)) .as_ref(), entities::TCComputation::ComputeNow, + Extensions::all_available(), ) .map(Entities) } From 9ee479a37b31a19d456f49eb99516d19fd21ccdc Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 13 Oct 2023 17:44:28 +0000 Subject: [PATCH 05/39] more --- cedar-policy-core/src/entities.rs | 80 +++++++------- .../src/entities/json/entities.rs | 10 +- cedar-policy-core/src/evaluator.rs | 6 +- cedar-policy/src/api.rs | 104 +++++++++++++----- 4 files changed, 126 insertions(+), 74 deletions(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 6b9a665dff..19b72471df 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -515,7 +515,7 @@ mod json_parsing_tests { #[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!([ {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"george\"", "Test::\"janet\""]}]); @@ -542,7 +542,7 @@ mod json_parsing_tests { #[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!([ {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"henry\""]}]); @@ -568,7 +568,7 @@ mod json_parsing_tests { #[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!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"alice\""]}]); @@ -594,7 +594,7 @@ mod json_parsing_tests { #[test] fn enforces_tc_success() { - let parser: EntityJsonParser<'_> = + let parser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow); let new = serde_json::json!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"alice\"", "Test::\"bob\""]}]); @@ -618,7 +618,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!([ {"uid":{"__expr":"Test::\"george\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"henry\""] }]); @@ -642,7 +642,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!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"alice\""]}]); @@ -665,7 +665,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!([ {"uid":{"__expr":"Test::\"jeff\""}, "attrs" : { "foo" : 3 }, "parents" : ["Test::\"susan\""]}]); @@ -690,7 +690,7 @@ 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":{"__expr":"Test::\"jeff\""}, "attrs" : {}, "parents" : []}, @@ -715,7 +715,7 @@ 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":{"__expr":"Test::\"alice\""}, "attrs" : {}, "parents" : []}]); @@ -734,7 +734,7 @@ mod json_parsing_tests { } } - fn simple_entities(parser: &EntityJsonParser<'_>) -> Entities { + fn simple_entities(parser: &EntityJsonParser<'_, '_>) -> Entities { let json = serde_json::json!( [ { @@ -802,7 +802,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) @@ -845,7 +845,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"); @@ -899,7 +899,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"); @@ -978,7 +978,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"); @@ -1004,7 +1004,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!( @@ -1119,7 +1119,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")) } @@ -1265,7 +1265,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) @@ -1546,7 +1546,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()) @@ -1612,7 +1612,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, ); @@ -1751,7 +1751,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -1797,7 +1797,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -1841,7 +1841,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -1887,7 +1887,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -1934,7 +1934,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -1979,7 +1979,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2025,7 +2025,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2103,7 +2103,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2147,7 +2147,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2194,7 +2194,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2244,7 +2244,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2273,7 +2273,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2302,7 +2302,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2335,7 +2335,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2368,7 +2368,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2401,7 +2401,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2432,7 +2432,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2466,7 +2466,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2497,7 +2497,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2531,7 +2531,7 @@ mod schema_based_parsing_tests { ] ); let eparser = EntityJsonParser::new( - Some(MockSchema), + Some(&MockSchema), Extensions::all_available(), TCComputation::ComputeNow, ); @@ -2626,7 +2626,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/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 0568aa28c3..6828a4e634 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -46,7 +46,7 @@ pub struct EntityJSON { /// Struct used to parse entities from JSON. #[derive(Debug, Clone)] -pub struct EntityJsonParser<'e, S: Schema = NoEntitiesSchema> { +pub struct EntityJsonParser<'e, 's, S: Schema = NoEntitiesSchema> { /// `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 @@ -61,7 +61,7 @@ pub struct EntityJsonParser<'e, S: Schema = NoEntitiesSchema> { /// 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, + schema: Option<&'s S>, /// Extensions which are active for the JSON parsing. extensions: Extensions<'e>, @@ -83,7 +83,7 @@ enum EntitySchemaInfo { NonAction(E), } -impl<'e, S: Schema> EntityJsonParser<'e, S> { +impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { /// Create a new `EntityJsonParser`. /// /// `schema` represents a source of `Action` entities, which will be added @@ -104,7 +104,7 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { /// 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 { @@ -216,7 +216,7 @@ impl<'e, S: Schema> EntityJsonParser<'e, S> { .collect::>()?; Entities::from_entities( entities, - self.schema.as_ref(), + self.schema, self.tc_computation, self.extensions, ) diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index ccaaf63d21..d4e9c1bc27 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -3506,7 +3506,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(); @@ -3619,7 +3619,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(); @@ -3676,7 +3676,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/src/api.rs b/cedar-policy/src/api.rs index 00d79cedc2..b76c7fd5c9 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -245,13 +245,17 @@ 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`. - /// (In this version of Cedar, the entities in `entities` are not actually - /// validated against the `schema`.) + /// `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, those entities' definitions must match + /// exactly or an error is returned.) /// - /// Returns an error if the entities cannot be read or if the entities - /// hierarchy is cyclic. + /// 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>, @@ -267,87 +271,132 @@ impl Entities { .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 will inform the parsing: for instance, it + /// 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` if they are + /// not already present.) + /// + /// 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), + 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). + /// 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` if they are + /// not already present.) /// - /// 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_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)?; 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` if they are + /// not already present.) /// - /// 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)?; 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` if they are + /// not already present.) /// - /// 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)?; Ok(Self(self.0.add_entities( new_entities, + schema.as_ref(), entities::TCComputation::ComputeNow, + Extensions::all_available(), )?)) } @@ -402,8 +451,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, ); @@ -455,8 +505,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, ); @@ -484,8 +535,9 @@ impl Entities { 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, ); From 6916717463883cfe85943f8e0ebac92cd8d557db Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 13 Oct 2023 18:02:50 +0000 Subject: [PATCH 06/39] update changelog and add Entity::validate() --- cedar-policy/CHANGELOG.md | 5 +++++ cedar-policy/src/api.rs | 11 ++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 4c7bf7085e..cea407be6c 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -12,6 +12,7 @@ - Added an API, `unknown_entities`, to `PolicySet` to collect unknown entity UIDs from `PartialResponse`. - Added APIs `remove`, `remove_template` and `unlink` to remove policies from the `PolicySet` - Added API `get_linked_policies` to get the policies linked to a `Template` +- Added `Entity::validate()` to validate an entity against a `Schema` ### Changed @@ -22,6 +23,10 @@ - The `Response::new()` constructor now expects a `Vec` as its third argument. - Implements RFC #19, making validation slightly more strict, but more explainable. - Improved formatting for error messages. +- `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 ## 2.4.1 diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index b76c7fd5c9..4b9a153466 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -26,7 +26,7 @@ use cedar_policy_core::ast; use cedar_policy_core::ast::RestrictedExprError; use cedar_policy_core::authorizer; pub use cedar_policy_core::authorizer::AuthorizationError; -use cedar_policy_core::entities; +use cedar_policy_core::entities::{self, EntitySchemaConformanceChecker}; use cedar_policy_core::entities::JsonDeserializationErrorContext; use cedar_policy_core::entities::{ContextSchema, Dereference, JsonDeserializationError}; use cedar_policy_core::est; @@ -188,6 +188,15 @@ impl Entity { .map(EvalResult::from), ) } + + /// Validate this `Entity` against the given `schema`. + /// + /// If the entity does not conform to the `schema`, an error is returned. + pub fn validate(&self, schema: &Schema) -> Result<(), impl std::error::Error> { + let schema = cedar_policy_validator::CoreSchema::new(&schema.0); + let checker = EntitySchemaConformanceChecker::new(&schema, Extensions::all_available()); + checker.validate_entity(&self.0) + } } impl std::fmt::Display for Entity { From f1f90408044bdc3a55a29772ebcfeed50d2fea92 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 16 Oct 2023 18:24:59 +0000 Subject: [PATCH 07/39] tests --- cedar-policy/src/api.rs | 195 ++++++++++++++++++++ cedar-policy/tests/example_use_cases_doc.rs | 2 +- 2 files changed, 196 insertions(+), 1 deletion(-) diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 4b9a153466..999b461b8c 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -4618,6 +4618,201 @@ mod ancestors_tests { } } +/// These tests are for `Entity::validate()`. +/// 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") + } + + #[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()) + ])) + ])), + ("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(), + ); + entity.validate(&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()) + ])) + ])), + ("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 entity.validate(&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()) + ])) + ])), + ("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 entity.validate(&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 an 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()) + ])) + ])), + ("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 entity.validate(&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 entity.validate(&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. /// diff --git a/cedar-policy/tests/example_use_cases_doc.rs b/cedar-policy/tests/example_use_cases_doc.rs index c323125c9d..e8d4d12ad5 100644 --- a/cedar-policy/tests/example_use_cases_doc.rs +++ b/cedar-policy/tests/example_use_cases_doc.rs @@ -69,7 +69,7 @@ fn scenario_4a() { /// currently failing, as the validator does not support action attributes #[should_panic( - expected = "error occurred while evaluating policy `policy0`: entity `Action::\\\"view\\\"` does not exist" + expected = "Action::\\\"view\\\"` does not have the attribute `readOnly`" )] #[test] fn scenario_4c() { From 4c80128ddf6c4c221cac0a6745ee10c987cd2713 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 16 Oct 2023 18:43:37 +0000 Subject: [PATCH 08/39] cargo fmt --- .../src/entities/json/entities.rs | 7 +- cedar-policy/src/api.rs | 234 +++++++++++++----- cedar-policy/tests/example_use_cases_doc.rs | 4 +- 3 files changed, 177 insertions(+), 68 deletions(-) diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 6828a4e634..931ed76a25 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -214,12 +214,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { .into_iter() .map(|ejson| self.parse_ejson(ejson)) .collect::>()?; - Entities::from_entities( - entities, - self.schema, - self.tc_computation, - self.extensions, - ) + Entities::from_entities(entities, self.schema, self.tc_computation, self.extensions) } /// internal function that parses an `EntityJSON` into an `Entity` diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 999b461b8c..da519fd46c 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -26,8 +26,8 @@ use cedar_policy_core::ast; use cedar_policy_core::ast::RestrictedExprError; use cedar_policy_core::authorizer; pub use cedar_policy_core::authorizer::AuthorizationError; -use cedar_policy_core::entities::{self, EntitySchemaConformanceChecker}; use cedar_policy_core::entities::JsonDeserializationErrorContext; +use cedar_policy_core::entities::{self, EntitySchemaConformanceChecker}; use cedar_policy_core::entities::{ContextSchema, Dereference, JsonDeserializationError}; use cedar_policy_core::est; pub use cedar_policy_core::evaluator::{EvaluationError, EvaluationErrorKind}; @@ -299,12 +299,16 @@ impl Entities { entities: impl IntoIterator, schema: Option<&Schema>, ) -> Result { - 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(), - )?)) + 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 @@ -4677,20 +4681,48 @@ mod entity_validate_tests { 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()), + ( + "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()) - ])) - ])), - ("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()), + ( + "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(), + )]), + ), + ]), + ), + ( + "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(), ); @@ -4705,20 +4737,48 @@ mod entity_validate_tests { 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()), + ( + "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()) - ])) - ])), - ("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()), + ( + "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(), + )]), + ), + ]), + ), + ( + "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")]), ); @@ -4736,20 +4796,48 @@ mod entity_validate_tests { 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()), + ( + "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()) - ])) - ])), - ("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()), + ( + "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(), + )]), + ), + ]), + ), + ( + "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(), ); @@ -4769,20 +4857,48 @@ mod entity_validate_tests { ("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()), + ( + "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()) - ])) - ])), - ("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()), + ( + "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(), + )]), + ), + ]), + ), + ( + "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(), ); diff --git a/cedar-policy/tests/example_use_cases_doc.rs b/cedar-policy/tests/example_use_cases_doc.rs index e8d4d12ad5..b78a20eae9 100644 --- a/cedar-policy/tests/example_use_cases_doc.rs +++ b/cedar-policy/tests/example_use_cases_doc.rs @@ -68,9 +68,7 @@ fn scenario_4a() { // note: 4b currently omitted because it requires date/timestamp functionality /// currently failing, as the validator does not support action attributes -#[should_panic( - expected = "Action::\\\"view\\\"` does not have the attribute `readOnly`" -)] +#[should_panic(expected = "Action::\\\"view\\\"` does not have the attribute `readOnly`")] #[test] fn scenario_4c() { perform_integration_test_from_json(folder().join("4c.json")); From 84be6685648f00d783c52f25bba5ef58c185dd5d Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 16 Oct 2023 18:59:35 +0000 Subject: [PATCH 09/39] changelog tweaks --- cedar-policy/CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index cea407be6c..43ede6f522 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -12,7 +12,7 @@ - Added an API, `unknown_entities`, to `PolicySet` to collect unknown entity UIDs from `PartialResponse`. - Added APIs `remove`, `remove_template` and `unlink` to remove policies from the `PolicySet` - Added API `get_linked_policies` to get the policies linked to a `Template` -- Added `Entity::validate()` to validate an entity against a `Schema` +- Added `Entity::validate()` to validate an entity against a `Schema`. ### Changed @@ -24,9 +24,10 @@ - Implements RFC #19, making validation slightly more strict, but more explainable. - Improved formatting for error messages. - `Entities::from_*()` methods now automatically add action entities present in the `schema` - to the constructed `Entities`, if a `schema` is provided + to the constructed `Entities`, if a `schema` is provided. - `Entities::from_*()` methods now validate the entities against the `schema`, if a `schema` - is provided + is provided. +- `Entities::from_entities()` and `Entities::add_entities()` now take an optional schema argument. ## 2.4.1 From debbb89ddbf83e223c0ae39373b64132596e7b2d Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 17 Oct 2023 14:08:15 +0000 Subject: [PATCH 10/39] fix clippy --- cedar-policy-core/src/entities/json/context.rs | 2 +- cedar-policy-core/src/entities/json/entities.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cedar-policy-core/src/entities/json/context.rs b/cedar-policy-core/src/entities/json/context.rs index 826174ec4e..23e57e8853 100644 --- a/cedar-policy-core/src/entities/json/context.rs +++ b/cedar-policy-core/src/entities/json/context.rs @@ -75,7 +75,7 @@ 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(), || { JsonDeserializationErrorContext::Context diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 931ed76a25..bd459e2d3a 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -253,7 +253,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { } } }; - let vparser = ValueParser::new(self.extensions.clone()); + let vparser = ValueParser::new(self.extensions); let attrs: HashMap = ejson .attrs .into_iter() From 4bee8c421e5f0dcb40602acd34aa7d6547eadcd0 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 15:17:33 +0000 Subject: [PATCH 11/39] adjust wording in comments --- cedar-policy-core/src/entities.rs | 3 +-- cedar-policy/src/api.rs | 12 ++++-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 8b40f63405..a3ad9dea2e 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -118,8 +118,7 @@ impl Entities { /// 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 they are - /// not already present.) + /// (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. diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 7e834af977..a9bc790428 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -289,8 +289,7 @@ impl Entities { /// 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` if they are - /// not already present.) + /// (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. @@ -320,8 +319,7 @@ impl Entities { /// 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` if they are - /// not already present.) + /// (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. @@ -354,8 +352,7 @@ impl Entities { /// 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` if they are - /// not already present.) + /// (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. @@ -388,8 +385,7 @@ impl Entities { /// 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` if they are - /// not already present.) + /// (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. From 38ba22b963336f834172e86ae05a3f49cd60ac95 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 15:46:43 +0000 Subject: [PATCH 12/39] fix bad merge --- cedar-policy-core/src/entities/json/entities.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 3172a1923f..ce042a3eab 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -402,10 +402,9 @@ impl EntityJSON { // PANIC SAFETY unit test code #[allow(clippy::panic)] mod test { - use cool_asserts::assert_matches; - use super::*; + #[test] fn reject_duplicates() { let json = serde_json::json!([ @@ -426,7 +425,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(); From dd5082fbe3c43c4ec9ebce5710861cfa58648097 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 15:51:45 +0000 Subject: [PATCH 13/39] cargo fmt --- cedar-policy-core/src/entities/json/entities.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index ce042a3eab..49745b326c 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -402,8 +402,8 @@ impl EntityJSON { // PANIC SAFETY unit test code #[allow(clippy::panic)] mod test { - use cool_asserts::assert_matches; use super::*; + use cool_asserts::assert_matches; #[test] fn reject_duplicates() { From e07d833ccc4932e6a22d5bfa32d1ea353c8c0411 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 15:52:37 -0400 Subject: [PATCH 14/39] Apply suggestions from code review Co-authored-by: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com> --- cedar-policy-core/src/entities/conformance.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index 567bc5e292..ba3aa73767 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -8,8 +8,7 @@ use thiserror::Error; /// Errors raised when entities do not conform to the schema #[derive(Debug, Error)] pub enum EntitySchemaConformanceError { - /// Encountered this attribute on this entity, but that attribute shouldn't - /// exist on entities of this type + /// 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 @@ -17,9 +16,8 @@ pub enum EntitySchemaConformanceError { /// Name of the attribute that was unexpected attr: SmolStr, }, - /// 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")] + /// 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, @@ -27,7 +25,7 @@ pub enum EntitySchemaConformanceError { attr: SmolStr, }, /// The given attribute on the given entity had a different type than the - /// schema indicated to expect + /// 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 From 59e2b2f5856f2299cb8727036df078810425bc34 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 20:42:26 +0000 Subject: [PATCH 15/39] renames suggested in code review --- cedar-policy-core/src/entities/conformance.rs | 22 +++++----- .../src/entities/json/entities.rs | 35 ++++++++-------- .../src/entities/json/jsonvalue.rs | 40 ++++++++++++------- 3 files changed, 54 insertions(+), 43 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index ba3aa73767..a5a89eda8e 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -182,7 +182,7 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { Some(expected_ty) => { // typecheck: ensure that the entity attribute value matches // the expected type - match type_of_rexpr(val, self.extensions) { + match type_of_restricted_expr(val, self.extensions) { Ok(actual_ty) => { if actual_ty.is_consistent_with(&expected_ty) { // typecheck passes @@ -195,14 +195,14 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { }); } } - Err(TypeOfRexprError::HeterogeneousSet(err)) => { + Err(TypeOfRestrictedExprError::HeterogeneousSet(err)) => { return Err(EntitySchemaConformanceError::HeterogeneousSet { uid: uid.clone(), attr: attr.into(), err, }); } - Err(TypeOfRexprError::Extension(err)) => { + Err(TypeOfRestrictedExprError::Extension(err)) => { return Err(EntitySchemaConformanceError::Extension { uid: uid.clone(), attr: attr.into(), @@ -234,9 +234,9 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { } } -/// Errors thrown by [`type_of_rexpr()`] +/// Errors thrown by [`type_of_restricted_expr()`] #[derive(Debug, Error)] -pub enum TypeOfRexprError { +pub enum TypeOfRestrictedExprError { /// Encountered a heterogeneous set #[error(transparent)] HeterogeneousSet(#[from] HeterogeneousSetError), @@ -250,10 +250,10 @@ pub enum TypeOfRexprError { /// 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( +pub fn type_of_restricted_expr( rexpr: BorrowedRestrictedExpr<'_>, extensions: Extensions<'_>, -) -> Result { +) -> Result { match rexpr.expr_kind() { ExprKind::Lit(Literal::Bool(_)) => Ok(SchemaType::Bool), ExprKind::Lit(Literal::Long(_)) => Ok(SchemaType::Long), @@ -261,13 +261,13 @@ pub fn type_of_rexpr( 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_rexpr(BorrowedRestrictedExpr::new_unchecked(el), extensions) // assuming the invariant holds for the set as a whole, it will also hold for each element + 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 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) }), @@ -283,7 +283,7 @@ pub fn type_of_rexpr( ExprKind::Record { pairs } => { Ok(SchemaType::Record { attrs: { pairs.iter().map(|(k, v)| { - let attr_type = type_of_rexpr( + 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, )?; @@ -291,7 +291,7 @@ pub fn type_of_rexpr( // 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::, TypeOfRexprError>>()? + }).collect::, TypeOfRestrictedExprError>>()? }}) } ExprKind::ExtensionFunctionApp { fn_name, .. } => { diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 49745b326c..ac3dabca0f 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -21,8 +21,8 @@ use super::{ }; use crate::ast::{Entity, EntityType, EntityUID, RestrictedExpr}; use crate::entities::{ - type_of_rexpr, unwrap_or_clone, Entities, EntitiesError, EntitySchemaConformanceError, - TCComputation, TypeOfRexprError, + type_of_restricted_expr, unwrap_or_clone, Entities, EntitiesError, + EntitySchemaConformanceError, TCComputation, TypeOfRestrictedExprError, }; use crate::extensions::Extensions; use serde::{Deserialize, Serialize}; @@ -309,21 +309,22 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { } Some(rexpr) => rexpr, }; - let expected_ty = type_of_rexpr(expected_rexpr.as_borrowed(), self.extensions) - .map_err(|e| match e { - TypeOfRexprError::HeterogeneousSet(err) => { - JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::HeterogeneousSet { - uid: uid.clone(), - attr: k.clone(), - err, - }, - ) - } - TypeOfRexprError::Extension(err) => { - JsonDeserializationError::FailedExtensionFunctionLookup(err) - } - })?; + let expected_ty = + 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::Extension(err) => { + JsonDeserializationError::FailedExtensionFunctionLookup(err) + } + })?; let actual_rexpr = vparser.val_into_rexpr(v, Some(&expected_ty), || { JsonDeserializationErrorContext::EntityAttribute { uid: uid.clone(), diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index d7b0b9be39..11758e7776 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -20,7 +20,9 @@ use super::{ use crate::ast::{ BorrowedRestrictedExpr, Eid, EntityUID, Expr, ExprKind, Literal, Name, RestrictedExpr, }; -use crate::entities::{type_of_rexpr, EntitySchemaConformanceError, EscapeKind, TypeOfRexprError}; +use crate::entities::{ + type_of_restricted_expr, EntitySchemaConformanceError, EscapeKind, TypeOfRestrictedExprError, +}; use crate::extensions::Extensions; use crate::FromNormalizedStr; use serde::{Deserialize, Serialize}; @@ -352,10 +354,13 @@ impl<'e> ValueParser<'e> { let expected = Box::new(expected_ty.clone()); let actual = { let jvalue: JSONValue = serde_json::from_value(val)?; - let ty = type_of_rexpr(jvalue.into_expr()?.as_borrowed(), self.extensions) - .map_err(|e| { - type_of_rexpr_error_to_json_deserialization_error(e, ctx()) - })?; + let ty = type_of_restricted_expr( + jvalue.into_expr()?.as_borrowed(), + self.extensions, + ) + .map_err(|e| { + type_of_restricted_expr_error_to_json_deserialization_error(e, ctx()) + })?; Box::new(ty) }; match ctx() { @@ -419,10 +424,13 @@ impl<'e> ValueParser<'e> { let expected = Box::new(expected_ty.clone()); let actual = { let jvalue: JSONValue = serde_json::from_value(val)?; - let ty = type_of_rexpr(jvalue.into_expr()?.as_borrowed(), self.extensions) - .map_err(|e| { - type_of_rexpr_error_to_json_deserialization_error(e, ctx()) - })?; + let ty = type_of_restricted_expr( + jvalue.into_expr()?.as_borrowed(), + self.extensions, + ) + .map_err(|e| { + type_of_restricted_expr_error_to_json_deserialization_error(e, ctx()) + })?; Box::new(ty) }; match ctx() { @@ -483,8 +491,10 @@ impl<'e> ValueParser<'e> { } ExtnValueJSON::ImplicitConstructor(val) => { let arg = val.into_expr()?; - let argty = type_of_rexpr(arg.as_borrowed(), self.extensions) - .map_err(|e| type_of_rexpr_error_to_json_deserialization_error(e, ctx()))?; + 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( @@ -509,12 +519,12 @@ impl<'e> ValueParser<'e> { } } -fn type_of_rexpr_error_to_json_deserialization_error( - torerr: TypeOfRexprError, +fn type_of_restricted_expr_error_to_json_deserialization_error( + torerr: TypeOfRestrictedExprError, ctx: JsonDeserializationErrorContext, ) -> JsonDeserializationError { match torerr { - TypeOfRexprError::HeterogeneousSet(err) => match ctx { + TypeOfRestrictedExprError::HeterogeneousSet(err) => match ctx { JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { JsonDeserializationError::EntitySchemaConformance( EntitySchemaConformanceError::HeterogeneousSet { uid, attr, err } @@ -525,7 +535,7 @@ fn type_of_rexpr_error_to_json_deserialization_error( } ctx => panic!("heterogenous sets can only occur in entity attributes or in context, but somehow found one {ctx}"), } - TypeOfRexprError::Extension(err) => match ctx { + TypeOfRestrictedExprError::Extension(err) => match ctx { JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { JsonDeserializationError::EntitySchemaConformance( EntitySchemaConformanceError::Extension { uid, attr, err } From 84ad2b9344eea9a58452f7e8b37665cf8eb13d4b Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 16:43:28 -0400 Subject: [PATCH 16/39] Update cedar-policy-core/src/entities/json/err.rs Co-authored-by: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com> --- cedar-policy-core/src/entities/json/err.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index 10348ae5c8..8e38226947 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -138,7 +138,7 @@ pub enum JsonDeserializationError { record_attr: SmolStr, }, /// During schema-based parsing of the Context, found a different type than - /// the schema indicated to expect + /// the schema indicated /// /// (type mismatches in entity attributes will be /// `EntitySchemaConformanceError`) From ee1acb6cad6d4950f942dcbac1b1337d1028f985 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 20:45:38 +0000 Subject: [PATCH 17/39] tweak doc comment --- cedar-policy-core/src/extensions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-core/src/extensions.rs b/cedar-policy-core/src/extensions.rs index 1c30a24860..d51e8a4fc7 100644 --- a/cedar-policy-core/src/extensions.rs +++ b/cedar-policy-core/src/extensions.rs @@ -39,7 +39,7 @@ lazy_static::lazy_static! { /// Holds data on all the Extensions which are active for a given evaluation. /// -/// Clone is cheap for this type, and in fact we commit to this by marking it Copy +/// Clone is cheap for this type. #[derive(Debug, Clone, Copy)] pub struct Extensions<'a> { /// the actual extensions From 03274ae550074e8ee10efcaf08e0cadd88d4483c Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 16:46:26 -0400 Subject: [PATCH 18/39] Update cedar-policy-validator/src/schema.rs Co-authored-by: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com> --- cedar-policy-validator/src/schema.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 8a124a9c5d..a53b64b7ab 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -1218,7 +1218,7 @@ impl ValidatorSchema { } /// Invert the action hierarchy to get the ancestor relation expected for - /// the `Entity` datatype instead of descendant as stored by the schema. + /// 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 From bf23cf3ddf36c686781d61742e9008ad1e852607 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 18 Oct 2023 20:52:28 +0000 Subject: [PATCH 19/39] fix tests --- cedar-policy-core/src/entities.rs | 2 +- cedar-policy/src/api.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index a3ad9dea2e..dd27f738ac 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -2167,7 +2167,7 @@ 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 ); diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index a9bc790428..d65b0c37c9 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -4830,7 +4830,7 @@ mod entity_validate_tests { Ok(_) => panic!("expected an error due to missing attribute `numDirectReports`"), Err(e) => { assert!( - e.to_string().contains(r#"expected entity `Employee::"123"` to have an attribute `numDirectReports`, but it does not"#), + e.to_string().contains(r#"expected entity `Employee::"123"` to have attribute `numDirectReports`, but it does not"#), "actual error message was {e}", ) } From 0eac125e333f5c03bcb4f470b9a0f0b1633092b9 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 19 Oct 2023 16:51:00 +0000 Subject: [PATCH 20/39] change wording in some docs --- cedar-policy/src/api.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index d65b0c37c9..59ee921800 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -257,8 +257,9 @@ impl Entities { /// `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, those entities' definitions must match - /// exactly or an error is returned.) + /// `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 @@ -414,8 +415,8 @@ impl Entities { /// `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, those entities' definitions must match exactly or an - /// error is returned.) + /// 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. @@ -475,8 +476,8 @@ impl Entities { /// `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, those entities' definitions must match exactly or an - /// error is returned.) + /// 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. @@ -529,8 +530,8 @@ impl Entities { /// `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, those entities' definitions must match exactly or an - /// error is returned.) + /// 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. From 3f8534448087f9a35cde5f362b63f48a9e137155 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 19 Oct 2023 16:52:40 +0000 Subject: [PATCH 21/39] change wording in some more docs --- cedar-policy-core/src/entities/json/entities.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index ac3dabca0f..4844fa933f 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -50,8 +50,8 @@ pub struct EntityJsonParser<'e, 's, S: Schema = NoEntitiesSchema> { /// `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, those entities' definitions must match exactly or an - /// error is returned.) + /// 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. @@ -89,8 +89,8 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { /// `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, those entities' definitions must match exactly or an - /// error is returned.) + /// 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. From 94835b66b044856e32d34302c25338a13edbe0fb Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 19 Oct 2023 17:09:06 +0000 Subject: [PATCH 22/39] rename val_into_rexpr --- cedar-policy-core/src/entities/json/context.rs | 2 +- cedar-policy-core/src/entities/json/entities.rs | 17 +++++++++-------- .../src/entities/json/jsonvalue.rs | 8 +++++--- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/cedar-policy-core/src/entities/json/context.rs b/cedar-policy-core/src/entities/json/context.rs index 23e57e8853..c212237bcb 100644 --- a/cedar-policy-core/src/entities/json/context.rs +++ b/cedar-policy-core/src/entities/json/context.rs @@ -77,7 +77,7 @@ impl<'e, 's, S: ContextSchema> ContextJsonParser<'e, 's, S> { ) -> Result { 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 4844fa933f..310c99904c 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -260,7 +260,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { .map(|(k, v)| match &entity_schema_info { EntitySchemaInfo::NoSchema => Ok(( k.clone(), - vparser.val_into_rexpr(v, None, || { + vparser.val_into_restricted_expr(v, None, || { JsonDeserializationErrorContext::EntityAttribute { uid: uid.clone(), attr: k.clone(), @@ -282,7 +282,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { )) } Some(expected_ty) => { - vparser.val_into_rexpr(v, Some(&expected_ty), || { + vparser.val_into_restricted_expr(v, Some(&expected_ty), || { JsonDeserializationErrorContext::EntityAttribute { uid: uid.clone(), attr: k.clone(), @@ -325,12 +325,13 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { JsonDeserializationError::FailedExtensionFunctionLookup(err) } })?; - let actual_rexpr = vparser.val_into_rexpr(v, Some(&expected_ty), || { - JsonDeserializationErrorContext::EntityAttribute { - uid: uid.clone(), - attr: k.clone(), - } - })?; + let actual_rexpr = + vparser.val_into_restricted_expr(v, Some(&expected_ty), || { + JsonDeserializationErrorContext::EntityAttribute { + uid: uid.clone(), + attr: k.clone(), + } + })?; if actual_rexpr == *expected_rexpr { Ok((k, actual_rexpr)) } else { diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index 11758e7776..dcaff8d534 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -312,7 +312,7 @@ impl<'e> ValueParser<'e> { /// `RestrictedExpr`. Performs schema-based parsing if `expected_ty` is /// provided. This does not mean that this function fully validates the /// value against `expected_ty` -- it does not. - pub fn val_into_rexpr( + pub fn val_into_restricted_expr( &self, val: serde_json::Value, expected_ty: Option<&SchemaType>, @@ -347,7 +347,9 @@ 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>>()?, )), _ => { @@ -397,7 +399,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)), } From e3f763dd77be07e5e511079acedcfe56619b75e0 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 19 Oct 2023 17:22:18 +0000 Subject: [PATCH 23/39] add a note to a doc string --- cedar-policy-core/src/entities/conformance.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index a5a89eda8e..eb9eaa9806 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -250,6 +250,16 @@ pub enum TypeOfRestrictedExprError { /// 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<'_>, From 0292dabd53f3455f8e5f24f5cd25fe73fd80dea6 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 19 Oct 2023 17:43:30 +0000 Subject: [PATCH 24/39] remove a panic --- cedar-policy-core/src/entities/json/err.rs | 14 +++++++++++++ .../src/entities/json/jsonvalue.rs | 21 ++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index 8e38226947..586f3fda50 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -158,6 +158,20 @@ pub enum JsonDeserializationError { /// getting information about any extension functions referenced in Context values. #[error("while parsing context, {0}")] ContextExtension(ExtensionFunctionLookupError), + /// Type mismatch somewhere other than entity attributes or context, which + /// would result in `Self::EntitySchemaConformance` or + /// `Self::ContextTypeMismatch` respectively. + /// This should never occur, but is propagated as this error instead of a panic. + #[error("{ctx}, type mismatch: expected type {expected}, but actually has type {actual}")] + OtherTypeMismatch { + /// Context of this error, which will be something other than `EntityAttribute` + /// or `Context` + ctx: Box, + /// Type which was expected + expected: Box, + /// Type which was encountered instead + actual: Box, + }, } /// Errors thrown during serialization to JSON diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index dcaff8d534..b86fd59e95 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -379,7 +379,11 @@ impl<'e> ValueParser<'e> { JsonDeserializationErrorContext::Context => { Err(JsonDeserializationError::ContextTypeMismatch { expected, actual }) } - ctx => panic!("type mismatches can only occur in entity attributes or in context, but somehow found one {ctx}"), + ctx => Err(JsonDeserializationError::OtherTypeMismatch { + ctx: Box::new(ctx), + expected, + actual, + }), } } }, @@ -437,12 +441,23 @@ impl<'e> ValueParser<'e> { }; match ctx() { JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { - Err(JsonDeserializationError::EntitySchemaConformance(EntitySchemaConformanceError::TypeMismatch { uid, attr, expected, actual })) + Err(JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::TypeMismatch { + uid, + attr, + expected, + actual, + }, + )) } JsonDeserializationErrorContext::Context => { Err(JsonDeserializationError::ContextTypeMismatch { expected, actual }) } - ctx => panic!("type mismatches can only occur in entity attributes or in context, but somehow found one {ctx}") + ctx => Err(JsonDeserializationError::OtherTypeMismatch { + ctx: Box::new(ctx), + expected, + actual, + }), } } }, From ce306ebab299ae8288edde74fedd2091259cbaeb Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 19 Oct 2023 19:06:32 +0000 Subject: [PATCH 25/39] remove more panics --- cedar-policy-core/src/entities/json/err.rs | 26 ++++++++++++++++++- .../src/entities/json/jsonvalue.rs | 8 ++++-- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index 586f3fda50..d6ceede239 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -115,7 +115,7 @@ pub enum JsonDeserializationError { /// During schema-based parsing, encountered an entity which does not /// conform to the schema. /// - /// This error contains the Entity analogues of the Record error cases + /// This error contains the Entity analogues of the Context error cases /// listed below, among other things. #[error(transparent)] EntitySchemaConformance(EntitySchemaConformanceError), @@ -172,6 +172,30 @@ pub enum JsonDeserializationError { /// Type which was encountered instead actual: Box, }, + /// Heterogeneous set found somewhere other than entity attributes or + /// context, which would result in `Self::EntitySchemaConformance` or + /// `Self::ContextHeterogeneousSet` respectively. + /// This should never occur, but is propagated as this error instead of a panic. + #[error("{ctx}, {err}")] + OtherHeterogeneousSet { + /// Context of this error, which will be something other than + /// `EntityAttribute` or `Context` + ctx: Box, + /// Underlying error + err: HeterogeneousSetError, + }, + /// Extension function lookup error somewhere other than entity attributes + /// or context, which would result in `Self::EntitySchemaConformance` or + /// `Self::ContextExtension` respectively. + /// This should never occur, but is propagated as this error instead of a panic. + #[error("{ctx}, {err}")] + OtherExtension { + /// Context of this error, which will be something other than + /// `EntityAttribute` or `Context` + ctx: Box, + /// Underlying error + err: ExtensionFunctionLookupError, + }, } /// Errors thrown during serialization to JSON diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index b86fd59e95..21d51de6a8 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -550,7 +550,9 @@ fn type_of_restricted_expr_error_to_json_deserialization_error( JsonDeserializationErrorContext::Context => { JsonDeserializationError::ContextHeterogeneousSet(err) } - ctx => panic!("heterogenous sets can only occur in entity attributes or in context, but somehow found one {ctx}"), + ctx => { + JsonDeserializationError::OtherHeterogeneousSet { ctx: Box::new(ctx), err } + } } TypeOfRestrictedExprError::Extension(err) => match ctx { JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { @@ -561,7 +563,9 @@ fn type_of_restricted_expr_error_to_json_deserialization_error( JsonDeserializationErrorContext::Context => { JsonDeserializationError::ContextExtension(err) } - ctx => panic!("failed extension function lookups can only occur in entity attributes or in context, but somehow found one {ctx}"), + ctx => { + JsonDeserializationError::OtherExtension { ctx: Box::new(ctx), err } + } } } } From 8d30a07c29cc4178a7fe496a1c4121f8c71280d8 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 19 Oct 2023 19:06:44 +0000 Subject: [PATCH 26/39] cargo fmt --- .../src/entities/json/jsonvalue.rs | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index 21d51de6a8..eabbd7dd9a 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -541,34 +541,36 @@ fn type_of_restricted_expr_error_to_json_deserialization_error( ctx: JsonDeserializationErrorContext, ) -> JsonDeserializationError { match torerr { - TypeOfRestrictedExprError::HeterogeneousSet(err) => match ctx { - JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { - JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::HeterogeneousSet { uid, attr, err } - ) - } - JsonDeserializationErrorContext::Context => { - JsonDeserializationError::ContextHeterogeneousSet(err) - } - ctx => { - JsonDeserializationError::OtherHeterogeneousSet { ctx: Box::new(ctx), err } - } - } - TypeOfRestrictedExprError::Extension(err) => match ctx { - JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { - JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::Extension { uid, attr, err } - ) - } - JsonDeserializationErrorContext::Context => { - JsonDeserializationError::ContextExtension(err) - } - ctx => { - JsonDeserializationError::OtherExtension { ctx: Box::new(ctx), err } - } + TypeOfRestrictedExprError::HeterogeneousSet(err) => match ctx { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::HeterogeneousSet { uid, attr, err }, + ) + } + JsonDeserializationErrorContext::Context => { + JsonDeserializationError::ContextHeterogeneousSet(err) + } + ctx => JsonDeserializationError::OtherHeterogeneousSet { + ctx: Box::new(ctx), + err, + }, + }, + TypeOfRestrictedExprError::Extension(err) => match ctx { + JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::Extension { uid, attr, err }, + ) + } + JsonDeserializationErrorContext::Context => { + JsonDeserializationError::ContextExtension(err) + } + ctx => JsonDeserializationError::OtherExtension { + ctx: Box::new(ctx), + err, + }, + }, } } -} /// Serde JSON format for Cedar values where we know we're expecting an entity /// reference From ac9bcb7a4a500edf220a268416a6312146967e0e Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 20 Oct 2023 13:43:06 +0000 Subject: [PATCH 27/39] a number of tests require decimal extension, not just ipaddr --- cedar-policy-core/src/entities.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index dd27f738ac..73c855bb71 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -1524,7 +1524,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() { @@ -1731,7 +1731,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() { @@ -1777,7 +1777,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() { @@ -1824,7 +1824,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() { @@ -1867,7 +1867,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() { @@ -1913,7 +1913,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] @@ -1960,7 +1960,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 @@ -2005,7 +2005,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() { @@ -2082,7 +2082,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() { @@ -2129,6 +2129,7 @@ mod schema_based_parsing_tests { ); } + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// entity is missing a required attribute #[test] fn missing_required_attr() { @@ -2173,7 +2174,7 @@ mod schema_based_parsing_tests { ); } - #[cfg(feature = "ipaddr")] + #[cfg(all(feature = "decimal", feature = "ipaddr"))] /// unexpected entity attribute #[test] fn unexpected_entity_attr() { @@ -2222,7 +2223,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() { From 84df846abb2e7f4ddf193e60ae19310b9ba9c48f Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 24 Oct 2023 14:14:57 +0000 Subject: [PATCH 28/39] fix bad merge --- cedar-policy-core/src/entities/json/entities.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 2cefcb1fda..e552f5591d 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -217,7 +217,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { 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` From 6dfef835ba4d4c6e5d5ee80ea666850af99d0120 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 24 Oct 2023 14:19:33 +0000 Subject: [PATCH 29/39] imports nit --- cedar-policy/src/api.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 6a3af25a3a..aaccabb5a4 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -26,9 +26,10 @@ use cedar_policy_core::ast; use cedar_policy_core::ast::RestrictedExprError; use cedar_policy_core::authorizer; pub use cedar_policy_core::authorizer::AuthorizationError; -use cedar_policy_core::entities::JsonDeserializationErrorContext; -use cedar_policy_core::entities::{self, EntitySchemaConformanceChecker}; -use cedar_policy_core::entities::{ContextSchema, Dereference, JsonDeserializationError}; +use cedar_policy_core::entities::{ + self, ContextSchema, Dereference, EntitySchemaConformanceChecker, JsonDeserializationError, + JsonDeserializationErrorContext, +}; use cedar_policy_core::est; pub use cedar_policy_core::evaluator::{EvaluationError, EvaluationErrorKind}; use cedar_policy_core::evaluator::{Evaluator, RestrictedEvaluator}; From 41a7a09a122423dc70cbd61fe576cfd0f2cd7aa8 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 24 Oct 2023 11:57:47 -0400 Subject: [PATCH 30/39] Apply suggestions from code review Co-authored-by: Kesha Hietala --- cedar-policy-core/src/entities/conformance.rs | 2 +- cedar-policy-core/src/entities/json/entities.rs | 6 +++--- cedar-policy-core/src/entities/json/err.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index eb9eaa9806..ca699be5c0 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -247,7 +247,7 @@ pub enum TypeOfRestrictedExprError { /// Get the [`SchemaType`] of a restricted expression. /// -/// This isn't possible for general `Expr`s (without a Request, full schema, +/// 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`. /// diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index e552f5591d..18c4f0506e 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -184,7 +184,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { self.iter_ejson_to_iter_entity(ejsons) } - /// internal function that converts an iterator over [`EntityJson`] into an + /// 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( @@ -201,7 +201,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { 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 @@ -217,7 +217,7 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { 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` diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index bf092ada4f..16a6773d92 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -115,7 +115,7 @@ pub enum JsonDeserializationError { /// During schema-based parsing, encountered an entity which does not /// conform to the schema. /// - /// This error contains the Entity analogues of the Context error cases + /// This error contains the `Entity` analogues of the `Context` error cases /// listed below, among other things. #[error(transparent)] EntitySchemaConformance(EntitySchemaConformanceError), From 65ee245b024aa377f0cf8d9c9cc0dbbf9e3ef9ed Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 24 Oct 2023 15:58:10 +0000 Subject: [PATCH 31/39] more backticks in doc comments --- cedar-policy-core/src/entities/json/err.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index 16a6773d92..b80f456294 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -137,8 +137,8 @@ pub enum JsonDeserializationError { /// Name of the (Record) attribute which was expected record_attr: SmolStr, }, - /// During schema-based parsing of the Context, found a different type than - /// the schema indicated + /// During schema-based parsing of the `Context`, found a different type + /// than the schema indicated /// /// (type mismatches in entity attributes will be /// `EntitySchemaConformanceError`) @@ -149,13 +149,13 @@ pub enum JsonDeserializationError { /// Type which was encountered instead actual: Box, }, - /// During schema-based parsing of the Context, found a set whose elements + /// During schema-based parsing of the `Context`, found a set whose elements /// don't all have the same type. This doesn't match any possible schema. #[error("while parsing context, {0}")] ContextHeterogeneousSet(HeterogeneousSetError), - /// During schema-based parsing of the Context, error looking up an extension function. - /// This error can occur when parsing the Context because that may require - /// getting information about any extension functions referenced in Context values. + /// During schema-based parsing of the `Context`, error looking up an extension function. + /// This error can occur when parsing the `Context` because that may require + /// getting information about any extension functions referenced in `Context` values. #[error("while parsing context, {0}")] ContextExtension(ExtensionFunctionLookupError), /// Type mismatch somewhere other than entity attributes or context, which From 5426eeabacbcafa5322ed5a0ae3371999e64427e Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 24 Oct 2023 16:16:06 +0000 Subject: [PATCH 32/39] fix doc comment --- cedar-policy/src/api.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index aaccabb5a4..e041d48d78 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -286,12 +286,10 @@ impl Entities { /// Add all of the [`Entity`]s in the collection to 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. - /// 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. + /// 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 From ea002dcdfa902b448605cb7113d0ff470f82fb10 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 24 Oct 2023 17:53:27 +0000 Subject: [PATCH 33/39] update doc comment on private field --- cedar-policy-core/src/entities/json/entities.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 18c4f0506e..9cba45b28c 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -47,20 +47,11 @@ pub struct EntityJson { /// Struct used to parse entities from JSON. #[derive(Debug, Clone)] pub struct EntityJsonParser<'e, 's, S: Schema = NoEntitiesSchema> { - /// `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.) + /// See comments on [`EntityJsonParser::new()`] for the interpretation and + /// effects of this `schema` field. /// - /// 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. + /// (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. From 1aef41d57a0fe3bfd8c68b19174400ebe73eae23 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 24 Oct 2023 20:42:45 +0000 Subject: [PATCH 34/39] cargo fmt --- cedar-policy/src/api.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index a741cc4215..89f5c1d5bb 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -4699,9 +4699,11 @@ mod entity_validate_tests { RestrictedExpression::new_record([( "innerinner".into(), RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), - )]).unwrap(), + )]) + .unwrap(), ), - ]).unwrap(), + ]) + .unwrap(), ), ( "home_ip".into(), @@ -4755,9 +4757,11 @@ mod entity_validate_tests { RestrictedExpression::new_record([( "innerinner".into(), RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), - )]).unwrap(), + )]) + .unwrap(), ), - ]).unwrap(), + ]) + .unwrap(), ), ( "home_ip".into(), @@ -4814,9 +4818,11 @@ mod entity_validate_tests { RestrictedExpression::new_record([( "innerinner".into(), RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), - )]).unwrap(), + )]) + .unwrap(), ), - ]).unwrap(), + ]) + .unwrap(), ), ( "home_ip".into(), @@ -4875,9 +4881,11 @@ mod entity_validate_tests { RestrictedExpression::new_record([( "innerinner".into(), RestrictedExpression::from_str(r#"Employee::"abc""#).unwrap(), - )]).unwrap(), + )]) + .unwrap(), ), - ]).unwrap(), + ]) + .unwrap(), ), ( "home_ip".into(), From de4e2ee4bbbb3c27a06644a7e5feb4e174b4c1e6 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 25 Oct 2023 14:22:47 +0000 Subject: [PATCH 35/39] reorg/simplify errors --- cedar-policy-core/src/entities/conformance.rs | 18 ++-- .../src/entities/json/entities.rs | 10 +- cedar-policy-core/src/entities/json/err.rs | 95 +++++++++---------- cedar-policy-core/src/entities/json/value.rs | 30 +++--- 4 files changed, 72 insertions(+), 81 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index e9a5d97817..4d1d933348 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -91,7 +91,7 @@ pub enum EntitySchemaConformanceError { /// 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}")] - Extension { + ExtensionFunctionLookup { /// Entity where the error occurred uid: EntityUID, /// Name of the attribute where the error occurred @@ -202,12 +202,14 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { err, }); } - Err(TypeOfRestrictedExprError::Extension(err)) => { - return Err(EntitySchemaConformanceError::Extension { - uid: uid.clone(), - attr: attr.into(), - err, - }); + Err(TypeOfRestrictedExprError::ExtensionFunctionLookup(err)) => { + return Err( + EntitySchemaConformanceError::ExtensionFunctionLookup { + uid: uid.clone(), + attr: attr.into(), + err, + }, + ); } } } @@ -242,7 +244,7 @@ pub enum TypeOfRestrictedExprError { HeterogeneousSet(#[from] HeterogeneousSetError), /// Error looking up an extension function #[error(transparent)] - Extension(#[from] ExtensionFunctionLookupError), + ExtensionFunctionLookup(#[from] ExtensionFunctionLookupError), } /// Get the [`SchemaType`] of a restricted expression. diff --git a/cedar-policy-core/src/entities/json/entities.rs b/cedar-policy-core/src/entities/json/entities.rs index 17c99aeada..078cb1d5b0 100644 --- a/cedar-policy-core/src/entities/json/entities.rs +++ b/cedar-policy-core/src/entities/json/entities.rs @@ -319,8 +319,14 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> { }, ) } - TypeOfRestrictedExprError::Extension(err) => { - JsonDeserializationError::FailedExtensionFunctionLookup(err) + TypeOfRestrictedExprError::ExtensionFunctionLookup(err) => { + JsonDeserializationError::EntitySchemaConformance( + EntitySchemaConformanceError::ExtensionFunctionLookup { + uid: uid.clone(), + attr: k.clone(), + err, + }, + ) } })?; let actual_rexpr = diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs index 6891018261..174ce744d5 100644 --- a/cedar-policy-core/src/entities/json/err.rs +++ b/cedar-policy-core/src/entities/json/err.rs @@ -68,9 +68,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 `{got}`")] ExpectedLiteralEntityRef { @@ -112,10 +109,18 @@ pub enum JsonDeserializationError { /// argument type of the constructor we were looking for arg_type: 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 + 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 of the `Context` error cases + /// This error contains the `Entity` analogues some of the other errors /// listed below, among other things. #[error(transparent)] EntitySchemaConformance(EntitySchemaConformanceError), @@ -137,73 +142,59 @@ pub enum JsonDeserializationError { /// Name of the (Record) attribute which was expected record_attr: SmolStr, }, - /// During schema-based parsing of the `Context`, found a different type - /// than the schema indicated + /// During schema-based parsing, found a different type than the schema indicated. /// - /// (type mismatches in entity attributes will be - /// `EntitySchemaConformanceError`) - #[error("while parsing context, type mismatch: expected type {expected}, but actually has type {actual}")] - ContextTypeMismatch { - /// Type which was expected - expected: Box, - /// Type which was encountered instead - actual: Box, - }, - /// During schema-based parsing of the `Context`, found a set whose elements - /// don't all have the same type. This doesn't match any possible schema. - #[error("while parsing context, {0}")] - ContextHeterogeneousSet(HeterogeneousSetError), - /// During schema-based parsing of the `Context`, error looking up an extension function. - /// This error can occur when parsing the `Context` because that may require - /// getting information about any extension functions referenced in `Context` values. - #[error("while parsing context, {0}")] - ContextExtension(ExtensionFunctionLookupError), - /// Type mismatch somewhere other than entity attributes or context, which - /// would result in `Self::EntitySchemaConformance` or - /// `Self::ContextTypeMismatch` respectively. - /// This should never occur, but is propagated as this error instead of a panic. + /// (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}")] - OtherTypeMismatch { - /// Context of this error, which will be something other than `EntityAttribute` - /// or `Context` + TypeMismatch { + /// 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, }, - /// Heterogeneous set found somewhere other than entity attributes or - /// context, which would result in `Self::EntitySchemaConformance` or - /// `Self::ContextHeterogeneousSet` respectively. - /// This should never occur, but is propagated as this error instead of a panic. + /// 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}")] - OtherHeterogeneousSet { - /// Context of this error, which will be something other than - /// `EntityAttribute` or `Context` + HeterogeneousSet { + /// Context of this error, which will be something other than `EntityAttribute`. + /// (Heterogeneous sets in entity attributes are reported as + /// `Self::EntitySchemaConformance`.) ctx: Box, /// Underlying error err: HeterogeneousSetError, }, - /// Extension function lookup error somewhere other than entity attributes - /// or context, which would result in `Self::EntitySchemaConformance` or - /// `Self::ContextExtension` respectively. - /// This should never occur, but is propagated as this error instead of a panic. + /// 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}")] - OtherExtension { + ExtensionFunctionLookup { /// Context of this error, which will be something other than - /// `EntityAttribute` or `Context` + /// `EntityAttribute`. + /// (Extension function lookup errors in entity attributes are reported + /// as `Self::EntitySchemaConformance`.) ctx: Box, /// Underlying error err: ExtensionFunctionLookupError, }, - /// 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, - }, } /// Errors thrown during serialization to JSON diff --git a/cedar-policy-core/src/entities/json/value.rs b/cedar-policy-core/src/entities/json/value.rs index 57a01cde47..dea84f8df6 100644 --- a/cedar-policy-core/src/entities/json/value.rs +++ b/cedar-policy-core/src/entities/json/value.rs @@ -427,10 +427,7 @@ impl<'e> ValueParser<'e> { }, )) } - JsonDeserializationErrorContext::Context => { - Err(JsonDeserializationError::ContextTypeMismatch { expected, actual }) - } - ctx => Err(JsonDeserializationError::OtherTypeMismatch { + ctx => Err(JsonDeserializationError::TypeMismatch { ctx: Box::new(ctx), expected, actual, @@ -512,10 +509,7 @@ impl<'e> ValueParser<'e> { }, )) } - JsonDeserializationErrorContext::Context => { - Err(JsonDeserializationError::ContextTypeMismatch { expected, actual }) - } - ctx => Err(JsonDeserializationError::OtherTypeMismatch { + ctx => Err(JsonDeserializationError::TypeMismatch { ctx: Box::new(ctx), expected, actual, @@ -583,7 +577,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 { @@ -611,24 +609,18 @@ fn type_of_restricted_expr_error_to_json_deserialization_error( EntitySchemaConformanceError::HeterogeneousSet { uid, attr, err }, ) } - JsonDeserializationErrorContext::Context => { - JsonDeserializationError::ContextHeterogeneousSet(err) - } - ctx => JsonDeserializationError::OtherHeterogeneousSet { + ctx => JsonDeserializationError::HeterogeneousSet { ctx: Box::new(ctx), err, }, }, - TypeOfRestrictedExprError::Extension(err) => match ctx { + TypeOfRestrictedExprError::ExtensionFunctionLookup(err) => match ctx { JsonDeserializationErrorContext::EntityAttribute { uid, attr } => { JsonDeserializationError::EntitySchemaConformance( - EntitySchemaConformanceError::Extension { uid, attr, err }, + EntitySchemaConformanceError::ExtensionFunctionLookup { uid, attr, err }, ) } - JsonDeserializationErrorContext::Context => { - JsonDeserializationError::ContextExtension(err) - } - ctx => JsonDeserializationError::OtherExtension { + ctx => JsonDeserializationError::ExtensionFunctionLookup { ctx: Box::new(ctx), err, }, From 8037b64b1d3230950e536a8c505f8c1370831fb3 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 26 Oct 2023 14:21:43 +0000 Subject: [PATCH 36/39] fix bad merge --- cedar-policy/CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index da04c76622..9fb159ab94 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -42,7 +42,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the behavior of `.isInRange()`. - Standardize on duplicates being errors instead of last-write-wins in the JSON-based `is_authorized` APIs. - `::Error` is now `Infallible` instead of `ParseErrors`. -- Improved formatting for error messages. ## [2.4.2] - 2023-10-23 From 588e76b73f06f12400d125455233ff07aa5b814c Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 27 Oct 2023 13:22:59 +0000 Subject: [PATCH 37/39] remove Entity::validate() --- cedar-policy/CHANGELOG.md | 1 - cedar-policy/src/api.rs | 9 --------- 2 files changed, 10 deletions(-) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 4d0f973209..0031e58f78 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -17,7 +17,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Experimental API `PolicySet::unknown_entities` to collect unknown entity UIDs from a `PartialResponse`. - `PolicySet::remove_static`, `PolicySet::remove_template` and `PolicySet::unlink` to remove policies from the policy set. - `PolicySet::get_linked_policies` to get the policies linked to a `Template`. -- `Entity::validate()` to validate an entity against a `Schema`. ### Changed diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 7b42f52fa5..90262cf034 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -191,15 +191,6 @@ impl Entity { .map(EvalResult::from), ) } - - /// Validate this `Entity` against the given `schema`. - /// - /// If the entity does not conform to the `schema`, an error is returned. - pub fn validate(&self, schema: &Schema) -> Result<(), impl std::error::Error> { - let schema = cedar_policy_validator::CoreSchema::new(&schema.0); - let checker = EntitySchemaConformanceChecker::new(&schema, Extensions::all_available()); - checker.validate_entity(&self.0) - } } impl std::fmt::Display for Entity { From f1fc4872c29e05249983a53de97eb11eaf3063e7 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 27 Oct 2023 13:41:53 +0000 Subject: [PATCH 38/39] fix tests --- cedar-policy/src/api.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 90262cf034..f6827a6ad3 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -4642,7 +4642,7 @@ mod ancestors_tests { } } -/// These tests are for `Entity::validate()`. +/// A few tests of validating entities. /// Many other validation-related tests are in the separate module focusing on /// schema-based parsing. #[cfg(test)] @@ -4694,6 +4694,11 @@ mod entity_validate_tests { .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( @@ -4748,7 +4753,7 @@ mod entity_validate_tests { ]), HashSet::new(), ); - entity.validate(&schema()).unwrap(); + validate_entity(entity, &schema()).unwrap(); } #[test] @@ -4806,7 +4811,7 @@ mod entity_validate_tests { ]), HashSet::from_iter([EntityUid::from_strs("Manager", "jane")]), ); - match entity.validate(&schema) { + match validate_entity(entity, &schema) { Ok(_) => panic!("expected an error due to extraneous parent"), Err(e) => { assert!( @@ -4867,7 +4872,7 @@ mod entity_validate_tests { ]), HashSet::new(), ); - match entity.validate(&schema) { + match validate_entity(entity, &schema) { Ok(_) => panic!("expected an error due to missing attribute `numDirectReports`"), Err(e) => { assert!( @@ -4930,7 +4935,7 @@ mod entity_validate_tests { ]), HashSet::new(), ); - match entity.validate(&schema) { + match validate_entity(entity, &schema) { Ok(_) => panic!("expected an error due to extraneous attribute"), Err(e) => { assert!( @@ -4945,7 +4950,7 @@ mod entity_validate_tests { HashMap::new(), HashSet::new(), ); - match entity.validate(&schema) { + match validate_entity(entity, &schema) { Ok(_) => panic!("expected an error due to unexpected entity type"), Err(e) => { assert!( From e65427801d647873cc5d22b96d55d92cea082025 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 27 Oct 2023 13:48:20 +0000 Subject: [PATCH 39/39] fix warning --- cedar-policy/src/api.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index f6827a6ad3..cf76f07196 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -27,8 +27,7 @@ use cedar_policy_core::ast::{ExprConstructionError, RestrictedExprError}; use cedar_policy_core::authorizer; pub use cedar_policy_core::authorizer::AuthorizationError; use cedar_policy_core::entities::{ - self, ContextSchema, Dereference, EntitySchemaConformanceChecker, JsonDeserializationError, - JsonDeserializationErrorContext, + self, ContextSchema, Dereference, JsonDeserializationError, JsonDeserializationErrorContext, }; use cedar_policy_core::est; pub use cedar_policy_core::evaluator::{EvaluationError, EvaluationErrorKind};