From ab6ac6475b6fc3ceaf1450a3574274384d0c5a64 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20Arg=C3=A9rus?= <external.john.argerus2@de.bosch.com>
Date: Thu, 14 Sep 2023 11:12:46 +0200
Subject: [PATCH] [databroker] Fix permissions bug. (#655)

Fix permissions bug inadvertently introduced when adding
`iter_entries()`.
---
 kuksa_databroker/databroker/src/broker.rs     | 69 ++++++++++++++++---
 .../src/grpc/sdv_databroker_v1/broker.rs      |  2 +-
 kuksa_databroker/databroker/src/main.rs       |  2 +-
 3 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/kuksa_databroker/databroker/src/broker.rs b/kuksa_databroker/databroker/src/broker.rs
index b1950806..5e9a18e3 100644
--- a/kuksa_databroker/databroker/src/broker.rs
+++ b/kuksa_databroker/databroker/src/broker.rs
@@ -39,7 +39,7 @@ pub enum UpdateError {
     PermissionExpired,
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub enum ReadError {
     NotFound,
     PermissionDenied,
@@ -850,16 +850,59 @@ pub struct DatabaseWriteAccess<'a, 'b> {
     permissions: &'b Permissions,
 }
 
-pub struct EntryIterator<'a> {
+pub enum EntryReadAccess<'a> {
+    Entry(&'a Entry),
+    Err(&'a Metadata, ReadError),
+}
+
+impl<'a> EntryReadAccess<'a> {
+    pub fn datapoint(&self) -> Result<&Datapoint, ReadError> {
+        match self {
+            Self::Entry(entry) => Ok(&entry.datapoint),
+            Self::Err(_, err) => Err(err.clone()),
+        }
+    }
+
+    pub fn actuator_target(&self) -> Result<&Option<Datapoint>, ReadError> {
+        match self {
+            Self::Entry(entry) => Ok(&entry.actuator_target),
+            Self::Err(_, err) => Err(err.clone()),
+        }
+    }
+
+    pub fn metadata(&self) -> &Metadata {
+        match self {
+            Self::Entry(entry) => &entry.metadata,
+            Self::Err(metadata, _) => metadata,
+        }
+    }
+}
+
+impl<'a> EntryReadAccess<'a> {
+    fn new(entry: &'a Entry, permissions: &Permissions) -> Self {
+        match permissions.can_read(&entry.metadata.path) {
+            Ok(()) => Self::Entry(entry),
+            Err(PermissionError::Denied) => Self::Err(&entry.metadata, ReadError::PermissionDenied),
+            Err(PermissionError::Expired) => {
+                Self::Err(&entry.metadata, ReadError::PermissionExpired)
+            }
+        }
+    }
+}
+
+pub struct EntryReadIterator<'a, 'b> {
     inner: std::collections::hash_map::Values<'a, i32, Entry>,
+    permissions: &'b Permissions,
 }
 
-impl<'a> Iterator for EntryIterator<'a> {
-    type Item = &'a Entry;
+impl<'a, 'b> Iterator for EntryReadIterator<'a, 'b> {
+    type Item = EntryReadAccess<'a>;
 
     #[inline]
     fn next(&mut self) -> Option<Self::Item> {
-        self.inner.next()
+        self.inner
+            .next()
+            .map(|entry| EntryReadAccess::new(entry, self.permissions))
     }
 
     #[inline]
@@ -910,9 +953,10 @@ impl<'a, 'b> DatabaseReadAccess<'a, 'b> {
         self.get_metadata_by_id(*id)
     }
 
-    pub fn iter_entries(&self) -> EntryIterator {
-        EntryIterator {
+    pub fn iter_entries(&self) -> EntryReadIterator {
+        EntryReadIterator {
             inner: self.db.entries.values(),
+            permissions: self.permissions,
         }
     }
 }
@@ -1206,7 +1250,7 @@ impl<'a, 'b> AuthorizedAccess<'a, 'b> {
             .cloned()
     }
 
-    pub async fn for_each_entry(&self, f: impl FnMut(&Entry)) {
+    pub async fn for_each_entry(&self, f: impl FnMut(EntryReadAccess)) {
         self.broker
             .database
             .read()
@@ -1216,7 +1260,7 @@ impl<'a, 'b> AuthorizedAccess<'a, 'b> {
             .for_each(f)
     }
 
-    pub async fn map_entries<T>(&self, f: impl FnMut(&Entry) -> T) -> Vec<T> {
+    pub async fn map_entries<T>(&self, f: impl FnMut(EntryReadAccess) -> T) -> Vec<T> {
         self.broker
             .database
             .read()
@@ -1227,7 +1271,10 @@ impl<'a, 'b> AuthorizedAccess<'a, 'b> {
             .collect()
     }
 
-    pub async fn filter_map_entries<T>(&self, f: impl FnMut(&Entry) -> Option<T>) -> Vec<T> {
+    pub async fn filter_map_entries<T>(
+        &self,
+        f: impl FnMut(EntryReadAccess) -> Option<T>,
+    ) -> Vec<T> {
         self.broker
             .database
             .read()
@@ -2880,7 +2927,7 @@ mod tests {
         // No permissions
         let permissions = Permissions::builder().build().unwrap();
         let broker = db.authorized_access(&permissions);
-        let metadata = broker.map_entries(|entry| entry.metadata.clone()).await;
+        let metadata = broker.map_entries(|entry| entry.metadata().clone()).await;
         for entry in metadata {
             match entry.path.as_str() {
                 "Vehicle.Test1" => assert_eq!(entry.id, id1),
diff --git a/kuksa_databroker/databroker/src/grpc/sdv_databroker_v1/broker.rs b/kuksa_databroker/databroker/src/grpc/sdv_databroker_v1/broker.rs
index c1ec94cf..086abfa3 100644
--- a/kuksa_databroker/databroker/src/grpc/sdv_databroker_v1/broker.rs
+++ b/kuksa_databroker/databroker/src/grpc/sdv_databroker_v1/broker.rs
@@ -208,7 +208,7 @@ impl proto::broker_server::Broker for broker::DataBroker {
 
         let list = if request.names.is_empty() {
             broker
-                .map_entries(|entry| proto::Metadata::from(&entry.metadata))
+                .map_entries(|entry| proto::Metadata::from(entry.metadata()))
                 .await
         } else {
             broker
diff --git a/kuksa_databroker/databroker/src/main.rs b/kuksa_databroker/databroker/src/main.rs
index 60177b41..c8acd52c 100644
--- a/kuksa_databroker/databroker/src/main.rs
+++ b/kuksa_databroker/databroker/src/main.rs
@@ -269,7 +269,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
             Arg::new("dummy-metadata")
                 .display_order(10)
                 .long("dummy-metadata")
-                .action(ArgAction::Set)
+                .action(ArgAction::SetTrue)
                 .help("Populate data broker with dummy metadata")
                 .required(false),
         );