Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add localpart to CSV import #100

Merged
merged 2 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/bin/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ mod tests {
true,
None,
external_user_id.to_owned(),
None,
)
}

Expand Down Expand Up @@ -296,4 +297,28 @@ mod tests {
// 'i'=0x69, 'd'=0x64 => "706c61696e5f6964"
run_conversion_test(original_id, ExternalIdEncoding::Plain, "706c61696e5f6964");
}

#[tokio::test]
async fn test_localpart_preservation() {
// Test that migration preserves localpart values
let original_user = SyncUser::new(
"first name".to_owned(),
"last name".to_owned(),
"[email protected]".to_owned(),
None,
true,
None,
"Y2FmZQ==".to_owned(), // base64 encoded external ID
Some("test.localpart".to_owned()), // localpart should be preserved
);

let migrated_user = original_user
.create_user_with_converted_external_id(ExternalIdEncoding::Base64)
.expect("Should successfully convert user");

// 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"));
}
}
8 changes: 8 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,15 @@ pub async fn get_next_zitadel_user(
.ok()
.and_then(|metadata| metadata.metadata().value());

let localpart = zitadel
.zitadel_client
.get_user_metadata(&zitadel_user.1, "localpart")
.await
.ok()
.and_then(|metadata| metadata.metadata().value());

zitadel_user.0.preferred_username = preferred_username;
zitadel_user.0.localpart = localpart;

Ok(Some(zitadel_user))
}
Expand Down
102 changes: 92 additions & 10 deletions src/sources/csv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@
last_name: String,
/// The user's phone number
phone: String,
/// The user's localpart (optional)
#[serde(default)]
localpart: String,
}

