From 7ba7c2590eed1efa65a4a7820c288e7cfd45c087 Mon Sep 17 00:00:00 2001 From: cole foos <33300547+foosiee@users.noreply.github.com> Date: Mon, 26 Feb 2024 11:56:15 -0500 Subject: [PATCH] Add better provider parsing errors messages (#68) * #67 Add cause of provider error in wrapper error * Update changelog * Fix failing test and semvar refactor to avoid major version upgrade --- CHANGELOG.md | 1 + src/public/file/entity_provider.rs | 73 +++++++++++++++++--------- src/public/file/policy_set_provider.rs | 52 ++++++++++++------ 3 files changed, 87 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b7b497..9812ddc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased - Added derive Clone to RefreshRate struct. +- Add cause for entity and policy provider errors ### Added diff --git a/src/public/file/entity_provider.rs b/src/public/file/entity_provider.rs index 90ef17a..2fa3dcf 100644 --- a/src/public/file/entity_provider.rs +++ b/src/public/file/entity_provider.rs @@ -20,7 +20,7 @@ use std::io::Error; use std::sync::Arc; use async_trait::async_trait; -use cedar_policy::{Entities, Request, Schema}; +use cedar_policy::{Entities, EntitiesError, Request, Schema}; use derive_builder::Builder; use thiserror::Error; use tokio::sync::RwLock; @@ -75,7 +75,7 @@ pub enum ProviderError { #[error("The Schema file failed to be parsed at path: {0}")] SchemaParseError(String), /// Entities file is malformed in some way - #[error("The Entities failed to be parsed at path: {0}")] + #[error("{0}")] EntitiesError(String), } @@ -83,6 +83,7 @@ pub enum ProviderError { struct EntitiesErrorWrapper { // This is the path to the file to load entities entity_file_path: String, + cause: EntitiesError, } /// A wrapper that wraps `SchemaParseError` to map the error message @@ -94,10 +95,11 @@ struct SchemaParseErrorWrapper { /// Implements the constructor for the `EntitiesErrorWrapper`. impl EntitiesErrorWrapper { /// Creates a new wrapper of the `EntitiesError` - fn new(entity_file_path: String) -> Self { + fn new(entity_file_path: String, cause: EntitiesError) -> Self { Self { // This is the path to the file to load entities. entity_file_path, + cause, } } } @@ -127,10 +129,17 @@ impl From for ProviderError { } } +fn create_entity_error_message(value: &EntitiesErrorWrapper) -> String { + format!( + "The Entities failed to be parsed at path: {}. Cause: {}", + value.entity_file_path, value.cause + ) +} + /// Map the `EntitiesErrorWrapper` to the `ProvideError::EntitiesError` with the file path impl From for ProviderError { fn from(value: EntitiesErrorWrapper) -> Self { - Self::EntitiesError(value.entity_file_path) + Self::EntitiesError(create_entity_error_message(&value)) } } @@ -165,16 +174,17 @@ impl EntityProvider { let schema_file = File::open(schema_path)?; let schema = Schema::from_file(schema_file) .map_err(|_schema_error| SchemaParseErrorWrapper::new(schema_path.clone()))?; - let res = Entities::from_json_file(entities_file, Some(&schema)) - .map_err(|_entities_error| EntitiesErrorWrapper::new(entities_path.clone()))?; + let res = Entities::from_json_file(entities_file, Some(&schema)).map_err( + |entities_error| { + EntitiesErrorWrapper::new(entities_path.clone(), entities_error) + }, + )?; debug!("Fetched Entities from file with Schema: entities_file_path={entities_path:?}: schema_file_path={schema_path:?}"); res } else { let res = - Entities::from_json_file(entities_file, None).map_err(|_entities_error| { - EntitiesErrorWrapper { - entity_file_path: entities_path.clone(), - } + Entities::from_json_file(entities_file, None).map_err(|entities_error| { + EntitiesErrorWrapper::new(entities_path.clone(), entities_error) })?; debug!("Fetched Entities from file: entities_file_path={entities_path:?}"); res @@ -222,19 +232,22 @@ impl UpdateProviderData for EntityProvider { schema_path.to_string(), ))) })?; - let res = Entities::from_json_file(entities_file, Some(&schema)).map_err(|_| { - UpdateProviderDataError::General(Box::new(ProviderError::EntitiesError( - entities_path.to_string(), - ))) - })?; + let res = Entities::from_json_file(entities_file, Some(&schema)).map_err( + |entities_error| { + UpdateProviderDataError::General(Box::new(ProviderError::from( + EntitiesErrorWrapper::new(entities_path.clone(), entities_error), + ))) + }, + )?; debug!("Updated Entities from file with Schema: entities_file_path={entities_path:?}: schema_file_path={schema_path:?}"); res } else { - let res = Entities::from_json_file(entities_file, None).map_err(|_| { - UpdateProviderDataError::General(Box::new(ProviderError::EntitiesError( - entities_path.to_string(), - ))) - })?; + let res = + Entities::from_json_file(entities_file, None).map_err(|entities_error| { + UpdateProviderDataError::General(Box::new(ProviderError::from( + EntitiesErrorWrapper::new(entities_path.clone(), entities_error), + ))) + })?; debug!("Updated Entities from file: entities_file_path={entities_path:?}"); res }; @@ -267,12 +280,16 @@ impl SimpleEntityProvider for EntityProvider { #[cfg(test)] mod test { - use cedar_policy::{Context, Request}; + use cedar_policy::{Context, Entities, Request}; + use std::fs::File; use std::io::ErrorKind; use std::{fs, io}; use tempfile::NamedTempFile; - use crate::public::file::entity_provider::{ConfigBuilder, EntityProvider, ProviderError}; + use crate::public::file::entity_provider::{ + create_entity_error_message, ConfigBuilder, EntitiesErrorWrapper, EntityProvider, + ProviderError, + }; use crate::public::{SimpleEntityProvider, UpdateProviderData, UpdateProviderDataError}; #[test] @@ -382,17 +399,25 @@ mod test { #[test] fn entity_provider_malformed_entities() { + let entities_path = "tests/data/malformed_entities.json"; + let entities_file = File::open(entities_path).unwrap(); let error = EntityProvider::new( ConfigBuilder::default() - .entities_path("tests/data/malformed_entities.json") + .entities_path(entities_path) .build() .unwrap(), ); + let cedar_error = Entities::from_json_file(entities_file, None); assert!(error.is_err()); + assert!(cedar_error.is_err()); + assert_eq!( error.err().unwrap().to_string(), - "The Entities failed to be parsed at path: tests/data/malformed_entities.json" + create_entity_error_message(&EntitiesErrorWrapper::new( + entities_path.into(), + cedar_error.unwrap_err() + )) ); } diff --git a/src/public/file/policy_set_provider.rs b/src/public/file/policy_set_provider.rs index 7569c45..c7c6742 100644 --- a/src/public/file/policy_set_provider.rs +++ b/src/public/file/policy_set_provider.rs @@ -20,7 +20,7 @@ use std::str::FromStr; use std::sync::Arc; use async_trait::async_trait; -use cedar_policy::{PolicySet, Request}; +use cedar_policy::{ParseErrors, PolicySet, Request}; use derive_builder::Builder; use thiserror::Error; use tokio::sync::RwLock; @@ -63,7 +63,7 @@ pub struct PolicySetProvider { #[derive(Error, Debug)] pub enum ProviderError { /// Policy set file is malformed in some way - #[error("The Policy Set failed to be parsed at path: {0}")] + #[error("{0}")] PolicySetParseError(String), /// Can't read from disk or find the file #[error("IO Error: {0}")] @@ -74,15 +74,17 @@ pub enum ProviderError { struct ParseErrorWrapper { // This is the path to the file to load the policy set policy_set_path: String, + cause: ParseErrors, } /// Implements the constructor for the `ParseErrorWrapper`. impl ParseErrorWrapper { /// Creates a new wrapper of the `ParseErrors` - fn new(policy_set_path: String) -> Self { + fn new(policy_set_path: String, cause: ParseErrors) -> Self { Self { // This is the path to the file to load the policy set policy_set_path, + cause, } } } @@ -94,10 +96,17 @@ impl From for ProviderError { } } +fn create_policy_parse_error_message(value: &ParseErrorWrapper) -> String { + format!( + "The Policy Set failed to be parsed at path: {}. Cause: {}", + value.policy_set_path, value.cause + ) +} + /// Map the `ParseErrorWrapper` to the `ProviderError::PolicySetParseError` with the file path impl From for ProviderError { fn from(value: ParseErrorWrapper) -> Self { - Self::PolicySetParseError(value.policy_set_path) + Self::PolicySetParseError(create_policy_parse_error_message(&value)) } } @@ -124,8 +133,9 @@ impl PolicySetProvider { pub fn new(configuration: Config) -> Result { let policy_set_path = configuration.policy_set_path; let policy_set_src = std::fs::read_to_string(Path::new(policy_set_path.as_str()))?; - let policy_set = PolicySet::from_str(&policy_set_src) - .map_err(|_parse_errors| ParseErrorWrapper::new(policy_set_path.clone()))?; + let policy_set = PolicySet::from_str(&policy_set_src).map_err(|parse_errors| { + ParseErrorWrapper::new(policy_set_path.clone(), parse_errors) + })?; let policy_ids = policy_set .policies() .map(cedar_policy::Policy::id) @@ -147,11 +157,12 @@ impl UpdateProviderData for PolicySetProvider { let policy_set_path = self.policy_set_path.clone(); let policy_set_src = std::fs::read_to_string(Path::new(policy_set_path.as_str())) .map_err(|e| UpdateProviderDataError::General(Box::new(ProviderError::IOError(e))))?; - let policy_set = PolicySet::from_str(&policy_set_src).map_err(|_| { - UpdateProviderDataError::General(Box::new(ProviderError::PolicySetParseError( - policy_set_path.clone(), - ))) - })?; + let policy_set = + PolicySet::from_str(&policy_set_src).map_err(|parse_error: ParseErrors| { + UpdateProviderDataError::General(Box::new(ProviderError::from( + ParseErrorWrapper::new(policy_set_path.clone(), parse_error), + ))) + })?; { let mut policy_set_guard = self.policy_set.write().await; *policy_set_guard = Arc::new(policy_set.clone()); @@ -180,14 +191,17 @@ impl SimplePolicySetProvider for PolicySetProvider { #[cfg(test)] mod test { - use cedar_policy::{Context, Request}; + use cedar_policy::{Context, PolicySet, Request}; use std::io::ErrorKind; + use std::path::Path; + use std::str::FromStr; use std::{fs, io}; use tempfile::NamedTempFile; use crate::public::file::policy_set_provider::ProviderError::PolicySetParseError; use crate::public::file::policy_set_provider::{ - ConfigBuilder, PolicySetProvider, ProviderError, + create_policy_parse_error_message, ConfigBuilder, ParseErrorWrapper, PolicySetProvider, + ProviderError, }; use crate::public::{SimplePolicySetProvider, UpdateProviderData, UpdateProviderDataError}; #[test] @@ -219,17 +233,25 @@ mod test { #[test] fn simple_policy_provider_is_malformed() { + let policy_path = "tests/data/malformed_policies.cedar"; + let policy_set_src = std::fs::read_to_string(Path::new(policy_path)).unwrap(); let error = PolicySetProvider::new( ConfigBuilder::default() - .policy_set_path("tests/data/malformed_policies.cedar") + .policy_set_path(policy_path) .build() .unwrap(), ); + let cedar_error = PolicySet::from_str(&policy_set_src); + assert!(error.is_err()); + assert!(cedar_error.is_err()); assert_eq!( error.err().unwrap().to_string(), - "The Policy Set failed to be parsed at path: tests/data/malformed_policies.cedar" + create_policy_parse_error_message(&ParseErrorWrapper::new( + policy_path.into(), + cedar_error.unwrap_err() + )) ); }