Chasing wooly quadrapeds again (#2163)

* I really like well-tended yaks
* documenting yaks
* spellink
* less surprise more good
* schema test fix
* clippyisms

Cargo.toml

@@ -126,6 +126,7 @@ gix = { version = "0.53.1", default-features = false }
 gloo = "^0.8.1"
 hashbrown = { version = "0.14.1", features = ["serde", "inline-more", "ahash"] }
 hex = "^0.4.3"
+http = "0.2.9"
 hyper = { version = "0.14.27", features = ["full"] }
 hyper-tls = "0.5.0"
 idlset = "^0.2.4"

libs/client/src/lib.rs

@@ -172,7 +172,6 @@ impl KanidmClientBuilder {
             }
         }
 
-        // TODO #725: Handle these errors better, or at least provide diagnostics - this currently fails silently
         let mut f = File::open(ca_path).map_err(|e| {
             error!("{:?}", e);
             ClientError::ConfigParseIssue(format!("{:?}", e))

proto/src/constants.rs

@@ -161,13 +161,6 @@ pub const ATTR_USERPASSWORD: &str = "userpassword";
 pub const ATTR_UUID: &str = "uuid";
 pub const ATTR_VERSION: &str = "version";
 
-// TODO: rust can't deal with this being compiled out, don't try and #[cfg()] them
-pub const TEST_ATTR_NON_EXIST: &str = "non-exist";
-pub const TEST_ATTR_TEST_ATTR: &str = "testattr";
-pub const TEST_ATTR_EXTRA: &str = "extra";
-pub const TEST_ATTR_NUMBER: &str = "testattrnumber";
-pub const TEST_ATTR_NOTALLOWED: &str = "notallowed";
-
 pub const OAUTH2_SCOPE_EMAIL: &str = ATTR_EMAIL;
 pub const OAUTH2_SCOPE_GROUPS: &str = "groups";
 pub const OAUTH2_SCOPE_OPENID: &str = "openid";
@@ -188,3 +181,10 @@ pub const LDAP_ATTR_MEMBER: &str = "member";
 pub const LDAP_ATTR_NAME: &str = "name";
 pub const LDAP_ATTR_OBJECTCLASS: &str = "objectClass";
 pub const LDAP_ATTR_OU: &str = "ou";
+
+// rust can't deal with this being compiled out, don't try and #[cfg()] them
+pub const TEST_ATTR_NON_EXIST: &str = "non-exist";
+pub const TEST_ATTR_TEST_ATTR: &str = "testattr";
+pub const TEST_ATTR_EXTRA: &str = "extra";
+pub const TEST_ATTR_NUMBER: &str = "testattrnumber";
+pub const TEST_ATTR_NOTALLOWED: &str = "notallowed";

proto/src/v1.rs

@@ -19,8 +19,22 @@ use crate::constants::{ATTR_GROUP, ATTR_LDAP_SSHPUBLICKEY};
 
 // These proto implementations are here because they have public definitions
 
-/* ===== errors ===== */
+#[derive(Clone, Copy, Debug)]
+pub enum AccountType {
+    Person,
+    ServiceAccount,
+}
 
+impl ToString for AccountType {
+    fn to_string(&self) -> String {
+        match self {
+            AccountType::Person => "person".to_string(),
+            AccountType::ServiceAccount => "service_account".to_string(),
+        }
+    }
+}
+
+/* ===== errors ===== */
 #[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
 #[serde(rename_all = "lowercase")]
 pub enum SchemaError {

server/core/Cargo.toml

@@ -24,7 +24,8 @@ compact_jwt = { workspace = true }
 cron = { workspace = true }
 futures = { workspace = true }
 futures-util = { workspace = true }
-http = "0.2.9"
+http = { workspace = true }
+
 hyper = { workspace = true }
 kanidm_proto = { workspace = true }
 kanidm_utils_users = { workspace = true }

server/core/src/https/middleware/compression.rs

@@ -1,23 +1,31 @@
-/*
+//! Let's build a compression middleware!
-    Let's build a compression middleware!
+//!
-
+//! The threat of the TLS BREACH attack (1) was considered as part of adding
-    The threat of the TLS BREACH attack [1] was considered as part of adding
+//! the CompressMiddleware configuration.
-    the CompressMiddleware configuration.
+//!
-
+//! The attack targets secrets which are compressed and encrypted in flight
-    The attack targets secrets compressed and encrypted in flight with the intent
+//! with the intent to infer their content.
-    to infer their content.
+//!
-
+//! This is not a concern for the paths covered by this configuration:
-    This is not a concern for the paths covered by this configuration
+//!
-    ( /, /ui/<and all sub-paths>, /pkg/<and all sub-paths> ),
+//! * `/`
-    as they're all static content with no secrets in transit - all that data should
+//! * `/ui/<and all sub-paths>`
-    come from Kanidm's REST API, which is on a different path and not covered by
+//! * `/pkg/<and all sub-paths>`
-    the compression middleware.
+//!
-
+//! as they're all static content with no secrets in transit - all that data should
-
+//! come from Kanidm's REST API, which is on a different path and not covered by
-    [1] - https://resources.infosecinstitute.com/topic/the-breach-attack/
+//! the compression middleware.
-*/
+//!
+//! (1) - <https://resources.infosecinstitute.com/topic/the-breach-attack/>
+//!
 
 use tower_http::compression::CompressionLayer;
+// TODO: this should skip compression on responses smaller than ~256 bytes because gzip can make them bigger.
+/// This builds a compression layer with the following configuration:
+///
+/// * No brotli compression - because that's *very* slow to compress dynamically
+/// * "Best" quality of compression, usually produces the smallest size.
+///
 pub fn new() -> CompressionLayer {
     CompressionLayer::new()
         .no_br()

server/lib/src/be/idl_sqlite.rs

@@ -1149,9 +1149,7 @@ impl IdlSqliteWriteTransaction {
             .prepare(&format!("SELECT cid FROM {}.ruv", self.get_db_name()))
             .map_err(sqlite_error)?;
 
-        let kh_iter = stmt
+        let kh_iter = stmt.query_map([], |row| row.get(0)).map_err(sqlite_error)?;
-            .query_map([], |row| Ok(row.get(0)?))
-            .map_err(sqlite_error)?;
 
         kh_iter
             .map(|v| {

server/lib/src/constants/entries.rs

@@ -9,7 +9,7 @@ use crate::idm::account::Account;
 use crate::value::PartialValue;
 use crate::value::Value;
 use kanidm_proto::constants::*;
-use kanidm_proto::v1::{Filter, OperationError, UiHint};
+use kanidm_proto::v1::{AccountType, Filter, OperationError, UiHint};
 
 #[cfg(test)]
 use uuid::uuid;
@@ -734,13 +734,7 @@ impl TryFrom<BuiltinGroup> for EntryInitNew {
 lazy_static! {
     /// Builtin System Admin account.
     pub static ref BUILTIN_ACCOUNT_IDM_ADMIN: BuiltinAccount = BuiltinAccount {
-        // TODO: this really should be a "are you a service account or a person" enum
+        account_type: AccountType::ServiceAccount,
-        classes: vec![
-            EntryClass::Account,
-            EntryClass::ServiceAccount,
-            EntryClass::MemberOf,
-            EntryClass::Object,
-        ],
         name: "idm_admin",
         uuid: UUID_IDM_ADMIN,
         description: "Builtin IDM Admin account.",
@@ -1218,8 +1212,7 @@ Attribute::Description,
 #[derive(Debug, Clone)]
 /// Built in accounts such as anonymous, idm_admin and admin
 pub struct BuiltinAccount {
-    // TODO: this really should be a "are you a service account or a person" enum
+    pub account_type: kanidm_proto::v1::AccountType,
-    pub classes: Vec<EntryClass>,
     pub name: &'static str,
     pub uuid: Uuid,
     pub description: &'static str,
@@ -1229,7 +1222,7 @@ pub struct BuiltinAccount {
 impl Default for BuiltinAccount {
     fn default() -> Self {
         BuiltinAccount {
-            classes: [EntryClass::Object].to_vec(),
+            account_type: AccountType::ServiceAccount,
             name: "",
             uuid: Uuid::new_v4(),
             description: "<set description>",
@@ -1260,17 +1253,20 @@ impl From<BuiltinAccount> for EntryInitNew {
         entry.add_ava(Attribute::Description, Value::new_utf8s(value.description));
         entry.add_ava(Attribute::DisplayName, Value::new_utf8s(value.displayname));
 
-        entry.add_ava(Attribute::Class, EntryClass::Object.to_value());
-        entry.add_ava(Attribute::Class, EntryClass::Account.to_value());
         entry.set_ava(
             Attribute::Class,
-            value
+            vec![
-                .classes
+                EntryClass::Account.to_value(),
-                .into_iter()
+                EntryClass::MemberOf.to_value(),
-                .map(|c| c.to_value())
+                EntryClass::Object.to_value(),
-                .collect::<Vec<Value>>(),
+            ],
         );
-
+        match value.account_type {
+            AccountType::Person => entry.add_ava(Attribute::Class, EntryClass::Person.to_value()),
+            AccountType::ServiceAccount => {
+                entry.add_ava(Attribute::Class, EntryClass::ServiceAccount.to_value())
+            }
+        }
         entry
     }
 }
@@ -1279,24 +1275,14 @@ lazy_static! {
 
     /// Builtin System Admin account.
     pub static ref BUILTIN_ACCOUNT_ADMIN: BuiltinAccount = BuiltinAccount {
-        classes: vec![
+        account_type: AccountType::ServiceAccount,
-            EntryClass::Account,
-            EntryClass::ServiceAccount,
-            EntryClass::MemberOf,
-            EntryClass::Object,
-        ],
         name: "admin",
         uuid: UUID_ADMIN,
         description: "Builtin System Admin account.",
         displayname: "System Administrator",
     };
     pub static ref BUILTIN_ACCOUNT_ANONYMOUS_V1: BuiltinAccount = BuiltinAccount {
-        classes: [
+        account_type: AccountType::ServiceAccount,
-            EntryClass::Account,
-            EntryClass::ServiceAccount,
-            EntryClass::Object,
-        ]
-        .to_vec(),
         name: "anonymous",
         uuid: UUID_ANONYMOUS,
         description: "Anonymous access account.",

server/lib/src/constants/schema.rs

@@ -770,7 +770,6 @@ pub static ref SCHEMA_CLASS_OAUTH2_RS_BASIC: SchemaClass = SchemaClass {
 
     systemmay: vec![ Attribute::OAuth2AllowInsecureClientDisablePkce.into()],
     systemmust: vec![ Attribute::OAuth2RsBasicSecret.into()],
-    // TODO: is this a class exclude or an attribute exclude?
     systemexcludes: vec![ EntryClass::OAuth2ResourceServerPublic.into()],
     ..Default::default()
 };
@@ -781,7 +780,6 @@ pub static ref SCHEMA_CLASS_OAUTH2_RS_PUBLIC: SchemaClass = SchemaClass {
     name: EntryClass::OAuth2ResourceServerPublic.into(),
 
     description: "The class representing a configured Oauth2 Resource Server with public clients and pkce verification".to_string(),
-    // TODO: is this a class exclude or an attribute exclude, or both?
     systemexcludes: vec![EntryClass::OAuth2ResourceServerBasic.into()],
     ..Default::default()
 };

server/lib/src/entry.rs

@@ -2971,11 +2971,10 @@ impl<VALID, STATE> Entry<VALID, STATE> {
             }
         }
 
-        let mut res = Vec::new();
+        let res: Vec<FC<'_>> = pairs
-        for (attr, pv) in pairs.into_iter() {
+            .into_iter()
-            // We use FC directly here instead of f_eq to avoid an excess clone.
+            .map(|(attr, pv)| FC::Eq(attr, pv))
-            res.push(FC::Eq(attr, pv)) // TODO: this is kinda terrible
+            .collect();
-        }
         Some(filter_all!(f_and(res)))
     }
 

server/lib/src/filter.rs

@@ -658,10 +658,10 @@ impl FilterComp {
                 }
             }
             FilterComp::Or(filters) => {
-                // If all filters are okay, return Ok(Filter::Or())
+                // * If all filters are okay, return Ok(Filter::Or())
-                // If any is invalid, return the error.
+                // * Any filter is invalid, return the error.
-                // TODO: ftweedal says an empty or is a valid filter
+                // * An empty "or" is a valid filter in mathematical terms, but we throw an
-                // in mathematical terms.
+                //   error to warn the user because it's super unlikey they want that
                 if filters.is_empty() {
                     return Err(SchemaError::EmptyFilter);
                 };
@@ -673,8 +673,10 @@ impl FilterComp {
                 x.map(FilterComp::Or)
             }
             FilterComp::And(filters) => {
-                // TODO: ftweedal says an empty or is a valid filter
+                // * If all filters are okay, return Ok(Filter::Or())
-                // in mathematical terms.
+                // * Any filter is invalid, return the error.
+                // * An empty "and" is a valid filter in mathematical terms, but we throw an
+                //   error to warn the user because it's super unlikey they want that
                 if filters.is_empty() {
                     return Err(SchemaError::EmptyFilter);
                 };
@@ -1013,14 +1015,12 @@ impl FilterResolved {
             }
             FilterComp::Pres(a) => {
                 let idx = idxmeta.contains(&(&a, &IndexType::Presence));
-                let idx = NonZeroU8::new(idx as u8);
+                FilterResolved::Pres(a, NonZeroU8::new(idx as u8))
-                FilterResolved::Pres(a, idx)
             }
             FilterComp::LessThan(a, v) => {
                 // let idx = idxmeta.contains(&(&a, &IndexType::ORDERING));
                 // TODO: For now, don't emit ordering indexes.
-                let idx = None;
+                FilterResolved::LessThan(a, v, None)
-                FilterResolved::LessThan(a, v, idx)
             }
             FilterComp::Or(vs) => FilterResolved::Or(
                 vs.into_iter()

server/lib/src/lib.rs

@@ -8,9 +8,7 @@
 #![deny(clippy::suspicious)]
 #![deny(clippy::perf)]
 // Specific lints to enforce.
-// TODO: can't use this until we have a better way to handle the 'todo' lint?
+#![deny(clippy::todo)]
-// #![deny(clippy::todo)]
-#![warn(clippy::todo)]
 #![deny(clippy::unimplemented)]
 #![deny(clippy::unwrap_used)]
 #![deny(clippy::expect_used)]

server/lib/src/value.rs

@@ -1133,7 +1133,6 @@ impl Value {
         matches!(self, Value::Iutf8(_))
     }
 
-    // TODO: take this away
     #[inline(always)]
     pub fn new_class(s: &str) -> Self {
         Value::Iutf8(s.to_lowercase())

server/lib/src/valueset/image/mod.rs

@@ -345,13 +345,7 @@ impl ValueSetT for ValueSetImage {
 
                 let res: Vec<bool> = imgset
                     .iter()
-                    .filter_map(|image| {
+                    .filter(|image| &image.hash_imagevalue() == pv)
-                        if &image.hash_imagevalue() == pv {
-                            Some(image)
-                        } else {
-                            None
-                        }
-                    })
                     .map(|image| self.set.remove(image))
                     .collect();
                 res.into_iter().any(|e| e)