impl CsvData {
Expand All @@ -81,6 +84,7 @@
preferred_username: Some(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),
}
}
}
Expand Down Expand Up @@ -139,11 +143,11 @@
fn test_get_users() {
let mut config = load_config();
let csv_content = indoc! {r#"
email,first_name,last_name,phone
[email protected],John,Doe,+1111111111
[email protected],Jane,Smith,+2222222222
[email protected],Alice,Johnson,
[email protected],Bob,Williams,+4444444444
email,first_name,last_name,phone,localpart
[email protected],John,Doe,+1111111111,john.doe
[email protected],Jane,Smith,+2222222222,
[email protected],Alice,Johnson,,alice.johnson
[email protected],Bob,Williams,+4444444444,
"#};
let _file = test_helpers::temp_csv_file(&mut config, csv_content);

Expand All @@ -155,18 +159,60 @@

let users = result.expect("Failed to get users");
assert_eq!(users.len(), 4, "Unexpected number of users");

// Test user with localpart
assert_eq!(users[0].first_name, "John", "Unexpected first name at index 0");
assert_eq!(users[0].email, "[email protected]", "Unexpected email at index 0");
assert_eq!(users[3].last_name, "Williams", "Unexpected last name at index 3");
assert_eq!(
users[0].external_user_id,
hex::encode("[email protected]".as_bytes()),
"Unexpected external_user_id at index 0"

Check warning on line 169 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L169

Added line #L169 was not covered by tests
);
assert_eq!(
users[0].localpart,
Some("john.doe".to_owned()),
"Unexpected localpart at index 0"

Check warning on line 174 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L174

Added line #L174 was not covered by tests
);

// Test user without localpart (empty string)
assert_eq!(users[1].email, "[email protected]", "Unexpected email at index 1");
assert_eq!(
users[1].external_user_id,
hex::encode("[email protected]".as_bytes()),
"Unexpected external_user_id at index 1"

Check warning on line 182 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L182

Added line #L182 was not covered by tests
);
assert_eq!(users[1].localpart, None, "Unexpected localpart at index 1");

// Test user with localpart but no phone
assert_eq!(users[2].email, "[email protected]", "Unexpected email at index 2");
assert_eq!(
users[2].external_user_id,
hex::encode("[email protected]".as_bytes()),
"Unexpected external_user_id at index 2"

Check warning on line 191 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L191

Added line #L191 was not covered by tests
);
assert_eq!(
users[2].localpart,
Some("alice.johnson".to_owned()),
"Unexpected localpart at index 2"

Check warning on line 196 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L196

Added line #L196 was not covered by tests
);
assert_eq!(users[2].phone, None, "Unexpected phone at index 2");

// Test user without localpart (empty string) but with phone
assert_eq!(users[3].email, "[email protected]", "Unexpected email at index 3");
assert_eq!(
users[3].external_user_id,
hex::encode("[email protected]".as_bytes()),
"Unexpected external_user_id at index 3"

Check warning on line 205 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L205

Added line #L205 was not covered by tests
);
assert_eq!(users[3].localpart, None, "Unexpected localpart at index 3");
assert_eq!(users[3].phone, Some("+4444444444".to_owned()), "Unexpected phone at index 3");
}

#[test]
fn test_get_users_empty_file() {
let mut config = load_config();
let csv_content = indoc! {r#"
email,first_name,last_name,phone
email,first_name,last_name,phone,localpart
"#};
let _file = test_helpers::temp_csv_file(&mut config, csv_content);

Expand Down Expand Up @@ -204,7 +250,7 @@
let mut config = load_config();
let csv_content = indoc! {r#"
first_name
[email protected],John,Doe,+1111111111
[email protected],John,Doe,+1111111111,john.doe
"#};
let _file = test_helpers::temp_csv_file(&mut config, csv_content);

Expand All @@ -220,9 +266,9 @@
fn test_get_users_invalid_content() {
let mut config = load_config();
let csv_content = indoc! {r#"
email,first_name,last_name,phone
email,first_name,last_name,phone,localpart
[email protected]
[email protected],Jane,Smith,+2222222222
[email protected],Jane,Smith,+2222222222,jane.smith
"#};
let _file = test_helpers::temp_csv_file(&mut config, csv_content);

Expand All @@ -236,5 +282,41 @@
assert_eq!(users.len(), 1, "Unexpected number of users");
assert_eq!(users[0].email, "[email protected]", "Unexpected email at index 0");
assert_eq!(users[0].last_name, "Smith", "Unexpected last name at index 0");
assert_eq!(
users[0].external_user_id,
hex::encode("[email protected]".as_bytes()),
"Unexpected external_user_id at index 0"

Check warning on line 288 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L288

Added line #L288 was not covered by tests
);
assert_eq!(
users[0].localpart,
Some("jane.smith".to_owned()),
"Unexpected localpart at index 0"

Check warning on line 293 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L293

Added line #L293 was not covered by tests
);
}

#[test]
fn test_backward_compatibility() {
// Test that old CSV format without localpart column still works
let mut config = load_config();
let csv_content = indoc! {r#"
email,first_name,last_name,phone
[email protected],John,Doe,+1111111111
[email protected],Jane,Smith,+2222222222
"#};
let _file = test_helpers::temp_csv_file(&mut config, csv_content);

let csv_config = config.sources.csv.expect("CsvSource configuration is missing");
let csv = CsvSource::new(csv_config);

let result = csv.read_csv();
assert!(result.is_ok(), "Failed to get users: {:?}", result);

let users = result.expect("Failed to get users");
assert_eq!(users.len(), 2, "Unexpected number of users");
// All users should have None localpart
assert!(
users.iter().all(|u| u.localpart.is_none()),
"Expected all users to have None localpart"

Check warning on line 319 in src/sources/csv.rs

View check run for this annotation

Codecov / codecov/patch

src/sources/csv.rs#L319

Added line #L319 was not covered by tests
);
}
}
1 change: 1 addition & 0 deletions src/sources/ldap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ impl LdapSource {
external_user_id: ldap_user_id,
phone,
enabled,
localpart: None,
})
}
}
Expand Down
25 changes: 23 additions & 2 deletions src/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,13 @@ pub struct User {
pub(crate) preferred_username: Option<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<String>,
}

impl User {
/// Create a new user instance, used in tests
#[allow(clippy::must_use_candidate)]
#[allow(clippy::must_use_candidate, clippy::too_many_arguments)]
pub fn new(
first_name: String,
last_name: String,
Expand All @@ -50,8 +52,18 @@ impl User {
enabled: bool,
preferred_username: Option<String>,
external_user_id: String,
localpart: Option<String>,
) -> Self {
Self { first_name, last_name, email, phone, enabled, preferred_username, external_user_id }
Self {
first_name,
last_name,
email,
phone,
enabled,
preferred_username,
external_user_id,
localpart,
}
}

/// Convert a Zitadel user to our internal representation
Expand Down Expand Up @@ -84,6 +96,7 @@ impl User {
preferred_username: None,
external_user_id: external_id,
enabled: true,
localpart: None,
})
}

Expand All @@ -93,6 +106,12 @@ impl User {
format!("{}, {}", self.last_name, self.first_name)
}

/// Get the localpart
#[must_use]
pub fn get_localpart(&self) -> Option<&str> {
self.localpart.as_deref()
}

/// Get the external user ID
#[must_use]
pub fn get_external_id(&self) -> &str {
Expand Down Expand Up @@ -210,6 +229,7 @@ impl PartialEq for User {
&& self.enabled == other.enabled
&& self.preferred_username == other.preferred_username
&& self.external_user_id == other.external_user_id
&& self.localpart == other.localpart
}
}

Expand All @@ -222,6 +242,7 @@ impl std::fmt::Debug for User {
.field("phone", &"***")
.field("preferred_username", &"***")
.field("external_user_id", &self.external_user_id)
.field("localpart", &self.localpart)
.field("enabled", &self.enabled)
.finish()
}
Expand Down
22 changes: 18 additions & 4 deletions src/zitadel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,17 @@ impl Zitadel {
return Ok(());
}

let localpart = if self.feature_flags.contains(&FeatureFlag::PlainLocalpart) {
// 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)];
let mut metadata = vec![SetMetadataEntry::new("localpart".to_owned(), localpart.clone())];

if let Some(preferred_username) = imported_user.preferred_username.clone() {
metadata
Expand All @@ -185,7 +188,8 @@ impl Zitadel {
.with_organization(
Organization::new().with_org_id(self.zitadel_config.organization_id.clone()),
)
.with_metadata(metadata);
.with_metadata(metadata)
.with_user_id(localpart); // Set the Zitadel userId to the localpart

if let Some(phone) = imported_user.phone.clone() {
user.set_phone(
Expand Down Expand Up @@ -250,6 +254,16 @@ impl Zitadel {
updated_user.external_user_id
);

// 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,
old_user.localpart,
updated_user.localpart
);
}

if self.feature_flags.is_enabled(FeatureFlag::DryRun) {
tracing::warn!("Skipping update due to dry run");
return Ok(());
Expand Down Expand Up @@ -336,7 +350,7 @@ pub fn search_result_to_user(user: ZitadelUser) -> Result<User> {
.ok_or(anyhow!("Missing external ID found for user"))?;

// TODO: If async closures become a reality, we
// should capture the correct preferred_username
// should capture the correct preferred_username and localpart from metadata
// here.
let user = User::try_from_zitadel_user(human_user.clone(), nick_name.clone())?;
Ok(user)
Expand Down
Loading
Loading