From 7ddbd2ac214791fd7b119022be1c5d1076703990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tristan=20Dani=C3=ABl=20Maat?= Date: Thu, 13 Feb 2025 14:48:17 +0800 Subject: [PATCH] fix: Rework internals to make localparts non-optional Since localparts from ldap syncs would end up empty, this resulted in lots of spurious logging about user updates that weren't actually happening. This gets rid of that by mapping types better to what's actually happening. --- src/bin/migrate.rs | 138 ++++++++++++++++++++++++++---------- src/lib.rs | 40 +++++++---- src/sources/csv.rs | 40 ++++++----- src/sources/ldap.rs | 26 ++++--- src/user.rs | 60 ++++------------ src/zitadel.rs | 168 ++++++++++++++++++++++++++++++++++---------- tests/e2e.rs | 108 +++++++++++++++++++++++++++- 7 files changed, 418 insertions(+), 162 deletions(-) diff --git a/src/bin/migrate.rs b/src/bin/migrate.rs index d838c0c..f28045e 100644 --- a/src/bin/migrate.rs +++ b/src/bin/migrate.rs @@ -111,23 +111,54 @@ fn detect_database_encoding(users: Vec) -> ExternalIdEncoding { #[cfg(test)] mod tests { + use base64::prelude::*; + use famedly_sync::user; + use super::*; use crate::{ExternalIdEncoding, SyncUser}; - fn create_test_user(external_user_id: &str) -> SyncUser { + enum UserId { + Hex(String), + Base64(String), + Plain(String), + } + + impl UserId { + fn to_owned(&self) -> String { + match self { + UserId::Hex(id) => id.to_owned(), + UserId::Base64(id) => id.to_owned(), + UserId::Plain(id) => id.to_owned(), + } + } + + fn get_localpart(&self) -> String { + match self { + Self::Hex(id) => user::compute_famedly_uuid( + &hex::decode(id).expect("Must be valid hex-encoded string"), + ), + UserId::Base64(id) => user::compute_famedly_uuid( + &BASE64_STANDARD.decode(id).expect("Must be valid base64-encoded string"), + ), + UserId::Plain(id) => user::compute_famedly_uuid(id.as_bytes()), + } + } + } + + fn create_test_user(external_user_id: UserId) -> SyncUser { SyncUser::new( "first name".to_owned(), "last name".to_owned(), "email@example.com".to_owned(), None, true, - None, + "Example User".to_owned(), external_user_id.to_owned(), - None, + external_user_id.get_localpart(), ) } - fn run_detection_test(user_ids: Vec<&str>, expected_encoding: ExternalIdEncoding) { + fn run_detection_test(user_ids: Vec, expected_encoding: ExternalIdEncoding) { let users: Vec = user_ids .into_iter() .map(create_test_user) // Assuming SyncUser::new(&str) exists @@ -142,7 +173,7 @@ mod tests { } fn run_conversion_test( - original_id: &str, + original_id: UserId, expected_encoding: ExternalIdEncoding, expected_result: &str, ) { @@ -160,7 +191,11 @@ mod tests { #[tokio::test] async fn test_all_hex() { // All users look like hex: "deadbeef", "cafebabe", "0123456789abcdef" - let user_ids = vec!["deadbeef", "cafebabe", "0123456789abcdef"]; + let user_ids = vec![ + UserId::Hex("deadbeef".to_owned()), + UserId::Hex("cafebabe".to_owned()), + UserId::Hex("0123456789abcdef".to_owned()), + ]; run_detection_test(user_ids, ExternalIdEncoding::Hex); } @@ -171,14 +206,25 @@ mod tests { // "Zm9v" decodes to "foo" // "YmFy" decodes to "bar" // All are valid base64 and length % 4 == 0 - let user_ids = vec!["Y2FmZQ==", "Zm9v", "YmFy"]; + let user_ids = vec![ + UserId::Base64("Y2FmZQ==".to_owned()), + UserId::Base64("Zm9v".to_owned()), + UserId::Base64("YmFy".to_owned()), + ]; run_detection_test(user_ids, ExternalIdEncoding::Base64); } #[tokio::test] async fn test_mixed_ambiguous() { // Some look hex, all look base64 - let user_ids = vec!["cafebabe", "deadbeef", "beefcafe", "Y2FmZQ==", "Zm9v", "YmFy"]; + let user_ids = vec![ + UserId::Hex("cafebabe".to_owned()), + UserId::Hex("deadbeef".to_owned()), + UserId::Hex("beefcafe".to_owned()), + UserId::Base64("Y2FmZQ==".to_owned()), + UserId::Base64("Zm9v".to_owned()), + UserId::Base64("YmFy".to_owned()), + ]; run_detection_test(user_ids, ExternalIdEncoding::Base64); } @@ -188,7 +234,11 @@ mod tests { // "cafeb" length is 5, not divisible by 2 or 4, so neither hex nor base64 // "abc" length is 3, not divisible by 4, and 'c' is hex valid but odd length -> // not hex. - let user_ids = vec!["cafe", "cafeb", "abc"]; + let user_ids = vec![ + UserId::Hex("cafe".to_owned()), + UserId::Plain("cafeb".to_owned()), + UserId::Plain("abc".to_owned()), + ]; // "cafe" might count for both hex and base64, but "cafeb" and "abc" won't count // for either. Out of 3, maybe 1 counts as hex/base64 and 2 are plain. Ratios: // hex = 1/3 ≈ 0.33, base64 = 1/3 ≈ 0.33, both < 0.8. @@ -199,7 +249,7 @@ mod tests { async fn test_invalid_characters() { // "zzz" is not hex. It's also not base64-safe (though 'z' is alphanumeric, // length=3 %4!=0) "+++" is not hex and length=3 not multiple of 4 for base64. - let user_ids = vec!["zzz", "+++"]; + let user_ids = vec![UserId::Plain("zzz".to_owned()), UserId::Plain("+++".to_owned())]; run_detection_test(user_ids, ExternalIdEncoding::Ambiguous); } @@ -211,14 +261,18 @@ mod tests { // - 3 plain (30%) let user_ids = vec![ // Hex format users (30%) - "deadbeef", "cafebabe", "12345678", + UserId::Hex("deadbeef".to_owned()), + UserId::Hex("cafebabe".to_owned()), + UserId::Hex("12345678".to_owned()), // Base64 format users (40%) - "Y2FmZQ==", // "cafe" - "Zm9vYmFy", // "foobar" - "aGVsbG8=", // "hello" - "d29ybGQ=", // "world" + UserId::Base64("Y2FmZQ==".to_owned()), // "cafe" + UserId::Base64("Zm9vYmFy".to_owned()), // "foobar" + UserId::Base64("aGVsbG8=".to_owned()), // "hello" + UserId::Base64("d29ybGQ=".to_owned()), // "world" // Plain format users (30%) - "plain_1", "plain_2", "plain_3", + UserId::Plain("plain_1".to_owned()), + UserId::Plain("plain_2".to_owned()), + UserId::Plain("plain_3".to_owned()), ]; // Both hex (30%) and base64 (40%) > 20% threshold @@ -232,8 +286,16 @@ mod tests { // Testing near 90% threshold for hex // 9 hex users and 1 plain = 90% exactly let user_ids = vec![ - "deadbeef", "cafebabe", "beefcafe", "12345678", "87654321", "abcdef12", "34567890", - "98765432", "fedcba98", "plain_id", + UserId::Hex("deadbeef".to_owned()), + UserId::Hex("cafebabe".to_owned()), + UserId::Hex("beefcafe".to_owned()), + UserId::Hex("12345678".to_owned()), + UserId::Hex("87654321".to_owned()), + UserId::Hex("abcdef12".to_owned()), + UserId::Hex("34567890".to_owned()), + UserId::Hex("98765432".to_owned()), + UserId::Hex("fedcba98".to_owned()), + UserId::Plain("plain_id".to_owned()), ]; // hex_ratio = 9/10 = 0.9 // Code requires > 0.9, not >=, so this should be Ambiguous @@ -245,16 +307,16 @@ mod tests { // Testing near 90% threshold for base64 // 9 base64 users and 1 plain = 90% exactly let user_ids = vec![ - "Y2FmZQ==", // cafe - "Zm9vYmFy", // foobar - "aGVsbG8=", // hello - "d29ybGQ=", // world - "dGVzdA==", // test - "YWJjZA==", // abcd - "eHl6Nzg=", // xyz78 - "cXdlcnQ=", // qwert - "MTIzNDU=", // 12345 - "plain_id", + UserId::Base64("Y2FmZQ==".to_owned()), // cafe + UserId::Base64("Zm9vYmFy".to_owned()), // foobar + UserId::Base64("aGVsbG8=".to_owned()), // hello + UserId::Base64("d29ybGQ=".to_owned()), // world + UserId::Base64("dGVzdA==".to_owned()), // test + UserId::Base64("YWJjZA==".to_owned()), // abcd + UserId::Base64("eHl6Nzg=".to_owned()), // xyz78 + UserId::Base64("cXdlcnQ=".to_owned()), // qwert + UserId::Base64("MTIzNDU=".to_owned()), // 12345 + UserId::Plain("plain_id".to_owned()), ]; // base64_ratio = 9/10 = 0.9 // Code requires > 0.9, not >=, so this should be Ambiguous @@ -265,7 +327,11 @@ mod tests { async fn test_empty_ids() { // Empty IDs should be skipped. Only one non-empty user which is hex. // hex_count=1, total=1 => ratio=1.0 > 0.8 => Hex - let user_ids = vec!["", "", "cafebabe"]; + let user_ids = vec![ + UserId::Plain("".to_owned()), + UserId::Plain("".to_owned()), + UserId::Hex("cafebabe".to_owned()), + ]; run_detection_test(user_ids, ExternalIdEncoding::Hex); } @@ -275,14 +341,14 @@ mod tests { #[tokio::test] async fn test_conversion_hex_to_hex() { - let original_id = "deadbeef"; + let original_id = UserId::Hex("deadbeef".to_owned()); // Expected hex, no changes should be made. run_conversion_test(original_id, ExternalIdEncoding::Hex, "deadbeef"); } #[tokio::test] async fn test_conversion_base64_to_hex() { - let original_id = "Y2FmZQ=="; // "cafe" + let original_id = UserId::Base64("Y2FmZQ==".to_owned()); // "cafe" // Expected base64, we decode base64 => "cafe" and then hex encode the bytes of // "cafe". "cafe" as ASCII: 0x63 0x61 0x66 0x65 in hex is "63616665" @@ -291,7 +357,7 @@ mod tests { #[tokio::test] async fn test_conversion_plain_to_hex() { - let original_id = "plain_id"; + let original_id = UserId::Plain("plain_id".to_owned()); // Expected plain without encoding, so just hex-encode the ASCII. // 'p' = 0x70, 'l' = 0x6c, 'a' = 0x61, 'i' = 0x69, 'n' = 0x6e, '_'=0x5f, // 'i'=0x69, 'd'=0x64 => "706c61696e5f6964" @@ -307,9 +373,9 @@ mod tests { "email@example.com".to_owned(), None, true, - None, - "Y2FmZQ==".to_owned(), // base64 encoded external ID - Some("test.localpart".to_owned()), // localpart should be preserved + "Example User".to_owned(), + "Y2FmZQ==".to_owned(), // base64 encoded external ID + "test.localpart".to_owned(), // localpart should be preserved ); let migrated_user = original_user @@ -319,6 +385,6 @@ mod tests { // External ID should be converted from base64 to hex assert_eq!(migrated_user.get_external_id(), hex::encode("cafe")); // Localpart should remain unchanged - assert_eq!(migrated_user.get_localpart(), Some("test.localpart")); + assert_eq!(migrated_user.get_localpart(), "test.localpart"); } } diff --git a/src/lib.rs b/src/lib.rs index f2b2713..65ec39f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,7 +2,7 @@ use anyhow::{Context, Result}; use futures::{Stream, StreamExt}; use user::User; -use zitadel::Zitadel; +use zitadel::{Zitadel, ZitadelUserBuilder}; mod config; mod sources; @@ -22,29 +22,41 @@ use sources::{csv::CsvSource, ldap::LdapSource, ukt::UktSource, Source}; // TODO: If async closures become a reality, this should be factored // into the `zitadel::search_result_to_user` function pub async fn get_next_zitadel_user( - stream: &mut (impl Stream> + Send + Unpin), + stream: &mut (impl Stream> + Send + Unpin), zitadel: &mut Zitadel, ) -> Result> { match stream.next().await.transpose()? { - Some(mut zitadel_user) => { - let preferred_username = zitadel + Some((zitadel_user, id)) => { + let Some(localpart) = zitadel .zitadel_client - .get_user_metadata(&zitadel_user.1, "preferred_username") + .get_user_metadata(&id, "localpart") .await .ok() - .and_then(|metadata| metadata.metadata().value()); + .and_then(|metadata| metadata.metadata().value()) + else { + tracing::warn!("Zitadel user without a valid localpart: {}", id); + return Box::pin(get_next_zitadel_user(stream, zitadel)).await; + }; - let localpart = zitadel + let Some(preferred_username) = zitadel .zitadel_client - .get_user_metadata(&zitadel_user.1, "localpart") + .get_user_metadata(&id, "preferred_username") .await .ok() - .and_then(|metadata| metadata.metadata().value()); - - zitadel_user.0.preferred_username = preferred_username; - zitadel_user.0.localpart = localpart; - - Ok(Some(zitadel_user)) + .and_then(|metadata| metadata.metadata().value()) + else { + tracing::warn!("Zitadel user without a preferred username: {}", id); + return Box::pin(get_next_zitadel_user(stream, zitadel)).await; + }; + + // We've supplied the required data, so this should never + // error in practice; if it does, that's a bug + let user = zitadel_user + .with_localpart(localpart) + .with_preferred_username(preferred_username) + .build()?; + + Ok(Some((user, id))) } None => Ok(None), } diff --git a/src/sources/csv.rs b/src/sources/csv.rs index 3f116bf..1ac0540 100644 --- a/src/sources/csv.rs +++ b/src/sources/csv.rs @@ -8,7 +8,7 @@ use csv::Reader; use serde::Deserialize; use super::Source; -use crate::user::User; +use crate::user::{self, User}; /// CSV Source pub struct CsvSource { @@ -76,15 +76,21 @@ struct CsvData { impl CsvData { /// Convert CsvData to User data fn to_user(csv_data: CsvData) -> User { + let localpart = if csv_data.localpart.is_empty() { + user::compute_famedly_uuid(csv_data.email.as_bytes()) + } else { + csv_data.localpart + }; + User { email: csv_data.email.clone(), first_name: csv_data.first_name, last_name: csv_data.last_name, phone: if csv_data.phone.is_empty() { None } else { Some(csv_data.phone) }, - preferred_username: Some(csv_data.email.clone()), + preferred_username: csv_data.email.clone(), external_user_id: hex::encode(csv_data.email), enabled: true, - localpart: (!csv_data.localpart.is_empty()).then_some(csv_data.localpart), + localpart, } } } @@ -168,11 +174,7 @@ mod tests { hex::encode("john.doe@example.com".as_bytes()), "Unexpected external_user_id at index 0" ); - assert_eq!( - users[0].localpart, - Some("john.doe".to_owned()), - "Unexpected localpart at index 0" - ); + assert_eq!(users[0].localpart, "john.doe".to_owned(), "Unexpected localpart at index 0"); // Test user without localpart (empty string) assert_eq!(users[1].email, "jane.smith@example.com", "Unexpected email at index 1"); @@ -181,7 +183,11 @@ mod tests { hex::encode("jane.smith@example.com".as_bytes()), "Unexpected external_user_id at index 1" ); - assert_eq!(users[1].localpart, None, "Unexpected localpart at index 1"); + assert_eq!( + users[1].localpart, + user::compute_famedly_uuid("jane.smith@example.com".as_bytes()), + "Unexpected localpart at index 1" + ); // Test user with localpart but no phone assert_eq!(users[2].email, "alice.johnson@example.com", "Unexpected email at index 2"); @@ -192,7 +198,7 @@ mod tests { ); assert_eq!( users[2].localpart, - Some("alice.johnson".to_owned()), + "alice.johnson".to_owned(), "Unexpected localpart at index 2" ); assert_eq!(users[2].phone, None, "Unexpected phone at index 2"); @@ -204,7 +210,11 @@ mod tests { hex::encode("bob.williams@example.com".as_bytes()), "Unexpected external_user_id at index 3" ); - assert_eq!(users[3].localpart, None, "Unexpected localpart at index 3"); + assert_eq!( + users[3].localpart, + user::compute_famedly_uuid("bob.williams@example.com".as_bytes()), + "Unexpected localpart at index 3" + ); assert_eq!(users[3].phone, Some("+4444444444".to_owned()), "Unexpected phone at index 3"); } @@ -287,11 +297,7 @@ mod tests { hex::encode("jane.smith@example.com".as_bytes()), "Unexpected external_user_id at index 0" ); - assert_eq!( - users[0].localpart, - Some("jane.smith".to_owned()), - "Unexpected localpart at index 0" - ); + assert_eq!(users[0].localpart, "jane.smith".to_owned(), "Unexpected localpart at index 0"); } #[test] @@ -315,7 +321,7 @@ mod tests { assert_eq!(users.len(), 2, "Unexpected number of users"); // All users should have None localpart assert!( - users.iter().all(|u| u.localpart.is_none()), + users.iter().all(|u| u.localpart == user::compute_famedly_uuid(u.email.as_bytes())), "Expected all users to have None localpart" ); } diff --git a/src/sources/ldap.rs b/src/sources/ldap.rs index f793351..6a40d03 100644 --- a/src/sources/ldap.rs +++ b/src/sources/ldap.rs @@ -11,7 +11,7 @@ use serde::Deserialize; use url::Url; use super::Source; -use crate::user::User; +use crate::user::{self, User}; /// LDAP sync source pub struct LdapSource { @@ -119,11 +119,17 @@ impl LdapSource { bail!("Binary status without disable_bitmasks"); }; - let ldap_user_id = match read_search_entry(&entry, &self.ldap_config.attributes.user_id)? { - // Use hex encoding instead of base64 for consistent alphabetical order - StringOrBytes::Bytes(byte_id) => hex::encode(byte_id), - StringOrBytes::String(string_id) => hex::encode(string_id.as_bytes()), - }; + let (ldap_user_id, localpart) = + match read_search_entry(&entry, &self.ldap_config.attributes.user_id)? { + // Use hex encoding instead of base64 for consistent alphabetical order + StringOrBytes::Bytes(byte_id) => { + (hex::encode(&byte_id), user::compute_famedly_uuid(&byte_id)) + } + StringOrBytes::String(string_id) => ( + hex::encode(string_id.as_bytes()), + user::compute_famedly_uuid(string_id.as_bytes()), + ), + }; let first_name = read_string_entry(&entry, &self.ldap_config.attributes.first_name, &ldap_user_id)?; @@ -141,12 +147,12 @@ impl LdapSource { Ok(User { first_name, last_name, - preferred_username: Some(preferred_username), + preferred_username, email, external_user_id: ldap_user_id, phone, enabled, - localpart: None, + localpart, }) } } @@ -532,10 +538,10 @@ mod tests { let user = result.unwrap(); assert_eq!(user.first_name, "Test"); assert_eq!(user.last_name, "User"); - assert_eq!(user.preferred_username, Some("testuser".to_owned())); + assert_eq!(user.preferred_username, "testuser".to_owned()); assert_eq!(user.email, "testuser@example.com"); assert_eq!(user.phone, Some("123456789".to_owned())); - assert_eq!(user.preferred_username, Some("testuser".to_owned())); + assert_eq!(user.preferred_username, "testuser".to_owned()); assert_eq!(user.external_user_id, hex::encode("testuser")); assert!(user.enabled); } diff --git a/src/user.rs b/src/user.rs index dec86f5..95c1c8f 100644 --- a/src/user.rs +++ b/src/user.rs @@ -1,8 +1,7 @@ //! User data helpers -use anyhow::{anyhow, Context, Result}; +use anyhow::{Context, Result}; use base64::{engine::general_purpose, Engine as _}; use uuid::{uuid, Uuid}; -use zitadel_rust_client::v2::users::HumanUser; /// The Famedly UUID namespace to use to generate v5 UUIDs. const FAMEDLY_NAMESPACE: Uuid = uuid!("d9979cff-abee-4666-bc88-1ec45a843fb8"); @@ -20,6 +19,12 @@ pub enum ExternalIdEncoding { Ambiguous, } +/// Compute the famedly UUID for a given byte string +#[must_use] +pub fn compute_famedly_uuid(external_id: &[u8]) -> String { + Uuid::new_v5(&FAMEDLY_NAMESPACE, external_id).to_string() +} + /// Source-agnostic representation of a user #[derive(Clone)] pub struct User { @@ -34,11 +39,11 @@ pub struct User { /// Whether the user is enabled pub(crate) enabled: bool, /// The user's preferred username - pub(crate) preferred_username: Option, + pub(crate) preferred_username: String, /// The user's external (non-Zitadel) ID pub(crate) external_user_id: String, /// The user's localpart (used as Zitadel userId) - pub(crate) localpart: Option, + pub(crate) localpart: String, } impl User { @@ -50,9 +55,9 @@ impl User { email: String, phone: Option, enabled: bool, - preferred_username: Option, + preferred_username: String, external_user_id: String, - localpart: Option, + localpart: String, ) -> Self { Self { first_name, @@ -66,40 +71,6 @@ impl User { } } - /// Convert a Zitadel user to our internal representation - pub fn try_from_zitadel_user(user: HumanUser, external_id: String) -> Result { - let first_name = user - .profile() - .and_then(|profile| profile.given_name()) - .ok_or(anyhow!("Missing first name for {}", external_id))? - .clone(); - - let last_name = user - .profile() - .and_then(|profile| profile.family_name()) - .ok_or(anyhow!("Missing last name for {}", external_id))? - .clone(); - - let email = user - .email() - .and_then(|human_email| human_email.email()) - .ok_or(anyhow!("Missing email address for {}", external_id))? - .clone(); - - let phone = user.phone().and_then(|human_phone| human_phone.phone()); - - Ok(Self { - first_name, - last_name, - email, - phone: phone.cloned(), - preferred_username: None, - external_user_id: external_id, - enabled: true, - localpart: None, - }) - } - /// Get a display name for this user #[must_use] pub fn get_display_name(&self) -> String { @@ -108,8 +79,8 @@ impl User { /// Get the localpart #[must_use] - pub fn get_localpart(&self) -> Option<&str> { - self.localpart.as_deref() + pub fn get_localpart(&self) -> &str { + self.localpart.as_ref() } /// Get the external user ID @@ -128,11 +99,6 @@ impl User { hex::decode(&self.external_user_id).context("Invalid external user ID") } - /// Get the famedly UUID of this user - pub fn get_famedly_uuid(&self) -> Result { - Ok(Uuid::new_v5(&FAMEDLY_NAMESPACE, self.get_external_id_bytes()?.as_slice()).to_string()) - } - /// Convert external user ID to a new format based on the detected encoding pub fn create_user_with_converted_external_id( &self, diff --git a/src/zitadel.rs b/src/zitadel.rs index ae205be..9f15181 100644 --- a/src/zitadel.rs +++ b/src/zitadel.rs @@ -1,7 +1,7 @@ //! Helper functions for submitting data to Zitadel use std::path::PathBuf; -use anyhow::{anyhow, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use base64::prelude::{Engine, BASE64_STANDARD}; use futures::{Stream, StreamExt}; use serde::{Deserialize, Serialize}; @@ -70,7 +70,7 @@ impl Zitadel { pub fn get_users_by_email( &mut self, emails: Vec, - ) -> Result> + Send> { + ) -> Result> + Send> { self.zitadel_client .list_users( ListUsersRequest::new(vec![ @@ -92,7 +92,9 @@ impl Zitadel { } /// Return a stream of Zitadel users - pub fn list_users(&mut self) -> Result> + Send> { + pub fn list_users( + &mut self, + ) -> Result> + Send> { self.zitadel_client .list_users( ListUsersRequest::new(vec![ @@ -161,22 +163,12 @@ impl Zitadel { return Ok(()); } - // Use the localpart from the user if available, otherwise generate one - let localpart = if let Some(localpart) = &imported_user.localpart { - localpart.clone() - } else if self.feature_flags.contains(&FeatureFlag::PlainLocalpart) { - String::from_utf8(imported_user.get_external_id_bytes()?) - .context(format!("Unsupported binary external ID for user: {:?}", imported_user))? - } else { - imported_user.get_famedly_uuid()? - }; - - let mut metadata = vec![SetMetadataEntry::new("localpart".to_owned(), localpart.clone())]; - - if let Some(preferred_username) = imported_user.preferred_username.clone() { - metadata - .push(SetMetadataEntry::new("preferred_username".to_owned(), preferred_username)); - } + let mut metadata = + vec![SetMetadataEntry::new("localpart".to_owned(), imported_user.localpart.clone())]; + metadata.push(SetMetadataEntry::new( + "preferred_username".to_owned(), + imported_user.preferred_username.clone(), + )); let mut user = AddHumanUserRequest::new( SetHumanProfile::new(imported_user.first_name.clone(), imported_user.last_name.clone()) @@ -189,7 +181,7 @@ impl Zitadel { Organization::new().with_org_id(self.zitadel_config.organization_id.clone()), ) .with_metadata(metadata) - .with_user_id(localpart); // Set the Zitadel userId to the localpart + .with_user_id(imported_user.localpart.clone()); // Set the Zitadel userId to the localpart if let Some(phone) = imported_user.phone.clone() { user.set_phone( @@ -257,8 +249,8 @@ impl Zitadel { // Check if localpart has changed and emit warning if it has if old_user.localpart != updated_user.localpart { tracing::warn!( - "Cannot update Zitadel userId (localpart) for user {:?} having old localpart: {:?}, and new localpart: {:?}", - old_user, + "Refusing to change user localparts for user {} from {} to {}", + old_user.external_user_id, old_user.localpart, updated_user.localpart ); @@ -324,17 +316,13 @@ impl Zitadel { }; if old_user.preferred_username != updated_user.preferred_username { - if let Some(preferred_username) = updated_user.preferred_username.clone() { - self.zitadel_client - .set_user_metadata( - zitadel_id, - "preferred_username", - &preferred_username.clone(), - ) - .await?; - } else { - self.zitadel_client.delete_user_metadata(zitadel_id, "preferred_username").await?; - } + self.zitadel_client + .set_user_metadata( + zitadel_id, + "preferred_username", + &updated_user.preferred_username.clone(), + ) + .await?; } Ok(()) @@ -342,20 +330,128 @@ impl Zitadel { } /// Convert a Zitadel search result to a user -pub fn search_result_to_user(user: ZitadelUser) -> Result { +pub fn search_result_to_user(user: ZitadelUser) -> Result { let human_user = user.human().ok_or(anyhow!("Machine user found in human user search"))?; - let nick_name = human_user + let external_id = human_user .profile() .and_then(|p| p.nick_name()) .ok_or(anyhow!("Missing external ID found for user"))?; + let first_name = human_user + .profile() + .and_then(|profile| profile.given_name()) + .ok_or(anyhow!("Missing first name for {}", external_id))? + .clone(); + + let last_name = human_user + .profile() + .and_then(|profile| profile.family_name()) + .ok_or(anyhow!("Missing last name for {}", external_id))? + .clone(); + + let email = human_user + .email() + .and_then(|human_email| human_email.email()) + .ok_or(anyhow!("Missing email address for {}", external_id))? + .clone(); + + let phone = human_user.phone().and_then(|human_phone| human_phone.phone()); + // TODO: If async closures become a reality, we // should capture the correct preferred_username and localpart from metadata // here. - let user = User::try_from_zitadel_user(human_user.clone(), nick_name.clone())?; + let user = ZitadelUserBuilder::new( + first_name, + last_name, + email, + external_id.to_owned(), + phone.cloned(), + ); Ok(user) } +/// A builder for a `User` to be used for users gathered from Zitadel +#[derive(Debug)] +pub struct ZitadelUserBuilder { + /// The user's first name + first_name: String, + /// The user's last name + last_name: String, + /// The user's email address + email: String, + /// The user's external ID + external_user_id: String, + + /// The user's preferred username - must be set before building + preferred_username: Option, + /// The user's localpart - must be set before building + localpart: Option, + + /// The user's phone number + phone: Option, +} + +impl ZitadelUserBuilder { + /// Basic constructor + #[must_use] + pub fn new( + first_name: String, + last_name: String, + email: String, + external_user_id: String, + phone: Option, + ) -> Self { + Self { + first_name, + last_name, + email, + external_user_id, + phone, + + preferred_username: None, + localpart: None, + } + } + + /// Set the user's localpart + #[must_use] + pub fn with_localpart(mut self, localpart: String) -> Self { + self.localpart = Some(localpart); + self + } + + /// Set the user's preferred username + #[must_use] + pub fn with_preferred_username(mut self, preferred_username: String) -> Self { + self.preferred_username = Some(preferred_username); + self + } + + /// Build the resulting user struct - this will return an `Err` + /// variant if the localpart or preferred username are missing + pub fn build(self) -> Result { + let Some(localpart) = self.localpart else { + bail!("No valid localpart set"); + }; + + let Some(preferred_username) = self.preferred_username else { + bail!("No valid preferred username set"); + }; + + Ok(User { + first_name: self.first_name, + last_name: self.last_name, + email: self.email, + phone: self.phone, + enabled: true, + external_user_id: self.external_user_id, + + localpart, + preferred_username, + }) + } +} + /// Get a base64-encoded external user ID, if the ID is raw bytes, /// or a UTF-8 string if not. /// diff --git a/tests/e2e.rs b/tests/e2e.rs index d008583..e017328 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -504,6 +504,50 @@ async fn test_e2e_sync_deletion() { assert!(user.is_err_and(|error| matches!(error, ZitadelError::TonicResponseError(status) if status.code() == TonicErrorCode::NotFound))); } +#[test(tokio::test)] +#[test_log(default_log_filter = "debug")] +async fn test_e2e_user_no_localpart_skipped() { + let config = ldap_config().await.clone(); + + // Prepare Zitadel client + let zitadel = open_zitadel_connection().await; + + // Create user in Zitadel + let user = ImportHumanUserRequest { + user_name: "maxmustermann".to_owned(), + profile: Some(Profile { + first_name: "Test".to_owned(), + last_name: "User".to_owned(), + display_name: "User, Test".to_owned(), + gender: Gender::Unspecified.into(), + nick_name: "deadbeef".to_owned(), + preferred_language: String::default(), + }), + email: Some(Email { email: "max@mustermann.de".to_owned(), is_email_verified: true }), + phone: Some(Phone { phone: "+12345678901".to_owned(), is_phone_verified: true }), + password: String::default(), + hashed_password: None, + password_change_required: false, + request_passwordless_registration: false, + otp_code: String::default(), + idps: vec![], + }; + + zitadel + .create_human_user(&config.zitadel.organization_id, user) + .await + .expect("Failed to create user"); + + // Explicitly do not set a localpart for this user + + perform_sync(&config).await.expect("syncing failed"); + zitadel + .get_user_by_login_name("maxmustermann") + .await + .expect("user query failed") + .expect("user should not have been deleted"); +} + #[test(tokio::test)] #[test_log(default_log_filter = "debug")] async fn test_e2e_ldaps() { @@ -1172,11 +1216,31 @@ async fn test_e2e_ukt_sync() { }; let zitadel = open_zitadel_connection().await; - zitadel + let user = zitadel .create_human_user(&config.zitadel.organization_id, user) .await .expect("failed to create user"); + zitadel + .set_user_metadata( + Some(&config.zitadel.organization_id), + user.clone(), + "localpart".to_owned(), + "irrelevant", + ) + .await + .expect("Failed to set user localpart"); + + zitadel + .set_user_metadata( + Some(&config.zitadel.organization_id), + user.clone(), + "preferred_username".to_owned(), + "irrelevant", + ) + .await + .expect("Failed to set user preferred name"); + let user = zitadel .get_user_by_login_name("delete_me@famedly.de") .await @@ -1602,6 +1666,26 @@ async fn test_e2e_migrate_ambiguous_id_as_base64() { .await .expect("Failed to create user"); + zitadel + .set_user_metadata( + Some(&config.zitadel.organization_id), + temp_user.clone(), + "localpart".to_owned(), + "irrelevant", + ) + .await + .expect("Failed to set user localpart"); + + zitadel + .set_user_metadata( + Some(&config.zitadel.organization_id), + temp_user.clone(), + "preferred_username".to_owned(), + "irrelevant", + ) + .await + .expect("Failed to set user preferred name"); + let user_name = "ambiguous_user_two"; // "beefcafe" appears both as a valid hex and base64 @@ -1855,11 +1939,31 @@ async fn run_migration_test( idps: vec![], }; - zitadel + let user_id = zitadel .create_human_user(&config.zitadel.organization_id, user) .await .expect("Failed to create user"); + zitadel + .set_user_metadata( + Some(&config.zitadel.organization_id), + user_id.clone(), + "localpart".to_owned(), + "irrelevant", + ) + .await + .expect("Failed to set user localpart"); + + zitadel + .set_user_metadata( + Some(&config.zitadel.organization_id), + user_id, + "preferred_username".to_owned(), + "irrelevant", + ) + .await + .expect("Failed to set user preferred name"); + // Run migration run_migration_binary(config.feature_flags.contains(&FeatureFlag::DryRun));