From 9f499f3913485eef4d3afe49c1c203533af33a6c Mon Sep 17 00:00:00 2001 From: Firstyear Date: Fri, 20 Dec 2024 17:16:07 +1000 Subject: [PATCH] Further SCIM sync testing, minor fixes (#3305) This adds further testing of SCIM sync, especially around conversion of the SCIM Sync Person and Group types into SCIM Entry. This test would have prevented #3298 and #3299 from occuring. During testing two more fixes were found. external_id should have been required (not optional) and a group with no members would cause a serialisation issue. --- proto/src/scim_v1/mod.rs | 70 +++++++++-------- proto/src/scim_v1/synch.rs | 24 +++--- server/lib/src/idm/scim.rs | 98 +++++++++++++++++++++++- tools/iam_migrations/freeipa/src/main.rs | 9 +-- tools/iam_migrations/ldap/src/main.rs | 9 +-- 5 files changed, 146 insertions(+), 64 deletions(-) diff --git a/proto/src/scim_v1/mod.rs b/proto/src/scim_v1/mod.rs index 93333e69f..76b6c807d 100644 --- a/proto/src/scim_v1/mod.rs +++ b/proto/src/scim_v1/mod.rs @@ -120,12 +120,15 @@ mod tests { // Group let group_uuid = uuid::uuid!("2d0a9e7c-cc08-4ca2-8d7f-114f9abcfc8a"); - let group = ScimSyncGroup::builder("testgroup".to_string(), group_uuid) - .set_description(Some("test desc".to_string())) - .set_gidnumber(Some(12345)) - .set_members(vec!["member_a".to_string(), "member_a".to_string()].into_iter()) - .set_external_id(Some("cn=testgroup".to_string())) - .build(); + let group = ScimSyncGroup::builder( + group_uuid, + "cn=testgroup".to_string(), + "testgroup".to_string(), + ) + .set_description(Some("test desc".to_string())) + .set_gidnumber(Some(12345)) + .set_members(vec!["member_a".to_string(), "member_a".to_string()].into_iter()) + .build(); let entry: Result = group.try_into(); @@ -136,32 +139,35 @@ mod tests { let user_sshkey = "sk-ecdsa-sha2-nistp256@openssh.com AAAAInNrLWVjZHNhLXNoYTItbmlzdHAyNTZAb3BlbnNzaC5jb20AAAAIbmlzdHAyNTYAAABBBENubZikrb8hu+HeVRdZ0pp/VAk2qv4JDbuJhvD0yNdWDL2e3cBbERiDeNPkWx58Q4rVnxkbV1fa8E2waRtT91wAAAAEc3NoOg== testuser@fidokey"; - let person = - ScimSyncPerson::builder(user_uuid, "testuser".to_string(), "Test User".to_string()) - .set_password_import(Some("new_password".to_string())) - .set_unix_password_import(Some("new_password".to_string())) - .set_totp_import(vec![ScimTotp { - external_id: "Totp".to_string(), - secret: "abcd".to_string(), - algo: "SHA3".to_string(), - step: 60, - digits: 8, - }]) - .set_mail(vec![MultiValueAttr { - primary: Some(true), - value: "testuser@example.com".to_string(), - ..Default::default() - }]) - .set_ssh_publickey(vec![ScimSshPubKey { - label: "Key McKeyface".to_string(), - value: user_sshkey.to_string(), - }]) - .set_login_shell(Some("/bin/false".to_string())) - .set_account_valid_from(Some("2023-11-28T04:57:55Z".to_string())) - .set_account_expire(Some("2023-11-28T04:57:55Z".to_string())) - .set_gidnumber(Some(54321)) - .set_external_id(Some("cn=testuser".to_string())) - .build(); + let person = ScimSyncPerson::builder( + user_uuid, + "cn=testuser".to_string(), + "testuser".to_string(), + "Test User".to_string(), + ) + .set_password_import(Some("new_password".to_string())) + .set_unix_password_import(Some("new_password".to_string())) + .set_totp_import(vec![ScimTotp { + external_id: "Totp".to_string(), + secret: "abcd".to_string(), + algo: "SHA3".to_string(), + step: 60, + digits: 8, + }]) + .set_mail(vec![MultiValueAttr { + primary: Some(true), + value: "testuser@example.com".to_string(), + ..Default::default() + }]) + .set_ssh_publickey(vec![ScimSshPubKey { + label: "Key McKeyface".to_string(), + value: user_sshkey.to_string(), + }]) + .set_login_shell(Some("/bin/false".to_string())) + .set_account_valid_from(Some("2023-11-28T04:57:55Z".to_string())) + .set_account_expire(Some("2023-11-28T04:57:55Z".to_string())) + .set_gidnumber(Some(54321)) + .build(); let entry: Result = person.try_into(); diff --git a/proto/src/scim_v1/synch.rs b/proto/src/scim_v1/synch.rs index ac359c5da..30eeee9c7 100644 --- a/proto/src/scim_v1/synch.rs +++ b/proto/src/scim_v1/synch.rs @@ -119,7 +119,12 @@ pub struct ScimSyncPersonBuilder { } impl ScimSyncPerson { - pub fn builder(id: Uuid, name: String, displayname: String) -> ScimSyncPersonBuilder { + pub fn builder( + id: Uuid, + external_id: String, + name: String, + displayname: String, + ) -> ScimSyncPersonBuilder { ScimSyncPersonBuilder { inner: ScimSyncPerson { entry: ScimEntryHeader { @@ -128,7 +133,7 @@ impl ScimSyncPerson { SCIM_SCHEMA_SYNC_PERSON.to_string(), ], id, - external_id: None, + external_id: Some(external_id), meta: None, }, name, @@ -205,11 +210,6 @@ impl ScimSyncPersonBuilder { self } - pub fn set_external_id(mut self, external_id: Option) -> Self { - self.inner.entry.external_id = external_id; - self - } - pub fn build(self) -> ScimSyncPerson { self.inner } @@ -230,6 +230,7 @@ pub struct ScimSyncGroup { pub name: String, pub description: Option, pub gidnumber: Option, + #[serde(default, skip_serializing_if = "Vec::is_empty")] pub member: Vec, } @@ -248,13 +249,13 @@ pub struct ScimSyncGroupBuilder { } impl ScimSyncGroup { - pub fn builder(name: String, id: Uuid) -> ScimSyncGroupBuilder { + pub fn builder(id: Uuid, external_id: String, name: String) -> ScimSyncGroupBuilder { ScimSyncGroupBuilder { inner: ScimSyncGroup { entry: ScimEntryHeader { schemas: vec![SCIM_SCHEMA_SYNC_GROUP.to_string()], id, - external_id: None, + external_id: Some(external_id), meta: None, }, name, @@ -295,11 +296,6 @@ impl ScimSyncGroupBuilder { self } - pub fn set_external_id(mut self, external_id: Option) -> Self { - self.inner.entry.external_id = external_id; - self - } - pub fn build(self) -> ScimSyncGroup { self.inner } diff --git a/server/lib/src/idm/scim.rs b/server/lib/src/idm/scim.rs index f127e375a..65eb3eeb8 100644 --- a/server/lib/src/idm/scim.rs +++ b/server/lib/src/idm/scim.rs @@ -1584,10 +1584,6 @@ mod tests { assert!(matches!(sync_state, ScimSyncState::Refresh)); drop(idms_prox_read); - - // Use the current state and update. - - // TODO!!! } #[idm_test] @@ -3167,6 +3163,100 @@ mod tests { assert!(idms_prox_write.commit().is_ok()); } + #[idm_test] + /// Assert that a SCIM JSON proto entry correctly serialises and deserialises + /// and can be applied as a changeset. This serialisation is performed during + /// the ScimEntry::try_from step. + async fn test_idm_scim_sync_json_proto(idms: &IdmServer, _idms_delayed: &mut IdmServerDelayed) { + let ct = Duration::from_secs(TEST_CURRENT_TIME); + let mut idms_prox_write = idms.proxy_write(ct).await.unwrap(); + let (_sync_uuid, ident) = test_scim_sync_apply_setup_ident(&mut idms_prox_write, ct); + let sse = ScimSyncUpdateEvent { ident }; + + // Minimum Viable Person + let person_1 = ScimSyncPerson::builder( + Uuid::new_v4(), + "cn=testperson_1".to_string(), + "testperson_1".to_string(), + "Test Person One".to_string(), + ) + .build() + .try_into() + .unwrap(); + + // Minimum Viable Group + let group_1 = ScimSyncGroup::builder( + Uuid::new_v4(), + "cn=testgroup_1".to_string(), + "testgroup_1".to_string(), + ) + .build() + .try_into() + .unwrap(); + + let user_sshkey = "sk-ecdsa-sha2-nistp256@openssh.com AAAAInNrLWVjZHNhLXNoYTItbmlzdHAyNTZAb3BlbnNzaC5jb20AAAAIbmlzdHAyNTYAAABBBENubZikrb8hu+HeVRdZ0pp/VAk2qv4JDbuJhvD0yNdWDL2e3cBbERiDeNPkWx58Q4rVnxkbV1fa8E2waRtT91wAAAAEc3NoOg== testuser@fidokey"; + + // All Attribute Person + let person_2 = ScimSyncPerson::builder( + Uuid::new_v4(), + "cn=testperson_2".to_string(), + "testperson_2".to_string(), + "Test Person Two".to_string(), + ) + .set_password_import(Some("ipaNTHash: iEb36u6PsRetBr3YMLdYbA".to_string())) + .set_unix_password_import(Some("ipaNTHash: iEb36u6PsRetBr3YMLdYbA".to_string())) + .set_totp_import(vec![ScimTotp { + external_id: "Totp".to_string(), + secret: "QICWZTON72IBS5MXWNURKAONC3JNOOOFMLKNRTIPXBYQ4BLRSEBM7KF5".to_string(), + algo: "sha256".to_string(), + step: 60, + digits: 8, + }]) + .set_mail(vec![MultiValueAttr { + primary: Some(true), + value: "testuser@example.com".to_string(), + ..Default::default() + }]) + .set_ssh_publickey(vec![ScimSshPubKey { + label: "Key McKeyface".to_string(), + value: user_sshkey.to_string(), + }]) + .set_login_shell(Some("/bin/zsh".to_string())) + .set_account_valid_from(Some("2023-11-28T04:57:55Z".to_string())) + .set_account_expire(Some("2023-11-28T04:57:55Z".to_string())) + .set_gidnumber(Some(12346)) + .build() + .try_into() + .unwrap(); + + // All Attribute Group + let group_2 = ScimSyncGroup::builder( + Uuid::new_v4(), + "cn=testgroup_2".to_string(), + "testgroup_2".to_string(), + ) + .set_description(Some("description".to_string())) + .set_gidnumber(Some(12345)) + .set_members(vec!["cn=testperson_1".to_string(), "cn=testperson_2".to_string()].into_iter()) + .build() + .try_into() + .unwrap(); + + let entries = vec![person_1, group_1, person_2, group_2]; + + let changes = ScimSyncRequest { + from_state: ScimSyncState::Refresh, + to_state: ScimSyncState::Active { + cookie: vec![1, 2, 3, 4], + }, + entries, + retain: ScimSyncRetentionMode::Ignore, + }; + + assert!(idms_prox_write.scim_sync_apply(&sse, &changes, ct).is_ok()); + assert!(idms_prox_write.commit().is_ok()); + } + const TEST_SYNC_SCIM_IPA_1: &str = r#" { "from_state": "Refresh", diff --git a/tools/iam_migrations/freeipa/src/main.rs b/tools/iam_migrations/freeipa/src/main.rs index 24f621c45..7e682b69c 100644 --- a/tools/iam_migrations/freeipa/src/main.rs +++ b/tools/iam_migrations/freeipa/src/main.rs @@ -926,9 +926,8 @@ fn ipa_to_scim_entry( let account_valid_from = None; let login_shell = entry.remove_ava_single(Attribute::LoginShell.as_ref()); - let external_id = Some(entry.dn); - let scim_sync_person = ScimSyncPerson::builder(id, user_name, display_name) + let scim_sync_person = ScimSyncPerson::builder(id, entry.dn, user_name, display_name) .set_gidnumber(gidnumber) .set_password_import(password_import) .set_unix_password_import(unix_password_import) @@ -938,7 +937,6 @@ fn ipa_to_scim_entry( .set_ssh_publickey(ssh_publickey) .set_account_expire(account_expire) .set_account_valid_from(account_valid_from) - .set_external_id(external_id) .build(); let scim_entry_generic: ScimEntry = scim_sync_person.try_into().map_err(|json_err| { @@ -983,13 +981,10 @@ fn ipa_to_scim_entry( .map(|set| set.into_iter().collect()) .unwrap_or_default(); - let external_id = Some(entry.dn); - - let scim_sync_group = ScimSyncGroup::builder(name, id) + let scim_sync_group = ScimSyncGroup::builder(id, entry.dn, name) .set_description(description) .set_gidnumber(gidnumber) .set_members(members.into_iter()) - .set_external_id(external_id) .build(); let scim_entry_generic: ScimEntry = scim_sync_group.try_into().map_err(|json_err| { diff --git a/tools/iam_migrations/ldap/src/main.rs b/tools/iam_migrations/ldap/src/main.rs index 05e939632..8e5d52894 100644 --- a/tools/iam_migrations/ldap/src/main.rs +++ b/tools/iam_migrations/ldap/src/main.rs @@ -617,9 +617,8 @@ fn ldap_to_scim_entry( let login_shell = entry .get_ava_single(&sync_config.person_attr_login_shell) .map(str::to_string); - let external_id = Some(entry.dn); - let scim_sync_person = ScimSyncPerson::builder(id, user_name, display_name) + let scim_sync_person = ScimSyncPerson::builder(id, entry.dn, user_name, display_name) .set_gidnumber(gidnumber) .set_password_import(password_import) .set_unix_password_import(unix_password_import) @@ -629,7 +628,6 @@ fn ldap_to_scim_entry( .set_ssh_publickey(ssh_publickey) .set_account_expire(account_expire) .set_account_valid_from(account_valid_from) - .set_external_id(external_id) .build(); let scim_entry_generic: ScimEntry = scim_sync_person.try_into().map_err(|json_err| { @@ -678,13 +676,10 @@ fn ldap_to_scim_entry( .map(|set| set.into_iter().collect()) .unwrap_or_default(); - let external_id = Some(entry.dn); - - let scim_sync_group = ScimSyncGroup::builder(name, id) + let scim_sync_group = ScimSyncGroup::builder(id, entry.dn, name) .set_description(description) .set_gidnumber(gidnumber) .set_members(members.into_iter()) - .set_external_id(external_id) .build(); let scim_entry_generic: ScimEntry = scim_sync_group.try_into().map_err(|json_err| {