Improve the common file library to avoid duplications

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

5 files changed, 154 insertions, 127 deletions

diff --git a/crates/common/tedge_utils/Cargo.toml b/crates/common/tedge_utils/Cargo.toml
index 426c41c1..95329275 100644
--- a/crates/common/tedge_utils/Cargo.toml
+++ b/crates/common/tedge_utils/Cargo.toml
@@ -18,13 +18,14 @@ futures = "0.3"
 nix = "0.23.1"
 tempfile = "3.2"
 thiserror = "1.0"
-tokio = { version = "1.12", default_features = false, features = [ "fs", "io-util", "macros", "signal", "rt-multi-thread"] }
+tokio = { version = "1.12", default_features = false, features = [ "fs", "io-util", "macros", "signal"] }
 tracing = { version = "0.1", features = [], optional = true }
 tracing-subscriber = { version = "0.3", optional = true, features = [ "time" ] }
 users = "0.11.0"
 
 [dev-dependencies]
 assert_matches = "1.5"
+tokio = { version = "1.12", features = [ "rt-multi-thread"] }
 whoami = "1.2.1"
 
 
diff --git a/crates/common/tedge_utils/src/file.rs b/crates/common/tedge_utils/src/file.rs
index 03cb68b0..663ecde1 100644
--- a/crates/common/tedge_utils/src/file.rs
+++ b/crates/common/tedge_utils/src/file.rs
@@ -36,22 +36,14 @@ pub fn create_directory_with_user_group(
     group: &str,
     mode: u32,
 ) -> Result<(), FileError> {
-    match fs::create_dir(dir) {
-        Ok(_) => {
-            change_owner_and_permission(dir, user, group, mode)?;
-        }
+    let perm_entry = PermissionEntry::new(Some(user.into()), Some(group.into()), Some(mode));
+    let () = perm_entry.create_directory(Path::new(dir))?;
+    Ok(())
+}
 
-        Err(e) => {
-            if e.kind() == io::ErrorKind::AlreadyExists {
-                return Ok(());
-            } else {
-                return Err(FileError::DirectoryCreateFailed {
-                    dir: dir.to_string(),
-                    from: e,
-                });
-            }
-        }
-    }
+pub fn create_directory_with_mode(dir: &str, mode: u32) -> Result<(), FileError> {
+    let perm_entry = PermissionEntry::new(None, None, Some(mode));
+    let () = perm_entry.create_directory(Path::new(dir))?;
     Ok(())
 }
 
