Repository: lldap/lldap (language: rust)

Issue:
Expand attributes is called once per result entry
Instead of once per request


Discussion / hints:
And it shouldn't be called if we don't have entries either, to avoid spurious warnings when the base DN is too broad

Fix patch:
diff --git a/server/src/domain/ldap/group.rs b/server/src/domain/ldap/group.rs
index d59a78da..b41fc196 100644
--- a/server/src/domain/ldap/group.rs
+++ b/server/src/domain/ldap/group.rs
@@ -100,13 +100,11 @@ fn expand_group_attribute_wildcards(attributes: &[String]) -> Vec<&str> {
 fn make_ldap_search_group_result_entry(
     group: Group,
     base_dn_str: &str,
-    attributes: &[String],
+    expanded_attributes: &[&str],
     user_filter: &Option<UserId>,
     ignored_group_attributes: &[AttributeName],
     schema: &PublicSchema,
 ) -> LdapSearchResultEntry {
-    let expanded_attributes = expand_group_attribute_wildcards(attributes);
-
     LdapSearchResultEntry {
         dn: format!("cn={},ou=groups,{}", group.display_name, base_dn_str),
         attributes: expanded_attributes
@@ -267,11 +265,17 @@ pub fn convert_groups_to_ldap_op<'a>(
     user_filter: &'a Option<UserId>,
     schema: &'a PublicSchema,
 ) -> impl Iterator<Item = LdapOp> + 'a {
+    let expanded_attributes = if groups.is_empty() {
+        None
+    } else {
+        Some(expand_group_attribute_wildcards(attributes))
+    };
+
     groups.into_iter().map(move |g| {
         LdapOp::SearchResultEntry(make_ldap_search_group_result_entry(
             g,
             &ldap_info.base_dn_str,
-            attributes,
+            expanded_attributes.as_ref().unwrap(),
             user_filter,
             &ldap_info.ignored_group_attributes,
             schema,
diff --git a/server/src/domain/ldap/user.rs b/server/src/domain/ldap/user.rs
index f6b2fe86..925e8886 100644
--- a/server/src/domain/ldap/user.rs
+++ b/server/src/domain/ldap/user.rs
@@ -119,12 +119,11 @@ const ALL_USER_ATTRIBUTE_KEYS: &[&str] = &[
 fn make_ldap_search_user_result_entry(
     user: User,
     base_dn_str: &str,
-    attributes: &[String],
+    expanded_attributes: &[&str],
     groups: Option<&[GroupDetails]>,
     ignored_user_attributes: &[AttributeName],
     schema: &PublicSchema,
 ) -> LdapSearchResultEntry {
-    let expanded_attributes = expand_user_attribute_wildcards(attributes);
     let dn = format!("uid={},ou=people,{}", user.user_id.as_str(), base_dn_str);
     LdapSearchResultEntry {
         dn,
@@ -295,11 +294,16 @@ pub fn convert_users_to_ldap_op<'a>(
     ldap_info: &'a LdapInfo,
     schema: &'a PublicSchema,
 ) -> impl Iterator<Item = LdapOp> + 'a {
+    let expanded_attributes = if users.is_empty() {
+        None
+    } else {
+        Some(expand_user_attribute_wildcards(attributes))
+    };
     users.into_iter().map(move |u| {
         LdapOp::SearchResultEntry(make_ldap_search_user_result_entry(
             u.user,
             &ldap_info.base_dn_str,
-            attributes,
+            expanded_attributes.as_ref().unwrap(),
             u.groups.as_deref(),
             &ldap_info.ignored_user_attributes,
             schema,
diff --git a/server/src/domain/ldap/utils.rs b/server/src/domain/ldap/utils.rs
index a1696acb..ad580bf5 100644
--- a/server/src/domain/ldap/utils.rs
+++ b/server/src/domain/ldap/utils.rs
@@ -114,21 +114,21 @@ pub fn expand_attribute_wildcards<'a>(
     ldap_attributes: &'a [String],
     all_attribute_keys: &'a [&'static str],
 ) -> Vec<&'a str> {
-    let mut attributes_out = ldap_attributes
+    let extra_attributes =
+        if ldap_attributes.iter().any(|x| x == "*") || ldap_attributes.is_empty() {
+            all_attribute_keys
+        } else {
+            &[]
+        }
         .iter()
-        .map(String::as_str)
-        .collect::<Vec<_>>();
-
-    if attributes_out.iter().any(|&x| x == "*") || attributes_out.is_empty() {
-        // Remove occurrences of '*'
-        attributes_out.retain(|&x| x != "*");
-        // Splice in all non-operational attributes
-        attributes_out.extend(all_attribute_keys.iter());
-    }
+        .copied();
+    let attributes_out = ldap_attributes
+        .iter()
+        .map(|s| s.as_str())
+        .filter(|&s| s != "*" && s != "+" && s != "1.1");
 
     // Deduplicate, preserving order
-    let resolved_attributes = attributes_out
-        .into_iter()
+    let resolved_attributes = itertools::chain(attributes_out, extra_attributes)
         .unique_by(|a| a.to_ascii_lowercase())
         .collect_vec();
     debug!(?resolved_attributes);


Test patch: