Improve su_command parsing

proptest-regressions/conf.txt

@@ -0,0 +1,9 @@
+# Seeds for failure cases proptest has generated in the past. It is
+# automatically read and these particular cases re-run before any
+# novel cases are generated.
+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc 74b674162f67e0a42dccabd5bde0a39fa18f11dfaecb6971995502f93a07fa41 # shrinks to SuCommandWrapsIsReadFromConfigArgs = SuCommandWrapsIsReadFromConfigArgs { value: "" }
+cc 00d0bfa64cfe51948411edf0dd89386ccc18f9644800172acf487b5373839cf8 # shrinks to ConfigurationParsesArgs = ConfigurationParsesArgs { raw: Raw { su_command: Some(""), su_command_wraps: None, merge_strategy: None } }
+cc 7b467bb66998ed166bac12dfb9bc8980f02d852d65ae337e3fac7a5d93f03c9a # shrinks to ConfigurationParsesArgs = ConfigurationParsesArgs { raw: Raw { su_command: None, su_command_wraps: None, merge_strategy: Some("") } }

src/conf.rs

@@ -1,23 +1,29 @@
 use std::{
     collections::HashMap,
     fs::{self, DirEntry},
+    os::unix::fs::PermissionsExt as _,
     path::PathBuf,
 };
 
