Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(config): re-enable config pointers with a new whitelist #1775

Closed
wants to merge 1 commit into from
Closed

chore(config): re-enable config pointers with a new whitelist #1775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 100 additions & 16 deletions crates/papyrus_config/src/config_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashSet};
use std::env;
use std::fs::File;
use std::path::PathBuf;
Expand Down Expand Up @@ -257,24 +257,41 @@ fn test_pointers_flow() {
const POINTING_PARAM_DESCRIPTION: &str = "This is a.";
const PUBLIC_POINTING_PARAM_NAME: &str = "public_a.a";
const PRIVATE_POINTING_PARAM_NAME: &str = "private_a.a";
const WHITELISTED_POINTING_PARAM_NAME: &str = "non_pointing.a";
const VALUE: usize = 5;

let config_map = BTreeMap::from([
ser_param(
PUBLIC_POINTING_PARAM_NAME,
&json!(5),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Public,
),
ser_param(
PRIVATE_POINTING_PARAM_NAME,
&json!(5),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
ser_param(
WHITELISTED_POINTING_PARAM_NAME,
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
]);
let pointers =
vec![ser_pointer_target_param(TARGET_PARAM_NAME, &json!(10), TARGET_PARAM_DESCRIPTION)];
let stored_map = combine_config_map_and_pointers(config_map, &pointers).unwrap();
let pointers = vec![(
ser_pointer_target_param(TARGET_PARAM_NAME, &json!(10), TARGET_PARAM_DESCRIPTION),
HashSet::from([
PUBLIC_POINTING_PARAM_NAME.to_string(),
PRIVATE_POINTING_PARAM_NAME.to_string(),
]),
)];
let non_pointer_params = HashSet::from([WHITELISTED_POINTING_PARAM_NAME.to_string()]);
let stored_map =
combine_config_map_and_pointers(config_map, &pointers, &non_pointer_params).unwrap();

// Assert the pointing parameters are correctly set.
assert_eq!(
stored_map[PUBLIC_POINTING_PARAM_NAME],
json!(SerializedParam {
Expand All @@ -291,6 +308,18 @@ fn test_pointers_flow() {
privacy: ParamPrivacy::Private,
})
);

// Assert the whitelisted parameter is correctly set.
assert_eq!(
stored_map[WHITELISTED_POINTING_PARAM_NAME],
json!(SerializedParam {
description: POINTING_PARAM_DESCRIPTION.to_owned(),
content: SerializedContent::DefaultValue(json!(VALUE)),
privacy: ParamPrivacy::Private,
})
);

// Assert the pointed parameter is correctly set as a required parameter.
assert_eq!(
stored_map[TARGET_PARAM_NAME],
json!(SerializedParam {
Expand All @@ -316,27 +345,43 @@ fn test_required_pointers_flow() {
const POINTING_PARAM_DESCRIPTION: &str = "This is b.";
const PUBLIC_POINTING_PARAM_NAME: &str = "public_b.b";
const PRIVATE_POINTING_PARAM_NAME: &str = "private_b.b";
const WHITELISTED_POINTING_PARAM_NAME: &str = "non_pointing.b";
const VALUE: usize = 6;

let config_map = BTreeMap::from([
ser_param(
PUBLIC_POINTING_PARAM_NAME,
&json!(6),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Public,
),
ser_param(
PRIVATE_POINTING_PARAM_NAME,
&json!(6),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
ser_param(
WHITELISTED_POINTING_PARAM_NAME,
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
]);
let pointers = vec![ser_pointer_target_required_param(
REQUIRED_PARAM_NAME,
SerializationType::PositiveInteger,
REQUIRED_PARAM_DESCRIPTION,
let pointers = vec![(
ser_pointer_target_required_param(
REQUIRED_PARAM_NAME,
SerializationType::PositiveInteger,
REQUIRED_PARAM_DESCRIPTION,
),
HashSet::from([
PUBLIC_POINTING_PARAM_NAME.to_string(),
PRIVATE_POINTING_PARAM_NAME.to_string(),
]),
)];
let stored_map = combine_config_map_and_pointers(config_map, &pointers).unwrap();
let non_pointer_params = HashSet::from([WHITELISTED_POINTING_PARAM_NAME.to_string()]);
let stored_map =
combine_config_map_and_pointers(config_map, &pointers, &non_pointer_params).unwrap();

// Assert the pointing parameters are correctly set.
assert_eq!(
Expand All @@ -356,6 +401,16 @@ fn test_required_pointers_flow() {
})
);

// Assert the whitelisted parameter is correctly set.
assert_eq!(
stored_map[WHITELISTED_POINTING_PARAM_NAME],
json!(SerializedParam {
description: POINTING_PARAM_DESCRIPTION.to_owned(),
content: SerializedContent::DefaultValue(json!(VALUE)),
privacy: ParamPrivacy::Private,
})
);

// Assert the pointed parameter is correctly set as a required parameter.
assert_eq!(
stored_map[REQUIRED_PARAM_NAME],
Expand All @@ -367,6 +422,35 @@ fn test_required_pointers_flow() {
);
}

#[test]
#[should_panic(
expected = "The target param should_be_pointing.c should point to c, or to be whitelisted."
)]
fn test_missing_pointer_flow() {
const TARGET_PARAM_NAME: &str = "c";
const TARGET_PARAM_DESCRIPTION: &str = "This is common c.";
const PARAM_DESCRIPTION: &str = "This is c.";
const NON_POINTING_PARAM_NAME: &str = "should_be_pointing.c";

// Define a non-pointing parameter and a target pointer such that the parameter name matches the
// target.
let config_map = BTreeMap::from([ser_param(
NON_POINTING_PARAM_NAME,
&json!(7),
PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
)]);
let pointers = vec![(
ser_pointer_target_param(TARGET_PARAM_NAME, &json!(10), TARGET_PARAM_DESCRIPTION),
HashSet::new(),
)];
// Do not whitelist the non-pointing parameter.
let non_pointer_params = HashSet::new();

// Attempt to combine the config map and pointers. This should panic.
combine_config_map_and_pointers(config_map, &pointers, &non_pointer_params).unwrap();
}

#[test]
fn test_replace_pointers() {
let (mut config_map, _) = split_values_and_types(BTreeMap::from([ser_param(
Expand Down Expand Up @@ -416,7 +500,7 @@ fn load_custom_config(args: Vec<&str>) -> CustomConfig {
let dir = TempDir::new().unwrap();
let file_path = dir.path().join("config.json");
CustomConfig { param_path: "default value".to_owned(), seed: 5 }
.dump_to_file(&vec![], file_path.to_str().unwrap())
.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap())
.unwrap();

load_and_process_config::<CustomConfig>(
Expand Down Expand Up @@ -506,7 +590,7 @@ fn load_required_param_path(args: Vec<&str>) -> String {
let dir = TempDir::new().unwrap();
let file_path = dir.path().join("config.json");
RequiredConfig { param_path: "default value".to_owned(), num: 3 }
.dump_to_file(&vec![], file_path.to_str().unwrap())
.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap())
.unwrap();

let loaded_config = load_and_process_config::<CustomConfig>(
Expand Down Expand Up @@ -600,7 +684,7 @@ fn deeply_nested_optionals() {
let dir = TempDir::new().unwrap();
let file_path = dir.path().join("config2.json");
Level0 { level0_value: 1, level1: None }
.dump_to_file(&vec![], file_path.to_str().unwrap())
.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap())
.unwrap();

let l0 = load_and_process_config::<Level0>(
Expand Down
78 changes: 60 additions & 18 deletions crates/papyrus_config/src/dumping.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
//! }
//! ```

use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashSet};
use std::fs::File;
use std::io::{BufWriter, Write};

Expand All @@ -54,6 +54,10 @@ use crate::{
IS_NONE_MARK,
};

// TODO(Tsabary): introduce sub-types, and replace throughout.
/// Detailing pointers in the config map.
pub type ConfigPointers = Vec<((ParamPath, SerializedParam), HashSet<ParamPath>)>;

/// Serialization for configs.
pub trait SerializeConfig {
/// Conversion of a configuration to a mapping of flattened parameters to their descriptions and
Expand All @@ -65,11 +69,12 @@ pub trait SerializeConfig {
/// Takes a vector of {target pointer params, SerializedParam, and vector of pointing params},
/// adds the target pointer params with the description and a value, and replaces the value of
/// the pointing params to contain only the name of the target they point to.
/// Fails if a param is not pointing to a same-named pointer target nor whitelisted.
///
/// # Example
///
/// ```
/// # use std::collections::BTreeMap;
/// # use std::collections::{BTreeMap, HashSet};
///
/// # use papyrus_config::dumping::{ser_param, SerializeConfig};
/// # use papyrus_config::{ParamPath, ParamPrivacyInput, SerializedParam};
Expand All @@ -94,15 +99,17 @@ pub trait SerializeConfig {
///
/// let dir = TempDir::new().unwrap();
/// let file_path = dir.path().join("config.json");
/// ConfigExample { key: 42 }.dump_to_file(&vec![], file_path.to_str().unwrap());
/// ConfigExample { key: 42 }.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap());
/// ```
/// Note, in the case of a None sub configs, its elements will not be included in the file.
fn dump_to_file(
&self,
config_pointers: &Vec<(ParamPath, SerializedParam)>,
config_pointers: &ConfigPointers,
non_pointer_params: &HashSet<ParamPath>,
file_path: &str,
) -> Result<(), ConfigError> {
let combined_map = combine_config_map_and_pointers(self.dump(), config_pointers)?;
let combined_map =
combine_config_map_and_pointers(self.dump(), config_pointers, non_pointer_params)?;

// Create file writer.
let file = File::create(file_path)?;
Expand Down Expand Up @@ -291,32 +298,67 @@ pub fn ser_pointer_target_required_param(
/// Adds each target param to the config map.
/// Updates entries in the map to point to these targets, replacing values of entries that match
/// the target parameter paths to contain only the name of the target they point to.
/// Fails if a param is not pointing to a same-named pointer target nor whitelisted.
pub(crate) fn combine_config_map_and_pointers(
mut config_map: BTreeMap<ParamPath, SerializedParam>,
pointers: &Vec<(ParamPath, SerializedParam)>,
pointers: &ConfigPointers,
non_pointer_params: &HashSet<ParamPath>,
) -> Result<Value, ConfigError> {
// Update config with target params.
for (target_param, serialized_pointer) in pointers {
for ((target_param, serialized_pointer), pointing_params_vec) in pointers {
// Insert target param.
config_map.insert(target_param.clone(), serialized_pointer.clone());

// Update config entries that match the target param as pointers.
config_map.iter_mut().for_each(|(param_path, serialized_param)| {
// Check if the param is the target param.
if param_path.ends_with(format!("{FIELD_SEPARATOR}{target_param}").as_str()) {
// Point to the target param.
*serialized_param = SerializedParam {
description: serialized_param.description.clone(),
// Update pointing params to point at the target param.
for pointing_param in pointing_params_vec {
let pointing_serialized_param =
config_map.get(pointing_param).ok_or(ConfigError::PointerSourceNotFound {
pointing_param: pointing_param.to_owned(),
})?;
config_map.insert(
pointing_param.to_owned(),
SerializedParam {
description: pointing_serialized_param.description.clone(),
content: SerializedContent::PointerTarget(target_param.to_owned()),
privacy: serialized_param.privacy.clone(),
};
}
});
privacy: pointing_serialized_param.privacy.clone(),
},
);
}
}

verify_pointing_params_by_name(&config_map, pointers, non_pointer_params);

Ok(json!(config_map))
}

pub(crate) fn required_param_description(description: &str) -> String {
format!("A required param! {}", description)
}

/// Verifies that params whose name matches a pointer target either point at it, or are whitelisted.
fn verify_pointing_params_by_name(
config_map: &BTreeMap<ParamPath, SerializedParam>,
pointers: &ConfigPointers,
non_pointer_params: &HashSet<ParamPath>,
) {
// Iterate over the config, check that all parameters whose name matches a pointer target either
// point at it or are in the whitelist.
config_map.iter().for_each(|(param_path, serialized_param)| {
for ((target_param, _), _) in pointers {
// Check if the param name matches a pointer target, and that it is not in the
// whitelist.
if param_path.ends_with(format!("{FIELD_SEPARATOR}{target_param}").as_str())
&& !non_pointer_params.contains(param_path)
{
// Check that the param points to the target param.
assert!(
serialized_param.content
== SerializedContent::PointerTarget(target_param.to_owned()),
"The target param {} should point to {}, or to be whitelisted.",
param_path,
target_param
);
};
}
});
}
4 changes: 2 additions & 2 deletions crates/papyrus_config/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
//! # Example
//!
//! ```
//! use std::collections::BTreeMap;
//! use std::collections::{BTreeMap, HashSet};
//! use std::fs::File;
//! use std::path::Path;
//!
Expand Down Expand Up @@ -36,7 +36,7 @@
//!
//! let dir = TempDir::new().unwrap();
//! let file_path = dir.path().join("config.json");
//! ConfigExample { key: 42 }.dump_to_file(&vec![], file_path.to_str().unwrap());
//! ConfigExample { key: 42 }.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap());
//! let file = File::open(file_path).unwrap();
//! let loaded_config = load_and_process_config::<ConfigExample>(
//! file,
Expand Down
4 changes: 2 additions & 2 deletions crates/papyrus_node/src/bin/papyrus_dump_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#[cfg(feature = "rpc")]
use papyrus_config::dumping::SerializeConfig;
#[cfg(feature = "rpc")]
use papyrus_node::config::pointers::CONFIG_POINTERS;
use papyrus_node::config::pointers::{CONFIG_NON_POINTERS_WHITELIST, CONFIG_POINTERS};
#[cfg(feature = "rpc")]
use papyrus_node::config::{NodeConfig, DEFAULT_CONFIG_PATH};

Expand All @@ -15,7 +15,7 @@ use papyrus_node::config::{NodeConfig, DEFAULT_CONFIG_PATH};
fn main() {
#[cfg(feature = "rpc")]
NodeConfig::default()
.dump_to_file(&CONFIG_POINTERS, DEFAULT_CONFIG_PATH)
.dump_to_file(&CONFIG_POINTERS, &CONFIG_NON_POINTERS_WHITELIST, DEFAULT_CONFIG_PATH)
.expect("dump to file error");
// TODO(shahak): Try to find a way to remove this binary altogether when the feature rpc is
// turned off.
Expand Down
10 changes: 8 additions & 2 deletions crates/papyrus_node/src/config/config_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use tempfile::NamedTempFile;
use validator::Validate;

#[cfg(feature = "rpc")]
use crate::config::pointers::CONFIG_POINTERS;
use crate::config::pointers::{CONFIG_NON_POINTERS_WHITELIST, CONFIG_POINTERS};
use crate::config::{node_command, NodeConfig, DEFAULT_CONFIG_PATH};

// Returns the required and generated params in config/papyrus/default_config.json with the default
Expand Down Expand Up @@ -137,7 +137,13 @@ fn default_config_file_is_up_to_date() {
// Create a temporary file and dump the default config to it.
let mut tmp_file_path = env::temp_dir();
tmp_file_path.push("cfg.json");
NodeConfig::default().dump_to_file(&CONFIG_POINTERS, tmp_file_path.to_str().unwrap()).unwrap();
NodeConfig::default()
.dump_to_file(
&CONFIG_POINTERS,
&CONFIG_NON_POINTERS_WHITELIST,
tmp_file_path.to_str().unwrap(),
)
.unwrap();

// Read the dumped config from the file.
let from_code: serde_json::Value =
Expand Down
Loading
Loading