From 040bacb5b1a13b6a1429f111af3c7554b0708360 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Fri, 3 May 2024 13:06:11 -0400 Subject: [PATCH 01/10] first pass Signed-off-by: Kesha Hietala --- cedar-policy-core/src/authorizer.rs | 1 + cedar-policy/src/api.rs | 6 +- cedar-policy/src/ffi/is_authorized.rs | 80 ++++++++++++----------- cedar-policy/src/ffi/utils.rs | 13 ++-- cedar-policy/src/ffi/validate.rs | 83 +++++++++++++----------- cedar-policy/src/integration_testing.rs | 6 +- cedar-testing/src/cedar_test_impl.rs | 6 +- cedar-testing/src/integration_testing.rs | 7 +- 8 files changed, 112 insertions(+), 90 deletions(-) diff --git a/cedar-policy-core/src/authorizer.rs b/cedar-policy-core/src/authorizer.rs index b794d5e82..82c3cb82d 100644 --- a/cedar-policy-core/src/authorizer.rs +++ b/cedar-policy-core/src/authorizer.rs @@ -652,6 +652,7 @@ impl Response { #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Copy)] #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] #[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] +#[serde(rename_all = "camelCase")] pub enum Decision { /// The `Authorizer` determined that the request should be allowed Allow, diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index f8d168fee..8d11405b5 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -50,6 +50,7 @@ use cedar_policy_validator::RequestValidationError; // this type is unsuitable f use itertools::{Either, Itertools}; use miette::Diagnostic; use ref_cast::RefCast; +use serde::{Deserialize, Serialize}; use smol_str::SmolStr; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::str::FromStr; @@ -1060,7 +1061,10 @@ impl From for Response { } /// Used to select how a policy will be validated. -#[derive(Default, Eq, PartialEq, Copy, Clone, Debug)] +#[derive(Default, Eq, PartialEq, Copy, Clone, Debug, Serialize, Deserialize)] +#[cfg_attr(feature = "wasm", derive(tsify::Tsify))] +#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] +#[serde(rename_all = "camelCase")] #[non_exhaustive] pub enum ValidationMode { /// Validate that policies do not contain any type errors, and additionally diff --git a/cedar-policy/src/ffi/is_authorized.rs b/cedar-policy/src/ffi/is_authorized.rs index 73500591e..751b208af 100644 --- a/cedar-policy/src/ffi/is_authorized.rs +++ b/cedar-policy/src/ffi/is_authorized.rs @@ -22,7 +22,6 @@ use crate::{ Authorizer, Context, Decision, Entities, EntityId, EntityTypeName, EntityUid, PolicyId, Request, SlotId, }; -use cedar_policy_core::jsonvalue::JsonValueWithNoDuplicateKeys; use itertools::Itertools; use miette::{miette, Diagnostic, WrapErr}; use serde::{Deserialize, Serialize}; @@ -154,10 +153,10 @@ pub struct Response { #[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] #[serde(rename_all = "camelCase")] pub struct Diagnostics { - /// `PolicyId`s of the policies that contributed to the decision. + /// Ids of the policies that contributed to the decision. /// If no policies applied to the request, this set will be empty. - #[cfg_attr(feature = "wasm", tsify(type = "Set"))] - reason: HashSet, + #[cfg_attr(feature = "wasm", tsify(type = "Set"))] // REVIEW: still needed? + reason: HashSet, /// Set of errors that occurred errors: HashSet, } @@ -166,7 +165,7 @@ impl Response { /// Construct a `Response` pub fn new( decision: Decision, - reason: HashSet, + reason: HashSet, errors: HashSet, ) -> Self { Self { @@ -191,7 +190,7 @@ impl From for Response { let (reason, errors) = response.diagnostics.into_components(); Self::new( response.decision, - reason.collect(), + reason.map(|id| id.to_smolstr()).collect(), errors.map(Into::into).collect(), ) } @@ -208,7 +207,7 @@ impl TryFrom for Response { impl Diagnostics { /// Get the policies that contributed to the decision - pub fn reason(&self) -> impl Iterator { + pub fn reason(&self) -> impl Iterator { self.reason.iter() } @@ -254,7 +253,7 @@ impl From for AuthorizationError { fn from(e: crate::AuthorizationError) -> Self { match e { crate::AuthorizationError::PolicyEvaluationError { id, error } => { - AuthorizationError::new(id.to_smolstr(), error) + Self::new(id.to_smolstr(), error) } } } @@ -276,12 +275,12 @@ impl From for AuthorizationEr #[serde(rename_all = "camelCase")] pub struct ResidualResponse { decision: Option, - satisfied: HashSet, - errored: HashSet, - may_be_determining: HashSet, - must_be_determining: HashSet, - residuals: HashMap, - nontrivial_residuals: HashSet, + satisfied: HashSet, + errored: HashSet, + may_be_determining: HashSet, + must_be_determining: HashSet, + residuals: HashMap, + nontrivial_residuals: HashSet, } #[cfg(feature = "partial-eval")] @@ -292,22 +291,22 @@ impl ResidualResponse { } /// Set of all satisfied policy Ids - pub fn satisfied(&self) -> impl Iterator { + pub fn satisfied(&self) -> impl Iterator { self.satisfied.iter() } /// Set of all policy ids for policies that errored - pub fn errored(&self) -> impl Iterator { + pub fn errored(&self) -> impl Iterator { self.errored.iter() } /// Over approximation of policies that determine the auth decision - pub fn may_be_determining(&self) -> impl Iterator { + pub fn may_be_determining(&self) -> impl Iterator { self.may_be_determining.iter() } /// Under approximation of policies that determine the auth decision - pub fn must_be_determining(&self) -> impl Iterator { + pub fn must_be_determining(&self) -> impl Iterator { self.must_be_determining.iter() } @@ -321,8 +320,8 @@ impl ResidualResponse { self.residuals.into_values() } - /// Get the residual policy for a specified [`PolicyId`] if it exists - pub fn residual(&self, p: &PolicyId) -> Option<&serde_json::Value> { + /// Get the residual policy for a specified id if it exists + pub fn residual(&self, p: &SmolStr) -> Option<&serde_json::Value> { self.residuals.get(p) } @@ -338,7 +337,7 @@ impl ResidualResponse { } /// Iterator over the set of non-trivial residual policy ids - pub fn nontrivial_residual_ids(&self) -> impl Iterator { + pub fn nontrivial_residual_ids(&self) -> impl Iterator { self.nontrivial_residuals.iter() } } @@ -352,24 +351,27 @@ impl TryFrom for ResidualResponse { decision: partial_response.decision(), satisfied: partial_response .definitely_satisfied() - .map(|p| p.id().clone()) + .map(|p| p.id().to_smolstr()) + .collect(), + errored: partial_response + .definitely_errored() + .map(ToSmolStr::to_smolstr) .collect(), - errored: partial_response.definitely_errored().cloned().collect(), may_be_determining: partial_response .may_be_determining() - .map(|p| p.id().clone()) + .map(|p| p.id().to_smolstr()) .collect(), must_be_determining: partial_response .must_be_determining() - .map(|p| p.id().clone()) + .map(|p| p.id().to_smolstr()) .collect(), nontrivial_residuals: partial_response .nontrivial_residuals() - .map(|p| p.id().clone()) + .map(|p| p.id().to_smolstr()) .collect(), residuals: partial_response .all_residuals() - .map(|e| e.to_json().map(|json| (e.id().clone(), json))) + .map(|e| e.to_json().map(|json| (e.id().to_smolstr(), json))) .collect::>()?, }) } @@ -444,18 +446,18 @@ pub enum PartialAuthorizationAnswer { pub struct AuthorizationCall { /// The principal taking action #[cfg_attr(feature = "wasm", tsify(type = "string|{type: string, id: string}"))] - principal: Option, + principal: Option, /// The action the principal is taking #[cfg_attr(feature = "wasm", tsify(type = "string|{type: string, id: string}"))] - action: JsonValueWithNoDuplicateKeys, + action: serde_json::Value, /// The resource being acted on by the principal #[cfg_attr(feature = "wasm", tsify(type = "string|{type: string, id: string}"))] - resource: Option, + resource: Option, /// The context details specific to the request #[serde_as(as = "MapPreventDuplicates<_, _>")] #[cfg_attr(feature = "wasm", tsify(type = "Record"))] /// The context details specific to the request - context: HashMap, + context: HashMap, /// Optional schema. /// If present, this will inform the parsing: for instance, it will allow /// `__entity` and `__extn` escapes to be implicit, and it will error if @@ -479,11 +481,11 @@ fn constant_true() -> bool { /// Parses the given JSON into an [`EntityUid`], or if it fails, adds an /// appropriate error to `errs` and returns `None`. fn parse_entity_uid( - entity_uid_json: JsonValueWithNoDuplicateKeys, + entity_uid_json: serde_json::Value, category: &str, errs: &mut Vec, ) -> Option { - match EntityUid::from_json(entity_uid_json.into()) + match EntityUid::from_json(entity_uid_json) .wrap_err_with(|| format!("Failed to parse {category}")) { Ok(euid) => Some(euid), @@ -497,7 +499,7 @@ fn parse_entity_uid( /// Parses the given JSON context into a [`Context`], or if it fails, adds /// appropriate error(s) to `errs` and returns an empty context. fn parse_context( - context_map: HashMap, + context_map: HashMap, schema_ref: Option<&crate::Schema>, action_ref: Option<&EntityUid>, errs: &mut Vec, @@ -749,7 +751,7 @@ struct RecvdSlice { /// JSON object containing the entities data, in "natural JSON" form -- same /// format as expected by EntityJsonParser #[cfg_attr(feature = "wasm", tsify(type = "Array"))] - entities: JsonValueWithNoDuplicateKeys, + entities: serde_json::Value, /// Optional template policies. #[serde_as(as = "Option>")] @@ -816,7 +818,7 @@ impl RecvdSlice { crate::PolicySet::new() } }; - let entities = match Entities::from_json_value(entities.into(), schema) { + let entities = match Entities::from_json_value(entities, schema) { Ok(entities) => entities, Err(e) => { errs.push(miette::Report::new(e)); @@ -1906,12 +1908,12 @@ mod partial_test { assert_eq!(response.decision(), None); let errors: Vec<_> = response.errored().collect(); assert_eq!(errors.len(), 0, "{errors:?}"); - let actual_residuals: HashSet<_> = response.nontrivial_residual_ids().collect(); + let actual_residuals: HashSet<&str> = response.nontrivial_residual_ids().map(|id| id.as_str()).collect(); for id in &expected_residuals { - assert!(actual_residuals.contains(&PolicyId::from_str(id).ok().unwrap()), "expected nontrivial residual for {id}, but it's missing") + assert!(actual_residuals.contains(id), "expected nontrivial residual for {id}, but it's missing") } for id in &actual_residuals { - assert!(expected_residuals.contains(id.to_string().as_str()),"found unexpected nontrivial residual for {id}") + assert!(expected_residuals.contains(id),"found unexpected nontrivial residual for {id}") } }); } diff --git a/cedar-policy/src/ffi/utils.rs b/cedar-policy/src/ffi/utils.rs index 5d854a72d..8d2294921 100644 --- a/cedar-policy/src/ffi/utils.rs +++ b/cedar-policy/src/ffi/utils.rs @@ -16,7 +16,6 @@ //! Utility functions and types for JSON interface use crate::{Policy, SchemaWarning, Template}; -use cedar_policy_core::jsonvalue::JsonValueWithNoDuplicateKeys; use miette::WrapErr; use serde::{Deserialize, Serialize}; use std::{collections::HashMap, str::FromStr}; @@ -131,12 +130,10 @@ impl<'a, E: miette::Diagnostic + ?Sized> From<&'a E> for DetailedError { severity: diag.severity().map(Into::into), source_locations: diag .labels() - .map(|labels| labels.map(Into::into).collect()) - .unwrap_or_else(|| vec![]), + .map_or_else(Vec::new, |labels| labels.map(Into::into).collect()), related: diag .related() - .map(|errs| errs.map(|e| e.into()).collect()) - .unwrap_or_else(|| vec![]), + .map_or_else(Vec::new, |errs| errs.map(Into::into).collect()), } } } @@ -237,11 +234,9 @@ impl PolicySet { #[serde(rename_all = "camelCase")] pub enum Schema { /// Schema in the Cedar schema format. See - #[serde(rename = "human")] Human(String), /// Schema in Cedar's JSON schema format. See - #[serde(rename = "json")] - Json(JsonValueWithNoDuplicateKeys), + Json(serde_json::Value), } impl Schema { @@ -257,7 +252,7 @@ impl Schema { ) }) .map_err(miette::Report::new), - Self::Json(val) => crate::Schema::from_json_value(val.into()) + Self::Json(val) => crate::Schema::from_json_value(val) .map(|sch| { ( sch, diff --git a/cedar-policy/src/ffi/validate.rs b/cedar-policy/src/ffi/validate.rs index aa124f4b2..9b62179c6 100644 --- a/cedar-policy/src/ffi/validate.rs +++ b/cedar-policy/src/ffi/validate.rs @@ -32,12 +32,21 @@ extern crate tsify; pub fn validate(call: ValidationCall) -> ValidationAnswer { match call.get_components() { WithWarnings { - t: Ok((policies, schema)), + t: Ok((policies, schema, settings)), warnings, } => { + // if validation is not enabled, stop here + if !settings.enabled { + return ValidationAnswer::Success { + validation_errors: Vec::new(), + validation_warnings: Vec::new(), + other_warnings: warnings.into_iter().map(Into::into).collect(), + }; + } + // otherwise, call `Validator::validate` let validator = Validator::new(schema); let (validation_errors, validation_warnings) = validator - .validate(&policies, ValidationMode::default()) + .validate(&policies, settings.mode) .into_errors_and_warnings(); let validation_errors: Vec = validation_errors .map(|error| ValidationError { @@ -94,15 +103,17 @@ pub struct ValidationCall { #[cfg_attr(feature = "wasm", tsify(type = "Schema"))] pub schema: Schema, /// Policies to validate - pub policy_set: PolicySet, + pub policies: PolicySet, } impl ValidationCall { fn get_components( self, - ) -> WithWarnings>> { + ) -> WithWarnings< + Result<(crate::PolicySet, crate::Schema, ValidationSettings), Vec>, + > { let mut errs = vec![]; - let policies = match self.policy_set.parse(None) { + let policies = match self.policies.parse(None) { Ok(policies) => policies, Err(e) => { errs.extend(e); @@ -118,7 +129,7 @@ impl ValidationCall { }; match (errs.is_empty(), pair) { (true, Some((schema, warnings))) => WithWarnings { - t: Ok((policies, schema)), + t: Ok((policies, schema, self.validation_settings)), warnings: warnings.map(miette::Report::new).collect(), }, _ => WithWarnings { @@ -130,31 +141,24 @@ impl ValidationCall { } /// Configuration for the validation call -#[derive(Default, Serialize, Deserialize, Debug)] -#[cfg_attr(feature = "wasm", derive(tsify::Tsify))] -#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] -#[serde(rename_all = "camelCase")] -pub struct ValidationSettings { - /// Whether validation is enabled - enabled: ValidationEnabled, -} - -/// String enum for validation mode #[derive(Serialize, Deserialize, Debug)] #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] #[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))] #[serde(rename_all = "camelCase")] -pub enum ValidationEnabled { - /// Setting for which policies will be validated against the schema - #[serde(alias = "regular")] - On, - /// Setting for which no validation will be done - Off, +pub struct ValidationSettings { + /// Whether validation is enabled. If this flag is set to `false`, then + /// only parsing is performed. The default value is `true`. + enabled: bool, + /// Used to control how a policy is validated. See comments on [`ValidationMode`]. + mode: ValidationMode, } -impl Default for ValidationEnabled { +impl Default for ValidationSettings { fn default() -> Self { - Self::On + Self { + enabled: true, + mode: ValidationMode::default(), + } } } @@ -253,7 +257,7 @@ mod test { let call = ValidationCall { validation_settings: ValidationSettings::default(), schema: Schema::Json(json!({}).into()), - policy_set: PolicySet::Map(HashMap::new()), + policies: PolicySet::Map(HashMap::new()), }; assert_validates_without_errors(serde_json::to_value(&call).unwrap()); @@ -261,14 +265,14 @@ mod test { let call = ValidationCall { validation_settings: ValidationSettings::default(), schema: Schema::Human(String::new()), - policy_set: PolicySet::Map(HashMap::new()), + policies: PolicySet::Map(HashMap::new()), }; assert_validates_without_errors(serde_json::to_value(&call).unwrap()); let call = json!({ "schema": { "json": {} }, - "policySet": {} + "policies": {} }); assert_validates_without_errors(call); @@ -320,7 +324,7 @@ mod test { } } }}}, - "policySet": { + "policies": { "policy0": "permit(principal in UserGroup::\"alice_friends\", action == Action::\"viewPhoto\", resource);" }}); @@ -334,7 +338,7 @@ mod test { "entityTypes": {}, "actions": {} }}}, - "policySet": { + "policies": { "policy0": "azfghbjknnhbud" } }); @@ -366,7 +370,7 @@ mod test { } } }}}, - "policySet": { + "policies": { "policy0": "permit(principal == Photo::\"photo.jpg\", action == Action::\"viewPhoto\", resource == User::\"alice\");", "policy1": "permit(principal == Photo::\"photo2.jpg\", action == Action::\"viewPhoto\", resource == User::\"alice2\");" }}); @@ -420,7 +424,7 @@ mod test { } } }}}, - "policySet": { + "policies": { "policy0": "permit(principal in UserGroup::\"alice_friends\", action == Action::\"viewPhoto\", resource);" } }); @@ -435,7 +439,7 @@ mod test { "entityTypes": {}, "actions": {} }}}, - "policySet": "azfghbjknnhbud" + "policies": "azfghbjknnhbud" }); assert_is_failure( @@ -465,7 +469,7 @@ mod test { } } }}}, - "policySet": "forbid(principal, action, resource);permit(principal == Photo::\"photo.jpg\", action == Action::\"viewPhoto\", resource == User::\"alice\");" + "policies": "forbid(principal, action, resource);permit(principal == Photo::\"photo.jpg\", action == Action::\"viewPhoto\", resource == User::\"alice\");" }); assert_validates_with_errors(json, 1); @@ -478,7 +482,7 @@ mod test { "entityTypes": {}, "actions": {} }}}, - "policySet": "permit(principal, action, resource);forbid" + "policies": "permit(principal, action, resource);forbid" }); assert_is_failure( @@ -495,17 +499,20 @@ mod test { } #[test] + // No special handling for duplicates in JSON strings (serde_json silently drops them) fn test_validate_fails_on_duplicate_namespace() { let json = r#"{ "schema": { "json": { "foo": { "entityTypes": {}, "actions": {} }, "foo": { "entityTypes": {}, "actions": {} } }}, - "policySet": "" + "policies": "" }"#; - assert_matches!(validate_json_str(json), Err(e) => { - assert!(e.to_string().contains("the key `foo` occurs two or more times in the same JSON object"), "actual error message was {e}"); + let ans_val = validate_json_str(json).unwrap(); + let result: Result = serde_json::from_str(ans_val.as_str()); + assert_matches!(result, Ok(ValidationAnswer::Success { validation_errors, validation_warnings: _, other_warnings: _ }) => { + assert_eq!(validation_errors.len(), 0, "Unexpected validation errors: {validation_errors:?}"); }); } @@ -513,7 +520,7 @@ mod test { fn test_validate_fails_on_duplicate_policy_id() { let json = r#"{ "schema": { "json": { "": { "entityTypes": {}, "actions": {} } } }, - "policySet": { + "policies": { "ID0": "permit(principal, action, resource);", "ID0": "permit(principal, action, resource);" } diff --git a/cedar-policy/src/integration_testing.rs b/cedar-policy/src/integration_testing.rs index a13517f1d..6694100f6 100644 --- a/cedar-policy/src/integration_testing.rs +++ b/cedar-policy/src/integration_testing.rs @@ -373,7 +373,11 @@ pub fn perform_integration_test_from_json_custom( &json_request.desc ); // check reasons - let reasons: HashSet = response.diagnostics().reason().cloned().collect(); + let reasons: HashSet = response + .diagnostics() + .reason() + .map(|s| PolicyId::from_str(s).unwrap()) + .collect(); assert_eq!( reasons, json_request.reason.into_iter().collect(), diff --git a/cedar-testing/src/cedar_test_impl.rs b/cedar-testing/src/cedar_test_impl.rs index 6a0d74d6a..eecd8dc74 100644 --- a/cedar-testing/src/cedar_test_impl.rs +++ b/cedar-testing/src/cedar_test_impl.rs @@ -202,7 +202,11 @@ impl CedarTestImplementation for RustEngine { let response = cedar_policy::Response::from(response); let response = ffi::Response::new( response.decision(), - response.diagnostics().reason().cloned().collect(), + response + .diagnostics() + .reason() + .map(|id| id.to_smolstr()) + .collect(), response .diagnostics() .errors() diff --git a/cedar-testing/src/integration_testing.rs b/cedar-testing/src/integration_testing.rs index de3970ee4..d2051ce32 100644 --- a/cedar-testing/src/integration_testing.rs +++ b/cedar-testing/src/integration_testing.rs @@ -327,7 +327,12 @@ pub fn perform_integration_test( &json_request.desc ); // check reason - let reason: HashSet = response.response.diagnostics().reason().cloned().collect(); + let reason: HashSet = response + .response + .diagnostics() + .reason() + .map(|id| PolicyId::from_str(id).unwrap()) + .collect(); assert_eq!( reason, json_request.reason.into_iter().collect(), From 6f710d82c89b1ad66261dc565c7f809bd20cd772 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Fri, 3 May 2024 13:17:38 -0400 Subject: [PATCH 02/10] update changelog Signed-off-by: Kesha Hietala --- cedar-policy/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index aeafa0f6a..ff3a5fa73 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -22,7 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - for the `partial-eval` experimental feature: `PartialResponse` api has changed significantly - Moved `::Err` to `Infallible` (#588, resolving #551) - Overhauled the FFI interface in the `frontend` module, and renamed it to - `ffi`; see #757. (#760, #793, #794, #800, #824, more coming) + `ffi`; see #757. (#760, #793, #794, #800, #824, #837, more coming) - Much richer error information in the FFI interface (#800) - Deprecated the integration testing harness code. It will be removed from the `cedar-policy` crate in the next major version. From d7d2e9a06b456c8e2e41b01e244e8c648aa5930e Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Fri, 3 May 2024 13:49:12 -0400 Subject: [PATCH 03/10] more breaking changes in cedar-testing Signed-off-by: Kesha Hietala --- cedar-testing/src/cedar_test_impl.rs | 4 ++++ cedar-testing/src/integration_testing.rs | 28 ++++++++++++------------ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/cedar-testing/src/cedar_test_impl.rs b/cedar-testing/src/cedar_test_impl.rs index eecd8dc74..0d92dafb8 100644 --- a/cedar-testing/src/cedar_test_impl.rs +++ b/cedar-testing/src/cedar_test_impl.rs @@ -34,6 +34,7 @@ use std::time::{Duration, Instant}; /// Return type for `CedarTestImplementation` methods #[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] pub enum TestResult { /// The request succeeded Success(T), @@ -65,11 +66,13 @@ impl TestResult { /// Simple wrapper around u128 to remind ourselves that timing info is in microseconds. #[derive(Debug, Deserialize)] +#[serde(transparent)] pub struct Micros(pub u128); /// Version of `Response` used for testing. Includes a /// `ffi::Response` and a map with timing information. #[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] pub struct TestResponse { /// Actual response pub response: ffi::Response, @@ -80,6 +83,7 @@ pub struct TestResponse { /// Version of `ValidationResult` used for testing. #[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] pub struct TestValidationResult { /// Validation errors pub errors: Vec, diff --git a/cedar-testing/src/integration_testing.rs b/cedar-testing/src/integration_testing.rs index d2051ce32..206da7b99 100644 --- a/cedar-testing/src/integration_testing.rs +++ b/cedar-testing/src/integration_testing.rs @@ -43,6 +43,7 @@ use std::{ /// JSON representation of our integration test file format #[derive(Debug, Deserialize, Serialize)] #[serde(deny_unknown_fields)] +#[serde(rename_all = "camelCase")] pub struct JsonTest { /// Filename of the policies to use (in pure Cedar syntax) pub policies: String, @@ -55,7 +56,6 @@ pub struct JsonTest { pub should_validate: bool, /// Requests to perform on that data, along with their expected results /// Alias for backwards compatibility - #[serde(alias = "queries")] pub requests: Vec, } @@ -63,9 +63,10 @@ pub struct JsonTest { /// in our integration test file format #[derive(Debug, Deserialize, Serialize, Clone)] #[serde(deny_unknown_fields)] +#[serde(rename_all = "camelCase")] pub struct JsonRequest { /// Description for the request - pub desc: String, + pub description: String, /// Principal for the request, in either explicit or implicit `__entity` form /// /// Examples: @@ -92,11 +93,10 @@ pub struct JsonRequest { pub context: JsonValueWithNoDuplicateKeys, /// Whether to enable request validation for this request #[serde(default = "constant_true")] - pub enable_request_validation: bool, + pub validate_request: bool, /// Expected decision for the request pub decision: Decision, /// Expected policies that led to the decision - #[serde(alias = "reasons")] pub reason: Vec, /// Expected policies that resulted in errors pub errors: Vec, @@ -214,7 +214,7 @@ pub fn parse_request_from_test( .map(|json| { let error_string = format!( "Failed to parse principal for request \"{}\" in {}", - json_request.desc, test_name + json_request.description, test_name ); parse_entity_uid(json, error_string) }) @@ -225,7 +225,7 @@ pub fn parse_request_from_test( .map(|json| { let error_string = format!( "Failed to parse action for request \"{}\" in {}", - json_request.desc, test_name + json_request.description, test_name ); parse_entity_uid(json, error_string) }) @@ -236,7 +236,7 @@ pub fn parse_request_from_test( .map(|json| { let error_string = format!( "Failed to parse resource for request \"{}\" in {}", - json_request.desc, test_name + json_request.description, test_name ); parse_entity_uid(json, error_string) }) @@ -245,7 +245,7 @@ pub fn parse_request_from_test( .unwrap_or_else(|| { panic!( "Unknown action {} for request \"{}\" in {}", - action, json_request.desc, test_name + action, json_request.description, test_name ) }); let context = @@ -254,7 +254,7 @@ pub fn parse_request_from_test( .unwrap_or_else(|e| { panic!( "Failed to parse context for request \"{}\" in {}: {e}", - json_request.desc, test_name + json_request.description, test_name ) }); Request::new( @@ -262,7 +262,7 @@ pub fn parse_request_from_test( (action, None), (resource, None), context, - if json_request.enable_request_validation { + if json_request.validate_request { Some(schema) } else { None @@ -272,7 +272,7 @@ pub fn parse_request_from_test( .unwrap_or_else(|e| { panic!( "error validating request \"{}\" in {}: {e}", - json_request.desc, test_name + json_request.description, test_name ) }) } @@ -324,7 +324,7 @@ pub fn perform_integration_test( response.response.decision(), json_request.decision, "test {test_name} failed for request \"{}\": unexpected decision", - &json_request.desc + &json_request.description ); // check reason let reason: HashSet = response @@ -337,7 +337,7 @@ pub fn perform_integration_test( reason, json_request.reason.into_iter().collect(), "test {test_name} failed for request \"{}\": unexpected reason", - &json_request.desc + &json_request.description ); // check errors, if applicable // for now, the integration tests only support the `PolicyIds` comparison mode @@ -355,7 +355,7 @@ pub fn perform_integration_test( errors, json_request.errors.into_iter().collect(), "test {test_name} failed for request \"{}\": unexpected errors", - &json_request.desc + &json_request.description ); } } From 2176d6be5942db5855e2379c0abee7941ff09dad Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Thu, 9 May 2024 09:42:38 -0400 Subject: [PATCH 04/10] Update cedar-policy/src/ffi/is_authorized.rs Co-authored-by: Craig Disselkoen --- cedar-policy/src/ffi/is_authorized.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy/src/ffi/is_authorized.rs b/cedar-policy/src/ffi/is_authorized.rs index 751b208af..988074230 100644 --- a/cedar-policy/src/ffi/is_authorized.rs +++ b/cedar-policy/src/ffi/is_authorized.rs @@ -1913,7 +1913,7 @@ mod partial_test { assert!(actual_residuals.contains(id), "expected nontrivial residual for {id}, but it's missing") } for id in &actual_residuals { - assert!(expected_residuals.contains(id),"found unexpected nontrivial residual for {id}") + assert!(expected_residuals.contains(id), "found unexpected nontrivial residual for {id}") } }); } From 65ca7128a79e949cbb58ff5672b5973bad6dc5c2 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Thu, 9 May 2024 12:17:56 -0400 Subject: [PATCH 05/10] add into_smolstr --- cedar-policy-core/src/ast/policy.rs | 5 +++++ cedar-policy/src/api/id.rs | 6 ++++++ cedar-policy/src/ffi/is_authorized.rs | 2 +- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 8cdfedde4..a0533f4fe 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -1713,6 +1713,11 @@ impl PolicyID { pub fn from_smolstr(id: SmolStr) -> Self { Self(id) } + + /// Get the underlying `SmolStr` + pub fn into_smolstr(self) -> SmolStr { + self.0 + } } impl std::fmt::Display for PolicyID { diff --git a/cedar-policy/src/api/id.rs b/cedar-policy/src/api/id.rs index 537e92951..7d1dd086c 100644 --- a/cedar-policy/src/api/id.rs +++ b/cedar-policy/src/api/id.rs @@ -24,6 +24,7 @@ use cedar_policy_core::entities::json::err::JsonDeserializationErrorContext; use cedar_policy_core::FromNormalizedStr; use ref_cast::RefCast; use serde::{Deserialize, Serialize}; +use smol_str::SmolStr; use std::convert::Infallible; use std::str::FromStr; @@ -322,6 +323,11 @@ impl PolicyId { pub fn new(id: impl AsRef) -> Self { Self(ast::PolicyID::from_string(id.as_ref())) } + + /// Get the underlying `SmolStr` + pub(crate) fn into_smolstr(self) -> SmolStr { + self.0.into_smolstr() + } } impl FromStr for PolicyId { diff --git a/cedar-policy/src/ffi/is_authorized.rs b/cedar-policy/src/ffi/is_authorized.rs index ffc8b98b1..0436804a2 100644 --- a/cedar-policy/src/ffi/is_authorized.rs +++ b/cedar-policy/src/ffi/is_authorized.rs @@ -190,7 +190,7 @@ impl From for Response { let (reason, errors) = response.diagnostics.into_components(); Self::new( response.decision, - reason.map(|id| id.to_smolstr()).collect(), + reason.map(PolicyId::into_smolstr).collect(), errors.map(Into::into).collect(), ) } From e19711a3ede6e401a943a535efdf6da6e02ba546 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Mon, 3 Jun 2024 13:54:11 -0400 Subject: [PATCH 06/10] add back JsonValueWithNoDuplicateKeys --- cedar-policy-core/src/jsonvalue.rs | 4 ++++ cedar-policy/src/ffi/is_authorized.rs | 34 +++++++++++++-------------- cedar-policy/src/ffi/utils.rs | 9 +++++-- cedar-policy/src/ffi/validate.rs | 7 ++---- 4 files changed, 30 insertions(+), 24 deletions(-) diff --git a/cedar-policy-core/src/jsonvalue.rs b/cedar-policy-core/src/jsonvalue.rs index 57e4709c6..d3d0bed57 100644 --- a/cedar-policy-core/src/jsonvalue.rs +++ b/cedar-policy-core/src/jsonvalue.rs @@ -22,6 +22,10 @@ use serde::{Deserialize, Serialize}; /// Wrapper around `serde_json::Value`, with a different `Deserialize` /// implementation, such that duplicate keys in JSON objects (maps/records) are /// not allowed (result in an error). +// +// CAUTION: this type is publicly exported in `cedar-policy`. +// Don't make fields `pub`, don't make breaking changes, and use caution +// when adding public methods. #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub struct JsonValueWithNoDuplicateKeys(serde_json::Value); diff --git a/cedar-policy/src/ffi/is_authorized.rs b/cedar-policy/src/ffi/is_authorized.rs index 2e0f226d1..804b49d9a 100644 --- a/cedar-policy/src/ffi/is_authorized.rs +++ b/cedar-policy/src/ffi/is_authorized.rs @@ -17,7 +17,7 @@ //! This module contains the `is_authorized` entry points that other language //! FFIs can call #![allow(clippy::module_name_repetitions)] -use super::utils::{DetailedError, PolicySet, Schema, WithWarnings}; +use super::utils::*; use crate::{ Authorizer, Context, Decision, Entities, EntityId, EntityTypeName, EntityUid, PolicyId, Request, SlotId, @@ -155,7 +155,7 @@ pub struct Response { pub struct Diagnostics { /// Ids of the policies that contributed to the decision. /// If no policies applied to the request, this set will be empty. - #[cfg_attr(feature = "wasm", tsify(type = "Set"))] // REVIEW: still needed? + #[cfg_attr(feature = "wasm", tsify(type = "Set"))] reason: HashSet, /// Set of errors that occurred errors: HashSet, @@ -279,7 +279,7 @@ pub struct ResidualResponse { errored: HashSet, may_be_determining: HashSet, must_be_determining: HashSet, - residuals: HashMap, + residuals: HashMap, nontrivial_residuals: HashSet, } @@ -311,22 +311,22 @@ impl ResidualResponse { } /// (Borrowed) Iterator over the set of residual policies - pub fn residuals(&self) -> impl Iterator { + pub fn residuals(&self) -> impl Iterator { self.residuals.values() } /// (Owned) Iterator over the set of residual policies - pub fn into_residuals(self) -> impl Iterator { + pub fn into_residuals(self) -> impl Iterator { self.residuals.into_values() } /// Get the residual policy for a specified id if it exists - pub fn residual(&self, p: &SmolStr) -> Option<&serde_json::Value> { + pub fn residual(&self, p: &SmolStr) -> Option<&JsonValueWithNoDuplicateKeys> { self.residuals.get(p) } /// (Borrowed) Iterator over the set of non-trivial residual policies - pub fn nontrivial_residuals(&self) -> impl Iterator { + pub fn nontrivial_residuals(&self) -> impl Iterator { self.residuals.iter().filter_map(|(id, policy)| { if self.nontrivial_residuals.contains(id) { Some(policy) @@ -371,7 +371,7 @@ impl TryFrom for ResidualResponse { .collect(), residuals: partial_response .all_residuals() - .map(|e| e.to_json().map(|json| (e.id().to_smolstr(), json))) + .map(|e| e.to_json().map(|json| (e.id().to_smolstr(), json.into()))) .collect::>()?, }) } @@ -446,18 +446,18 @@ pub enum PartialAuthorizationAnswer { pub struct AuthorizationCall { /// The principal taking action #[cfg_attr(feature = "wasm", tsify(type = "{type: string, id: string}"))] - principal: Option, + principal: Option, /// The action the principal is taking #[cfg_attr(feature = "wasm", tsify(type = "{type: string, id: string}"))] - action: serde_json::Value, + action: JsonValueWithNoDuplicateKeys, /// The resource being acted on by the principal #[cfg_attr(feature = "wasm", tsify(type = "{type: string, id: string}"))] - resource: Option, + resource: Option, /// The context details specific to the request #[serde_as(as = "MapPreventDuplicates<_, _>")] #[cfg_attr(feature = "wasm", tsify(type = "Record"))] /// The context details specific to the request - context: HashMap, + context: HashMap, /// Optional schema. /// If present, this will inform the parsing: for instance, it will allow /// `__entity` and `__extn` escapes to be implicit, and it will error if @@ -481,11 +481,11 @@ fn constant_true() -> bool { /// Parses the given JSON into an [`EntityUid`], or if it fails, adds an /// appropriate error to `errs` and returns `None`. fn parse_entity_uid( - entity_uid_json: serde_json::Value, + entity_uid_json: JsonValueWithNoDuplicateKeys, category: &str, errs: &mut Vec, ) -> Option { - match EntityUid::from_json(entity_uid_json) + match EntityUid::from_json(entity_uid_json.into()) .wrap_err_with(|| format!("Failed to parse {category}")) { Ok(euid) => Some(euid), @@ -499,7 +499,7 @@ fn parse_entity_uid( /// Parses the given JSON context into a [`Context`], or if it fails, adds /// appropriate error(s) to `errs` and returns an empty context. fn parse_context( - context_map: HashMap, + context_map: HashMap, schema_ref: Option<&crate::Schema>, action_ref: Option<&EntityUid>, errs: &mut Vec, @@ -751,7 +751,7 @@ struct RecvdSlice { /// JSON object containing the entities data, in "natural JSON" form -- same /// format as expected by EntityJsonParser #[cfg_attr(feature = "wasm", tsify(type = "Array"))] - entities: serde_json::Value, + entities: JsonValueWithNoDuplicateKeys, /// Optional template policies. #[serde_as(as = "Option>")] @@ -818,7 +818,7 @@ impl RecvdSlice { crate::PolicySet::new() } }; - let entities = match Entities::from_json_value(entities, schema) { + let entities = match Entities::from_json_value(entities.into(), schema) { Ok(entities) => entities, Err(e) => { errs.push(miette::Report::new(e)); diff --git a/cedar-policy/src/ffi/utils.rs b/cedar-policy/src/ffi/utils.rs index 75b229b7c..9e689d29f 100644 --- a/cedar-policy/src/ffi/utils.rs +++ b/cedar-policy/src/ffi/utils.rs @@ -20,6 +20,11 @@ use miette::WrapErr; use serde::{Deserialize, Serialize}; use std::{collections::HashMap, str::FromStr}; +// Publicly expose the `JsonValueWithNoDuplicateKeys` type so that the +// `*_json_str` APIs will correctly error if the input JSON string contains +// duplicate keys. +pub use cedar_policy_core::jsonvalue::JsonValueWithNoDuplicateKeys; + #[cfg(feature = "wasm")] extern crate tsify; @@ -238,7 +243,7 @@ pub enum Schema { /// Schema in the Cedar schema format. See Human(String), /// Schema in Cedar's JSON schema format. See - Json(serde_json::Value), + Json(JsonValueWithNoDuplicateKeys), } impl Schema { @@ -254,7 +259,7 @@ impl Schema { ) }) .map_err(miette::Report::new), - Self::Json(val) => crate::Schema::from_json_value(val) + Self::Json(val) => crate::Schema::from_json_value(val.into()) .map(|sch| { ( sch, diff --git a/cedar-policy/src/ffi/validate.rs b/cedar-policy/src/ffi/validate.rs index d25090abd..1aa8d808d 100644 --- a/cedar-policy/src/ffi/validate.rs +++ b/cedar-policy/src/ffi/validate.rs @@ -499,7 +499,6 @@ mod test { } #[test] - // No special handling for duplicates in JSON strings (serde_json silently drops them) fn test_validate_fails_on_duplicate_namespace() { let json = r#"{ "schema": { "json": { @@ -509,10 +508,8 @@ mod test { "policies": "" }"#; - let ans_val = validate_json_str(json).unwrap(); - let result: Result = serde_json::from_str(ans_val.as_str()); - assert_matches!(result, Ok(ValidationAnswer::Success { validation_errors, validation_warnings: _, other_warnings: _ }) => { - assert_eq!(validation_errors.len(), 0, "Unexpected validation errors: {validation_errors:?}"); + assert_matches!(validate_json_str(json), Err(e) => { + assert!(e.to_string().contains("the key `foo` occurs two or more times in the same JSON object"), "actual error message was {e}"); }); } From 490aed08f8f0c7d2d82d2f4fadea4d4b75ddd776 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Mon, 3 Jun 2024 14:02:35 -0400 Subject: [PATCH 07/10] testing code nits --- cedar-testing/src/cedar_test_impl.rs | 5 +---- cedar-testing/src/integration_testing.rs | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/cedar-testing/src/cedar_test_impl.rs b/cedar-testing/src/cedar_test_impl.rs index a01663f59..e0e661c24 100644 --- a/cedar-testing/src/cedar_test_impl.rs +++ b/cedar-testing/src/cedar_test_impl.rs @@ -97,14 +97,11 @@ pub struct TestValidationResult { pub mod partial { use super::*; #[derive(Debug, Deserialize, PartialEq, Eq)] + #[serde(rename_all = "camelCase")] pub struct FlatPartialResponse { - #[serde(rename = "knownPermits")] pub known_permits: HashSet, - #[serde(rename = "knownForbids")] pub known_forbids: HashSet, - #[serde(rename = "determiningUnderApprox")] pub determining_under_approx: HashSet, - #[serde(rename = "determiningOverApprox")] pub determining_over_approx: HashSet, pub decision: Decision, } diff --git a/cedar-testing/src/integration_testing.rs b/cedar-testing/src/integration_testing.rs index 864f799e0..21cd4a2ed 100644 --- a/cedar-testing/src/integration_testing.rs +++ b/cedar-testing/src/integration_testing.rs @@ -41,11 +41,11 @@ use std::{ #[serde(deny_unknown_fields)] #[serde(rename_all = "camelCase")] pub struct JsonTest { - /// Filename of the policies to use (in pure Cedar syntax) + /// Filename of the policy set (in Cedar syntax) pub policies: String, /// Filename of a JSON file representing the entity hierarchy pub entities: String, - /// Filename of a JSON file containing the schema. + /// Filename of the schema (in Cedar syntax) pub schema: String, /// Whether the given policies are expected to pass the validator with this /// schema, or not From b7d1cb8cbaaf916658c5f8b9e0240d1d43e9ba2b Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Mon, 3 Jun 2024 15:01:21 -0400 Subject: [PATCH 08/10] switch to PolicyId --- cedar-policy/src/ffi/is_authorized.rs | 59 +++++++++++++-------------- cedar-policy/src/ffi/validate.rs | 9 ++-- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/cedar-policy/src/ffi/is_authorized.rs b/cedar-policy/src/ffi/is_authorized.rs index 804b49d9a..c6106bc2c 100644 --- a/cedar-policy/src/ffi/is_authorized.rs +++ b/cedar-policy/src/ffi/is_authorized.rs @@ -26,7 +26,7 @@ use itertools::Itertools; use miette::{miette, Diagnostic, WrapErr}; use serde::{Deserialize, Serialize}; use serde_with::{serde_as, MapPreventDuplicates}; -use smol_str::{SmolStr, ToSmolStr}; +use smol_str::ToSmolStr; use std::collections::{HashMap, HashSet}; #[cfg(feature = "partial-eval")] use std::convert::Infallible; @@ -156,7 +156,7 @@ pub struct Diagnostics { /// Ids of the policies that contributed to the decision. /// If no policies applied to the request, this set will be empty. #[cfg_attr(feature = "wasm", tsify(type = "Set"))] - reason: HashSet, + reason: HashSet, /// Set of errors that occurred errors: HashSet, } @@ -165,7 +165,7 @@ impl Response { /// Construct a `Response` pub fn new( decision: Decision, - reason: HashSet, + reason: HashSet, errors: HashSet, ) -> Self { Self { @@ -190,7 +190,7 @@ impl From for Response { let (reason, errors) = response.diagnostics.into_components(); Self::new( response.decision, - reason.map(PolicyId::into_smolstr).collect(), + reason.collect(), errors.map(Into::into).collect(), ) } @@ -207,7 +207,7 @@ impl TryFrom for Response { impl Diagnostics { /// Get the policies that contributed to the decision - pub fn reason(&self) -> impl Iterator { + pub fn reason(&self) -> impl Iterator { self.reason.iter() } @@ -224,7 +224,7 @@ impl Diagnostics { #[serde(rename_all = "camelCase")] pub struct AuthorizationError { /// Id of the policy where the error (or warning) occurred - pub policy_id: SmolStr, + pub policy_id: PolicyId, /// Error (or warning). /// You can look at the `severity` field to see whether it is actually an /// error or a warning. @@ -234,14 +234,14 @@ pub struct AuthorizationError { impl AuthorizationError { /// Create an `AuthorizationError` from a policy ID and any `miette` error pub fn new( - policy_id: impl Into, + policy_id: impl Into, error: impl miette::Diagnostic + Send + Sync + 'static, ) -> Self { Self::new_from_report(policy_id, miette::Report::new(error)) } /// Create an `AuthorizationError` from a policy ID and a `miette::Report` - pub fn new_from_report(policy_id: impl Into, report: miette::Report) -> Self { + pub fn new_from_report(policy_id: impl Into, report: miette::Report) -> Self { Self { policy_id: policy_id.into(), error: report.into(), @@ -275,12 +275,12 @@ impl From for AuthorizationEr #[serde(rename_all = "camelCase")] pub struct ResidualResponse { decision: Option, - satisfied: HashSet, - errored: HashSet, - may_be_determining: HashSet, - must_be_determining: HashSet, - residuals: HashMap, - nontrivial_residuals: HashSet, + satisfied: HashSet, + errored: HashSet, + may_be_determining: HashSet, + must_be_determining: HashSet, + residuals: HashMap, + nontrivial_residuals: HashSet, } #[cfg(feature = "partial-eval")] @@ -291,22 +291,22 @@ impl ResidualResponse { } /// Set of all satisfied policy Ids - pub fn satisfied(&self) -> impl Iterator { + pub fn satisfied(&self) -> impl Iterator { self.satisfied.iter() } /// Set of all policy ids for policies that errored - pub fn errored(&self) -> impl Iterator { + pub fn errored(&self) -> impl Iterator { self.errored.iter() } /// Over approximation of policies that determine the auth decision - pub fn may_be_determining(&self) -> impl Iterator { + pub fn may_be_determining(&self) -> impl Iterator { self.may_be_determining.iter() } /// Under approximation of policies that determine the auth decision - pub fn must_be_determining(&self) -> impl Iterator { + pub fn must_be_determining(&self) -> impl Iterator { self.must_be_determining.iter() } @@ -321,7 +321,7 @@ impl ResidualResponse { } /// Get the residual policy for a specified id if it exists - pub fn residual(&self, p: &SmolStr) -> Option<&JsonValueWithNoDuplicateKeys> { + pub fn residual(&self, p: &PolicyId) -> Option<&JsonValueWithNoDuplicateKeys> { self.residuals.get(p) } @@ -337,7 +337,7 @@ impl ResidualResponse { } /// Iterator over the set of non-trivial residual policy ids - pub fn nontrivial_residual_ids(&self) -> impl Iterator { + pub fn nontrivial_residual_ids(&self) -> impl Iterator { self.nontrivial_residuals.iter() } } @@ -351,27 +351,24 @@ impl TryFrom for ResidualResponse { decision: partial_response.decision(), satisfied: partial_response .definitely_satisfied() - .map(|p| p.id().to_smolstr()) - .collect(), - errored: partial_response - .definitely_errored() - .map(ToSmolStr::to_smolstr) + .map(|p| p.id().clone()) .collect(), + errored: partial_response.definitely_errored().cloned().collect(), may_be_determining: partial_response .may_be_determining() - .map(|p| p.id().to_smolstr()) + .map(|p| p.id().clone()) .collect(), must_be_determining: partial_response .must_be_determining() - .map(|p| p.id().to_smolstr()) + .map(|p| p.id().clone()) .collect(), nontrivial_residuals: partial_response .nontrivial_residuals() - .map(|p| p.id().to_smolstr()) + .map(|p| p.id().clone()) .collect(), residuals: partial_response .all_residuals() - .map(|e| e.to_json().map(|json| (e.id().to_smolstr(), json.into()))) + .map(|e| e.to_json().map(|json| (e.id().clone(), json.into()))) .collect::>()?, }) } @@ -469,7 +466,7 @@ pub struct AuthorizationCall { /// parsing of `context`, and not for request validation. /// If a schema is not provided, this option has no effect. #[serde(default = "constant_true")] - enable_request_validation: bool, + validate_request: bool, /// The slice containing entities and policies slice: RecvdSlice, } @@ -558,7 +555,7 @@ impl AuthorizationCall { action, resource, context, - if self.enable_request_validation { + if self.validate_request { schema.as_ref() } else { None diff --git a/cedar-policy/src/ffi/validate.rs b/cedar-policy/src/ffi/validate.rs index 1aa8d808d..741844627 100644 --- a/cedar-policy/src/ffi/validate.rs +++ b/cedar-policy/src/ffi/validate.rs @@ -18,9 +18,8 @@ //! call #![allow(clippy::module_name_repetitions)] use super::utils::{DetailedError, PolicySet, Schema, WithWarnings}; -use crate::{ValidationMode, Validator}; +use crate::{PolicyId, ValidationMode, Validator}; use serde::{Deserialize, Serialize}; -use smol_str::{SmolStr, ToSmolStr}; #[cfg(feature = "wasm")] extern crate tsify; @@ -50,13 +49,13 @@ pub fn validate(call: ValidationCall) -> ValidationAnswer { .into_errors_and_warnings(); let validation_errors: Vec = validation_errors .map(|error| ValidationError { - policy_id: error.policy_id().to_smolstr(), + policy_id: *error.policy_id(), error: miette::Report::new(error).into(), }) .collect(); let validation_warnings: Vec = validation_warnings .map(|error| ValidationError { - policy_id: error.policy_id().to_smolstr(), + policy_id: *error.policy_id(), error: miette::Report::new(error).into(), }) .collect(); @@ -169,7 +168,7 @@ impl Default for ValidationSettings { #[serde(rename_all = "camelCase")] pub struct ValidationError { /// Id of the policy where the error (or warning) occurred - pub policy_id: SmolStr, + pub policy_id: PolicyId, /// Error (or warning) itself. /// You can look at the `severity` field to see whether it is actually an /// error or a warning. From 3cfc30e56f5fed06cef13ad7ea82d77b600b0438 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Mon, 3 Jun 2024 15:37:39 -0400 Subject: [PATCH 09/10] switch back to PolicyId --- cedar-policy-core/src/ast/policy.rs | 5 ----- cedar-policy/src/api/id.rs | 5 ----- cedar-policy/src/ffi/is_authorized.rs | 15 +++++++-------- cedar-policy/src/ffi/validate.rs | 4 ++-- cedar-testing/src/cedar_test_impl.rs | 9 ++------- cedar-testing/src/integration_testing.rs | 10 ++-------- cedar-testing/tests/cedar-policy-cli/main.rs | 2 +- 7 files changed, 14 insertions(+), 36 deletions(-) diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 21f3bc0a0..753a1d61c 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -1718,11 +1718,6 @@ impl PolicyID { pub fn from_smolstr(id: SmolStr) -> Self { Self(id) } - - /// Get the underlying `SmolStr` - pub fn into_smolstr(self) -> SmolStr { - self.0 - } } impl std::fmt::Display for PolicyID { diff --git a/cedar-policy/src/api/id.rs b/cedar-policy/src/api/id.rs index ac6a8a242..3e31fa314 100644 --- a/cedar-policy/src/api/id.rs +++ b/cedar-policy/src/api/id.rs @@ -325,11 +325,6 @@ impl PolicyId { pub fn new(id: impl AsRef) -> Self { Self(ast::PolicyID::from_string(id.as_ref())) } - - /// Get the underlying `SmolStr` - pub(crate) fn into_smolstr(self) -> SmolStr { - self.0.into_smolstr() - } } impl FromStr for PolicyId { diff --git a/cedar-policy/src/ffi/is_authorized.rs b/cedar-policy/src/ffi/is_authorized.rs index c6106bc2c..990faf8bc 100644 --- a/cedar-policy/src/ffi/is_authorized.rs +++ b/cedar-policy/src/ffi/is_authorized.rs @@ -17,7 +17,7 @@ //! This module contains the `is_authorized` entry points that other language //! FFIs can call #![allow(clippy::module_name_repetitions)] -use super::utils::*; +use super::utils::{DetailedError, JsonValueWithNoDuplicateKeys, PolicySet, Schema, WithWarnings}; use crate::{ Authorizer, Context, Decision, Entities, EntityId, EntityTypeName, EntityUid, PolicyId, Request, SlotId, @@ -26,7 +26,6 @@ use itertools::Itertools; use miette::{miette, Diagnostic, WrapErr}; use serde::{Deserialize, Serialize}; use serde_with::{serde_as, MapPreventDuplicates}; -use smol_str::ToSmolStr; use std::collections::{HashMap, HashSet}; #[cfg(feature = "partial-eval")] use std::convert::Infallible; @@ -253,7 +252,7 @@ impl From for AuthorizationError { fn from(e: crate::AuthorizationError) -> Self { match e { crate::AuthorizationError::PolicyEvaluationError(e) => { - Self::new(e.id().to_smolstr(), e.into_inner()) + Self::new(e.id().clone(), e.into_inner()) } } } @@ -624,7 +623,7 @@ impl AuthorizationCall { b = b.resource(resource); } b = b.context(context); - let request = if self.enable_request_validation { + let request = if self.validate_request { match schema.as_ref() { Some(schema_ref) => match b.schema(schema_ref).build() { Ok(req) => Some(req), @@ -1856,7 +1855,7 @@ mod test { "schema": { "human": "entity User, Photo; action view appliesTo { principal: User, resource: Photo };" }, - "enableRequestValidation": false, + "validateRequest": false, }); assert_is_authorized_json(good_call); @@ -1905,12 +1904,12 @@ mod partial_test { assert_eq!(response.decision(), None); let errors: Vec<_> = response.errored().collect(); assert_eq!(errors.len(), 0, "{errors:?}"); - let actual_residuals: HashSet<&str> = response.nontrivial_residual_ids().map(|id| id.as_str()).collect(); + let actual_residuals: HashSet<_> = response.nontrivial_residual_ids().collect(); for id in &expected_residuals { - assert!(actual_residuals.contains(id), "expected nontrivial residual for {id}, but it's missing") + assert!(actual_residuals.contains(&PolicyId::from_str(id).ok().unwrap()), "expected nontrivial residual for {id}, but it's missing") } for id in &actual_residuals { - assert!(expected_residuals.contains(id), "found unexpected nontrivial residual for {id}") + assert!(expected_residuals.contains(id.to_string().as_str()),"found unexpected nontrivial residual for {id}") } }); } diff --git a/cedar-policy/src/ffi/validate.rs b/cedar-policy/src/ffi/validate.rs index 741844627..ba31292ef 100644 --- a/cedar-policy/src/ffi/validate.rs +++ b/cedar-policy/src/ffi/validate.rs @@ -49,13 +49,13 @@ pub fn validate(call: ValidationCall) -> ValidationAnswer { .into_errors_and_warnings(); let validation_errors: Vec = validation_errors .map(|error| ValidationError { - policy_id: *error.policy_id(), + policy_id: error.policy_id().clone(), error: miette::Report::new(error).into(), }) .collect(); let validation_warnings: Vec = validation_warnings .map(|error| ValidationError { - policy_id: *error.policy_id(), + policy_id: error.policy_id().clone(), error: miette::Report::new(error).into(), }) .collect(); diff --git a/cedar-testing/src/cedar_test_impl.rs b/cedar-testing/src/cedar_test_impl.rs index e0e661c24..8c935d2c3 100644 --- a/cedar-testing/src/cedar_test_impl.rs +++ b/cedar-testing/src/cedar_test_impl.rs @@ -29,7 +29,6 @@ use cedar_policy_core::extensions::Extensions; use cedar_policy_validator::{ValidationMode, Validator, ValidatorSchema}; use miette::miette; use serde::{Deserialize, Serialize}; -use smol_str::ToSmolStr; use std::collections::HashMap; use std::collections::HashSet; use std::time::{Duration, Instant}; @@ -282,11 +281,7 @@ impl CedarTestImplementation for RustEngine { let response = cedar_policy::Response::from(response); let response = ffi::Response::new( response.decision(), - response - .diagnostics() - .reason() - .map(|id| id.to_smolstr()) - .collect(), + response.diagnostics().reason().cloned().collect(), response .diagnostics() .errors() @@ -295,7 +290,7 @@ impl CedarTestImplementation for RustEngine { // `ErrorComparisonMode::PolicyIds` mode. let policy_id = e.id(); ffi::AuthorizationError::new_from_report( - policy_id.to_smolstr(), + policy_id.clone(), miette!("{policy_id}"), ) }) diff --git a/cedar-testing/src/integration_testing.rs b/cedar-testing/src/integration_testing.rs index 21cd4a2ed..b676416c5 100644 --- a/cedar-testing/src/integration_testing.rs +++ b/cedar-testing/src/integration_testing.rs @@ -33,7 +33,6 @@ use std::{ collections::HashSet, env, path::{Path, PathBuf}, - str::FromStr, }; /// JSON representation of our integration test file format @@ -323,12 +322,7 @@ pub fn perform_integration_test( &json_request.description ); // check reason - let reason: HashSet = response - .response - .diagnostics() - .reason() - .map(|id| PolicyId::from_str(id).unwrap()) - .collect(); + let reason: HashSet = response.response.diagnostics().reason().cloned().collect(); assert_eq!( reason, json_request.reason.into_iter().collect(), @@ -345,7 +339,7 @@ pub fn perform_integration_test( .response .diagnostics() .errors() - .map(|err| PolicyId::from_str(&err.policy_id).unwrap()) + .map(|err| err.policy_id.clone()) .collect(); assert_eq!( errors, diff --git a/cedar-testing/tests/cedar-policy-cli/main.rs b/cedar-testing/tests/cedar-policy-cli/main.rs index 0b1328c75..c6984a864 100644 --- a/cedar-testing/tests/cedar-policy-cli/main.rs +++ b/cedar-testing/tests/cedar-policy-cli/main.rs @@ -145,7 +145,7 @@ fn perform_integration_test_from_json(jsonfile: impl AsRef) { entity_args.push("--action".to_string()); entity_args.push(value_to_euid_string(s.into()).unwrap()); } - if !json_request.enable_request_validation { + if !json_request.validate_request { entity_args.push("--request-validation=false".to_string()); } From 6bd39c37db4a45657a31c9c4ea547f2348355c88 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Mon, 3 Jun 2024 16:54:19 -0400 Subject: [PATCH 10/10] add some tisfy annotations --- cedar-policy/src/ffi/is_authorized.rs | 1 + cedar-policy/src/ffi/validate.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/cedar-policy/src/ffi/is_authorized.rs b/cedar-policy/src/ffi/is_authorized.rs index 990faf8bc..135b1ca5a 100644 --- a/cedar-policy/src/ffi/is_authorized.rs +++ b/cedar-policy/src/ffi/is_authorized.rs @@ -223,6 +223,7 @@ impl Diagnostics { #[serde(rename_all = "camelCase")] pub struct AuthorizationError { /// Id of the policy where the error (or warning) occurred + #[cfg_attr(feature = "wasm", tsify(type = "string"))] pub policy_id: PolicyId, /// Error (or warning). /// You can look at the `severity` field to see whether it is actually an diff --git a/cedar-policy/src/ffi/validate.rs b/cedar-policy/src/ffi/validate.rs index ba31292ef..94bfaa0dd 100644 --- a/cedar-policy/src/ffi/validate.rs +++ b/cedar-policy/src/ffi/validate.rs @@ -168,6 +168,7 @@ impl Default for ValidationSettings { #[serde(rename_all = "camelCase")] pub struct ValidationError { /// Id of the policy where the error (or warning) occurred + #[cfg_attr(feature = "wasm", tsify(type = "string"))] pub policy_id: PolicyId, /// Error (or warning) itself. /// You can look at the `severity` field to see whether it is actually an