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

@@ -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<ScimEntry, _> = 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<ScimEntry, _> = person.try_into();
 

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<String>) -> 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<String>,
     pub gidnumber: Option<u32>,
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
     pub member: Vec<ScimExternalMember>,
 }
 
@@ -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<String>) -> Self {
-        self.inner.entry.external_id = external_id;
-        self
-    }
-
     pub fn build(self) -> ScimSyncGroup {
         self.inner
     }

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",

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| {

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| {