From b0fc44de8cda03062b7c41098c64c131be50e13f Mon Sep 17 00:00:00 2001 From: Rafael RL Date: Wed, 4 Oct 2023 16:18:09 +0200 Subject: [PATCH] Avoid using validation on call and add unit tests --- kuksa_databroker/databroker/src/broker.rs | 56 +++++++++++- .../databroker/src/grpc/kuksa_val_v1/val.rs | 11 --- kuksa_databroker/databroker/src/main.rs | 88 +++++++++---------- 3 files changed, 97 insertions(+), 58 deletions(-) diff --git a/kuksa_databroker/databroker/src/broker.rs b/kuksa_databroker/databroker/src/broker.rs index 330e516a..7bbe4410 100644 --- a/kuksa_databroker/databroker/src/broker.rs +++ b/kuksa_databroker/databroker/src/broker.rs @@ -29,6 +29,8 @@ use std::time::SystemTime; use tracing::{debug, info, warn}; +use crate::glob; + #[derive(Debug)] pub enum UpdateError { NotFound, @@ -46,7 +48,7 @@ pub enum ReadError { PermissionExpired, } -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum RegistrationError { ValidationError, PermissionDenied, @@ -1024,6 +1026,10 @@ impl<'a, 'b> DatabaseWriteAccess<'a, 'b> { allowed: Option, datapoint: Option, ) -> Result { + if !glob::is_valid_pattern(name.as_str()) { + return Err(RegistrationError::ValidationError); + } + self.permissions .can_create(&name) .map_err(|err| match err { @@ -2913,4 +2919,52 @@ mod tests { } } } + + #[tokio::test] + async fn test_register_invalid_and_valid_path() { + let broker = DataBroker::default(); + let broker = broker.authorized_access(&permissions::ALLOW_ALL); + + let error = broker + .add_entry( + "test. signal:3".to_owned(), + DataType::String, + ChangeType::OnChange, + EntryType::Sensor, + "Test signal 3".to_owned(), + Some(DataValue::Int32Array(Vec::from([1, 2, 3, 4]))), + ) + .await + .unwrap_err(); + assert_eq!(error, RegistrationError::ValidationError); + + let id = broker + .add_entry( + "Vehicle._kuksa.databroker.GitVersion.Do_you_not_like_smörgåstårta.tschö_mit_ö.東京_Москва_r#true".to_owned(), + DataType::Bool, + ChangeType::OnChange, + EntryType::Sensor, + "Test datapoint".to_owned(), + Some(DataValue::BoolArray(Vec::from([true]))), + ) + .await + .expect("Register datapoint should succeed"); + { + match broker.get_entry_by_id(id).await { + Ok(entry) => { + assert_eq!(entry.metadata.id, id); + assert_eq!(entry.metadata.path, "Vehicle._kuksa.databroker.GitVersion.Do_you_not_like_smörgåstårta.tschö_mit_ö.東京_Москва_r#true"); + assert_eq!(entry.metadata.data_type, DataType::Bool); + assert_eq!(entry.metadata.description, "Test datapoint"); + assert_eq!( + entry.metadata.allowed, + Some(DataValue::BoolArray(Vec::from([true]))) + ); + } + Err(_) => { + panic!("no metadata returned"); + } + } + } + } } diff --git a/kuksa_databroker/databroker/src/grpc/kuksa_val_v1/val.rs b/kuksa_databroker/databroker/src/grpc/kuksa_val_v1/val.rs index 6310aa19..b22e335c 100644 --- a/kuksa_databroker/databroker/src/grpc/kuksa_val_v1/val.rs +++ b/kuksa_databroker/databroker/src/grpc/kuksa_val_v1/val.rs @@ -72,17 +72,6 @@ impl proto::val_server::Val for broker::DataBroker { // Fill valid_requests structure. for request in requested { - if !glob::is_valid_pattern(&request.path) { - errors.push(proto::DataEntryError { - path: request.path, - error: Some(proto::Error { - code: 400, - reason: "bad_request".to_owned(), - message: "Bad Path Request".to_owned(), - }), - }); - continue; - } let view = proto::View::from_i32(request.view).ok_or_else(|| { tonic::Status::invalid_argument(format!("Invalid View (id: {}", request.view)) })?; diff --git a/kuksa_databroker/databroker/src/main.rs b/kuksa_databroker/databroker/src/main.rs index f9713832..c44c18f9 100644 --- a/kuksa_databroker/databroker/src/main.rs +++ b/kuksa_databroker/databroker/src/main.rs @@ -29,7 +29,7 @@ use tracing::{debug, error, info}; use clap::{Arg, ArgAction, Command}; -use databroker::{broker, glob, grpc, permissions, vss}; +use databroker::{broker, grpc, permissions, vss}; async fn shutdown_handler() { let mut sigint = @@ -111,55 +111,51 @@ async fn read_metadata_file<'a, 'b>( for (path, entry) in entries { debug!("Adding VSS datapoint type {}", path); - if glob::is_valid_pattern(&path) { - match database - .add_entry( - path.clone(), - entry.data_type, - databroker::broker::ChangeType::OnChange, - entry.entry_type, - entry.description, - entry.allowed, - ) - .await - { - Ok(id) => { - if let Some(default) = entry.default { - let ids = [( - id, - broker::EntryUpdate { - datapoint: Some(broker::Datapoint { - ts: std::time::SystemTime::now(), - value: default, - }), - path: None, - actuator_target: None, - entry_type: None, - data_type: None, - description: None, - allowed: None, - }, - )]; - if let Err(errors) = database.update_entries(ids).await { - // There's only one error (since we're only trying to set one) - if let Some(error) = errors.get(0) { - info!("Failed to set default value for {}: {:?}", path, error.1); - } + match database + .add_entry( + path.clone(), + entry.data_type, + databroker::broker::ChangeType::OnChange, + entry.entry_type, + entry.description, + entry.allowed, + ) + .await + { + Ok(id) => { + if let Some(default) = entry.default { + let ids = [( + id, + broker::EntryUpdate { + datapoint: Some(broker::Datapoint { + ts: std::time::SystemTime::now(), + value: default, + }), + path: None, + actuator_target: None, + entry_type: None, + data_type: None, + description: None, + allowed: None, + }, + )]; + if let Err(errors) = database.update_entries(ids).await { + // There's only one error (since we're only trying to set one) + if let Some(error) = errors.get(0) { + info!("Failed to set default value for {}: {:?}", path, error.1); } } } - Err(RegistrationError::PermissionDenied) => { - error!("Failed to add entry {path}: Permission denied") - } - Err(RegistrationError::PermissionExpired) => { - error!("Failed to add entry {path}: Permission expired") - } - Err(RegistrationError::ValidationError) => { - error!("Failed to add entry {path}: Validation failed") - } } - } else { - info!("Not valid path pattern for: {}", path); + Err(RegistrationError::PermissionDenied) => { + error!("Failed to add entry {path}: Permission denied") + } + Err(RegistrationError::PermissionExpired) => { + error!("Failed to add entry {path}: Permission expired") + } + Err(RegistrationError::ValidationError) => { + error!("Failed to add entry {path}: Validation failed") + } } } Ok(())