From b04b1eec7071830a8fc7190dc9034b8f170927f4 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Thu, 23 Feb 2023 11:32:09 -0800 Subject: [PATCH] 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 | 100 +++++++++++++++++ src/action/base/create_or_insert_into_file.rs | 101 ++++++++++++++++++ src/action/common/configure_shell_profile.rs | 8 +- src/action/mod.rs | 6 +- 4 files changed, 210 insertions(+), 5 deletions(-) diff --git a/src/action/base/create_file.rs b/src/action/base/create_file.rs index 669e30a..a61ba0b 100644 --- a/src/action/base/create_file.rs +++ b/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(()) + } } diff --git a/src/action/base/create_or_insert_into_file.rs b/src/action/base/create_or_insert_into_file.rs index 0471628..3c8d54b 100644 --- a/src/action/base/create_or_insert_into_file.rs +++ b/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(()) + } } diff --git a/src/action/common/configure_shell_profile.rs b/src/action/common/configure_shell_profile.rs index 3112f12..0f2361e 100644 --- a/src/action/common/configure_shell_profile.rs +++ b/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, ) diff --git a/src/action/mod.rs b/src/action/mod.rs index 5283de4..6939704 100644 --- a/src/action/mod.rs +++ b/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}`")]