diff --git a/cedar-policy-validator/src/entity_manifest.rs b/cedar-policy-validator/src/entity_manifest.rs index a25a0598a5..85c9547e6f 100644 --- a/cedar-policy-validator/src/entity_manifest.rs +++ b/cedar-policy-validator/src/entity_manifest.rs @@ -20,20 +20,19 @@ use std::collections::HashMap; use std::fmt::{Display, Formatter}; use cedar_policy_core::ast::{ - BinaryOp, EntityUID, Expr, ExprKind, Literal, PolicyID, PolicySet, RequestType, UnaryOp, Var, + BinaryOp, EntityUID, Expr, ExprKind, Literal, PolicySet, RequestType, UnaryOp, Var, }; use cedar_policy_core::entities::err::EntitiesError; -use cedar_policy_core::impl_diagnostic_from_source_loc_opt_field; -use cedar_policy_core::parser::Loc; use miette::Diagnostic; use serde::{Deserialize, Serialize}; use serde_with::serde_as; use smol_str::SmolStr; use thiserror::Error; +use crate::entity_manifest_analysis::{EntityManifestAnalysisResult, WrappedAccessPaths}; use crate::{ typecheck::{PolicyCheck, Typechecker}, - types::{EntityRecordKind, Type}, + types::Type, ValidationMode, ValidatorSchema, }; use crate::{ValidationResult, Validator}; @@ -148,10 +147,11 @@ pub struct AccessTrie { pub(crate) data: T, } -/// A data path that may end with requesting the parents of -/// an entity. -#[derive(Debug, Clone, PartialEq, Eq)] -struct AccessPath { +/// An access path represents path of fields, starting with an [`EntityRoot`]. +/// Fields may be record fields or entity fields. +/// If an access path ends with an entity type, it may also require the ancestors of the entity. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct AccessPath { /// The root variable that begins the data path pub root: EntityRoot, /// The path of fields of entities or structs @@ -160,49 +160,6 @@ struct AccessPath { pub ancestors_required: bool, } -/// Entity manifest computation does not handle the full -/// cedar language. In particular, the policies must follow the -/// following grammar: -/// ```text -/// = -/// in -/// + -/// if { } { } -/// ... all other cedar operators not mentioned by datapath-expr - -/// = . -/// has -/// -/// -/// ``` -/// The `get_expr_path` function handles `datapath-expr` expressions. -/// This error message tells the user not to use certain operators -/// before accessing record or entity attributes, breaking this grammar. -// CAUTION: this type is publicly exported in `cedar-policy`. -// Don't make fields `pub`, don't make breaking changes, and use caution -// when adding public methods. -#[derive(Debug, Clone, Error, Hash, Eq, PartialEq)] -#[error("for policy `{policy_id}`, failed to analyze expression while computing entity manifest`")] -pub struct FailedAnalysisError { - /// Source location - source_loc: Option, - /// Policy ID where the error occurred - policy_id: PolicyID, - /// The kind of the expression that was unexpected - expr_kind: ExprKind>, -} - -impl Diagnostic for FailedAnalysisError { - impl_diagnostic_from_source_loc_opt_field!(source_loc); - - fn help<'a>(&'a self) -> Option> { - Some(Box::new(format!( - "failed to compute entity manifest: {} operators are not allowed before accessing record or entity attributes", - self.expr_kind.operator_description() - ))) - } -} - /// Error when expressions are partial during entity /// manifest computation // CAUTION: this type is publicly exported in `cedar-policy`. @@ -225,8 +182,6 @@ pub struct PartialRequestError {} impl Diagnostic for PartialRequestError {} /// An error generated by entity slicing. -/// See [`FailedAnalysisError`] for details on the fragment -/// of Cedar handled by entity slicing. #[derive(Debug, Error)] pub enum EntityManifestError { /// A validation error was encountered @@ -243,12 +198,6 @@ pub enum EntityManifestError { /// A policy was partial #[error(transparent)] PartialExpression(#[from] PartialExpressionError), - - /// A policy was not analyzable because it used unsupported operators - /// before a [`ExprKind::GetAttr`] - /// See [`FailedAnalysisError`] for more details. - #[error(transparent)] - FailedAnalysis(#[from] FailedAnalysisError), } impl EntityManifest { @@ -264,7 +213,7 @@ fn union_fields(first: &Fields, second: &Fields) -> Fields { let mut res = first.clone(); for (key, value) in second { res.entry(key.clone()) - .and_modify(|existing| *existing = Box::new((*existing).union(value))) + .and_modify(|existing| existing.union_mut(value)) .or_insert(value.clone()); } res @@ -272,22 +221,24 @@ fn union_fields(first: &Fields, second: &Fields) -> Fields { impl AccessPath { /// Convert a [`AccessPath`] into corresponding [`RootAccessTrie`]. - fn to_root_access_trie(&self) -> RootAccessTrie { - self.to_root_access_trie_with_leaf(AccessTrie { - ancestors_required: true, - children: Default::default(), - data: (), - }) + pub fn to_root_access_trie(&self) -> RootAccessTrie { + self.to_root_access_trie_with_leaf(AccessTrie::default()) } /// Convert an [`AccessPath`] to a [`RootAccessTrie`], and also /// add a full trie as the leaf at the end. - fn to_root_access_trie_with_leaf(&self, leaf_trie: AccessTrie) -> RootAccessTrie { + 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() { 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_required: false, children: fields, @@ -296,7 +247,12 @@ impl AccessPath { } let mut primary_map = HashMap::new(); - primary_map.insert(self.root.clone(), current); + + // special case: if the path is completely empty, + // no need to insert anything + if current != AccessTrie::new() { + primary_map.insert(self.root.clone(), current); + } RootAccessTrie { trie: primary_map } } } @@ -321,15 +277,19 @@ impl RootAccessTrie { impl RootAccessTrie { /// Union two [`RootAccessTrie`]s together. /// The new trie requests the data from both of the original. - fn union(&self, other: &Self) -> Self { - let mut res = self.clone(); + pub fn union(mut self, other: &Self) -> Self { + self.union_mut(other); + self + } + + /// Like [`RootAccessTrie::union`], but modifies the current trie. + pub fn union_mut(&mut self, other: &Self) { for (key, value) in &other.trie { - res.trie + self.trie .entry(key.clone()) - .and_modify(|existing| *existing = (*existing).union(value)) + .and_modify(|existing| existing.union_mut(value)) .or_insert(value.clone()); } - res } } @@ -342,12 +302,15 @@ impl Default for RootAccessTrie { impl AccessTrie { /// Union two [`AccessTrie`]s together. /// The new trie requests the data from both of the original. - pub fn union(&self, other: &Self) -> Self { - Self { - children: union_fields(&self.children, &other.children), - ancestors_required: self.ancestors_required || other.ancestors_required, - data: self.data.clone(), - } + pub fn union(mut self, other: &Self) -> Self { + self.union_mut(other); + self + } + + /// 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; } /// Get the children of this [`AccessTrie`]. @@ -379,6 +342,12 @@ impl AccessTrie { } } +impl Default for AccessTrie { + fn default() -> Self { + Self::new() + } +} + /// Computes an [`EntityManifest`] from the schema and policies. /// The policies must validate against the schema in strict mode, /// otherwise an error is returned. @@ -405,7 +374,7 @@ pub fn compute_entity_manifest( PolicyCheck::Success(typechecked_expr) => { // compute the trie from the typechecked expr // using static analysis - compute_root_trie(&typechecked_expr, policy.id()) + entity_manifest_from_expr(&typechecked_expr).map(|val| val.global_trie) } PolicyCheck::Irrelevant(_) => { // this policy is irrelevant, so we need no data @@ -422,12 +391,9 @@ pub fn compute_entity_manifest( let request_type = request_env .to_request_type() .ok_or(PartialRequestError {})?; - // Add to the manifest based on the request type. manifest .entry(request_type) - .and_modify(|existing| { - *existing = existing.union(&new_primary_slice); - }) + .and_modify(|existing| existing.union_mut(&new_primary_slice)) .or_insert(new_primary_slice); } } @@ -440,238 +406,159 @@ pub fn compute_entity_manifest( /// A static analysis on type-annotated cedar expressions. /// Computes the [`RootAccessTrie`] representing all the data required /// to evaluate the expression. -fn compute_root_trie( +fn entity_manifest_from_expr( expr: &Expr>, - policy_id: &PolicyID, -) -> Result { - let mut primary_slice = RootAccessTrie::new(); - add_to_root_trie(&mut primary_slice, expr, policy_id, false)?; - Ok(primary_slice) -} - -/// Add the expression's requested data to the [`RootAccessTrie`]. -/// This handles s from the grammar (see [`FailedAnalysisError`]) -/// while [`get_expr_path`] handles the s. -fn add_to_root_trie( - root_trie: &mut RootAccessTrie, - expr: &Expr>, - policy_id: &PolicyID, - should_load_all: bool, -) -> Result<(), EntityManifestError> { +) -> Result { match expr.expr_kind() { - // Literals, variables, and unkonwns without any GetAttr operations - // on them are okay, since no fields need to be loaded. - ExprKind::Lit(_) => Ok(()), - ExprKind::Var(_) => Ok(()), - ExprKind::Slot(_) => Ok(()), + ExprKind::Slot(slot_id) => { + if slot_id.is_principal() { + Ok(EntityManifestAnalysisResult::from_root(EntityRoot::Var( + Var::Principal, + ))) + } else { + assert!(slot_id.is_resource()); + Ok(EntityManifestAnalysisResult::from_root(EntityRoot::Var( + Var::Resource, + ))) + } + } + ExprKind::Var(var) => Ok(EntityManifestAnalysisResult::from_root(EntityRoot::Var( + *var, + ))), + ExprKind::Lit(Literal::EntityUID(literal)) => Ok(EntityManifestAnalysisResult::from_root( + EntityRoot::Literal((**literal).clone()), + )), ExprKind::Unknown(_) => Err(PartialExpressionError {})?, + + // Non-entity literals need no fields to be loaded. + ExprKind::Lit(_) => Ok(EntityManifestAnalysisResult::default()), ExprKind::If { test_expr, then_expr, else_expr, - } => { - add_to_root_trie(root_trie, test_expr, policy_id, should_load_all)?; - add_to_root_trie(root_trie, then_expr, policy_id, should_load_all)?; - add_to_root_trie(root_trie, else_expr, policy_id, should_load_all)?; - Ok(()) - } - ExprKind::And { left, right } => { - add_to_root_trie(root_trie, left, policy_id, should_load_all)?; - add_to_root_trie(root_trie, right, policy_id, should_load_all)?; - Ok(()) - } - ExprKind::Or { left, right } => { - add_to_root_trie(root_trie, left, policy_id, should_load_all)?; - add_to_root_trie(root_trie, right, policy_id, should_load_all)?; - Ok(()) - } + } => Ok(entity_manifest_from_expr(test_expr)? + .empty_paths() + .union(&entity_manifest_from_expr(then_expr)?) + .union(&entity_manifest_from_expr(else_expr)?)), + ExprKind::And { left, right } + | ExprKind::Or { left, right } + | ExprKind::BinaryApp { + op: BinaryOp::Less | BinaryOp::LessEq | BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul, + arg1: left, + arg2: right, + } => Ok(entity_manifest_from_expr(left)? + .empty_paths() + .union(&entity_manifest_from_expr(right)?.empty_paths())), ExprKind::UnaryApp { op, arg } => { match op { - UnaryOp::Not => add_to_root_trie(root_trie, arg, policy_id, should_load_all)?, - UnaryOp::Neg => add_to_root_trie(root_trie, arg, policy_id, should_load_all)?, - }; - Ok(()) - } - ExprKind::BinaryApp { op, arg1, arg2 } => match op { - // Special case! Equality between records requires - // that we load all fields. - // This could be made more precise if we check the type. - BinaryOp::Eq => { - add_to_root_trie(root_trie, arg1, policy_id, true)?; - add_to_root_trie(root_trie, arg2, policy_id, true)?; - Ok(()) - } - BinaryOp::In => { - // Recur normally on the rhs - add_to_root_trie(root_trie, arg2, policy_id, should_load_all)?; - - // The lhs should be a datapath expression. - let mut flat_slice = get_expr_path(arg1, policy_id)?; - flat_slice.ancestors_required = true; - *root_trie = root_trie.union(&flat_slice.to_root_access_trie()); - Ok(()) - } - BinaryOp::Contains | BinaryOp::ContainsAll | BinaryOp::ContainsAny => { - // Like equality, another special case for records. - add_to_root_trie(root_trie, arg1, policy_id, true)?; - add_to_root_trie(root_trie, arg2, policy_id, true)?; - Ok(()) + // both unary ops are on booleans, so they are simple + UnaryOp::Not | UnaryOp::Neg => Ok(entity_manifest_from_expr(arg)?.empty_paths()), } - BinaryOp::Less | BinaryOp::LessEq | BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul => { - // These operators work on literals, so no special - // case is needed. - add_to_root_trie(root_trie, arg1, policy_id, should_load_all)?; - add_to_root_trie(root_trie, arg2, policy_id, should_load_all)?; - Ok(()) + } + ExprKind::BinaryApp { + op: + BinaryOp::Eq + | BinaryOp::In + | BinaryOp::Contains + | BinaryOp::ContainsAll + | BinaryOp::ContainsAny, + arg1, + arg2, + } => { + // TODO Is there more elegant way to bind op using rust pattern matching? + // PANIC SAFETY: Matched a binary app above, so expr must still be a binary app. + #[allow(clippy::panic)] + let ExprKind::BinaryApp { op, .. } = expr.expr_kind() else { + panic!("Matched above"); + }; + + // First, find the data paths for each argument + let mut arg1_res = entity_manifest_from_expr(arg1)?; + let arg2_res = entity_manifest_from_expr(arg2)?; + + // PANIC SAFETY: Typechecking succeeded, so type annotations are present. + #[allow(clippy::expect_used)] + let ty1 = arg1 + .data() + .as_ref() + .expect("Expected annotated types after typechecking"); + // PANIC SAFETY: Typechecking succeeded, so type annotations are present. + #[allow(clippy::expect_used)] + let ty2 = arg2 + .data() + .as_ref() + .expect("Expected annotated types after typechecking"); + + // For the `in` operator, we need the ancestors of entities. + if let BinaryOp::In = op { + arg1_res = arg1_res.ancestors_required(ty1); } - }, + + // 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))) + } ExprKind::ExtensionFunctionApp { fn_name: _, args } => { // WARNING: this code assumes that extension functions - // don't take full structs as inputs. - // If they did, we would need to use logic similar to the Eq binary operator. + // all take primitives as inputs and produce + // primitives as outputs. + // If not, we would need to use logic similar to the Eq binary operator. + + let mut res = EntityManifestAnalysisResult::default(); + for arg in args.iter() { - add_to_root_trie(root_trie, arg, policy_id, should_load_all)?; + res = res.union(&entity_manifest_from_expr(arg)?); } - Ok(()) - } - ExprKind::Like { expr, pattern: _ } => { - add_to_root_trie(root_trie, expr, policy_id, should_load_all)?; - Ok(()) + Ok(res) } - ExprKind::Is { + ExprKind::Like { expr, pattern: _ } + | ExprKind::Is { expr, entity_type: _, } => { - add_to_root_trie(root_trie, expr, policy_id, should_load_all)?; - Ok(()) + // drop paths since boolean returned + Ok(entity_manifest_from_expr(expr)?.empty_paths()) } ExprKind::Set(contents) => { + let mut res = EntityManifestAnalysisResult::default(); + + // take union of all of the contents for expr in &**contents { - add_to_root_trie(root_trie, expr, policy_id, should_load_all)?; - } - Ok(()) - } - ExprKind::Record(content) => { - for expr in content.values() { - add_to_root_trie(root_trie, expr, policy_id, should_load_all)?; + let content = entity_manifest_from_expr(expr)?; + + res = res.union(&content); } - Ok(()) - } - ExprKind::HasAttr { expr, attr } => { - let mut flat_slice = get_expr_path(expr, policy_id)?; - flat_slice.path.push(attr.clone()); - *root_trie = root_trie.union(&flat_slice.to_root_access_trie()); - Ok(()) - } - ExprKind::GetAttr { .. } => { - let flat_slice = get_expr_path(expr, policy_id)?; - // PANIC SAFETY: Successfuly typechecked expressions should always have annotated types. - #[allow(clippy::expect_used)] - let leaf_field = if should_load_all { - type_to_access_trie( - expr.data() - .as_ref() - .expect("Typechecked expression missing type"), - ) - } else { - AccessTrie::new() - }; + // now, wrap result in a set + res.resulting_paths = WrappedAccessPaths::SetLiteral(Box::new(res.resulting_paths)); - *root_trie = root_trie.union(&flat_slice.to_root_access_trie_with_leaf(leaf_field)); - Ok(()) + Ok(res) } - } -} + ExprKind::Record(content) => { + let mut record_contents = HashMap::new(); + let mut global_trie = RootAccessTrie::default(); -/// Compute the full [`AccessTrie`] required for the type. -fn type_to_access_trie(ty: &Type) -> AccessTrie { - match ty { - // if it's not an entity or record, slice ends here - Type::ExtensionType { .. } - | Type::Never - | Type::True - | Type::False - | Type::Primitive { .. } - | Type::Set { .. } => AccessTrie::new(), - Type::EntityOrRecord(record_type) => entity_or_record_to_access_trie(record_type), - } -} + for (key, child_expr) in content.iter() { + let res = entity_manifest_from_expr(child_expr)?; + record_contents.insert(key.clone(), Box::new(res.resulting_paths)); -/// Compute the full [`AccessTrie`] for the given entity or record type. -fn entity_or_record_to_access_trie(ty: &EntityRecordKind) -> AccessTrie { - match ty { - EntityRecordKind::ActionEntity { attrs, .. } | EntityRecordKind::Record { attrs, .. } => { - let mut fields = HashMap::new(); - for (attr_name, attr_type) in attrs.iter() { - fields.insert( - attr_name.clone(), - Box::new(type_to_access_trie(&attr_type.attr_type)), - ); - } - AccessTrie { - children: fields, - ancestors_required: false, - data: (), + global_trie = global_trie.union(&res.global_trie); } - } - EntityRecordKind::Entity(_) | EntityRecordKind::AnyEntity => { - // no need to load data for entities, which are compared - // using ids - AccessTrie::new() + Ok(EntityManifestAnalysisResult { + resulting_paths: WrappedAccessPaths::RecordLiteral(record_contents), + global_trie, + }) } - } -} - -/// Given an expression, get the corresponding data path -/// starting with a variable. -/// If the expression is not a ``, return an error. -/// See [`FailedAnalysisError`] for more information. -fn get_expr_path( - expr: &Expr>, - policy_id: &PolicyID, -) -> Result { - Ok(match expr.expr_kind() { - ExprKind::Slot(slot_id) => { - if slot_id.is_principal() { - AccessPath { - root: EntityRoot::Var(Var::Principal), - path: vec![], - ancestors_required: false, - } - } else { - assert!(slot_id.is_resource()); - AccessPath { - root: EntityRoot::Var(Var::Resource), - path: vec![], - ancestors_required: false, - } - } - } - ExprKind::Var(var) => AccessPath { - root: EntityRoot::Var(*var), - path: vec![], - ancestors_required: false, - }, ExprKind::GetAttr { expr, attr } => { - let mut slice = get_expr_path(expr, policy_id)?; - slice.path.push(attr.clone()); - slice + Ok(entity_manifest_from_expr(expr)?.get_or_has_attr(attr)) } - ExprKind::Lit(Literal::EntityUID(literal)) => AccessPath { - root: EntityRoot::Literal((**literal).clone()), - path: vec![], - ancestors_required: false, - }, - ExprKind::Unknown(_) => Err(PartialExpressionError {})?, - // all other variants of expressions result in failure to analyze. - _ => Err(EntityManifestError::FailedAnalysis(FailedAnalysisError { - source_loc: expr.source_loc().cloned(), - policy_id: policy_id.clone(), - expr_kind: expr.expr_kind().clone(), - }))?, - }) + ExprKind::HasAttr { expr, attr } => Ok(entity_manifest_from_expr(expr)? + .get_or_has_attr(attr) + .empty_paths()), + } } #[cfg(test)] @@ -701,6 +588,29 @@ action Read appliesTo { .0 } + fn document_fields_schema() -> ValidatorSchema { + ValidatorSchema::from_cedarschema_str( + " +entity User = { +name: String, +}; + +entity Document = { +owner: User, +viewer: User, +}; + +action Read appliesTo { +principal: [User], +resource: [Document] +}; +", + Extensions::all_available(), + ) + .unwrap() + .0 + } + #[test] fn test_simple_entity_manifest() { let mut pset = PolicySet::new(); @@ -1071,7 +981,7 @@ action Read appliesTo { ], "ancestorsRequired": false } - ] + ], ] } ] @@ -1301,4 +1211,214 @@ action Hello appliesTo { let expected_manifest = serde_json::from_value(expected).unwrap(); assert_eq!(entity_manifest, expected_manifest); } + + #[test] + fn test_entity_manifest_with_if() { + let mut pset = PolicySet::new(); + + let schema = document_fields_schema(); + + let policy = parse_policy( + None, + "permit(principal, action, resource) +when { + if principal.name == \"John\" + then resource.owner.name == User::\"oliver\".name + else resource.viewer == User::\"oliver\" +};", + ) + .expect("should succeed"); + pset.add(policy.into()).expect("should succeed"); + + let entity_manifest = compute_entity_manifest(&schema, &pset).expect("Should succeed"); + let expected = serde_json::json! ( { + "perAction": [ + [ + { + "principal": "User", + "action": { + "ty": "Action", + "eid": "Read" + }, + "resource": "Document" + }, + { + "trie": [ + [ + { + "var": "principal" + }, + { + "children": [ + [ + "name", + { + "children": [], + "ancestorsRequired": false + } + ] + ], + "ancestorsRequired": false + } + ], + [ + { + "literal": { + "ty": "User", + "eid": "oliver" + } + }, + { + "children": [ + [ + "name", + { + "children": [], + "ancestorsRequired": false + } + ] + ], + "ancestorsRequired": false + } + ], + [ + { + "var": "resource" + }, + { + "children": [ + [ + "viewer", + { + "children": [], + "ancestorsRequired": false + } + ], + [ + "owner", + { + "children": [ + [ + "name", + { + "children": [], + "ancestorsRequired": false + } + ] + ], + "ancestorsRequired": false + } + ] + ], + "ancestorsRequired": false + } + ] + ] + } + ] + ] + } + ); + let expected_manifest = serde_json::from_value(expected).unwrap(); + assert_eq!(entity_manifest, expected_manifest); + } + + #[test] + fn test_entity_manifest_if_literal_record() { + let mut pset = PolicySet::new(); + + let schema = document_fields_schema(); + + let policy = parse_policy( + None, + "permit(principal, action, resource) +when { + { + \"myfield\": + { + \"secondfield\": + if principal.name == \"yihong\" + then principal + else resource.owner, + \"ignored but still important due to errors\": + resource.viewer + } + }[\"myfield\"][\"secondfield\"].name == \"pavel\" +};", + ) + .expect("should succeed"); + pset.add(policy.into()).expect("should succeed"); + + let entity_manifest = compute_entity_manifest(&schema, &pset).expect("Should succeed"); + let expected = serde_json::json! ( { + "perAction": [ + [ + { + "principal": "User", + "action": { + "ty": "Action", + "eid": "Read" + }, + "resource": "Document" + }, + { + "trie": [ + [ + { + "var": "principal" + }, + { + "children": [ + [ + "name", + { + "children": [], + "ancestorsRequired": false + } + ] + ], + "ancestorsRequired": false + } + ], + [ + { + "var": "resource" + }, + { + "children": [ + [ + "viewer", + { + "children": [], + "ancestorsRequired": false + } + ], + [ + "owner", + { + "children": [ + [ + "name", + { + "children": [], + "ancestorsRequired": false + } + ] + ], + "ancestorsRequired": false + } + ] + ], + "ancestorsRequired": false + } + ] + ] + } + ] + ] + } + ); + let expected_manifest = serde_json::from_value(expected).unwrap(); + assert_eq!(entity_manifest, expected_manifest); + } } diff --git a/cedar-policy-validator/src/entity_manifest_analysis.rs b/cedar-policy-validator/src/entity_manifest_analysis.rs new file mode 100644 index 0000000000..8453e4afde --- /dev/null +++ b/cedar-policy-validator/src/entity_manifest_analysis.rs @@ -0,0 +1,326 @@ +use std::collections::HashMap; + +use smol_str::SmolStr; + +use crate::{ + entity_manifest::{AccessPath, AccessTrie, EntityRoot, RootAccessTrie}, + types::{EntityRecordKind, Type}, +}; + +/// Represents [`AccessPath`]s possibly +/// wrapped in record or set literals. +/// +/// This allows the Entity Manifest to soundly handle +/// data that is wrapped in record or set literals, then used in equality +/// operators or dereferenced. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub(crate) enum WrappedAccessPaths { + /// No access paths are needed. + #[default] + Empty, + /// A single access path, starting with a cedar variable. + AccessPath(AccessPath), + /// The union of two [`WrappedAccessPaths`], denoting that + /// all access paths from both are required. + /// This is useful for join points in the analysis (`if`, set literals, ect) + Union(Box, Box), + /// A record literal, each field having access paths. + RecordLiteral(HashMap>), + /// A set literal containing access paths. + /// Used to note that this type is wrapped in a literal set. + SetLiteral(Box), +} + +/// During Entity Manifest analysis, each sub-expression +/// produces an [`EntityManifestAnalysisResult`]. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub(crate) struct EntityManifestAnalysisResult { + /// INVARIANT: The `global_trie` stores all of the data paths this sub-expression + /// could have accessed, including all those in `resulting_paths`. + pub(crate) global_trie: RootAccessTrie, + /// `resulting_paths` stores a list of `AccessPathRecord`, + /// Each representing a data path + /// (possibly wrapped in a record literal) + /// that could be accessed using the `.` operator. + pub(crate) resulting_paths: WrappedAccessPaths, +} + +impl EntityManifestAnalysisResult { + /// Drop the resulting paths part of the analysis. + /// This is necessary when the expression is a primitive value, so it + /// can't be dereferenced. + pub fn empty_paths(mut self) -> Self { + self.resulting_paths = Default::default(); + self + } + + /// Union two [`EntityManifestAnalysisResult`]s together, + /// keeping the paths from both global tries and concatenating + /// the resulting paths. + pub fn union(mut self, other: &Self) -> Self { + self.global_trie = self.global_trie.union(&other.global_trie); + self.resulting_paths = WrappedAccessPaths::Union( + Box::new(self.resulting_paths), + Box::new(other.resulting_paths.clone()), + ); + self + } + + /// 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, + }; + Self { + global_trie: path.to_root_access_trie(), + resulting_paths: WrappedAccessPaths::AccessPath(path), + } + } + + /// Extend all the access paths with this attr, + /// adding all the new paths to the global trie. + pub fn get_or_has_attr(mut self, attr: &SmolStr) -> Self { + self.resulting_paths = self.resulting_paths.get_or_has_attr(attr); + + self.restore_global_trie_invariant() + } + + /// Restores the `global_trie` invariant by adding all paths + /// 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 + } + + /// 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() + } + + /// For equality or containment checks, all paths in the type + /// are required. + /// This function extends the paths with the fields mentioned + /// by the type, adding these to the global trie. + /// + /// It also drops the resulting paths, since these checks result + /// in booleans. + pub(crate) fn full_type_required(mut self, ty: &Type) -> Self { + let mut paths = Default::default(); + std::mem::swap(&mut self.resulting_paths, &mut paths); + + self.global_trie = self.global_trie.union(&paths.full_type_required(ty)); + + self + } +} + +impl WrappedAccessPaths { + /// Add accessting this attribute to all access paths + fn get_or_has_attr(self, attr: &SmolStr) -> Self { + match self { + WrappedAccessPaths::AccessPath(mut access_path) => { + access_path.path.push(attr.clone()); + WrappedAccessPaths::AccessPath(access_path) + } + WrappedAccessPaths::RecordLiteral(mut record) => { + #[allow(clippy::panic)] + if let Some(field) = record.remove(attr) { + *field + } else { + // otherwise, this is a `has` expression + // but the record literal didn't have it. + // do nothing in this case + WrappedAccessPaths::RecordLiteral(record) + } + } + // PANIC SAFETY: Type checker should prevent using `.` operator on a set type. + #[allow(clippy::panic)] + WrappedAccessPaths::SetLiteral(_) => { + panic!("Attempted to dereference a set literal.") + } + WrappedAccessPaths::Empty => WrappedAccessPaths::Empty, + WrappedAccessPaths::Union(left, right) => WrappedAccessPaths::Union( + Box::new(left.get_or_has_attr(attr)), + Box::new(right.get_or_has_attr(attr)), + ), + } + } + + /// 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) => { + let leaf_trie = type_to_access_trie(ty); + path.to_root_access_trie_with_leaf(leaf_trie.clone()) + } + WrappedAccessPaths::RecordLiteral(mut literal_fields) => match ty { + Type::EntityOrRecord(EntityRecordKind::Record { + attrs: record_attrs, + .. + }) => { + let mut res = RootAccessTrie::new(); + for (attr, attr_ty) in &record_attrs.attrs { + // PANIC SAFETY: Record literals should have attributes that match the type. + #[allow(clippy::panic)] + let field = literal_fields + .remove(attr) + .unwrap_or_else(|| panic!("Missing field {attr} in record literal")); + + res = res.union(&field.full_type_required(&attr_ty.attr_type)); + } + + res + } + // PANIC SAFETY: Typechecking should identity record literals as record types. + #[allow(clippy::panic)] + _ => { + panic!("Found record literal when expected {} type", ty); + } + }, + WrappedAccessPaths::SetLiteral(elements) => match ty { + Type::Set { element_type } => { + // 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.full_type_required(ele_type) + } + // PANIC SAFETY: Typechecking should identity set literals as set types. + #[allow(clippy::panic)] + _ => { + panic!("Found set literal when expected {} type", ty); + } + }, + WrappedAccessPaths::Empty => RootAccessTrie::new(), + WrappedAccessPaths::Union(left, right) => left + .full_type_required(ty) + .union(&right.full_type_required(ty)), + } + } +} + +impl RootAccessTrie { + pub(crate) fn add_wrapped_access_paths(&mut self, path: &WrappedAccessPaths) { + match path { + WrappedAccessPaths::AccessPath(access_path) => { + self.add_access_path(access_path, &AccessTrie::new()); + } + WrappedAccessPaths::RecordLiteral(record) => { + for field in record.values() { + self.add_wrapped_access_paths(field); + } + } + WrappedAccessPaths::SetLiteral(elements) => self.add_wrapped_access_paths(elements), + WrappedAccessPaths::Empty => (), + WrappedAccessPaths::Union(left, right) => { + self.add_wrapped_access_paths(left); + self.add_wrapped_access_paths(right); + } + } + } + + pub(crate) fn add_access_path(&mut self, access_path: &AccessPath, leaf_trie: &AccessTrie) { + // could be more efficient by mutating self + // instead we use the existing union function. + let other_trie = access_path.to_root_access_trie_with_leaf(leaf_trie.clone()); + self.union_mut(&other_trie) + } +} + +/// Compute the full [`AccessTrie`] required for the type. +fn type_to_access_trie(ty: &Type) -> AccessTrie { + match ty { + // if it's not an entity or record, slice ends here + Type::ExtensionType { .. } + | Type::Never + | Type::True + | Type::False + | Type::Primitive { .. } + | Type::Set { .. } => AccessTrie::new(), + Type::EntityOrRecord(record_type) => entity_or_record_to_access_trie(record_type), + } +} + +/// Compute the full [`AccessTrie`] for the given entity or record type. +fn entity_or_record_to_access_trie(ty: &EntityRecordKind) -> AccessTrie { + match ty { + EntityRecordKind::ActionEntity { attrs, .. } | EntityRecordKind::Record { attrs, .. } => { + let mut fields = HashMap::new(); + for (attr_name, attr_type) in attrs.iter() { + fields.insert( + attr_name.clone(), + Box::new(type_to_access_trie(&attr_type.attr_type)), + ); + } + AccessTrie { + children: fields, + ancestors_required: false, + data: (), + } + } + + EntityRecordKind::Entity(_) | EntityRecordKind::AnyEntity => { + // no need to load data for entities, which are compared + // using ids + AccessTrie::new() + } + } +} diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index 66b1337753..d9e4ffce01 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -38,6 +38,8 @@ use std::collections::HashSet; #[cfg(feature = "entity-manifest")] pub mod entity_manifest; #[cfg(feature = "entity-manifest")] +mod entity_manifest_analysis; +#[cfg(feature = "entity-manifest")] pub mod entity_slicing; mod err; pub use err::*; diff --git a/cedar-policy/src/api/err.rs b/cedar-policy/src/api/err.rs index 025a2b9a15..48416baad0 100644 --- a/cedar-policy/src/api/err.rs +++ b/cedar-policy/src/api/err.rs @@ -30,9 +30,7 @@ pub use cedar_policy_core::extensions::{ use cedar_policy_core::{ast, authorizer, est}; pub use cedar_policy_validator::cedar_schema::{schema_warnings, SchemaWarning}; #[cfg(feature = "entity-manifest")] -use cedar_policy_validator::entity_manifest::{ - self, FailedAnalysisError, PartialExpressionError, PartialRequestError, -}; +use cedar_policy_validator::entity_manifest::{self, PartialExpressionError, PartialRequestError}; #[cfg(feature = "entity-manifest")] pub use cedar_policy_validator::entity_slicing::EntitySliceError; pub use cedar_policy_validator::{schema_errors, SchemaError}; @@ -1174,11 +1172,6 @@ pub mod request_validation_errors { } /// An error generated by entity slicing. -/// See [`FailedAnalysisError`] for details on the fragment -/// of Cedar handled by entity slicing. -// CAUTION: this type is publicly exported in `cedar-policy`. -// Don't make fields `pub`, don't make breaking changes, and use caution -// when adding public methods. #[derive(Debug, Error, Diagnostic)] #[non_exhaustive] #[cfg(feature = "entity-manifest")] @@ -1200,12 +1193,6 @@ pub enum EntityManifestError { #[error(transparent)] #[diagnostic(transparent)] PartialExpression(#[from] PartialExpressionError), - - /// A policy was not analyzable because it used unsupported operators. - /// See [`FailedAnalysisError`] for more details. - #[error(transparent)] - #[diagnostic(transparent)] - FailedAnalysis(#[from] FailedAnalysisError), } #[cfg(feature = "entity-manifest")] @@ -1218,7 +1205,6 @@ impl From for EntityManifestError { entity_manifest::EntityManifestError::PartialExpression(e) => { Self::PartialExpression(e) } - entity_manifest::EntityManifestError::FailedAnalysis(e) => Self::FailedAnalysis(e), } } }