conf.rs: check that all conf flags are recognized in validation

This commit adds logic in configuration file validation that checks that each account "extra" field is empty after getting it back from the backend validation. This is to ensure the user doesn't set options that are invalidly stated in the documentation or by accident. Closes #135 Configuration error (xxx): the following flags are set but are not recognized: ["index_style"] https://git.meli.delivery/meli/meli/issues/135
2022-03-20 16:31:55 +02:00 · 2022-03-20 16:31:55 +02:00 · f5dc25ae0d
parent d0de04854e
commit f5dc25ae0d
13 changed files with 179 additions and 41 deletions
--- a/Cargo.lock
+++ b/Cargo.lock
@ -14,6 +14,15 @@ version = "0.4.7"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "739f4a8db6605981345c5654f3a85b056ce52f37a39d34da03f25bf2151ea16e"

+[[package]]
+name = "aho-corasick"
+version = "0.7.15"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7404febffaa47dac81aa44dba71523c9d069b1bdc50a77db41195149e17f68e5"
+dependencies = [
+ "memchr",
+]
+
 [[package]]
 name = "arrayref"
 version = "0.3.6"
@ -1117,6 +1126,7 @@ dependencies = [
 "pcre2",
 "proc-macro2",
 "quote 1.0.8",
+ "regex",
 "serde",
 "serde_derive",
 "serde_json",
@ -1673,6 +1683,23 @@ dependencies = [
 "rust-argon2",
 ]

+[[package]]
+name = "regex"
+version = "1.4.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2a26af418b574bd56588335b3a3659a65725d4e636eb1016c2f9e3b38c7cc759"
+dependencies = [
+ "aho-corasick",
+ "memchr",
+ "regex-syntax",
+]
+
+[[package]]
+name = "regex-syntax"
+version = "0.6.25"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f497285884f3fcff424ffc933e56d7cbca511def0c9831a7f9b5f6153e3cc89b"
+
 [[package]]
 name = "remove_dir_all"
 version = "0.5.3"
--- a/Cargo.toml
+++ b/Cargo.toml
@ -54,7 +54,7 @@ futures = "0.3.5"
 async-task = "3.0.0"
 num_cpus = "1.12.0"
 flate2 = { version = "1.0.16", optional = true }
- 
+
 [target.'cfg(target_os="linux")'.dependencies]
 notify-rust = { version = "^4", optional = true }

@ -64,6 +64,9 @@ quote = "^1.0"
 proc-macro2 = "1.0.18"
 flate2 = { version = "1.0.16", optional = true }

+[dev-dependencies]
+regex = "1"
+
 [profile.release]
 lto = "fat"
 codegen-units = 1
--- a/docs/meli.conf.5
+++ b/docs/meli.conf.5
@ -77,7 +77,7 @@ example configuration
 [accounts.account-name]
 root_mailbox = "/path/to/root/folder"
 format = "Maildir"
-index_style = "Compact"
+listing.index_style = "Compact"
 identity="email@example.com"
 subscribed_mailboxes = ["folder", "folder/Sent"] # or [ "*", ] for all mailboxes
 display_name = "Name"
@ -93,7 +93,7 @@ display_name = "Name"
 [accounts.mbox]
 root_mailbox = "/var/mail/username"
 format = "mbox"
-index_style = "Compact"
+listing.index_style = "Compact"
 identity="username@hostname.local"
 composing.send_mail = { hostname = "localhost", port = 25, auth = { type = "none" }, security = { type = "none" } }

@ -138,8 +138,6 @@ The glob wildcard
 can be used to match every mailbox name and path.
 .It Ic identity Ar String
 Your e-mail address that is inserted in the From: headers of outgoing mail.
-.It Ic index_style Ar String
-Sets the way mailboxes are displayed.
 .El
 .TS
 allbox tab(:);
--- a/docs/samples/sample-config.toml
+++ b/docs/samples/sample-config.toml
@ -11,7 +11,7 @@
 #[accounts.account-name]
 #root_mailbox = "/path/to/root/mailbox"
 #format = "Maildir"
-#index_style = "Conversations" # or [plain, threaded, compact]
+#listing.index_style = "Conversations" # or [plain, threaded, compact]
 #identity="email@example.com"
 #display_name = "Name"
 #subscribed_mailboxes = ["INBOX", "INBOX/Sent", "INBOX/Drafts", "INBOX/Junk"]
@ -26,7 +26,7 @@
 #[accounts.mbox]
 #root_mailbox = "/var/mail/username"
 #format = "mbox"
-#index_style = "Compact"
+#listing.index_style = "Compact"
 #identity="username@hostname.local"
 #
 ## Setting up an IMAP account
@ -36,10 +36,10 @@
 #server_hostname="mail.example.com"
 #server_password="pha2hiLohs2eeeish2phaii1We3ood4chakaiv0hien2ahie3m"
 #server_username="username@example.com"
-#server_port="993" # imaps
+##server_port="993" # imaps
 #server_port="143" # STARTTLS
 #use_starttls=true #optional
-#index_style = "Conversations"
+#listing.index_style = "Conversations"
 #identity = "username@example.com"
 #display_name = "Name Name"
 ### match every mailbox:
@ -51,7 +51,7 @@
 #[accounts.notmuch]
 #root_mailbox = "/path/to/folder" # where .notmuch/ directory is located
 #format = "notmuch"
-#index_style = "conversations"
+#listing.index_style = "conversations"
 #identity="username@example.com"
 #display_name = "Name Name"
 #  # notmuch mailboxes are virtual, they are defined by their alias and the notmuch query that corresponds to their content.
@ -68,7 +68,7 @@
 #server_password="password"
 #server_username="username@gmail.com"
 #server_port="993"
-#index_style = "Conversations"
+#listing.index_style = "Conversations"
 #identity = "username@gmail.com"
 #display_name = "Name Name"
 ### match every mailbox:
--- a/melib/src/backends.rs
+++ b/melib/src/backends.rs
@ -97,7 +97,7 @@ pub struct Backends {

 pub struct Backend {
    pub create_fn: Box<dyn Fn() -> BackendCreator>,
-    pub validate_conf_fn: Box<dyn Fn(&AccountSettings) -> Result<()>>,
+    pub validate_conf_fn: Box<dyn Fn(&mut AccountSettings) -> Result<()>>,
 }

 impl Default for Backends {
@ -200,7 +200,7 @@ impl Backends {
        self.map.insert(key, backend);
    }

-    pub fn validate_config(&self, key: &str, s: &AccountSettings) -> Result<()> {
+    pub fn validate_config(&self, key: &str, s: &mut AccountSettings) -> Result<()> {
        (self
            .map
            .get(key)
--- a/melib/src/backends/imap.rs
+++ b/melib/src/backends/imap.rs
@ -1500,12 +1500,12 @@ impl ImapType {
        Ok(mailboxes)
    }

-    pub fn validate_config(s: &AccountSettings) -> Result<()> {
+    pub fn validate_config(s: &mut AccountSettings) -> Result<()> {
        let mut keys: HashSet<&'static str> = Default::default();
        macro_rules! get_conf_val {
            ($s:ident[$var:literal]) => {{
                keys.insert($var);
-                $s.extra.get($var).ok_or_else(|| {
+                $s.extra.remove($var).ok_or_else(|| {
                    MeliError::new(format!(
                        "Configuration error ({}): IMAP connection requires the field `{}` set",
                        $s.name.as_str(),
@ -1516,9 +1516,9 @@ impl ImapType {
            ($s:ident[$var:literal], $default:expr) => {{
                keys.insert($var);
                $s.extra
-                    .get($var)
+                    .remove($var)
                    .map(|v| {
-                        <_>::from_str(v).map_err(|e| {
+                        <_>::from_str(&v).map_err(|e| {
                            MeliError::new(format!(
                                "Configuration error ({}): Invalid value for field `{}`: {}\n{}",
                                $s.name.as_str(),
--- a/melib/src/backends/jmap.rs
+++ b/melib/src/backends/jmap.rs
@ -874,7 +874,34 @@ impl JmapType {
        }))
    }

-    pub fn validate_config(s: &AccountSettings) -> Result<()> {
+    pub fn validate_config(s: &mut AccountSettings) -> Result<()> {
+        macro_rules! get_conf_val {
+            ($s:ident[$var:literal]) => {
+                $s.extra.remove($var).ok_or_else(|| {
+                    MeliError::new(format!(
+                        "Configuration error ({}): JMAP connection requires the field `{}` set",
+                        $s.name.as_str(),
+                        $var
+                    ))
+                })
+            };
+            ($s:ident[$var:literal], $default:expr) => {
+                $s.extra
+                    .remove($var)
+                    .map(|v| {
+                        <_>::from_str(&v).map_err(|e| {
+                            MeliError::new(format!(
+                                "Configuration error ({}): Invalid value for field `{}`: {}\n{}",
+                                $s.name.as_str(),
+                                $var,
+                                v,
+                                e
+                            ))
+                        })
+                    })
+                    .unwrap_or_else(|| Ok($default))
+            };
+        }
        get_conf_val!(s["server_hostname"])?;
        get_conf_val!(s["server_username"])?;
        get_conf_val!(s["server_password"])?;
--- a/melib/src/backends/maildir/backend.rs
+++ b/melib/src/backends/maildir/backend.rs
@ -1316,7 +1316,7 @@ impl MaildirType {
        Ok(())
    }

-    pub fn validate_config(s: &AccountSettings) -> Result<()> {
+    pub fn validate_config(s: &mut AccountSettings) -> Result<()> {
        let root_path = PathBuf::from(s.root_mailbox()).expand();
        if !root_path.exists() {
            return Err(MeliError::new(format!(
--- a/melib/src/backends/mbox.rs
+++ b/melib/src/backends/mbox.rs
@ -1339,7 +1339,34 @@ impl MboxType {
        Ok(Box::new(ret))
    }

-    pub fn validate_config(s: &AccountSettings) -> Result<()> {
+    pub fn validate_config(s: &mut AccountSettings) -> Result<()> {
+        macro_rules! get_conf_val {
+            ($s:ident[$var:literal]) => {
+                $s.extra.remove($var).ok_or_else(|| {
+                    MeliError::new(format!(
+                        "Configuration error ({}): mbox backend requires the field `{}` set",
+                        $s.name.as_str(),
+                        $var
+                    ))
+                })
+            };
+            ($s:ident[$var:literal], $default:expr) => {
+                $s.extra
+                    .remove($var)
+                    .map(|v| {
+                        <_>::from_str(&v).map_err(|e| {
+                            MeliError::new(format!(
+                                "Configuration error ({}): Invalid value for field `{}`: {}\n{}",
+                                $s.name.as_str(),
+                                $var,
+                                v,
+                                e
+                            ))
+                        })
+                    })
+                    .unwrap_or_else(|| Ok($default))
+            };
+        }
        let path = Path::new(s.root_mailbox.as_str()).expand();
        if !path.exists() {
            return Err(MeliError::new(format!(
--- a/melib/src/backends/nntp.rs
+++ b/melib/src/backends/nntp.rs
@ -654,12 +654,12 @@ impl NntpType {
        Ok(())
    }

-    pub fn validate_config(s: &AccountSettings) -> Result<()> {
+    pub fn validate_config(s: &mut AccountSettings) -> Result<()> {
        let mut keys: HashSet<&'static str> = Default::default();
        macro_rules! get_conf_val {
    ($s:ident[$var:literal]) => {{
        keys.insert($var);
-        $s.extra.get($var).ok_or_else(|| {
+        $s.extra.remove($var).ok_or_else(|| {
            MeliError::new(format!(
                "Configuration error ({}): NNTP connection requires the field `{}` set",
                $s.name.as_str(),
@ -670,9 +670,9 @@ impl NntpType {
    ($s:ident[$var:literal], $default:expr) => {{
        keys.insert($var);
        $s.extra
-            .get($var)
+            .remove($var)
            .map(|v| {
-                <_>::from_str(v).map_err(|e| {
+                <_>::from_str(&v).map_err(|e| {
                    MeliError::new(format!(
                        "Configuration error ({}) NNTP: Invalid value for field `{}`: {}\n{}",
                        $s.name.as_str(),
--- a/melib/src/backends/notmuch.rs
+++ b/melib/src/backends/notmuch.rs
@ -372,7 +372,7 @@ impl NotmuchDb {
        }))
    }

-    pub fn validate_config(s: &AccountSettings) -> Result<()> {
+    pub fn validate_config(s: &mut AccountSettings) -> Result<()> {
        let path = Path::new(s.root_mailbox.as_str()).expand();
        if !path.exists() {
            return Err(MeliError::new(format!(
@ -381,8 +381,8 @@ impl NotmuchDb {
                s.name()
            )));
        }
-        for (k, f) in s.mailboxes.iter() {
-            if f.extra.get("query").is_none() {
+        for (k, f) in s.mailboxes.iter_mut() {
+            if f.extra.remove("query").is_none() {
                return Err(MeliError::new(format!(
                    "notmuch mailbox configuration entry \"{}\" should have a \"query\" value set.",
                    k
--- a/src/bin.rs
+++ b/src/bin.rs
@ -223,7 +223,7 @@ fn run_app(opt: Opt) -> Result<()> {
            } else {
                crate::conf::get_config_file()?
            };
-            conf::FileSettings::validate(config_path, true)?; // TODO: test for tty/interaction
+            conf::FileSettings::validate(config_path, true, false)?; // TODO: test for tty/interaction
            return Ok(());
        }
        Some(SubCommand::CreateConfig { path }) => {
--- a/src/conf.rs
+++ b/src/conf.rs
@ -350,10 +350,10 @@ impl FileSettings {
            return Err(MeliError::new("No configuration file found."));
        }

-        FileSettings::validate(config_path, true)
+        FileSettings::validate(config_path, true, false)
    }

-    pub fn validate(path: PathBuf, interactive: bool) -> Result<Self> {
+    pub fn validate(path: PathBuf, interactive: bool, clear_extras: bool) -> Result<Self> {
        let s = pp::pp(&path)?;
        let map: toml::map::Map<String, toml::value::Value> = toml::from_str(&s).map_err(|e| {
            MeliError::new(format!(
@ -391,7 +391,7 @@ This is required so that you don't accidentally start meli and find out later th
                                err
                            ))
                        })?;
-                    return FileSettings::validate(path, interactive);
+                    return FileSettings::validate(path, interactive, clear_extras);
                }
            }
            return Err(MeliError::new(format!(
@ -439,7 +439,7 @@ This is required so that you don't accidentally start meli and find out later th
        }

        s.terminal.themes.validate()?;
-        for (name, acc) in &s.accounts {
+        for (name, acc) in s.accounts.iter_mut() {
            let FileAccount {
                root_mailbox,
                format,
@ -456,7 +456,7 @@ This is required so that you don't accidentally start meli and find out later th
            } = acc.clone();

            let lowercase_format = format.to_lowercase();
-            let s = AccountSettings {
+            let mut s = AccountSettings {
                name: name.to_string(),
                root_mailbox,
                format: format.clone(),
@ -471,7 +471,16 @@ This is required so that you don't accidentally start meli and find out later th
                    .collect(),
                extra: extra.into_iter().collect(),
            };
-            backends.validate_config(&lowercase_format, &s)?;
+            backends.validate_config(&lowercase_format, &mut s)?;
+            if !s.extra.is_empty() {
+                return Err(MeliError::new(format!(
+                    "Unrecognised configuration values: {:?}",
+                    s.extra
+                )));
+            }
+            if clear_extras {
+                acc.extra.clear();
+            }
        }

        Ok(s)
@ -1136,7 +1145,7 @@ fn test_config_parse() {
 [accounts.account-name]
 root_mailbox = "/path/to/root/mailbox"
 format = "Maildir"
-index_style = "Conversations" # or [plain, threaded, compact]
+listing.index_style = "Conversations" # or [plain, threaded, compact]
 identity="email@example.com"
 display_name = "Name"
 subscribed_mailboxes = ["INBOX", "INBOX/Sent", "INBOX/Drafts", "INBOX/Junk"]
@ -1151,11 +1160,25 @@ subscribed_mailboxes = ["INBOX", "INBOX/Sent", "INBOX/Drafts", "INBOX/Junk"]
 [accounts.mbox]
 root_mailbox = "/var/mail/username"
 format = "mbox"
-index_style = "Compact"
+listing.index_style = "Compact"
 identity="username@hostname.local"
 "#;
+
+    const EXTRA_CONFIG: &str = r#"
+[accounts.mbox]
+root_mailbox = "/"
+format = "mbox"
+index_style = "Compact"
+identity="username@hostname.local"
+
+[composing]
+send_mail = '/bin/false'
+"#;
+
+    const EXAMPLE_CONFIG: &str = include_str!("../docs/samples/sample-config.toml");
+
    impl ConfigFile {
-        fn new() -> std::result::Result<Self, std::io::Error> {
+        fn new(content: &str) -> std::result::Result<Self, std::io::Error> {
            let mut f = fs::File::open("/dev/urandom")?;
            let mut buf = [0u8; 16];
            f.read_exact(&mut buf)?;
@ -1169,7 +1192,7 @@ identity="username@hostname.local"
                .create_new(true)
                .append(true)
                .open(&path)?;
-            file.write_all(TEST_CONFIG.as_bytes())?;
+            file.write_all(content.as_bytes())?;
            Ok(ConfigFile { path, file })
        }
    }
@ -1180,13 +1203,46 @@ identity="username@hostname.local"
        }
    }

-    let mut new_file = ConfigFile::new().unwrap();
-    let err = FileSettings::validate(new_file.path.clone(), false).unwrap_err();
+    let mut new_file = ConfigFile::new(TEST_CONFIG).unwrap();
+    let err = FileSettings::validate(new_file.path.clone(), false, true).unwrap_err();
    assert!(err.details.as_ref().starts_with("You must set a global `composing` option. If you override `composing` in each account, you can use a dummy global like follows"));
    new_file
        .file
        .write_all("[composing]\nsend_mail = '/bin/false'\n".as_bytes())
        .unwrap();
-    let err = FileSettings::validate(new_file.path.clone(), false).unwrap_err();
+    let err = FileSettings::validate(new_file.path.clone(), false, true).unwrap_err();
    assert_eq!(err.details.as_ref(), "Configuration error (account-name): root_path `/path/to/root/mailbox` is not a valid directory.");
+
+    /* Test unrecognised configuration entries error */
+
+    let new_file = ConfigFile::new(EXTRA_CONFIG).unwrap();
+    let err = FileSettings::validate(new_file.path.clone(), false, true).unwrap_err();
+    assert_eq!(
+        err.details.as_ref(),
+        "Unrecognised configuration values: {\"index_style\": \"Compact\"}"
+    );
+
+    /* Test sample config */
+
+    let example_config = EXAMPLE_CONFIG.replace("\n#", "\n");
+    let re = regex::Regex::new(r#"root_mailbox\s*=\s*"[^"]*""#).unwrap();
+    let example_config = re.replace_all(
+        &example_config,
+        &format!(
+            r#"root_mailbox = "{}""#,
+            std::env::temp_dir().to_str().unwrap()
+        ),
+    );
+
+    let new_file = ConfigFile::new(&example_config).unwrap();
+    let config = FileSettings::validate(new_file.path.clone(), false, true)
+        .expect("Could not parse example config!");
+    for (accname, acc) in config.accounts.iter() {
+        if !acc.extra.is_empty() {
+            panic!(
+                "In example config, account `{}` has unrecognised configuration entries: {:?}",
+                accname, acc.extra
+            );
+        }
+    }
 }