From 8d54d0b44cedad2c391498e7445634ae0e039fce Mon Sep 17 00:00:00 2001 From: Maurus Decimus <11444311+mdecimus@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:34:04 +0200 Subject: [PATCH] Fix Directory: Update local groups only when the external directory includes a group attribute --- CHANGELOG.md | 1 + crates/common/src/cache/directory.rs | 46 +++++++++++---------- crates/directory/src/backend/ldap/lookup.rs | 45 +++++++++++--------- crates/directory/src/backend/oidc/lookup.rs | 25 ++++++----- crates/directory/src/backend/sql/lookup.rs | 6 ++- crates/directory/src/lib.rs | 2 +- tests/src/directory/ldap.rs | 10 ++--- tests/src/directory/oidc.rs | 6 +-- tests/src/directory/sql.rs | 6 +-- tests/src/directory/synchronization.rs | 43 ++++++++++++++++--- 10 files changed, 116 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 810c2509..d9339e13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ If you are upgrading from v0.16.x, replace the binary (or run `docker pull`). If - JMAP: - VacationResponse: `isEnabled` reset to false whenever properties are changed. - Capabilities: Return RFC-3339-conformant UTCDate literals in capabilities: min `0001-01-01T00:00:00Z`, max `9999-12-31T23:59:59Z`. +- Directory: Update local groups only when the external directory includes a group attribute. - Reject invalid duration values (e.g. `1h30m`). - Branding: Custom logos for domains do not work. - Sieve: add `Received` headers to auto-generated messages and detect loops. diff --git a/crates/common/src/cache/directory.rs b/crates/common/src/cache/directory.rs index 94766294..06b9d672 100644 --- a/crates/common/src/cache/directory.rs +++ b/crates/common/src/cache/directory.rs @@ -91,26 +91,28 @@ impl Server { has_changes = true; } } - let mut member_group_ids = Vec::with_capacity(account.groups.len()); - for email in account.groups { - member_group_ids.push( - self.synchronize_group(directory::Group { - email, - ..Default::default() - }) - .await - .caused_by(trc::location!())? - .into(), - ); - } - if updated_account.member_group_ids.len() != member_group_ids.len() - || !updated_account - .member_group_ids - .iter() - .all(|id| member_group_ids.contains(id)) - { - updated_account.member_group_ids = member_group_ids.into(); - has_changes = true; + if let Some(groups) = account.groups { + let mut member_group_ids = Vec::with_capacity(groups.len()); + for email in groups { + member_group_ids.push( + self.synchronize_group(directory::Group { + email, + ..Default::default() + }) + .await + .caused_by(trc::location!())? + .into(), + ); + } + if updated_account.member_group_ids.len() != member_group_ids.len() + || !updated_account + .member_group_ids + .iter() + .all(|id| member_group_ids.contains(id)) + { + updated_account.member_group_ids = member_group_ids.into(); + has_changes = true; + } } if has_changes { @@ -168,8 +170,8 @@ impl Server { }); } } - let mut member_group_ids = Vec::with_capacity(account.groups.len()); - for email in account.groups { + let mut member_group_ids = Vec::new(); + for email in account.groups.unwrap_or_default() { member_group_ids.push( self.synchronize_group(directory::Group { email, diff --git a/crates/directory/src/backend/ldap/lookup.rs b/crates/directory/src/backend/ldap/lookup.rs index 0e9bccb5..d6e0432b 100644 --- a/crates/directory/src/backend/ldap/lookup.rs +++ b/crates/directory/src/backend/ldap/lookup.rs @@ -120,11 +120,9 @@ impl LdapDirectory { conn: &mut Ldap, result: &mut LdapResult, ) -> trc::Result<()> { - if !result.account.groups.is_empty() { - for name in std::mem::take(&mut result.account.groups) - .into_iter() - .filter(|name| name.contains('=')) - { + if let Some(group_dns) = result.account.groups.take() { + let mut groups = Vec::new(); + for name in group_dns.into_iter().filter(|name| name.contains('=')) { let (rs, _res) = conn .search( &name, @@ -142,12 +140,13 @@ impl LdapDirectory { && let Some(email) = value.first().map(|s| s.as_str()).and_then(sanitize_email) { - result.account.groups.push(email); + groups.push(email); break 'outer; } } } } + result.account.groups = if groups.is_empty() { None } else { Some(groups) }; } else if let Some(filter) = &self.mappings.filter_member_of { let filter = filter.build(&result.dn); let rs = conn @@ -162,25 +161,31 @@ impl LdapDirectory { .success() .map_err(|err| err.into_error().caused_by(trc::location!()))? .0; + let had_entries = !rs.is_empty(); + let mut groups = Vec::new(); for entry in rs { for (attr, value) in SearchEntry::construct(entry).attrs { if self.mappings.attr_email.contains(&attr.to_lowercase()) { - result - .account - .groups - .extend(value.into_iter().filter_map(|v| { - sanitize_email(&v).or_else(|| { - trc::event!( - Store(trc::StoreEvent::LdapWarning), - Reason = "Group entry missing valid email attribute", - Details = v - ); - None - }) - })); + groups.extend(value.into_iter().filter_map(|v| { + sanitize_email(&v).or_else(|| { + trc::event!( + Store(trc::StoreEvent::LdapWarning), + Reason = "Group entry missing valid email attribute", + Details = v + ); + None + }) + })); } } } + result.account.groups = if had_entries && groups.is_empty() { + None + } else { + Some(groups) + }; + } else { + result.account.groups = Some(Vec::new()); } Ok(()) @@ -253,7 +258,7 @@ impl LdapMappings { account.description = Some(desc); } } else if self.attr_groups.contains(&attr) { - account.groups.extend(value); + account.groups.get_or_insert_default().extend(value); } else if self.attr_class.contains(&attr) { for value in value { is_group |= value.eq_ignore_ascii_case(&self.group_class); diff --git a/crates/directory/src/backend/oidc/lookup.rs b/crates/directory/src/backend/oidc/lookup.rs index 211e6f10..7fc5e78c 100644 --- a/crates/directory/src/backend/oidc/lookup.rs +++ b/crates/directory/src/backend/oidc/lookup.rs @@ -234,19 +234,18 @@ impl OpenIdDirectory { email, email_aliases: Vec::new(), secret: None, - groups: self - .config - .claim_groups - .as_ref() - .and_then(|groups_claim| claims.get(groups_claim)) - .map(extract_string_list) - .unwrap_or_default() - .into_iter() - .map(|group| match &self.config.default_domain { - Some(domain) if !group.contains('@') => format!("{group}@{domain}"), - _ => group, - }) - .collect(), + groups: self.config.claim_groups.as_ref().map(|groups_claim| { + claims + .get(groups_claim) + .map(extract_string_list) + .unwrap_or_default() + .into_iter() + .map(|group| match &self.config.default_domain { + Some(domain) if !group.contains('@') => format!("{group}@{domain}"), + _ => group, + }) + .collect() + }), description: self .config .claim_name diff --git a/crates/directory/src/backend/sql/lookup.rs b/crates/directory/src/backend/sql/lookup.rs index bab6c9af..bd6f1223 100644 --- a/crates/directory/src/backend/sql/lookup.rs +++ b/crates/directory/src/backend/sql/lookup.rs @@ -52,6 +52,7 @@ impl SqlDirectory { // Obtain members if let Some(query) = &self.mappings.query_member_of { + let members = account.groups.get_or_insert_default(); for row in self .sql_store .sql_query::(query, vec![username.into()]) @@ -62,7 +63,7 @@ impl SqlDirectory { if let Some(Value::Text(address)) = row.values.first() && let Some(email) = sanitize_email(address) { - account.groups.push(email); + members.push(email); } } } @@ -103,6 +104,7 @@ impl SqlDirectory { Recipient::Account(mut account) => { // Obtain members if let Some(query) = &self.mappings.query_member_of { + let members = account.groups.get_or_insert_default(); for row in self .sql_store .sql_query::(query, vec![account.email.as_str().into()]) @@ -113,7 +115,7 @@ impl SqlDirectory { if let Some(Value::Text(address)) = row.values.first() && let Some(email) = sanitize_email(address) { - account.groups.push(email); + members.push(email); } } } diff --git a/crates/directory/src/lib.rs b/crates/directory/src/lib.rs index c225df71..35226139 100644 --- a/crates/directory/src/lib.rs +++ b/crates/directory/src/lib.rs @@ -47,7 +47,7 @@ pub struct Account { pub email: String, pub email_aliases: Vec, pub secret: Option, - pub groups: Vec, + pub groups: Option>, pub description: Option, } diff --git a/tests/src/directory/ldap.rs b/tests/src/directory/ldap.rs index b3d7cddb..a748fc03 100644 --- a/tests/src/directory/ldap.rs +++ b/tests/src/directory/ldap.rs @@ -29,7 +29,7 @@ pub async fn test() { email: "john.doe@example.org".into(), email_aliases: vec!["john@example.org".into()], secret: Some("$app$8958830913002348890$".into()), - groups: vec!["sales@example.org".into()], + groups: Some(vec!["sales@example.org".into()]), description: Some("John Doe".into()), } ); @@ -45,7 +45,7 @@ pub async fn test() { email: "jane.smith@example.org".into(), email_aliases: vec![], secret: Some("$app$4096614298472586996$".into()), - groups: vec!["sales@example.org".into(), "corporate@example.org".into()], + groups: Some(vec!["sales@example.org".into(), "corporate@example.org".into()]), description: Some("Jane Smith".into()), } ); @@ -76,7 +76,7 @@ pub async fn test() { email: "john.doe@example.org".into(), email_aliases: vec!["john@example.org".into()], secret: Some("this is John's LDAP password".into()), - groups: vec!["sales@example.org".into()], + groups: Some(vec!["sales@example.org".into()]), description: Some("John Doe".into()), } ); @@ -97,7 +97,7 @@ pub async fn test() { email: "john.doe@example.org".into(), email_aliases: vec!["john@example.org".into()], secret: Some("this is John's LDAP password".into()), - groups: vec!["sales@example.org".into()], + groups: Some(vec!["sales@example.org".into()]), description: Some("John Doe".into()) }) ); @@ -107,7 +107,7 @@ pub async fn test() { email: "jane.smith@example.org".into(), email_aliases: vec![], secret: Some("this is Jane's LDAP password".into()), - groups: vec!["sales@example.org".into(), "corporate@example.org".into()], + groups: Some(vec!["sales@example.org".into(), "corporate@example.org".into()]), description: Some("Jane Smith".into()) }) ); diff --git a/tests/src/directory/oidc.rs b/tests/src/directory/oidc.rs index dd0bb0b3..72f32158 100644 --- a/tests/src/directory/oidc.rs +++ b/tests/src/directory/oidc.rs @@ -44,7 +44,7 @@ pub async fn test() { email: "john.doe@example.org".to_string(), email_aliases: vec![], secret: None, - groups: vec!["sales@example.org".to_string()], + groups: Some(vec!["sales@example.org".to_string()]), description: Some("John Doe".to_string()) } ); @@ -66,7 +66,7 @@ pub async fn test() { email: "john.doe@example.org".to_string(), email_aliases: vec![], secret: None, - groups: vec!["sales@example.org".to_string()], + groups: Some(vec!["sales@example.org".to_string()]), description: Some("John Doe".to_string()) } ); @@ -115,7 +115,7 @@ pub async fn test() { email: "john.doe@example.org".to_string(), email_aliases: vec![], secret: None, - groups: vec!["sales@example.org".to_string()], + groups: Some(vec!["sales@example.org".to_string()]), description: None, } ); diff --git a/tests/src/directory/sql.rs b/tests/src/directory/sql.rs index f909974d..c09d9f88 100644 --- a/tests/src/directory/sql.rs +++ b/tests/src/directory/sql.rs @@ -97,7 +97,7 @@ pub async fn test() { email: "john@example.org".to_string(), email_aliases: vec!["john.doe@example.org".to_string(),], secret: Some("john secret".to_string()), - groups: vec!["sales@example.org".to_string()], + groups: Some(vec!["sales@example.org".to_string()]), description: Some("John Doe".to_string()), } ); @@ -118,7 +118,7 @@ pub async fn test() { email: "john@example.org".to_string(), email_aliases: vec!["john.doe@example.org".to_string()], secret: Some("john secret".to_string()), - groups: vec!["sales@example.org".to_string()], + groups: Some(vec!["sales@example.org".to_string()]), description: Some("John Doe".to_string()), }) ); @@ -128,7 +128,7 @@ pub async fn test() { email: "jane@example.org".to_string(), email_aliases: vec![], secret: Some("jane secret".to_string()), - groups: vec!["sales@example.org".to_string()], + groups: Some(vec!["sales@example.org".to_string()]), description: Some("Jane Doe".to_string()), }) ); diff --git a/tests/src/directory/synchronization.rs b/tests/src/directory/synchronization.rs index 1413f512..8072e3ef 100644 --- a/tests/src/directory/synchronization.rs +++ b/tests/src/directory/synchronization.rs @@ -29,7 +29,7 @@ pub async fn test() { email: "john@unknown.org".to_string(), email_aliases: vec![], secret: "supersecret".to_string().into(), - groups: vec![], + groups: Some(vec![]), description: "John Doe".to_string().into(), }) .await @@ -44,10 +44,10 @@ pub async fn test() { "j.doe@example.org".to_string(), ], secret: "supersecret".to_string().into(), - groups: vec![ + groups: Some(vec![ "corporate@example.org".to_string(), "sales@example.org".to_string(), - ], + ]), description: "John Doe".to_string().into(), }; let result = test @@ -162,8 +162,9 @@ pub async fn test() { account_in .email_aliases .push("johnny@example.org".to_string()); - account_in.groups.pop(); - account_in.groups.push("support@example.org".to_string()); + let groups = account_in.groups.get_or_insert_default(); + groups.pop(); + groups.push("support@example.org".to_string()); account_in.secret = "evenmoresecret".to_string().into(); assert_eq!( test.server @@ -227,6 +228,38 @@ pub async fn test() { 4 ); + account_in.groups = None; + test.server + .synchronize_account(account_in.clone()) + .await + .unwrap(); + let account_out = test + .server + .registry() + .object::(account_id) + .await + .unwrap() + .unwrap() + .into_user() + .unwrap(); + assert_eq!(account_out.member_group_ids.len(), 2); + + account_in.groups = Some(vec![]); + test.server + .synchronize_account(account_in.clone()) + .await + .unwrap(); + let account_out = test + .server + .registry() + .object::(account_id) + .await + .unwrap() + .unwrap() + .into_user() + .unwrap(); + assert_eq!(account_out.member_group_ids.len(), 0); + // Synchronize a group assert_eq!( test.server