From 04b1ea3fc0f97b5e29186cfedd6502db41240574 Mon Sep 17 00:00:00 2001 From: oflatt Date: Tue, 3 Sep 2024 15:19:43 -0700 Subject: [PATCH 1/6] initial commit for ancestor tries, tests failing Signed-off-by: oflatt --- cedar-policy-validator/src/entity_manifest.rs | 142 +++++++++++------- .../src/entity_manifest_analysis.rs | 104 +++++-------- cedar-policy-validator/src/entity_slicing.rs | 5 +- 3 files changed, 130 insertions(+), 121 deletions(-) diff --git a/cedar-policy-validator/src/entity_manifest.rs b/cedar-policy-validator/src/entity_manifest.rs index 85c9547e6f..c43bb59a8a 100644 --- a/cedar-policy-validator/src/entity_manifest.rs +++ b/cedar-policy-validator/src/entity_manifest.rs @@ -137,10 +137,13 @@ pub struct AccessTrie { /// The keys are edges in the trie pointing to sub-trie values. #[serde_as(as = "Vec<(_, _)>")] pub(crate) children: Fields, - /// For entity types, this boolean may be `true` - /// to signal that all the ancestors in the entity hierarchy - /// are required (transitively). - pub(crate) ancestors_required: bool, + /// `ancestors_trie` is another [`RootAccessTrie`] representing + /// all of the ancestors of this entity that are required. + /// See the [`RootAccessTrie::is_ancestor`] annotation. + pub(crate) ancestors_trie: RootAccessTrie, + /// When ancestors are required, each node marked `is_ancestor` + /// represents an ancestor or set of ancestors that are required. + pub(crate) is_ancestor: bool, /// Optional data annotation, usually used for type information. #[serde(skip_serializing, skip_deserializing)] #[serde(bound(deserialize = "T: Default"))] @@ -156,8 +159,6 @@ pub(crate) struct AccessPath { pub root: EntityRoot, /// The path of fields of entities or structs pub path: Vec, - /// Request all the parents in the entity hierarchy of this entity. - pub ancestors_required: bool, } /// Error when expressions are partial during entity @@ -229,8 +230,6 @@ impl AccessPath { /// add a full trie as the leaf at the end. pub(crate) fn to_root_access_trie_with_leaf(&self, leaf_trie: AccessTrie) -> RootAccessTrie { let mut current = leaf_trie; - // set ancestors required for the leaf trie - current.ancestors_required = self.ancestors_required; // reverse the path, visiting the last access first for field in self.path.iter().rev() { @@ -240,7 +239,8 @@ impl AccessPath { // the first time we build an access trie is the leaf // of the path, so set the `ancestors_required` flag current = AccessTrie { - ancestors_required: false, + ancestors_trie: Default::default(), + is_ancestor: false, children: fields, data: (), }; @@ -310,7 +310,8 @@ impl AccessTrie { /// Like [`AccessTrie::union`], but modifies the current trie. pub fn union_mut(&mut self, other: &Self) { self.children = union_fields(&self.children, &other.children); - self.ancestors_required = self.ancestors_required || other.ancestors_required; + self.ancestors_trie.union_mut(&other.ancestors_trie); + self.is_ancestor = self.is_ancestor || other.is_ancestor; } /// Get the children of this [`AccessTrie`]. @@ -320,8 +321,8 @@ impl AccessTrie { /// Get a boolean which is true if this trie /// requires all ancestors of the entity to be loaded. - pub fn ancestors_required(&self) -> bool { - self.ancestors_required + pub fn ancestors_required(&self) -> &RootAccessTrie { + &self.ancestors_trie } /// Get the data associated with this [`AccessTrie`]. @@ -336,7 +337,8 @@ impl AccessTrie { pub fn new() -> Self { Self { children: Default::default(), - ancestors_required: false, + ancestors_trie: Default::default(), + is_ancestor: false, data: (), } } @@ -491,14 +493,16 @@ fn entity_manifest_from_expr( // For the `in` operator, we need the ancestors of entities. if let BinaryOp::In = op { - arg1_res = arg1_res.ancestors_required(ty1); + arg1_res = arg1_res + .with_ancestors_required(&arg2_res.resulting_paths.to_ancestor_access_trie()); } // Load all fields using `full_type_required`, since // these operations do equality checks. Ok(arg1_res .full_type_required(ty1) - .union(&arg2_res.full_type_required(ty2))) + .union(&arg2_res.full_type_required(ty2)) + .empty_paths()) } ExprKind::ExtensionFunctionApp { fn_name: _, args } => { // WARNING: this code assumes that extension functions @@ -650,11 +654,13 @@ when { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ] @@ -831,11 +837,13 @@ action Read appliesTo { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ] @@ -862,11 +870,13 @@ action Read appliesTo { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ] @@ -964,22 +974,26 @@ action Read appliesTo { "owner", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ "readers", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], ] @@ -1060,22 +1074,26 @@ action BeSad appliesTo { "nickname", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ "friends", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ] @@ -1153,22 +1171,26 @@ action Hello appliesTo { "friends", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ "nickname", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ @@ -1185,22 +1207,26 @@ action Hello appliesTo { "nickname", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ "friends", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ] @@ -1254,11 +1280,13 @@ when { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ @@ -1274,11 +1302,13 @@ when { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ @@ -1291,7 +1321,8 @@ when { "viewer", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ @@ -1302,15 +1333,18 @@ when { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ] @@ -1373,11 +1407,13 @@ when { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ @@ -1390,7 +1426,8 @@ when { "viewer", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ], [ @@ -1401,15 +1438,18 @@ when { "name", { "children": [], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ], - "ancestorsRequired": false + "ancestorsTrie": { "trie": []}, + "isAncestor": false } ] ] diff --git a/cedar-policy-validator/src/entity_manifest_analysis.rs b/cedar-policy-validator/src/entity_manifest_analysis.rs index 8453e4afde..a5fd174e21 100644 --- a/cedar-policy-validator/src/entity_manifest_analysis.rs +++ b/cedar-policy-validator/src/entity_manifest_analysis.rs @@ -68,11 +68,7 @@ impl EntityManifestAnalysisResult { /// Create an analysis result that starts with a cedar variable pub fn from_root(root: EntityRoot) -> Self { - let path = AccessPath { - root, - path: vec![], - ancestors_required: false, - }; + let path = AccessPath { root, path: vec![] }; Self { global_trie: path.to_root_access_trie(), resulting_paths: WrappedAccessPaths::AccessPath(path), @@ -91,17 +87,23 @@ impl EntityManifestAnalysisResult { /// in `resulting_paths` to the `global_trie`. /// This is necessary after modifying the `resulting_paths`. pub(crate) fn restore_global_trie_invariant(mut self) -> Self { - self.global_trie - .add_wrapped_access_paths(&self.resulting_paths); + self.global_trie.add_wrapped_access_paths( + &self.resulting_paths, + false, + &Default::default(), + ); self } /// Add the ancestors required flag to all of the /// resulting paths for this analysis result, but only set it /// for entity types. - pub(crate) fn ancestors_required(mut self, ty: &Type) -> Self { - self.resulting_paths.ancestors_required(ty); - self.restore_global_trie_invariant() + /// Add the ancestors required flag to all of the resulting + /// paths for this path record. + pub(crate) fn with_ancestors_required(mut self, ancestors_trie: &RootAccessTrie) -> Self { + self.global_trie + .add_wrapped_access_paths(&self.resulting_paths, false, ancestors_trie); + self } /// For equality or containment checks, all paths in the type @@ -153,57 +155,6 @@ impl WrappedAccessPaths { } } - /// Add the ancestors required flag to all of the resulting - /// paths for this path record. - fn ancestors_required(&mut self, ty: &Type) { - match self { - WrappedAccessPaths::AccessPath(path) => { - if let Type::EntityOrRecord(EntityRecordKind::Entity { .. }) = ty { - path.ancestors_required = true; - } - } - WrappedAccessPaths::RecordLiteral(record) => match ty { - Type::EntityOrRecord(EntityRecordKind::Record { attrs, .. }) => { - for (field, value) in record.iter_mut() { - // PANIC SAFETY: Record literals must have attributes that match the type. - #[allow(clippy::expect_used)] - let field_ty = &attrs - .get_attr(field) - .expect("Missing field in record type") - .attr_type; - value.ancestors_required(field_ty); - } - } - // PANIC SAFETY: Typechecking should identity record literals as record types. - #[allow(clippy::panic)] - _ => { - panic!("Found record literal when expected {} type", ty); - } - }, - WrappedAccessPaths::SetLiteral(elements) => { - // PANIC SAFETY: Typechecking should identity set literals as set types. - #[allow(clippy::panic)] - let Type::Set { element_type } = ty - else { - panic!("Found set literal when expected {} type", ty); - }; - - // PANIC SAFETY: Typechecking should give concrete types for set elements. - #[allow(clippy::expect_used)] - let ele_type = element_type - .as_ref() - .expect("Expected concrete set type after typechecking"); - - elements.ancestors_required(ele_type); - } - WrappedAccessPaths::Empty => (), - WrappedAccessPaths::Union(left, right) => { - left.ancestors_required(ty); - right.ancestors_required(ty); - } - } - } - fn full_type_required(self, ty: &Type) -> RootAccessTrie { match self { WrappedAccessPaths::AccessPath(path) => { @@ -255,24 +206,40 @@ impl WrappedAccessPaths { .union(&right.full_type_required(ty)), } } + + pub(crate) fn to_ancestor_access_trie(&self) -> RootAccessTrie { + let mut trie = RootAccessTrie::default(); + trie.add_wrapped_access_paths(self, true, &Default::default()); + trie + } } impl RootAccessTrie { - pub(crate) fn add_wrapped_access_paths(&mut self, path: &WrappedAccessPaths) { + pub(crate) fn add_wrapped_access_paths( + &mut self, + path: &WrappedAccessPaths, + is_ancestor: bool, + ancestors_trie: &RootAccessTrie, + ) { match path { WrappedAccessPaths::AccessPath(access_path) => { - self.add_access_path(access_path, &AccessTrie::new()); + let mut leaf = AccessTrie::new(); + leaf.is_ancestor = is_ancestor; + leaf.ancestors_trie = ancestors_trie.clone(); + self.add_access_path(access_path, &leaf); } WrappedAccessPaths::RecordLiteral(record) => { for field in record.values() { - self.add_wrapped_access_paths(field); + self.add_wrapped_access_paths(field, is_ancestor, ancestors_trie); } } - WrappedAccessPaths::SetLiteral(elements) => self.add_wrapped_access_paths(elements), + WrappedAccessPaths::SetLiteral(elements) => { + self.add_wrapped_access_paths(elements, is_ancestor, ancestors_trie) + } WrappedAccessPaths::Empty => (), WrappedAccessPaths::Union(left, right) => { - self.add_wrapped_access_paths(left); - self.add_wrapped_access_paths(right); + self.add_wrapped_access_paths(left, is_ancestor, ancestors_trie); + self.add_wrapped_access_paths(right, is_ancestor, ancestors_trie); } } } @@ -312,7 +279,8 @@ fn entity_or_record_to_access_trie(ty: &EntityRecordKind) -> AccessTrie { } AccessTrie { children: fields, - ancestors_required: false, + ancestors_trie: Default::default(), + is_ancestor: false, data: (), } } diff --git a/cedar-policy-validator/src/entity_slicing.rs b/cedar-policy-validator/src/entity_slicing.rs index 7d9f61787f..4e56175c56 100644 --- a/cedar-policy-validator/src/entity_slicing.rs +++ b/cedar-policy-validator/src/entity_slicing.rs @@ -181,7 +181,8 @@ impl AccessTrie { } } - let new_ancestors = if self.ancestors_required { + // TODO make more precise by only loading particular ancestors + let new_ancestors = if self.ancestors_trie != Default::default() { entity.ancestors().cloned().collect() } else { HashSet::new() @@ -211,7 +212,7 @@ impl AccessTrie { ) -> Result { // unless this is an entity id, parents should not be required assert!( - !self.ancestors_required + self.ancestors_trie == Default::default() || matches!(val.value_kind(), ValueKind::Lit(Literal::EntityUID(_))) ); From 37bc6e64eb607a0e693956ec042c0381e2ceeff8 Mon Sep 17 00:00:00 2001 From: oflatt Date: Tue, 3 Sep 2024 15:24:39 -0700 Subject: [PATCH 2/6] tests passing without more precise slicing Signed-off-by: oflatt --- cedar-policy-validator/src/entity_manifest.rs | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/cedar-policy-validator/src/entity_manifest.rs b/cedar-policy-validator/src/entity_manifest.rs index c43bb59a8a..47778c41d6 100644 --- a/cedar-policy-validator/src/entity_manifest.rs +++ b/cedar-policy-validator/src/entity_manifest.rs @@ -760,11 +760,39 @@ action Read appliesTo { "manager", { "children": [], - "ancestorsRequired": true + "ancestorsTrie": { + "trie": [ + [ + { + "var": "resource", + }, + { + "children": [], + "isAncestor": true, + "ancestorsTrie": { "trie": [] } + } + ] + ] + }, + "isAncestor": false } ] ], - "ancestorsRequired": true + "ancestorsTrie": { + "trie": [ + [ + { + "var": "resource", + }, + { + "children": [], + "isAncestor": true, + "ancestorsTrie": { "trie": [] } + } + ] + ] + }, + "isAncestor": false } ] ] From a5d0aa9ac84303aaabb5a2a6e40e423b3feaf00d Mon Sep 17 00:00:00 2001 From: oflatt Date: Tue, 3 Sep 2024 16:17:24 -0700 Subject: [PATCH 3/6] slicing of ancestors working and tested Signed-off-by: oflatt --- cedar-policy-validator/src/entity_slicing.rs | 191 +++++++++++++++---- 1 file changed, 150 insertions(+), 41 deletions(-) diff --git a/cedar-policy-validator/src/entity_slicing.rs b/cedar-policy-validator/src/entity_slicing.rs index 4e56175c56..19e646e365 100644 --- a/cedar-policy-validator/src/entity_slicing.rs +++ b/cedar-policy-validator/src/entity_slicing.rs @@ -113,23 +113,35 @@ impl RootAccessTrie { entities: &Entities, request: &Request, ) -> Result { + self.slice_entities_internal(entities, request) + .map(|res| res.0) + } + + /// Returns a new entity store and also the ancestor entities found + /// along the way. + fn slice_entities_internal( + &self, + entities: &Entities, + request: &Request, + ) -> Result<(Entities, HashSet), EntitySliceError> { let mut res = HashMap::::new(); + let mut ancestors = HashSet::new(); for (root, slice) in &self.trie { match root { EntityRoot::Literal(lit) => { - slice.slice_entity(entities, lit, &mut res)?; + slice.slice_entity(entities, request, lit, &mut res, &mut ancestors)?; } EntityRoot::Var(Var::Action) => { let entity_id = request.action().uid().ok_or(PartialRequestError {})?; - slice.slice_entity(entities, entity_id, &mut res)?; + slice.slice_entity(entities, request, entity_id, &mut res, &mut ancestors)?; } EntityRoot::Var(Var::Principal) => { let entity_id = request.principal().uid().ok_or(PartialRequestError {})?; - slice.slice_entity(entities, entity_id, &mut res)?; + slice.slice_entity(entities, request, entity_id, &mut res, &mut ancestors)?; } EntityRoot::Var(Var::Resource) => { let resource_id = request.resource().uid().ok_or(PartialRequestError {})?; - slice.slice_entity(entities, resource_id, &mut res)?; + slice.slice_entity(entities, request, resource_id, &mut res, &mut ancestors)?; } EntityRoot::Var(Var::Context) => { if slice.children.is_empty() { @@ -141,17 +153,20 @@ impl RootAccessTrie { let PartialValue::Value(val) = partial_val else { return Err(PartialRequestError {}.into()); }; - slice.slice_val(entities, &val, &mut res)?; + slice.slice_val(entities, request, &val, &mut res, &mut ancestors)?; } } } } - Ok(Entities::from_entities( - res.into_values(), - None::<&NoEntitiesSchema>, - TCComputation::AssumeAlreadyComputed, - Extensions::all_available(), - )?) + Ok(( + Entities::from_entities( + res.into_values(), + None::<&NoEntitiesSchema>, + TCComputation::AssumeAlreadyComputed, + Extensions::all_available(), + )?, + ancestors, + )) } } @@ -161,9 +176,16 @@ impl AccessTrie { fn slice_entity( &self, entities: &Entities, + request: &Request, lit: &EntityUID, res: &mut HashMap, + res_ancestors: &mut HashSet, ) -> Result<(), EntitySliceError> { + // add to the res_ancestors set if this is a relavent ancestor + if self.is_ancestor { + res_ancestors.insert(lit.clone()); + } + // If the entity is not present, no need to slice let Dereference::Data(entity) = entities.entity(lit) else { return Ok(()); @@ -175,15 +197,31 @@ impl AccessTrie { let PartialValue::Value(val) = pval else { return Err(PartialEntityError {}.into()); }; - let sliced = slice.slice_val(entities, &val, res)?; + let sliced = slice.slice_val(entities, request, &val, res, res_ancestors)?; new_entity.insert(field.clone(), PartialValue::Value(sliced)); } } - // TODO make more precise by only loading particular ancestors let new_ancestors = if self.ancestors_trie != Default::default() { - entity.ancestors().cloned().collect() + eprintln!("ancestors trie: {:?}", self.ancestors_trie); + let relavent_ancestors = self + .ancestors_trie + .slice_entities_internal(entities, request)? + .1; + eprintln!("relavent ancestors: {:?}", relavent_ancestors); + relavent_ancestors + .into_iter() + .filter(|ancestor| { + eprintln!( + "{} is decendant of {}: {}", + entity, + ancestor, + entity.is_descendant_of(ancestor) + ); + entity.is_descendant_of(ancestor) + }) + .collect() } else { HashSet::new() }; @@ -207,8 +245,10 @@ impl AccessTrie { fn slice_val( &self, entities: &Entities, + request: &Request, val: &Value, res: &mut HashMap, + res_ancestors: &mut HashSet, ) -> Result { // unless this is an entity id, parents should not be required assert!( @@ -218,7 +258,7 @@ impl AccessTrie { Ok(match val.value_kind() { ValueKind::Lit(Literal::EntityUID(id)) => { - self.slice_entity(entities, id, res)?; + self.slice_entity(entities, request, id, res, res_ancestors)?; val.clone() } ValueKind::Set(_) | ValueKind::ExtensionValue(_) | ValueKind::Lit(_) => { @@ -236,7 +276,10 @@ impl AccessTrie { for (field, slice) in &self.children { // only slice when field is available if let Some(v) = record.get(field) { - new_map.insert(field.clone(), slice.slice_val(entities, v, res)?); + new_map.insert( + field.clone(), + slice.slice_val(entities, request, v, res, res_ancestors)?, + ); } } @@ -279,6 +322,28 @@ action Read appliesTo { .0 } + fn schema_with_hierarchy() -> ValidatorSchema { + ValidatorSchema::from_cedarschema_str( + " +entity User in [Document] = { + name: String, + manager: User, + personaldoc: Document, +}; + +entity Document; + +action Read appliesTo { + principal: [User], + resource: [Document] +}; + ", + Extensions::all_available(), + ) + .unwrap() + .0 + } + fn expect_entity_slice_to( original: serde_json::Value, expected: serde_json::Value, @@ -489,7 +554,7 @@ when { } #[test] - fn test_entity_manifest_ancestors_required() { + fn test_entity_manifest_ancestors_skipped() { let mut pset = PolicySet::new(); let policy = parse_policy( None, @@ -501,24 +566,7 @@ when { .expect("should succeed"); pset.add(policy.into()).expect("should succeed"); - let schema = ValidatorSchema::from_cedarschema_str( - " -entity User in [Document] = { - name: String, - manager: User -}; - -entity Document; - -action Read appliesTo { - principal: [User], - resource: [Document] -}; - ", - Extensions::all_available(), - ) - .unwrap() - .0; + let schema = schema_with_hierarchy(); let entity_manifest = compute_entity_manifest(&schema, &pset).expect("Should succeed"); @@ -528,20 +576,22 @@ action Read appliesTo { "uid" : { "type" : "User", "id" : "oliver"}, "attrs" : { "name" : "Oliver", - "manager": { "type" : "User", "id" : "george"} + "manager": { "type" : "User", "id" : "george"}, + "personaldoc": { "type" : "Document", "id" : "oliverdocument"} }, "parents" : [ - { "type" : "Document", "id" : "oliverdocument"} + { "type" : "Document", "id" : "oliverdocument"}, + { "type" : "Document", "id" : "dummy"} ] }, { "uid" : { "type" : "User", "id" : "george"}, "attrs" : { "name" : "George", - "manager": { "type" : "User", "id" : "george"} + "manager": { "type" : "User", "id" : "george"}, + "personaldoc": { "type" : "Document", "id" : "georgedocument"} }, "parents" : [ - { "type" : "Document", "id" : "georgedocument"} ] }, ] @@ -555,7 +605,7 @@ action Read appliesTo { "manager": { "__entity": { "type" : "User", "id" : "george"} } }, "parents" : [ - { "type" : "Document", "id" : "oliverdocument"} + { "type" : "Document", "id" : "dummy"} ] }, { @@ -563,7 +613,6 @@ action Read appliesTo { "attrs" : { }, "parents" : [ - { "type" : "Document", "id" : "georgedocument"} ] }, ] @@ -577,6 +626,66 @@ action Read appliesTo { ); } + #[test] + fn test_entity_manifest_possible_ancestors() { + let mut pset = PolicySet::new(); + let policy = parse_policy( + None, + "permit(principal, action, resource) +when { + principal in (if 2 > 3 + then Document::\"dummy\" + else principal.personaldoc) +};", + ) + .expect("should succeed"); + pset.add(policy.into()).expect("should succeed"); + + let schema = schema_with_hierarchy(); + + let entity_manifest = compute_entity_manifest(&schema, &pset).expect("Should succeed"); + + let entities_json = serde_json::json!( + [ + { + "uid" : { "type" : "User", "id" : "oliver"}, + "attrs" : { + "name" : "Oliver", + "manager": { "type" : "User", "id" : "george"}, + "personaldoc": { "type" : "Document", "id" : "oliverdocument"} + }, + "parents" : [ + { "type" : "Document", "id" : "oliverdocument"}, + { "type" : "Document", "id" : "georgedocument"}, + { "type" : "Document", "id" : "dummy"} + ] + }, + ] + ); + + let expected_entities_json = serde_json::json!( + [ + { + "uid" : { "type" : "User", "id" : "oliver"}, + "attrs" : { + "personaldoc":{"__entity":{"type":"Document","id":"oliverdocument"}}, + }, + "parents" : [ + { "type" : "Document", "id" : "dummy"}, + { "type" : "Document", "id" : "oliverdocument"} + ] + } + ] + ); + + expect_entity_slice_to( + entities_json, + expected_entities_json, + &schema, + &entity_manifest, + ); + } + #[test] fn test_entity_manifest_multiple_branches() { let mut pset = PolicySet::new(); From 0cc5319811d20d89cb3dd6d949e29f1c6593a11f Mon Sep 17 00:00:00 2001 From: oflatt Date: Tue, 3 Sep 2024 16:36:11 -0700 Subject: [PATCH 4/6] fix slicing for sets of entities Signed-off-by: oflatt --- cedar-policy-validator/src/entity_slicing.rs | 130 +++++++++++++++++-- 1 file changed, 119 insertions(+), 11 deletions(-) diff --git a/cedar-policy-validator/src/entity_slicing.rs b/cedar-policy-validator/src/entity_slicing.rs index 19e646e365..2b31387c99 100644 --- a/cedar-policy-validator/src/entity_slicing.rs +++ b/cedar-policy-validator/src/entity_slicing.rs @@ -204,23 +204,13 @@ impl AccessTrie { } let new_ancestors = if self.ancestors_trie != Default::default() { - eprintln!("ancestors trie: {:?}", self.ancestors_trie); let relavent_ancestors = self .ancestors_trie .slice_entities_internal(entities, request)? .1; - eprintln!("relavent ancestors: {:?}", relavent_ancestors); relavent_ancestors .into_iter() - .filter(|ancestor| { - eprintln!( - "{} is decendant of {}: {}", - entity, - ancestor, - entity.is_descendant_of(ancestor) - ); - entity.is_descendant_of(ancestor) - }) + .filter(|ancestor| entity.is_descendant_of(ancestor)) .collect() } else { HashSet::new() @@ -256,6 +246,16 @@ impl AccessTrie { || matches!(val.value_kind(), ValueKind::Lit(Literal::EntityUID(_))) ); + // unless this is an entity id or set, it should not be an + // ancestor + assert!( + !self.is_ancestor + || matches!( + val.value_kind(), + ValueKind::Lit(Literal::EntityUID(_)) | ValueKind::Set(_) + ) + ); + Ok(match val.value_kind() { ValueKind::Lit(Literal::EntityUID(id)) => { self.slice_entity(entities, request, id, res, res_ancestors)?; @@ -269,6 +269,32 @@ impl AccessTrie { .into()); } + // when this is an ancestor, request all of the entities + // in this set + if self.is_ancestor { + // PANIC SAFETY: is_ancestor is only called on the rhs of an `is`, which the typechecker ensures is an entity or set of entity type. + #[allow(clippy::panic)] + let ValueKind::Set(set) = val.value_kind() else { + panic!("Found is_ancestor on non-entity type {}", val.value_kind()) + }; + + for val in set.iter() { + match val.value_kind() { + ValueKind::Lit(Literal::EntityUID(id)) => { + res_ancestors.insert((**id).clone()); + } + // PANIC SAFETY: see above panic- set must contain entities + #[allow(clippy::panic)] + _ => { + panic!( + "Found is_ancestor on set of non-entity-type {}", + val.value_kind() + ); + } + } + } + } + val.clone() } ValueKind::Record(record) => { @@ -686,6 +712,88 @@ when { ); } + #[test] + fn test_entity_manifest_set_of_ancestors() { + let mut pset = PolicySet::new(); + let policy = parse_policy( + None, + "permit(principal, action, resource) +when { + principal in principal.managers +};", + ) + .expect("should succeed"); + pset.add(policy.into()).expect("should succeed"); + + let schema = ValidatorSchema::from_cedarschema_str( + " +entity User in [User] = { + name: String, + managers: Set +}; + +entity Document; + +action Read appliesTo { + principal: [User], + resource: [Document] +}; + ", + Extensions::all_available(), + ) + .unwrap() + .0; + + let entity_manifest = compute_entity_manifest(&schema, &pset).expect("Should succeed"); + + let entities_json = serde_json::json!( + [ + { + "uid" : { "type" : "User", "id" : "oliver"}, + "attrs" : { + "name" : "Oliver", + "managers": [ + { "type" : "User", "id" : "george"}, + { "type" : "User", "id" : "yihong"}, + { "type" : "User", "id" : "ignored"}, + ] + }, + "parents" : [ + { "type" : "User", "id" : "dummy"}, + { "type" : "User", "id" : "george"}, + { "type" : "User", "id" : "yihong"}, + ] + }, + ] + ); + + let expected_entities_json = serde_json::json!( + [ + { + "uid" : { "type" : "User", "id" : "oliver"}, + "attrs" : { + "managers": [ + { "__entity": { "type" : "User", "id" : "george"}}, + { "__entity": { "type" : "User", "id" : "yihong"}}, + { "__entity": { "type" : "User", "id" : "ignored"}}, + ] + }, + "parents" : [ + { "type" : "User", "id" : "george"}, + { "type" : "User", "id" : "yihong"}, + ] + }, + ] + ); + + expect_entity_slice_to( + entities_json, + expected_entities_json, + &schema, + &entity_manifest, + ); + } + #[test] fn test_entity_manifest_multiple_branches() { let mut pset = PolicySet::new(); From 4bd61c2c4d9ef05eaeca256f96dd830d897cba7e Mon Sep 17 00:00:00 2001 From: oflatt Date: Fri, 6 Sep 2024 12:26:54 -0700 Subject: [PATCH 5/6] better description --- cedar-policy-validator/src/entity_manifest.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cedar-policy-validator/src/entity_manifest.rs b/cedar-policy-validator/src/entity_manifest.rs index 47778c41d6..507d15faae 100644 --- a/cedar-policy-validator/src/entity_manifest.rs +++ b/cedar-policy-validator/src/entity_manifest.rs @@ -139,10 +139,13 @@ pub struct AccessTrie { pub(crate) children: Fields, /// `ancestors_trie` is another [`RootAccessTrie`] representing /// all of the ancestors of this entity that are required. + /// The ancestors trie is a subset of the original [`RootAccessTrie`]. /// See the [`RootAccessTrie::is_ancestor`] annotation. pub(crate) ancestors_trie: RootAccessTrie, /// When ancestors are required, each node marked `is_ancestor` /// represents an ancestor or set of ancestors that are required. + /// An ancestor trie can be thought of as a set of pointers to + /// nodes in the original trie, one `is_ancestor`-marked node per pointer. pub(crate) is_ancestor: bool, /// Optional data annotation, usually used for type information. #[serde(skip_serializing, skip_deserializing)] From 8e3129a6d6704b05c7e058504c535e2b02d3801d Mon Sep 17 00:00:00 2001 From: oflatt Date: Tue, 10 Sep 2024 11:42:56 -0700 Subject: [PATCH 6/6] remove confusing comment Signed-off-by: oflatt --- cedar-policy-validator/src/entity_manifest.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/cedar-policy-validator/src/entity_manifest.rs b/cedar-policy-validator/src/entity_manifest.rs index 507d15faae..1b0c443c9f 100644 --- a/cedar-policy-validator/src/entity_manifest.rs +++ b/cedar-policy-validator/src/entity_manifest.rs @@ -239,8 +239,6 @@ impl AccessPath { let mut fields = HashMap::new(); fields.insert(field.clone(), Box::new(current)); - // the first time we build an access trie is the leaf - // of the path, so set the `ancestors_required` flag current = AccessTrie { ancestors_trie: Default::default(), is_ancestor: false,