From 77381c1a2aa83cf9469f153f88d2a18afe726833 Mon Sep 17 00:00:00 2001
From: James Hodgkinson <james@terminaloutcomes.com>
Date: Mon, 26 Apr 2021 11:52:13 +1000
Subject: [PATCH] User feedback improvements, also handling a permissions issue
 (#424)

---
 .clippy.toml                   | 13 +++++++++++++
 kanidm_client/src/lib.rs       | 25 +++++++++++++++++++++----
 kanidm_tools/src/cli/common.rs |  2 +-
 kanidm_tools/src/cli/lib.rs    | 23 ++++++++++++++---------
 kanidm_tools/src/cli/login.rs  | 26 ++++++++++++++++++++------
 kanidmd/src/lib/core/ldaps.rs  |  6 ++++++
 kanidmd/src/lib/entry.rs       | 24 +++++++++++-------------
 kanidmd/src/lib/filter.rs      |  8 ++++----
 kanidmd/src/lib/ldap.rs        |  2 --
 9 files changed, 90 insertions(+), 39 deletions(-)
 create mode 100644 .clippy.toml

diff --git a/.clippy.toml b/.clippy.toml
new file mode 100644
index 000000000..2f6941e9f
--- /dev/null
+++ b/.clippy.toml
@@ -0,0 +1,13 @@
+########################################################################
+# complexity clippy settings
+#
+# because the clippy devs also acknowledge that complexity is hard to define
+# https://github.com/rust-lang/rust-clippy/issues/5418#issuecomment-610054361
+#
+########################################################################
+
+# default is 7, 8's ok. https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
+too-many-arguments-threshold = 8
+
+# default's 250
+type-complexity-threshold = 300
diff --git a/kanidm_client/src/lib.rs b/kanidm_client/src/lib.rs
index 050a0f670..94f735042 100644
--- a/kanidm_client/src/lib.rs
+++ b/kanidm_client/src/lib.rs
@@ -15,6 +15,7 @@ use serde_derive::Deserialize;
 use serde_json::error::Error as SerdeJsonError;
 use std::collections::BTreeSet as Set;
 use std::fs::{metadata, File, Metadata};
+use std::io::ErrorKind;
 use std::io::Read;
 use std::os::unix::fs::MetadataExt;
 use std::path::Path;
@@ -151,10 +152,26 @@ impl KanidmClientBuilder {
         let mut f = match File::open(&config_path) {
             Ok(f) => f,
             Err(e) => {
-                debug!(
-                    "Unable to open config file {:#?} [{:?}], skipping ...",
-                    &config_path, e
-                );
+                match e.kind() {
+                    ErrorKind::NotFound => {
+                        debug!(
+                            "Configuration file {:#?} not found, skipping ...",
+                            &config_path
+                        );
+                    }
+                    ErrorKind::PermissionDenied => {
+                        warn!(
+                            "Permission denied loading configuration file {:#?}, skipping ...",
+                            &config_path
+                        );
+                    }
+                    _ => {
+                        debug!(
+                            "Unable to open config file {:#?} [{:?}], skipping ...",
+                            &config_path, e
+                        );
+                    }
+                };
                 return Ok(self);
             }
         };
diff --git a/kanidm_tools/src/cli/common.rs b/kanidm_tools/src/cli/common.rs
index 96904c020..cac6d5ac0 100644
--- a/kanidm_tools/src/cli/common.rs
+++ b/kanidm_tools/src/cli/common.rs
@@ -81,7 +81,7 @@ impl CommonOpt {
                     #[allow(clippy::expect_used)]
                     let (f_uname, f_token) = tokens.iter().next().expect("Memory Corruption");
                     // else pick the first token
-                    info!("Authenticated as {}", f_uname);
+                    info!("Using cached token for name {}", f_uname);
                     f_token.clone()
                 } else {
                     // Unable to select
diff --git a/kanidm_tools/src/cli/lib.rs b/kanidm_tools/src/cli/lib.rs
index 98e10e42c..df019e178 100644
--- a/kanidm_tools/src/cli/lib.rs
+++ b/kanidm_tools/src/cli/lib.rs
@@ -36,13 +36,18 @@ impl SelfOpt {
                 let client = copt.to_client();
 
                 match client.whoami() {
-                    Ok(o_ent) => match o_ent {
-                        Some((ent, uat)) => {
-                            debug!("{:?}", ent);
-                            println!("{}", uat);
+                    Ok(o_ent) => {
+                        match o_ent {
+                            Some((ent, uat)) => {
+                                debug!("{:?}", ent);
+                                println!("{}", uat);
+                            }
+                            None => {
+                                error!("Authentication with cached token failed, can't query information.");
+                                // TODO: remove token when we know it's not valid
+                            }
                         }
-                        None => println!("Unauthenticated"),
-                    },
+                    }
                     Err(e) => println!("Error: {:?}", e),
                 }
             }
@@ -53,13 +58,13 @@ impl SelfOpt {
                 let password = match rpassword::prompt_password_stderr("Enter new password: ") {
                     Ok(p) => p,
                     Err(e) => {
-                        eprintln!("Error -> {:?}", e);
+                        error!("Error -> {:?}", e);
                         return;
                     }
                 };
 
                 if let Err(e) = client.idm_account_set_password(password) {
-                    eprintln!("Error -> {:?}", e);
+                    error!("Error -> {:?}", e);
                 }
             }
         }
@@ -106,7 +111,7 @@ pub(crate) fn password_prompt(prompt: &str) -> Option<String> {
         if password == password_confirm {
             return Some(password);
         } else {
-            eprintln!("Passwords do not match");
+            error!("Passwords do not match");
         }
     }
     None
diff --git a/kanidm_tools/src/cli/login.rs b/kanidm_tools/src/cli/login.rs
index 2884fb7a9..b4b8dfe70 100644
--- a/kanidm_tools/src/cli/login.rs
+++ b/kanidm_tools/src/cli/login.rs
@@ -4,6 +4,7 @@ use kanidm_proto::v1::{AuthAllowed, AuthResponse, AuthState};
 use libc::umask;
 use std::collections::BTreeMap;
 use std::fs::{create_dir, File};
+use std::io::ErrorKind;
 use std::io::{self, BufReader, BufWriter};
 use std::path::PathBuf;
 use webauthn_authenticator_rs::{u2fhid::U2FHid, RequestChallengeResponse, WebauthnAuthenticator};
@@ -15,7 +16,7 @@ pub fn read_tokens() -> Result<BTreeMap<String, String>, ()> {
     let token_path = PathBuf::from(shellexpand::tilde(TOKEN_PATH).into_owned());
     if !token_path.exists() {
         debug!(
-            "Token path {} does not exist, assuming empty ... ",
+            "Token cache file path {:?} does not exist, returning an empty token store.",
             TOKEN_PATH
         );
         return Ok(BTreeMap::new());
@@ -26,11 +27,24 @@ pub fn read_tokens() -> Result<BTreeMap<String, String>, ()> {
     let file = match File::open(&token_path) {
         Ok(f) => f,
         Err(e) => {
-            warn!(
-                "Cannot read tokens from {} due to error: {:?} ... continuing.",
-                TOKEN_PATH, e
-            );
-            return Ok(BTreeMap::new());
+            match e.kind() {
+                ErrorKind::PermissionDenied => {
+                    // we bail here because you won't be able to write them back...
+                    error!(
+                        "Permission denied reading token store file {:?}",
+                        &token_path
+                    );
+                    return Err(());
+                }
+                // other errors are OK to continue past
+                _ => {
+                    warn!(
+                        "Cannot read tokens from {} due to error: {:?} ... continuing.",
+                        TOKEN_PATH, e
+                    );
+                    return Ok(BTreeMap::new());
+                }
+            };
         }
     };
     let reader = BufReader::new(file);
diff --git a/kanidmd/src/lib/core/ldaps.rs b/kanidmd/src/lib/core/ldaps.rs
index edd9f8eab..699f82e22 100644
--- a/kanidmd/src/lib/core/ldaps.rs
+++ b/kanidmd/src/lib/core/ldaps.rs
@@ -153,6 +153,12 @@ pub(crate) async fn create_ldap_server(
     opt_tls_params: Option<SslAcceptorBuilder>,
     qe_r_ref: &'static QueryServerReadV1,
 ) -> Result<(), ()> {
+    if address.starts_with(":::") {
+        // takes :::xxxx to xxxx
+        let port = address.replacen(":::", "", 1);
+        eprintln!("Address '{}' looks like an attempt to wildcard bind with IPv6 on port {} - please try using ldapbindaddress = '[::]:{}'", address, port, port);
+    };
+
     let addr = net::SocketAddr::from_str(address).map_err(|e| {
         eprintln!("Could not parse ldap server address {} -> {:?}", address, e);
     })?;
diff --git a/kanidmd/src/lib/entry.rs b/kanidmd/src/lib/entry.rs
index a6c7afd12..72d6707a4 100644
--- a/kanidmd/src/lib/entry.rs
+++ b/kanidmd/src/lib/entry.rs
@@ -2,23 +2,21 @@
 //! concepts along with [`filter`]s and [`schema`] that everything else builds upon.
 //!
 //! An [`Entry`] is a collection of attribute-value sets. There are sometimes called attribute value
-//! assertions, or avas. The attribute is a "key" and it holds 1 to infinitite associtade values
-//! with no ordering. An entry has many avas. A pseudo example, minus schema and typing:
+//! assertions, or AVAs. The attribute is a "key" and it holds 1 to infinite associated values
+//! with no ordering. An entry has many AVAs. A pseudo example, minus schema and typing:
 //!
-//! ```
-//! /*
+//! ```text
 //! Entry {
 //!   "name": ["william"],
 //!   "uuid": ["..."],
 //!   "mail": ["maila@example.com", "mailb@example.com"],
-//! }
-//! */
+//! };
 //! ```
 //!
 //! There are three rules for entries:
-//! * Must have an ava for UUID containing a single value.
-//! * Any ava with zero values will be removed.
-//! * Avas are stored with no sorting.
+//! * Must have an AVA for UUID containing a single value.
+//! * Any AVA with zero values will be removed.
+//! * AVAs are stored with no sorting.
 //!
 //! For more, see the [`Entry`] type.
 //!
@@ -56,7 +54,7 @@ use uuid::Uuid;
 // use std::str::FromStr;
 
 // make a trait entry for everything to adhere to?
-//  * How to get indexs out?
+//  * How to get indexes out?
 //  * How to track pending diffs?
 
 // Entry is really similar to serde Value, but limits the possibility
@@ -172,17 +170,17 @@ fn compare_attrs(left: &Map<AttrString, Set<Value>>, right: &Map<AttrString, Set
 /// Entry is the core data storage type of the server. Almost every aspect of the server is
 /// designed to read, handle and manipulate entries.
 ///
-/// Entries store attribute value assertions, or ava. These are sets of key-values.
+/// Entries store attribute value assertions, or AVA. These are sets of key-values.
 ///
 /// Entries have a lifecycle within a single operation, and as part of replication.
 /// The lifecycle for operations is defined through state and valid types. Each entry has a pair
-/// Of these types at anytime. The first is the ava [`schema`] and [`access`] control assertion
+/// Of these types at anytime. The first is the AVA [`schema`] and [`access`] control assertion
 /// state. This is represented by the type `VALID` as one of `EntryValid`, `EntryInvalid` or
 /// `EntryReduced`. Every entry starts as `EntryInvalid`, and when checked by the schema for
 /// correctness, transitions to `EntryValid`. While an entry is `EntryValid` it can not be
 /// altered - you must invalidate it to `EntryInvalid`, then modify, then check again.
 /// An entry that has had access controls applied moves from `EntryValid` to `EntryReduced`,
-/// to show that the avas have reduced to the valid read set of the current [`event`] user.
+/// to show that the AVAs have reduced to the valid read set of the current [`event`] user.
 ///
 /// The second type of `STATE` represents the database commit state and internal db ID's. A
 /// new entry that has never been committed is `EntryNew`, but an entry that has been retrieved
diff --git a/kanidmd/src/lib/filter.rs b/kanidmd/src/lib/filter.rs
index f84135ed5..d216ebe02 100644
--- a/kanidmd/src/lib/filter.rs
+++ b/kanidmd/src/lib/filter.rs
@@ -1,9 +1,9 @@
 //! [`Filter`]s are one of the three foundational concepts of the design in kanidm.
-//! They are used in nearly every aspect ofthe server to provide searching of
-//! datasets, and assertion of entry properties.
+//! They are used in nearly every aspect of the server to provide searching of
+//! datasets and assertion of entry properties.
 //!
-//! A filter is a logical statement of properties that an [`Entry`] and it's
-//! avas must uphold to be considered true.
+//! A filter is a logical statement of properties that an [`Entry`] and its
+//! AVAs must uphold to be considered true.
 //!
 //! [`Filter`]: struct.Filter.html
 //! [`Entry`]: ../entry/struct.Entry.html
diff --git a/kanidmd/src/lib/ldap.rs b/kanidmd/src/lib/ldap.rs
index 7ccb952cb..a660dd4ae 100644
--- a/kanidmd/src/lib/ldap.rs
+++ b/kanidmd/src/lib/ldap.rs
@@ -431,8 +431,6 @@ fn ldap_domain_to_dc(input: &str) -> String {
         output.push_str(dc);
         #[allow(clippy::single_char_add_str)]
         output.push_str(",");
-        //  Can't use concat as it's evalled at compile, not run time.
-        // output.push_str(concat!("dc=", dc, ","));
     });
     // Remove the last ','
     output.pop();