From 20b054b50ce147d4fcc2de0a86df406ff169e218 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Fri, 10 Feb 2023 10:56:22 -0800 Subject: [PATCH] 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 | 52 ++++-- src/action/base/create_file.rs | 157 ++++++++++++++++-- src/action/base/create_or_insert_into_file.rs | 125 +++++++++++++- src/action/common/configure_shell_profile.rs | 2 +- src/action/mod.rs | 19 ++- 5 files changed, 317 insertions(+), 38 deletions(-) diff --git a/src/action/base/create_directory.rs b/src/action/base/create_directory.rs index 8b24931..acc55f8 100644 --- a/src/action/base/create_directory.rs +++ b/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>, force_prune_on_revert: bool, ) -> Result, 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))?; - if metadata.is_dir() { - tracing::debug!( - "Creating directory `{}` already complete, skipping", - path.display(), - ); - // TODO: Validate owner/group... - ActionState::Skipped - } else { + .map_err(|e| ActionError::GettingMetadata(path.clone(), e))?; + if !metadata.is_dir() { 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 }; diff --git a/src/action/base/create_file.rs b/src/action/base/create_file.rs index b5c950f..669e30a 100644 --- a/src/action/base/create_file.rs +++ b/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}, - io::AsyncWriteExt, + fs::{remove_file, File, OpenOptions}, + io::{AsyncReadExt, AsyncWriteExt}, }; use crate::action::{Action, ActionDescription, ActionError, StatefulAction}; @@ -36,20 +39,87 @@ impl CreateFile { force: bool, ) -> Result, ActionError> { let path = path.as_ref().to_path_buf(); - - if path.exists() && !force { - return Err(ActionError::Exists(path.to_path_buf())); - } - - Ok(Self { + let mode = mode.into(); + let user = user.into(); + let group = group.into(); + let this = Self { path, - user: user.into(), - group: group.into(), - mode: mode.into(), + user, + group, + 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(()) + } } diff --git a/src/action/base/create_or_insert_into_file.rs b/src/action/base/create_or_insert_into_file.rs index 4641453..0471628 100644 --- a/src/action/base/create_or_insert_into_file.rs +++ b/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, ActionError> { let path = path.as_ref().to_path_buf(); - - Ok(Self { + let mode = mode.into(); + let user = user.into(); + let group = group.into(); + let this = Self { path, - user: user.into(), - group: group.into(), - mode: mode.into(), + user, + group, + 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(()) + } } diff --git a/src/action/common/configure_shell_profile.rs b/src/action/common/configure_shell_profile.rs index d26bf88..81a88f0 100644 --- a/src/action/common/configure_shell_profile.rs +++ b/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, ) diff --git a/src/action/mod.rs b/src/action/mod.rs index 0903349..3b9fce1 100644 --- a/src/action/mod.rs +++ b/src/action/mod.rs @@ -262,8 +262,16 @@ pub enum ActionError { }).collect::>().join(" & "))] Children(Vec>), /// 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> { - None + fn expected<'a>(&'a self) -> Option> { + match self { + Self::PathUserMismatch(_, _, _) + | Self::PathGroupMismatch(_, _, _) + | Self::PathModeMismatch(_, _, _) => Some(Box::new(self)), + _ => None, + } } }