From 1512107b1e02385be6b12fdda23c97dd8ba757f9 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Thu, 2 Jan 2025 15:17:25 -0500 Subject: [PATCH 1/2] Propagate some source locations from cedar syntax schema parser to JSON syntax structures Signed-off-by: Craig Disselkoen --- Cargo.lock | 1 + cedar-policy-validator/Cargo.toml | 1 + .../src/cedar_schema/test.rs | 3 ++ .../src/cedar_schema/to_json_schema.rs | 46 +++++++++---------- cedar-policy-validator/src/json_schema.rs | 45 ++++++++++++++++-- cedar-policy-validator/src/lib.rs | 3 ++ cedar-policy-validator/src/rbac.rs | 22 +++++++++ .../src/typecheck/test/expr.rs | 2 + 8 files changed, 97 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b90336ba5..c3e1bd0e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -386,6 +386,7 @@ dependencies = [ "arbitrary", "cedar-policy-core", "cool_asserts", + "educe", "itertools 0.13.0", "lalrpop", "lalrpop-util", diff --git a/cedar-policy-validator/Cargo.toml b/cedar-policy-validator/Cargo.toml index bc42f34a5..01dc9f4ce 100644 --- a/cedar-policy-validator/Cargo.toml +++ b/cedar-policy-validator/Cargo.toml @@ -26,6 +26,7 @@ arbitrary = { version = "1", features = ["derive"], optional = true } lalrpop-util = { version = "0.22.0", features = ["lexer", "unicode"] } lazy_static = "1.4.0" nonempty = "0.10.0" +educe = "0.6.0" # wasm dependencies serde-wasm-bindgen = { version = "0.6", optional = true } diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs index a5c48b2ef..c345b128f 100644 --- a/cedar-policy-validator/src/cedar_schema/test.rs +++ b/cedar-policy-validator/src/cedar_schema/test.rs @@ -343,6 +343,7 @@ mod demo_tests { applies_to: None, member_of: None, annotations: Annotations::new(), + loc: None, }; let namespace = json_schema::NamespaceDefinition::new(empty(), once(("foo".to_smolstr(), action))); @@ -445,6 +446,7 @@ namespace Baz {action "Foo" appliesTo { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, )]), actions: BTreeMap::from([( @@ -458,6 +460,7 @@ namespace Baz {action "Foo" appliesTo { }), member_of: None, annotations: Annotations::new(), + loc: None, }, )]), annotations: Annotations::new(), diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index d84190c71..fe66c9a3f 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -193,16 +193,17 @@ impl TryFrom> for json_schema::NamespaceDefinition let common_types = common_types .into_iter() .map(|decl| { - let name_loc = decl.data.name.loc.clone(); - let id = UnreservedId::try_from(decl.data.name.node) + let name_loc = decl.data.node.name.loc.clone(); + let id = UnreservedId::try_from(decl.data.node.name.node) .map_err(|e| ToJsonSchemaError::reserved_name(e.name(), name_loc.clone()))?; let ctid = json_schema::CommonTypeId::new(id) .map_err(|e| ToJsonSchemaError::reserved_keyword(&e.id, name_loc))?; Ok(( ctid, CommonType { - ty: cedar_type_to_json_type(decl.data.def), + ty: cedar_type_to_json_type(decl.data.node.def), annotations: decl.annotations.into(), + loc: Some(decl.data.loc), }, )) }) @@ -219,13 +220,13 @@ impl TryFrom> for json_schema::NamespaceDefinition /// Converts action type decls fn convert_action_decl( - a: Annotated, + a: Annotated>, ) -> Result)>, ToJsonSchemaErrors> { let ActionDecl { names, parents, app_decls, - } = a.data; + } = a.data.node; // Create the internal type from the 'applies_to' clause and 'member_of' let applies_to = app_decls .map(|decls| convert_app_decls(&names.first().node, &names.first().loc, decls)) @@ -241,6 +242,7 @@ fn convert_action_decl( applies_to: Some(applies_to), member_of, annotations: a.annotations.into(), + loc: Some(a.data.loc), }; // Then map that type across all of the bound names Ok(names.into_iter().map(move |name| (name.node, ty.clone()))) @@ -356,7 +358,7 @@ fn convert_id(node: Node) -> Result { /// Convert Entity declarations fn convert_entity_decl( - e: Annotated, + e: Annotated>, ) -> Result< impl Iterator)>, ToJsonSchemaErrors, @@ -365,24 +367,21 @@ fn convert_entity_decl( let etype = json_schema::EntityType { member_of_types: e .data + .node .member_of_types .into_iter() .map(RawName::from) .collect(), - shape: convert_attr_decls(e.data.attrs), - tags: e.data.tags.map(cedar_type_to_json_type), + shape: convert_attr_decls(e.data.node.attrs), + tags: e.data.node.tags.map(cedar_type_to_json_type), annotations: e.annotations.into(), + loc: Some(e.data.loc), }; // Then map over all of the bound names - collect_all_errors( - e.data - .names - .into_iter() - .map(move |name| -> Result<_, ToJsonSchemaErrors> { - Ok((convert_id(name)?, etype.clone())) - }), - ) + collect_all_errors(e.data.node.names.into_iter().map( + move |name| -> Result<_, ToJsonSchemaErrors> { Ok((convert_id(name)?, etype.clone())) }, + )) } /// Create a [`json_schema::AttributesOrContext`] from a series of `AttrDecl`s @@ -642,28 +641,29 @@ fn partition_decls( } fn into_partition_decls( - decls: Vec>>, + decls: impl IntoIterator>>, ) -> ( - Vec>, - Vec>, - Vec>, + Vec>>, + Vec>>, + Vec>>, ) { let mut entities = vec![]; let mut actions = vec![]; let mut types = vec![]; for decl in decls.into_iter() { + let loc = decl.data.loc; match decl.data.node { Declaration::Entity(e) => entities.push(Annotated { - data: e, + data: Node { node: e, loc }, annotations: decl.annotations, }), Declaration::Action(a) => actions.push(Annotated { - data: a, + data: Node { node: a, loc }, annotations: decl.annotations, }), Declaration::Type(t) => types.push(Annotated { - data: t, + data: Node { node: t, loc }, annotations: decl.annotations, }), } diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index d5825c690..b2017ce01 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -21,8 +21,10 @@ use cedar_policy_core::{ entities::CedarValueJson, est::Annotations, extensions::Extensions, + parser::Loc, FromNormalizedStr, }; +use educe::Educe; use nonempty::nonempty; use serde::{ de::{MapAccess, Visitor}, @@ -48,7 +50,8 @@ use crate::{ }; /// Represents the definition of a common type in the schema. -#[derive(Debug, Clone, Serialize, PartialEq, Eq, Deserialize)] +#[derive(Educe, Debug, Clone, Serialize, Deserialize)] +#[educe(PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] pub struct CommonType { /// The referred type @@ -58,6 +61,14 @@ pub struct CommonType { #[serde(default)] #[serde(skip_serializing_if = "Annotations::is_empty")] pub annotations: Annotations, + /// Source location + /// + /// (As of this writing, this is not populated when parsing from JSON. + /// It is only populated if constructing this structure from the + /// corresponding Cedar-syntax structure.) + #[serde(skip)] + #[educe(PartialEq(ignore))] + pub loc: Option, } /// A [`Fragment`] is split into multiple namespace definitions, and is just a @@ -353,6 +364,7 @@ impl NamespaceDefinition { CommonType { ty: v.ty.conditionally_qualify_type_references(ns), annotations: v.annotations, + loc: v.loc, }, ) }) @@ -393,6 +405,7 @@ impl NamespaceDefinition { CommonType { ty: v.ty.fully_qualify_type_references(all_defs)?, annotations: v.annotations, + loc: v.loc, }, )) }) @@ -420,7 +433,8 @@ impl NamespaceDefinition { /// The parameter `N` is the type of entity type names and common type names in /// this [`EntityType`], including recursively. /// See notes on [`Fragment`]. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Educe, Debug, Clone, Serialize, Deserialize)] +#[educe(PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] #[serde(deny_unknown_fields)] #[serde(rename_all = "camelCase")] @@ -444,6 +458,14 @@ pub struct EntityType { #[serde(default)] #[serde(skip_serializing_if = "Annotations::is_empty")] pub annotations: Annotations, + /// Source location + /// + /// (As of this writing, this is not populated when parsing from JSON. + /// It is only populated if constructing this structure from the + /// corresponding Cedar-syntax structure.) + #[serde(skip)] + #[educe(PartialEq(ignore))] + pub loc: Option, } impl EntityType { @@ -463,6 +485,7 @@ impl EntityType { .tags .map(|ty| ty.conditionally_qualify_type_references(ns)), annotations: self.annotations, + loc: self.loc, } } } @@ -490,6 +513,7 @@ impl EntityType { .map(|ty| ty.fully_qualify_type_references(all_defs)) .transpose()?, annotations: self.annotations, + loc: self.loc, }) } } @@ -575,7 +599,8 @@ impl AttributesOrContext { /// The parameter `N` is the type of entity type names and common type names in /// this [`ActionType`], including recursively. /// See notes on [`Fragment`]. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Educe, Debug, Clone, Serialize, Deserialize)] +#[educe(PartialEq, Eq)] #[serde(bound(deserialize = "N: Deserialize<'de> + From"))] #[serde(deny_unknown_fields)] #[serde(rename_all = "camelCase")] @@ -600,6 +625,14 @@ pub struct ActionType { #[serde(default)] #[serde(skip_serializing_if = "Annotations::is_empty")] pub annotations: Annotations, + /// Source location + /// + /// (As of this writing, this is not populated when parsing from JSON. + /// It is only populated if constructing this structure from the + /// corresponding Cedar-syntax structure.) + #[serde(skip)] + #[educe(PartialEq(ignore))] + pub loc: Option, } impl ActionType { @@ -619,6 +652,7 @@ impl ActionType { .collect() }), annotations: self.annotations, + loc: self.loc, } } } @@ -649,6 +683,7 @@ impl ActionType { }) .transpose()?, annotations: self.annotations, + loc: self.loc, }) } } @@ -2814,6 +2849,7 @@ mod test_json_roundtrip { }))), tags: None, annotations: Annotations::new(), + loc: None, }, )]), actions: BTreeMap::from([( @@ -2832,6 +2868,7 @@ mod test_json_roundtrip { }), member_of: None, annotations: Annotations::new(), + loc: None, }, )]), annotations: Annotations::new(), @@ -2859,6 +2896,7 @@ mod test_json_roundtrip { ))), tags: None, annotations: Annotations::new(), + loc: None, }, )]), actions: BTreeMap::new(), @@ -2886,6 +2924,7 @@ mod test_json_roundtrip { }), member_of: None, annotations: Annotations::new(), + loc: None, }, )]), annotations: Annotations::new(), diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index 9a44d451d..201e18db8 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -294,6 +294,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ( @@ -303,6 +304,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ], @@ -317,6 +319,7 @@ mod test { member_of: None, attributes: None, annotations: Annotations::new(), + loc: None, }, )], ); diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index 04ca08156..611289d3f 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -487,6 +487,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, )], [], @@ -523,6 +524,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, )], [], @@ -576,6 +578,7 @@ mod test { member_of: None, attributes: None, annotations: Annotations::new(), + loc: None, }, )], ); @@ -611,6 +614,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, )], [], @@ -637,6 +641,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, )], [], @@ -663,6 +668,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, )], [], @@ -709,6 +715,7 @@ mod test { member_of: None, attributes: None, annotations: Annotations::new(), + loc: None, }, )], ); @@ -742,6 +749,7 @@ mod test { member_of: None, attributes: None, annotations: Annotations::new(), + loc: None, }, )], ); @@ -942,6 +950,7 @@ mod test { member_of: None, attributes: None, annotations: Annotations::new(), + loc: None, }, )], ); @@ -970,6 +979,7 @@ mod test { member_of: None, attributes: None, annotations: Annotations::new(), + loc: None, }, )], ); @@ -998,6 +1008,7 @@ mod test { member_of: None, attributes: None, annotations: Annotations::new(), + loc: None, }, )], ); @@ -1025,6 +1036,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, )], [], @@ -1060,6 +1072,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ( @@ -1069,6 +1082,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ], @@ -1083,6 +1097,7 @@ mod test { member_of: Some(vec![]), attributes: None, annotations: Annotations::new(), + loc: None, }, )], ) @@ -1454,6 +1469,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ( @@ -1463,6 +1479,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ( @@ -1472,6 +1489,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ( @@ -1481,6 +1499,7 @@ mod test { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }, ), ], @@ -1499,6 +1518,7 @@ mod test { )]), attributes: None, annotations: Annotations::new(), + loc: None, }, ), ( @@ -1511,6 +1531,7 @@ mod test { )]), attributes: None, annotations: Annotations::new(), + loc: None, }, ), ( @@ -1520,6 +1541,7 @@ mod test { member_of: Some(vec![]), attributes: None, annotations: Annotations::new(), + loc: None, }, ), ], diff --git a/cedar-policy-validator/src/typecheck/test/expr.rs b/cedar-policy-validator/src/typecheck/test/expr.rs index f29799352..d9b7960c7 100644 --- a/cedar-policy-validator/src/typecheck/test/expr.rs +++ b/cedar-policy-validator/src/typecheck/test/expr.rs @@ -69,6 +69,7 @@ fn slot_in_typechecks() { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }; let schema = json_schema::NamespaceDefinition::new([("typename".parse().unwrap(), etype)], []); assert_typechecks_for_mode( @@ -100,6 +101,7 @@ fn slot_equals_typechecks() { shape: json_schema::AttributesOrContext::default(), tags: None, annotations: Annotations::new(), + loc: None, }; // These don't typecheck in strict mode because the test_util expression // typechecker doesn't have access to a schema, so it can't link From 2344b94b8dcb8c6978700526477aab529957a50e Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 3 Jan 2025 08:52:13 -0500 Subject: [PATCH 2/2] add test Signed-off-by: Craig Disselkoen --- .../src/cedar_schema/to_json_schema.rs | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs index fe66c9a3f..661949b82 100644 --- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs +++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs @@ -671,3 +671,93 @@ fn into_partition_decls( (entities, actions, types) } + +#[cfg(test)] +mod preserves_source_locations { + use super::*; + use cool_asserts::assert_matches; + + #[test] + fn entity_action_and_common_type_decls() { + let (schema, _) = json_schema::Fragment::from_cedarschema_str( + r#" + namespace NS { + type S = String; + entity A; + entity B in A; + entity C in A { + bool: Bool, + s: S, + a: Set, + b: { inner: B }, + }; + type AA = A; + action Read, Write; + action List in Read appliesTo { + principal: [A], + resource: [B, C], + context: { + s: Set, + ab: { a: AA, b: B }, + } + }; + } + "#, + &Extensions::all_available(), + ) + .unwrap(); + let ns = schema + .0 + .get(&Some(Name::parse_unqualified_name("NS").unwrap())) + .expect("couldn't find namespace NS"); + + let entityA = ns + .entity_types + .get(&"A".parse().unwrap()) + .expect("couldn't find entity A"); + let entityB = ns + .entity_types + .get(&"B".parse().unwrap()) + .expect("couldn't find entity B"); + let entityC = ns + .entity_types + .get(&"C".parse().unwrap()) + .expect("couldn't find entity C"); + let ctypeS = ns + .common_types + .get(&json_schema::CommonTypeId::new("S".parse().unwrap()).unwrap()) + .expect("couldn't find common type S"); + let ctypeAA = ns + .common_types + .get(&json_schema::CommonTypeId::new("AA".parse().unwrap()).unwrap()) + .expect("couldn't find common type AA"); + let actionRead = ns.actions.get("Read").expect("couldn't find action Read"); + let actionWrite = ns.actions.get("Write").expect("couldn't find action Write"); + let actionList = ns.actions.get("List").expect("couldn't find action List"); + + assert_matches!(&entityA.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("entity A;") + )); + assert_matches!(&entityB.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("entity B in A;") + )); + assert_matches!(&entityC.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("entity C in A {\n bool: Bool,\n s: S,\n a: Set,\n b: { inner: B },\n };") + )); + assert_matches!(&ctypeS.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("type S = String;") + )); + assert_matches!(&ctypeAA.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("type AA = A;") + )); + assert_matches!(&actionRead.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("action Read, Write;") + )); + assert_matches!(&actionWrite.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("action Read, Write;") + )); + assert_matches!(&actionList.loc, Some(loc) => assert_matches!(loc.snippet(), + Some("action List in Read appliesTo {\n principal: [A],\n resource: [B, C],\n context: {\n s: Set,\n ab: { a: AA, b: B },\n }\n };") + )); + } +}