From aa8526adba5d6e797b356086d85bdb7958bd8390 Mon Sep 17 00:00:00 2001 From: rafaeling Date: Fri, 29 Sep 2023 15:10:11 +0200 Subject: [PATCH] Support all valid vss paths --- kuksa_databroker/databroker/src/broker.rs | 56 +++++++- kuksa_databroker/databroker/src/glob.rs | 120 ++++++++++++++---- .../databroker/src/grpc/kuksa_val_v1/val.rs | 5 +- 3 files changed, 151 insertions(+), 30 deletions(-) diff --git a/kuksa_databroker/databroker/src/broker.rs b/kuksa_databroker/databroker/src/broker.rs index 330e516a..9ba3091d 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_path(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/glob.rs b/kuksa_databroker/databroker/src/glob.rs index 390d810f..b156f680 100644 --- a/kuksa_databroker/databroker/src/glob.rs +++ b/kuksa_databroker/databroker/src/glob.rs @@ -40,21 +40,21 @@ pub fn to_regex_string(glob: &str) -> String { if glob.starts_with("**.") { regex_string = regex_string.replace("**\\.", ".+\\."); } else if glob.starts_with("*.") { - regex_string = regex_string.replace("*\\.", "[^.]+\\."); + regex_string = regex_string.replace("*\\.", "[^.\\s\\:]+\\."); } if glob.ends_with(".*") { - regex_string = regex_string.replace("\\.*", "\\.[^.]+"); + regex_string = regex_string.replace("\\.*", "\\.[^.\\s\\:]+"); } else if glob.ends_with(".**") { regex_string = regex_string.replace("\\.**", "\\..*"); } if glob.contains(".**.") { - regex_string = regex_string.replace("\\.**", "[\\.[A-Z][a-zA-Z0-9]*]*"); + regex_string = regex_string.replace("\\.**", "[\\.[^.\\s\\:]+]*"); } if glob.contains(".*.") { - regex_string = regex_string.replace("\\.*", "\\.[^.]+"); + regex_string = regex_string.replace("\\.*", "\\.[^.\\s\\:]+"); } } @@ -77,7 +77,7 @@ lazy_static! { ^ # anchor at start (only match full paths) # At least one subpath (consisting of either of three): (?: - [A-Z][a-zA-Z0-9]* # An alphanumeric name (first letter capitalized) + [^\.\s\:]+ # Any character except :, whitespace and . | \* # An asterisk | @@ -88,12 +88,12 @@ lazy_static! { (?: \. # Separator, literal dot (?: - [A-Z][a-zA-Z0-9]* # Alphanumeric name (first letter capitalized) + [^\.\s\:]+ # Any character except :, whitespace and . | \* # An asterisk | \*\* # A double asterisk - ) + )+ )* $ # anchor at end (to match only full paths) ", @@ -105,11 +105,40 @@ pub fn is_valid_pattern(input: &str) -> bool { REGEX_VALID_PATTERN.is_match(input) } +lazy_static! { + static ref REGEX_VALID_PATH: regex::Regex = regex::Regex::new( + r"(?x) + ^ # anchor at start (only match full paths) + # At least one subpath (consisting of either of three): + (?: + [^\.\s\:\*]+ # Any character except :, whitespace and . + ) + # which can be followed by ( separator + another subpath ) + # repeated any number of times + (?: + \. # Separator, literal dot + (?: + [^\.\s\:\*]+ # Any character except :, whitespace and . + )+ + )* + $ # anchor at end (to match only full paths) + ", + ) + .expect("regex compilation (of static path) should always succeed"); +} + +pub fn is_valid_path(input: &str) -> bool { + REGEX_VALID_PATH.is_match(input) +} + #[cfg(test)] mod tests { use super::*; static ALL_SIGNALS: &[&str] = &[ + "Vehicle._kuksa.databroker.GitVersion", + "Vehicle._kuksa.databroker.CargoVersion", + "Vehicle._kuksa.databroker.GitVersion", "Vehicle.ADAS.ABS.IsEnabled", "Vehicle.ADAS.ABS.IsEngaged", "Vehicle.ADAS.ABS.IsError", @@ -1250,7 +1279,7 @@ mod tests { #[test] fn test_regex_wildcard_at_end() { assert!(using_glob("Vehicle.Cabin.Sunroof.*") - .should_equal_regex_pattern(r"^Vehicle\.Cabin\.Sunroof\.[^.]+$")); + .should_equal_regex_pattern(r"^Vehicle\.Cabin\.Sunroof\.[^.\s\:]+$")); } #[test] @@ -1267,7 +1296,7 @@ mod tests { #[test] fn test_regex_single_wildcard_in_middle() { assert!(using_glob("Vehicle.Cabin.Sunroof.*.Position") - .should_equal_regex_pattern(r"^Vehicle\.Cabin\.Sunroof\.[^.]+\.Position$")); + .should_equal_regex_pattern(r"^Vehicle\.Cabin\.Sunroof\.[^.\s\:]+\.Position$")); } #[test] @@ -1357,7 +1386,7 @@ mod tests { #[ignore] #[test] fn test_regex_single_wildcard_at_the_beginning() { - assert!(using_glob("*.Sunroof").should_equal_regex_pattern(r"^[^.]+\.Sunroof$")); + assert!(using_glob("*.Sunroof").should_equal_regex_pattern(r"^[^.\s\:]+\.Sunroof$")); } #[test] @@ -1380,6 +1409,17 @@ mod tests { .should_match_no_signals()); } + #[test] + fn test_matches_underscore_cases() { + assert!(using_glob("Vehicle._kuksa.**") + .with_signals(ALL_SIGNALS) + .should_match_signals(&[ + "Vehicle._kuksa.databroker.GitVersion", + "Vehicle._kuksa.databroker.CargoVersion", + "Vehicle._kuksa.databroker.GitVersion", + ],)); + } + #[test] fn test_is_valid_pattern() { assert!(is_valid_pattern("String.*")); @@ -1387,7 +1427,6 @@ mod tests { assert!(is_valid_pattern("Vehicle.Chassis.Axle.Row2.Wheel.*")); assert!(is_valid_pattern("String.String.String.String.*")); assert!(is_valid_pattern("String.String.String.String.**")); - assert!(is_valid_pattern("String.String.String.String")); assert!(is_valid_pattern("String.String.String.String.String.**")); assert!(is_valid_pattern("String.String.String.*.String")); assert!(is_valid_pattern("String.String.String.**.String")); @@ -1397,28 +1436,55 @@ mod tests { assert!(is_valid_pattern( "String.String.String.String.*.String.String" )); - assert!(is_valid_pattern("String.*.String.String")); assert!(is_valid_pattern("String.String.**.String.String")); assert!(is_valid_pattern("String.**.String.String")); assert!(is_valid_pattern("**.String.String.String.**")); - assert!(is_valid_pattern("**.String.String.String.*")); + assert!(is_valid_pattern("**.String.String.9_tring.*")); + assert!(is_valid_pattern("**.string.String.String.*")); + assert!(is_valid_pattern("**._string.String.String.*")); + assert!(is_valid_pattern("String._kuksa.tring.9tring.*")); assert!(is_valid_pattern("**.String")); assert!(is_valid_pattern("*.String.String.String")); assert!(is_valid_pattern("*.String")); - assert!(!is_valid_pattern("String.String.String.")); - assert!(!is_valid_pattern("String.String.String.String..")); + assert!(is_valid_pattern("*.String._")); + assert!(!is_valid_pattern("String.*.String.String..")); assert!(!is_valid_pattern("*.String.String.String..")); + assert!(!is_valid_pattern("String.**.St ring.String")); + assert!(!is_valid_pattern("String.**:String. String")); + assert!(!is_valid_pattern("String.**.St. .ring.String")); + assert!(!is_valid_pattern("String.**.St. : .ring.String")); + } + + #[test] + fn test_is_valid_path() { + assert!(is_valid_path("String.String.String.String")); + assert!(is_valid_path("String._kuksa.tring.9tring")); + + assert!(is_valid_path("Vehicle.Con_ñ_de_España,_sí")); + assert!(is_valid_path("Vehicle.Do_you_not_like_smörgåstårta")); + assert!(is_valid_path("Vehicle.tschö_mit_ö")); + assert!(is_valid_path("Vehicle.wie_heißt_das_lied")); + assert!(is_valid_path("Vehicle.東京_Москва_r#true")); + + assert!(!is_valid_path("String.String.String.")); + assert!(!is_valid_path("String.String.String.String..")); + assert!(!is_valid_path("String:String.String")); + assert!(!is_valid_path("String.St ring.String")); + assert!(!is_valid_path("String:String. String")); + assert!(!is_valid_path("String.St. .ring.String")); + assert!(!is_valid_path("String.St. : .ring.String")); + assert!(!is_valid_path("*.String:String. String")); } #[test] fn test_valid_regex_path() { - assert_eq!(to_regex_string("String.*"), "^String\\.[^.]+$"); + assert_eq!(to_regex_string("String.*"), "^String\\.[^.\\s\\:]+$"); assert_eq!(to_regex_string("String.**"), "^String\\..*$"); assert_eq!( to_regex_string("String.String.String.String.*"), - "^String\\.String\\.String\\.String\\.[^.]+$" + "^String\\.String\\.String\\.String\\.[^.\\s\\:]+$" ); assert_eq!( to_regex_string("String.String.String.String.**"), @@ -1434,31 +1500,31 @@ mod tests { ); assert_eq!( to_regex_string("String.String.String.*.String"), - "^String\\.String\\.String\\.[^.]+\\.String$" + "^String\\.String\\.String\\.[^.\\s\\:]+\\.String$" ); assert_eq!( to_regex_string("String.String.String.**.String"), - "^String\\.String\\.String[\\.[A-Z][a-zA-Z0-9]*]*\\.String$" + "^String\\.String\\.String[\\.[^.\\s\\:]+]*\\.String$" ); assert_eq!( to_regex_string("String.String.String.String.String.**.String"), - "^String\\.String\\.String\\.String\\.String[\\.[A-Z][a-zA-Z0-9]*]*\\.String$" + "^String\\.String\\.String\\.String\\.String[\\.[^.\\s\\:]+]*\\.String$" ); assert_eq!( to_regex_string("String.String.String.String.*.String.String"), - "^String\\.String\\.String\\.String\\.[^.]+\\.String\\.String$" + "^String\\.String\\.String\\.String\\.[^.\\s\\:]+\\.String\\.String$" ); assert_eq!( to_regex_string("String.*.String.String"), - "^String\\.[^.]+\\.String\\.String$" + "^String\\.[^.\\s\\:]+\\.String\\.String$" ); assert_eq!( to_regex_string("String.String.**.String.String"), - "^String\\.String[\\.[A-Z][a-zA-Z0-9]*]*\\.String\\.String$" + "^String\\.String[\\.[^.\\s\\:]+]*\\.String\\.String$" ); assert_eq!( to_regex_string("String.**.String.String"), - "^String[\\.[A-Z][a-zA-Z0-9]*]*\\.String\\.String$" + "^String[\\.[^.\\s\\:]+]*\\.String\\.String$" ); assert_eq!( to_regex_string("**.String.String.String.**"), @@ -1466,13 +1532,13 @@ mod tests { ); assert_eq!( to_regex_string("**.String.String.String.*"), - "^.+\\.String\\.String\\.String\\.[^.]+$" + "^.+\\.String\\.String\\.String\\.[^.\\s\\:]+$" ); assert_eq!(to_regex_string("**.String"), "^.+\\.String$"); - assert_eq!(to_regex_string("*.String"), "^[^.]+\\.String$"); + assert_eq!(to_regex_string("*.String"), "^[^.\\s\\:]+\\.String$"); assert_eq!( to_regex_string("*.String.String.String"), - "^[^.]+\\.String\\.String\\.String$" + "^[^.\\s\\:]+\\.String\\.String\\.String$" ); } } 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..28637fca 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,18 @@ impl proto::val_server::Val for broker::DataBroker { // Fill valid_requests structure. for request in requested { - if !glob::is_valid_pattern(&request.path) { + if request.path.contains('*') && !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(), + message: "Bad Wildcard Pattern 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)) })?;