From f695498603cffc680cffc820fc68c1a7c85dd14d Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Fri, 27 Oct 2023 17:10:45 +0000 Subject: [PATCH 01/11] New API for validating requests --- cedar-policy-core/src/ast/request.rs | 44 ++++- cedar-policy-validator/src/schema.rs | 171 ++++++++++++++++++-- cedar-policy-validator/src/schema/action.rs | 35 ++-- cedar-policy/src/api.rs | 82 ++++++++-- 4 files changed, 294 insertions(+), 38 deletions(-) diff --git a/cedar-policy-core/src/ast/request.rs b/cedar-policy-core/src/ast/request.rs index 59db1ae333..c1564515c1 100644 --- a/cedar-policy-core/src/ast/request.rs +++ b/cedar-policy-core/src/ast/request.rs @@ -94,7 +94,7 @@ impl Request { } } - /// Create a new request with potentially unknown (for partial eval) head variables + /// Create a new `Request` with potentially unknown (for partial eval) variables pub fn new_with_unknowns( principal: EntityUIDEntry, action: EntityUIDEntry, @@ -109,6 +109,36 @@ impl Request { } } + /// Create a new `Request` and validate that it complies with the given `schema` + pub fn new_with_validation( + principal: EntityUID, + action: EntityUID, + resource: EntityUID, + context: Context, + schema: &S, + extensions: Extensions<'_>, + ) -> Result { + let req = Self::new(principal, action, resource, context); + schema.validate_request(&req, extensions)?; + Ok(req) + } + + /// Create a new `Request` with potentially unknown (for partial eval) + /// variables, and validate that it complies with the given `schema` (at + /// least to the extent that we can validate with the given information) + pub fn new_with_unknowns_and_validation( + principal: EntityUIDEntry, + action: EntityUIDEntry, + resource: EntityUIDEntry, + context: Option, + schema: &S, + extensions: Extensions<'_>, + ) -> Result { + let req = Self::new_with_unknowns(principal, action, resource, context); + schema.validate_request(&req, extensions)?; + Ok(req) + } + /// Get the principal associated with the request pub fn principal(&self) -> &EntityUIDEntry { &self.principal @@ -262,6 +292,18 @@ impl std::fmt::Display for Context { } } +/// Trait for schemas capable of validating `Request`s +pub trait RequestSchema { + /// Error type returned when a request fails validation + type Error: std::error::Error; + /// Validate the given `request`, returning `Err` if it fails validation + fn validate_request<'a>( + &self, + request: &Request, + extensions: Extensions<'a>, + ) -> Result<(), Self::Error>; +} + #[cfg(test)] mod test { diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index c2f817214b..1c0ebba3f7 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -32,9 +32,9 @@ use cedar_policy_core::{ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use smol_str::SmolStr; +use thiserror::Error; use super::NamespaceDefinition; -use crate::types::OpenTag; use crate::{ err::*, types::{Attributes, EntityRecordKind, Type}, @@ -530,21 +530,16 @@ impl ValidatorSchema { &self, action: &EntityUID, ) -> Option { - self.get_action_id(action).map(|action_id| { - // The invariant on `ContextSchema` requires that the inner type is - // representable as a schema type. Here we build a closed record - // type, which are representable as long as their values are - // representable. The values are representable because they are - // taken from the context of a `ValidatorActionId` which was - // constructed directly from a schema. - ContextSchema(crate::types::Type::record_with_attributes( - action_id - .context - .iter() - .map(|(k, v)| (k.clone(), v.clone())), - OpenTag::ClosedAttributes, - )) - }) + let ty = self + .get_action_id(action) + .map(ValidatorActionId::context_type); + // The invariant on `ContextSchema` requires that the inner type is + // representable as a schema type. `ValidatorActionId::context_type` + // always returns a closed record type, which are representable as long + // as their values are representable. The values are representable + // because they are taken from the context of a `ValidatorActionId` + // which was constructed directly from a schema. + ty.map(ContextSchema) } /// Invert the action hierarchy to get the ancestor relation expected for @@ -714,6 +709,150 @@ impl cedar_policy_core::entities::EntityTypeDescription for EntityTypeDescriptio } } +impl<'a> cedar_policy_core::ast::RequestSchema for CoreSchema<'a> { + type Error = RequestValidationError; + fn validate_request<'e>( + &self, + request: &cedar_policy_core::ast::Request, + extensions: Extensions<'e>, + ) -> std::result::Result<(), Self::Error> { + use cedar_policy_core::ast::EntityUIDEntry; + // first check that principal and resource are of types that exist in + // the schema, or unspecified. + // we can do this check even if action is unknown. + if let EntityUIDEntry::Concrete(principal) = request.principal() { + match principal.entity_type() { + EntityType::Concrete(name) => { + if !self.schema.entity_types.contains_key(name) { + return Err(RequestValidationError::UndeclaredPrincipalType { + principal_ty: principal.entity_type().clone(), + }); + } + } + EntityType::Unspecified => {} // unspecified principal is allowed, unless we find it is not allowed for this action, which we will check below + } + } + if let EntityUIDEntry::Concrete(resource) = request.resource() { + match resource.entity_type() { + EntityType::Concrete(name) => { + if !self.schema.entity_types.contains_key(name) { + return Err(RequestValidationError::UndeclaredResourceType { + resource_ty: resource.entity_type().clone(), + }); + } + } + EntityType::Unspecified => {} // unspecified resource is allowed, unless we find it is not allowed for this action, which we will check below + } + } + + // the remaining checks require knowing about the action. + match request.action() { + EntityUIDEntry::Concrete(action) => { + let validator_action_id = self.schema.get_action_id(&*action).ok_or_else(|| { + RequestValidationError::UndeclaredAction { + action: Arc::clone(action), + } + })?; + if let EntityUIDEntry::Concrete(principal) = request.principal() { + if !validator_action_id + .applies_to + .is_applicable_principal_type(principal.entity_type()) + { + return Err(RequestValidationError::InvalidPrincipalType { + principal_ty: principal.entity_type().clone(), + action: Arc::clone(action), + }); + } + } + if let EntityUIDEntry::Concrete(resource) = request.resource() { + if !validator_action_id + .applies_to + .is_applicable_resource_type(resource.entity_type()) + { + return Err(RequestValidationError::InvalidResourceType { + resource_ty: resource.entity_type().clone(), + action: Arc::clone(action), + }); + } + } + if let Some(context) = request.context() { + let actual_context_ty = cedar_policy_core::entities::type_of_restricted_expr( + context.as_ref().as_borrowed(), + extensions, + )?; + let expected_context_ty = validator_action_id.context_type(); + if !expected_context_ty.is_consistent_with(&actual_context_ty) { + return Err(RequestValidationError::InvalidContext { + context: context.clone(), + action: Arc::clone(action), + }); + } + } + } + EntityUIDEntry::Unknown => { + // We could hypothetically ensure that the concrete parts of the + // request are valid for _some_ action, but this is probably more + // expensive than we want for this validation step. + // Instead, we just let the above checks (that principal and + // resource are of types that at least _exist_ in the schema) + // suffice. + } + } + Ok(()) + } +} + +#[derive(Debug, Error)] +pub enum RequestValidationError { + /// Request action is not declared in the schema + #[error("request's action `{action}` is not declared in the schema")] + UndeclaredAction { + /// Action which was not declared in the schema + action: Arc, + }, + /// Request principal is of a type not declared in the schema + #[error("principal type `{principal_ty}` is not declared in the schema")] + UndeclaredPrincipalType { + /// Principal type which was not declared in the schema + principal_ty: EntityType, + }, + /// Request resource is of a type not declared in the schema + #[error("resource type `{resource_ty}` is not declared in the schema")] + UndeclaredResourceType { + /// Resource type which was not declared in the schema + resource_ty: EntityType, + }, + /// Request principal is of a type that is declared in the schema, but is + /// not valid for the request action + #[error("principal type `{principal_ty}` is not valid for `{action}`")] + InvalidPrincipalType { + /// Principal type which is not valid + principal_ty: EntityType, + /// Action which it is not valid for + action: Arc, + }, + /// Request resource is of a type that is declared in the schema, but is + /// not valid for the request action + #[error("resource type `{resource_ty}` is not valid for `{action}`")] + InvalidResourceType { + /// Resource type which is not valid + resource_ty: EntityType, + /// Action which it is not valid for + action: Arc, + }, + /// Context does not comply with the shape specified for the request action + #[error("context `{context}` is not valid for `{action}`")] + InvalidContext { + /// Context which is not valid + context: cedar_policy_core::ast::Context, + /// Action which it is not valid for + action: Arc, + }, + /// Error getting the type of the `Context` + #[error("error while computing the type of the request context: {0}")] + TypeOfContext(#[from] cedar_policy_core::entities::TypeOfRestrictedExprError), +} + /// Struct which carries enough information that it can impl Core's /// `ContextSchema` INVARIANT: The `Type` stored in this struct must be /// representable as a `SchemaType` to avoid panicking in `context_type`. diff --git a/cedar-policy-validator/src/schema/action.rs b/cedar-policy-validator/src/schema/action.rs index 9723e8e91b..8da619a8c4 100644 --- a/cedar-policy-validator/src/schema/action.rs +++ b/cedar-policy-validator/src/schema/action.rs @@ -8,7 +8,7 @@ use serde::Serialize; use smol_str::SmolStr; use std::collections::{HashMap, HashSet}; -use crate::types::{AttributeType, Attributes}; +use crate::types::{Attributes, OpenTag, Type}; /// Contains information about actions used by the validator. The contents of /// the struct are the same as the schema entity type structure, but the @@ -42,9 +42,14 @@ pub struct ValidatorActionId { } impl ValidatorActionId { - /// An iterator over the attributes of this action's required context - pub fn context(&self) -> impl Iterator { - self.context.iter() + /// The `Type` that this action requires for its context. + /// + /// This always returns a closed record type. + pub fn context_type(&self) -> Type { + Type::record_with_attributes( + self.context.iter().map(|(k, v)| (k.clone(), v.clone())), + OpenTag::ClosedAttributes, + ) } } @@ -72,9 +77,9 @@ pub(crate) struct ValidatorApplySpec { /// The principal entity types the action can be applied to. This set may /// be a singleton set containing the unspecified entity type when the /// `principalTypes` list is omitted in the schema. A non-singleton set - /// shouldn't contain the unspecified entity type, but validation will give - /// the same success/failure result as when it is the only element of the - /// set, perhaps with extra type errors. + /// shouldn't contain the unspecified entity type, but (policy) validation + /// will give the same success/failure result as when it is the only element + /// of the set, perhaps with extra type errors. #[serde(rename = "principalApplySpec")] principal_apply_spec: HashSet, @@ -87,7 +92,7 @@ pub(crate) struct ValidatorApplySpec { impl ValidatorApplySpec { /// Create an apply spec for an action that can only be applied to some /// specific entities. - pub(crate) fn new( + pub fn new( principal_apply_spec: HashSet, resource_apply_spec: HashSet, ) -> Self { @@ -97,13 +102,23 @@ impl ValidatorApplySpec { } } + /// Is the given principal type applicable for this spec? + pub fn is_applicable_principal_type(&self, ty: &EntityType) -> bool { + self.principal_apply_spec.contains(ty) + } + /// Get the applicable principal types for this spec. - pub(crate) fn applicable_principal_types(&self) -> impl Iterator { + pub fn applicable_principal_types(&self) -> impl Iterator { self.principal_apply_spec.iter() } + /// Is the given resource type applicable for this spec? + pub fn is_applicable_resource_type(&self, ty: &EntityType) -> bool { + self.resource_apply_spec.contains(ty) + } + /// Get the applicable resource types for this spec. - pub(crate) fn applicable_resource_types(&self) -> impl Iterator { + pub fn applicable_resource_types(&self) -> impl Iterator { self.resource_apply_spec.iter() } } diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 038f5ecd2b..dd84e2023e 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -38,6 +38,7 @@ use cedar_policy_core::parser; pub use cedar_policy_core::parser::err::ParseErrors; use cedar_policy_core::parser::SourceInfo; use cedar_policy_core::FromNormalizedStr; +use cedar_policy_validator::RequestValidationError; // this type is unsuitable for `pub use` because it contains internal types like `EntityUID` and `EntityType` pub use cedar_policy_validator::{ TypeErrorKind, UnsupportedFeature, ValidationErrorKind, ValidationWarningKind, }; @@ -2866,28 +2867,31 @@ impl FromStr for RestrictedExpression { /// for partial evaluation. #[cfg(feature = "partial-eval")] #[derive(Debug)] -pub struct RequestBuilder { +pub struct RequestBuilder<'a> { principal: ast::EntityUIDEntry, action: ast::EntityUIDEntry, resource: ast::EntityUIDEntry, /// Here, `None` means unknown context: Option, + /// Here, `None` means no request validation is performed + schema: Option<&'a Schema>, } #[cfg(feature = "partial-eval")] -impl Default for RequestBuilder { +impl<'a> Default for RequestBuilder<'a> { fn default() -> Self { Self { principal: ast::EntityUIDEntry::Unknown, action: ast::EntityUIDEntry::Unknown, resource: ast::EntityUIDEntry::Unknown, context: None, + schema: None, } } } #[cfg(feature = "partial-eval")] -impl RequestBuilder { +impl<'a> RequestBuilder<'a> { /// Set the principal. /// /// Note that you can create the `EntityUid` using `.parse()` on any @@ -2959,14 +2963,32 @@ impl RequestBuilder { } } + /// Set the schema. If present, this will be used for request validation. + pub fn schema(self, schema: &'a Schema) -> Self { + Self { + schema: Some(schema), + ..self + } + } + /// Create the [`Request`] - pub fn build(self) -> Request { - Request(ast::Request::new_with_unknowns( - self.principal, - self.action, - self.resource, - self.context, - )) + pub fn build(self) -> Result { + match self.schema { + None => Ok(Request(ast::Request::new_with_unknowns( + self.principal, + self.action, + self.resource, + self.context, + ))), + Some(schema) => Ok(Request(ast::Request::new_with_unknowns_and_validation( + self.principal, + self.action, + self.resource, + self.context, + &cedar_policy_validator::CoreSchema::new(&schema.0), + Extensions::all_available(), + )?)), + } } } @@ -2978,7 +3000,7 @@ pub struct Request(pub(crate) ast::Request); impl Request { /// Create a [`RequestBuilder`] #[cfg(feature = "partial-eval")] - pub fn builder() -> RequestBuilder { + pub fn builder<'a>() -> RequestBuilder<'a> { RequestBuilder::default() } @@ -3012,6 +3034,44 @@ impl Request { Self(ast::Request::new(p, a, r, context.0)) } + /// Create a Request, validating that it complies with the given `Schema`. + /// + /// Note that you can create the `EntityUid`s using `.parse()` on any + /// string (via the `FromStr` implementation for `EntityUid`). + /// The principal, action, and resource fields are optional to support + /// the case where these fields do not contribute to authorization + /// decisions (e.g., because they are not used in your policies). + /// If any of the fields are `None`, we will automatically generate + /// a unique entity UID that is not equal to any UID in the store. + pub fn new_with_validation( + principal: Option, + action: Option, + resource: Option, + context: Context, + schema: &Schema, + ) -> Result { + let p = match principal { + Some(p) => p.0, + None => ast::EntityUID::unspecified_from_eid(ast::Eid::new("principal")), + }; + let a = match action { + Some(a) => a.0, + None => ast::EntityUID::unspecified_from_eid(ast::Eid::new("action")), + }; + let r = match resource { + Some(r) => r.0, + None => ast::EntityUID::unspecified_from_eid(ast::Eid::new("resource")), + }; + Ok(Self(ast::Request::new_with_validation( + p, + a, + r, + context.0, + &cedar_policy_validator::CoreSchema::new(&schema.0), + Extensions::all_available(), + )?)) + } + /// Get the principal component of the request. Returns `None` if the principal is /// "unspecified" (i.e., constructed by passing `None` into the constructor) or /// "unknown" (i.e., constructed using the partial evaluation APIs). From b7b418df67a64d5ab11b297355e9b731846b625e Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 30 Oct 2023 15:42:03 +0000 Subject: [PATCH 02/11] update CHANGELOG --- cedar-policy/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index dc70befe03..9e7e5bfb6d 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -17,6 +17,7 @@ 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`. +- `Request::new_with_validation` to construct a `Request` while validating it against a `Schema`. ### Changed From 41b3842a13acbdf9931f939d3d84ccefff7edd95 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 31 Oct 2023 14:14:21 +0000 Subject: [PATCH 03/11] add tests; move a bunch of stuff to new module --- cedar-policy-core/src/ast/request.rs | 2 +- cedar-policy-validator/Cargo.toml | 3 + cedar-policy-validator/src/coreschema.rs | 806 +++++++++++++++++++++++ cedar-policy-validator/src/lib.rs | 15 +- cedar-policy-validator/src/schema.rs | 323 +-------- cedar-policy-validator/src/types.rs | 6 +- cedar-policy/src/api.rs | 11 +- 7 files changed, 836 insertions(+), 330 deletions(-) create mode 100644 cedar-policy-validator/src/coreschema.rs diff --git a/cedar-policy-core/src/ast/request.rs b/cedar-policy-core/src/ast/request.rs index c1564515c1..474df23001 100644 --- a/cedar-policy-core/src/ast/request.rs +++ b/cedar-policy-core/src/ast/request.rs @@ -182,7 +182,7 @@ impl std::fmt::Display for Request { } /// `Context` field of a `Request` -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Context { /// an `Expr::Record` that qualifies as a "restricted expression" /// INVARIANT: This must be of the variant `Record` diff --git a/cedar-policy-validator/Cargo.toml b/cedar-policy-validator/Cargo.toml index 7471c26178..e785a4b677 100644 --- a/cedar-policy-validator/Cargo.toml +++ b/cedar-policy-validator/Cargo.toml @@ -31,3 +31,6 @@ decimal = ["cedar-policy-core/decimal"] # Enables `Arbitrary` implementations for several types in this crate arbitrary = ["dep:arbitrary"] + +[dev-dependencies] +cool_asserts = "2.0" diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs new file mode 100644 index 0000000000..d94fec18ad --- /dev/null +++ b/cedar-policy-validator/src/coreschema.rs @@ -0,0 +1,806 @@ +use crate::{ValidatorEntityType, ValidatorSchema}; +use cedar_policy_core::extensions::Extensions; +use cedar_policy_core::{ast, entities}; +use smol_str::SmolStr; +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; +use thiserror::Error; + +/// Struct which carries enough information that it can (efficiently) impl Core's `Schema` +pub struct CoreSchema<'a> { + /// Contains all the information + schema: &'a ValidatorSchema, + /// For easy lookup, this is a map from action name to `Entity` object + /// for each action in the schema. This information is contained in the + /// `ValidatorSchema`, but not efficient to extract -- getting the `Entity` + /// from the `ValidatorSchema` is O(N) as of this writing, but with this + /// cache it's O(1). + actions: HashMap>, +} + +impl<'a> CoreSchema<'a> { + pub fn new(schema: &'a ValidatorSchema) -> Self { + Self { + actions: schema + .action_entities_iter() + .map(|e| (e.uid(), Arc::new(e))) + .collect(), + schema, + } + } +} + +impl<'a> entities::Schema for CoreSchema<'a> { + type EntityTypeDescription = EntityTypeDescription; + type ActionEntityIterator = Vec>; + + fn entity_type(&self, entity_type: &ast::EntityType) -> Option { + match entity_type { + ast::EntityType::Unspecified => None, // Unspecified entities cannot be declared in the schema and should not appear in JSON data + ast::EntityType::Concrete(name) => EntityTypeDescription::new(self.schema, name), + } + } + + fn action(&self, action: &ast::EntityUID) -> Option> { + self.actions.get(action).map(Arc::clone) + } + + fn entity_types_with_basename<'b>( + &'b self, + basename: &'b ast::Id, + ) -> Box + 'b> { + Box::new(self.schema.entity_types().filter_map(move |(name, _)| { + if name.basename() == basename { + Some(ast::EntityType::Concrete(name.clone())) + } else { + None + } + })) + } + + 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` +pub struct EntityTypeDescription { + /// Core `EntityType` this is describing + core_type: ast::EntityType, + /// Contains most of the schema information for this entity type + validator_type: ValidatorEntityType, + /// Allowed parent types for this entity type. (As of this writing, this + /// information is not contained in the `validator_type` by itself.) + allowed_parent_types: Arc>, +} + +impl EntityTypeDescription { + /// Create a description of the given type in the given schema. + /// Returns `None` if the given type is not in the given schema. + pub fn new(schema: &ValidatorSchema, type_name: &ast::Name) -> Option { + Some(Self { + core_type: ast::EntityType::Concrete(type_name.clone()), + validator_type: schema.get_entity_type(type_name).cloned()?, + allowed_parent_types: { + let mut set = HashSet::new(); + for (possible_parent_typename, possible_parent_et) in schema.entity_types() { + if possible_parent_et.descendants.contains(type_name) { + set.insert(ast::EntityType::Concrete(possible_parent_typename.clone())); + } + } + Arc::new(set) + }, + }) + } +} + +impl entities::EntityTypeDescription for EntityTypeDescription { + fn entity_type(&self) -> ast::EntityType { + self.core_type.clone() + } + + fn attr_type(&self, attr: &str) -> Option { + let attr_type: &crate::types::Type = &self.validator_type.attr(attr)?.attr_type; + // This converts a type from a schema into the representation of schema + // types used by core. `attr_type` is taken from a `ValidatorEntityType` + // which was constructed from a schema. + // PANIC SAFETY: see above + #[allow(clippy::expect_used)] + let core_schema_type: entities::SchemaType = attr_type + .clone() + .try_into() + .expect("failed to convert validator type into Core SchemaType"); + debug_assert!(attr_type.is_consistent_with(&core_schema_type)); + Some(core_schema_type) + } + + fn required_attrs<'s>(&'s self) -> Box + 's> { + Box::new( + self.validator_type + .attributes + .iter() + .filter(|(_, ty)| ty.is_required) + .map(|(attr, _)| attr.clone()), + ) + } + + fn allowed_parent_types(&self) -> Arc> { + Arc::clone(&self.allowed_parent_types) + } +} + +impl ast::RequestSchema for ValidatorSchema { + type Error = RequestValidationError; + fn validate_request<'e>( + &self, + request: &ast::Request, + extensions: Extensions<'e>, + ) -> std::result::Result<(), Self::Error> { + use ast::EntityUIDEntry; + // first check that principal and resource are of types that exist in + // the schema, or unspecified. + // we can do this check even if action is unknown. + if let EntityUIDEntry::Concrete(principal) = request.principal() { + match principal.entity_type() { + ast::EntityType::Concrete(name) => { + if self.get_entity_type(name).is_none() { + return Err(RequestValidationError::UndeclaredPrincipalType { + principal_ty: principal.entity_type().clone(), + }); + } + } + ast::EntityType::Unspecified => {} // unspecified principal is allowed, unless we find it is not allowed for this action, which we will check below + } + } + if let EntityUIDEntry::Concrete(resource) = request.resource() { + match resource.entity_type() { + ast::EntityType::Concrete(name) => { + if self.get_entity_type(name).is_none() { + return Err(RequestValidationError::UndeclaredResourceType { + resource_ty: resource.entity_type().clone(), + }); + } + } + ast::EntityType::Unspecified => {} // unspecified resource is allowed, unless we find it is not allowed for this action, which we will check below + } + } + + // the remaining checks require knowing about the action. + match request.action() { + EntityUIDEntry::Concrete(action) => { + let validator_action_id = self.get_action_id(&*action).ok_or_else(|| { + RequestValidationError::UndeclaredAction { + action: Arc::clone(action), + } + })?; + if let EntityUIDEntry::Concrete(principal) = request.principal() { + if !validator_action_id + .applies_to + .is_applicable_principal_type(principal.entity_type()) + { + return Err(RequestValidationError::InvalidPrincipalType { + principal_ty: principal.entity_type().clone(), + action: Arc::clone(action), + }); + } + } + if let EntityUIDEntry::Concrete(resource) = request.resource() { + if !validator_action_id + .applies_to + .is_applicable_resource_type(resource.entity_type()) + { + return Err(RequestValidationError::InvalidResourceType { + resource_ty: resource.entity_type().clone(), + action: Arc::clone(action), + }); + } + } + if let Some(context) = request.context() { + let actual_context_ty = entities::type_of_restricted_expr( + context.as_ref().as_borrowed(), + extensions, + )?; + println!("actual context ty is {actual_context_ty}"); + let expected_context_ty = validator_action_id.context_type(); + println!("expected context ty is {expected_context_ty}"); + if !expected_context_ty.is_consistent_with(&actual_context_ty) { + return Err(RequestValidationError::InvalidContext { + context: context.clone(), + action: Arc::clone(action), + }); + } + } + } + EntityUIDEntry::Unknown => { + // We could hypothetically ensure that the concrete parts of the + // request are valid for _some_ action, but this is probably more + // expensive than we want for this validation step. + // Instead, we just let the above checks (that principal and + // resource are of types that at least _exist_ in the schema) + // suffice. + } + } + Ok(()) + } +} + +impl<'a> ast::RequestSchema for CoreSchema<'a> { + type Error = RequestValidationError; + fn validate_request<'e>( + &self, + request: &ast::Request, + extensions: Extensions<'e>, + ) -> Result<(), Self::Error> { + self.schema.validate_request(request, extensions) + } +} + +#[derive(Debug, Error)] +pub enum RequestValidationError { + /// Request action is not declared in the schema + #[error("request's action `{action}` is not declared in the schema")] + UndeclaredAction { + /// Action which was not declared in the schema + action: Arc, + }, + /// Request principal is of a type not declared in the schema + #[error("principal type `{principal_ty}` is not declared in the schema")] + UndeclaredPrincipalType { + /// Principal type which was not declared in the schema + principal_ty: ast::EntityType, + }, + /// Request resource is of a type not declared in the schema + #[error("resource type `{resource_ty}` is not declared in the schema")] + UndeclaredResourceType { + /// Resource type which was not declared in the schema + resource_ty: ast::EntityType, + }, + /// Request principal is of a type that is declared in the schema, but is + /// not valid for the request action + #[error("principal type `{principal_ty}` is not valid for `{action}`")] + InvalidPrincipalType { + /// Principal type which is not valid + principal_ty: ast::EntityType, + /// Action which it is not valid for + action: Arc, + }, + /// Request resource is of a type that is declared in the schema, but is + /// not valid for the request action + #[error("resource type `{resource_ty}` is not valid for `{action}`")] + InvalidResourceType { + /// Resource type which is not valid + resource_ty: ast::EntityType, + /// Action which it is not valid for + action: Arc, + }, + /// Context does not comply with the shape specified for the request action + #[error("context `{context}` is not valid for `{action}`")] + InvalidContext { + /// Context which is not valid + context: ast::Context, + /// Action which it is not valid for + action: Arc, + }, + /// Error getting the type of the `Context` + #[error("error while computing the type of the request context: {0}")] + TypeOfContext(#[from] entities::TypeOfRestrictedExprError), +} + +/// Struct which carries enough information that it can impl Core's +/// `ContextSchema`. +pub struct ContextSchema( + // INVARIANT: The `Type` stored in this struct must be representable as a + // `SchemaType` to avoid panicking in `context_type`. + crate::types::Type, +); + +/// A `Type` contains all the information we need for a Core `ContextSchema`. +impl entities::ContextSchema for ContextSchema { + fn context_type(&self) -> entities::SchemaType { + // PANIC SAFETY: By `ContextSchema` invariant, `self.0` is representable as a schema type. + #[allow(clippy::expect_used)] + self.0 + .clone() + .try_into() + .expect("failed to convert validator type into Core SchemaType") + } +} + +/// Since different Actions have different schemas for `Context`, you must +/// specify the `Action` in order to get a `ContextSchema`. +/// +/// Returns `None` if the action is not in the schema. +pub fn context_schema_for_action( + schema: &ValidatorSchema, + action: &ast::EntityUID, +) -> Option { + // The invariant on `ContextSchema` requires that the inner type is + // representable as a schema type. `ValidatorSchema::context_type` + // always returns a closed record type, which are representable as long + // as their values are representable. The values are representable + // because they are taken from the context of a `ValidatorActionId` + // which was constructed directly from a schema. + schema.context_type(action).map(ContextSchema) +} + +#[cfg(test)] +mod test { + use super::*; + use cool_asserts::assert_matches; + use serde_json::json; + + fn schema() -> ValidatorSchema { + let src = json!( + { "": { + "entityTypes": { + "User": { + "memberOfTypes": [ "Group" ] + }, + "Group": { + "memberOfTypes": [] + }, + "Photo": { + "memberOfTypes": [ "Album" ] + }, + "Album": { + "memberOfTypes": [] + } + }, + "actions": { + "view_photo": { + "appliesTo": { + "principalTypes": ["User", "Group"], + "resourceTypes": ["Photo"] + } + }, + "edit_photo": { + "appliesTo": { + "principalTypes": ["User", "Group"], + "resourceTypes": ["Photo"], + "context": { + "type": "Record", + "attributes": { + "admin_approval": { + "type": "Boolean", + "required": true, + } + } + } + } + } + } + }}); + ValidatorSchema::from_json_value(src).expect("failed to create ValidatorSchema") + } + + /// basic success with concrete request and no context + #[test] + fn success_concrete_request_no_context() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Ok(_) + ); + } + + /// basic success with concrete request and a context + #[test] + fn success_concrete_request_with_context() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::from_pairs([( + "admin_approval".into(), + ast::RestrictedExpr::val(true) + )]) + .unwrap(), + &schema(), + Extensions::all_available(), + ), + Ok(_) + ); + } + + /// success leaving principal unknown + #[test] + fn success_principal_unknown() { + assert_matches!( + ast::Request::new_with_unknowns_and_validation( + ast::EntityUIDEntry::Unknown, + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap() + ), + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap() + ), + Some(ast::Context::empty()), + &schema(), + Extensions::all_available(), + ), + Ok(_) + ); + } + + /// success leaving action unknown + #[test] + fn success_action_unknown() { + assert_matches!( + ast::Request::new_with_unknowns_and_validation( + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap() + ), + ast::EntityUIDEntry::Unknown, + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap() + ), + Some(ast::Context::empty()), + &schema(), + Extensions::all_available(), + ), + Ok(_) + ); + } + + /// success leaving resource unknown + #[test] + fn success_resource_unknown() { + assert_matches!( + ast::Request::new_with_unknowns_and_validation( + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap() + ), + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap() + ), + ast::EntityUIDEntry::Unknown, + Some(ast::Context::empty()), + &schema(), + Extensions::all_available(), + ), + Ok(_) + ); + } + + /// success leaving context unknown + #[test] + fn success_context_unknown() { + assert_matches!( + ast::Request::new_with_unknowns_and_validation( + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap() + ), + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap() + ), + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap() + ), + None, + &schema(), + Extensions::all_available(), + ), + Ok(_) + ) + } + + /// success leaving everything unknown + #[test] + fn success_everything_unspecified() { + assert_matches!( + ast::Request::new_with_unknowns_and_validation( + ast::EntityUIDEntry::Unknown, + ast::EntityUIDEntry::Unknown, + ast::EntityUIDEntry::Unknown, + None, + &schema(), + Extensions::all_available(), + ), + Ok(_) + ); + } + + /// this succeeds for now: unknown action, concrete principal and + /// resource of valid types, but none of the schema's actions would work + /// with this principal and resource type + #[test] + fn success_unknown_action_but_invalid_types() { + assert_matches!( + ast::Request::new_with_unknowns_and_validation( + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("Album", "abc123").unwrap() + ), + ast::EntityUIDEntry::Unknown, + ast::EntityUIDEntry::concrete( + ast::EntityUID::with_eid_and_type("User", "alice").unwrap() + ), + None, + &schema(), + Extensions::all_available(), + ), + Ok(_) + ); + } + + /// request action not declared in the schema + #[test] + fn action_not_declared() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "destroy").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::UndeclaredAction { action }) => { + assert_eq!(&*action, &ast::EntityUID::with_eid_and_type("Action", "destroy").unwrap()); + } + ); + } + + /// request action unspecified (and not declared in the schema) + #[test] + fn action_unspecified() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::unspecified_from_eid(ast::Eid::new("blahblah")), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::UndeclaredAction { action }) => { + assert_eq!(&*action, &ast::EntityUID::unspecified_from_eid(ast::Eid::new("blahblah"))); + } + ); + } + + /// request principal type not declared in the schema (action concrete) + #[test] + fn principal_type_not_declared() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("Foo", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::UndeclaredPrincipalType { principal_ty }) => { + assert_eq!(principal_ty, ast::EntityType::Concrete(ast::Name::parse_unqualified_name("Foo").unwrap())); + } + ); + } + + /// request principal type not declared in the schema (action unspecified) + #[test] + fn principal_type_not_declared_action_unspecified() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("Foo", "abc123").unwrap(), + ast::EntityUID::unspecified_from_eid(ast::Eid::new("blahblah")), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::UndeclaredPrincipalType { principal_ty }) => { + assert_eq!(principal_ty, ast::EntityType::Concrete(ast::Name::parse_unqualified_name("Foo").unwrap())); + } + ); + } + + /// request principal type unspecified (and not declared in the schema) + #[test] + fn principal_unspecified() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::unspecified_from_eid(ast::Eid::new("principal")), + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::InvalidPrincipalType { principal_ty, .. }) => { + assert_eq!(principal_ty, ast::EntityType::Unspecified); + } + ); + } + + /// request resource type not declared in the schema (action concrete) + #[test] + fn resource_type_not_declared() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Foo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::UndeclaredResourceType { resource_ty }) => { + assert_eq!(resource_ty, ast::EntityType::Concrete(ast::Name::parse_unqualified_name("Foo").unwrap())); + } + ); + } + + /// request resource type not declared in the schema (action unspecified) + #[test] + fn resource_type_not_declared_action_unspecified() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::unspecified_from_eid(ast::Eid::new("blahblah")), + ast::EntityUID::with_eid_and_type("Foo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::UndeclaredResourceType { resource_ty }) => { + assert_eq!(resource_ty, ast::EntityType::Concrete(ast::Name::parse_unqualified_name("Foo").unwrap())); + } + ); + } + + /// request resource type unspecified (and not declared in the schema) + #[test] + fn resource_unspecified() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), + ast::EntityUID::unspecified_from_eid(ast::Eid::new("resource")), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::InvalidResourceType { resource_ty, .. }) => { + assert_eq!(resource_ty, ast::EntityType::Unspecified); + } + ); + } + + /// request principal type declared, but invalid for request's action + #[test] + fn principal_type_invalid() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("Album", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::InvalidPrincipalType { principal_ty, action }) => { + assert_eq!(principal_ty, ast::EntityType::Concrete(ast::Name::parse_unqualified_name("Album").unwrap())); + assert_eq!(&*action, &ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap()); + } + ); + } + + /// request resource type declared, but invalid for request's action + #[test] + fn resource_type_invalid() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Group", "coders").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::InvalidResourceType { resource_ty, action }) => { + assert_eq!(resource_ty, ast::EntityType::Concrete(ast::Name::parse_unqualified_name("Group").unwrap())); + assert_eq!(&*action, &ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap()); + } + ); + } + + /// request context does not comply with specification: missing attribute + #[test] + fn context_missing_attribute() { + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + ast::Context::empty(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::InvalidContext { context, action }) => { + assert_eq!(context, ast::Context::empty()); + assert_eq!(&*action, &ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap()); + } + ); + } + + /// request context does not comply with specification: extra attribute + #[test] + fn context_extra_attribute() { + let context_with_extra_attr = ast::Context::from_pairs([ + ("admin_approval".into(), ast::RestrictedExpr::val(true)), + ("extra".into(), ast::RestrictedExpr::val(42)), + ]) + .unwrap(); + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + context_with_extra_attr.clone(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::InvalidContext { context, action }) => { + assert_eq!(context, context_with_extra_attr); + assert_eq!(&*action, &ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap()); + } + ); + } + + /// request context does not comply with specification: attribute is wrong type + #[test] + fn context_attribute_wrong_type() { + let context_with_wrong_type_attr = ast::Context::from_pairs([( + "admin_approval".into(), + ast::RestrictedExpr::set([ast::RestrictedExpr::val(true)]), + )]) + .unwrap(); + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + context_with_wrong_type_attr.clone(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::InvalidContext { context, action }) => { + assert_eq!(context, context_with_wrong_type_attr); + assert_eq!(&*action, &ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap()); + } + ); + } + + /// request context contains heterogeneous set + #[test] + fn context_attribute_heterogeneous_set() { + let context_with_heterogeneous_set = ast::Context::from_pairs([( + "admin_approval".into(), + ast::RestrictedExpr::set([ + ast::RestrictedExpr::val(true), + ast::RestrictedExpr::val(-1001), + ]), + )]) + .unwrap(); + assert_matches!( + ast::Request::new_with_validation( + ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), + ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), + ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), + context_with_heterogeneous_set.clone(), + &schema(), + Extensions::all_available(), + ), + Err(RequestValidationError::TypeOfContext(entities::TypeOfRestrictedExprError::HeterogeneousSet(err))) => { + assert!(err.to_string().contains("set elements have different types: bool and long"), "actual error message was {err}"); + } + ); + } +} diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index 7dc91bbd64..7a21305b85 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -17,34 +17,33 @@ //! Validator for Cedar policies #![forbid(unsafe_code)] -use std::collections::HashSet; - use cedar_policy_core::ast::{Policy, PolicySet, Template}; +use serde::Serialize; +use std::collections::HashSet; mod err; -mod str_checks; pub use err::*; +mod coreschema; +pub use coreschema::*; mod expr_iterator; mod extension_schema; mod extensions; mod fuzzy_match; mod validation_result; -use serde::Serialize; pub use validation_result::*; mod rbac; mod schema; pub use schema::*; mod schema_file_format; pub use schema_file_format::*; +mod str_checks; +pub use str_checks::{confusable_string_checks, ValidationWarning, ValidationWarningKind}; mod type_error; pub use type_error::*; pub mod typecheck; +use typecheck::Typechecker; pub mod types; -pub use str_checks::{confusable_string_checks, ValidationWarning, ValidationWarningKind}; - -use self::typecheck::Typechecker; - /// Used to select how a policy will be validated. #[derive(Default, Eq, PartialEq, Copy, Clone, Debug, Serialize)] pub enum ValidationMode { diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 1c0ebba3f7..85da2f4052 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -21,18 +21,15 @@ //! computed to obtain a `descendants` relation. use std::collections::{hash_map::Entry, HashMap, HashSet}; -use std::sync::Arc; use cedar_policy_core::{ - ast::{Entity, EntityType, EntityUID, Id, Name}, + ast::{Entity, EntityType, EntityUID, Name}, entities::{Entities, TCComputation}, extensions::Extensions, transitive_closure::compute_tc, }; use serde::{Deserialize, Serialize}; use serde_with::serde_as; -use smol_str::SmolStr; -use thiserror::Error; use super::NamespaceDefinition; use crate::{ @@ -522,29 +519,22 @@ impl ValidatorSchema { .flat_map(move |e| self.get_entities_in(var, e)) } - /// Since different Actions have different schemas for `Context`, you must - /// specify the `Action` in order to get a `ContextSchema`. + /// Get the `Type` of context expected for the given `action`. + /// This always reutrns a closed record type. /// /// Returns `None` if the action is not in the schema. - pub fn get_context_schema( - &self, - action: &EntityUID, - ) -> Option { - let ty = self - .get_action_id(action) - .map(ValidatorActionId::context_type); - // The invariant on `ContextSchema` requires that the inner type is - // representable as a schema type. `ValidatorActionId::context_type` - // always returns a closed record type, which are representable as long - // as their values are representable. The values are representable - // because they are taken from the context of a `ValidatorActionId` - // which was constructed directly from a schema. - ty.map(ContextSchema) + pub fn context_type(&self, action: &EntityUID) -> Option { + // INVARIANT: `ValidatorActionId::context_type` always returns a closed + // record type + self.get_action_id(action) + .map(ValidatorActionId::context_type) } /// Invert the action hierarchy to get the ancestor relation expected for /// the `Entity` datatype instead of descendants as stored by the schema. - fn action_entities_iter(&self) -> impl Iterator + '_ { + pub(crate) 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 // actually less error prone, as it minimizes the threading of data @@ -579,297 +569,6 @@ impl ValidatorSchema { } } -/// Struct which carries enough information that it can (efficiently) impl Core's `Schema` -pub struct CoreSchema<'a> { - /// Contains all the information - schema: &'a ValidatorSchema, - /// For easy lookup, this is a map from action name to `Entity` object - /// for each action in the schema. This information is contained in the - /// `ValidatorSchema`, but not efficient to extract -- getting the `Entity` - /// from the `ValidatorSchema` is O(N) as of this writing, but with this - /// cache it's O(1). - actions: HashMap>, -} - -impl<'a> CoreSchema<'a> { - pub fn new(schema: &'a ValidatorSchema) -> Self { - Self { - actions: schema - .action_entities_iter() - .map(|e| (e.uid(), Arc::new(e))) - .collect(), - schema, - } - } -} - -impl<'a> cedar_policy_core::entities::Schema for CoreSchema<'a> { - type EntityTypeDescription = EntityTypeDescription; - type ActionEntityIterator = Vec>; - - fn entity_type( - &self, - entity_type: &cedar_policy_core::ast::EntityType, - ) -> Option { - match entity_type { - cedar_policy_core::ast::EntityType::Unspecified => None, // Unspecified entities cannot be declared in the schema and should not appear in JSON data - cedar_policy_core::ast::EntityType::Concrete(name) => { - EntityTypeDescription::new(self.schema, name) - } - } - } - - fn action(&self, action: &EntityUID) -> Option> { - self.actions.get(action).map(Arc::clone) - } - - fn entity_types_with_basename<'b>( - &'b self, - basename: &'b Id, - ) -> Box + 'b> { - Box::new(self.schema.entity_types().filter_map(move |(name, _)| { - if name.basename() == basename { - Some(EntityType::Concrete(name.clone())) - } else { - None - } - })) - } - - 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` -pub struct EntityTypeDescription { - /// Core `EntityType` this is describing - core_type: cedar_policy_core::ast::EntityType, - /// Contains most of the schema information for this entity type - validator_type: ValidatorEntityType, - /// Allowed parent types for this entity type. (As of this writing, this - /// information is not contained in the `validator_type` by itself.) - allowed_parent_types: Arc>, -} - -impl EntityTypeDescription { - /// Create a description of the given type in the given schema. - /// Returns `None` if the given type is not in the given schema. - pub fn new(schema: &ValidatorSchema, type_name: &Name) -> Option { - Some(Self { - core_type: cedar_policy_core::ast::EntityType::Concrete(type_name.clone()), - validator_type: schema.get_entity_type(type_name).cloned()?, - allowed_parent_types: { - let mut set = HashSet::new(); - for (possible_parent_typename, possible_parent_et) in &schema.entity_types { - if possible_parent_et.descendants.contains(type_name) { - set.insert(cedar_policy_core::ast::EntityType::Concrete( - possible_parent_typename.clone(), - )); - } - } - Arc::new(set) - }, - }) - } -} - -impl cedar_policy_core::entities::EntityTypeDescription for EntityTypeDescription { - fn entity_type(&self) -> cedar_policy_core::ast::EntityType { - self.core_type.clone() - } - - fn attr_type(&self, attr: &str) -> Option { - let attr_type: &crate::types::Type = &self.validator_type.attr(attr)?.attr_type; - // This converts a type from a schema into the representation of schema - // types used by core. `attr_type` is taken from a `ValidatorEntityType` - // which was constructed from a schema. - // PANIC SAFETY: see above - #[allow(clippy::expect_used)] - let core_schema_type: cedar_policy_core::entities::SchemaType = attr_type - .clone() - .try_into() - .expect("failed to convert validator type into Core SchemaType"); - debug_assert!(attr_type.is_consistent_with(&core_schema_type)); - Some(core_schema_type) - } - - fn required_attrs<'s>(&'s self) -> Box + 's> { - Box::new( - self.validator_type - .attributes - .iter() - .filter(|(_, ty)| ty.is_required) - .map(|(attr, _)| attr.clone()), - ) - } - - fn allowed_parent_types(&self) -> Arc> { - Arc::clone(&self.allowed_parent_types) - } -} - -impl<'a> cedar_policy_core::ast::RequestSchema for CoreSchema<'a> { - type Error = RequestValidationError; - fn validate_request<'e>( - &self, - request: &cedar_policy_core::ast::Request, - extensions: Extensions<'e>, - ) -> std::result::Result<(), Self::Error> { - use cedar_policy_core::ast::EntityUIDEntry; - // first check that principal and resource are of types that exist in - // the schema, or unspecified. - // we can do this check even if action is unknown. - if let EntityUIDEntry::Concrete(principal) = request.principal() { - match principal.entity_type() { - EntityType::Concrete(name) => { - if !self.schema.entity_types.contains_key(name) { - return Err(RequestValidationError::UndeclaredPrincipalType { - principal_ty: principal.entity_type().clone(), - }); - } - } - EntityType::Unspecified => {} // unspecified principal is allowed, unless we find it is not allowed for this action, which we will check below - } - } - if let EntityUIDEntry::Concrete(resource) = request.resource() { - match resource.entity_type() { - EntityType::Concrete(name) => { - if !self.schema.entity_types.contains_key(name) { - return Err(RequestValidationError::UndeclaredResourceType { - resource_ty: resource.entity_type().clone(), - }); - } - } - EntityType::Unspecified => {} // unspecified resource is allowed, unless we find it is not allowed for this action, which we will check below - } - } - - // the remaining checks require knowing about the action. - match request.action() { - EntityUIDEntry::Concrete(action) => { - let validator_action_id = self.schema.get_action_id(&*action).ok_or_else(|| { - RequestValidationError::UndeclaredAction { - action: Arc::clone(action), - } - })?; - if let EntityUIDEntry::Concrete(principal) = request.principal() { - if !validator_action_id - .applies_to - .is_applicable_principal_type(principal.entity_type()) - { - return Err(RequestValidationError::InvalidPrincipalType { - principal_ty: principal.entity_type().clone(), - action: Arc::clone(action), - }); - } - } - if let EntityUIDEntry::Concrete(resource) = request.resource() { - if !validator_action_id - .applies_to - .is_applicable_resource_type(resource.entity_type()) - { - return Err(RequestValidationError::InvalidResourceType { - resource_ty: resource.entity_type().clone(), - action: Arc::clone(action), - }); - } - } - if let Some(context) = request.context() { - let actual_context_ty = cedar_policy_core::entities::type_of_restricted_expr( - context.as_ref().as_borrowed(), - extensions, - )?; - let expected_context_ty = validator_action_id.context_type(); - if !expected_context_ty.is_consistent_with(&actual_context_ty) { - return Err(RequestValidationError::InvalidContext { - context: context.clone(), - action: Arc::clone(action), - }); - } - } - } - EntityUIDEntry::Unknown => { - // We could hypothetically ensure that the concrete parts of the - // request are valid for _some_ action, but this is probably more - // expensive than we want for this validation step. - // Instead, we just let the above checks (that principal and - // resource are of types that at least _exist_ in the schema) - // suffice. - } - } - Ok(()) - } -} - -#[derive(Debug, Error)] -pub enum RequestValidationError { - /// Request action is not declared in the schema - #[error("request's action `{action}` is not declared in the schema")] - UndeclaredAction { - /// Action which was not declared in the schema - action: Arc, - }, - /// Request principal is of a type not declared in the schema - #[error("principal type `{principal_ty}` is not declared in the schema")] - UndeclaredPrincipalType { - /// Principal type which was not declared in the schema - principal_ty: EntityType, - }, - /// Request resource is of a type not declared in the schema - #[error("resource type `{resource_ty}` is not declared in the schema")] - UndeclaredResourceType { - /// Resource type which was not declared in the schema - resource_ty: EntityType, - }, - /// Request principal is of a type that is declared in the schema, but is - /// not valid for the request action - #[error("principal type `{principal_ty}` is not valid for `{action}`")] - InvalidPrincipalType { - /// Principal type which is not valid - principal_ty: EntityType, - /// Action which it is not valid for - action: Arc, - }, - /// Request resource is of a type that is declared in the schema, but is - /// not valid for the request action - #[error("resource type `{resource_ty}` is not valid for `{action}`")] - InvalidResourceType { - /// Resource type which is not valid - resource_ty: EntityType, - /// Action which it is not valid for - action: Arc, - }, - /// Context does not comply with the shape specified for the request action - #[error("context `{context}` is not valid for `{action}`")] - InvalidContext { - /// Context which is not valid - context: cedar_policy_core::ast::Context, - /// Action which it is not valid for - action: Arc, - }, - /// Error getting the type of the `Context` - #[error("error while computing the type of the request context: {0}")] - TypeOfContext(#[from] cedar_policy_core::entities::TypeOfRestrictedExprError), -} - -/// Struct which carries enough information that it can impl Core's -/// `ContextSchema` INVARIANT: The `Type` stored in this struct must be -/// representable as a `SchemaType` to avoid panicking in `context_type`. -struct ContextSchema(crate::types::Type); - -/// A `Type` contains all the information we need for a Core `ContextSchema`. -impl cedar_policy_core::entities::ContextSchema for ContextSchema { - fn context_type(&self) -> cedar_policy_core::entities::SchemaType { - // PANIC SAFETY: By `ContextSchema` invariant, `self.0` is representable as a schema type. - #[allow(clippy::expect_used)] - self.0 - .clone() - .try_into() - .expect("failed to convert validator type into Core SchemaType") - } -} - /// This trait configures what sort of entity (principals, actions, or resources) /// are returned by the function `get_entities_satisfying_constraint`. pub(crate) trait HeadVar: Copy { diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index c5725594f0..2b8126d77f 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -556,9 +556,9 @@ impl Type { } None => { // attrs has the attribute, self_attrs does not. - // if required in attrs, incompatible. - // otherwise fine - !v.is_required() + // we require all attributes are declared in + // the schema (at least as optional). + false } } }) && self_attrs.iter().all(|(k, v)| { diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index dd84e2023e..77fcad7812 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -3067,7 +3067,7 @@ impl Request { a, r, context.0, - &cedar_policy_validator::CoreSchema::new(&schema.0), + &schema.0, Extensions::all_available(), )?)) } @@ -3339,12 +3339,11 @@ impl Context { schema: &Schema, action: &EntityUid, ) -> Result { - schema - .0 - .get_context_schema(&action.0) - .ok_or_else(|| ContextJsonError::MissingAction { + cedar_policy_validator::context_schema_for_action(&schema.0, &action.0).ok_or_else(|| { + ContextJsonError::MissingAction { action: action.clone(), - }) + } + }) } } From f74a7808506ded555e0d9eacd32cc55b0924578a Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 31 Oct 2023 15:30:28 -0400 Subject: [PATCH 04/11] Update cedar-policy-validator/src/types.rs Co-authored-by: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com> --- cedar-policy-validator/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index 2b8126d77f..91a9b29616 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -556,7 +556,7 @@ impl Type { } None => { // attrs has the attribute, self_attrs does not. - // we require all attributes are declared in + // we require that all attributes are declared in // the schema (at least as optional). false } From 3b50c449a3a115debe61ea4e09945b0093c75fc6 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 31 Oct 2023 21:21:45 +0000 Subject: [PATCH 05/11] remove debug printlns --- cedar-policy-validator/src/coreschema.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index d94fec18ad..e6bb120418 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -200,9 +200,7 @@ impl ast::RequestSchema for ValidatorSchema { context.as_ref().as_borrowed(), extensions, )?; - println!("actual context ty is {actual_context_ty}"); let expected_context_ty = validator_action_id.context_type(); - println!("expected context ty is {expected_context_ty}"); if !expected_context_ty.is_consistent_with(&actual_context_ty) { return Err(RequestValidationError::InvalidContext { context: context.clone(), From 3a6c4763c2762768da526635cad93d38c9659247 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 1 Nov 2023 17:01:18 +0000 Subject: [PATCH 06/11] better solution for typechecking Context against a validator type --- cedar-policy-core/src/ast/restricted_expr.rs | 95 ++++++++++- cedar-policy-validator/src/coreschema.rs | 23 +-- cedar-policy-validator/src/types.rs | 169 ++++++++++++++++--- 3 files changed, 249 insertions(+), 38 deletions(-) diff --git a/cedar-policy-core/src/ast/restricted_expr.rs b/cedar-policy-core/src/ast/restricted_expr.rs index a7c8025ad9..4b3fe1fb62 100644 --- a/cedar-policy-core/src/ast/restricted_expr.rs +++ b/cedar-policy-core/src/ast/restricted_expr.rs @@ -14,7 +14,7 @@ * limitations under the License. */ -use super::{Expr, ExprConstructionError, ExprKind, Literal, Name}; +use super::{EntityUID, Expr, ExprConstructionError, ExprKind, Literal, Name, Type}; use crate::entities::JsonSerializationError; use crate::parser; use crate::parser::err::ParseErrors; @@ -179,6 +179,99 @@ impl<'a> BorrowedRestrictedExpr<'a> { crate::entities::CedarValueJson::from_expr(self)?, )?) } + + /// Get the `bool` value of this `RestrictedExpr` if it's a boolean, or + /// `None` if it is not a boolean + pub fn as_bool(&self) -> Option { + // the only way a `RestrictedExpr` can be a boolean is if it's a literal + match self.expr_kind() { + ExprKind::Lit(Literal::Bool(b)) => Some(*b), + _ => None, + } + } + + /// Get the `i64` value of this `RestrictedExpr` if it's a long, or `None` + /// if it is not a long + pub fn as_long(&self) -> Option { + // the only way a `RestrictedExpr` can be a long is if it's a literal + match self.expr_kind() { + ExprKind::Lit(Literal::Long(i)) => Some(*i), + _ => None, + } + } + + /// Get the `SmolStr` value of this `RestrictedExpr` if it's a string, or + /// `None` if it is not a string + pub fn as_string(&self) -> Option<&SmolStr> { + // the only way a `RestrictedExpr` can be a string is if it's a literal + match self.expr_kind() { + ExprKind::Lit(Literal::String(s)) => Some(s), + _ => None, + } + } + + /// Get the `EntityUID` value of this `RestrictedExpr` if it's an entity + /// reference, or `None` if it is not an entity reference + pub fn as_euid(&self) -> Option<&EntityUID> { + // the only way a `RestrictedExpr` can be an entity reference is if it's + // a literal + match self.expr_kind() { + ExprKind::Lit(Literal::EntityUID(e)) => Some(e), + _ => None, + } + } + + /// Get the name and type annotation of the `Unknown` if this + /// `RestrictedExpr` is an `Unknown`, or `None` if it is not an `Unknown` + pub fn as_unknown(&self) -> Option<(&SmolStr, &Option)> { + match self.expr_kind() { + ExprKind::Unknown { + name, + type_annotation, + } => Some((name, type_annotation)), + _ => None, + } + } + + /// Iterate over the elements of the set if this `RestrictedExpr` is a set, + /// or `None` if it is not a set + pub fn as_set_elements<'s>( + &'s self, + ) -> Option>> { + match self.expr_kind() { + ExprKind::Set(set) => Some(set.iter().map(BorrowedRestrictedExpr::new_unchecked)), // since the RestrictedExpr invariant holds for the input set, it will hold for each element as well + _ => None, + } + } + + /// Iterate over the (key, value) pairs of the record if this + /// `RestrictedExpr` is a record, or `None` if it is not a record + pub fn as_record_pairs<'s>( + &'s self, + ) -> Option)>> { + match self.expr_kind() { + ExprKind::Record(map) => Some( + map.iter() + .map(|(k, v)| (k, BorrowedRestrictedExpr::new_unchecked(v))), + ), // since the RestrictedExpr invariant holds for the input record, it will hold for each attr value as well + _ => None, + } + } + + /// Get the name and args of the called extension function if this + /// `RestrictedExpr` is an extension function call, or `None` if it is not + /// an extension function call + pub fn as_extn_fn_call<'s>( + &'s self, + ) -> Option<(&Name, impl Iterator>)> { + match self.expr_kind() { + ExprKind::ExtensionFunctionApp { fn_name, args } => Some(( + fn_name, + args.iter().map(BorrowedRestrictedExpr::new_unchecked), + )), // since the RestrictedExpr invariant holds for the input call, it will hold for each argument as well + _ => None, + } + } } /// Helper function: does the given `Expr` qualify as a "restricted" expression. diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index e6bb120418..6dec19722b 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -1,5 +1,5 @@ use crate::{ValidatorEntityType, ValidatorSchema}; -use cedar_policy_core::extensions::Extensions; +use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use cedar_policy_core::{ast, entities}; use smol_str::SmolStr; use std::collections::{HashMap, HashSet}; @@ -196,12 +196,10 @@ impl ast::RequestSchema for ValidatorSchema { } } if let Some(context) = request.context() { - let actual_context_ty = entities::type_of_restricted_expr( - context.as_ref().as_borrowed(), - extensions, - )?; let expected_context_ty = validator_action_id.context_type(); - if !expected_context_ty.is_consistent_with(&actual_context_ty) { + if !expected_context_ty + .typecheck_restricted_expr(context.as_ref().as_borrowed(), extensions)? + { return Err(RequestValidationError::InvalidContext { context: context.clone(), action: Arc::clone(action), @@ -279,9 +277,11 @@ pub enum RequestValidationError { /// Action which it is not valid for action: Arc, }, - /// Error getting the type of the `Context` - #[error("error while computing the type of the request context: {0}")] - TypeOfContext(#[from] entities::TypeOfRestrictedExprError), + /// Error looking up an extension function, which may be necessary for + /// `Context`s that contain extension function calls -- not to actually + /// call the extension function, but to get metadata about it + #[error(transparent)] + ExtensionFunctionLookup(#[from] ExtensionFunctionLookupError), } /// Struct which carries enough information that it can impl Core's @@ -796,8 +796,9 @@ mod test { &schema(), Extensions::all_available(), ), - Err(RequestValidationError::TypeOfContext(entities::TypeOfRestrictedExprError::HeterogeneousSet(err))) => { - assert!(err.to_string().contains("set elements have different types: bool and long"), "actual error message was {err}"); + Err(RequestValidationError::InvalidContext { context, action }) => { + assert_eq!(context, context_with_heterogeneous_set); + assert_eq!(&*action, &ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap()); } ); } diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index 91a9b29616..993eed7660 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -20,11 +20,14 @@ use serde::Serialize; use smol_str::SmolStr; use std::{ - collections::{BTreeMap, BTreeSet, HashSet}, + collections::{BTreeMap, BTreeSet, HashMap, HashSet}, fmt::Display, }; -use cedar_policy_core::ast::{EntityType, EntityUID, Expr, ExprShapeOnly, Name}; +use cedar_policy_core::ast::{ + BorrowedRestrictedExpr, EntityType, EntityUID, Expr, ExprShapeOnly, Name, +}; +use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use crate::ValidationMode; @@ -540,40 +543,48 @@ impl Type { CoreSchemaType::Set { element_ty } => { matches!(self, Type::Set { element_type: Some(element_type) } if element_type.is_consistent_with(element_ty)) } - CoreSchemaType::EmptySet => matches!(self, Type::Set { .. }), // empty-set matches a set of any element type - CoreSchemaType::Record { attrs } => match self { + CoreSchemaType::EmptySet => { + // for any given validator Set type, there is some value (namely, the empty set) + // that could have the EmptySet CoreSchemaType and that validator Set type. + matches!(self, Type::Set { .. }) + } + CoreSchemaType::Record { attrs: core_attrs } => match self { Type::EntityOrRecord(kind) => match kind { EntityRecordKind::Record { attrs: self_attrs, .. } => { - attrs.iter().all(|(k, v)| { + core_attrs.iter().all(|(k, core_attr_ty)| { match self_attrs.get_attr(k) { - Some(ty) => { + Some(self_attr_ty) => { // both have the attribute, doesn't matter // if one or both consider it required or // optional - ty.attr_type.is_consistent_with(v.schema_type()) + self_attr_ty + .attr_type + .is_consistent_with(core_attr_ty.schema_type()) } None => { - // attrs has the attribute, self_attrs does not. - // we require that all attributes are declared in - // the schema (at least as optional). - false + // core_attrs has the attribute, self_attrs does not. + // if required in core_attrs, incompatible. + // otherwise fine + !core_attr_ty.is_required() } } - }) && self_attrs.iter().all(|(k, v)| { - match attrs.get(k) { - Some(ty) => { + }) && self_attrs.iter().all(|(k, self_attr_ty)| { + match core_attrs.get(k) { + Some(core_attr_ty) => { // both have the attribute, doesn't matter // if one or both consider it required or // optional - v.attr_type.is_consistent_with(ty.schema_type()) + self_attr_ty + .attr_type + .is_consistent_with(core_attr_ty.schema_type()) } None => { - // self_attrs has the attribute, attrs does not. + // self_attrs has the attribute, core_attrs does not. // if required in self_attrs, incompatible. // otherwise fine - !v.is_required + !self_attr_ty.is_required } } }) @@ -614,6 +625,109 @@ impl Type { } } } + + /// Does the given `BorrowedRestrictedExpr` have this validator type? + pub(crate) fn typecheck_restricted_expr( + &self, + restricted_expr: BorrowedRestrictedExpr<'_>, + extensions: Extensions<'_>, + ) -> Result { + match self { + Type::Never => Ok(false), // no expr has type Never + Type::Primitive { + primitive_type: Primitive::Bool, + } => Ok(restricted_expr.as_bool().is_some()), + Type::Primitive { + primitive_type: Primitive::Long, + } => Ok(restricted_expr.as_long().is_some()), + Type::Primitive { + primitive_type: Primitive::String, + } => Ok(restricted_expr.as_string().is_some()), + Type::True => Ok(restricted_expr.as_bool() == Some(true)), + Type::False => Ok(restricted_expr.as_bool() == Some(false)), + Type::Set { element_type: None } => Ok(restricted_expr.as_set_elements().is_some()), + Type::Set { + element_type: Some(el_type), + } => match restricted_expr.as_set_elements() { + Some(elts) => { + for elt in elts { + if !el_type.typecheck_restricted_expr(elt, extensions)? { + return Ok(false); + } + } + Ok(true) + } + None => Ok(false), + }, + Type::EntityOrRecord(EntityRecordKind::Entity(lub)) => { + match restricted_expr.as_euid() { + Some(euid) => match euid.entity_type() { + EntityType::Concrete(name) => Ok(lub.contains(name)), + EntityType::Unspecified => Ok(false), // Unspecified can only have the validator type `AnyEntity`, not any specific lub + }, + None => Ok(false), + } + } + Type::EntityOrRecord(EntityRecordKind::ActionEntity { name, .. }) => { + match restricted_expr.as_euid() { + Some(euid) if euid.is_action() => match euid.entity_type() { + EntityType::Concrete(euid_name) => Ok(euid_name == name), + EntityType::Unspecified => Ok(false), // `euid.is_action()` should never be true for an Unspecified, so this should be unreachable, but it's safe to return Ok(false), because Unspecified can only have the validator type `AnyEntity` anyway + }, + _ => Ok(false), + } + } + Type::EntityOrRecord(EntityRecordKind::AnyEntity) => { + Ok(restricted_expr.as_euid().is_some()) + } + Type::EntityOrRecord(EntityRecordKind::Record { + attrs, + open_attributes, + }) => match restricted_expr.as_record_pairs() { + Some(pairs) => { + let record: HashMap<_, BorrowedRestrictedExpr<'_>> = pairs.collect(); + for (k, attr_val) in &record { + match attrs.get_attr(&k) { + Some(attr_ty) => { + if !attr_ty + .attr_type + .typecheck_restricted_expr(attr_val.to_owned(), extensions)? + { + return Ok(false); + } + } + None => { + if open_attributes != &OpenTag::OpenAttributes { + // the restricted expr has an attribute not + // listed in the Type, and the Type doesn't + // have open attributes + return Ok(false); + } + } + } + } + // we've now checked that all of the attrs in `restricted_expr` are OK and have the right types. + // what remains is making sure that all the required attrs are actually in `restricted_expr` + for (k, attr_ty) in attrs.iter() { + if attr_ty.is_required && !record.contains_key(k) { + return Ok(false); + } + } + Ok(true) + } + None => Ok(false), + }, + Type::ExtensionType { name } => match restricted_expr.as_extn_fn_call() { + Some((fn_name, _)) => match extensions.func(fn_name)?.return_type() { + Some(cedar_policy_core::entities::SchemaType::Extension { + name: actual_name, + }) => Ok(actual_name == name), + _ => Ok(false), + }, + None => Ok(false), + }, + } + } } impl Display for Type { @@ -632,7 +746,7 @@ impl TryFrom for cedar_policy_core::entities::SchemaType { use cedar_policy_core::entities::AttributeType as CoreAttributeType; use cedar_policy_core::entities::SchemaType as CoreSchemaType; match ty { - Type::Never => Err("'Never' type is not representable in core::Type".into()), + Type::Never => Err("'Never' type is not representable in core::SchemaType".into()), Type::True | Type::False => Ok(CoreSchemaType::Bool), Type::Primitive { primitive_type: Primitive::Bool, @@ -648,12 +762,9 @@ impl TryFrom for cedar_policy_core::entities::SchemaType { } => Ok(CoreSchemaType::Set { element_ty: Box::new(CoreSchemaType::try_from(*element_type)?), }), - Type::Set { element_type: None } => { - Err("Set type is not representable in core::SchemaType".into()) - } + Type::Set { element_type: None } => Ok(CoreSchemaType::EmptySet), Type::EntityOrRecord(kind @ EntityRecordKind::AnyEntity) => Err(format!( - "any-entity type is not representable in core::Type: {:?}", - kind + "any-entity type is not representable in core::SchemaType: {kind:?}" )), Type::EntityOrRecord(EntityRecordKind::ActionEntity { name, .. }) => { Ok(CoreSchemaType::Entity { @@ -683,9 +794,9 @@ impl TryFrom for cedar_policy_core::entities::SchemaType { Some(name) => Ok(CoreSchemaType::Entity { ty: EntityType::Concrete(name), }), - None => { - Err("non-singleton LUB type is not representable in core::Type".to_string()) - } + None => Err( + "non-singleton LUB type is not representable in core::SchemaType".to_string(), + ), }, Type::ExtensionType { name } => Ok(CoreSchemaType::Extension { name }), } @@ -803,6 +914,12 @@ impl EntityLUB { self.lub_elements.is_disjoint(&other.lub_elements) } + /// Return true if the given entity type `Name` is in the set of entity + /// types comprising this LUB. + pub(crate) fn contains(&self, ty: &Name) -> bool { + self.lub_elements.contains(ty) + } + /// An iterator over the entity type `Name`s in the set of entity types /// comprising this LUB. pub(crate) fn iter(&self) -> impl Iterator { From d700c6e8a7e5b7f7ff497ba37e0771f126e65bed Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 6 Nov 2023 14:50:09 +0000 Subject: [PATCH 07/11] move to one-constructor approach; thread request validation through CLI --- .../sandbox_a/schema.cedarschema.json | 4 +- cedar-integration-tests/tests/multi/5.json | 5 +- cedar-policy-cli/src/lib.rs | 37 ++++- .../tests/integration_tests/main.rs | 34 ++++- cedar-policy-cli/tests/sample.rs | 4 + cedar-policy-core/src/ast/request.rs | 77 +++++----- cedar-policy-core/src/authorizer.rs | 49 ++++-- cedar-policy-core/src/evaluator.rs | 55 ++++++- cedar-policy-validator/src/coreschema.rs | 88 +++++------ cedar-policy/CHANGELOG.md | 2 +- cedar-policy/README.md | 4 +- cedar-policy/benches/cedar_benchmarks.rs | 6 +- cedar-policy/src/api.rs | 140 +++++++----------- cedar-policy/src/frontend/is_authorized.rs | 14 +- cedar-policy/src/integration_testing.rs | 26 +++- cedar-policy/tests/public_interface.rs | 35 ++++- 16 files changed, 367 insertions(+), 213 deletions(-) diff --git a/cedar-integration-tests/sample-data/sandbox_a/schema.cedarschema.json b/cedar-integration-tests/sample-data/sandbox_a/schema.cedarschema.json index 172c08b13b..2532fc4b0f 100644 --- a/cedar-integration-tests/sample-data/sandbox_a/schema.cedarschema.json +++ b/cedar-integration-tests/sample-data/sandbox_a/schema.cedarschema.json @@ -76,10 +76,10 @@ "view": { "appliesTo": { "resourceTypes": [ - "Photo" + "Photo", "Video" ], "principalTypes": [ - "User" + "User", "Administrator" ], "context": { "type": "Record", diff --git a/cedar-integration-tests/tests/multi/5.json b/cedar-integration-tests/tests/multi/5.json index 3a7dd2f04f..2339bb135c 100644 --- a/cedar-integration-tests/tests/multi/5.json +++ b/cedar-integration-tests/tests/multi/5.json @@ -41,6 +41,7 @@ "context": { "authenticated": true }, + "enable_request_validation": false, "decision": "Deny", "reasons": [], "errors": [ @@ -83,6 +84,7 @@ "context": { "authenticated": false }, + "enable_request_validation": false, "decision": "Deny", "reasons": [ "policy1" @@ -128,6 +130,7 @@ "context": { "authenticated": true }, + "enable_request_validation": false, "decision": "Allow", "reasons": [ "policy2" @@ -135,4 +138,4 @@ "errors": [] } ] -} \ No newline at end of file +} diff --git a/cedar-policy-cli/src/lib.rs b/cedar-policy-cli/src/lib.rs index 015a9636e0..7babe14f95 100644 --- a/cedar-policy-cli/src/lib.rs +++ b/cedar-policy-cli/src/lib.rs @@ -21,7 +21,7 @@ mod err; -use clap::{Args, Parser, Subcommand, ValueEnum}; +use clap::{ArgAction, Args, Parser, Subcommand, ValueEnum}; use miette::{miette, IntoDiagnostic, NamedSource, Report, Result, WrapErr}; use serde::{Deserialize, Serialize}; use std::{ @@ -139,10 +139,19 @@ pub struct RequestArgs { /// --principal, --action, etc. #[arg(long = "request-json", value_name = "FILE", conflicts_with_all = &["principal", "action", "resource", "context_json_file"])] pub request_json_file: Option, + /// Whether to enable request validation. This has no effect if a schema is + /// not provided. + #[arg(long = "request-validation", action = ArgAction::Set, default_value_t = true)] + pub request_validation: bool, } impl RequestArgs { /// Turn this `RequestArgs` into the appropriate `Request` object + /// + /// `schema` will be used for schema-based parsing of the context, and also + /// (if `self.request_validation` is `true`) for request validation. + /// + /// `self.request_validation` has no effect if `schema` is `None`. fn get_request(&self, schema: Option<&Schema>) -> Result { match &self.request_json_file { Some(jsonfile) => { @@ -182,7 +191,18 @@ impl RequestArgs { ) .into_diagnostic() .wrap_err_with(|| format!("failed to create a context from {jsonfile}"))?; - Ok(Request::new(principal, action, resource, context)) + Request::new( + principal, + action, + resource, + context, + if self.request_validation { + schema + } else { + None + }, + ) + .map_err(|e| miette!("{e}")) } None => { let principal = self @@ -224,7 +244,18 @@ impl RequestArgs { })?, }, }; - Ok(Request::new(principal, action, resource, context)) + Request::new( + principal, + action, + resource, + context, + if self.request_validation { + schema + } else { + None + }, + ) + .map_err(|e| miette!("{e}")) } } } diff --git a/cedar-policy-cli/tests/integration_tests/main.rs b/cedar-policy-cli/tests/integration_tests/main.rs index 1344ebaf0e..2802c52725 100644 --- a/cedar-policy-cli/tests/integration_tests/main.rs +++ b/cedar-policy-cli/tests/integration_tests/main.rs @@ -71,6 +71,9 @@ struct JsonRequest { resource: Option, /// Context for the request context: serde_json::Value, + /// Whether to enable request validation for this request + #[serde(default = "constant_true")] + enable_request_validation: bool, /// Expected decision for the request decision: Decision, /// Expected "reasons" for the request @@ -79,6 +82,10 @@ struct JsonRequest { errors: Vec, } +fn constant_true() -> bool { + true +} + fn value_to_euid_string(v: serde_json::Value) -> Result { EntityUid::from_json(v).map(|euid| euid.to_string()) } @@ -186,6 +193,9 @@ fn perform_integration_test_from_json(jsonfile: impl AsRef) { entity_args.push("--action".to_string()); entity_args.push(value_to_euid_string(s).unwrap()); } + if !json_request.enable_request_validation { + entity_args.push("--request-validation=false".to_string()); + } let authorize_cmd = assert_cmd::Command::cargo_bin("cedar") .expect("bin exists") @@ -212,15 +222,33 @@ fn perform_integration_test_from_json(jsonfile: impl AsRef) { .expect("output should be valid UTF-8"); for error in &json_request.errors { - assert!(output.contains(error)); + assert!( + output.contains(error), + "test {} failed for request \"{}\": output does not contain expected error {error:?}.\noutput was: {output}\nstderr was: {}", + jsonfile.display(), + &json_request.desc, + String::from_utf8(authorize_cmd.get_output().stderr.clone()).expect("stderr should be valid UTF-8"), + ); } if json_request.reasons.is_empty() { - assert!(output.contains("no policies applied to this request")); + assert!( + output.contains("no policies applied to this request"), + "test {} failed for request \"{}\": output does not contain the string \"no policies applied to this request\", as expected.\noutput was: {output}\nstderr was: {}", + jsonfile.display(), + &json_request.desc, + String::from_utf8(authorize_cmd.get_output().stderr.clone()).expect("stderr should be valid UTF-8"), + ); } else { assert!(output.contains("this decision was due to the following policies")); for reason in &json_request.reasons { - assert!(output.contains(&reason.escape_debug().to_string())); + assert!( + output.contains(&reason.escape_debug().to_string()), + "test {} failed for request \"{}\": output does not contain the reason string {reason:?}.\noutput was: {output}\nstderr was: {}", + jsonfile.display(), + &json_request.desc, + String::from_utf8(authorize_cmd.get_output().stderr.clone()).expect("stderr should be valid UTF-8"), + ); } }; } diff --git a/cedar-policy-cli/tests/sample.rs b/cedar-policy-cli/tests/sample.rs index dd6082d563..eca58b7ef5 100644 --- a/cedar-policy-cli/tests/sample.rs +++ b/cedar-policy-cli/tests/sample.rs @@ -71,6 +71,7 @@ fn run_authorize_test_with_linked_policies( resource: Some(resource.into()), context_json_file: None, request_json_file: None, + request_validation: true, }, policies_file: policies_file.into(), template_linked_file: links_file.map(|x| x.to_string()), @@ -134,6 +135,7 @@ fn run_authorize_test_context( resource: Some(resource.into()), context_json_file: Some(context_file.into()), request_json_file: None, + request_validation: true, }, policies_file: policies_file.into(), template_linked_file: None, @@ -159,6 +161,7 @@ fn run_authorize_test_json( resource: None, context_json_file: None, request_json_file: Some(request_json.into()), + request_validation: true, }, policies_file: policies_file.into(), template_linked_file: None, @@ -541,6 +544,7 @@ fn run_evaluate_test( resource: None, context_json_file: None, request_json_file: Some(request_json_file.into()), + request_validation: true, }, expression: expression.to_owned(), }; diff --git a/cedar-policy-core/src/ast/request.rs b/cedar-policy-core/src/ast/request.rs index 474df23001..a6e46eb37c 100644 --- a/cedar-policy-core/src/ast/request.rs +++ b/cedar-policy-core/src/ast/request.rs @@ -79,63 +79,52 @@ impl EntityUIDEntry { } impl Request { - /// Default constructor - pub fn new( + /// Default constructor. + /// + /// If `schema` is provided, this constructor validates that this `Request` + /// complies with the given `schema`. + pub fn new( principal: EntityUID, action: EntityUID, resource: EntityUID, context: Context, - ) -> Self { - Self { + schema: Option<&S>, + extensions: Extensions<'_>, + ) -> Result { + let req = Self { principal: EntityUIDEntry::concrete(principal), action: EntityUIDEntry::concrete(action), resource: EntityUIDEntry::concrete(resource), context: Some(context), + }; + if let Some(schema) = schema { + schema.validate_request(&req, extensions)?; } + Ok(req) } - /// Create a new `Request` with potentially unknown (for partial eval) variables - pub fn new_with_unknowns( + /// Create a new `Request` with potentially unknown (for partial eval) variables. + /// + /// If `schema` is provided, this constructor validates that this `Request` + /// complies with the given `schema` (at least to the extent that we can + /// validate with the given information) + pub fn new_with_unknowns( principal: EntityUIDEntry, action: EntityUIDEntry, resource: EntityUIDEntry, context: Option, - ) -> Self { - Self { + schema: Option<&S>, + extensions: Extensions<'_>, + ) -> Result { + let req = Self { principal, action, resource, context, + }; + if let Some(schema) = schema { + schema.validate_request(&req, extensions)?; } - } - - /// Create a new `Request` and validate that it complies with the given `schema` - pub fn new_with_validation( - principal: EntityUID, - action: EntityUID, - resource: EntityUID, - context: Context, - schema: &S, - extensions: Extensions<'_>, - ) -> Result { - let req = Self::new(principal, action, resource, context); - schema.validate_request(&req, extensions)?; - Ok(req) - } - - /// Create a new `Request` with potentially unknown (for partial eval) - /// variables, and validate that it complies with the given `schema` (at - /// least to the extent that we can validate with the given information) - pub fn new_with_unknowns_and_validation( - principal: EntityUIDEntry, - action: EntityUIDEntry, - resource: EntityUIDEntry, - context: Option, - schema: &S, - extensions: Extensions<'_>, - ) -> Result { - let req = Self::new_with_unknowns(principal, action, resource, context); - schema.validate_request(&req, extensions)?; Ok(req) } @@ -304,6 +293,20 @@ pub trait RequestSchema { ) -> Result<(), Self::Error>; } +/// A `RequestSchema` that does no validation and always reports a passing result +#[derive(Debug, Clone)] +pub struct RequestSchemaAllPass; +impl RequestSchema for RequestSchemaAllPass { + type Error = std::convert::Infallible; + fn validate_request<'a>( + &self, + _request: &Request, + _extensions: Extensions<'a>, + ) -> Result<(), Self::Error> { + Ok(()) + } +} + #[cfg(test)] mod test { diff --git a/cedar-policy-core/src/authorizer.rs b/cedar-policy-core/src/authorizer.rs index 4539aa41b6..dd5fa6b9d2 100644 --- a/cedar-policy-core/src/authorizer.rs +++ b/cedar-policy-core/src/authorizer.rs @@ -399,9 +399,9 @@ impl std::fmt::Debug for Authorizer { mod test { use std::collections::BTreeMap; - use crate::parser; - use super::*; + use crate::ast::RequestSchemaAllPass; + use crate::parser; /// Sanity unit test case for is_authorized. /// More robust testing is accomplished through the integration tests. @@ -413,7 +413,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let pset = PolicySet::new(); let entities = Entities::new(); let ans = a.is_authorized(&q, &pset, &entities); @@ -429,7 +432,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let mut pset = PolicySet::new(); let entities = Entities::new(); @@ -509,7 +515,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let mut pset = PolicySet::new(); pset.add_static(true_policy("0", Effect::Permit)) .expect("Policy ID already in PolicySet"); @@ -534,7 +543,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), context, - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let mut pset = PolicySet::new(); pset.add_static(true_policy("0", Effect::Permit)) .expect("Policy ID already in PolicySet"); @@ -569,7 +581,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let mut pset = PolicySet::new(); pset.add_static(true_policy("0", Effect::Permit)) .expect("Policy ID already in PolicySet"); @@ -587,7 +602,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let a = Authorizer::new(); let mut pset = PolicySet::new(); let es = Entities::new(); @@ -628,7 +646,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let a = Authorizer::new(); let mut pset = PolicySet::new(); let es = Entities::new(); @@ -690,7 +711,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let a = Authorizer::new(); let mut pset = PolicySet::new(); let es = Entities::new(); @@ -738,7 +762,10 @@ mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + None::<&RequestSchemaAllPass>, + Extensions::none(), + ) + .unwrap(); let a = Authorizer::new(); let mut pset = PolicySet::new(); let es = Entities::new(); diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index ed2a34cf7b..88251703a3 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -867,7 +867,10 @@ pub mod test { ), ]) .unwrap(), + Some(&RequestSchemaAllPass), + Extensions::none(), ) + .unwrap() } // Many of these tests use this basic `Entities` @@ -3042,7 +3045,10 @@ pub mod test { EntityUID::with_eid("test_action"), EntityUID::with_eid("test_resource"), Context::empty(), - ); + Some(&RequestSchemaAllPass), + Extensions::none(), + ) + .unwrap(); //Alice has parent "Friends" but we don't add "Friends" to the slice let mut alice = Entity::with_uid(EntityUID::with_eid("Alice")); let parent = Entity::with_uid(EntityUID::with_eid("Friends")); @@ -3725,7 +3731,10 @@ pub mod test { EntityUID::with_eid("a"), EntityUID::with_eid("r"), Context::empty(), - ); + Some(&RequestSchemaAllPass), + Extensions::none(), + ) + .unwrap(); let eparser: EntityJsonParser<'_, '_> = EntityJsonParser::new(None, Extensions::none(), TCComputation::ComputeNow); let entities = eparser.from_json_str("[]").expect("empty slice"); @@ -3933,7 +3942,10 @@ pub mod test { EntityUIDEntry::Unknown, EntityUIDEntry::Unknown, Some(Context::empty()), - ); + Some(&RequestSchemaAllPass), + Extensions::none(), + ) + .unwrap(); let es = Entities::new(); let exts = Extensions::none(); let e = Evaluator::new(&q, &es, &exts).expect("failed to create evaluator"); @@ -3960,7 +3972,15 @@ pub mod test { let rexpr = RestrictedExpr::new(context_expr) .expect("Context Expression was not a restricted expression"); let context = Context::from_expr(rexpr).unwrap(); - let q = Request::new(euid.clone(), euid.clone(), euid, context); + let q = Request::new( + euid.clone(), + euid.clone(), + euid, + context, + Some(&RequestSchemaAllPass), + Extensions::none(), + ) + .unwrap(); let es = Entities::new(); let exts = Extensions::none(); let eval = Evaluator::new(&q, &es, &exts).expect("Failed to instantiate evaluator"); @@ -4092,7 +4112,15 @@ pub mod test { )) .unwrap(); let euid: EntityUID = r#"Test::"test""#.parse().unwrap(); - let q = Request::new(euid.clone(), euid.clone(), euid, context); + let q = Request::new( + euid.clone(), + euid.clone(), + euid, + context, + Some(&RequestSchemaAllPass), + Extensions::none(), + ) + .unwrap(); let es = Entities::new(); let exts = Extensions::none(); let eval = Evaluator::new(&q, &es, &exts).expect("Failed to instantiate evaluator"); @@ -4130,7 +4158,15 @@ pub mod test { .expect("should qualify as restricted"); let context = Context::from_expr(c_expr).unwrap(); - let q = Request::new(p, a, r, context); + let q = Request::new( + p, + a, + r, + context, + Some(&RequestSchemaAllPass), + Extensions::none(), + ) + .unwrap(); let exts = Extensions::none(); let eval = Evaluator::new(&q, &es, &exts).expect("Could not create evaluator"); @@ -4148,7 +4184,7 @@ pub mod test { let a: EntityUID = r#"a::"Action""#.parse().unwrap(); let r: EntityUID = r#"r::"Resource""#.parse().unwrap(); let c = Context::empty(); - Request::new(p, a, r, c) + Request::new(p, a, r, c, Some(&RequestSchemaAllPass), Extensions::none()).unwrap() } #[test] @@ -4202,7 +4238,10 @@ pub mod test { Expr::record([("condition".into(), Expr::unknown("unknown_condition"))]).unwrap(), )) .unwrap(), - ); + Some(&RequestSchemaAllPass), + Extensions::none(), + ) + .unwrap(); let eval = Evaluator::new(&q, &es, &exts).unwrap(); let r = eval.partial_interpret(&e, &HashMap::new()).unwrap(); diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index 6dec19722b..b34c0a14cc 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -375,12 +375,12 @@ mod test { #[test] fn success_concrete_request_no_context() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -391,7 +391,7 @@ mod test { #[test] fn success_concrete_request_with_context() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), @@ -400,7 +400,7 @@ mod test { ast::RestrictedExpr::val(true) )]) .unwrap(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -411,7 +411,7 @@ mod test { #[test] fn success_principal_unknown() { assert_matches!( - ast::Request::new_with_unknowns_and_validation( + ast::Request::new_with_unknowns( ast::EntityUIDEntry::Unknown, ast::EntityUIDEntry::concrete( ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap() @@ -420,7 +420,7 @@ mod test { ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap() ), Some(ast::Context::empty()), - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -431,7 +431,7 @@ mod test { #[test] fn success_action_unknown() { assert_matches!( - ast::Request::new_with_unknowns_and_validation( + ast::Request::new_with_unknowns( ast::EntityUIDEntry::concrete( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap() ), @@ -440,7 +440,7 @@ mod test { ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap() ), Some(ast::Context::empty()), - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -451,7 +451,7 @@ mod test { #[test] fn success_resource_unknown() { assert_matches!( - ast::Request::new_with_unknowns_and_validation( + ast::Request::new_with_unknowns( ast::EntityUIDEntry::concrete( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap() ), @@ -460,7 +460,7 @@ mod test { ), ast::EntityUIDEntry::Unknown, Some(ast::Context::empty()), - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -471,7 +471,7 @@ mod test { #[test] fn success_context_unknown() { assert_matches!( - ast::Request::new_with_unknowns_and_validation( + ast::Request::new_with_unknowns( ast::EntityUIDEntry::concrete( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap() ), @@ -482,7 +482,7 @@ mod test { ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap() ), None, - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -493,12 +493,12 @@ mod test { #[test] fn success_everything_unspecified() { assert_matches!( - ast::Request::new_with_unknowns_and_validation( + ast::Request::new_with_unknowns( ast::EntityUIDEntry::Unknown, ast::EntityUIDEntry::Unknown, ast::EntityUIDEntry::Unknown, None, - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -511,7 +511,7 @@ mod test { #[test] fn success_unknown_action_but_invalid_types() { assert_matches!( - ast::Request::new_with_unknowns_and_validation( + ast::Request::new_with_unknowns( ast::EntityUIDEntry::concrete( ast::EntityUID::with_eid_and_type("Album", "abc123").unwrap() ), @@ -520,7 +520,7 @@ mod test { ast::EntityUID::with_eid_and_type("User", "alice").unwrap() ), None, - &schema(), + Some(&schema()), Extensions::all_available(), ), Ok(_) @@ -531,12 +531,12 @@ mod test { #[test] fn action_not_declared() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "destroy").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::UndeclaredAction { action }) => { @@ -549,12 +549,12 @@ mod test { #[test] fn action_unspecified() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::unspecified_from_eid(ast::Eid::new("blahblah")), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::UndeclaredAction { action }) => { @@ -567,12 +567,12 @@ mod test { #[test] fn principal_type_not_declared() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("Foo", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::UndeclaredPrincipalType { principal_ty }) => { @@ -585,12 +585,12 @@ mod test { #[test] fn principal_type_not_declared_action_unspecified() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("Foo", "abc123").unwrap(), ast::EntityUID::unspecified_from_eid(ast::Eid::new("blahblah")), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::UndeclaredPrincipalType { principal_ty }) => { @@ -603,12 +603,12 @@ mod test { #[test] fn principal_unspecified() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::unspecified_from_eid(ast::Eid::new("principal")), ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidPrincipalType { principal_ty, .. }) => { @@ -621,12 +621,12 @@ mod test { #[test] fn resource_type_not_declared() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), ast::EntityUID::with_eid_and_type("Foo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::UndeclaredResourceType { resource_ty }) => { @@ -639,12 +639,12 @@ mod test { #[test] fn resource_type_not_declared_action_unspecified() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::unspecified_from_eid(ast::Eid::new("blahblah")), ast::EntityUID::with_eid_and_type("Foo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::UndeclaredResourceType { resource_ty }) => { @@ -657,12 +657,12 @@ mod test { #[test] fn resource_unspecified() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), ast::EntityUID::unspecified_from_eid(ast::Eid::new("resource")), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidResourceType { resource_ty, .. }) => { @@ -675,12 +675,12 @@ mod test { #[test] fn principal_type_invalid() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("Album", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidPrincipalType { principal_ty, action }) => { @@ -694,12 +694,12 @@ mod test { #[test] fn resource_type_invalid() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "view_photo").unwrap(), ast::EntityUID::with_eid_and_type("Group", "coders").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidResourceType { resource_ty, action }) => { @@ -713,12 +713,12 @@ mod test { #[test] fn context_missing_attribute() { assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), ast::Context::empty(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidContext { context, action }) => { @@ -737,12 +737,12 @@ mod test { ]) .unwrap(); assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), context_with_extra_attr.clone(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidContext { context, action }) => { @@ -761,12 +761,12 @@ mod test { )]) .unwrap(); assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), context_with_wrong_type_attr.clone(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidContext { context, action }) => { @@ -788,12 +788,12 @@ mod test { )]) .unwrap(); assert_matches!( - ast::Request::new_with_validation( + ast::Request::new( ast::EntityUID::with_eid_and_type("User", "abc123").unwrap(), ast::EntityUID::with_eid_and_type("Action", "edit_photo").unwrap(), ast::EntityUID::with_eid_and_type("Photo", "vacationphoto94.jpg").unwrap(), context_with_heterogeneous_set.clone(), - &schema(), + Some(&schema()), Extensions::all_available(), ), Err(RequestValidationError::InvalidContext { context, action }) => { diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 9e7e5bfb6d..b5f227754a 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`. -- `Request::new_with_validation` to construct a `Request` while validating it against a `Schema`. ### Changed @@ -37,6 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `Entities::from_*()` methods now validate the entities against the `schema`, if a `schema` is provided. - `Entities::from_entities()` and `Entities::add_entities()` now take an optional schema argument. +- `Request::new()` now takes an optional schema argument, and returns a `Result`. - Change the semantics of equality for IP ranges. For example, `ip("192.168.0.1/24") == ip("192.168.0.3/24")` was previously `true` and is now `false`. The behavior of equality on single IP addresses is unchanged, and so is diff --git a/cedar-policy/README.md b/cedar-policy/README.md index 178ad5c1f7..2bb3c1cdd9 100644 --- a/cedar-policy/README.md +++ b/cedar-policy/README.md @@ -38,7 +38,7 @@ permit(principal == User::"alice", action == Action::"view", resource == File::" let action = r#"Action::"view""#.parse().unwrap(); let alice = r#"User::"alice""#.parse().unwrap(); let file = r#"File::"93""#.parse().unwrap(); - let request = Request::new(Some(alice), Some(action), Some(file), Context::empty()); + let request = Request::new(Some(alice), Some(action), Some(file), Context::empty(), None).unwrap(); let entities = Entities::empty(); let authorizer = Authorizer::new(); @@ -50,7 +50,7 @@ permit(principal == User::"alice", action == Action::"view", resource == File::" let action = r#"Action::"view""#.parse().unwrap(); let bob = r#"User::"bob""#.parse().unwrap(); let file = r#"File::"93""#.parse().unwrap(); - let request = Request::new(Some(bob), Some(action), Some(file), Context::empty()); + let request = Request::new(Some(bob), Some(action), Some(file), Context::empty(), None).unwrap(); let answer = authorizer.is_authorized(&request, &policy, &entities); diff --git a/cedar-policy/benches/cedar_benchmarks.rs b/cedar-policy/benches/cedar_benchmarks.rs index 5c0a6214dc..24669df1f7 100644 --- a/cedar-policy/benches/cedar_benchmarks.rs +++ b/cedar-policy/benches/cedar_benchmarks.rs @@ -122,7 +122,9 @@ pub fn criterion_benchmark(c: &mut Criterion) { Some(action.clone()), Some(resource.clone()), Context::from_pairs(context.clone()).expect("no duplicate keys in this context"), - ); + None, + ) + .unwrap(); c.bench_function("request_new", |b| { b.iter(|| { @@ -134,7 +136,9 @@ pub fn criterion_benchmark(c: &mut Criterion) { Context::from_pairs(context.clone()) .expect("no duplicate keys in this context"), ), + None, ) + .unwrap() }) }); diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 77fcad7812..75f76e1c73 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -620,7 +620,7 @@ impl Authorizer { /// # /// # let c = Context::empty(); /// # - /// # let request: Request = Request::new(Some(p), Some(a), Some(r), c); + /// # let request: Request = Request::new(Some(p), Some(a), Some(r), c, None).unwrap(); /// # /// # // create a policy /// # let s = r#"permit( @@ -676,7 +676,7 @@ impl Authorizer { /// /// let c = Context::empty(); /// - /// let request: Request = Request::new(Some(p), Some(a), Some(r), c); + /// let request: Request = Request::new(Some(p), Some(a), Some(r), c, None).unwrap(); /// /// // create a policy /// let s = r#" @@ -802,7 +802,7 @@ impl Diagnostics { /// # /// # let c = Context::empty(); /// # - /// # let request: Request = Request::new(Some(p), Some(a), Some(r), c); + /// # let request: Request = Request::new(Some(p), Some(a), Some(r), c, None).unwrap(); /// # /// # // create a policy /// # let s = r#"permit( @@ -861,7 +861,7 @@ impl Diagnostics { /// # /// # let c = Context::empty(); /// # - /// # let request: Request = Request::new(Some(p), Some(a), Some(r), c); + /// # let request: Request = Request::new(Some(p), Some(a), Some(r), c, None).unwrap(); /// # /// # // create a policy /// # let s = r#"permit( @@ -2973,22 +2973,14 @@ impl<'a> RequestBuilder<'a> { /// Create the [`Request`] pub fn build(self) -> Result { - match self.schema { - None => Ok(Request(ast::Request::new_with_unknowns( - self.principal, - self.action, - self.resource, - self.context, - ))), - Some(schema) => Ok(Request(ast::Request::new_with_unknowns_and_validation( - self.principal, - self.action, - self.resource, - self.context, - &cedar_policy_validator::CoreSchema::new(&schema.0), - Extensions::all_available(), - )?)), - } + Ok(Request(ast::Request::new_with_unknowns( + self.principal, + self.action, + self.resource, + self.context, + self.schema.map(|schema| &schema.0), + Extensions::all_available(), + )?)) } } @@ -3013,42 +3005,15 @@ impl Request { /// decisions (e.g., because they are not used in your policies). /// If any of the fields are `None`, we will automatically generate /// a unique entity UID that is not equal to any UID in the store. - pub fn new( - principal: Option, - action: Option, - resource: Option, - context: Context, - ) -> Self { - let p = match principal { - Some(p) => p.0, - None => ast::EntityUID::unspecified_from_eid(ast::Eid::new("principal")), - }; - let a = match action { - Some(a) => a.0, - None => ast::EntityUID::unspecified_from_eid(ast::Eid::new("action")), - }; - let r = match resource { - Some(r) => r.0, - None => ast::EntityUID::unspecified_from_eid(ast::Eid::new("resource")), - }; - Self(ast::Request::new(p, a, r, context.0)) - } - - /// Create a Request, validating that it complies with the given `Schema`. /// - /// Note that you can create the `EntityUid`s using `.parse()` on any - /// string (via the `FromStr` implementation for `EntityUid`). - /// The principal, action, and resource fields are optional to support - /// the case where these fields do not contribute to authorization - /// decisions (e.g., because they are not used in your policies). - /// If any of the fields are `None`, we will automatically generate - /// a unique entity UID that is not equal to any UID in the store. - pub fn new_with_validation( + /// If `schema` is present, this constructor will validate that the + /// `Request` complies with the given `schema`. + pub fn new( principal: Option, action: Option, resource: Option, context: Context, - schema: &Schema, + schema: Option<&Schema>, ) -> Result { let p = match principal { Some(p) => p.0, @@ -3062,12 +3027,12 @@ impl Request { Some(r) => r.0, None => ast::EntityUID::unspecified_from_eid(ast::Eid::new("resource")), }; - Ok(Self(ast::Request::new_with_validation( + Ok(Self(ast::Request::new( p, a, r, context.0, - &schema.0, + schema.map(|schema| &schema.0), Extensions::all_available(), )?)) } @@ -3124,8 +3089,7 @@ impl Context { /// Create an empty `Context` /// ``` /// use cedar_policy::Context; - /// let c = Context::empty(); - /// // let request: Request = Request::new(Some(principal), Some(action), Some(resource), c); + /// let context = Context::empty(); /// ``` pub fn empty() -> Self { Self(ast::Context::empty()) @@ -3136,7 +3100,7 @@ impl Context { /// of `(key, restricted expression)` pairs. /// ``` /// use cedar_policy::{Context, RestrictedExpression}; - /// use std::collections::HashMap; + /// # use std::collections::HashMap; /// use std::str::FromStr; /// # use cedar_policy::{Entities, EntityId, EntityTypeName, EntityUid, Request,PolicySet}; /// let data : serde_json::Value = serde_json::json!({ @@ -3163,7 +3127,7 @@ impl Context { /// # let r_eid = EntityId::from_str("trip").unwrap(); /// # let r_name: EntityTypeName = EntityTypeName::from_str("Album").unwrap(); /// # let r = EntityUid::from_type_name_and_id(r_name, r_eid); - /// let request: Request = Request::new(Some(p), Some(a), Some(r), context); + /// # let request: Request = Request::new(Some(p), Some(a), Some(r), context, None).unwrap(); /// ``` pub fn from_pairs( pairs: impl IntoIterator, @@ -3185,7 +3149,7 @@ impl Context { /// must specify the `Action` for schema-based parsing. /// ``` /// use cedar_policy::{Context, RestrictedExpression}; - /// use std::collections::HashMap; + /// # use std::collections::HashMap; /// use std::str::FromStr; /// # use cedar_policy::{Entities, EntityId, EntityTypeName, EntityUid, Request,PolicySet}; /// let data =r#"{ @@ -3209,7 +3173,7 @@ impl Context { /// # let r_eid = EntityId::from_str("trip").unwrap(); /// # let r_name: EntityTypeName = EntityTypeName::from_str("Album").unwrap(); /// # let r = EntityUid::from_type_name_and_id(r_name, r_eid); - /// let request: Request = Request::new(Some(p), Some(a), Some(r), context); + /// # let request: Request = Request::new(Some(p), Some(a), Some(r), context, None).unwrap(); /// ``` pub fn from_json_str( json: &str, @@ -3236,33 +3200,36 @@ impl Context { /// must specify the `Action` for schema-based parsing. /// ``` /// use cedar_policy::{Context, RestrictedExpression, Schema}; - /// use std::collections::HashMap; + /// # use std::collections::HashMap; /// use std::str::FromStr; /// # use cedar_policy::{Entities, EntityId, EntityTypeName, EntityUid, Request,PolicySet}; /// let data = serde_json::json!( /// { - /// "sub": "1234" + /// "sub": 1234 /// }); - /// let schema_data =r#" + /// let schema_json = serde_json::json!( /// { /// "": { - /// "entityTypes": {}, - /// "actions": { - /// "view": { - /// "appliesTo": { - /// "principalTypes": [], - /// "resourceTypes": [], - /// "context": { - /// "type": "Record", - /// "attributes": { - /// "sub": { "type": "Long" } - /// } + /// "entityTypes": { + /// "User": {}, + /// "Album": {}, + /// }, + /// "actions": { + /// "view": { + /// "appliesTo": { + /// "principalTypes": ["User"], + /// "resourceTypes": ["Album"], + /// "context": { + /// "type": "Record", + /// "attributes": { + /// "sub": { "type": "Long" } /// } /// } /// } + /// } /// } /// } - /// }"#; + /// }); /// # // create a request /// # let p_eid = EntityId::from_str("alice").unwrap(); /// # let p_name: EntityTypeName = EntityTypeName::from_str("User").unwrap(); @@ -3273,9 +3240,9 @@ impl Context { /// # let r_eid = EntityId::from_str("trip").unwrap(); /// # let r_name: EntityTypeName = EntityTypeName::from_str("Album").unwrap(); /// # let resource = EntityUid::from_type_name_and_id(r_name, r_eid); - /// let schema = Schema::from_str(schema_data).unwrap(); + /// let schema = Schema::from_json_value(schema_json).unwrap(); /// let context = Context::from_json_value(data, Some((&schema, &action))).unwrap(); - /// let request: Request = Request::new(Some(principal), Some(action), Some(resource), context); + /// # let request: Request = Request::new(Some(principal), Some(action), Some(resource), context, Some(&schema)).unwrap(); /// ``` pub fn from_json_value( json: serde_json::Value, @@ -3306,7 +3273,7 @@ impl Context { /// # use std::collections::HashMap; /// # use std::str::FromStr; /// # use std::fs::File; - /// let mut json = File::open("json_file.txt").expect("failed"); + /// let mut json = File::open("json_file.txt").unwrap(); /// let context = Context::from_json_file(&json, None).unwrap(); /// # // create a request /// # let p_eid = EntityId::from_str("alice").unwrap(); @@ -3319,7 +3286,7 @@ impl Context { /// # let r_eid = EntityId::from_str("trip").unwrap(); /// # let r_name: EntityTypeName = EntityTypeName::from_str("Album").unwrap(); /// # let r = EntityUid::from_type_name_and_id(r_name, r_eid); - /// let request: Request = Request::new(Some(p), Some(a), Some(r), context); + /// # let request: Request = Request::new(Some(p), Some(a), Some(r), context, None).unwrap(); /// ``` pub fn from_json_file( json: impl std::io::Read, @@ -3542,6 +3509,7 @@ mod partial_eval_test { #[cfg(test)] mod entity_uid_tests { use super::*; + use cool_asserts::assert_matches; /// building an `EntityUid` from components #[test] @@ -3753,13 +3721,13 @@ permit(principal == A :: B #[test] fn accessing_unspecified_entity_returns_none() { let c = Context::empty(); - let request: Request = Request::new(None, None, None, c); + let request = Request::new(None, None, None, c, None).unwrap(); let p = request.principal(); let a = request.action(); let r = request.resource(); - assert!(p.is_none()); - assert!(a.is_none()); - assert!(r.is_none()); + assert_matches!(p, None); + assert_matches!(a, None); + assert_matches!(r, None); } } @@ -4054,7 +4022,9 @@ mod policy_set_tests { Some(EntityUid::from_strs("Action", "a")), Some(EntityUid::from_strs("Resource", "b")), Context::empty(), - ); + None, + ) + .unwrap(); let e = r#"[ { @@ -4336,7 +4306,9 @@ mod policy_set_tests { Some(EntityUid::from_strs("Action", "a")), Some(EntityUid::from_strs("Resource", "b")), Context::empty(), - ); + None, + ) + .unwrap(); let e = r#"[ { diff --git a/cedar-policy/src/frontend/is_authorized.rs b/cedar-policy/src/frontend/is_authorized.rs index 745c9b7c25..66326c24ab 100644 --- a/cedar-policy/src/frontend/is_authorized.rs +++ b/cedar-policy/src/frontend/is_authorized.rs @@ -185,7 +185,7 @@ impl AuthorizationCall { .map_err(|e| [format!("Error encoding the context as JSON: {e}")])?; let context = Context::from_json_value(context, schema.as_ref().map(|s| (s, &action))) .map_err(|e| [e.to_string()])?; - let q = Request::new(principal, Some(action), resource, context); + let q = Request::new(principal, Some(action), resource, context, None).unwrap(); let (policies, entities) = self.slice.try_into(schema.as_ref())?; Ok((q, policies, entities)) } @@ -1334,21 +1334,21 @@ mod test { "slice" : { "policies" : {}, "entities" : [ - { + { "uid": { "type" : "User", "id" : "alice" }, - "attrs": {}, - "parents": [] + "attrs": {}, + "parents": [] }, - { + { "uid": { "type" : "User", "id" : "alice" }, - "attrs": {}, - "parents": [] + "attrs": {}, + "parents": [] } ], "templates" : {}, diff --git a/cedar-policy/src/integration_testing.rs b/cedar-policy/src/integration_testing.rs index e01b3314f9..8d21cc7fba 100644 --- a/cedar-policy/src/integration_testing.rs +++ b/cedar-policy/src/integration_testing.rs @@ -93,6 +93,9 @@ pub struct JsonRequest { /// Context for the request. This should be a JSON object, not any other kind /// of JSON value context: serde_json::Value, + /// Whether to enable request validation for this request + #[serde(default = "constant_true")] + enable_request_validation: bool, /// Expected decision for the request decision: Decision, /// Expected "reasons" for the request @@ -101,6 +104,10 @@ pub struct JsonRequest { errors: Vec, } +fn constant_true() -> bool { + true +} + /// For relative paths, return the absolute path, assuming that the path /// is relative to the root of the `CedarIntegrationTests` repo. /// For absolute paths, return them unchanged. @@ -299,7 +306,24 @@ pub fn perform_integration_test_from_json_custom( jsonfile.display() ) }); - let request = Request::new(principal, action, resource, context); + let request = Request::new( + principal, + action, + resource, + context, + if json_request.enable_request_validation { + Some(&schema) + } else { + None + }, + ) + .unwrap_or_else(|e| { + panic!( + "error validating request \"{}\" in {}: {e}", + json_request.desc, + jsonfile.display() + ) + }); let response = if let Some(custom_impl) = custom_impl_opt { custom_impl.is_authorized(&request.0, &policies.ast, &entities.0) } else { diff --git a/cedar-policy/tests/public_interface.rs b/cedar-policy/tests/public_interface.rs index cae52091e9..9431285701 100644 --- a/cedar-policy/tests/public_interface.rs +++ b/cedar-policy/tests/public_interface.rs @@ -171,7 +171,9 @@ fn authorize_custom_request() -> Result<(), Box> { Some(action.clone()), Some(resource.clone()), context, - ); + None, + ) + .unwrap(); // Check that we got the "Deny" result assert_eq!( @@ -186,7 +188,9 @@ fn authorize_custom_request() -> Result<(), Box> { Some(action.clone()), Some(resource.clone()), Context::empty(), - ); + None, + ) + .unwrap(); // Check that we got the "Allow" result and it was based on the added policy assert_eq!( @@ -204,7 +208,9 @@ fn authorize_custom_request() -> Result<(), Box> { None, Some(resource.clone()), Context::empty(), - ); + None, + ) + .unwrap(); // Check that we got an "Allow" result assert_eq!( @@ -214,13 +220,21 @@ fn authorize_custom_request() -> Result<(), Box> { ); // Requesting with an unspecified principal or resource will return Deny (but not fail) - let request4 = Request::new(None, Some(action.clone()), Some(resource), Context::empty()); + let request4 = Request::new( + None, + Some(action.clone()), + Some(resource), + Context::empty(), + None, + ) + .unwrap(); assert_eq!( auth.is_authorized(&request4, &policies, &entities) .decision(), Decision::Deny ); - let request5 = Request::new(Some(principal), Some(action), None, Context::empty()); + let request5 = + Request::new(Some(principal), Some(action), None, Context::empty(), None).unwrap(); assert_eq!( auth.is_authorized(&request5, &policies, &entities) .decision(), @@ -259,7 +273,9 @@ fn expression_eval_1() -> Result<(), Box> { Some(action), Some(resource), Context::empty(), - ); + None, + ) + .unwrap(); //try an evaluation let result = eval_expression( @@ -304,7 +320,9 @@ fn expression_eval_attr() -> Result<(), Box> { Some(action), Some(resource), Context::empty(), - ); + None, + ) + .unwrap(); //try an evaluation let result = eval_expression( @@ -355,7 +373,8 @@ fn expression_eval_context() -> Result<(), Box> { .unwrap(); // Combine into request - let request = Request::new(Some(principal), Some(action), Some(resource), context); + let request = + Request::new(Some(principal), Some(action), Some(resource), context, None).unwrap(); //try an evaluation let result = eval_expression( From cf90e19e7be72649ef96f73abeee7476c86e3cee Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 6 Nov 2023 15:46:57 +0000 Subject: [PATCH 08/11] fix a panic, other clippy nits --- cedar-policy-core/src/ast/request.rs | 8 ++++---- cedar-policy-validator/src/coreschema.rs | 4 ++-- cedar-policy/src/frontend/is_authorized.rs | 3 ++- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/cedar-policy-core/src/ast/request.rs b/cedar-policy-core/src/ast/request.rs index a6e46eb37c..4f4f109e3f 100644 --- a/cedar-policy-core/src/ast/request.rs +++ b/cedar-policy-core/src/ast/request.rs @@ -286,10 +286,10 @@ pub trait RequestSchema { /// Error type returned when a request fails validation type Error: std::error::Error; /// Validate the given `request`, returning `Err` if it fails validation - fn validate_request<'a>( + fn validate_request( &self, request: &Request, - extensions: Extensions<'a>, + extensions: Extensions<'_>, ) -> Result<(), Self::Error>; } @@ -298,10 +298,10 @@ pub trait RequestSchema { pub struct RequestSchemaAllPass; impl RequestSchema for RequestSchemaAllPass { type Error = std::convert::Infallible; - fn validate_request<'a>( + fn validate_request( &self, _request: &Request, - _extensions: Extensions<'a>, + _extensions: Extensions<'_>, ) -> Result<(), Self::Error> { Ok(()) } diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index b34c0a14cc..aeed0c5f90 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -131,10 +131,10 @@ impl entities::EntityTypeDescription for EntityTypeDescription { impl ast::RequestSchema for ValidatorSchema { type Error = RequestValidationError; - fn validate_request<'e>( + fn validate_request( &self, request: &ast::Request, - extensions: Extensions<'e>, + extensions: Extensions<'_>, ) -> std::result::Result<(), Self::Error> { use ast::EntityUIDEntry; // first check that principal and resource are of types that exist in diff --git a/cedar-policy/src/frontend/is_authorized.rs b/cedar-policy/src/frontend/is_authorized.rs index 66326c24ab..cf53a3caff 100644 --- a/cedar-policy/src/frontend/is_authorized.rs +++ b/cedar-policy/src/frontend/is_authorized.rs @@ -185,7 +185,8 @@ impl AuthorizationCall { .map_err(|e| [format!("Error encoding the context as JSON: {e}")])?; let context = Context::from_json_value(context, schema.as_ref().map(|s| (s, &action))) .map_err(|e| [e.to_string()])?; - let q = Request::new(principal, Some(action), resource, context, None).unwrap(); + let q = Request::new(principal, Some(action), resource, context, schema.as_ref()) + .map_err(|e| [e.to_string()])?; let (policies, entities) = self.slice.try_into(schema.as_ref())?; Ok((q, policies, entities)) } From 8908ee504958200ad749fec464ad1f3b12806080 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 6 Nov 2023 18:25:21 +0000 Subject: [PATCH 09/11] typecheck arguments to extn fn calls in Context too --- cedar-policy-core/src/entities/conformance.rs | 7 +++- cedar-policy-validator/src/coreschema.rs | 15 +++---- cedar-policy-validator/src/types.rs | 42 ++++++++++++++----- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/cedar-policy-core/src/entities/conformance.rs b/cedar-policy-core/src/entities/conformance.rs index 4d1d933348..04c66df8a0 100644 --- a/cedar-policy-core/src/entities/conformance.rs +++ b/cedar-policy-core/src/entities/conformance.rs @@ -239,10 +239,13 @@ impl<'a, S: Schema> EntitySchemaConformanceChecker<'a, S> { /// Errors thrown by [`type_of_restricted_expr()`] #[derive(Debug, Error)] pub enum TypeOfRestrictedExprError { - /// Encountered a heterogeneous set + /// Encountered a heterogeneous set. Heterogeneous sets do not have a valid + /// `SchemaType`. #[error(transparent)] HeterogeneousSet(#[from] HeterogeneousSetError), - /// Error looking up an extension function + /// Error looking up an extension function, which may be necessary for + /// expressions that contain extension function calls -- not to actually + /// call the extension function, but to get metadata about it #[error(transparent)] ExtensionFunctionLookup(#[from] ExtensionFunctionLookupError), } diff --git a/cedar-policy-validator/src/coreschema.rs b/cedar-policy-validator/src/coreschema.rs index aeed0c5f90..cf14a8dbfd 100644 --- a/cedar-policy-validator/src/coreschema.rs +++ b/cedar-policy-validator/src/coreschema.rs @@ -1,5 +1,6 @@ use crate::{ValidatorEntityType, ValidatorSchema}; -use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; +use cedar_policy_core::entities::TypeOfRestrictedExprError; +use cedar_policy_core::extensions::Extensions; use cedar_policy_core::{ast, entities}; use smol_str::SmolStr; use std::collections::{HashMap, HashSet}; @@ -198,7 +199,8 @@ impl ast::RequestSchema for ValidatorSchema { if let Some(context) = request.context() { let expected_context_ty = validator_action_id.context_type(); if !expected_context_ty - .typecheck_restricted_expr(context.as_ref().as_borrowed(), extensions)? + .typecheck_restricted_expr(context.as_ref().as_borrowed(), extensions) + .map_err(RequestValidationError::TypeOfContext)? { return Err(RequestValidationError::InvalidContext { context: context.clone(), @@ -277,11 +279,10 @@ pub enum RequestValidationError { /// Action which it is not valid for action: Arc, }, - /// Error looking up an extension function, which may be necessary for - /// `Context`s that contain extension function calls -- not to actually - /// call the extension function, but to get metadata about it - #[error(transparent)] - ExtensionFunctionLookup(#[from] ExtensionFunctionLookupError), + /// Error computing the type of the `Context`; see the contained error type + /// for details about the kinds of errors that can occur + #[error("context is not valid: {0}")] + TypeOfContext(TypeOfRestrictedExprError), } /// Struct which carries enough information that it can impl Core's diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index 993eed7660..4ca8cc5ba5 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -24,10 +24,11 @@ use std::{ fmt::Display, }; -use cedar_policy_core::ast::{ - BorrowedRestrictedExpr, EntityType, EntityUID, Expr, ExprShapeOnly, Name, +use cedar_policy_core::{ + ast::{BorrowedRestrictedExpr, EntityType, EntityUID, Expr, ExprShapeOnly, Name}, + entities::{type_of_restricted_expr, TypeOfRestrictedExprError}, + extensions::Extensions, }; -use cedar_policy_core::extensions::{ExtensionFunctionLookupError, Extensions}; use crate::ValidationMode; @@ -631,7 +632,7 @@ impl Type { &self, restricted_expr: BorrowedRestrictedExpr<'_>, extensions: Extensions<'_>, - ) -> Result { + ) -> Result { match self { Type::Never => Ok(false), // no expr has type Never Type::Primitive { @@ -718,13 +719,32 @@ impl Type { None => Ok(false), }, Type::ExtensionType { name } => match restricted_expr.as_extn_fn_call() { - Some((fn_name, _)) => match extensions.func(fn_name)?.return_type() { - Some(cedar_policy_core::entities::SchemaType::Extension { - name: actual_name, - }) => Ok(actual_name == name), - _ => Ok(false), - }, - None => Ok(false), + Some((fn_name, args)) => { + let func = extensions.func(fn_name)?; + match func.return_type() { + Some(cedar_policy_core::entities::SchemaType::Extension { + name: actual_name, + }) => { + if actual_name != name { + return Ok(false); + } + } + _ => return Ok(false), + } + for (actual_arg, expected_arg_ty) in args.zip(func.arg_types()) { + match expected_arg_ty { + None => {} // in this case, the docs on `.arg_types()` say that multiple types are allowed, we just approximate as saying you can pass any type to this argument + Some(ty) => { + if &type_of_restricted_expr(actual_arg, extensions)? != ty { + return Ok(false); + } + } + } + } + // if we got here, then the return type and arg types typecheck + Ok(true) + } + None => Ok(false), // no other kinds of restricted expr (other than fn calls) can produce extension-typed values }, } } From c3a1ad7956f44b622f88d2636a64649853166a15 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 7 Nov 2023 14:43:01 +0000 Subject: [PATCH 10/11] CLI changelog --- cedar-policy-cli/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cedar-policy-cli/CHANGELOG.md b/cedar-policy-cli/CHANGELOG.md index 7723b64d21..744b984249 100644 --- a/cedar-policy-cli/CHANGELOG.md +++ b/cedar-policy-cli/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +- Requests are now validated by default. This can be disabled with + `--request-validation=false`. + ## 2.4.2 ## 2.4.1 From 7ad62bdde0b32b73d13ebbf6942e2677038539b2 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 7 Nov 2023 14:49:11 +0000 Subject: [PATCH 11/11] tweak CLI changelog --- cedar-policy-cli/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cedar-policy-cli/CHANGELOG.md b/cedar-policy-cli/CHANGELOG.md index 744b984249..f6a785c22b 100644 --- a/cedar-policy-cli/CHANGELOG.md +++ b/cedar-policy-cli/CHANGELOG.md @@ -2,8 +2,8 @@ ## Unreleased -- Requests are now validated by default. This can be disabled with - `--request-validation=false`. +- Requests are now validated by default if a schema is provided. This can be + disabled with `--request-validation=false`. ## 2.4.2