From a712ad9ca85d4fd79a8439b0bbc4f361d98cea34 Mon Sep 17 00:00:00 2001 From: Andre Weber Date: Mon, 7 Oct 2024 08:12:44 +0200 Subject: [PATCH] Minor Refactorings --- databroker/src/broker.rs | 110 +++++++++--------------- databroker/src/grpc/kuksa_val_v2/val.rs | 11 +-- databroker/src/permissions.rs | 22 +++-- proto/kuksa/val/v2/val.proto | 2 +- 4 files changed, 61 insertions(+), 84 deletions(-) diff --git a/databroker/src/broker.rs b/databroker/src/broker.rs index bd54504e..dd02066f 100644 --- a/databroker/src/broker.rs +++ b/databroker/src/broker.rs @@ -705,40 +705,23 @@ impl Subscriptions { if sub.sender.is_closed() { info!("Subscriber gone: removing subscription"); false + } else if sub.permissions.expired() { + info!("Permissions of Subscriber expired: removing subscription"); + false } else { - match &sub.permissions.expired() { - Ok(()) => true, - Err(PermissionError::Expired) => { - info!("Token expired: removing subscription"); - false - } - Err(err) => { - info!("Error: {:?} -> removing subscription", err); - false - } - } + true } }); self.actuation_subscriptions.retain(|sub| { if !sub.actuation_provider.is_available() { - info!("actuation provider gone: removing subscription"); + info!("Provider gone: removing subscription"); + false + } else if sub.permissions.expired() { + info!("Permissions of Provider expired: removing subscription"); false } else { - match sub.permissions.expired() { - Ok(()) => true, - Err(PermissionError::Expired) => { - info!("Token expired for actuation provider: removing subscription"); - false - } - Err(err) => { - info!( - "actuation provider error: {:?} -> removing subscription", - err - ); - false - } - } + true } }); } @@ -1752,30 +1735,26 @@ impl<'a, 'b> AuthorizedAccess<'a, 'b> { match opt_actuation_subscription { Some(actuation_subscription) => { let is_expired = actuation_subscription.permissions.expired(); - match is_expired { - Err(_) => { - let message = format!( - "Permission for vss_ids {:?} expired", - actuation_subscription.vss_ids - ); - return Err((ActuationError::PermissionExpired, message)); - } - Ok(_) => { - if !actuation_subscription.actuation_provider.is_available() { - let message = - format!("Provider for vss_id {} does not exist", vss_id); - return Err((ActuationError::ProviderNotAvailable, message)); - } + if is_expired { + let message = format!( + "Permission for vss_ids {:?} expired", + actuation_subscription.vss_ids + ); + return Err((ActuationError::PermissionExpired, message)); + } - actuation_subscription - .actuation_provider - .actuate(actuation_changes) - .await? - } + if !actuation_subscription.actuation_provider.is_available() { + let message = format!("Provider for vss_id {} does not exist", vss_id); + return Err((ActuationError::ProviderNotAvailable, message)); } + + actuation_subscription + .actuation_provider + .actuate(actuation_changes) + .await? } None => { - let message = format!("actuation provider for vss_id {} not available", vss_id); + let message = format!("Provider for vss_id {} not available", vss_id); return Err((ActuationError::ProviderNotAvailable, message)); } } @@ -1834,29 +1813,26 @@ impl<'a, 'b> AuthorizedAccess<'a, 'b> { match opt_actuation_subscription { Some(actuation_subscription) => { let is_expired = actuation_subscription.permissions.expired(); - match is_expired { - Err(_) => { - let message = format!( - "Permission for vss_ids {:?} expired", - actuation_subscription.vss_ids - ); - Err((ActuationError::PermissionExpired, message)) - } - Ok(_) => { - if !actuation_subscription.actuation_provider.is_available() { - let message = format!("Provider for vss_id {} does not exist", vss_id); - return Err((ActuationError::ProviderNotAvailable, message)); - } + if is_expired { + let message = format!( + "Permission for vss_ids {:?} expired", + actuation_subscription.vss_ids + ); + return Err((ActuationError::PermissionExpired, message)); + } - actuation_subscription - .actuation_provider - .actuate(vec![ActuationChange { - id: vss_id, - data_value: data_value.clone(), - }]) - .await - } + if !actuation_subscription.actuation_provider.is_available() { + let message = format!("Provider for vss_id {} does not exist", vss_id); + return Err((ActuationError::ProviderNotAvailable, message)); } + + actuation_subscription + .actuation_provider + .actuate(vec![ActuationChange { + id: vss_id, + data_value: data_value.clone(), + }]) + .await } None => { let message = format!("Provider for vss_id {} does not exist", vss_id); diff --git a/databroker/src/grpc/kuksa_val_v2/val.rs b/databroker/src/grpc/kuksa_val_v2/val.rs index 4edcb897..aa1ff4f1 100644 --- a/databroker/src/grpc/kuksa_val_v2/val.rs +++ b/databroker/src/grpc/kuksa_val_v2/val.rs @@ -73,13 +73,6 @@ impl ActuationProvider for Provider { let result = self.sender.send(Ok(response)).await; if result.is_err() { - let send_error = result.unwrap_err().0; - if send_error.is_err() { - return Err(( - broker::ActuationError::TransmissionFailure, - "An error occured while sending the data".to_string(), - )); - } return Err(( broker::ActuationError::TransmissionFailure, "An error occured while sending the data".to_string(), @@ -197,7 +190,7 @@ impl proto::val_server::Val for broker::DataBroker { // // Returns (GRPC error code): // NOT_FOUND if the actuator does not exist. - // PERMISSION_DENIED if access is denied for of the actuator. + // PERMISSION_DENIED if access is denied for the actuator. // UNAVAILABLE if there is no provider currently providing the actuator // INVALID_ARGUMENT // - if the data type used in the request does not match @@ -813,7 +806,7 @@ async fn provide_actuation( for (index, opt_vss_id) in resolved_opt_vss_ids.iter().enumerate() { if opt_vss_id.is_none() { let message = format!( - "could not resolve id of vss_path: {}", + "Could not resolve id of vss_path: {}", vss_paths.get(index).unwrap() ); return Err(tonic::Status::not_found(message)); diff --git a/databroker/src/permissions.rs b/databroker/src/permissions.rs index 7da1eae1..79e7a5b2 100644 --- a/databroker/src/permissions.rs +++ b/databroker/src/permissions.rs @@ -165,7 +165,9 @@ impl Permissions { } pub fn can_read(&self, path: &str) -> Result<(), PermissionError> { - self.expired()?; + if self.expired() { + return Err(PermissionError::Expired); + } if self.read.is_match(path) { return Ok(()); @@ -187,7 +189,9 @@ impl Permissions { } pub fn can_write_actuator_target(&self, path: &str) -> Result<(), PermissionError> { - self.expired()?; + if self.expired() { + return Err(PermissionError::Expired); + } if self.actuate.is_match(path) { return Ok(()); @@ -196,7 +200,9 @@ impl Permissions { } pub fn can_write_datapoint(&self, path: &str) -> Result<(), PermissionError> { - self.expired()?; + if self.expired() { + return Err(PermissionError::Expired); + } if self.provide.is_match(path) { return Ok(()); @@ -205,7 +211,9 @@ impl Permissions { } pub fn can_create(&self, path: &str) -> Result<(), PermissionError> { - self.expired()?; + if self.expired() { + return Err(PermissionError::Expired); + } if self.create.is_match(path) { return Ok(()); @@ -214,13 +222,13 @@ impl Permissions { } #[inline] - pub fn expired(&self) -> Result<(), PermissionError> { + pub fn expired(&self) -> bool { if let Some(expires_at) = self.expires_at { if expires_at < SystemTime::now() { - return Err(PermissionError::Expired); + return true; } } - Ok(()) + false } } diff --git a/proto/kuksa/val/v2/val.proto b/proto/kuksa/val/v2/val.proto index fc1c5b3c..cbe49db1 100644 --- a/proto/kuksa/val/v2/val.proto +++ b/proto/kuksa/val/v2/val.proto @@ -60,7 +60,7 @@ service VAL { // // Returns (GRPC error code): // NOT_FOUND if the actuator does not exist. - // PERMISSION_DENIED if access is denied for of the actuator. + // PERMISSION_DENIED if access is denied for the actuator. // UNAVAILABLE if there is no provider currently providing the actuator // INVALID_ARGUMENT // - if the data type used in the request does not match