Better support existing files with CreateFile and CreateorInsertIntoFile (#239)

* Better support existing files with CreateFile and CreateorInsertIntoFile

* Skip instead of complete if found

* Fixup some permissions

* Fixup nix version used

* Use unix module instead of linux one

* Use unix module instead of linux one in other spot

* Also cover CreateDirectory

* Add some more tests

* Fixup messaging

* Corect some logic inversions

src/action/base/create_directory.rs

@@ -1,4 +1,4 @@
-use std::os::unix::prelude::PermissionsExt;
+use std::os::unix::fs::{MetadataExt, PermissionsExt};
 use std::path::{Path, PathBuf};
 
 use nix::unistd::{chown, Group, User};
@@ -32,25 +32,53 @@ impl CreateDirectory {
         mode: impl Into<Option<u32>>,
         force_prune_on_revert: bool,
     ) -> Result<StatefulAction<Self>, ActionError> {
-        let path = path.as_ref();
+        let path = path.as_ref().to_path_buf();
         let user = user.into();
         let group = group.into();
         let mode = mode.into();
 
         let action_state = if path.exists() {
-            let metadata = tokio::fs::metadata(path)
+            let metadata = tokio::fs::metadata(&path)
                 .await
-                .map_err(|e| ActionError::GettingMetadata(path.to_path_buf(), e))?;
+                .map_err(|e| ActionError::GettingMetadata(path.clone(), e))?;
-            if metadata.is_dir() {
+            if !metadata.is_dir() {
-                tracing::debug!(
-                    "Creating directory `{}` already complete, skipping",
-                    path.display(),
-                );
-                // TODO: Validate owner/group...
-                ActionState::Skipped
-            } else {
                 return Err(ActionError::Exists(path.to_owned()));
             }
+
+            // Does it have the right user/group?
+            if let Some(user) = &user {
+                // If the file exists, the user must also exist to be correct.
+                let expected_uid = User::from_name(user.as_str())
+                    .map_err(|e| ActionError::GettingUserId(user.clone(), e))?
+                    .ok_or_else(|| ActionError::NoUser(user.clone()))?
+                    .uid;
+                let found_uid = metadata.uid();
+                if found_uid != expected_uid.as_raw() {
+                    return Err(ActionError::PathUserMismatch(
+                        path.clone(),
+                        found_uid,
+                        expected_uid.as_raw(),
+                    ));
+                }
+            }
+            if let Some(group) = &group {
+                // If the file exists, the group must also exist to be correct.
+                let expected_gid = Group::from_name(group.as_str())
+                    .map_err(|e| ActionError::GettingGroupId(group.clone(), e))?
+                    .ok_or_else(|| ActionError::NoUser(group.clone()))?
+                    .gid;
+                let found_gid = metadata.gid();
+                if found_gid != expected_gid.as_raw() {
+                    return Err(ActionError::PathGroupMismatch(
+                        path.clone(),
+                        found_gid,
+                        expected_gid.as_raw(),
+                    ));
+                }
+            }
+
+            tracing::debug!("Creating directory `{}` already complete", path.display(),);
+            ActionState::Skipped
         } else {
             ActionState::Uncompleted
         };

src/action/base/create_file.rs

@@ -1,10 +1,13 @@
 use nix::unistd::{chown, Group, User};
 use tracing::{span, Span};
 
-use std::path::{Path, PathBuf};
+use std::{
+    os::{unix::fs::MetadataExt, unix::fs::PermissionsExt},
+    path::{Path, PathBuf},
+};
 use tokio::{
-    fs::{remove_file, OpenOptions},
+    fs::{remove_file, File, OpenOptions},
-    io::AsyncWriteExt,
+    io::{AsyncReadExt, AsyncWriteExt},
 };
 
 use crate::action::{Action, ActionDescription, ActionError, StatefulAction};
@@ -36,20 +39,87 @@ impl CreateFile {
         force: bool,
     ) -> Result<StatefulAction<Self>, ActionError> {
         let path = path.as_ref().to_path_buf();
-
+        let mode = mode.into();
-        if path.exists() && !force {
+        let user = user.into();
-            return Err(ActionError::Exists(path.to_path_buf()));
+        let group = group.into();
-        }
+        let this = Self {
-
-        Ok(Self {
             path,
-            user: user.into(),
+            user,
-            group: group.into(),
+            group,
-            mode: mode.into(),
+            mode,
             buf,
             force,
+        };
+
+        if this.path.exists() {
+            // If the path exists, perhaps we can just skip this
+            let mut file = File::open(&this.path)
+                .await
+                .map_err(|e| ActionError::Open(this.path.clone(), e))?;
+
+            let metadata = file
+                .metadata()
+                .await
+                .map_err(|e| ActionError::GettingMetadata(this.path.clone(), e))?;
+            if let Some(mode) = mode {
+                // Does the file have the right permissions?
+                let discovered_mode = metadata.permissions().mode();
+                if discovered_mode != mode {
+                    return Err(ActionError::PathModeMismatch(
+                        this.path.clone(),
+                        discovered_mode,
+                        mode,
+                    ));
+                }
+            }
+
+            // Does it have the right user/group?
+            if let Some(user) = &this.user {
+                // If the file exists, the user must also exist to be correct.
+                let expected_uid = User::from_name(user.as_str())
+                    .map_err(|e| ActionError::GettingUserId(user.clone(), e))?
+                    .ok_or_else(|| ActionError::NoUser(user.clone()))?
+                    .uid;
+                let found_uid = metadata.uid();
+                if found_uid != expected_uid.as_raw() {
+                    return Err(ActionError::PathUserMismatch(
+                        this.path.clone(),
+                        found_uid,
+                        expected_uid.as_raw(),
+                    ));
+                }
+            }
+            if let Some(group) = &this.group {
+                // If the file exists, the group must also exist to be correct.
+                let expected_gid = Group::from_name(group.as_str())
+                    .map_err(|e| ActionError::GettingGroupId(group.clone(), e))?
+                    .ok_or_else(|| ActionError::NoUser(group.clone()))?
+                    .gid;
+                let found_gid = metadata.gid();
+                if found_gid != expected_gid.as_raw() {
+                    return Err(ActionError::PathGroupMismatch(
+                        this.path.clone(),
+                        found_gid,
+                        expected_gid.as_raw(),
+                    ));
+                }
+            }
+
+            // Does it have the right content?
+            let mut discovered_buf = String::new();
+            file.read_to_string(&mut discovered_buf)
+                .await
+                .map_err(|e| ActionError::Read(this.path.clone(), e))?;
+
+            if discovered_buf != this.buf {
+                return Err(ActionError::Exists(this.path.clone()));
+            }
+
+            tracing::debug!("Creating file `{}` already complete", this.path.display());
+            return Ok(StatefulAction::completed(this));
         }
-        .into())
+
+        Ok(StatefulAction::uncompleted(this))
     }
 }
 
@@ -178,6 +248,8 @@ impl Action for CreateFile {
 #[cfg(test)]
 mod test {
     use super::*;
+    use eyre::eyre;
+    use tokio::fs::write;
 
     #[tokio::test]
     async fn creates_and_deletes_file() -> eyre::Result<()> {
@@ -206,7 +278,7 @@ mod test {
 
         action.try_execute().await?;
 
-        tokio::fs::write(test_file.as_path(), "More content").await?;
+        write(test_file.as_path(), "More content").await?;
 
         action.try_revert().await?;
 
@@ -214,4 +286,61 @@ mod test {
 
         Ok(())
     }
+
+    #[tokio::test]
+    async fn recognizes_existing_exact_files_and_reverts_them() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_file")?;
+        let test_file = temp_dir
+            .path()
+            .join("recognizes_existing_exact_files_and_reverts_them");
+
+        let test_content = "Some content";
+        write(test_file.as_path(), test_content).await?;
+
+        let mut action = CreateFile::plan(
+            test_file.clone(),
+            None,
+            None,
+            None,
+            test_content.into(),
+            false,
+        )
+        .await?;
+
+        action.try_execute().await?;
+
+        action.try_revert().await?;
+
+        assert!(!test_file.exists(), "File should have been deleted");
+
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn recognizes_existing_different_files_and_errors() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_file")?;
+        let test_file = temp_dir
+            .path()
+            .join("recognizes_existing_different_files_and_errors");
+
+        write(test_file.as_path(), "Some content").await?;
+
+        match CreateFile::plan(
+            test_file.clone(),
+            None,
+            None,
+            None,
+            "Some different content".into(),
+            false,
+        )
+        .await
+        {
+            Err(ActionError::Exists(path)) => assert_eq!(path, test_file.as_path()),
+            _ => return Err(eyre!("Should have returned an ActionError::Exists error")),
+        }
+
+        assert!(test_file.exists(), "File should have not been deleted");
+
+        Ok(())
+    }
 }

src/action/base/create_or_insert_into_file.rs

@@ -4,11 +4,11 @@ use crate::action::{Action, ActionDescription, ActionError, StatefulAction};
 use rand::Rng;
 use std::{
     io::SeekFrom,
-    os::unix::prelude::PermissionsExt,
+    os::{unix::fs::MetadataExt, unix::prelude::PermissionsExt},
     path::{Path, PathBuf},
 };
 use tokio::{
-    fs::{remove_file, OpenOptions},
+    fs::{remove_file, File, OpenOptions},
     io::{AsyncReadExt, AsyncSeekExt, AsyncWriteExt},
 };
 use tracing::{span, Span};
@@ -46,16 +46,86 @@ impl CreateOrInsertIntoFile {
         position: Position,
     ) -> Result<StatefulAction<Self>, ActionError> {
         let path = path.as_ref().to_path_buf();
-
+        let mode = mode.into();
-        Ok(Self {
+        let user = user.into();
+        let group = group.into();
+        let this = Self {
             path,
-            user: user.into(),
+            user,
-            group: group.into(),
+            group,
-            mode: mode.into(),
+            mode,
             buf,
             position,
+        };
+        if this.path.exists() {
+            // If the path exists, perhaps we can just skip this
+            let mut file = File::open(&this.path)
+                .await
+                .map_err(|e| ActionError::Open(this.path.clone(), e))?;
+
+            let metadata = file
+                .metadata()
+                .await
+                .map_err(|e| ActionError::GettingMetadata(this.path.clone(), e))?;
+            if let Some(mode) = mode {
+                // Does the file have the right permissions?
+                let discovered_mode = metadata.permissions().mode();
+                if discovered_mode != mode {
+                    return Err(ActionError::PathModeMismatch(
+                        this.path.clone(),
+                        discovered_mode,
+                        mode,
+                    ));
+                }
+            }
+
+            // Does it have the right user/group?
+            if let Some(user) = &this.user {
+                // If the file exists, the user must also exist to be correct.
+                let expected_uid = User::from_name(user.as_str())
+                    .map_err(|e| ActionError::GettingUserId(user.clone(), e))?
+                    .ok_or_else(|| ActionError::NoUser(user.clone()))?
+                    .uid;
+                let found_uid = metadata.uid();
+                if found_uid != expected_uid.as_raw() {
+                    return Err(ActionError::PathUserMismatch(
+                        this.path.clone(),
+                        found_uid,
+                        expected_uid.as_raw(),
+                    ));
+                }
+            }
+            if let Some(group) = &this.group {
+                // If the file exists, the group must also exist to be correct.
+                let expected_gid = Group::from_name(group.as_str())
+                    .map_err(|e| ActionError::GettingGroupId(group.clone(), e))?
+                    .ok_or_else(|| ActionError::NoUser(group.clone()))?
+                    .gid;
+                let found_gid = metadata.gid();
+                if found_gid != expected_gid.as_raw() {
+                    return Err(ActionError::PathGroupMismatch(
+                        this.path.clone(),
+                        found_gid,
+                        expected_gid.as_raw(),
+                    ));
+                }
+            }
+
+            // Does it have the right content?
+            let mut discovered_buf = String::new();
+            file.read_to_string(&mut discovered_buf)
+                .await
+                .map_err(|e| ActionError::Read(this.path.clone(), e))?;
+
+            if discovered_buf.contains(&this.buf) {
+                tracing::debug!("Inserting into `{}` already complete", this.path.display(),);
+                return Ok(StatefulAction::completed(this));
+            }
+
+            // If not, we can't skip this, so we still do it
         }
-        .into())
+
+        Ok(StatefulAction::uncompleted(this))
     }
 }
 
@@ -278,6 +348,7 @@ impl Action for CreateOrInsertIntoFile {
 #[cfg(test)]
 mod test {
     use super::*;
+    use tokio::fs::{read_to_string, write};
 
     #[tokio::test]
     async fn creates_and_deletes_file() -> eyre::Result<()> {
@@ -336,4 +407,42 @@ mod test {
 
         Ok(())
     }
+
+    #[tokio::test]
+    async fn recognizes_existing_containing_exact_contents_and_reverts_it() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_create_or_insert_into_file")?;
+        let test_file = temp_dir
+            .path()
+            .join("recognizes_existing_containing_exact_contents_and_reverts_it");
+
+        let expected_content = "Some expected content";
+        write(test_file.as_path(), expected_content).await?;
+
+        let added_content = "\nSome more expected content";
+        write(test_file.as_path(), added_content).await?;
+
+        // We test all `Position` options
+        let positions = [Position::Beginning, Position::End];
+        for position in positions {
+            let mut action = CreateOrInsertIntoFile::plan(
+                test_file.clone(),
+                None,
+                None,
+                None,
+                expected_content.into(),
+                position,
+            )
+            .await?;
+
+            action.try_execute().await?;
+
+            action.try_revert().await?;
+
+            assert!(test_file.exists(), "File should have not been deleted");
+            let after_revert_content = read_to_string(&test_file).await?;
+            assert_eq!(after_revert_content, added_content);
+        }
+
+        Ok(())
+    }
 }

src/action/common/configure_shell_profile.rs

@@ -70,7 +70,7 @@ impl ConfigureShellProfile {
                         profile_target_path,
                         None,
                         None,
-                        0o0755,
+                        None,
                         shell_buf.to_string(),
                         create_or_insert_into_file::Position::Beginning,
                     )

src/action/mod.rs

@@ -262,8 +262,16 @@ pub enum ActionError {
     }).collect::<Vec<_>>().join(" & "))]
     Children(Vec<Box<ActionError>>),
     /// The path already exists
-    #[error("Path exists `{0}`")]
+    #[error(
+        "`{0}` exists with different content than planned, consider removing it with `rm {0}`"
+    )]
     Exists(std::path::PathBuf),
+    #[error("`{0}` exists with a different uid ({1}) than planned ({2}), consider removing it with `rm {0}`")]
+    PathUserMismatch(std::path::PathBuf, u32, u32),
+    #[error("`{0}` exists with a different gid ({1}) than planned ({2}), consider removing it with `rm {0}`")]
+    PathGroupMismatch(std::path::PathBuf, u32, u32),
+    #[error("`{0}` exists with a different mode ({1:o}) than planned ({2:o}), consider removing it with `rm {0}`")]
+    PathModeMismatch(std::path::PathBuf, u32, u32),
     #[error("Getting metadata for {0}`")]
     GettingMetadata(std::path::PathBuf, #[source] std::io::Error),
     #[error("Creating directory `{0}`")]
@@ -339,7 +347,12 @@ pub enum ActionError {
 }
 
 impl HasExpectedErrors for ActionError {
-    fn expected(&self) -> Option<Box<dyn std::error::Error>> {
+    fn expected<'a>(&'a self) -> Option<Box<dyn std::error::Error + 'a>> {
-        None
+        match self {
+            Self::PathUserMismatch(_, _, _)
+            | Self::PathGroupMismatch(_, _, _)
+            | Self::PathModeMismatch(_, _, _) => Some(Box::new(self)),
+            _ => None,
+        }
     }
 }