Improve permissions checking when dealing with existing files (#267)

* create_file: check if path is file, improve permissions checking

* create_or_insert_into_file: check if path is file, improve permissions checking

* fixup: 100xxx -> xxx modes

* fixup: suggest legal mode for chmod

src/action/base/create_file.rs

@@ -61,9 +61,17 @@ impl CreateFile {
                 .metadata()
                 .await
                 .map_err(|e| ActionError::GettingMetadata(this.path.clone(), e))?;
+
+            if !metadata.is_file() {
+                return Err(ActionError::PathWasNotFile(this.path));
+            }
+
             if let Some(mode) = mode {
                 // Does the file have the right permissions?
                 let discovered_mode = metadata.permissions().mode();
+                // We only care about user-group-other permissions
+                let discovered_mode = discovered_mode & 0o777;
+
                 if discovered_mode != mode {
                     return Err(ActionError::PathModeMismatch(
                         this.path.clone(),
@@ -343,4 +351,96 @@ mod test {
 
         Ok(())
     }
+
+    #[tokio::test]
+    async fn recognizes_wrong_mode_and_errors() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_file")?;
+        let test_file = temp_dir.path().join("recognizes_wrong_mode_and_errors");
+        let initial_mode = 0o777;
+        let expected_mode = 0o000;
+
+        write(test_file.as_path(), "Some content").await?;
+        tokio::fs::set_permissions(test_file.as_path(), PermissionsExt::from_mode(initial_mode))
+            .await?;
+
+        match CreateFile::plan(
+            test_file.clone(),
+            None,
+            None,
+            Some(expected_mode),
+            "Some different content".into(),
+            false,
+        )
+        .await
+        {
+            Err(ActionError::PathModeMismatch(path, got, expected)) => {
+                assert_eq!(path, test_file.as_path());
+                assert_eq!(expected, expected_mode);
+                assert_eq!(got, initial_mode);
+            },
+            _ => {
+                return Err(eyre!(
+                    "Should have returned an ActionError::PathModeMismatch error"
+                ))
+            },
+        }
+
+        assert!(test_file.exists(), "File should have not been deleted");
+
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn recognizes_correct_mode() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_file")?;
+        let test_file = temp_dir.path().join("recognizes_correct_mode");
+        let initial_mode = 0o777;
+
+        write(test_file.as_path(), "Some content").await?;
+        tokio::fs::set_permissions(test_file.as_path(), PermissionsExt::from_mode(initial_mode))
+            .await?;
+
+        let mut action = CreateFile::plan(
+            test_file.clone(),
+            None,
+            None,
+            Some(initial_mode),
+            "Some 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 errors_on_dir() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_file")?;
+
+        match CreateFile::plan(
+            temp_dir.path(),
+            None,
+            None,
+            None,
+            "Some different content".into(),
+            false,
+        )
+        .await
+        {
+            Err(ActionError::PathWasNotFile(path)) => assert_eq!(path, temp_dir.path()),
+            _ => {
+                return Err(eyre!(
+                    "Should have returned an ActionError::PathWasNotFile error"
+                ))
+            },
+        }
+
+        Ok(())
+    }
 }

src/action/base/create_or_insert_into_file.rs

@@ -67,9 +67,17 @@ impl CreateOrInsertIntoFile {
                 .metadata()
                 .await
                 .map_err(|e| ActionError::GettingMetadata(this.path.clone(), e))?;
+
+            if !metadata.is_file() {
+                return Err(ActionError::PathWasNotFile(this.path));
+            }
+
             if let Some(mode) = mode {
                 // Does the file have the right permissions?
                 let discovered_mode = metadata.permissions().mode();
+                // We only care about user-group-other permissions
+                let discovered_mode = discovered_mode & 0o777;
+
                 if discovered_mode != mode {
                     return Err(ActionError::PathModeMismatch(
                         this.path.clone(),
@@ -348,6 +356,7 @@ impl Action for CreateOrInsertIntoFile {
 #[cfg(test)]
 mod test {
     use super::*;
+    use eyre::eyre;
     use tokio::fs::{read_to_string, write};
 
     #[tokio::test]
@@ -445,4 +454,96 @@ mod test {
 
         Ok(())
     }
+
+    #[tokio::test]
+    async fn recognizes_wrong_mode_and_errors() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_or_insert_into_file")?;
+        let test_file = temp_dir.path().join("recognizes_wrong_mode_and_errors");
+        let initial_mode = 0o777;
+        let expected_mode = 0o000;
+
+        write(test_file.as_path(), "Some content").await?;
+        tokio::fs::set_permissions(test_file.as_path(), PermissionsExt::from_mode(initial_mode))
+            .await?;
+
+        match CreateOrInsertIntoFile::plan(
+            test_file.clone(),
+            None,
+            None,
+            Some(expected_mode),
+            "Some different content".into(),
+            Position::End,
+        )
+        .await
+        {
+            Err(ActionError::PathModeMismatch(path, got, expected)) => {
+                assert_eq!(path, test_file.as_path());
+                assert_eq!(expected, expected_mode);
+                assert_eq!(got, initial_mode);
+            },
+            _ => {
+                return Err(eyre!(
+                    "Should have returned an ActionError::PathModeMismatch error"
+                ))
+            },
+        }
+
+        assert!(test_file.exists(), "File should have not been deleted");
+
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn recognizes_correct_mode() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_or_insert_into_file")?;
+        let test_file = temp_dir.path().join("recognizes_correct_mode");
+        let initial_mode = 0o777;
+
+        write(test_file.as_path(), "Some content").await?;
+        tokio::fs::set_permissions(test_file.as_path(), PermissionsExt::from_mode(initial_mode))
+            .await?;
+
+        let mut action = CreateOrInsertIntoFile::plan(
+            test_file.clone(),
+            None,
+            None,
+            Some(initial_mode),
+            "Some content".into(),
+            Position::End,
+        )
+        .await?;
+
+        action.try_execute().await?;
+
+        action.try_revert().await?;
+
+        assert!(!test_file.exists(), "File should have been deleted");
+
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn errors_on_dir() -> eyre::Result<()> {
+        let temp_dir = tempdir::TempDir::new("nix_installer_tests_create_or_insert_into_file")?;
+
+        match CreateOrInsertIntoFile::plan(
+            temp_dir.path(),
+            None,
+            None,
+            None,
+            "Some different content".into(),
+            Position::End,
+        )
+        .await
+        {
+            Err(ActionError::PathWasNotFile(path)) => assert_eq!(path, temp_dir.path()),
+            _ => {
+                return Err(eyre!(
+                    "Should have returned an ActionError::PathWasNotFile error"
+                ))
+            },
+        }
+
+        Ok(())
+    }
 }

src/action/common/configure_shell_profile.rs

@@ -73,8 +73,8 @@ impl ConfigureShellProfile {
                         minor: _,
                         patch: _,
                     }
-                    | OperatingSystem::Darwin => 0o100444,
-                    _ => 0o100644,
+                    | OperatingSystem::Darwin => 0o444,
+                    _ => 0o644,
                 };
                 create_or_insert_files.push(
                     CreateOrInsertIntoFile::plan(
@@ -114,7 +114,7 @@ impl ConfigureShellProfile {
 
             if let Some(conf_d) = profile_target.parent() {
                 create_directories.push(
-                    CreateDirectory::plan(conf_d.to_path_buf(), None, None, 0o0644, false).await?,
+                    CreateDirectory::plan(conf_d.to_path_buf(), None, None, 0o755, false).await?,
                 );
             }
 
@@ -123,7 +123,7 @@ impl ConfigureShellProfile {
                     profile_target,
                     None,
                     None,
-                    0o100644,
+                    0o644,
                     fish_buf.to_string(),
                     create_or_insert_into_file::Position::Beginning,
                 )

src/action/mod.rs

@@ -270,8 +270,12 @@ pub enum ActionError {
     PathUserMismatch(std::path::PathBuf, u32, u32),
     #[error("`{0}` exists with a different gid ({1}) than planned ({2}), consider updating it with `chgrp {2} {0}`")]
     PathGroupMismatch(std::path::PathBuf, u32, u32),
-    #[error("`{0}` exists with a different mode ({1:o}) than planned ({2:o}), consider updating it with `chmod {2:o} {0}`")]
+    #[error("`{0}` exists with a different mode ({existing_mode:o}) than planned ({planned_mode:o}), consider updating it with `chmod {planned_mode:o} {0}`",
+        existing_mode = .1 & 0o777,
+        planned_mode = .2 & 0o777)]
     PathModeMismatch(std::path::PathBuf, u32, u32),
+    #[error("`{0}` was not a file")]
+    PathWasNotFile(std::path::PathBuf),
     #[error("Getting metadata for {0}`")]
     GettingMetadata(std::path::PathBuf, #[source] std::io::Error),
     #[error("Creating directory `{0}`")]