From 1fc5f31ed5beb04d991baef038b33ca270cfef91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 5 Mar 2026 12:19:20 +0100 Subject: [PATCH 1/9] Change user_as_ldap_mod --- .../defguard_core/src/enterprise/ldap/mod.rs | 26 +++--- .../src/enterprise/ldap/model.rs | 42 ++++++---- .../src/enterprise/ldap/tests.rs | 82 +++++++++++++++---- 3 files changed, 105 insertions(+), 45 deletions(-) diff --git a/crates/defguard_core/src/enterprise/ldap/mod.rs b/crates/defguard_core/src/enterprise/ldap/mod.rs index fde3c12859..699b476d91 100644 --- a/crates/defguard_core/src/enterprise/ldap/mod.rs +++ b/crates/defguard_core/src/enterprise/ldap/mod.rs @@ -225,7 +225,8 @@ impl LDAPConfig { /// Constructs user distinguished name. /// /// This function is used to construct the user's DN based on the RDN value and user path. - /// Prefer using `user_dn_from_user` method to ensure that the RDN value and user path are correctly derived from the user object. + /// Prefer using `user_dn_from_user` method to ensure that the RDN value and user path are + /// correctly derived from the user object. /// /// Use it only if you need to construct a user DN manually. #[must_use] @@ -595,10 +596,10 @@ impl LDAPConnection { /// Returns an error if the user doesn't exist at the specified DN. pub async fn get_user_by_dn(&mut self, user: &User) -> Result { let dn = self.config.user_dn_from_user(user); - debug!("Trying to retrieve LDAP user with the following DN: {}", dn); + debug!("Trying to retrieve LDAP user with the following DN: {dn}"); match self.get(&dn).await? { Some(entry) => { - info!("Found LDAP user with DN: {}", dn); + info!("Found LDAP user with DN: {dn}"); user_from_searchentry(&entry, &user.username, None) } None => Err(LdapError::ObjectNotFound(format!("User {dn} not found",))), @@ -622,9 +623,9 @@ impl LDAPConnection { debug!("Using provided password for user {user}"); password.to_string() } else { - // ldap may not accept no password, this is a workaround when we don't have access to the - // user's password - debug!("Generating random password for user {user}, as no password has been specified",); + // LDAP may not accept no password, this is a workaround when we don't have access to + // the user's password + debug!("Generating random password for user {user}, as no password has been specified"); let random_password = rand::thread_rng() .sample_iter(&rand::distributions::Alphanumeric) .take(32) @@ -704,8 +705,7 @@ impl LDAPConnection { }; let old_dn = self.config.user_dn(old_rdn, user_dn_path); let new_dn = self.config.user_dn(new_rdn, user_dn_path); - let config = self.config.clone(); - let mods = user_as_ldap_mod(user, &config); + let mods = user_as_ldap_mod(user, &self.config); self.modify(&old_dn, &new_dn, mods).await?; info!("Modified user {old_username} in LDAP"); @@ -839,7 +839,7 @@ impl LDAPConnection { group_name: &str, members: Vec<&User>, ) -> Result<(), LdapError> { - debug!("Adding LDAP group {}", group_name); + debug!("Adding LDAP group {group_name}"); let dn = self.config.group_dn(group_name); let group_obj_class = self.config.ldap_group_obj_class.clone(); let groupname_attr = self.config.ldap_groupname_attr.clone(); @@ -853,7 +853,10 @@ impl LDAPConnection { .map(|member| self.config.user_dn_from_user(member)) .collect::>(); let member_group_attr = self.config.ldap_group_member_attr.clone(); - let member_refs: HashSet<&str> = member_dns.iter().map(String::as_str).collect(); + let member_refs = member_dns + .iter() + .map(String::as_str) + .collect::>(); for member_ref in member_refs { group_attrs.push((member_group_attr.as_str(), hashset![member_ref])); @@ -861,8 +864,7 @@ impl LDAPConnection { self.add(&dn, group_attrs).await?; info!( - "Added LDAP group {} with members {}", - group_name, + "Added LDAP group {group_name} with members {}", member_dns .iter() .map(String::as_str) diff --git a/crates/defguard_core/src/enterprise/ldap/model.rs b/crates/defguard_core/src/enterprise/ldap/model.rs index a99d5709ee..4fb08741fe 100644 --- a/crates/defguard_core/src/enterprise/ldap/model.rs +++ b/crates/defguard_core/src/enterprise/ldap/model.rs @@ -105,22 +105,26 @@ pub(crate) fn update_from_ldap_user(user: &mut User, ldap_user: &User, con } } +/// Return a vector of LDAP modifications for a given [`User`]. #[must_use] -pub fn user_as_ldap_mod<'a, I>(user: &'a User, config: &'a LDAPConfig) -> Vec> { +pub fn user_as_ldap_mod(user: &User, config: &LDAPConfig) -> Vec> { let obj_classes = config.get_all_user_obj_classes(); - let mut changes = vec![]; + let mut changes = Vec::new(); if obj_classes.contains(&UserObjectClass::InetOrgPerson.into()) || obj_classes.contains(&UserObjectClass::User.into()) { changes.extend_from_slice(&[ - Mod::Replace("sn", hashset![user.last_name.as_str()]), - Mod::Replace("givenName", hashset![user.first_name.as_str()]), - Mod::Replace("mail", hashset![user.email.as_str()]), + Mod::Replace("sn".to_string(), hashset![user.last_name.clone()]), + Mod::Replace("givenName".to_string(), hashset![user.first_name.clone()]), + Mod::Replace("mail".to_string(), hashset![user.email.clone()]), ]); // Allow renaming the user if the CN is not a part of the RDN if !config.get_rdn_attr().eq_ignore_ascii_case("cn") { - changes.push(Mod::Replace("cn", hashset![user.username.as_str()])); + changes.push(Mod::Replace( + "cn".to_string(), + hashset![user.username.clone()], + )); } if !config.ldap_username_attr.eq_ignore_ascii_case("uid") @@ -129,15 +133,21 @@ pub fn user_as_ldap_mod<'a, I>(user: &'a User, config: &'a LDAPConfig) -> Vec .as_ref() .is_some_and(|rdn_attr| rdn_attr.eq_ignore_ascii_case("uid")) { - changes.push(Mod::Replace("uid", hashset![user.username.as_str()])); + changes.push(Mod::Replace( + "uid".to_string(), + hashset![user.username.clone()], + )); } if let Some(phone) = &user.phone { - if phone.is_empty() { - changes.push(Mod::Replace("mobile", HashSet::new())); - } else { - changes.push(Mod::Replace("mobile", hashset![phone.as_str()])); - } + changes.push(Mod::Replace( + "mobile".to_string(), + if phone.is_empty() { + HashSet::::new() + } else { + hashset![phone.clone()] + }, + )); } } else { warn!( @@ -148,8 +158,8 @@ pub fn user_as_ldap_mod<'a, I>(user: &'a User, config: &'a LDAPConfig) -> Vec if config.ldap_uses_ad && !config.get_rdn_attr().eq_ignore_ascii_case("sAMAccountName") { changes.push(Mod::Replace( - "sAMAccountName", - hashset![user.username.as_str()], + "sAMAccountName".to_string(), + hashset![user.username.clone()], )); } @@ -164,8 +174,8 @@ pub fn user_as_ldap_mod<'a, I>(user: &'a User, config: &'a LDAPConfig) -> Vec .is_some_and(|rdn_attr| rdn_attr.eq_ignore_ascii_case(username_attr)) { changes.push(Mod::Replace( - username_attr, - hashset![user.username.as_str()], + username_attr.to_string(), + hashset![user.username.clone()], )); } diff --git a/crates/defguard_core/src/enterprise/ldap/tests.rs b/crates/defguard_core/src/enterprise/ldap/tests.rs index 4ef6c09c70..b00e84076a 100644 --- a/crates/defguard_core/src/enterprise/ldap/tests.rs +++ b/crates/defguard_core/src/enterprise/ldap/tests.rs @@ -3018,10 +3018,22 @@ fn test_as_ldap_mod_inetorgperson() { }; let mods = user_as_ldap_mod(&user, &config); - assert!(mods.contains(&Mod::Replace("sn", hashset!["Smith"]))); - assert!(mods.contains(&Mod::Replace("givenName", hashset!["John"]))); - assert!(mods.contains(&Mod::Replace("mail", hashset!["john.smith@example.com"]))); - assert!(mods.contains(&Mod::Replace("mobile", hashset!["5551234"]))); + assert!(mods.contains(&Mod::Replace( + "sn".to_string(), + hashset!["Smith".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "givenName".to_string(), + hashset!["John".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "mail".to_string(), + hashset!["john.smith@example.com".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "mobile".to_string(), + hashset!["5551234".to_string()], + ))); } #[test] @@ -3043,10 +3055,19 @@ fn test_as_ldap_mod_with_empty_phone() { let mods = user_as_ldap_mod(&user, &config); - assert!(mods.contains(&Mod::Replace("sn", hashset!["Smith"]))); - assert!(mods.contains(&Mod::Replace("givenName", hashset!["John"]))); - assert!(mods.contains(&Mod::Replace("mail", hashset!["john.smith@example.com"]))); - assert!(mods.contains(&Mod::Replace("mobile", HashSet::new()))); + assert!(mods.contains(&Mod::Replace( + "sn".to_string(), + hashset!["Smith".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "givenName".to_string(), + hashset!["John".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "mail".to_string(), + hashset!["john.smith@example.com".to_string()], + ))); + assert!(mods.contains(&Mod::Replace("mobile".to_string(), HashSet::new(),))); } #[test] @@ -3070,10 +3091,22 @@ fn test_as_ldap_mod_with_active_directory() { let mods = user_as_ldap_mod(&user, &config); - assert!(mods.contains(&Mod::Replace("sn", hashset!["Smith"]))); - assert!(mods.contains(&Mod::Replace("givenName", hashset!["John"]))); - assert!(mods.contains(&Mod::Replace("mail", hashset!["john.smith@example.com"]))); - assert!(mods.contains(&Mod::Replace("sAMAccountName", hashset!["testuser"]))); + assert!(mods.contains(&Mod::Replace( + "sn".to_string(), + hashset!["Smith".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "givenName".to_string(), + hashset!["John".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "mail".to_string(), + hashset!["john.smith@example.com".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "sAMAccountName".to_string(), + hashset!["testuser".to_string()], + ))); } #[test] @@ -3096,11 +3129,26 @@ fn test_as_ldap_mod_with_custom_rdn() { let mods = user_as_ldap_mod(&user, &config); - assert!(mods.contains(&Mod::Replace("sn", hashset!["Smith"]))); - assert!(mods.contains(&Mod::Replace("givenName", hashset!["John"]))); - assert!(mods.contains(&Mod::Replace("mail", hashset!["john.smith@example.com"]))); - assert!(mods.contains(&Mod::Replace("cn", hashset!["testuser"]))); - assert!(mods.contains(&Mod::Replace("sAMAccountName", hashset!["testuser"]))); + assert!(mods.contains(&Mod::Replace( + "sn".to_string(), + hashset!["Smith".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "givenName".to_string(), + hashset!["John".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "mail".to_string(), + hashset!["john.smith@example.com".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "cn".to_string(), + hashset!["testuser".to_string()], + ))); + assert!(mods.contains(&Mod::Replace( + "sAMAccountName".to_string(), + hashset!["testuser".to_string()], + ))); } #[test] From 4fedf570b3283cb65a118d9bc3883d2f5a500371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 5 Mar 2026 13:06:53 +0100 Subject: [PATCH 2/9] Optimisations --- crates/defguard_core/src/auth/mod.rs | 8 +-- .../enterprise/directory_sync/jumpcloud.rs | 4 +- .../enterprise/directory_sync/microsoft.rs | 24 +++---- .../src/enterprise/directory_sync/tests.rs | 2 +- .../src/enterprise/ldap/client.rs | 41 ++++------- .../defguard_core/src/enterprise/ldap/mod.rs | 24 ++----- .../src/enterprise/ldap/model.rs | 4 +- .../defguard_core/src/enterprise/ldap/sync.rs | 4 +- .../src/enterprise/ldap/test_client.rs | 7 +- .../src/enterprise/ldap/tests.rs | 68 +++++++++++-------- .../src/handlers/network_devices.rs | 2 +- .../api/wireguard_network_allowed_groups.rs | 4 +- crates/defguard_setup/src/auto_adoption.rs | 2 +- 13 files changed, 91 insertions(+), 103 deletions(-) diff --git a/crates/defguard_core/src/auth/mod.rs b/crates/defguard_core/src/auth/mod.rs index e9ee894fc3..6ba158f277 100644 --- a/crates/defguard_core/src/auth/mod.rs +++ b/crates/defguard_core/src/auth/mod.rs @@ -383,19 +383,19 @@ mod tests { // No requested scopes let all_scopes = vec!["email".to_string(), "profile".to_string()]; - let requested_scopes = vec![]; + let requested_scopes = Vec::new(); let result = get_available_scopes(&all_scopes, &requested_scopes); assert_eq!(result, Vec::<&str>::new()); // No available scopes - let all_scopes = vec![]; + let all_scopes = Vec::new(); let requested_scopes = vec!["email".to_string(), "profile".to_string()]; let result = get_available_scopes(&all_scopes, &requested_scopes); assert_eq!(result, Vec::<&str>::new()); // Both empty - let all_scopes = vec![]; - let requested_scopes = vec![]; + let all_scopes = Vec::new(); + let requested_scopes = Vec::new(); let result = get_available_scopes(&all_scopes, &requested_scopes); assert_eq!(result, Vec::<&str>::new()); diff --git a/crates/defguard_core/src/enterprise/directory_sync/jumpcloud.rs b/crates/defguard_core/src/enterprise/directory_sync/jumpcloud.rs index 93d8f78280..8c8b86b936 100644 --- a/crates/defguard_core/src/enterprise/directory_sync/jumpcloud.rs +++ b/crates/defguard_core/src/enterprise/directory_sync/jumpcloud.rs @@ -731,7 +731,7 @@ mod tests { let group_empty_ldap = UserGroup { id: "group789".to_string(), compiled_attributes: CompiledAttributes { - ldap_groups: vec![], + ldap_groups: Vec::new(), }, }; let directory_group_empty_ldap: DirectoryGroup = group_empty_ldap.into(); @@ -743,7 +743,7 @@ mod tests { fn test_response_collection_conversions() { // Test empty UsersResponse conversion let empty_users_response = UsersResponse { - results: vec![], + results: Vec::new(), total_count: 0, }; let empty_directory_users: Vec = empty_users_response.into(); diff --git a/crates/defguard_core/src/enterprise/directory_sync/microsoft.rs b/crates/defguard_core/src/enterprise/directory_sync/microsoft.rs index 7e02645e7f..5b90b176cb 100644 --- a/crates/defguard_core/src/enterprise/directory_sync/microsoft.rs +++ b/crates/defguard_core/src/enterprise/directory_sync/microsoft.rs @@ -581,7 +581,7 @@ mod tests { "client_id".to_string(), "client_secret".to_string(), "https://login.microsoftonline.com/tenant-id-123/v2.0".to_string(), - vec![], + Vec::new(), ); let tenant = provider.extract_tenant().unwrap(); assert_eq!(tenant, "tenant-id-123"); @@ -593,7 +593,7 @@ mod tests { "id".to_string(), "secret".to_string(), "https://login.microsoftonline.com/tenant-id-123/v2.0".to_string(), - vec![], + Vec::new(), ); // no token @@ -644,12 +644,12 @@ mod tests { display_name: "User 1".to_string(), mail: Some("email@email.com".to_string()), account_enabled: true, - other_mails: vec![], + other_mails: Vec::new(), id: "user1-id".into(), given_name: Some("User".into()), surname: Some("One".into()), mobile_phone: Some("555555555".into()), - business_phones: vec![], + business_phones: Vec::new(), }, User { display_name: "User 2".to_string(), @@ -660,18 +660,18 @@ mod tests { given_name: Some("User".into()), surname: Some("Two".into()), mobile_phone: None, - business_phones: vec![], + business_phones: Vec::new(), }, User { display_name: "User 3".to_string(), mail: None, account_enabled: true, - other_mails: vec![], + other_mails: Vec::new(), id: "user3-id".into(), given_name: Some("User".into()), surname: Some("Three".into()), mobile_phone: None, - business_phones: vec![], + business_phones: Vec::new(), }, ], }; @@ -691,12 +691,12 @@ mod tests { display_name: "User 1".to_string(), mail: Some("email@email.com".to_string()), account_enabled: true, - other_mails: vec![], + other_mails: Vec::new(), id: "user1-id".into(), given_name: Some("User".into()), surname: None, mobile_phone: None, - business_phones: vec![], + business_phones: Vec::new(), }, User { display_name: "User 2".to_string(), @@ -707,18 +707,18 @@ mod tests { given_name: None, surname: None, mobile_phone: Some("555555555".into()), - business_phones: vec![], + business_phones: Vec::new(), }, User { display_name: "User 3".to_string(), mail: None, account_enabled: true, - other_mails: vec![], + other_mails: Vec::new(), id: "user3-id".into(), given_name: Some("User".into()), surname: Some("Three".into()), mobile_phone: Some("555555555".into()), - business_phones: vec![], + business_phones: Vec::new(), }, ], }; diff --git a/crates/defguard_core/src/enterprise/directory_sync/tests.rs b/crates/defguard_core/src/enterprise/directory_sync/tests.rs index e06ca653b6..cdc6c8317b 100644 --- a/crates/defguard_core/src/enterprise/directory_sync/tests.rs +++ b/crates/defguard_core/src/enterprise/directory_sync/tests.rs @@ -90,7 +90,7 @@ mod test { target, None, None, - vec![], + Vec::new(), None, prefetch_users, ) diff --git a/crates/defguard_core/src/enterprise/ldap/client.rs b/crates/defguard_core/src/enterprise/ldap/client.rs index 35567bfd66..0526ade48d 100644 --- a/crates/defguard_core/src/enterprise/ldap/client.rs +++ b/crates/defguard_core/src/enterprise/ldap/client.rs @@ -70,7 +70,7 @@ impl super::LDAPConnection { Ok((mut rs, res)) => { debug!("LDAP search result: {res:?}"); if let Some(entry) = rs.pop() { - debug!("Found LDAP object with DN {dn}: {:?}", entry); + debug!("Found LDAP object with DN {dn}: {entry:?}"); Ok(Some(SearchEntry::construct(entry))) } else { debug!("No LDAP object found with DN {dn}"); @@ -78,14 +78,14 @@ impl super::LDAPConnection { } } // LDAP returns Error if no entries found, so we should handle it gracefully - Err(e) => { - debug!("LDAP search failed for DN {dn}: {e:?}"); + Err(err) => { + debug!("LDAP search failed for DN {dn}: {err:?}"); Ok(None) } }, - Err(e) => { - debug!("LDAP connection/search error for DN {dn}: {e:?}"); - Err(LdapError::from(e)) + Err(err) => { + debug!("LDAP connection/search error for DN {dn}: {err:?}"); + Err(LdapError::from(err)) } } } @@ -106,22 +106,19 @@ impl super::LDAPConnection { Ok(()) } - // Check what groups user is member of + /// Returns groups that the given user is a member of. pub(super) async fn get_user_groups( &mut self, user_dn: &str, ) -> Result, LdapError> { let user_dn_escaped = ldap_escape(user_dn); - let filter = format!( - "({}={})", - self.config.ldap_group_member_attr, user_dn_escaped - ); + let filter = format!("({}={user_dn_escaped})", self.config.ldap_group_member_attr); let (rs, res) = self .ldap .search( &self.config.ldap_group_search_base, Scope::Subtree, - filter.as_str(), + &filter, vec![&self.config.ldap_groupname_attr], ) .await? @@ -258,12 +255,10 @@ impl super::LDAPConnection { let user_dn_escaped = ldap_escape(user_dn); let groupname_escaped = ldap_escape(groupname); let filter = format!( - "(&(objectClass={})({}={})({}={}))", + "(&(objectClass={})({}={groupname_escaped})({}={user_dn_escaped}))", self.config.ldap_group_obj_class, self.config.ldap_groupname_attr, - groupname_escaped, self.config.ldap_group_member_attr, - user_dn_escaped ); debug!( "Using the following filter for group search: {filter} and base: {}", @@ -287,7 +282,7 @@ impl super::LDAPConnection { &mut self, groupname: &str, ) -> Result, LdapError> { - debug!("Searching for group memberships for group {}", groupname); + debug!("Searching for group memberships for group {groupname}"); let groupname_escaped = ldap_escape(groupname); let filter = format!( "(&(objectClass={})({}={}))", @@ -324,10 +319,7 @@ impl super::LDAPConnection { }) }) .unwrap_or_default(); - debug!( - "Performed LDAP group memberships search for group {}", - groupname - ); + debug!("Performed LDAP group memberships search for group {groupname}"); Ok(members) } @@ -341,7 +333,7 @@ impl super::LDAPConnection { "LDAP sync groups are defined, filtering users by those groups: {:?}", self.config.ldap_sync_groups ); - let mut group_filters = vec![]; + let mut group_filters = Vec::new(); for group in &self.config.ldap_sync_groups { let group_dn = self.config.group_dn(group); let group_dn_escaped = ldap_escape(&group_dn); @@ -350,10 +342,7 @@ impl super::LDAPConnection { self.config.ldap_member_attr, group_dn_escaped )); } - debug!( - "Using the following group filters for user search: {:?}", - group_filters - ); + debug!("Using the following group filters for user search: {group_filters:?}"); format!( "(&(objectClass={})(|{}))", self.config.ldap_user_obj_class, @@ -377,7 +366,7 @@ impl super::LDAPConnection { ) .await?; - let mut entries = vec![]; + let mut entries = Vec::new(); while let Some(entry) = search_stream.next().await? { entries.push(SearchEntry::construct(entry)); } diff --git a/crates/defguard_core/src/enterprise/ldap/mod.rs b/crates/defguard_core/src/enterprise/ldap/mod.rs index 699b476d91..b4f48acc46 100644 --- a/crates/defguard_core/src/enterprise/ldap/mod.rs +++ b/crates/defguard_core/src/enterprise/ldap/mod.rs @@ -36,21 +36,6 @@ pub mod sync; pub mod test_client; pub mod utils; -#[cfg(test)] -fn set_test_license_business() { - use crate::enterprise::license::set_cached_license; - - let license = crate::enterprise::license::License { - customer_id: "0c4dcb5400544d47ad8617fcdf2704cb".into(), - limits: None, - subscription: false, - tier: crate::enterprise::license::LicenseTier::Enterprise, - valid_until: None, - version_date_limit: None, - }; - set_cached_license(Some(license)); -} - /// Performs LDAP synchronization if enabled and enterprise features are available. /// /// This function may trigger either full and incremental sync based on the current sync status. @@ -196,7 +181,7 @@ impl Default for LDAPConfig { ldap_groupname_attr: "cn".to_string(), ldap_group_member_attr: "uniqueMember".to_string(), ldap_member_attr: "memberOf".to_string(), - ldap_user_auxiliary_obj_classes: vec![], + ldap_user_auxiliary_obj_classes: Vec::new(), ldap_uses_ad: false, ldap_user_rdn_attr: None, ldap_sync_groups: Vec::new(), @@ -424,13 +409,14 @@ impl LDAPConnection { } /// Checks if user belongs to one of the defined sync groups in the LDAP server. - /// Returns true if no sync groups are defined (sync all users) or if user is in at least one sync group. + /// Returns `true` if no sync groups are defined (sync all users) or if user is in at least one + /// sync group. async fn user_in_ldap_sync_groups(&mut self, user: &User) -> Result { debug!("Checking if user {user} is in LDAP sync groups"); - // Sync groups empty, we should sync all users + // Sync groups are empty, we should sync all users. if self.config.ldap_sync_groups.is_empty() { - debug!("Sync groups were not defined, user {user} will be synced"); + debug!("Sync groups are not defined, user {user} will be synced"); return Ok(true); } diff --git a/crates/defguard_core/src/enterprise/ldap/model.rs b/crates/defguard_core/src/enterprise/ldap/model.rs index 4fb08741fe..3aead41ff8 100644 --- a/crates/defguard_core/src/enterprise/ldap/model.rs +++ b/crates/defguard_core/src/enterprise/ldap/model.rs @@ -203,7 +203,7 @@ pub fn user_as_ldap_attrs<'a, I>( username_attr: &'a str, rdn_attr: &'a str, ) -> Vec<(&'a str, HashSet<&'a str>)> { - let mut attrs = vec![]; + let mut attrs = Vec::new(); attrs.push((rdn_attr, hashset![user.ldap_rdn_value()])); if object_classes.contains(UserObjectClass::InetOrgPerson.into()) || object_classes.contains(UserObjectClass::User.into()) @@ -379,7 +379,7 @@ mod tests { assert!(!in_attrs(&attrs, "uid")); // Test empty attributes vector - let empty_attrs = vec![]; + let empty_attrs = Vec::new(); assert!(!in_attrs(&empty_attrs, "cn")); assert!(!in_attrs(&empty_attrs, "any")); diff --git a/crates/defguard_core/src/enterprise/ldap/sync.rs b/crates/defguard_core/src/enterprise/ldap/sync.rs index cf99165afd..f571154fb3 100644 --- a/crates/defguard_core/src/enterprise/ldap/sync.rs +++ b/crates/defguard_core/src/enterprise/ldap/sync.rs @@ -406,8 +406,8 @@ pub(super) fn extract_intersecting_users( ldap_users: &mut Vec, ldap_config: &LDAPConfig, ) -> Vec<(User, User)> { - let mut intersecting_users = vec![]; - let mut intersecting_users_ldap = vec![]; + let mut intersecting_users = Vec::new(); + let mut intersecting_users_ldap = Vec::new(); for defguard_user in defguard_users.iter() { if let Some(ldap_user) = ldap_users diff --git a/crates/defguard_core/src/enterprise/ldap/test_client.rs b/crates/defguard_core/src/enterprise/ldap/test_client.rs index 907a6bd4df..de870421b5 100644 --- a/crates/defguard_core/src/enterprise/ldap/test_client.rs +++ b/crates/defguard_core/src/enterprise/ldap/test_client.rs @@ -163,8 +163,9 @@ impl Object { /// LDAP operations don't actually modify any data, but emit corresponding events that may /// be verified using the `events_match` method. /// -/// To modify (setup) the mock data, use the `add_test_user`, `add_test_group`, and `add_test_membership` methods. -#[derive(Debug, Default)] +/// To modify (setup) the mock data, use the `add_test_user`, `add_test_group`, and +/// `add_test_membership` methods. +#[derive(Default)] pub struct TestClient { events: Vec, // DN: Object @@ -374,7 +375,7 @@ impl super::LDAPConnection { return Ok(groups); } - Ok(vec![]) + Ok(Vec::new()) } pub(super) async fn add( diff --git a/crates/defguard_core/src/enterprise/ldap/tests.rs b/crates/defguard_core/src/enterprise/ldap/tests.rs index b00e84076a..aff782d1ba 100644 --- a/crates/defguard_core/src/enterprise/ldap/tests.rs +++ b/crates/defguard_core/src/enterprise/ldap/tests.rs @@ -5,16 +5,16 @@ use ldap3::SearchEntry; use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; use super::*; +use super::{ + model::{extract_rdn_value, get_users_without_ldap_path, user_from_searchentry}, + sync::{ + Authority, compute_group_sync_changes, compute_user_sync_changes, + extract_intersecting_users, + }, + test_client::{LdapEvent, group_to_test_attrs, user_to_test_attrs}, +}; use crate::{ enterprise::{ - ldap::{ - model::{extract_rdn_value, get_users_without_ldap_path, user_from_searchentry}, - sync::{ - Authority, compute_group_sync_changes, compute_user_sync_changes, - extract_intersecting_users, - }, - test_client::{LdapEvent, group_to_test_attrs, user_to_test_attrs}, - }, license::{License, LicenseTier, set_cached_license}, limits::get_counts, }, @@ -41,6 +41,18 @@ fn make_test_user( user } +fn set_test_license_business() { + let license = License { + customer_id: "0c4dcb5400544d47ad8617fcdf2704cb".into(), + limits: None, + subscription: false, + tier: LicenseTier::Enterprise, + valid_until: None, + version_date_limit: None, + }; + set_cached_license(Some(license)); +} + #[test] fn test_get_rdn_attr() { // Default configuration should use 'cn' as the RDN attribute @@ -164,12 +176,12 @@ fn test_get_all_user_obj_classes() { }; let obj_classes = config.get_all_user_obj_classes(); assert_eq!(obj_classes.len(), 2); - assert!(obj_classes.contains(&"inetOrgPerson".to_string())); - assert!(obj_classes.contains(&"simpleSecurityObject".to_string())); + assert!(obj_classes.iter().any(|e| e == "inetOrgPerson")); + assert!(obj_classes.iter().any(|e| e == "simpleSecurityObject")); // Should always include the base object class even with no auxiliaries let config = LDAPConfig { - ldap_user_auxiliary_obj_classes: vec![], + ldap_user_auxiliary_obj_classes: Vec::new(), ..LDAPConfig::default() }; let obj_classes = config.get_all_user_obj_classes(); @@ -183,8 +195,8 @@ fn test_get_all_user_obj_classes() { }; let obj_classes = config.get_all_user_obj_classes(); assert_eq!(obj_classes.len(), 2); - assert!(obj_classes.contains(&"inetOrgPerson".to_string())); - assert!(obj_classes.contains(&"customUser".to_string())); + assert!(obj_classes.iter().any(|e| e == "inetOrgPerson")); + assert!(obj_classes.iter().any(|e| e == "customUser")); // Multiple auxiliary classes let config = LDAPConfig { @@ -197,10 +209,10 @@ fn test_get_all_user_obj_classes() { }; let obj_classes = config.get_all_user_obj_classes(); assert_eq!(obj_classes.len(), 4); - assert!(obj_classes.contains(&"inetOrgPerson".to_string())); - assert!(obj_classes.contains(&"posixAccount".to_string())); - assert!(obj_classes.contains(&"mailUser".to_string())); - assert!(obj_classes.contains(&"customAttribute".to_string())); + assert!(obj_classes.iter().any(|e| e == "inetOrgPerson")); + assert!(obj_classes.iter().any(|e| e == "posixAccount")); + assert!(obj_classes.iter().any(|e| e == "mailUser")); + assert!(obj_classes.iter().any(|e| e == "customAttribute")); } #[test] @@ -680,8 +692,8 @@ async fn test_user_in_ldap_sync_groups() { #[test] fn test_compute_user_sync_changes_empty_lists() { - let mut ldap_users: Vec = vec![]; - let mut defguard_users: Vec> = vec![]; + let mut ldap_users: Vec = Vec::new(); + let mut defguard_users: Vec> = Vec::new(); let changes = compute_user_sync_changes( &mut ldap_users, @@ -709,7 +721,7 @@ fn test_ldap_authority_add_to_defguard() { ); let mut ldap_users = vec![ldap_user]; - let mut defguard_users: Vec> = vec![]; + let mut defguard_users: Vec> = Vec::new(); let changes = compute_user_sync_changes( &mut ldap_users, @@ -742,7 +754,7 @@ fn test_ldap_authority_delete_from_defguard(_: PgPoolOptions, options: PgConnect .await .unwrap(); - let mut ldap_users: Vec = vec![]; + let mut ldap_users: Vec = Vec::new(); let mut defguard_users = vec![defguard_user]; let changes = compute_user_sync_changes( @@ -776,7 +788,7 @@ fn test_defguard_authority_add_to_ldap(_: PgPoolOptions, options: PgConnectOptio .await .unwrap(); - let mut ldap_users: Vec = vec![]; + let mut ldap_users: Vec = Vec::new(); let mut defguard_users = vec![defguard_user]; let changes = compute_user_sync_changes( @@ -806,7 +818,7 @@ fn test_defguard_authority_delete_from_ldap() { ); let mut ldap_users = vec![ldap_user]; - let mut defguard_users: Vec> = vec![]; + let mut defguard_users: Vec> = Vec::new(); let changes = compute_user_sync_changes( &mut ldap_users, @@ -2431,12 +2443,12 @@ async fn test_sync_group_membership_with_intersecting_users( ); let user1_groups = updated_user1.member_of_names(&pool).await.unwrap(); - assert!(user1_groups.contains(&"engineering".to_string())); - assert!(user1_groups.contains(&"management".to_string())); // Added from LDAP + assert!(user1_groups.iter().any(|e| e == "engineering")); + assert!(user1_groups.iter().any(|e| e == "management")); // Added from LDAP let user2_groups = updated_user2.member_of_names(&pool).await.unwrap(); - assert!(user2_groups.contains(&"management".to_string())); - assert!(!user2_groups.contains(&"engineering".to_string())); // Removed from LDAP + assert!(user2_groups.iter().any(|e| e == "management")); + assert!(!user2_groups.iter().any(|e| e == "engineering")); // Removed from LDAP assert!(ldap_conn.test_client.get_events().is_empty()); } @@ -2707,7 +2719,7 @@ fn test_from_searchentry() { // empty attribute values { let mut attrs = HashMap::new(); - attrs.insert("sn".to_string(), vec![]); + attrs.insert("sn".to_string(), Vec::new()); attrs.insert("givenName".to_string(), vec!["firstname1".to_string()]); attrs.insert("mail".to_string(), vec!["user1@example.com".to_string()]); diff --git a/crates/defguard_core/src/handlers/network_devices.rs b/crates/defguard_core/src/handlers/network_devices.rs index 06bc6e98ea..f7294732eb 100644 --- a/crates/defguard_core/src/handlers/network_devices.rs +++ b/crates/defguard_core/src/handlers/network_devices.rs @@ -177,7 +177,7 @@ pub(crate) async fn list_network_devices( State(appstate): State, ) -> ApiResult { debug!("Listing all network devices"); - let mut devices_response: Vec = vec![]; + let mut devices_response: Vec = Vec::new(); let mut transaction = appstate.pool.begin().await?; let devices = Device::find_by_type(&mut *transaction, DeviceType::Network).await?; for device in devices { diff --git a/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs b/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs index 9fb3b58fe7..7c1eba543b 100644 --- a/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs +++ b/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs @@ -626,7 +626,7 @@ async fn test_modify_user(_: PgPoolOptions, options: PgConnectOptions) { // remove user from allowed group let mut user_details = fetch_user_details(&client, "hpotter").await; - user_details.user.groups = vec![]; + user_details.user.groups = Vec::new(); let response = client .put("/api/v1/user/hpotter") .json(&user_details.user) @@ -646,7 +646,7 @@ async fn test_modify_user(_: PgPoolOptions, options: PgConnectOptions) { // remove user from unrelated group let mut user_details = fetch_user_details(&client, "ssnape").await; - user_details.user.groups = vec![]; + user_details.user.groups = Vec::new(); let response = client .put("/api/v1/user/ssnape") .json(&user_details.user) diff --git a/crates/defguard_setup/src/auto_adoption.rs b/crates/defguard_setup/src/auto_adoption.rs index 8f53cdcafb..66ce592c29 100644 --- a/crates/defguard_setup/src/auto_adoption.rs +++ b/crates/defguard_setup/src/auto_adoption.rs @@ -679,7 +679,7 @@ id={} for new gateway", None, DEFAULT_WIREGUARD_MTU, 0, - vec![], + Vec::new(), DEFAULT_KEEPALIVE_INTERVAL, DEFAULT_DISCONNECT_THRESHOLD, false, From 731eb1e4f0ef4d5b6fb2a91a7887ebf3a9ea1807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 5 Mar 2026 13:54:45 +0100 Subject: [PATCH 3/9] get_user_groups returns Vec --- .../src/enterprise/ldap/client.rs | 21 +++-- .../defguard_core/src/enterprise/ldap/mod.rs | 81 +++++++------------ .../defguard_core/src/enterprise/ldap/sync.rs | 13 +-- .../src/enterprise/ldap/test_client.rs | 26 +++--- 4 files changed, 57 insertions(+), 84 deletions(-) diff --git a/crates/defguard_core/src/enterprise/ldap/client.rs b/crates/defguard_core/src/enterprise/ldap/client.rs index 0526ade48d..dc9500250b 100644 --- a/crates/defguard_core/src/enterprise/ldap/client.rs +++ b/crates/defguard_core/src/enterprise/ldap/client.rs @@ -110,10 +110,10 @@ impl super::LDAPConnection { pub(super) async fn get_user_groups( &mut self, user_dn: &str, - ) -> Result, LdapError> { + ) -> Result, LdapError> { let user_dn_escaped = ldap_escape(user_dn); let filter = format!("({}={user_dn_escaped})", self.config.ldap_group_member_attr); - let (rs, res) = self + let (entries, result) = self .ldap .search( &self.config.ldap_group_search_base, @@ -123,10 +123,21 @@ impl super::LDAPConnection { ) .await? .success()?; - debug!("LDAP user groups search result: {res}"); + debug!("LDAP user groups search result: {result}"); debug!("Performed LDAP group search with filter = {filter}"); - debug!("Found groups: {rs:?}"); - Ok(rs.into_iter().map(SearchEntry::construct).collect()) + debug!("Found groups: {entries:?}"); + + let mut groups = Vec::new(); + for entry in entries { + let se = SearchEntry::construct(entry); + for (key, mut values) in se.attrs { + if key.eq_ignore_ascii_case(&self.config.ldap_groupname_attr) { + groups.append(&mut values); + } + } + } + + Ok(groups) } /// Searches LDAP for groups. diff --git a/crates/defguard_core/src/enterprise/ldap/mod.rs b/crates/defguard_core/src/enterprise/ldap/mod.rs index b4f48acc46..a7aaeeb3a4 100644 --- a/crates/defguard_core/src/enterprise/ldap/mod.rs +++ b/crates/defguard_core/src/enterprise/ldap/mod.rs @@ -10,7 +10,7 @@ use defguard_common::db::{ }; #[cfg(not(test))] use ldap3::Ldap; -use ldap3::{Mod, SearchEntry, ldap_escape}; +use ldap3::{Mod, ldap_escape}; use model::UserObjectClass; use rand::Rng; use sqlx::PgPool; @@ -61,7 +61,8 @@ pub(crate) async fn do_ldap_sync(pool: &PgPool) -> Result<(), LdapError> { if !is_business_license_active() { info!( - "Enterprise features are disabled, not performing LDAP sync and automatically disabling it" + "Enterprise features are disabled, not performing LDAP sync and automatically \ + disabling it" ); settings.ldap_sync_enabled = false; update_current_settings(pool, settings).await?; @@ -351,10 +352,12 @@ pub struct LDAPConnection { impl LDAPConnection { /// Updates user state in LDAP based on the following rules: - /// - If the user is disabled in Defguard, he will be removed from LDAP - /// - If there are no sync groups defined or the user is in them but doesn't exist yet in LDAP, he will be added to LDAP and assigned to his groups + /// - If the user is disabled in Defguard, he will be removed from LDAP. + /// - If there are no sync groups defined, or the user is in them but doesn't exist yet in LDAP, + /// the user will be added to LDAP and assigned to its groups. /// - /// Make sure to call this every time one of the above conditions changes (e.g. group addition, user disabling) + /// Make sure to call this every time one of the above conditions changes (e.g. group addition, + /// user disabling). pub(crate) async fn update_users_state( &mut self, users: Vec<&mut User>, @@ -394,8 +397,8 @@ impl LDAPConnection { continue; } - // We may bring user into the synchronization scope, sync his data (email, groups, etc.) based on - // the authority + // We may bring user into the synchronization scope, sync his data (email, groups, etc.) + // based on the authority. if user_exists_in_ldap { debug!( "User {user} is in LDAP and is allowed to be synced, synchronizing his data" @@ -427,25 +430,15 @@ impl LDAPConnection { return Ok(false); } - let user_groups_entries = self.get_user_groups(&dn).await?; - let user_groups_names = user_groups_entries - .iter() - .filter_map(|entry| { - entry - .attrs - .get(&self.config.ldap_groupname_attr) - .and_then(|v| v.first()) - }) - .collect::>(); - + let user_groups = self.get_user_groups(&dn).await?; debug!( - "User groups: {user_groups_names:?}, sync groups: {:?}", + "User groups: {user_groups:?}, sync groups: {:?}", self.config.ldap_sync_groups ); - if user_groups_names + if user_groups .into_iter() - .any(|group| self.config.ldap_sync_groups.contains(group)) + .any(|group| self.config.ldap_sync_groups.contains(&group)) { debug!("User {user} is in sync groups, syncing user"); Ok(true) @@ -668,7 +661,8 @@ impl LDAPConnection { user: &User, ) -> Result<(), LdapError> { debug!("Modifying user {old_username} in LDAP"); - // If we're using the username as the RDN, also update the RDN value on user if his username has been changed + // If we're using the username as the RDN, also update the RDN value on user if his username + // has been changed. let old_rdn = if self.config.using_username_as_rdn() { old_username } else { @@ -698,21 +692,6 @@ impl LDAPConnection { Ok(()) } - /// Extracts group name from LDAP group search entry. - /// Returns an error if the group name attribute is not found. - fn group_entry_to_name(&self, entry: SearchEntry) -> Result { - entry - .attrs - .get(&self.config.ldap_groupname_attr) - .and_then(|v| v.first()) - .map(ToString::to_string) - .ok_or_else(|| { - LdapError::ObjectNotFound(format!( - "Couldn't extract a group name from searchentry {entry:?}." - )) - }) - } - /// Deletes user from LDAP. /// First removes the user from all group memberships (if any), then deletes the user entry. pub async fn delete_user(&mut self, user: &User) -> Result<(), LdapError> { @@ -721,17 +700,10 @@ impl LDAPConnection { debug!("Removing group memberships first..."); let user_groups = self.get_user_groups(&dn).await?; debug!("Removing user from groups: {user_groups:?}"); - for group in user_groups { - debug!("Removing user from group {group:?}"); - match self.group_entry_to_name(group) { - Ok(groupname) => { - self.remove_user_from_group(user, &groupname).await?; - debug!("Removed user from group {groupname}"); - } - Err(e) => { - warn!("Failed to remove user from group: {e}"); - } - } + for groupname in user_groups { + debug!("Removing user from group {groupname:?}"); + self.remove_user_from_group(user, &groupname).await?; + debug!("Removed user from group {groupname}"); } self.delete(&dn).await?; info!("Deleted user {user}"); @@ -740,7 +712,8 @@ impl LDAPConnection { } /// Activates an Active Directory user account. - /// Sets userAccountControl to enable the account and pwdLastSet to avoid password change requirement. + /// Sets userAccountControl to enable the account and pwdLastSet to avoid password change + /// requirement. pub async fn activate_ad_user(&mut self, user_dn: &str) -> Result<(), LdapError> { debug!("Activating user {user_dn}"); self.modify( @@ -760,7 +733,8 @@ impl LDAPConnection { } /// Changes user password in LDAP. - /// Handles both Active Directory (unicodePwd) and standard LDAP (userPassword/sambaNTPassword) formats. + /// Handles both Active Directory (unicodePwd) and standard LDAP (userPassword/sambaNTPassword) + /// formats. pub async fn set_password( &mut self, user: &User, @@ -807,7 +781,8 @@ impl LDAPConnection { if mods.is_empty() { return Err(LdapError::MissingSettings(format!( - "Can't set password as no password object class has been defined for the user {user}." + "Can't set password as no password object class has been defined for user \ + {user}." ))); } @@ -906,9 +881,7 @@ impl LDAPConnection { user: &User, groupname: &str, ) -> Result<(), LdapError> { - debug!( - "Adding user {user} to group {groupname} in LDAP, checking if that group exists first..." - ); + debug!("Adding user {user} to group {groupname} in LDAP, checking if that group exists..."); let user_dn = self.config.user_dn_from_user(user); if self.is_member_of(&user_dn, groupname).await? { debug!("User {user} is already a member of group {groupname}, skipping"); diff --git a/crates/defguard_core/src/enterprise/ldap/sync.rs b/crates/defguard_core/src/enterprise/ldap/sync.rs index f571154fb3..a0fb6e169f 100644 --- a/crates/defguard_core/src/enterprise/ldap/sync.rs +++ b/crates/defguard_core/src/enterprise/ldap/sync.rs @@ -500,18 +500,7 @@ impl super::LDAPConnection { let user_dn = self.config.user_dn_from_user(user); let ldap_user = self.get_user_by_dn(user).await?; let defguard_groups = user.member_of_names(pool).await?; - let mut ldap_groups = Vec::new(); - for group_entry in self.get_user_groups(&user_dn).await? { - match self.group_entry_to_name(group_entry) { - Ok(group_name) => ldap_groups.push(group_name), - Err(err) => { - warn!( - "Failed to convert group entry to name during user synchronization: \ - {err}. This group will be skipped" - ); - } - } - } + let ldap_groups = self.get_user_groups(&user_dn).await?; debug!("User {user} is a member of the following groups in Defguard: {defguard_groups:?}"); debug!("User {user} is a member of the following groups in LDAP: {ldap_groups:?}"); diff --git a/crates/defguard_core/src/enterprise/ldap/test_client.rs b/crates/defguard_core/src/enterprise/ldap/test_client.rs index de870421b5..3922c562a5 100644 --- a/crates/defguard_core/src/enterprise/ldap/test_client.rs +++ b/crates/defguard_core/src/enterprise/ldap/test_client.rs @@ -127,11 +127,9 @@ impl PartialEq for LdapEvent { } } -#[derive(Debug, Clone, PartialEq)] -#[allow(clippy::large_enum_variant)] pub(super) enum Object { - User(User), - Group(Group), + User(Box), + Group(Box), } impl Object { @@ -196,7 +194,8 @@ impl TestClient { pub(super) fn add_test_user(&mut self, user: &User, config: &super::LDAPConfig) { let dn = config.user_dn_from_user(user); - self.objects.insert(dn, Object::User(user.clone())); + self.objects + .insert(dn, Object::User(Box::new(user.clone()))); } pub(super) fn remove_test_user(&mut self, user: &User, config: &super::LDAPConfig) { @@ -206,7 +205,8 @@ impl TestClient { pub(super) fn add_test_group(&mut self, group: &Group, config: &super::LDAPConfig) { let dn = config.group_dn(&group.name); - self.objects.insert(dn, Object::Group(group.clone())); + self.objects + .insert(dn, Object::Group(Box::new(group.clone()))); } pub(super) fn add_test_membership( @@ -351,7 +351,9 @@ impl super::LDAPConnection { pub(super) async fn get_user_groups( &mut self, user_dn: &str, - ) -> Result, LdapError> { + ) -> Result, LdapError> { + let mut groups = Vec::new(); + if let Some(Object::User(_)) = self.test_client.objects.get(user_dn) { let group_dns = self .test_client @@ -366,16 +368,14 @@ impl super::LDAPConnection { }) .collect::>(); - let mut groups = Vec::new(); for group_dn in group_dns { - if let Some(group_object) = self.test_client.objects.get(group_dn) { - groups.push(group_object.to_search_entry(group_dn, &self.config)); + if let Some(Object::Group(group)) = self.test_client.objects.get(group_dn) { + groups.push(group.name.clone()); } } - - return Ok(groups); } - Ok(Vec::new()) + + Ok(groups) } pub(super) async fn add( From 10d13f8aff92c46c4ec1feeb5d4ba5fea7bc9122 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 5 Mar 2026 14:41:16 +0100 Subject: [PATCH 4/9] Refactor UserObjectClass --- .../src/enterprise/ldap/client.rs | 8 ++-- .../defguard_core/src/enterprise/ldap/mod.rs | 8 ++-- .../src/enterprise/ldap/model.rs | 45 ++++++++----------- .../src/enterprise/ldap/test_client.rs | 37 +++++++-------- .../src/enterprise/ldap/tests.rs | 14 +++--- 5 files changed, 50 insertions(+), 62 deletions(-) diff --git a/crates/defguard_core/src/enterprise/ldap/client.rs b/crates/defguard_core/src/enterprise/ldap/client.rs index dc9500250b..eb5fdd3661 100644 --- a/crates/defguard_core/src/enterprise/ldap/client.rs +++ b/crates/defguard_core/src/enterprise/ldap/client.rs @@ -10,13 +10,13 @@ use ldap3::{ ldap_escape, }; -use super::error::LdapError; +use super::{LDAPConfig, LDAPConnection, error::LdapError}; use crate::enterprise::ldap::model::extract_rdn_value; -impl super::LDAPConnection { - pub(crate) async fn create() -> Result { +impl LDAPConnection { + pub(crate) async fn create() -> Result { let settings = Settings::get_current_settings(); - let config = super::LDAPConfig::try_from(settings.clone())?; + let config = LDAPConfig::try_from(settings.clone())?; let url = settings.ldap_url.ok_or(LdapError::MissingSettings( "LDAP URL is required for LDAP configuration to work".to_string(), ))?; diff --git a/crates/defguard_core/src/enterprise/ldap/mod.rs b/crates/defguard_core/src/enterprise/ldap/mod.rs index a7aaeeb3a4..0b98ea2875 100644 --- a/crates/defguard_core/src/enterprise/ldap/mod.rs +++ b/crates/defguard_core/src/enterprise/ldap/mod.rs @@ -566,7 +566,7 @@ impl LDAPConnection { user_from_searchentry(&entry, username, None) } else { Err(LdapError::ObjectNotFound(format!( - "User {username} not found", + "User {username} not found" ))) } } @@ -760,7 +760,8 @@ impl LDAPConnection { if self .config .ldap_user_auxiliary_obj_classes - .contains(&UserObjectClass::SimpleSecurityObject.into()) + .iter() + .any(|e| e == UserObjectClass::SimpleSecurityObject.name()) { mods.push(Mod::Replace( "userPassword", @@ -771,7 +772,8 @@ impl LDAPConnection { if self .config .ldap_user_auxiliary_obj_classes - .contains(&UserObjectClass::SambaSamAccount.into()) + .iter() + .any(|e| e == UserObjectClass::SambaSamAccount.name()) { mods.push(Mod::Replace( "sambaNTPassword", diff --git a/crates/defguard_core/src/enterprise/ldap/model.rs b/crates/defguard_core/src/enterprise/ldap/model.rs index 3aead41ff8..0ee580318b 100644 --- a/crates/defguard_core/src/enterprise/ldap/model.rs +++ b/crates/defguard_core/src/enterprise/ldap/model.rs @@ -17,33 +17,20 @@ pub(crate) enum UserObjectClass { User, } -impl<'a> From<&'a UserObjectClass> for &'static str { - fn from(obj_class: &'a UserObjectClass) -> &'static str { - match obj_class { - UserObjectClass::SambaSamAccount => "sambaSamAccount", - UserObjectClass::InetOrgPerson => "inetOrgPerson", - UserObjectClass::SimpleSecurityObject => "simpleSecurityObject", - UserObjectClass::User => "user", +impl UserObjectClass { + pub(crate) fn name(&self) -> &'static str { + match self { + Self::SambaSamAccount => "sambaSamAccount", + Self::InetOrgPerson => "inetOrgPerson", + Self::SimpleSecurityObject => "simpleSecurityObject", + Self::User => "user", } } } -impl From for &'static str { - fn from(obj_class: UserObjectClass) -> &'static str { - (&obj_class).into() - } -} - -impl From for String { - fn from(obj_class: UserObjectClass) -> String { - let str: &str = obj_class.into(); - str.to_string() - } -} - impl PartialEq<&str> for UserObjectClass { fn eq(&self, other: &&str) -> bool { - let str: &str = self.into(); + let str: &str = self.name(); str == *other } } @@ -110,8 +97,12 @@ pub(crate) fn update_from_ldap_user(user: &mut User, ldap_user: &User, con pub fn user_as_ldap_mod(user: &User, config: &LDAPConfig) -> Vec> { let obj_classes = config.get_all_user_obj_classes(); let mut changes = Vec::new(); - if obj_classes.contains(&UserObjectClass::InetOrgPerson.into()) - || obj_classes.contains(&UserObjectClass::User.into()) + if obj_classes + .iter() + .any(|e| e == UserObjectClass::InetOrgPerson.name()) + || obj_classes + .iter() + .any(|e| e == UserObjectClass::User.name()) { changes.extend_from_slice(&[ Mod::Replace("sn".to_string(), hashset![user.last_name.clone()]), @@ -205,8 +196,8 @@ pub fn user_as_ldap_attrs<'a, I>( ) -> Vec<(&'a str, HashSet<&'a str>)> { let mut attrs = Vec::new(); attrs.push((rdn_attr, hashset![user.ldap_rdn_value()])); - if object_classes.contains(UserObjectClass::InetOrgPerson.into()) - || object_classes.contains(UserObjectClass::User.into()) + if object_classes.contains(UserObjectClass::InetOrgPerson.name()) + || object_classes.contains(UserObjectClass::User.name()) { attrs.extend_from_slice(&[ ("sn", hashset![user.last_name.as_str()]), @@ -228,11 +219,11 @@ pub fn user_as_ldap_attrs<'a, I>( } } } - if object_classes.contains(UserObjectClass::SimpleSecurityObject.into()) { + if object_classes.contains(UserObjectClass::SimpleSecurityObject.name()) { // simpleSecurityObject attrs.push(("userPassword", hashset![ssha_password])); } - if object_classes.contains(UserObjectClass::SambaSamAccount.into()) { + if object_classes.contains(UserObjectClass::SambaSamAccount.name()) { // sambaSamAccount attrs.push(("sambaSID", hashset!["0"])); attrs.push(("sambaNTPassword", hashset![nt_password])); diff --git a/crates/defguard_core/src/enterprise/ldap/test_client.rs b/crates/defguard_core/src/enterprise/ldap/test_client.rs index 3922c562a5..eaead30e1b 100644 --- a/crates/defguard_core/src/enterprise/ldap/test_client.rs +++ b/crates/defguard_core/src/enterprise/ldap/test_client.rs @@ -7,7 +7,7 @@ use std::{ use defguard_common::db::models::{User, group::Group}; use ldap3::{Mod, SearchEntry}; -use super::error::LdapError; +use super::{LDAPConfig, LDAPConnection, error::LdapError}; use crate::enterprise::ldap::model::{extract_rdn_value, user_as_ldap_attrs}; /// Extract attribute value from LDAP filter @@ -133,7 +133,7 @@ pub(super) enum Object { } impl Object { - fn to_search_entry(&self, dn: &str, config: &super::LDAPConfig) -> SearchEntry { + fn to_search_entry(&self, dn: &str, config: &LDAPConfig) -> SearchEntry { match self { Object::User(user) => SearchEntry { dn: dn.to_string(), @@ -192,29 +192,24 @@ impl TestClient { self.events.clear(); } - pub(super) fn add_test_user(&mut self, user: &User, config: &super::LDAPConfig) { + pub(super) fn add_test_user(&mut self, user: &User, config: &LDAPConfig) { let dn = config.user_dn_from_user(user); self.objects .insert(dn, Object::User(Box::new(user.clone()))); } - pub(super) fn remove_test_user(&mut self, user: &User, config: &super::LDAPConfig) { + pub(super) fn remove_test_user(&mut self, user: &User, config: &LDAPConfig) { let dn = config.user_dn_from_user(user); self.objects.remove(&dn); } - pub(super) fn add_test_group(&mut self, group: &Group, config: &super::LDAPConfig) { + pub(super) fn add_test_group(&mut self, group: &Group, config: &LDAPConfig) { let dn = config.group_dn(&group.name); self.objects .insert(dn, Object::Group(Box::new(group.clone()))); } - pub(super) fn add_test_membership( - &mut self, - group: &Group, - user: &User, - config: &super::LDAPConfig, - ) { + pub(super) fn add_test_membership(&mut self, group: &Group, user: &User, config: &LDAPConfig) { let group_dn = config.group_dn(&group.name); let user_dn = config.user_dn_from_user(user); self.memberships @@ -227,7 +222,7 @@ impl TestClient { &mut self, group: &Group, user: &User, - config: &super::LDAPConfig, + config: &LDAPConfig, ) { let group_dn = config.group_dn(&group.name); let user_dn = config.user_dn_from_user(user); @@ -244,10 +239,10 @@ impl TestClient { } } -impl super::LDAPConnection { - pub(crate) async fn create() -> Result { +impl LDAPConnection { + pub(crate) async fn create() -> Result { Ok(Self { - config: super::LDAPConfig::default(), + config: LDAPConfig::default(), url: String::new(), test_client: TestClient::default(), }) @@ -551,7 +546,7 @@ impl super::LDAPConnection { pub(super) fn user_to_test_attrs( user: &User, password: Option<&str>, - config: &super::LDAPConfig, + config: &LDAPConfig, ) -> Vec<(String, HashSet)> { let rdn_attr = config .ldap_user_rdn_attr @@ -590,7 +585,7 @@ pub(super) fn user_to_test_attrs( #[cfg(test)] pub(super) fn group_to_test_attrs( group: &Group, - config: &super::LDAPConfig, + config: &LDAPConfig, members: Option<&Vec<&User>>, ) -> Vec<(String, HashSet)> { use crate::hashset; @@ -618,11 +613,11 @@ pub(super) fn group_to_test_attrs( #[cfg(test)] mod tests { - use defguard_common::db::models::User; + use super::*; #[tokio::test] async fn test_search_users_by_username() { - let mut ldap_conn = super::super::LDAPConnection::create().await.unwrap(); + let mut ldap_conn = LDAPConnection::create().await.unwrap(); let config = ldap_conn.config.clone(); let test_user = User::new( @@ -648,7 +643,7 @@ mod tests { #[tokio::test] async fn test_search_users_compound_filter() { - let mut ldap_conn = super::super::LDAPConnection::create().await.unwrap(); + let mut ldap_conn = LDAPConnection::create().await.unwrap(); let config = ldap_conn.config.clone(); let test_user = User::new( @@ -674,7 +669,7 @@ mod tests { #[tokio::test] async fn test_search_users_no_match() { - let mut ldap_conn = super::super::LDAPConnection::create().await.unwrap(); + let mut ldap_conn = LDAPConnection::create().await.unwrap(); let config = ldap_conn.config.clone(); let test_user = User::new( diff --git a/crates/defguard_core/src/enterprise/ldap/tests.rs b/crates/defguard_core/src/enterprise/ldap/tests.rs index aff782d1ba..c81085a895 100644 --- a/crates/defguard_core/src/enterprise/ldap/tests.rs +++ b/crates/defguard_core/src/enterprise/ldap/tests.rs @@ -481,7 +481,7 @@ async fn test_get_user() { let mut ldap_conn = LDAPConnection::create().await.unwrap(); ldap_conn.config = LDAPConfig { - ldap_user_auxiliary_obj_classes: vec![UserObjectClass::InetOrgPerson.into()], + ldap_user_auxiliary_obj_classes: vec![UserObjectClass::InetOrgPerson.name().to_string()], ..ldap_conn.config }; @@ -2927,7 +2927,7 @@ fn test_as_ldap_attrs() { &user, "{SSHA}hashedpw", "NT_HASH", - hashset![UserObjectClass::InetOrgPerson.into()], + hashset![UserObjectClass::InetOrgPerson.name()], false, "uid", "cn", @@ -2945,7 +2945,7 @@ fn test_as_ldap_attrs() { &user, "{SSHA}hashedpw", "NT_HASH", - hashset![UserObjectClass::User.into()], + hashset![UserObjectClass::User.name()], true, "uid", "cn", @@ -2959,8 +2959,8 @@ fn test_as_ldap_attrs() { "{SSHA}hashedpw", "NT_HASH", hashset![ - UserObjectClass::SimpleSecurityObject.into(), - UserObjectClass::SambaSamAccount.into() + UserObjectClass::SimpleSecurityObject.name(), + UserObjectClass::SambaSamAccount.name() ], false, "uid", @@ -2976,7 +2976,7 @@ fn test_as_ldap_attrs() { &user, "{SSHA}hashedpw", "NT_HASH", - hashset![UserObjectClass::User.into()], + hashset![UserObjectClass::User.name()], false, "uid", "customRDN", @@ -2999,7 +2999,7 @@ fn test_as_ldap_attrs() { &user_no_phone, "{SSHA}hashedpw", "NT_HASH", - hashset![UserObjectClass::InetOrgPerson.into()], + hashset![UserObjectClass::InetOrgPerson.name()], false, "uid", "cn", From 21b58dd2c8831dfe426dfc8d3fceecc755447fd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Fri, 6 Mar 2026 09:48:37 +0100 Subject: [PATCH 5/9] More optimisations --- Cargo.lock | 8 +- .../src/enterprise/ldap/client.rs | 70 ++++---- .../defguard_core/src/enterprise/ldap/hash.rs | 13 +- .../defguard_core/src/enterprise/ldap/mod.rs | 19 +- .../src/enterprise/ldap/model.rs | 21 +-- .../defguard_core/src/enterprise/ldap/sync.rs | 167 +++++++++++------- .../src/enterprise/ldap/test_client.rs | 4 +- .../src/enterprise/ldap/tests.rs | 28 +-- crates/defguard_core/src/handlers/auth.rs | 4 +- crates/model_derive/src/lib.rs | 2 +- 10 files changed, 173 insertions(+), 163 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e9530c1277..5288a2c92d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6901,9 +6901,9 @@ checksum = "e2eebbbfe4093922c2b6734d7c679ebfebd704a0d7e56dfcb0d05818ce28977d" [[package]] name = "uuid" -version = "1.21.0" +version = "1.22.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b672338555252d43fd2240c714dc444b8c6fb0a5c5335e65a07bba7742735ddb" +checksum = "a68d3c8f01c0cfa54a75291d83601161799e4a89a39e0929f4b0354d88757a37" dependencies = [ "getrandom 0.4.2", "js-sys", @@ -7604,9 +7604,9 @@ checksum = "d6bbff5f0aada427a1e5a6da5f1f98158182f26556f345ac9e04d36d0ebed650" [[package]] name = "winnow" -version = "0.7.14" +version = "0.7.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5a5364e9d77fcdeeaa6062ced926ee3381faa2ee02d3eb83a5c27a8825540829" +checksum = "df79d97927682d2fd8adb29682d1140b343be4ac0f08fd68b7765d9c059d3945" dependencies = [ "memchr", ] diff --git a/crates/defguard_core/src/enterprise/ldap/client.rs b/crates/defguard_core/src/enterprise/ldap/client.rs index eb5fdd3661..9de76e355e 100644 --- a/crates/defguard_core/src/enterprise/ldap/client.rs +++ b/crates/defguard_core/src/enterprise/ldap/client.rs @@ -13,6 +13,8 @@ use ldap3::{ use super::{LDAPConfig, LDAPConnection, error::LdapError}; use crate::enterprise::ldap::model::extract_rdn_value; +const STREAMING_PAGE_SIZE: i32 = 500; + impl LDAPConnection { pub(crate) async fn create() -> Result { let settings = Settings::get_current_settings(); @@ -34,7 +36,7 @@ impl LDAPConnection { .await? .success()?; - Ok(Self { config, ldap, url }) + Ok(Self { config, url, ldap }) } /// Searches LDAP for users. @@ -42,34 +44,35 @@ impl LDAPConnection { &mut self, filter: &str, ) -> Result, LdapError> { - let (rs, res) = self + debug!("Performing LDAP user search with filter: {filter}"); + let (entries, result) = self .ldap .search( &self.config.ldap_user_search_base, Scope::Subtree, filter, - vec!["*", &self.config.ldap_member_attr], + &["*", &self.config.ldap_member_attr], ) .await? .success()?; - debug!("LDAP user search result: {res:?}"); - debug!("Performed LDAP user search with filter = {filter}"); + debug!("LDAP user search result: {result:?}"); + debug!("Found users: {entries:?}"); - Ok(rs.into_iter().map(SearchEntry::construct).collect()) + Ok(entries.into_iter().map(SearchEntry::construct).collect()) } pub(crate) async fn get(&mut self, dn: &str) -> Result, LdapError> { debug!("Searching for LDAP object with DN {dn}"); let search_result = self .ldap - .search(dn, Scope::Base, "(objectClass=*)", vec!["*"]) + .search(dn, Scope::Base, "(objectClass=*)", &["*"]) .await; match search_result { Ok(ldap_result) => match ldap_result.success() { - Ok((mut rs, res)) => { - debug!("LDAP search result: {res:?}"); - if let Some(entry) = rs.pop() { + Ok((mut entries, result)) => { + debug!("LDAP search result: {result:?}"); + if let Some(entry) = entries.pop() { debug!("Found LDAP object with DN {dn}: {entry:?}"); Ok(Some(SearchEntry::construct(entry))) } else { @@ -113,18 +116,18 @@ impl LDAPConnection { ) -> Result, LdapError> { let user_dn_escaped = ldap_escape(user_dn); let filter = format!("({}={user_dn_escaped})", self.config.ldap_group_member_attr); + debug!("Performing LDAP group search with filter: {filter}"); let (entries, result) = self .ldap .search( &self.config.ldap_group_search_base, Scope::Subtree, &filter, - vec![&self.config.ldap_groupname_attr], + &[&self.config.ldap_groupname_attr], ) .await? .success()?; debug!("LDAP user groups search result: {result}"); - debug!("Performed LDAP group search with filter = {filter}"); debug!("Found groups: {entries:?}"); let mut groups = Vec::new(); @@ -151,7 +154,7 @@ impl LDAPConnection { &self.config.ldap_group_search_base, Scope::Subtree, filter, - vec![ + &[ &self.config.ldap_username_attr, &self.config.ldap_group_member_attr, ], @@ -217,7 +220,7 @@ impl LDAPConnection { ) -> Result>, LdapError> { debug!("Retrieving LDAP group memberships"); let mut membership_entries = self.list_group_memberships().await?; - let mut memberships: HashMap> = HashMap::new(); + let mut memberships = HashMap::new(); // dn: user map let dn_map = all_ldap_users .iter() @@ -239,7 +242,8 @@ impl LDAPConnection { Some(*user) } else { debug!( - "LDAP group {groupname} contains member {v} that does not belong to the filtered LDAP users list, skipping" + "LDAP group {groupname} contains member {v} that does not \ + belong to the filtered LDAP users list; skipping" ); None } @@ -275,18 +279,18 @@ impl LDAPConnection { "Using the following filter for group search: {filter} and base: {}", self.config.ldap_group_search_base ); - let (rs, res) = self + let (entries, result) = self .ldap .search( &self.config.ldap_group_search_base, Scope::Subtree, - filter.as_str(), - vec!["*"], + &filter, + &["*"], ) .await? .success()?; - debug!("LDAP group membership search result: {res:?}"); - Ok(!rs.is_empty()) + debug!("LDAP group membership search result: {result:?}"); + Ok(!entries.is_empty()) } pub(super) async fn get_group_members( @@ -303,14 +307,15 @@ impl LDAPConnection { "Using the following filter for group search: {filter} and base: {}", self.config.ldap_group_search_base ); + let attrs = [&self.config.ldap_group_member_attr]; let mut search_stream = self .ldap .streaming_search_with( - PagedResults::new(500), + PagedResults::new(STREAMING_PAGE_SIZE), &self.config.ldap_group_search_base, Scope::Subtree, - filter.as_str(), - vec![&self.config.ldap_group_member_attr], + &filter, + &attrs, ) .await?; @@ -365,15 +370,15 @@ impl LDAPConnection { "Using the following filter for user search: {filter} and base: {}", self.config.ldap_user_search_base ); - + let attrs = ["*", &self.config.ldap_member_attr]; let mut search_stream = self .ldap .streaming_search_with( - PagedResults::new(500), + PagedResults::new(STREAMING_PAGE_SIZE), &self.config.ldap_user_search_base, Scope::Subtree, &filter, - vec!["*", &self.config.ldap_member_attr], + &attrs, ) .await?; @@ -397,17 +402,18 @@ impl LDAPConnection { "Using the following filter for group search: {filter} and base: {}", self.config.ldap_group_search_base ); + let attrs = [ + &self.config.ldap_groupname_attr, + &self.config.ldap_group_member_attr, + ]; let mut search_stream = self .ldap .streaming_search_with( - PagedResults::new(500), + PagedResults::new(STREAMING_PAGE_SIZE), &self.config.ldap_group_search_base, Scope::Subtree, - filter.as_str(), - vec![ - &self.config.ldap_groupname_attr, - &self.config.ldap_group_member_attr, - ], + &filter, + &attrs, ) .await?; diff --git a/crates/defguard_core/src/enterprise/ldap/hash.rs b/crates/defguard_core/src/enterprise/ldap/hash.rs index 86aec6fa7a..37a21c268e 100644 --- a/crates/defguard_core/src/enterprise/ldap/hash.rs +++ b/crates/defguard_core/src/enterprise/ldap/hash.rs @@ -1,4 +1,4 @@ -use base64::Engine; +use base64::{Engine, prelude::BASE64_STANDARD}; use defguard_common::hex::to_lower_hex; use md4::Md4; use rand::{RngCore, rngs::OsRng}; @@ -22,10 +22,7 @@ pub fn salted_sha1_hash(password: &str) -> String { #[allow(deprecated)] let checksum = checksum.concat(GenericArray::from(salt)); - format!( - "{{SSHA}}{}", - base64::prelude::BASE64_STANDARD.encode(checksum) - ) + format!("{{SSHA}}{}", BASE64_STANDARD.encode(checksum)) } /// Calculate Windows NT-HASH; used for `sambaNTPassword`. @@ -42,9 +39,7 @@ pub fn nthash(password: &str) -> String { #[must_use] pub fn unicode_pwd(password: &str) -> Vec { let quoted = format!("\"{password}\""); - let utf16_bytes: Vec = quoted.encode_utf16().flat_map(u16::to_le_bytes).collect(); - - utf16_bytes + quoted.encode_utf16().flat_map(u16::to_le_bytes).collect() } #[cfg(test)] @@ -54,7 +49,7 @@ mod tests { #[test] fn test_unicode_pwd() { let encoded = unicode_pwd("newPassword"); - let res = base64::prelude::BASE64_STANDARD.encode(encoded); + let res = BASE64_STANDARD.encode(encoded); assert_eq!(res, "IgBuAGUAdwBQAGEAcwBzAHcAbwByAGQAIgA="); } diff --git a/crates/defguard_core/src/enterprise/ldap/mod.rs b/crates/defguard_core/src/enterprise/ldap/mod.rs index 0b98ea2875..ea9af3c40d 100644 --- a/crates/defguard_core/src/enterprise/ldap/mod.rs +++ b/crates/defguard_core/src/enterprise/ldap/mod.rs @@ -336,17 +336,12 @@ impl TryFrom for LDAPConfig { } } -#[cfg(not(test))] -pub struct LDAPConnection { - pub config: LDAPConfig, - pub ldap: Ldap, - pub url: String, -} - -#[cfg(test)] pub struct LDAPConnection { pub config: LDAPConfig, pub url: String, + #[cfg(not(test))] + pub ldap: Ldap, + #[cfg(test)] pub test_client: test_client::TestClient, } @@ -810,7 +805,7 @@ impl LDAPConnection { ("objectClass", hashset![group_obj_class.as_str()]), (groupname_attr.as_str(), hashset![group_name]), ]; - // extent the group attr with multiple members + // Extend the group attr with multiple members. let member_dns = members .into_iter() .map(|member| self.config.user_dn_from_user(member)) @@ -828,11 +823,7 @@ impl LDAPConnection { self.add(&dn, group_attrs).await?; info!( "Added LDAP group {group_name} with members {}", - member_dns - .iter() - .map(String::as_str) - .collect::>() - .join(", ") + member_dns.join(", ") ); Ok(()) diff --git a/crates/defguard_core/src/enterprise/ldap/model.rs b/crates/defguard_core/src/enterprise/ldap/model.rs index 0ee580318b..cbb0ee9e40 100644 --- a/crates/defguard_core/src/enterprise/ldap/model.rs +++ b/crates/defguard_core/src/enterprise/ldap/model.rs @@ -28,19 +28,6 @@ impl UserObjectClass { } } -impl PartialEq<&str> for UserObjectClass { - fn eq(&self, other: &&str) -> bool { - let str: &str = self.name(); - str == *other - } -} - -impl PartialEq for &str { - fn eq(&self, other: &UserObjectClass) -> bool { - other == self - } -} - pub(crate) fn user_from_searchentry( entry: &SearchEntry, username: &str, @@ -68,8 +55,8 @@ pub(crate) fn user_from_searchentry( // Print the warning only if everything else checks out if check_username(username).is_err() { warn!( - "LDAP User \"{username}\" has username that cannot be used in Defguard, \ - change the LDAP username attribute or change the username in LDAP to a valid one", + "LDAP User \"{username}\" has username that cannot be used in Defguard; change the \ + LDAP username attribute or change the username in LDAP to a valid one" ); return Err(LdapError::InvalidUsername(username.to_string())); } @@ -94,7 +81,7 @@ pub(crate) fn update_from_ldap_user(user: &mut User, ldap_user: &User, con /// Return a vector of LDAP modifications for a given [`User`]. #[must_use] -pub fn user_as_ldap_mod(user: &User, config: &LDAPConfig) -> Vec> { +pub(crate) fn user_as_ldap_mod(user: &User, config: &LDAPConfig) -> Vec> { let obj_classes = config.get_all_user_obj_classes(); let mut changes = Vec::new(); if obj_classes @@ -185,7 +172,7 @@ fn in_attrs<'a>(attrs: &'a Vec<(&'a str, HashSet<&'a str>)>, key: &str) -> bool } #[must_use] -pub fn user_as_ldap_attrs<'a, I>( +pub(crate) fn user_as_ldap_attrs<'a, I>( user: &'a User, ssha_password: &'a str, nt_password: &'a str, diff --git a/crates/defguard_core/src/enterprise/ldap/sync.rs b/crates/defguard_core/src/enterprise/ldap/sync.rs index a0fb6e169f..aa8b39dc0b 100644 --- a/crates/defguard_core/src/enterprise/ldap/sync.rs +++ b/crates/defguard_core/src/enterprise/ldap/sync.rs @@ -5,52 +5,68 @@ //! //! # Sync status //! -//! The sync status is stored in the database and can be either `InSync` or `OutOfSync`. The status is used to determine -//! whether the full sync should be performed or not. The status is set to `OutOfSync` when some Defguard changes -//! couldn't be propagated to LDAP (e.g. LDAP outage). The status is set to `InSync` when the sync is completed successfully. +//! The sync status is stored in the database and can be either `InSync` or `OutOfSync`. The status +//! is used to determine whether the full sync should be performed or not. The status is set to +//! `OutOfSync` when some Defguard changes couldn't be propagated to LDAP (e.g. LDAP outage). The +//! status is set to `InSync` when the sync is completed successfully. //! //! # Full synchronization //! -//! The full synchronization takes all objects (users, groups and their memberships) from one source, -//! compares it with the other one and computes appropriate changes to make the two sources roughly equal. +//! The full synchronization takes all objects (users, groups and their memberships) from one +//! source, compares it with the other one and computes appropriate changes to make the two sources +//! roughly equal. //! //! The full sync is performed only when the sync status is set to `OutOfSync`. //! -//! The changes are computed with regard to a specified authority, which determines which source is considered to -//! be the more important one and which is expected to be edited more often. The authority can be either LDAP or Defguard. +//! The changes are computed with regard to a specified authority, which determines which source is +//! considered to be the more important one and which is expected to be edited more often. The +//! authority can be either LDAP or Defguard. //! -//! The authority has been introduced to solve the problem of ambiguity when some object is not present in one of the sources. -//! Such scenario may occur when a user is deleted from one of the sources OR when a user is added to one of the sources. -//! In each case, a different action should be taken to make the two sources equal (deletion or addition). For example: +//! The authority has been introduced to solve the problem of ambiguity when some object is not +//! present in one of the sources. Such scenario may occur when a user is deleted from one of the +//! sources OR when a user is added to one of the sources. In each case, a different action should +//! be taken to make the two sources equal (deletion or addition). For example: //! - User is not present in LDAP but is present in Defguard -//! - Did we just add the user to Defguard but couldn't propagate that change or did we delete the user from LDAP? -//! - If the authority is LDAP, we should delete the user from Defguard, as we assume that it was more probable that the change was made in LDAP. -//! - If the authority is Defguard, we should add the user to LDAP, as we assume that it was more probable that the change was made in Defguard. +//! - Did we just add the user to Defguard but couldn't propagate that change or did we delete the +//! user from LDAP? +//! - If the authority is LDAP, we should delete the user from Defguard, as we assume that it was +//! more probable that the change was made in LDAP. +//! - If the authority is Defguard, we should add the user to LDAP, as we assume that it was more +//! probable that the change was made in Defguard. //! -//! If the LDAP connection is never lost and no other issues arise, the full sync should be performed only once, when the LDAP sync is enabled. -//! So this is a more of a damage control mechanism rather than something that should be invoked regularly. +//! If the LDAP connection is never lost and no other issues arise, the full sync should be +//! performed only once, when the LDAP sync is enabled. So this is a more of a damage control +//! mechanism rather than something that should be invoked regularly. //! //! # Incremental synchronization //! -//! The incremental synchronization is a regular synchronization operation which comes in two varieties: synchronous and asynchronous. +//! The incremental synchronization is a regular synchronization operation which comes in two +//! varieties: synchronous and asynchronous. //! -//! Changes from Defguard are propagated to LDAP in real-time, synchronously, to keep LDAP up-to-date with Defguard instantly. This is done by -//! calling appropriate LDAP operations after each change in Defguard. Changes other way around (from LDAP to Defguard) are pulled asynchronously -//! at regular intervals (every 5 minutes by default). Implementation-wise it's done by running a full sync with LDAP authority, as it has the same effect -//! when we consider that LDAP has the most recent Defguard changes (due to synchronous change propagation). +//! Changes from Defguard are propagated to LDAP in real-time, synchronously, to keep LDAP +//! up-to-date with Defguard instantly. This is done by calling appropriate LDAP operations after +//! each change in Defguard. Changes other way around (from LDAP to Defguard) are pulled +//! asynchronously at regular intervals (every 5 minutes by default). Implementation-wise it's done +//! by running a full sync with LDAP authority, as it has the same effect when we consider that LDAP +//! has the most recent Defguard changes (due to synchronous change propagation). //! //! This synchronization should work reliably most of the time, given that: //! - LDAP connection is stable //! - The LDAP change pull is performed relatively often -//! - One object is not changed in both sources between two asynchronous syncs (may cause overwriting of changes), but this sounds like an unlikely scenario +//! - One object is not changed in both sources between two asynchronous syncs (may cause +//! overwriting of changes), but this sounds like an unlikely scenario //! //! # Potential improvements and issues //! -//! - Some optimizations could be made using the implementation-specific object modification/creation timestamps in LDAP. Currently everything is compared -//! as is, without any regard to the time of the change. We could skip some operations on objects that haven't changed since the last sync. There is however -//! still an issue with objects that have been deleted, LDAP doesn't store deleted objects by default, so we may still need to compare full object lists. -//! - There is no real pagination and everything is loaded into the memory at once. This may be an issue at some point. 10k LDAP records wasn't a problem in testing. -//! We may have bigger issues with other parts of Defguard with that user count though. +//! - Some optimizations could be made using the implementation-specific object +//! modification/creation timestamps in LDAP. Currently everything is compared as is, without any +//! regard to the time of the change. We could skip some operations on objects that haven't +//! changed since the last sync. There is however still an issue with objects that have been +//! deleted, LDAP doesn't store deleted objects by default, so we may still need to compare full +//! object lists. +//! - There is no real pagination and everything is loaded into the memory at once. This may be an +//! issue at some point. 10k LDAP records wasn't a problem in testing. We may have bigger issues +//! with other parts of Defguard with that user count though. //! use std::collections::{HashMap, HashSet}; @@ -147,8 +163,8 @@ pub(super) fn compute_user_sync_changes( .map(|u| ldap_config.user_dn_from_user(u)) .collect::>(); - trace!("Defguard identifiers: {:?}", defguard_identifiers); - trace!("LDAP identifiers: {:?}", ldap_identifiers); + trace!("Defguard identifiers: {defguard_identifiers:?}"); + trace!("LDAP identifiers: {ldap_identifiers:?}"); for user in all_ldap_users.drain(..) { ldap_identifiers.insert(ldap_config.user_dn_from_user(&user)); @@ -225,7 +241,7 @@ pub(super) struct GroupSyncChanges<'a> { /// Computes what groups should be added/deleted and where pub(super) fn compute_group_sync_changes<'a>( - defguard_memberships: HashMap>>, + defguard_memberships: &HashMap>>, ldap_memberships: HashMap>, authority: Authority, ldap_config: &LDAPConfig, @@ -236,9 +252,9 @@ pub(super) fn compute_group_sync_changes<'a>( let mut delete_ldap = HashMap::new(); let mut add_ldap = HashMap::new(); - for (group, members) in defguard_memberships.clone() { + for (group, members) in defguard_memberships { debug!("Checking group {} for changes", group); - if let Some(ldap_members) = ldap_memberships.get(&group) { + if let Some(ldap_members) = ldap_memberships.get(group) { debug!("Group {group:?} found in LDAP, checking for membership differences"); let missing_from_defguard = ldap_members .iter() @@ -261,7 +277,8 @@ pub(super) fn compute_group_sync_changes<'a>( .collect::>(); trace!( - "Group {group:?} members missing from Defguard: {missing_from_defguard:?}, missing from LDAP: {missing_from_ldap:?}" + "Group {group:?} members missing from Defguard: {missing_from_defguard:?}, missing \ + from LDAP: {missing_from_ldap:?}" ); if missing_from_defguard.is_empty() { @@ -270,13 +287,15 @@ pub(super) fn compute_group_sync_changes<'a>( match authority { Authority::Defguard => { debug!( - "Group {group:?} has members missing from Defguard, marking them for deletion in LDAP: {missing_from_defguard:?}" + "Group {group:?} has members missing from Defguard, marking them for \ + deletion in LDAP: {missing_from_defguard:?}" ); delete_ldap.insert(group.clone(), missing_from_defguard); } Authority::LDAP => { debug!( - "Group {group:?} has members missing from Defguard, marking them for addition in Defguard: {missing_from_defguard:?}" + "Group {group:?} has members missing from Defguard, marking them for \ + addition in Defguard: {missing_from_defguard:?}" ); add_defguard.insert(group.clone(), missing_from_defguard); } @@ -289,13 +308,15 @@ pub(super) fn compute_group_sync_changes<'a>( match authority { Authority::Defguard => { debug!( - "Group {group:?} has members missing from LDAP, marking them for addition to LDAP: {missing_from_ldap:?}" + "Group {group:?} has members missing from LDAP, marking them for \ + addition to LDAP: {missing_from_ldap:?}" ); add_ldap.insert(group.clone(), missing_from_ldap); } Authority::LDAP => { debug!( - "Group {group:?} has members missing from LDAP, marking them for deletion in Defguard: {missing_from_ldap:?}" + "Group {group:?} has members missing from LDAP, marking them for \ + deletion in Defguard: {missing_from_ldap:?}" ); delete_defguard.insert(group.clone(), missing_from_ldap); } @@ -305,15 +326,17 @@ pub(super) fn compute_group_sync_changes<'a>( match authority { Authority::Defguard => { debug!( - "Group {group:?} is missing from LDAP, marking it for addition to LDAP along with all members due to Defguard authority" + "Group {group:?} is missing from LDAP, marking it for addition to LDAP \ + along with all members due to Defguard authority" ); - add_ldap.insert(group.clone(), members); + add_ldap.insert(group.clone(), members.clone()); } Authority::LDAP => { debug!( - "Group {group:?} is missing from LDAP, marking all its member for deletion from Defguard due to LDAP authority" + "Group {group:?} is missing from LDAP, marking all its member for deletion \ + from Defguard due to LDAP authority" ); - delete_defguard.insert(group.clone(), members); + delete_defguard.insert(group.clone(), members.clone()); } } } @@ -324,13 +347,15 @@ pub(super) fn compute_group_sync_changes<'a>( match authority { Authority::Defguard => { debug!( - "Group {group:?} is missing from Defguard, marking all its member for deletion from LDAP due to Defguard authority" + "Group {group:?} is missing from Defguard, marking all its member for \ + deletion from LDAP due to Defguard authority" ); delete_ldap.insert(group, members); } Authority::LDAP => { debug!( - "Group {group:?} is missing from Defguard, marking all its member for addition to Defguard due to LDAP authority" + "Group {group:?} is missing from Defguard, marking all its member for \ + addition to Defguard due to LDAP authority" ); add_defguard.insert(group, members); } @@ -400,7 +425,8 @@ fn attrs_different(defguard_user: &User, ldap_user: &User, config: &LDAPConf different } -/// Extracts users that are in both sources for later comparison and attritubte modification (emails, phone numbers) +/// Extracts users that are in both sources for later comparison and attritubte modification +/// (emails, phone numbers). pub(super) fn extract_intersecting_users( defguard_users: &mut Vec>, ldap_users: &mut Vec, @@ -479,9 +505,10 @@ impl super::LDAPConnection { Ok(()) } - /// Allows to synchronize user data (e.g. email, groups) between Defguard and LDAP based on the authority for a single user + /// Allows to synchronize user data (e.g. email, groups) between Defguard and LDAP based on the + /// authority for a single user. /// - /// Does nothing if the two way sync is disabled + /// Does nothing if the two way sync is disabled. pub(crate) async fn sync_user_data( &mut self, user: &User, @@ -490,7 +517,8 @@ impl super::LDAPConnection { debug!("Syncing user data for {user}"); let settings = Settings::get_current_settings(); - // Force update user data in LDAP if the two-way sync is disabled, otherwise respect the authority + // Force update user data in LDAP if the two-way sync is disabled, otherwise respect the + // authority. let authority = if !settings.ldap_sync_enabled || !settings.ldap_is_authoritative { Authority::Defguard } else { @@ -507,7 +535,7 @@ impl super::LDAPConnection { let intersecting_users = vec![(ldap_user.clone(), user.clone())]; - // create a hashmap for the calculate group membership changes function + // Create a hashmap for the calculated group membership changes function. let defguard_memberships = defguard_groups .iter() .map(|g| (g.clone(), hashset![user.clone()])) @@ -522,7 +550,7 @@ impl super::LDAPConnection { .await?; let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, authority, &self.config, @@ -533,12 +561,14 @@ impl super::LDAPConnection { } /// Fixes users with missing LDAP path - /// This is for compatibility with older Defguard versions that didn't store LDAP paths in the database - /// It will try to fetch the LDAP path from the LDAP server for users that have it missing - /// If the user is not found in LDAP, it will skip fixing that user + /// This is for compatibility with older Defguard versions that didn't store LDAP paths in the + /// database. + /// It will try to fetch the LDAP path from the LDAP server for users that have it missing. + /// If the user is not found in LDAP, it will skip fixing that user. /// - /// This function matches the user by username first, as those should be unique in both Defguard and LDAP. - /// Then, just to make sure the user wasn't renamed, it checks if the RDN values match. + /// This function matches the user by username first, as those should be unique in both Defguard + /// and LDAP. Then, just to make sure the user wasn't renamed, it checks if the RDN values + /// match. pub(crate) async fn fix_missing_user_path(&mut self, pool: &PgPool) -> Result<(), LdapError> { debug!("Fixing missing user path in LDAP"); @@ -566,10 +596,10 @@ impl super::LDAPConnection { if defguard_user_rdn != ldap_user_rdn { warn!( - "User {} has different RDN in Defguard ({}) and LDAP ({}), \ - cannot fix missing LDAP path. Please update their username manually, so - they match in both sources.", - defguard_user.username, defguard_user_rdn, ldap_user_rdn + "User {} has different RDN in Defguard ({defguard_user_rdn}) and \ + LDAP ({ldap_user_rdn}), cannot fix missing LDAP path. Please, \ + update the username manually, so it matches in both sources.", + defguard_user.username, ); continue; } @@ -583,14 +613,16 @@ impl super::LDAPConnection { defguard_user.save(&mut *transaction).await?; } else { warn!( - "User {} has no LDAP path in LDAP, skipping fixing their path in Defguard", + "User {} has no LDAP path in LDAP, skipping fixing their path in \ + Defguard", defguard_user.username ); } } Err(err) => { debug!( - "Failed to get user {} from LDAP: {err}, cannot update their DN in Defguard", + "Failed to get user {} from LDAP: {err}, cannot update their DN in \ + Defguard", defguard_user.username ); } @@ -632,8 +664,8 @@ impl super::LDAPConnection { } debug!( - "The following groups were defined for sync: {:?}, only Defguard users belonging to these groups will be synced", - sync_groups + "The following groups were defined for sync: {sync_groups:?}, only Defguard users \ + belonging to these groups will be synced" ); let mut sync_group_members = HashSet::new(); for sync_group in &sync_groups { @@ -696,7 +728,7 @@ impl super::LDAPConnection { ); let membership_changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, authority, &self.config, @@ -838,10 +870,10 @@ impl super::LDAPConnection { ); } else { warn!( - "LDAP user with username {} already exists in Defguard. \ - Those users have different DNs: {} (Defguard) vs {} (LDAP). \ - All usernames must be unique, so this LDAP user will not be added to Defguard.", - user.username, ldap_user_dn, defguard_user_dn + "LDAP user with username {} already exists in Defguard. Those users have \ + different DNs: {ldap_user_dn} (Defguard) vs {defguard_user_dn} (LDAP). All \ + usernames must be unique, so this LDAP user will not be added to Defguard.", + user.username, ); } } else { @@ -902,7 +934,8 @@ impl super::LDAPConnection { Ok(user) => all_users.push(user), Err(err) => { warn!( - "Failed to create user {username} from LDAP entry, error: {err}. The user will be skipped during sync" + "Failed to create user {username} from LDAP entry, error: {err}. The user \ + will be skipped during sync" ); debug!("Skipping user {username} due to error: {err}"); } diff --git a/crates/defguard_core/src/enterprise/ldap/test_client.rs b/crates/defguard_core/src/enterprise/ldap/test_client.rs index eaead30e1b..cd0c857178 100644 --- a/crates/defguard_core/src/enterprise/ldap/test_client.rs +++ b/crates/defguard_core/src/enterprise/ldap/test_client.rs @@ -402,7 +402,7 @@ impl LDAPConnection { where S: AsRef<[u8]> + Eq + Hash, { - let to_string = |s: S| String::from_utf8(s.as_ref().to_vec()).unwrap(); + let to_string = |s: S| str::from_utf8(s.as_ref()).unwrap().to_string(); let mods = mods .into_iter() .map(|modification| match modification { @@ -586,7 +586,7 @@ pub(super) fn user_to_test_attrs( pub(super) fn group_to_test_attrs( group: &Group, config: &LDAPConfig, - members: Option<&Vec<&User>>, + members: Option<&[&User]>, ) -> Vec<(String, HashSet)> { use crate::hashset; diff --git a/crates/defguard_core/src/enterprise/ldap/tests.rs b/crates/defguard_core/src/enterprise/ldap/tests.rs index c81085a895..576eb1dd26 100644 --- a/crates/defguard_core/src/enterprise/ldap/tests.rs +++ b/crates/defguard_core/src/enterprise/ldap/tests.rs @@ -896,7 +896,7 @@ fn test_compute_group_sync_changes_empty_maps() { let ldap_memberships = HashMap::new(); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::LDAP, &LDAPConfig::default(), @@ -920,7 +920,7 @@ fn test_ldap_authority_add_group_to_defguard() { ); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::LDAP, &LDAPConfig::default(), @@ -951,7 +951,7 @@ fn test_ldap_authority_delete_group_from_defguard(_: PgPoolOptions, options: PgC let ldap_memberships = HashMap::new(); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::LDAP, &LDAPConfig::default(), @@ -982,7 +982,7 @@ fn test_defguard_authority_add_group_to_ldap(_: PgPoolOptions, options: PgConnec let ldap_memberships = HashMap::new(); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::Defguard, &LDAPConfig::default(), @@ -1009,7 +1009,7 @@ fn test_defguard_authority_delete_group_from_ldap() { ); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::Defguard, &LDAPConfig::default(), @@ -1043,7 +1043,7 @@ fn test_matching_groups_no_changes(_: PgPoolOptions, options: PgConnectOptions) // Test both authority directions with identical group memberships let changes_ldap = compute_group_sync_changes( - defguard_memberships.clone(), + &defguard_memberships, ldap_memberships.clone(), Authority::LDAP, &LDAPConfig::default(), @@ -1063,7 +1063,7 @@ fn test_matching_groups_no_changes(_: PgPoolOptions, options: PgConnectOptions) assert!(changes_ldap.add_ldap.is_empty() || changes_ldap.add_ldap["test_group"].is_empty()); let changes_defguard = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::Defguard, &LDAPConfig::default(), @@ -1104,7 +1104,7 @@ fn test_ldap_authority_add_users_to_group(_: PgPoolOptions, options: PgConnectOp ); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::LDAP, &LDAPConfig::default(), @@ -1140,7 +1140,7 @@ fn test_ldap_authority_remove_users_from_group(_: PgPoolOptions, options: PgConn ); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::LDAP, &LDAPConfig::default(), @@ -1192,7 +1192,7 @@ fn test_multiple_groups_ldap_authority(_: PgPoolOptions, options: PgConnectOptio ); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::LDAP, &LDAPConfig::default(), @@ -1263,7 +1263,7 @@ fn test_multiple_groups_defguard_authority(_: PgPoolOptions, options: PgConnectO ldap_memberships.insert("group2".to_string(), HashSet::from_iter(vec![&user3])); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::Defguard, &LDAPConfig::default(), @@ -1301,7 +1301,7 @@ fn test_empty_groups() { ldap_memberships.insert("empty_group2".to_string(), HashSet::new()); let changes = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::LDAP, &LDAPConfig::default(), @@ -1372,7 +1372,7 @@ fn test_complex_group_memberships(_: PgPoolOptions, options: PgConnectOptions) { // Test with LDAP as authority let changes_ldap = compute_group_sync_changes( - defguard_memberships.clone(), + &defguard_memberships, ldap_memberships.clone(), Authority::LDAP, &LDAPConfig::default(), @@ -1406,7 +1406,7 @@ fn test_complex_group_memberships(_: PgPoolOptions, options: PgConnectOptions) { // Test with Defguard as authority let changes_defguard = compute_group_sync_changes( - defguard_memberships, + &defguard_memberships, ldap_memberships, Authority::Defguard, &LDAPConfig::default(), diff --git a/crates/defguard_core/src/handlers/auth.rs b/crates/defguard_core/src/handlers/auth.rs index a5ccd001e0..9ae24895cb 100644 --- a/crates/defguard_core/src/handlers/auth.rs +++ b/crates/defguard_core/src/handlers/auth.rs @@ -533,9 +533,7 @@ pub async fn webauthn_end( Ok(auth_result) => { if auth_result.needs_update() { // Find `Passkey` and try to update its credentials - for mut webauthn in - WebAuthn::all_for_user(&appstate.pool, session.user_id).await? - { + for webauthn in WebAuthn::all_for_user(&appstate.pool, session.user_id).await? { if let Some(true) = webauthn.passkey()?.update_credential(&auth_result) { webauthn.save(&appstate.pool).await?; } diff --git a/crates/model_derive/src/lib.rs b/crates/model_derive/src/lib.rs index edb91db3d7..c3c477278b 100644 --- a/crates/model_derive/src/lib.rs +++ b/crates/model_derive/src/lib.rs @@ -258,7 +258,7 @@ pub fn derive(input: TokenStream) -> TokenStream { Ok(()) } - pub async fn save<'e, E>(&mut self, executor: E) -> Result<(), sqlx::Error> + pub async fn save<'e, E>(&self, executor: E) -> Result<(), sqlx::Error> where E: sqlx::PgExecutor<'e> { From 6123693123a2e6fa175a98314ee297498eb61b1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Fri, 6 Mar 2026 09:54:21 +0100 Subject: [PATCH 6/9] Fixed by clippy --- .../src/enterprise/ldap/tests.rs | 2 +- .../defguard_core/src/enterprise/mod.rs.hack | 163 ++++++++++++++++++ crates/defguard_setup/tests/initial_setup.rs | 3 +- 3 files changed, 165 insertions(+), 3 deletions(-) create mode 100644 crates/defguard_core/src/enterprise/mod.rs.hack diff --git a/crates/defguard_core/src/enterprise/ldap/tests.rs b/crates/defguard_core/src/enterprise/ldap/tests.rs index 576eb1dd26..106ccecf95 100644 --- a/crates/defguard_core/src/enterprise/ldap/tests.rs +++ b/crates/defguard_core/src/enterprise/ldap/tests.rs @@ -357,7 +357,7 @@ async fn test_update_users_state(_: PgPoolOptions, options: PgConnectOptions) { attrs: group_to_test_attrs( &group, &ldap_conn.config, - Some(&vec![&active_user_in_ldap]) + Some(&[&active_user_in_ldap]) ), }], false diff --git a/crates/defguard_core/src/enterprise/mod.rs.hack b/crates/defguard_core/src/enterprise/mod.rs.hack new file mode 100644 index 0000000000..00eeb25e17 --- /dev/null +++ b/crates/defguard_core/src/enterprise/mod.rs.hack @@ -0,0 +1,163 @@ +pub mod activity_log_stream; +pub mod db; +pub mod directory_sync; +pub mod firewall; +pub mod grpc; +pub mod handlers; +pub mod ldap; +pub mod license; +pub mod limits; +pub mod snat; +mod utils; + +use license::{get_cached_license, validate_license}; +use limits::get_counts; + +use crate::enterprise::license::LicenseTier; + +/// Helper function to gate features which require a base license (Team or Business tier) +#[must_use] +pub fn is_business_license_active() -> bool { + // is_license_tier_active(LicenseTier::Business) + true +} + +/// Helper function to gate features which require an Enterprise tier license +#[must_use] +pub fn is_enterprise_license_active() -> bool { + // is_license_tier_active(LicenseTier::Enterprise) + true +} + +/// Shared logic for gating features to specific license tiers +fn is_license_tier_active(tier: LicenseTier) -> bool { + // debug!("Checking if features for {tier} license tier should be enabled"); + + // // get current object counts + // let counts = get_counts(); + + // // only check license if object count exceed free limit + // if counts.needs_paid_license() { + // debug!("User is over limit, checking his license"); + // let license = get_cached_license(); + // let validation_result = validate_license(license.as_ref(), &counts, tier); + // debug!("License validation result: {:?}", validation_result); + // validation_result.is_ok() + // } else { + // debug!("User is not over limit, allowing {tier} tier features"); + // true + // } + true +} + +// Is it the free version of the enterprise? +// Free = no valid license + not over the limit +// Paid = valid license or over the limit +pub(crate) fn is_enterprise_free() -> bool { + debug!("Checking if enterprise features are a part of the free version"); + let counts = get_counts(); + let license = get_cached_license(); + if validate_license(license.as_ref(), &counts, LicenseTier::Business).is_ok() { + false + } else if counts.needs_paid_license() { + debug!("User is over limit, the enterprise features are not free"); + false + } else { + debug!("User is not over limit, the enterprise features are free"); + true + } +} + +#[cfg(test)] +mod test { + use chrono::{TimeDelta, Utc}; + + use crate::{ + enterprise::{ + is_business_license_active, is_enterprise_free, is_enterprise_license_active, + license::{License, LicenseTier, set_cached_license}, + limits::{Counts, set_counts}, + }, + grpc::proto::enterprise::license::LicenseLimits, + }; + + #[test] + fn test_feature_gates_no_license() { + set_cached_license(None); + + // free limits are not exceeded + let counts = Counts::new(1, 1, 1, 1); + set_counts(counts); + + assert!(is_business_license_active()); + assert!(is_enterprise_license_active()); + assert!(is_enterprise_free()); + + // exceed free limits + let counts = Counts::new(1, 1, 5, 1); + set_counts(counts); + + assert!(!is_business_license_active()); + assert!(!is_enterprise_license_active()); + assert!(!is_enterprise_free()); + } + + #[test] + fn test_feature_gates_with_license() { + // exceed free limits + let counts = Counts::new(1, 1, 5, 1); + set_counts(counts); + + // set Business license + let users_limit = 15; + let devices_limit = 35; + let locations_limit = 5; + let network_devices_limit = 10; + + let limits = LicenseLimits { + users: users_limit, + devices: devices_limit, + locations: locations_limit, + network_devices: Some(network_devices_limit), + }; + let license = License::new( + "test".to_string(), + true, + Some(Utc::now() + TimeDelta::days(1)), + Some(limits), + None, + LicenseTier::Business, + ); + set_cached_license(Some(license)); + + assert!(is_business_license_active()); + assert!(!is_enterprise_license_active()); + assert!(!is_enterprise_free()); + + // set Enterprise license + let users_limit = 15; + let devices_limit = 35; + let locations_limit = 5; + let network_devices_limit = 10; + + let limits = LicenseLimits { + users: users_limit, + devices: devices_limit, + locations: locations_limit, + network_devices: Some(network_devices_limit), + }; + let license = License::new( + "test".to_string(), + true, + Some(Utc::now() + TimeDelta::days(1)), + Some(limits), + None, + LicenseTier::Enterprise, + ); + set_cached_license(Some(license)); + + assert!(is_business_license_active()); + assert!(is_enterprise_license_active()); + assert!(!is_enterprise_free()); + } +} diff --git a/crates/defguard_setup/tests/initial_setup.rs b/crates/defguard_setup/tests/initial_setup.rs index 303fd7042f..5fc450d6cc 100644 --- a/crates/defguard_setup/tests/initial_setup.rs +++ b/crates/defguard_setup/tests/initial_setup.rs @@ -41,8 +41,7 @@ async fn assert_setup_step(pool: &sqlx::PgPool, expected: InitialSetupStep) { let step = wizard .initial_setup_state .as_ref() - .map(|s| s.step) - .unwrap_or(InitialSetupStep::Welcome); + .map_or(InitialSetupStep::Welcome, |s| s.step); assert_eq!(step, expected); } From ac6b788a5949bad49a8530fcfe60443e0bb773bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Fri, 6 Mar 2026 13:45:01 +0100 Subject: [PATCH 7/9] Integration test for LDAP --- crates/defguard_common/src/db/models/user.rs | 2 +- .../src/enterprise/ldap/client.rs | 7 +- .../defguard_core/src/enterprise/ldap/mod.rs | 6 +- .../src/enterprise/ldap/test_client.rs | 12 +- .../src/enterprise/ldap/tests.rs | 14 +-- .../tests/integration/ldap/mod.rs | 112 ++++++++++++++++++ .../defguard_core/tests/integration/main.rs | 1 + crates/defguard_mail/src/tests.rs | 8 +- 8 files changed, 132 insertions(+), 30 deletions(-) create mode 100644 crates/defguard_core/tests/integration/ldap/mod.rs diff --git a/crates/defguard_common/src/db/models/user.rs b/crates/defguard_common/src/db/models/user.rs index 5d2fe78023..3d6f8d50e7 100644 --- a/crates/defguard_common/src/db/models/user.rs +++ b/crates/defguard_common/src/db/models/user.rs @@ -145,7 +145,7 @@ pub struct User { } // TODO: Refactor the user struct to use SecretStringWrapper instead of this -impl fmt::Debug for User { +impl fmt::Debug for User { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let Self { id, diff --git a/crates/defguard_core/src/enterprise/ldap/client.rs b/crates/defguard_core/src/enterprise/ldap/client.rs index 9de76e355e..3cbaad5605 100644 --- a/crates/defguard_core/src/enterprise/ldap/client.rs +++ b/crates/defguard_core/src/enterprise/ldap/client.rs @@ -16,7 +16,7 @@ use crate::enterprise::ldap::model::extract_rdn_value; const STREAMING_PAGE_SIZE: i32 = 500; impl LDAPConnection { - pub(crate) async fn create() -> Result { + pub async fn create() -> Result { let settings = Settings::get_current_settings(); let config = LDAPConfig::try_from(settings.clone())?; let url = settings.ldap_url.ok_or(LdapError::MissingSettings( @@ -110,10 +110,7 @@ impl LDAPConnection { } /// Returns groups that the given user is a member of. - pub(super) async fn get_user_groups( - &mut self, - user_dn: &str, - ) -> Result, LdapError> { + pub async fn get_user_groups(&mut self, user_dn: &str) -> Result, LdapError> { let user_dn_escaped = ldap_escape(user_dn); let filter = format!("({}={user_dn_escaped})", self.config.ldap_group_member_attr); debug!("Performing LDAP group search with filter: {filter}"); diff --git a/crates/defguard_core/src/enterprise/ldap/mod.rs b/crates/defguard_core/src/enterprise/ldap/mod.rs index ea9af3c40d..73a48ff5de 100644 --- a/crates/defguard_core/src/enterprise/ldap/mod.rs +++ b/crates/defguard_core/src/enterprise/ldap/mod.rs @@ -275,7 +275,7 @@ impl TryFrom for LDAPConfig { fn try_from(settings: Settings) -> Result { /// Helper function to validate non-empty string settings. /// Returns an error if the setting is None or is an empty string. - /// This is to prevent constructing an invalid LDAPConfig + /// This is to prevent constructing an invalid LDAPConfig. fn validate_string_setting( value: Option, setting_name: &str, @@ -795,7 +795,7 @@ impl LDAPConnection { pub async fn add_group_with_members( &mut self, group_name: &str, - members: Vec<&User>, + members: &[&User], ) -> Result<(), LdapError> { debug!("Adding LDAP group {group_name}"); let dn = self.config.group_dn(group_name); @@ -895,7 +895,7 @@ impl LDAPConnection { debug!("Added user {user} to group {groupname} in LDAP"); } else { debug!("Group {groupname} doesn't exist in LDAP, creating it"); - self.add_group_with_members(groupname, vec![user]).await?; + self.add_group_with_members(groupname, &[user]).await?; debug!("Group {groupname} created and member added in LDAP"); } info!("Added user {user} to group {groupname} in LDAP"); diff --git a/crates/defguard_core/src/enterprise/ldap/test_client.rs b/crates/defguard_core/src/enterprise/ldap/test_client.rs index cd0c857178..79681ed325 100644 --- a/crates/defguard_core/src/enterprise/ldap/test_client.rs +++ b/crates/defguard_core/src/enterprise/ldap/test_client.rs @@ -179,9 +179,8 @@ impl TestClient { pub(super) fn events_match(&self, expected: &[LdapEvent], order_matters: bool) -> bool { if self.events.len() != expected.len() { - return false; - } - if order_matters { + false + } else if order_matters { self.events == expected } else { vecs_equal_unordered(&self.events, expected) @@ -240,7 +239,7 @@ impl TestClient { } impl LDAPConnection { - pub(crate) async fn create() -> Result { + pub async fn create() -> Result { Ok(Self { config: LDAPConfig::default(), url: String::new(), @@ -343,10 +342,7 @@ impl LDAPConnection { Ok(()) } - pub(super) async fn get_user_groups( - &mut self, - user_dn: &str, - ) -> Result, LdapError> { + pub async fn get_user_groups(&mut self, user_dn: &str) -> Result, LdapError> { let mut groups = Vec::new(); if let Some(Object::User(_)) = self.test_client.objects.get(user_dn) { diff --git a/crates/defguard_core/src/enterprise/ldap/tests.rs b/crates/defguard_core/src/enterprise/ldap/tests.rs index 106ccecf95..d49fae12a7 100644 --- a/crates/defguard_core/src/enterprise/ldap/tests.rs +++ b/crates/defguard_core/src/enterprise/ldap/tests.rs @@ -29,11 +29,11 @@ fn make_test_user( ldap_user_path: Option, ) -> User { let mut user = User::new( - username, + username.to_string(), Some(PASSWORD), - "last name", - "first name", - format!("{username}@example.com").as_str(), + "last name".to_string(), + "first name".to_string(), + format!("{username}@example.com"), None, ); user.ldap_rdn = ldap_rdn; @@ -354,11 +354,7 @@ async fn test_update_users_state(_: PgPoolOptions, options: PgConnectOptions) { assert!(ldap_conn.test_client.events_match( &[LdapEvent::ObjectAdded { dn: ldap_conn.config.group_dn(&group.name), - attrs: group_to_test_attrs( - &group, - &ldap_conn.config, - Some(&[&active_user_in_ldap]) - ), + attrs: group_to_test_attrs(&group, &ldap_conn.config, Some(&[&active_user_in_ldap])), }], false )); diff --git a/crates/defguard_core/tests/integration/ldap/mod.rs b/crates/defguard_core/tests/integration/ldap/mod.rs new file mode 100644 index 0000000000..8eac1c2443 --- /dev/null +++ b/crates/defguard_core/tests/integration/ldap/mod.rs @@ -0,0 +1,112 @@ +//! Integration tests that require a running LDAP server. + +use std::{env, str::FromStr}; + +use defguard_common::{ + config::{DefGuardConfig, SERVER_CONFIG}, + db::{ + models::{ + Settings, User, + group::Group, + settings::{initialize_current_settings, set_settings}, + }, + setup_pool, + }, + secret::SecretStringWrapper, +}; +use defguard_core::enterprise::ldap::LDAPConnection; +use sqlx::{ + PgPool, + postgres::{PgConnectOptions, PgPoolOptions}, +}; + +/// Set LDAP settings from environment variables. +async fn set_ldap_settings(pool: &PgPool) { + let config = DefGuardConfig::new_test_config(); + let _ = SERVER_CONFIG.set(config); + initialize_current_settings(pool).await.unwrap(); + + let mut settings = Settings::get_current_settings(); + settings.ldap_url = env::var("LDAP_URL").ok(); + settings.ldap_bind_username = env::var("LDAP_BIND_USERNAME").ok(); + settings.ldap_bind_password = env::var("LDAP_BIND_PASSWORD") + .map(|pass| SecretStringWrapper::from_str(&pass).unwrap()) + .ok(); + settings.ldap_group_search_base = env::var("LDAP_GROUP_SEARCH_BASE").ok(); + settings.ldap_user_search_base = env::var("LDAP_USER_SEARCH_BASE").ok(); + settings.ldap_user_obj_class = env::var("LDAP_USER_CLASS").ok(); + settings.ldap_group_obj_class = env::var("LDAP_GROUP_CLASS").ok(); + settings.ldap_username_attr = env::var("LDAP_USERNAME_ATTR").ok(); + settings.ldap_groupname_attr = env::var("LDAP_GROUPNAME_ATTR").ok(); + settings.ldap_group_member_attr = env::var("LDAP_GROU_MEMBER_ATTR").ok(); + settings.ldap_member_attr = env::var("LDAP_MEMBER_ATTR").ok(); + settings.ldap_use_starttls = env::var("LDAP_STARTTLS").is_ok(); + settings.ldap_tls_verify_cert = env::var("LDAP_TLS_VERIFY").is_ok(); + settings.ldap_enabled = true; + set_settings(Some(settings)); +} + +#[ignore = "Requires LDAP server"] +#[sqlx::test] +async fn test_ldap(_: PgPoolOptions, options: PgConnectOptions) { + let pool = setup_pool(options).await; + set_ldap_settings(&pool).await; + + let password = "pass123"; + let mut user = User::new( + "user1", + Some(password), + "Test", + "One", + "user1@test.defguard", + None, + ) + .save(&pool) + .await + .unwrap(); + let group = Group::new("testers").save(&pool).await.unwrap(); + + let mut ldap_conn = LDAPConnection::create().await.unwrap(); + ldap_conn.config.ldap_sync_groups = vec![String::from("testers")]; + // Try to remove user first, in case the previous test run failed. + let _ = ldap_conn.delete_user(&mut user).await; + + // Add user to LDAP. + ldap_conn + .add_user(&mut user, Some(password), &pool) + .await + .unwrap(); + + let groups = ldap_conn + .get_user_groups(user.ldap_rdn.as_ref().unwrap()) + .await + .unwrap(); + assert_eq!(groups.len(), 0); + + // Add group to LDAP. This is redundant as `add_user_to_group` does the same. + ldap_conn + .add_group_with_members(&group.name, &[&user]) + .await + .unwrap(); + // Add user to group; `add_group_with_members` doesn't do it. + ldap_conn + .add_user_to_group(&user, &group.name) + .await + .unwrap(); + + // Build user DN. + let dn = format!( + "{}={},{}", + ldap_conn.config.ldap_username_attr, + user.ldap_rdn.as_ref().unwrap(), + user.ldap_user_path.as_ref().unwrap() + ); + // Get groups the user belongs to. + let groups = ldap_conn.get_user_groups(&dn).await.unwrap(); + assert_eq!(groups.len(), 1); + assert_eq!(groups[0], group.name); + + // Cleanup + ldap_conn.delete_group(&group.name).await.unwrap(); + ldap_conn.delete_user(&mut user).await.unwrap(); +} diff --git a/crates/defguard_core/tests/integration/main.rs b/crates/defguard_core/tests/integration/main.rs index b3793ede28..43ffcc1303 100644 --- a/crates/defguard_core/tests/integration/main.rs +++ b/crates/defguard_core/tests/integration/main.rs @@ -1,3 +1,4 @@ mod api; mod common; // mod grpc; +mod ldap; diff --git a/crates/defguard_mail/src/tests.rs b/crates/defguard_mail/src/tests.rs index cbcd01e21b..1d153ca08d 100644 --- a/crates/defguard_mail/src/tests.rs +++ b/crates/defguard_mail/src/tests.rs @@ -39,7 +39,7 @@ async fn set_smtp_settings(pool: &PgPool) { set_settings(Some(settings)); } -#[ignore] +#[ignore = "Requires SMTP server"] #[sqlx::test] fn send_desktop_start(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; @@ -63,7 +63,7 @@ fn send_desktop_start(_: PgPoolOptions, options: PgConnectOptions) { tokio::time::sleep(Duration::from_secs(2)).await; } -#[ignore] +#[ignore = "Requires SMTP server"] #[sqlx::test] fn send_new_device_added(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; @@ -98,7 +98,7 @@ fn send_new_device_added(_: PgPoolOptions, options: PgConnectOptions) { tokio::time::sleep(Duration::from_secs(2)).await; } -#[ignore] +#[ignore = "Requires SMTP server"] #[sqlx::test] fn send_mfa_code(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; @@ -121,7 +121,7 @@ fn send_mfa_code(_: PgPoolOptions, options: PgConnectOptions) { tokio::time::sleep(Duration::from_secs(2)).await; } -#[ignore] +#[ignore = "Requires SMTP server"] #[sqlx::test] fn send_new_account(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; From b67b39c2dda172dc83a1cdd883f4e2380632f22b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Fri, 6 Mar 2026 14:05:24 +0100 Subject: [PATCH 8/9] lint --- crates/defguard_core/src/enterprise/ldap/sync.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/defguard_core/src/enterprise/ldap/sync.rs b/crates/defguard_core/src/enterprise/ldap/sync.rs index aa8b39dc0b..edf315203d 100644 --- a/crates/defguard_core/src/enterprise/ldap/sync.rs +++ b/crates/defguard_core/src/enterprise/ldap/sync.rs @@ -54,7 +54,7 @@ //! - LDAP connection is stable //! - The LDAP change pull is performed relatively often //! - One object is not changed in both sources between two asynchronous syncs (may cause -//! overwriting of changes), but this sounds like an unlikely scenario +//! overwriting of changes), but this sounds like an unlikely scenario //! //! # Potential improvements and issues //! From 347d297cdb463f48cc81c67e86f53d9954021d9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Fri, 6 Mar 2026 15:00:56 +0100 Subject: [PATCH 9/9] Fix Dockerfile --- Dockerfile | 1 + 1 file changed, 1 insertion(+) diff --git a/Dockerfile b/Dockerfile index a288fe9cec..d7855cc335 100644 --- a/Dockerfile +++ b/Dockerfile @@ -37,6 +37,7 @@ COPY Cargo.toml Cargo.lock ./ COPY .git .git COPY .sqlx .sqlx COPY crates crates +COPY tools tools COPY proto proto COPY migrations migrations RUN cargo install --locked --bin defguard --path ./crates/defguard --root /build