-use crate::{
+use proptest_derive::Arbitrary;
-    dev::{log::elog},
+
-    run::Command,
+use crate::{dev::log::elog, run::Command};
-};
+
+#[derive(Debug, Arbitrary)]
+struct Raw {
+    su_command: Option<String>,
+    su_command_wraps: Option<String>,
+    merge_strategy: Option<String>,
+}
 
 pub fn load() -> Result<Configuration, Error> {
     elog("Loading configuration");
 
-    let mut candidate = Configuration::default();
-
     let root = get_root();
     elog(&format!("Reading 'tori.conf' from: {root:?}"));
+
     let contents = fs::read_to_string(root.join("tori.conf"))?;
-    elog(&format!("Read configuration: {contents:?}"));
+    elog(&format!("Read configuration file: {contents:?}"));
 
     let map: HashMap<String, String> = contents
         .lines()
@@ -27,24 +33,49 @@ pub fn load() -> Result<Configuration, Error> {
 
     elog(&format!("Assembled configuration map: {map:#?}"));
 
-    if let Some(su_command) = map.get("su_command") {
+    let raw = Raw {
-        let wraps = map.get("su_command_wraps").is_some_and(|v| v == "true");
+        su_command: map.get("su_command").cloned(),
-        candidate.su_command = parse_su_command(su_command, wraps)?;
+        su_command_wraps: map.get("su_command_wraps").cloned(),
-    }
+        merge_strategy: map.get("merge_strategy").cloned(),
+    };
 
-    if let Some(merge_strategy) = map.get("merge_strategy") {
+    elog(&format!("Read raw configuration: {raw:?}"));
-        candidate.merge_strategy = match merge_strategy.as_str() {
+    Ok(parse(&raw))
-            "prefer configuration" => MergeStrategy::PreferConfig,
+}
-            "prefer system" => MergeStrategy::PreferSystem,
+
-            _ => MergeStrategy::default(),
+fn parse(raw: &Raw) -> Configuration {
+    let default = Configuration::default();
+    let mut candidate = default;
+
+    if let Some(su_command_value) = &raw.su_command {
+        let wraps_value = match &raw.su_command_wraps {
+            Some(wraps) => wraps == "true",
+            None => false,
+        };
+
+        match parse_su_command(su_command_value, wraps_value) {
+            Ok(s) => candidate.su_command = s,
+            Err(error) => println!("Failed parsing su_comand configuration value: {error}"),
         }
     }
 
-    elog(&format!("Assembled configuration candidate: {candidate:?}"));
+    if let Some(merge_strategy) = &raw.merge_strategy {
-    Ok(candidate)
+        candidate.merge_strategy = match merge_strategy.as_str() {
+            "prefer configuration" => MergeStrategy::PreferConfig,
+            "prefer system" => MergeStrategy::PreferSystem,
+            any => {
+                println!("Unrecognized merge strategy: {any}");
+                MergeStrategy::default()
+            }
+        }
+    }
+
+    elog(&format!("Parsed configuration candidate: {candidate:?}"));
+    candidate
 }
 
 fn parse_su_command(config_value: &str, wraps: bool) -> Result<SuCommand, Error> {
+    // TODO this is a horrible way to split because it will unquote everything
     let split: Vec<&str> = config_value.split(' ').filter(|s| !s.is_empty()).collect();
 
     let Some((base, args)) = split.split_first() else {
@@ -54,13 +85,29 @@ fn parse_su_command(config_value: &str, wraps: bool) -> Result<SuCommand, Error>
         ));
     };
 
-    let Ok(resolved_base) = resolve_command(base) else {
+    let resolved_base = if PathBuf::from(base).is_absolute() {
-        return Err(Error::new(
+        PathBuf::from(base)
-            "su_command does not resolve to a command in PATH",
+    } else {
-            ErrorKind::CommandNotInPath,
+        resolve_command(base)?
-        ));
     };
 
+    if resolved_base.is_file()
+        && let Ok(metadata) = resolved_base.metadata()
+    {
+        let mode = metadata.permissions().mode();
+        if mode & 0o111 == 0 {
+            return Err(Error::new(
+                "su_command path does not point to an executable file",
+                ErrorKind::WrongPermissions,
+            ));
+        }
+    } else {
+        return Err(Error::new(
+            "su_command path does not point to a file or its metadata is unreadable",
+            ErrorKind::MetadataUnreadable,
+        ));
+    }
+
     let Some(resolved_base_str) = resolved_base.to_str() else {
         return Err(Error::new(
             "su_command path contains invalid characters (expected UTF-8)",
@@ -227,11 +274,20 @@ impl From<std::io::Error> for Error {
     }
 }
 
+#[cfg(test)]
+impl From<Error> for proptest::test_runner::TestCaseError {
+    fn from(error: Error) -> proptest::test_runner::TestCaseError {
+        proptest::test_runner::TestCaseError::fail(format!("{}: {}", error.kind, error.message))
+    }
+}
+
 #[derive(Debug)]
 pub enum ErrorKind {
     CommandNotInPath,
     VarError,
     MalformedConfigLine,
+    MetadataUnreadable,
+    WrongPermissions,
     UTF8,
     IO,
 }
@@ -243,6 +299,8 @@ impl std::fmt::Display for ErrorKind {
             VarError => "Environment variable error",
             CommandNotInPath => "Command not in PATH",
             MalformedConfigLine => "Malformed configuration line",
+            MetadataUnreadable => "Metadata unreadable",
+            WrongPermissions => "Wrong permissions",
             UTF8 => "Invalid characters could not be decoded (expected UTF-8)",
             IO => "Input/Output error",
         };
@@ -250,13 +308,15 @@ impl std::fmt::Display for ErrorKind {
     }
 }
 
+// TODO review this test
 #[cfg(test)]
-#[expect(clippy::panic_in_result_fn, //clippy::unwrap_in_result
+#[expect(clippy::panic_in_result_fn)]
-)]
 mod serial_tests {
-    use std::{env, fs, os::unix::fs::PermissionsExt as _, io::{Write as _}};
+    use proptest::property_test;
+    use std::{env, fs, io::Write as _, os::unix::fs::PermissionsExt as _};
+
     use super::*;
-    use crate::{dev::test::{Directories, Error}};
+    use crate::dev::test::{Directories, Error};
 
     #[test]
     fn failed_config_read() -> Result<(), Error> {
@@ -299,8 +359,78 @@ mod serial_tests {
         let configuration = load()?;
         println!("configuration: {configuration:#?}");
 
-        assert!(matches!(configuration.merge_strategy, MergeStrategy::PreferSystem));
+        assert!(matches!(
+            configuration.merge_strategy,
+            MergeStrategy::PreferSystem
+        ));
 
         Ok(())
     }
+
+    #[property_test]
+    fn su_command_wrap_is_read_from_config(value: String) -> Result<(), Error> {
+        let dirs = Directories::setup("su_command_wraps_is_read_from_config")?;
+
+        let mut conf = fs::File::create_new(&dirs.conf)?;
+        conf.write_all(format!("su_command_wraps = {value}").as_bytes())?;
+        conf.sync_all()?;
+
+        let configuration = load()?;
+        let default = Configuration::default();
+
+        if value == "false" {
+            assert!(!configuration.su_command.wraps);
+        } else if configuration.su_command == default.su_command {
+            assert!(configuration.su_command.wraps);
+        } else {
+            assert!(value == "true");
+        }
+
+        Ok(())
+    }
+
+    #[property_test]
+    fn configuration_parses(raw: Raw) {
+        let parsed = parse(&raw);
+        let default = Configuration::default();
+
+        if let Some(su_command_value) = raw.su_command {
+            // these duplicated extractions are also in the tested
+            // code, this shpuld be in Command::from(&str)
+            let (base, args_opt) = match su_command_value.split_once(' ') {
+                Some((b, a)) => (b, Some(a)),
+                None => (su_command_value.as_str(), None),
+            };
+
+            let args = match args_opt {
+                Some(a) => vec![a],
+                None => vec![],
+            };
+
+            // this could also be a method of Command
+            if let Ok(resolved_su_command) = resolve_command(base) {
+                assert_eq!(parsed.su_command.command.base, resolved_su_command);
+            } else {
+                assert_eq!(parsed.su_command, default.su_command);
+            }
+        } else {
+            assert_eq!(parsed.su_command, default.su_command);
+        }
+
+        if let Some(merge_strategy) = &raw.merge_strategy {
+            use MergeStrategy::*;
+
+            // i guess this is fine (could be a match?) but it makes
+            // you think about how tests duplicate tautologies
+            if merge_strategy == "prefer system" {
+                assert!(matches!(parsed.merge_strategy, PreferSystem));
+            } else if merge_strategy == "prefer configuration" {
+                assert!(matches!(parsed.merge_strategy, PreferConfig));
+            } else {
+                assert!(matches!(parsed.merge_strategy, Interactive));
+            }
+        }
+
+        // TODO match raw.su_command_wraps {}
+    }
 }

src/dev/test.rs

@@ -1,6 +1,6 @@
 use std::{env, fs, io, path::PathBuf};
 
-use crate::{dev::log::elog, conf};
+use crate::{conf, dev::log::elog};
 
 #[derive(Debug)]
 pub struct Directories {
@@ -31,14 +31,16 @@ impl Directories {
             return Err(Error::with_io(
                 "Failed configuration root directory creation",
                 error,
-            ))
+            ));
         }
 
         if let Err(error) = env::set_current_dir(&tube) {
-            return Err(Error::with_io("Failed current directory change", error))
+            return Err(Error::with_io("Failed current directory change", error));
         }
 
-        unsafe { env::set_var("XDG_CONFIG_DIR", &xdg_conf); }
+        unsafe {
+            env::set_var("XDG_CONFIG_DIR", &xdg_conf);
+        }
 
         Ok(Directories {
             original,
@@ -76,7 +78,10 @@ impl Error {
     fn with_io(message: &str, inner: io::Error) -> Error {
         Error {
             message: String::from(message),
-            inner: Some(InnerErrors { io: Some(inner), conf: None }),
+            inner: Some(InnerErrors {
+                io: Some(inner),
+                conf: None,
+            }),
         }
     }
 }
@@ -103,13 +108,18 @@ impl From<String> for Error {
 }
 
 impl From<&str> for Error {
-    fn from(str: &str) -> Error { Error::from(String::from(str)) }
+    fn from(str: &str) -> Error {
+        Error::from(String::from(str))
+    }
 }
 
 impl From<io::Error> for Error {
     fn from(inner: io::Error) -> Error {
         let mut error = Error::from(inner.to_string());
-        error.inner = Some(InnerErrors { io: Some(inner), ..InnerErrors::default() });
+        error.inner = Some(InnerErrors {
+            io: Some(inner),
+            ..InnerErrors::default()
+        });
         error
     }
 }
@@ -118,7 +128,21 @@ impl From<conf::Error> for Error {
     fn from(conf_error: conf::Error) -> Error {
         Error {
             message: conf_error.message.clone(),
-            inner: Some(InnerErrors { conf: Some(conf_error), io: None }),
+            inner: Some(InnerErrors {
+                conf: Some(conf_error),
+                io: None,
+            }),
         }
     }
 }
+
+#[cfg(test)]
+impl From<Error> for proptest::test_runner::TestCaseError {
+    fn from(error: Error) -> proptest::test_runner::TestCaseError {
+        proptest::test_runner::TestCaseError::fail(if let Some(inner) = error.inner {
+            format!("{}: {:#?}", error.message, inner)
+        } else {
+            error.message
+        })
+    }
+}