@@ -61,36 +53,81 @@ pub fn create_file_with_user_group(
     group: &str,
     mode: u32,
 ) -> Result<(), FileError> {
-    match File::create(file) {
-        Ok(_) => {
-            change_owner_and_permission(file, user, group, mode)?;
+    let perm_entry = PermissionEntry::new(Some(user.into()), Some(group.into()), Some(mode));
+    let () = perm_entry.create_file(Path::new(file))?;
+    Ok(())
+}
+
+#[derive(Debug, PartialEq, Eq, Default, Clone)]
+pub struct PermissionEntry {
+    pub user: Option<String>,
+    pub group: Option<String>,
+    pub mode: Option<u32>,
+}
+
+impl PermissionEntry {
+    pub fn new(user: Option<String>, group: Option<String>, mode: Option<u32>) -> Self {
+        Self { user, group, mode }
+    }
+
+    pub fn apply(&self, path: &Path) -> Result<(), FileError> {
+        match (&self.user, &self.group) {
+            (Some(user), Some(group)) => {
+                let () = change_user_and_group(path, user, group)?;
+            }
+            (Some(user), None) => {
+                let () = change_user(path, user)?;
+            }
+            (None, Some(group)) => {
+                let () = change_group(path, group)?;
+            }
+            (None, None) => {}
         }
-        Err(e) => {
-            if e.kind() == io::ErrorKind::AlreadyExists {
-                return Ok(());
-            } else {
-                return Err(FileError::FileCreateFailed {
-                    file: file.to_string(),
-                    from: e,
-                });
+
+        if let Some(mode) = &self.mode {
+            let () = change_mode(path, *mode)?;
+        }
+
+        Ok(())
+    }
+
+    pub fn create_directory(&self, dir: &Path) -> Result<(), FileError> {
+        match fs::create_dir(dir) {
+            Ok(_) => {
+                let () = self.apply(dir)?;
+                Ok(())
             }
+            Err(e) if e.kind() == io::ErrorKind::AlreadyExists => Ok(()),
+            Err(e) => Err(FileError::DirectoryCreateFailed {
+                dir: dir.display().to_string(),
+                from: e,
+            }),
+        }
+    }
+
+    pub fn create_file(&self, file: &Path) -> Result<(), FileError> {
+        match File::create(file) {
+            Ok(_) => {
+                let () = self.apply(file)?;
+                Ok(())
+            }
+            Err(e) if e.kind() == io::ErrorKind::AlreadyExists => Ok(()),
+            Err(e) => Err(FileError::FileCreateFailed {
+                file: file.display().to_string(),
+                from: e,
+            }),
         }
     }
-    Ok(())
 }
 
-fn change_owner_and_permission(
-    file: &str,
-    user: &str,
-    group: &str,
-    mode: u32,
-) -> Result<(), FileError> {
+fn change_user_and_group(file: &Path, user: &str, group: &str) -> Result<(), FileError> {
     let ud = match get_user_by_name(user) {
         Some(user) => user.uid(),
         None => {
             return Err(FileError::UserNotFound { user: user.into() });
         }
     };
+    let uid = get_metadata(Path::new(file))?.st_uid();
 
     let gd = match get_group_by_name(group) {
         Some(group) => group.gid(),
@@ -100,53 +137,21 @@ fn change_owner_and_permission(
             });
         }
     };
-
-    let uid = get_metadata(Path::new(file))?.st_uid();
     let gid = get_metadata(Path::new(file))?.st_gid();
 
-    // if user and group is same as existing, then do not change
+    // if user and group are same as existing, then do not change
     if (ud != uid) && (gd != gid) {
-        chown(file, Some(Uid::from_raw(ud)), Some(Gid::from_raw(gd)))?;
+        chown(
+            file,
+            Some(Uid::from_raw(ud.into())),
+            Some(Gid::from_raw(gd.into())),
+        )?;
     }
 
-    let mut perm = get_metadata(Path::new(file))?.permissions();
-    perm.set_mode(mode);
-
-    fs::set_permissions(file, perm).map_err(|e| FileError::MetaDataError {
-        name: file.to_string(),
-        from: e,
-    })?;
-
     Ok(())
 }
 
-#[derive(Debug, PartialEq, Eq, Default, Clone)]
-pub struct FilePermissions {
-    pub user: Option<String>,
-    pub group: Option<String>,
-    pub mode: Option<u32>,
-}
-
-impl FilePermissions {
-    pub fn new(user: Option<String>, group: Option<String>, mode: Option<u32>) -> Self {
-        Self { user, group, mode }
-    }
-
-    pub fn change_permissions(&self, file: &str) -> Result<(), FileError> {
-        if let Some(user) = &self.user {
-            let () = change_user(file, user)?;
-        }
-        if let Some(group) = &self.group {
-            let () = change_group(file, group)?;
-        }
-        if let Some(mode) = &self.mode {
-            let () = change_mode(file, *mode)?;
-        }
-        Ok(())
-    }
-}
-
-fn change_user(file: &str, user: &str) -> Result<(), FileError> {
+fn change_user(file: &Path, user: &str) -> Result<(), FileError> {
     let ud = match get_user_by_name(user) {
         Some(user) => user.uid(),
         None => {
@@ -164,7 +169,7 @@ fn change_user(file: &str, user: &str) -> Result<(), FileError> {
     Ok(())
 }
 
-fn change_group(file: &str, group: &str) -> Result<(), FileError> {
+fn change_group(file: &Path, group: &str) -> Result<(), FileError> {
     let gd = match get_group_by_name(group) {
         Some(group) => group.gid(),
         None => {
@@ -184,12 +189,12 @@ fn change_group(file: &str, group: &str) -> Result<(), FileError> {
     Ok(())
 }
 
-fn change_mode(file: &str, mode: u32) -> Result<(), FileError> {
+fn change_mode(file: &Path, mode: u32) -> Result<(), FileError> {
     let mut perm = get_metadata(Path::new(file))?.permissions();
     perm.set_mode(mode);
 
     fs::set_permissions(file, perm).map_err(|e| FileError::MetaDataError {
-        name: file.to_string(),
+        name: file.display().to_string(),
         from: e,
     })?;
 
@@ -309,10 +314,8 @@ mod tests {
         let perm = meta.permissions();
         assert!(format!("{:o}", perm.mode()).contains("644"));
 
-        let file_permissions = FilePermissions::new(None, None, Some(0o444));
-        let () = file_permissions
-            .change_permissions(file_path.as_str())
-            .unwrap();
+        let permission_set = PermissionEntry::new(None, None, Some(0o444));
+        let () = permission_set.apply(Path::new(file_path.as_str())).unwrap();
 
         let meta = fs::metadata(file_path.as_str()).unwrap();
         let perm = meta.permissions();
diff --git a/plugins/c8y_configuration_plugin/src/config.rs b/plugins/c8y_configuration_plugin/src/config.rs
index 0c42a804..892d157a 100644
--- a/plugins/c8y_configuration_plugin/src/config.rs
+++ b/plugins/c8y_configuration_plugin/src/config.rs
@@ -7,8 +7,8 @@ use std::collections::HashSet;
 use std::fs;
 use std::hash::{Hash, Hasher};
 use std::path::Path;
-use tedge_utils::file::FilePermissions;
-use tracing::{info, warn};
+use tedge_utils::file::PermissionEntry;
+use tracing::{error, info};
 
 #[derive(Deserialize, Debug, Default)]
 #[serde(deny_unknown_fields)]
@@ -36,7 +36,7 @@ pub struct PluginConfig {
 pub struct FileEntry {
     pub path: String,
     config_type: String,
-    pub file_permissions: FilePermissions,
+    pub file_permissions: PermissionEntry,
 }
 
 impl Hash for FileEntry {
@@ -68,7 +68,7 @@ impl FileEntry {
         Self {
             path,
             config_type,
-            file_permissions: FilePermissions { user, group, mode },
+            file_permissions: PermissionEntry { user, group, mode },
         }
     }
 }
@@ -81,12 +81,12 @@ impl RawPluginConfig {
             Ok(contents) => match toml::from_str(contents.as_str()) {
                 Ok(config) => config,
                 Err(err) => {
-                    warn!("The config file {path_str} is malformed. {err}");
+                    error!("The config file {path_str} is malformed. {err}");
                     Self::default()
                 }
             },
             Err(err) => {
-                warn!("The config file {path_str} does not exist or is not readable. {err}");
+                error!("The config file {path_str} does not exist or is not readable. {err}");
                 Self::default()
             }
         }
@@ -121,7 +121,7 @@ impl PluginConfig {
                 .unwrap_or_else(|| raw_entry.path.clone());
 
             if config_type.contains(&['+', '#']) {
-                warn!(
+                error!(
                     "The config type '{}' contains the forbidden characters, '+' or '#'.",
                     config_type
                 );
@@ -136,7 +136,7 @@ impl PluginConfig {
                 raw_entry.mode,
             );
             if !self.files.insert(entry) {
-                warn!("The config file has the duplicated type '{}'.", config_type);
+                error!("The config file has the duplicated type '{}'.", config_type);
                 return original_plugin_config;
             }
         }
@@ -207,6 +207,12 @@ mod tests {
             [[files]]
                 path = "/etc/mosquitto/mosquitto.conf"
                 type = "'single quotation'"
+            [[files]]
+                path = "path"
+                type = "type"
+                user = "user"
+                group = "group"
+                mode = 0o444
              "#,
         )
         .unwrap();
@@ -233,7 +239,14 @@ mod tests {
                 RawFileEntry::new_with_path_and_type(
                     "/etc/mosquitto/mosquitto.conf".to_string(),
                     Some("'single quotation'".to_string())
-                )
+                ),
+                RawFileEntry {
+                    path: "path".to_string(),
+                    config_type: Some("type".to_string()),
+                    user: Some("user".to_string()),
+                    group: Some("group".to_string()),
+                    mode: Some(0o444)
+                }
             ]
         );
     }
@@ -248,6 +261,7 @@ mod tests {
                    { path = "/etc/tedge/mosquitto-conf/c8y-bridge.conf" },
                    { path = "/etc/tedge/mosquitto-conf/tedge-mosquitto.conf", type = '"double quotation"' },
                    { path = "/etc/mosquitto/mosquitto.conf", type = "'single quotation'" },
+                   { path = "path", type = "type", user = "user", group = "group", mode = 0o444 },
                  ]
              "#,
         )
@@ -275,7 +289,14 @@ mod tests {
                 RawFileEntry::new_with_path_and_type(
                     "/etc/mosquitto/mosquitto.conf".to_string(),
                     Some("'single quotation'".to_string())
-                )
+                ),
+                RawFileEntry {
+                    path: "path".to_string(),
+                    config_type: Some("type".to_string()),
+                    user: Some("user".to_string()),
+                    group: Some("group".to_string()),
+                    mode: Some(0o444)
+                }
             ]
         );
     }
@@ -392,6 +413,31 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn get_smartrest_single_type() {
+        let plugin_config = PluginConfig {
+            files: HashSet::from([FileEntry::new_with_path_and_type(
+                "/path/to/file".to_string(),
+                "typeA".to_string(),
+            )]),
+        };
+        let output = plugin_config.to_smartrest_payload();
+        assert_eq!(output, "119,typeA");
+    }
+
+    #[test]
+    fn get_smartrest_multiple_types() {
+        let plugin_config = PluginConfig {
+            files: HashSet::from([
+                FileEntry::new_with_path_and_type("path1".to_string(), "typeA".to_string()),
+                FileEntry::new_with_path_and_type("path2".to_string(), "typeB".to_string()),
+                FileEntry::new_with_path_and_type("path3".to_string(), "typeC".to_string()),
+            ]),
+        };
+        let output = plugin_config.to_smartrest_payload();
+        assert_eq!(output, ("119,typeA,typeB,typeC"));
+    }
+
     impl RawFileEntry {
         pub fn new_with_path_and_type(path: String, config_type: Option<String>) -> Self {
             Self {
@@ -409,7 +455,7 @@ mod tests {
             Self {
                 path,
                 config_type,
-                file_permissions: FilePermissions::default(),
+                file_permissions: PermissionEntry::default(),
             }
         }
     }
@@ -432,29 +478,4 @@ mod tests {
         file.write_all(content.as_bytes())?;
         Ok((temp_dir, config_root))
     }
-
-    #[test]
-    fn get_smartrest_single_type() {
-        let plugin_config = PluginConfig {
-            files: HashSet::from([FileEntry::new_with_path_and_type(
-                "/path/to/file".to_string(),
-                "typeA".to_string(),
-            )]),
-        };
-        let output = plugin_config.to_smartrest_payload();
-        assert_eq!(output, "119,typeA");
-    }
-
-    #[test]
-    fn get_smartrest_multiple_types() {
-        let plugin_config = PluginConfig {
-            files: HashSet::from([
-                FileEntry::new_with_path_and_type("path1".to_string(), "typeA".to_string()),
-                FileEntry::new_with_path_and_type("path2".to_string(), "typeB".to_string()),
-                FileEntry::new_with_path_and_type("path3".to_string(), "typeC".to_string()),
-            ]),
-        };
-        let output = plugin_config.to_smartrest_payload();
-        assert_eq!(output, ("119,typeA,typeB,typeC"));
-    }
 }
diff --git a/plugins/c8y_configuration_plugin/src/download.rs b/plugins/c8y_configuration_plugin/src/download.rs
index 9a345385..b281ec78 100644
--- a/plugins/c8y_configuration_plugin/src/download.rs
+++ b/plugins/c8y_configuration_plugin/src/download.rs
@@ -14,7 +14,7 @@ use serde_json::json;
 use std::fs;
 use std::os::unix::fs::PermissionsExt;
 use std::path::PathBuf;
-use tedge_utils::file::{get_filename, get_metadata, FilePermissions};
+use tedge_utils::file::{get_filename, get_metadata, PermissionEntry};
 use tracing::{info, warn};
 
 pub async fn handle_config_download_request(
@@ -64,7 +64,7 @@ async fn download_config_file(
     download_url: &str,
     file_path: PathBuf,
     tmp_dir: PathBuf,
-    file_permissions: FilePermissions,
+    file_permissions: PermissionEntry,
     http_client: &mut impl C8YHttpProxy,
 ) -> Result<(), anyhow::Error> {
     // Convert smartrest request to config download request struct
@@ -97,7 +97,7 @@ pub struct ConfigDownloadRequest {
     pub download_info: DownloadInfo,
     pub file_path: PathBuf,
     pub tmp_dir: PathBuf,
-    pub file_permissions: FilePermissions,
+    pub file_permissions: PermissionEntry,
     pub file_name: String,
 }
 
@@ -106,7 +106,7 @@ impl ConfigDownloadRequest {
         download_url: &str,
         file_path: PathBuf,
         tmp_dir: PathBuf,
-        file_permissions: FilePermissions,
+        file_permissions: PermissionEntry,
     ) -> Result<Self, ConfigManagementError> {
         let file_name = get_filename(file_path.clone()).ok_or_else(|| {
             ConfigManagementError::FileNameNotFound {
@@ -127,7 +127,6 @@ impl ConfigDownloadRequest {
     }
 
     fn has_write_access(&self) -> Result<(), ConfigManagementError> {
-        // The file does not exist before downloading a file
         let metadata =
             if self.file_path.is_file() {
                 get_metadata(&self.file_path)?
@@ -175,13 +174,13 @@ impl ConfigDownloadRequest {
 
         let file_permissions = if let Some(mode) = original_permission_mode {
             // Use the same file permission as the original one
-            FilePermissions::new(None, None, Some(mode))
+            PermissionEntry::new(None, None, Some(mode))
         } else {
             // Set the user, group, and mode as given for a new file
             self.file_permissions.clone()
         };
 
-        let () = file_permissions.change_permissions(&self.file_path.display().to_string())?;
+        let () = file_permissions.apply(&self.file_path)?;
 
         Ok(())
     }
@@ -232,7 +231,7 @@ mod tests {
             "https://test.cumulocity.com/inventory/binaries/70208",
             PathBuf::from("/etc/tedge/tedge.toml"),
             PathBuf::from("/tmp"),
-            FilePermissions::default(),
+            PermissionEntry::default(),
         )?;
 
         assert_eq!(
@@ -244,7 +243,7 @@ mod tests {
                 },
                 file_path: PathBuf::from("/etc/tedge/tedge.toml"),
                 tmp_dir: PathBuf::from("/tmp"),
-                file_permissions: FilePermissions::new(None, None, None),
+                file_permissions: PermissionEntry::new(None, None, None),
                 file_name: "tedge.toml".to_string()
             }
         );
@@ -257,7 +256,7 @@ mod tests {
             "https://test.cumulocity.com/inventory/binaries/70208",
             PathBuf::from("/"),
             PathBuf::from("/tmp"),
-            FilePermissions::default(),
+            PermissionEntry::default(),
         )
         .unwrap_err();
 
diff --git a/plugins/c8y_configuration_plugin/src/main.rs b/plugins/c8y_configuration_plugin/src/main.rs
index 71b1d9ea..b8d56fa0 100644
--- a/plugins/c8y_configuration_plugin/src/main.rs
+++ b/plugins/c8y_configuration_plugin/src/main.rs
@@ -19,7 +19,9 @@ use tedge_config::{
     ConfigRepository, ConfigSettingAccessor, MqttPortSetting, TEdgeConfig, TmpPathSetting,
     DEFAULT_TEDGE_CONFIG_PATH,
 };
-use tedge_utils::file::{create_directory_with_user_group, create_file_with_user_group};
+use tedge_utils::file::{
+    create_directory_with_mode, create_directory_with_user_group, create_file_with_user_group,
+};
 use tracing::{debug, error, info};
 
 pub const DEFAULT_PLUGIN_CONFIG_FILE_PATH: &str = "/etc/tedge/c8y/c8y-configuration-plugin.toml";
@@ -203,6 +205,7 @@ fn init(cfg_dir: PathBuf) -> Result<(), anyhow::Error> {
 }
 
 fn create_operation_files(config_dir: &str) -> Result<(), anyhow::Error> {
+    create_directory_with_mode(&format!("{config_dir}/c8y"), 0o755)?;
     create_directory_with_user_group(
         &format!("{config_dir}/operations/c8y"),
         "tedge",