From 22f3c2dd765675b7a5fcf8c9465ae2224bda7d33 Mon Sep 17 00:00:00 2001 From: Chip Senkbeil Date: Sun, 16 Jul 2023 13:38:26 -0400 Subject: [PATCH] Fix bugs in set permissions for CLI and distant-local --- CHANGELOG.md | 8 + distant-local/src/api.rs | 12 +- distant-protocol/src/common/permissions.rs | 378 ++++++++++++++++++++- src/cli/commands/client.rs | 93 +++-- 4 files changed, 451 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05e2665..c38d337 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Bug in `distant fs set-permissions` where partial permissions such as `go-w` + would result in clearing all permissions +- Bug in `distant-local` implementation of `SetPermissions` where read-only + status was being set/cleared prior to Unix permissions being applied, + resulting in applying an invalid change to the permissions + ## [0.20.0] All changes described in these alpha releases: diff --git a/distant-local/src/api.rs b/distant-local/src/api.rs index a0560e0..a11daba 100644 --- a/distant-local/src/api.rs +++ b/distant-local/src/api.rs @@ -451,9 +451,11 @@ impl DistantApi for Api { })? .permissions(); - // Apply the readonly flag for all platforms - if let Some(readonly) = permissions.is_readonly() { - std_permissions.set_readonly(readonly); + // Apply the readonly flag for all platforms but junix + if !cfg!(unix) { + if let Some(readonly) = permissions.is_readonly() { + std_permissions.set_readonly(readonly); + } } // On Unix platforms, we can apply a bitset change @@ -462,7 +464,9 @@ impl DistantApi for Api { use std::os::unix::prelude::*; let mut current = Permissions::from(std_permissions.clone()); current.apply_from(permissions); - std_permissions.set_mode(current.to_unix_mode()); + + let mode = current.to_unix_mode(); + std_permissions.set_mode(mode); } Ok(std_permissions) diff --git a/distant-protocol/src/common/permissions.rs b/distant-protocol/src/common/permissions.rs index ec564ee..1db4dbc 100644 --- a/distant-protocol/src/common/permissions.rs +++ b/distant-protocol/src/common/permissions.rs @@ -296,7 +296,7 @@ impl Permissions { /// Converts a Unix `mode` into the permission set. pub fn from_unix_mode(mode: u32) -> Self { - let flags = UnixFilePermissionFlags::from_bits_truncate(mode); + let flags = UnixFilePermissionFlags::from_bits_truncate(mode & 0o777); Self { owner_read: Some(flags.contains(UnixFilePermissionFlags::OWNER_READ)), owner_write: Some(flags.contains(UnixFilePermissionFlags::OWNER_WRITE)), @@ -426,15 +426,15 @@ impl From for std::fs::Permissions { bitflags! { struct UnixFilePermissionFlags: u32 { - const OWNER_READ = 0o400; - const OWNER_WRITE = 0o200; - const OWNER_EXEC = 0o100; - const GROUP_READ = 0o40; - const GROUP_WRITE = 0o20; - const GROUP_EXEC = 0o10; - const OTHER_READ = 0o4; - const OTHER_WRITE = 0o2; - const OTHER_EXEC = 0o1; + const OWNER_READ = 0o400; + const OWNER_WRITE = 0o200; + const OWNER_EXEC = 0o100; + const GROUP_READ = 0o040; + const GROUP_WRITE = 0o020; + const GROUP_EXEC = 0o010; + const OTHER_READ = 0o004; + const OTHER_WRITE = 0o002; + const OTHER_EXEC = 0o001; } } @@ -442,6 +442,364 @@ bitflags! { mod tests { use super::*; + #[test] + fn should_properly_parse_unix_mode_into_permissions() { + let permissions = Permissions::from_unix_mode(0o400); + assert_eq!( + permissions, + Permissions { + owner_read: Some(true), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o200); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(true), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o100); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(true), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o040); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(true), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o020); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(true), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o010); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(true), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o004); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(true), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o002); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(true), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o001); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(true), + } + ); + + let permissions = Permissions::from_unix_mode(0o000); + assert_eq!( + permissions, + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + ); + + let permissions = Permissions::from_unix_mode(0o777); + assert_eq!( + permissions, + Permissions { + owner_read: Some(true), + owner_write: Some(true), + owner_exec: Some(true), + group_read: Some(true), + group_write: Some(true), + group_exec: Some(true), + other_read: Some(true), + other_write: Some(true), + other_exec: Some(true), + } + ); + } + + #[test] + fn should_properly_convert_into_unix_mode() { + assert_eq!( + Permissions { + owner_read: Some(true), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o400 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(true), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o200 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(true), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o100 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(true), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o040 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(true), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o020 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(true), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o010 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(true), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o004 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(true), + other_exec: Some(false), + } + .to_unix_mode(), + 0o002 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(true), + } + .to_unix_mode(), + 0o001 + ); + + assert_eq!( + Permissions { + owner_read: Some(false), + owner_write: Some(false), + owner_exec: Some(false), + group_read: Some(false), + group_write: Some(false), + group_exec: Some(false), + other_read: Some(false), + other_write: Some(false), + other_exec: Some(false), + } + .to_unix_mode(), + 0o000 + ); + + assert_eq!( + Permissions { + owner_read: Some(true), + owner_write: Some(true), + owner_exec: Some(true), + group_read: Some(true), + group_write: Some(true), + group_exec: Some(true), + other_read: Some(true), + other_write: Some(true), + other_exec: Some(true), + } + .to_unix_mode(), + 0o777 + ); + } + #[test] fn should_be_able_to_serialize_minimal_permissions_to_json() { let permissions = Permissions { diff --git a/src/cli/commands/client.rs b/src/cli/commands/client.rs index a043d81..1274c3f 100644 --- a/src/cli/commands/client.rs +++ b/src/cli/commands/client.rs @@ -1187,6 +1187,25 @@ async fn async_run(cmd: ClientSubcommand) -> CliResult { mode, path, }) => { + debug!("Connecting to manager"); + let mut client = Client::new(network) + .using_prompt_auth_handler() + .connect() + .await + .context("Failed to connect to manager")?; + + let mut cache = read_cache(&cache).await; + let connection_id = + use_or_lookup_connection_id(&mut cache, connection, &mut client).await?; + + debug!("Opening channel to connection {}", connection_id); + let mut channel: DistantChannel = client + .open_raw_channel(connection_id) + .await + .with_context(|| format!("Failed to open channel to connection {connection_id}"))? + .into_client() + .into_channel(); + debug!("Parsing {mode:?} into a proper set of permissions"); let permissions = { if mode.trim().eq_ignore_ascii_case("readonly") { @@ -1196,37 +1215,61 @@ async fn async_run(cmd: ClientSubcommand) -> CliResult { } else { // Attempt to parse an octal number (chmod absolute), falling back to // parsing the mode string similar to chmod's symbolic mode - let mode = match u32::from_str_radix(&mode, 8) { - Ok(absolute) => file_mode::Mode::from(absolute), + match u32::from_str_radix(&mode, 8) { + Ok(absolute) => { + Permissions::from_unix_mode(file_mode::Mode::from(absolute).mode()) + } Err(_) => { - let mut new_mode = file_mode::Mode::empty(); - new_mode + // The way parsing works, we need to parse and apply to two different + // situations + // + // 1. A mode that is all 1s so we can see if the mask would remove + // permission to some of the bits + // 2. A mode that is all 0s so we can see if the mask would add + // permission to some of the bits + let mut removals = file_mode::Mode::from(0o777); + removals + .set_str(&mode) + .context("Failed to parse mode string")?; + let removals_mask = !removals.mode(); + + let mut additions = file_mode::Mode::empty(); + additions .set_str(&mode) .context("Failed to parse mode string")?; - new_mode + let additions_mask = additions.mode(); + + macro_rules! get_mode { + ($mask:expr) => {{ + let is_false = removals_mask & $mask > 0; + let is_true = additions_mask & $mask > 0; + match (is_true, is_false) { + (true, false) => Some(true), + (false, true) => Some(false), + (false, false) => None, + (true, true) => { + unreachable!("Mask cannot be adding and removing") + } + } + }}; + } + + Permissions { + owner_read: get_mode!(0o400), + owner_write: get_mode!(0o200), + owner_exec: get_mode!(0o100), + group_read: get_mode!(0o040), + group_write: get_mode!(0o020), + group_exec: get_mode!(0o010), + other_read: get_mode!(0o004), + other_write: get_mode!(0o002), + other_exec: get_mode!(0o001), + } } - }; - Permissions::from_unix_mode(mode.mode()) + } } }; - debug!("Connecting to manager"); - let mut client = Client::new(network) - .using_prompt_auth_handler() - .connect() - .await - .context("Failed to connect to manager")?; - - let mut cache = read_cache(&cache).await; - let connection_id = - use_or_lookup_connection_id(&mut cache, connection, &mut client).await?; - - debug!("Opening channel to connection {}", connection_id); - let channel = client - .open_raw_channel(connection_id) - .await - .with_context(|| format!("Failed to open channel to connection {connection_id}"))?; - let options = SetPermissionsOptions { recursive, follow_symlinks, @@ -1234,8 +1277,6 @@ async fn async_run(cmd: ClientSubcommand) -> CliResult { }; debug!("Setting permissions for {path:?} as (permissions = {permissions:?}, options = {options:?})"); channel - .into_client() - .into_channel() .set_permissions(path.as_path(), permissions, options) .await .with_context(|| {