diff --git a/rs/boundary_node/rate_limits/api/src/lib.rs b/rs/boundary_node/rate_limits/api/src/lib.rs index 7829b05c2c2b..331689d84a9a 100644 --- a/rs/boundary_node/rate_limits/api/src/lib.rs +++ b/rs/boundary_node/rate_limits/api/src/lib.rs @@ -12,7 +12,7 @@ pub type IncidentId = String; pub type SchemaVersion = u64; pub type GetConfigResponse = Result; -pub type AddConfigResponse = Result<(), String>; +pub type AddConfigResponse = Result<(), AddConfigError>; pub type GetRuleByIdResponse = Result; pub type DiscloseRulesResponse = Result<(), DiscloseRulesError>; pub type GetRulesByIncidentIdResponse = Result, String>; @@ -23,6 +23,18 @@ pub enum DiscloseRulesArg { IncidentIds(Vec), } +#[derive(CandidType, Debug, Deserialize)] +pub enum AddConfigError { + /// Indicates an unauthorized attempt to add a new config + Unauthorized, + /// Signifies that the provided input config is malformed + InvalidInputConfig(String), + /// Signifies that a new configuration cannot be added due to some policy infringement + PolicyViolation(String), + /// Captures all unexpected internal errors during the disclosure process + Internal(String), +} + #[derive(CandidType, Debug, Deserialize)] pub enum DiscloseRulesError { /// Indicates an unauthorized attempt to disclose rules @@ -51,13 +63,13 @@ pub struct OutputConfig { pub rules: Vec, } -#[derive(CandidType, Deserialize, Debug)] +#[derive(CandidType, Deserialize, Debug, Clone)] pub struct InputConfig { pub schema_version: SchemaVersion, pub rules: Vec, } -#[derive(CandidType, Deserialize, Debug)] +#[derive(CandidType, Deserialize, Debug, Clone)] pub struct InputRule { pub incident_id: IncidentId, pub rule_raw: Vec, diff --git a/rs/boundary_node/rate_limits/canister/access_control.rs b/rs/boundary_node/rate_limits/canister/access_control.rs index 81cd19db8dbf..28232744bfd3 100644 --- a/rs/boundary_node/rate_limits/canister/access_control.rs +++ b/rs/boundary_node/rate_limits/canister/access_control.rs @@ -2,10 +2,10 @@ use candid::Principal; use mockall::automock; use crate::{ - add_config::{AddConfigError, AddsConfig}, + add_config::AddsConfig, disclose::DisclosesRules, state::CanisterApi, - types::{DiscloseRulesError, Timestamp}, + types::{AddConfigError, DiscloseRulesError, Timestamp}, }; #[automock] diff --git a/rs/boundary_node/rate_limits/canister/add_config.rs b/rs/boundary_node/rate_limits/canister/add_config.rs index 7ba74590c701..4ff239dfa260 100644 --- a/rs/boundary_node/rate_limits/canister/add_config.rs +++ b/rs/boundary_node/rate_limits/canister/add_config.rs @@ -1,12 +1,11 @@ use crate::{ storage::StorableIncidentMetadata, - types::{self, IncidentId, InputConfigError, Timestamp}, + types::{self, AddConfigError, IncidentId, Timestamp}, }; -use anyhow::Context; +use anyhow::{anyhow, Context}; use getrandom::getrandom; +use rate_limits_api as api; use std::collections::{HashMap, HashSet}; -use strum::AsRefStr; -use thiserror::Error; use uuid::Uuid; use crate::{ @@ -15,46 +14,22 @@ use crate::{ types::{InputConfig, RuleId, Version}, }; -pub const INIT_SCHEMA_VERSION: Version = 1; +pub const INIT_JSON_SCHEMA_VERSION: Version = 1; pub const INIT_VERSION: Version = 1; +/// Defines a trait for adding new rate-limit configuration to the canister. pub trait AddsConfig { - fn add_config( - &self, - config: rate_limits_api::InputConfig, - time: Timestamp, - ) -> Result<(), AddConfigError>; -} - -#[derive(Debug, Error, Clone)] -pub enum RulePolicyError { - #[error("Rule at index={index} is linked to an already disclosed incident_id={incident_id}")] - LinkingRuleToDisclosedIncident { - index: usize, - incident_id: IncidentId, - }, -} - -#[derive(Debug, Error, AsRefStr)] -pub enum AddConfigError { - #[error("Unauthorized operation")] - #[strum(serialize = "unauthorized_error")] - Unauthorized, - #[error("Invalid input configuration: {0}")] - #[strum(serialize = "invalid_input_error")] - InvalidInput(#[from] InputConfigError), - #[error("Rule violates policy: {0}")] - #[strum(serialize = "policy_violation_error")] - PolicyViolation(#[from] RulePolicyError), - #[error("Initial version and configuration were not set")] - #[strum(serialize = "missing_initial_version_error")] - MissingInitialVersion, - #[error("An unexpected internal error occurred: {0}")] - #[strum(serialize = "internal_error")] - Internal(#[from] anyhow::Error), + /// # Arguments + /// * `config` - new rate-limit configuration to be stored. + /// * `time` - the timestamp indicating when the config is added. + /// + /// # Returns + /// A result indicating success or a specific error + fn add_config(&self, config: api::InputConfig, time: Timestamp) -> Result<(), AddConfigError>; } pub struct ConfigAdder { + /// The canister API used for interacting with the underlying storage pub canister_api: A, } @@ -69,49 +44,53 @@ impl ConfigAdder { // - Rules order within a config is significant, as rules are applied in the order they appear in the config. // - Adding a new config requires providing an entire list of ordered rules; config version is increment by one for 'add' operation. // - Each rule is identified by its unique ID and its non-mutable context provided by the caller: -// - incident_id: each rule must be linked to a certain incident_id; multiple rules can be linked to the same incident_id -// - rule_raw: binary encoded JSON of the rate-limit rule -// - description: some info why this rule was introduced -// - Alongside an immutable context, each rule includes metadata: -// - disclosed_at -// - added_in_version -// - removed_in_version +// - `incident_id`: each rule must be linked to a certain incident_id; multiple rules can be linked to the same incident_id +// - `rule_raw`: binary encoded JSON of the rate-limit rule +// - `description`: some info why this rule was introduced +// - Alongside an immutable context, each rule includes metadata for a better auditability experience: +// - `disclosed_at`: a timestamp at which the rule became publicly accessible for viewing +// - `added_in_version`: config version in which the rule was first introduced (rule can persist across multiple config versions, if resubmitted) +// - `removed_in_version`: config version in which the rule was removed, this happens when an existing rule is not resubmitted // - The canister generates a unique, random ID for each newly submitted rule. -// - Rules can persist across config versions, if resubmitted. -// - Non-resubmitted rules are considered as "removed" and their metadata fields are updated. // - Individual rules or incidents (a set of rules sharing the same incident_id) can be disclosed. This implies that the context of the rule becomes visible for the callers with `RestrictedRead` access level. // - Disclosing rules or incidents multiple times has no additional effect. // Policies: -// - Immutability: rule's context (incident_id, rule_raw, description) cannot be modified. -// - Operation of resubmitting a "removed" rule would result in creation of a rule with a new ID. -// - New rules cannot be linked to already disclosed incidents (LinkingRuleToDisclosedIncident error) +// - Immutability: rule's context (incident_id, rule_raw, description) cannot be modified. Hence an operation of resubmitting a "removed" rule would result in creation of a rule with a new ID, generated by the canister. +// - New rules cannot be linked to already disclosed incidents (LinkingRuleToDisclosedIncident error). impl AddsConfig for ConfigAdder { fn add_config( &self, - input_config: rate_limits_api::InputConfig, + input_config: api::InputConfig, time: Timestamp, ) -> Result<(), AddConfigError> { - // Convert config from api type + // Convert config from api type (also performs validation of each rule) let next_config = types::InputConfig::try_from(input_config)?; let current_version = self .canister_api .get_version() - .ok_or_else(|| AddConfigError::MissingInitialVersion)?; - - let next_version = current_version + 1; + // this error indicates that canister was not initialized correctly + .ok_or_else(|| AddConfigError::Internal(anyhow!("No existing config version found")))?; let current_config: StorableConfig = self .canister_api .get_config(current_version) - .ok_or_else(|| AddConfigError::MissingInitialVersion)?; + .ok_or_else(|| { + // this error indicates that canister was not initialized correctly + AddConfigError::Internal(anyhow!("No config for version={current_version} found")) + })?; + + let current_full_config: InputConfig = self + .canister_api + .get_full_config(current_version) + .ok_or_else(|| { + // this error indicates that canister was not initialized correctly + AddConfigError::Internal(anyhow!("No config for version={current_version} found")) + })?; - let current_full_config: InputConfig = - self.canister_api - .get_full_config(current_version) - .ok_or_else(|| AddConfigError::MissingInitialVersion)?; + let next_version = current_version + 1; // Ordered IDs of all rules in the submitted config let mut rule_ids = Vec::::new(); @@ -135,8 +114,13 @@ impl AddsConfig for ConfigAdder { let rule_id = if let Some(rule_idx) = existing_rule_idx { current_config.rule_ids[rule_idx] } else { - // TODO: check for collisions and regenerate if needed let rule_id = RuleId(generate_random_uuid()?); + // If the generated UUID already exists, return the error (practically this should never happen). + if self.canister_api.get_rule(&rule_id).is_some() { + return Err(AddConfigError::Internal(anyhow!( + "Failed to generate a new uuid {rule_id}, please retry the operation." + ))); + } // Check if the new rule is linked to an existing incident let existing_incident = self.canister_api.get_incident(&input_rule.incident_id); @@ -144,12 +128,10 @@ impl AddsConfig for ConfigAdder { if let Some(incident) = existing_incident { // A new rule can't be linked to a disclosed incident if incident.is_disclosed { - Err(AddConfigError::PolicyViolation( - RulePolicyError::LinkingRuleToDisclosedIncident { - index: rule_idx, - incident_id: input_rule.incident_id, - }, - ))?; + Err(AddConfigError::LinkingRuleToDisclosedIncident { + index: rule_idx, + incident_id: input_rule.incident_id, + })?; } } @@ -203,13 +185,6 @@ impl AddsConfig for ConfigAdder { } } -impl From for String { - fn from(value: AddConfigError) -> Self { - value.to_string() - } -} - -// TODO: make it work with canister upgrade, post_upgrade fn generate_random_uuid() -> Result { let mut buf = [0u8; 16]; getrandom(&mut buf) @@ -227,29 +202,22 @@ fn commit_changes( new_rules_metadata: Vec<(RuleId, StorableRuleMetadata)>, incidents_map: HashMap>, ) { - // Update metadata of the removed rules + // Update metadata of the removed rules in the stable memory for rule_id in removed_rules { + // Rule should exist, it was already checked before. let mut rule_metadata = canister_api .get_rule(&rule_id) - .expect("rule_id = {rule_id} not found"); - + .expect("inconsistent state, rule_id={rule_id} not found"); rule_metadata.removed_in_version = Some(next_version); - - assert!( - canister_api.upsert_rule(rule_id, rule_metadata).is_some(), - "Rule with rule_id = {rule_id} not found, failed to update" - ); + canister_api.upsert_rule(rule_id, rule_metadata); } - // Add new rules + // Add new rules to the stable memory for (rule_id, rule_metadata) in new_rules_metadata { - assert!( - canister_api.upsert_rule(rule_id, rule_metadata).is_none(), - "Rule with rule_id = {rule_id} already exists, failed to add" - ); + canister_api.upsert_rule(rule_id, rule_metadata); } - // Upsert incidents, some of the incidents can be new, some already existed before + // Upsert incidents to the stable memory, some of the incidents can be new, some already existed before for (incident_id, rule_ids) in incidents_map { let incident_metadata = canister_api .get_incident(&incident_id) @@ -262,61 +230,413 @@ fn commit_changes( rule_ids: rule_ids.clone(), }); - let _ = canister_api.upsert_incident(incident_id, incident_metadata); + canister_api.upsert_incident(incident_id, incident_metadata); } - assert!( - canister_api - .upsert_config(next_version, storable_config) - .is_none(), - "Failed to add config for version {next_version}, config already exists" - ); + // Add a new config to the stable memory + canister_api.add_config(next_version, storable_config); } #[cfg(test)] mod tests { - use crate::access_control::{AccessLevel, MockResolveAccessLevel}; - use crate::state::MockCanisterApi; - use super::*; + use crate::state::CanisterState; + use rate_limits_api as api; + use types::InputConfigError; + + #[derive(Debug, PartialEq)] + struct FullConfig { + schema_version: api::SchemaVersion, + active_since: api::Timestamp, + rules: Vec, + } + + fn retrieve_full_config(canister_api: impl CanisterApi, version: u64) -> FullConfig { + let config = canister_api.get_config(version).unwrap(); + let mut full_config = FullConfig { + schema_version: config.schema_version, + active_since: config.active_since, + rules: vec![], + }; + + for rule_id in config.rule_ids.iter() { + let rule = canister_api.get_rule(rule_id).unwrap(); + full_config.rules.push(rule); + } + + full_config + } + + // A comprehensive test for adding new rate-limit configs #[test] fn test_add_config_success() { - let config = rate_limits_api::InputConfig { - schema_version: 1, + let current_time = 10u64; + let schema_version = 1; + let canister_state = CanisterState::from_static(); + // Add init config_1 corresponding to version=1 to the canister state + canister_state.add_config( + 1, + StorableConfig { + schema_version, + active_since: current_time, + rule_ids: vec![], + }, + ); + + let incident_id_1 = IncidentId(Uuid::new_v4()); + let incident_id_2 = IncidentId(Uuid::new_v4()); + let incident_id_3 = IncidentId(Uuid::new_v4()); + + // Config with two new rules + let config_2 = api::InputConfig { + schema_version, + rules: vec![ + api::InputRule { + incident_id: incident_id_1.0.to_string(), + rule_raw: b"{\"a\": 1, \"b\": 2}".to_vec(), + description: "best rule #1 ever".to_string(), + }, + api::InputRule { + incident_id: incident_id_1.0.to_string(), + rule_raw: b"{\"c\": 3, \"d\": 4}".to_vec(), + description: "best rule #2 ever".to_string(), + }, + ], + }; + // Config with identical rules. However, schema_version is now 2, hence all rules should be marked as "removed" and new ruled_ids must be regenerated by the canister + let config_3 = api::InputConfig { + schema_version: schema_version + 1, + rules: vec![ + api::InputRule { + incident_id: incident_id_1.0.to_string(), + rule_raw: b"{\"a\": 1, \"b\": 2}".to_vec(), + description: "best rule #1 ever".to_string(), + }, + api::InputRule { + incident_id: incident_id_1.0.to_string(), + rule_raw: b"{\"c\": 3, \"d\": 4}".to_vec(), + description: "best rule #2 ever".to_string(), + }, + ], + }; + // This config is identical to the previous one + // NOTE: rule binary representations have changed, but not the inner JSON, so rules are in fact unchanged + let config_4 = api::InputConfig { + schema_version: schema_version + 1, + rules: vec![ + api::InputRule { + incident_id: incident_id_1.0.to_string(), + // binary representation has changed, but not the inner JSON + rule_raw: b"{\"b\": 2, \"a\": 1}".to_vec(), + description: "best rule #1 ever".to_string(), + }, + api::InputRule { + incident_id: incident_id_1.0.to_string(), + // binary representation has changed, but not the inner JSON + rule_raw: b"{\"d\": 4, \"c\": 3}".to_vec(), + description: "best rule #2 ever".to_string(), + }, + ], + }; + // This config adds two new rules and removes the first. + let config_5 = api::InputConfig { + schema_version: schema_version + 1, + // New rule, as incident id and description changed + rules: vec![ + // Brand new rule + api::InputRule { + incident_id: incident_id_2.0.to_string(), + rule_raw: b"{\"e\": 5, \"f\": 6}".to_vec(), + description: "best rate-limit rule #4 ever".to_string(), + }, + // Brand new rule + api::InputRule { + incident_id: incident_id_3.0.to_string(), + rule_raw: b"{\"g\": 7, \"e\": 8}".to_vec(), + description: "best rate-limit rule #5 ever".to_string(), + }, + // This rule is unchanged from the previous config + api::InputRule { + incident_id: incident_id_1.0.to_string(), + rule_raw: b"{\"c\": 3, \"d\": 4}".to_vec(), + description: "best rule #2 ever".to_string(), + }, + ], + }; + // This config removes all existing rules, it is empty + let config_6 = api::InputConfig { + schema_version: schema_version + 1, rules: vec![], }; - let current_time = 0u64; - - let mut mock_access = MockResolveAccessLevel::new(); - mock_access - .expect_get_access_level() - .returning(|| AccessLevel::FullAccess); - let mut mock_canister_api = MockCanisterApi::new(); - - mock_canister_api.expect_get_rule().returning(|_| None); - mock_canister_api.expect_get_version().returning(|| Some(1)); - mock_canister_api.expect_get_config().returning(|_| { - Some(StorableConfig { + + let adder = ConfigAdder::new(canister_state.clone()); + + // Perform multiple add config operations + adder + .add_config(config_2.clone(), current_time) + .expect("failed to add config"); + adder + .add_config(config_3.clone(), current_time + 1) + .expect("failed to add config"); + adder + .add_config(config_4, current_time + 2) + .expect("failed to add config"); + adder + .add_config(config_5, current_time + 3) + .expect("failed to add config"); + adder + .add_config(config_6, current_time + 4) + .expect("failed to add config"); + + // Assert expected configs + let version = 1; + assert_eq!( + retrieve_full_config(canister_state.clone(), version), + FullConfig { schema_version: 1, - active_since: 1, - rule_ids: vec![], - }) - }); - mock_canister_api.expect_get_full_config().returning(|_| { - Some(InputConfig { + active_since: current_time, + rules: vec![], + } + ); + + let version = 2; + assert_eq!( + retrieve_full_config(canister_state.clone(), version), + FullConfig { schema_version: 1, + active_since: current_time, + rules: vec![ + StorableRuleMetadata { + incident_id: incident_id_1, + rule_raw: b"{\"a\": 1, \"b\": 2}".to_vec(), + description: "best rule #1 ever".to_string(), + disclosed_at: None, + added_in_version: 2, + removed_in_version: Some(3), + }, + StorableRuleMetadata { + incident_id: incident_id_1, + rule_raw: b"{\"c\": 3, \"d\": 4}".to_vec(), + description: "best rule #2 ever".to_string(), + disclosed_at: None, + added_in_version: 2, + removed_in_version: Some(3), + } + ], + } + ); + + let version = 3; + assert_eq!( + retrieve_full_config(canister_state.clone(), version), + FullConfig { + schema_version: 2, + active_since: current_time + 1, + rules: vec![ + StorableRuleMetadata { + incident_id: incident_id_1, + rule_raw: b"{\"a\": 1, \"b\": 2}".to_vec(), + description: "best rule #1 ever".to_string(), + disclosed_at: None, + added_in_version: 3, + removed_in_version: Some(5), + }, + StorableRuleMetadata { + incident_id: incident_id_1, + rule_raw: b"{\"c\": 3, \"d\": 4}".to_vec(), + description: "best rule #2 ever".to_string(), + disclosed_at: None, + added_in_version: 3, + removed_in_version: Some(6), + } + ], + } + ); + + let version = 4; + assert_eq!( + retrieve_full_config(canister_state.clone(), version), + FullConfig { + schema_version: 2, + active_since: current_time + 2, + rules: vec![ + StorableRuleMetadata { + incident_id: incident_id_1, + rule_raw: b"{\"a\": 1, \"b\": 2}".to_vec(), + description: "best rule #1 ever".to_string(), + disclosed_at: None, + added_in_version: 3, + removed_in_version: Some(5), + }, + StorableRuleMetadata { + incident_id: incident_id_1, + rule_raw: b"{\"c\": 3, \"d\": 4}".to_vec(), + description: "best rule #2 ever".to_string(), + disclosed_at: None, + added_in_version: 3, + removed_in_version: Some(6), + } + ], + } + ); + + let version = 5; + assert_eq!( + retrieve_full_config(canister_state.clone(), version), + FullConfig { + schema_version: 2, + active_since: current_time + 3, + rules: vec![ + StorableRuleMetadata { + incident_id: incident_id_2, + rule_raw: b"{\"e\": 5, \"f\": 6}".to_vec(), + description: "best rate-limit rule #4 ever".to_string(), + disclosed_at: None, + added_in_version: 5, + removed_in_version: Some(6), + }, + StorableRuleMetadata { + incident_id: incident_id_3, + rule_raw: b"{\"g\": 7, \"e\": 8}".to_vec(), + description: "best rate-limit rule #5 ever".to_string(), + disclosed_at: None, + added_in_version: 5, + removed_in_version: Some(6), + }, + StorableRuleMetadata { + incident_id: incident_id_1, + rule_raw: b"{\"c\": 3, \"d\": 4}".to_vec(), + description: "best rule #2 ever".to_string(), + disclosed_at: None, + added_in_version: 3, + removed_in_version: Some(6), + } + ], + } + ); + + let version = 6; + assert_eq!( + retrieve_full_config(canister_state.clone(), version), + FullConfig { + schema_version: 2, + active_since: current_time + 4, rules: vec![], - }) - }); - mock_canister_api - .expect_upsert_config() - .returning(|_, _| None); + } + ); - let adder = ConfigAdder::new(mock_canister_api); + assert_eq!(canister_state.incidents_count(), 3); + assert_eq!(canister_state.active_rules_count(), 0); + assert_eq!(canister_state.configs_count(), 6); + } - adder - .add_config(config, current_time) - .expect("failed to add a new config"); + #[test] + fn test_add_config_fails_with_invalid_inputs() { + // Arrange + let current_time = 10u64; + let canister_state = CanisterState::from_static(); + let invalid_config_1 = api::InputConfig { + schema_version: 1, + rules: vec![api::InputRule { + incident_id: "not_a_valid_uuid".to_string(), + rule_raw: b"{}".to_vec(), + description: "".to_string(), + }], + }; + let invalid_config_2 = api::InputConfig { + schema_version: 1, + rules: vec![ + api::InputRule { + incident_id: "ebe7dbb1-63c9-420e-980d-eb0f8c20a9fb".to_string(), + rule_raw: b"{}".to_vec(), + description: "".to_string(), + }, + api::InputRule { + incident_id: "ebe7dbb1-63c9-420e-980d-eb0f8c20a9fb".to_string(), + rule_raw: b"not_a_valid_json".to_vec(), + description: "".to_string(), + }, + ], + }; + // Act & assert + let adder = ConfigAdder::new(canister_state); + let error = adder + .add_config(invalid_config_1, current_time) + .unwrap_err(); + assert!( + matches!(error, AddConfigError::InvalidInputConfig(InputConfigError::InvalidIncidentUuidFormat(idx)) if idx == 0) + ); + let error = adder + .add_config(invalid_config_2, current_time) + .unwrap_err(); + assert!( + matches!(error, AddConfigError::InvalidInputConfig(InputConfigError::InvalidRuleJsonEncoding(idx)) if idx == 1) + ); + } + + #[test] + fn test_add_config_without_init_version_fails() { + // Arrange + let canister_state = CanisterState::from_static(); + let adder = ConfigAdder::new(canister_state); + let current_time = 10u64; + let config = api::InputConfig { + schema_version: 1, + rules: vec![], + }; + // Act & assert + let error = adder.add_config(config, current_time).unwrap_err(); + assert!( + matches!(error, AddConfigError::Internal(err) if err.to_string() == "No existing config version found") + ); + } + + #[test] + fn test_add_config_with_policy_violation_fails() { + // Arrange + let canister_state = CanisterState::from_static(); + canister_state.add_config( + 1, + StorableConfig { + schema_version: 1, + active_since: 1, + rule_ids: vec![], + }, + ); + let incident_id_1 = IncidentId(Uuid::new_v4()); + let incident_id_2 = IncidentId(Uuid::new_v4()); + let storable_incident_1 = StorableIncidentMetadata { + is_disclosed: false, + rule_ids: HashSet::new(), + }; + // This incident is disclosed, new rules can't be linked to it anymore. \ + let storable_incident_2 = StorableIncidentMetadata { + is_disclosed: true, + rule_ids: HashSet::new(), + }; + canister_state.upsert_incident(incident_id_1, storable_incident_1); + canister_state.upsert_incident(incident_id_2, storable_incident_2); + let config = api::InputConfig { + schema_version: 1, + rules: vec![ + api::InputRule { + incident_id: incident_id_1.0.to_string(), + rule_raw: b"{}".to_vec(), + description: "".to_string(), + }, + api::InputRule { + incident_id: incident_id_2.0.to_string(), // this incident is already disclosed, new rule can't be added + rule_raw: b"{}".to_vec(), + description: "".to_string(), + }, + ], + }; + // Act & assert + let adder = ConfigAdder::new(canister_state); + let error = adder.add_config(config, 1u64).unwrap_err(); + assert!( + matches!(error, AddConfigError::LinkingRuleToDisclosedIncident{index, incident_id} if index == 1 && incident_id == incident_id_2) + ); } } diff --git a/rs/boundary_node/rate_limits/canister/canister.rs b/rs/boundary_node/rate_limits/canister/canister.rs index 4a0f4b417418..08b649880c22 100644 --- a/rs/boundary_node/rate_limits/canister/canister.rs +++ b/rs/boundary_node/rate_limits/canister/canister.rs @@ -128,18 +128,22 @@ fn get_rules_by_incident_id(incident_id: IncidentId) -> GetRulesByIncidentIdResp Ok(response) } +/// Adds a new rate-limit configuration (containing a vector of rate-limit rules) to the canister +/// +/// Newly added configuration (including confidential rate-limit rules) can be retrieved by the API boundary nodes and enforced on their side. +/// This update method includes authorization check and metrics collection. #[update] fn add_config(config: InputConfig) -> AddConfigResponse { let caller_id = ic_cdk::api::caller(); let current_time = ic_cdk::api::time(); - with_canister_state(|state| { + let result = with_canister_state(|state| { let access_resolver = AccessLevelResolver::new(caller_id, state.clone()); let adder = ConfigAdder::new(state); let adder = WithAuthorization::new(adder, access_resolver); let adder = WithMetrics::new(adder); adder.add_config(config, current_time) })?; - Ok(()) + Ok(result) } /// Makes specified rules publicly accessible in the canister diff --git a/rs/boundary_node/rate_limits/canister/disclose.rs b/rs/boundary_node/rate_limits/canister/disclose.rs index b46f75d9cdcb..11c0fa7e82a3 100644 --- a/rs/boundary_node/rate_limits/canister/disclose.rs +++ b/rs/boundary_node/rate_limits/canister/disclose.rs @@ -72,7 +72,7 @@ fn disclose_rules( for (rule_id, mut metadata) in rules { if metadata.disclosed_at.is_none() { metadata.disclosed_at = Some(time); - let _ = canister_api.upsert_rule(*rule_id, metadata); + canister_api.upsert_rule(*rule_id, metadata); } } @@ -103,7 +103,7 @@ fn disclose_incidents( disclose_rules(canister_api, time, &rule_ids)?; // Mark incident as disclosed too metadata.is_disclosed = true; - let _ = canister_api.upsert_incident(incident_id, metadata); + canister_api.upsert_incident(incident_id, metadata); } } diff --git a/rs/boundary_node/rate_limits/canister/interface.did b/rs/boundary_node/rate_limits/canister/interface.did index 00a7e107fd3d..f2718dd10f8b 100644 --- a/rs/boundary_node/rate_limits/canister/interface.did +++ b/rs/boundary_node/rate_limits/canister/interface.did @@ -57,9 +57,16 @@ type GetConfigResponse = variant { Err: text; }; +type AddConfigError = variant { + Unauthorized; // Indicates an unauthorized attempt to add a new config + InvalidInputConfig: text; // Signifies that the provided input config is malformed + PolicyViolation: text; // Signifies that a new configuration cannot be added due to some policy infringement + Internal: text; // Captures all unexpected internal errors during process +}; + type AddConfigResponse = variant { Ok; - Err: text; + Err: AddConfigError; }; type DiscloseRulesError = variant { diff --git a/rs/boundary_node/rate_limits/canister/metrics.rs b/rs/boundary_node/rate_limits/canister/metrics.rs index 09a4a0c2aabb..a2c9ec0d865e 100644 --- a/rs/boundary_node/rate_limits/canister/metrics.rs +++ b/rs/boundary_node/rate_limits/canister/metrics.rs @@ -1,8 +1,8 @@ use crate::{ - add_config::{AddConfigError, AddsConfig}, + add_config::AddsConfig, disclose::DisclosesRules, state::CanisterApi, - types::{DiscloseRulesError, Timestamp}, + types::{AddConfigError, DiscloseRulesError, Timestamp}, }; use ic_canisters_http_types::{HttpResponse, HttpResponseBuilder}; use ic_cdk::api::stable::WASM_PAGE_SIZE_IN_BYTES; diff --git a/rs/boundary_node/rate_limits/canister/state.rs b/rs/boundary_node/rate_limits/canister/state.rs index 5d71cc8bd9ae..baec479a5e53 100644 --- a/rs/boundary_node/rate_limits/canister/state.rs +++ b/rs/boundary_node/rate_limits/canister/state.rs @@ -3,7 +3,7 @@ use mockall::automock; use std::collections::HashSet; use crate::{ - add_config::{INIT_SCHEMA_VERSION, INIT_VERSION}, + add_config::{INIT_JSON_SCHEMA_VERSION, INIT_VERSION}, storage::{ LocalRef, StableMap, StorableConfig, StorableIncidentId, StorableIncidentMetadata, StorablePrincipal, StorableRuleId, StorableRuleMetadata, StorableVersion, @@ -21,17 +21,9 @@ pub trait CanisterApi { fn get_config(&self, version: Version) -> Option; fn get_rule(&self, rule_id: &RuleId) -> Option; fn get_incident(&self, incident_id: &IncidentId) -> Option; - fn upsert_config(&self, version: Version, config: StorableConfig) -> Option; - fn upsert_rule( - &self, - rule_id: RuleId, - rule: StorableRuleMetadata, - ) -> Option; - fn upsert_incident( - &self, - incident_id: IncidentId, - rule_ids: StorableIncidentMetadata, - ) -> Option; + fn add_config(&self, version: Version, config: StorableConfig); + fn upsert_rule(&self, rule_id: RuleId, rule: StorableRuleMetadata); + fn upsert_incident(&self, incident_id: IncidentId, rule_ids: StorableIncidentMetadata); fn is_api_boundary_node_principal(&self, principal: &Principal) -> bool; fn set_api_boundary_nodes_principals(&self, principals: Vec); fn api_boundary_nodes_count(&self) -> u64; @@ -113,29 +105,21 @@ impl CanisterApi for CanisterState { .with(|cell| cell.borrow().get(&StorableIncidentId(incident_id.0))) } - fn upsert_config(&self, version: Version, config: StorableConfig) -> Option { + fn add_config(&self, version: Version, config: StorableConfig) { self.configs - .with(|cell| cell.borrow_mut().insert(version, config)) + .with(|cell| cell.borrow_mut().insert(version, config)); } - fn upsert_rule( - &self, - rule_id: RuleId, - rule: StorableRuleMetadata, - ) -> Option { + fn upsert_rule(&self, rule_id: RuleId, rule: StorableRuleMetadata) { self.rules - .with(|cell| cell.borrow_mut().insert(StorableRuleId(rule_id.0), rule)) + .with(|cell| cell.borrow_mut().insert(StorableRuleId(rule_id.0), rule)); } - fn upsert_incident( - &self, - incident_id: IncidentId, - rule_ids: StorableIncidentMetadata, - ) -> Option { + fn upsert_incident(&self, incident_id: IncidentId, rule_ids: StorableIncidentMetadata) { self.incidents.with(|cell| { cell.borrow_mut() .insert(StorableIncidentId(incident_id.0), rule_ids) - }) + }); } fn is_api_boundary_node_principal(&self, principal: &Principal) -> bool { @@ -173,14 +157,11 @@ impl CanisterApi for CanisterState { pub fn init_version_and_config(time: Timestamp, canister_api: impl CanisterApi) { // Initialize config with an empty vector of rules let config = StorableConfig { - schema_version: INIT_SCHEMA_VERSION, + schema_version: INIT_JSON_SCHEMA_VERSION, active_since: time, rule_ids: vec![], }; - assert!( - canister_api.upsert_config(INIT_VERSION, config).is_none(), - "Config for version={INIT_VERSION} already exists!" - ); + canister_api.add_config(INIT_VERSION, config); } pub fn with_canister_state(f: impl FnOnce(CanisterState) -> R) -> R { diff --git a/rs/boundary_node/rate_limits/canister/storage.rs b/rs/boundary_node/rate_limits/canister/storage.rs index d176ff61e17f..2e53fcac17ad 100644 --- a/rs/boundary_node/rate_limits/canister/storage.rs +++ b/rs/boundary_node/rate_limits/canister/storage.rs @@ -33,7 +33,7 @@ pub struct StorableRuleId(pub Uuid); #[derive(Clone, Debug, Serialize, Deserialize, PartialOrd, Ord, PartialEq, Eq)] pub struct StorableIncidentId(pub Uuid); -#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] pub struct StorableRuleMetadata { pub incident_id: IncidentId, pub rule_raw: Vec, diff --git a/rs/boundary_node/rate_limits/canister/types.rs b/rs/boundary_node/rate_limits/canister/types.rs index 3b4a420e3735..b1d4cf183b2b 100644 --- a/rs/boundary_node/rate_limits/canister/types.rs +++ b/rs/boundary_node/rate_limits/canister/types.rs @@ -10,6 +10,8 @@ pub type Version = u64; pub type Timestamp = u64; pub type SchemaVersion = u64; +use rate_limits_api as api; + #[derive(Debug, Clone, Copy, PartialEq, Hash, Eq, Serialize, Deserialize)] pub struct RuleId(pub Uuid); @@ -70,9 +72,9 @@ pub struct OutputRule { pub disclosed_at: Option, } -impl From for rate_limits_api::OutputRule { +impl From for api::OutputRule { fn from(value: OutputRule) -> Self { - rate_limits_api::OutputRule { + api::OutputRule { description: value.description, id: value.id.to_string(), incident_id: value.incident_id.to_string(), @@ -81,6 +83,40 @@ impl From for rate_limits_api::OutputRule { } } +#[derive(Debug, Error, AsRefStr)] +pub enum AddConfigError { + /// Indicates an unauthorized attempt to add a new config + #[error("Unauthorized operation")] + #[strum(serialize = "unauthorized_error")] + Unauthorized, + /// Signifies that the provided input config is malformed + #[error("Invalid input configuration: {0}")] + #[strum(serialize = "invalid_input_error")] + InvalidInputConfig(#[from] InputConfigError), + /// Signifies policy infringement, a newly added rule refers to an incident which was already disclosed + #[strum(serialize = "policy_violation_error")] + #[error("Rule at index={index} is linked to an already disclosed incident_id={incident_id}")] + LinkingRuleToDisclosedIncident { + index: usize, + incident_id: IncidentId, + }, + /// Captures all unexpected internal errors during process + #[error("An unexpected internal error occurred: {0}")] + #[strum(serialize = "internal_error")] + Internal(#[from] anyhow::Error), +} + +impl From for api::AddConfigError { + fn from(value: AddConfigError) -> Self { + match value { + AddConfigError::Unauthorized => api::AddConfigError::Unauthorized, + AddConfigError::InvalidInputConfig(err) => api::AddConfigError::InvalidInputConfig(err.to_string()), + AddConfigError::LinkingRuleToDisclosedIncident { index, incident_id } => api::AddConfigError::PolicyViolation(format!("Rule at index={index} is linked to an already disclosed incident_id={incident_id}")), + AddConfigError::Internal(error) => api::AddConfigError::Internal(error.to_string()), + } + } +} + #[derive(Debug, Error, AsRefStr)] pub enum DiscloseRulesError { /// Indicates an unauthorized attempt to disclose rules @@ -88,7 +124,7 @@ pub enum DiscloseRulesError { #[strum(serialize = "unauthorized_error")] Unauthorized, /// Signifies that an input ID provided for disclosure is not a valid UUID - #[error("Invalid UUID at index = {0}")] + #[error("Invalid UUID at index={0}")] InvalidUuidFormat(usize), /// Signifies that a specified incident ID could not be found #[error("Incident with ID={0} not found")] @@ -104,29 +140,24 @@ pub enum DiscloseRulesError { Internal(#[from] anyhow::Error), } -impl From for rate_limits_api::DiscloseRulesError { +impl From for api::DiscloseRulesError { fn from(value: DiscloseRulesError) -> Self { match value { - DiscloseRulesError::Unauthorized => rate_limits_api::DiscloseRulesError::Unauthorized, + DiscloseRulesError::Unauthorized => api::DiscloseRulesError::Unauthorized, DiscloseRulesError::InvalidUuidFormat(idx) => { - rate_limits_api::DiscloseRulesError::InvalidUuidFormat(format!( - "Invalid UUID at index = {idx}" - )) + api::DiscloseRulesError::InvalidUuidFormat(format!("Invalid UUID at index={idx}")) } DiscloseRulesError::IncidentIdNotFound(incident_id) => { - rate_limits_api::DiscloseRulesError::IncidentIdNotFound(format!( + api::DiscloseRulesError::IncidentIdNotFound(format!( "Incident with ID={} not found", incident_id.0 )) } - DiscloseRulesError::RuleIdNotFound(rule_id) => { - rate_limits_api::DiscloseRulesError::RuleIdNotFound(format!( - "Rule with ID={0} not found", - rule_id.0 - )) - } + DiscloseRulesError::RuleIdNotFound(rule_id) => api::DiscloseRulesError::RuleIdNotFound( + format!("Rule with ID={0} not found", rule_id.0), + ), DiscloseRulesError::Internal(error) => { - rate_limits_api::DiscloseRulesError::Internal(error.to_string()) + api::DiscloseRulesError::Internal(error.to_string()) } } } @@ -134,16 +165,16 @@ impl From for rate_limits_api::DiscloseRulesError { #[derive(Debug, Error, Clone)] pub enum InputConfigError { - #[error("Invalid JSON encoding of rule_raw for rule at index = {0}")] + #[error("Invalid JSON encoding of rule_raw for rule at index={0}")] InvalidRuleJsonEncoding(usize), - #[error("Invalid UUID format of incident_id for rule at index = {0}")] - InvalidUuidFormatForIncident(usize), + #[error("Invalid UUID format of incident_id for rule at index={0}")] + InvalidIncidentUuidFormat(usize), } -impl TryFrom for InputConfig { +impl TryFrom for InputConfig { type Error = InputConfigError; - fn try_from(value: rate_limits_api::InputConfig) -> Result { + fn try_from(value: api::InputConfig) -> Result { let mut rules = Vec::with_capacity(value.rules.len()); for (idx, rule) in value.rules.into_iter().enumerate() { @@ -153,7 +184,7 @@ impl TryFrom for InputConfig { let rule = InputRule { incident_id: IncidentId::try_from(rule.incident_id) - .map_err(|_| InputConfigError::InvalidUuidFormatForIncident(idx))?, + .map_err(|_| InputConfigError::InvalidIncidentUuidFormat(idx))?, rule_raw: rule.rule_raw, description: rule.description, }; @@ -170,9 +201,9 @@ impl TryFrom for InputConfig { } } -impl From for rate_limits_api::OutputConfig { +impl From for api::OutputConfig { fn from(value: OutputConfig) -> Self { - rate_limits_api::OutputConfig { + api::OutputConfig { schema_version: value.schema_version, rules: value.rules.into_iter().map(|r| r.into()).collect(), is_redacted: value.is_redacted, @@ -180,9 +211,9 @@ impl From for rate_limits_api::OutputConfig { } } -impl From for rate_limits_api::ConfigResponse { +impl From for api::ConfigResponse { fn from(value: ConfigResponse) -> Self { - rate_limits_api::ConfigResponse { + api::ConfigResponse { version: value.version, active_since: value.active_since, config: value.config.into(), @@ -201,9 +232,9 @@ pub struct OutputRuleMetadata { pub removed_in_version: Option, } -impl From for rate_limits_api::OutputRuleMetadata { +impl From for api::OutputRuleMetadata { fn from(value: OutputRuleMetadata) -> Self { - rate_limits_api::OutputRuleMetadata { + api::OutputRuleMetadata { id: value.id.0.to_string(), incident_id: value.incident_id.0.to_string(), rule_raw: value.rule_raw, @@ -215,12 +246,12 @@ impl From for rate_limits_api::OutputRuleMetadata { } } -impl TryFrom for DiscloseRulesArg { +impl TryFrom for DiscloseRulesArg { type Error = DiscloseRulesError; - fn try_from(value: rate_limits_api::DiscloseRulesArg) -> Result { + fn try_from(value: api::DiscloseRulesArg) -> Result { match value { - rate_limits_api::DiscloseRulesArg::RuleIds(rule_ids) => { + api::DiscloseRulesArg::RuleIds(rule_ids) => { let mut rules = Vec::with_capacity(rule_ids.len()); for (idx, rule_id) in rule_ids.into_iter().enumerate() { let uuid = RuleId::try_from(rule_id) @@ -229,7 +260,7 @@ impl TryFrom for DiscloseRulesArg { } Ok(DiscloseRulesArg::RuleIds(rules)) } - rate_limits_api::DiscloseRulesArg::IncidentIds(incident_ids) => { + api::DiscloseRulesArg::IncidentIds(incident_ids) => { let mut incidents = Vec::with_capacity(incident_ids.len()); for (idx, incident_id) in incident_ids.into_iter().enumerate() { let uuid = IncidentId::try_from(incident_id) @@ -242,19 +273,19 @@ impl TryFrom for DiscloseRulesArg { } } -impl TryFrom for RuleId { +impl TryFrom for RuleId { type Error = uuid::Error; - fn try_from(value: rate_limits_api::RuleId) -> Result { + fn try_from(value: api::RuleId) -> Result { let uuid = Uuid::parse_str(&value)?; Ok(Self(uuid)) } } -impl TryFrom for IncidentId { +impl TryFrom for IncidentId { type Error = uuid::Error; - fn try_from(value: rate_limits_api::IncidentId) -> Result { + fn try_from(value: api::IncidentId) -> Result { let uuid = Uuid::parse_str(&value)?; Ok(Self(uuid)) } diff --git a/rs/boundary_node/rate_limits/integration_tests/tests/rate_limit_canister_tests.rs b/rs/boundary_node/rate_limits/integration_tests/tests/rate_limit_canister_tests.rs index ae7495f0ed33..63c77a6b04b1 100644 --- a/rs/boundary_node/rate_limits/integration_tests/tests/rate_limit_canister_tests.rs +++ b/rs/boundary_node/rate_limits/integration_tests/tests/rate_limit_canister_tests.rs @@ -58,7 +58,7 @@ async fn main() { }) .unwrap(); - let response: AddConfigResponse = canister_call( + let response: Result<(), String> = canister_call( &pocket_ic, "add_config", "update",