ipagroup: Fix management of AD objects

When using AD objects, a user expects to use the more human readable form, like "[email protected]", but this impose some dificulties on evaluating which object is being referenced as AD has several forms to refer to the same object. Each object is AD is identified uniquely by its SID, and this is the identifier that IPA stores in its database. When managing AD objects, IPA finds its SID and works with that value. ansible-freeipa tried to process these objects using the human readable values, and it cause idempontence error when ensuring the values were present or modified, and, at least in some cases, prevented the objects to be made absent, as the object list created didn't match the SID to the value used as module parameter. By using SID to process the AD objects in ipagroup, the addition or removal of members works and idempotence of these members is ensured. The only issue with thils approach is that it only works no server nodes. In client nodes, the conversion to SID is not available and the same issues that existed before will still be present. Tests were updated to reflect these changes, a new test, specific to idempotence issues of AD objects was added: tests/group/test_group_ad_users.yml Resolves: https://issues.redhat.com/browse/RHEL-70023
rjeffman · Jan 29, 2025 · adfb93f · adfb93f
1 parent 8581b79
commit adfb93f
Show file tree

Hide file tree

Showing 6 changed files with 266 additions and 74 deletions.
diff --git a/README-group.md b/README-group.md
@@ -213,7 +213,7 @@ Example playbook to add members from a trusted realm to an external group:
 ---
 - name: Playbook to handle groups.
   hosts: ipaserver
-  
+
   tasks:
   - name: Create an external group and add members from a trust to it.
     ipagroup:
@@ -276,6 +276,11 @@ Example playbook to ensure groups are absent:
       state: absent
 ```
 
+Notes
+=====
+
+* The object to SID mapping is only available when running in an IPA server context, and this may trigger idempotency issues when managing external users and groups using client context.
+
 Variables
 =========
 

diff --git a/plugins/modules/ipagroup.py b/plugins/modules/ipagroup.py
@@ -327,7 +327,8 @@
 from ansible.module_utils._text import to_text
 from ansible.module_utils.ansible_freeipa_module import \
     IPAAnsibleModule, compare_args_ipa, gen_add_del_lists, \
-    gen_add_list, gen_intersection_list, api_check_param
+    gen_add_list, gen_intersection_list, api_check_param, \
+    convert_to_sid
 from ansible.module_utils import six
 if six.PY3:
     unicode = str
@@ -676,6 +677,23 @@ def main():
             # Make sure group exists
             res_find = find_group(ansible_module, name)
 
+            # external members must de handled as SID
+            externalmember = convert_to_sid(externalmember)
+
+            # idoverrides need to be compared through SID
+            idoverrideuser_sid = convert_to_sid(idoverrideuser)
+            res_idoverrideuser_sid = convert_to_sid(
+                (res_find or {}).get("member_idoverrideuser", []))
+            idoverride_set = set(
+                list(zip(idoverrideuser_sid or [], idoverrideuser or [])) +
+                list(
+                    zip(
+                        res_idoverrideuser_sid or [],
+                        (res_find or {}).get("member_idoverrideuser", [])
+                    )
+                )
+            )
+
             user_add, user_del = [], []
             group_add, group_del = [], []
             service_add, service_del = [], []
@@ -723,11 +741,12 @@ def main():
                         res_find = {}
 
                     # if we just created/modified the group, update res_find
-                    res_find.setdefault("objectclass", [])
+                    classes = list(res_find.setdefault("objectclass", []))
                     if external and not is_external_group(res_find):
-                        res_find["objectclass"].append("ipaexternalgroup")
+                        classes.append("ipaexternalgroup")
                     if posix and not is_posix_group(res_find):
-                        res_find["objectclass"].append("posixgroup")
+                        classes.append("posixgroup")
+                    res_find["objectclass"] = classes
 
                     member_args = gen_member_args(
                         user, group, service, externalmember, idoverrideuser
@@ -752,11 +771,23 @@ def main():
                             )
                         )
 
+                        # There are multiple ways to name an AD User, and any
+                        # can be used in idoverrides, so we create the add/del
+                        # lists based on SID, and then use the given user name
+                        # to the idoverride.
                         (idoverrides_add,
                          idoverrides_del) = gen_add_del_lists(
-                            idoverrideuser,
-                            res_find.get("member_idoverrideuser")
-                        )
+                            idoverrideuser_sid, res_idoverrideuser_sid)
+                        idoverrides_add = [
+                            given_name
+                            for sid, given_name in idoverride_set
+                            if sid in set(idoverrides_add)
+                        ]
+                        idoverrides_del = [
+                            given_name
+                            for sid, given_name in idoverride_set
+                            if sid in set(idoverrides_del)
+                        ]
 
                     membermanager_user_add, membermanager_user_del = \
                         gen_add_del_lists(
@@ -790,7 +821,12 @@ def main():
                         )
                     )
                     idoverrides_add = gen_add_list(
-                        idoverrideuser, res_find.get("member_idoverrideuser"))
+                        idoverrideuser_sid, res_idoverrideuser_sid)
+                    idoverrides_add = [
+                        given_name
+                        for sid, given_name in idoverride_set
+                        if sid in set(idoverrides_add)
+                    ]
 
                     membermanager_user_add = gen_add_list(
                         membermanager_user,
@@ -829,7 +865,12 @@ def main():
                         )
                     )
                     idoverrides_del = gen_intersection_list(
-                        idoverrideuser, res_find.get("member_idoverrideuser"))
+                        idoverrideuser_sid, res_idoverrideuser_sid)
+                    idoverrides_del = [
+                        given_name
+                        for sid, given_name in idoverride_set
+                        if sid in set(idoverrides_del)
+                    ]
 
                     membermanager_user_del = gen_intersection_list(
                         membermanager_user, res_find.get("membermanager_user"))
@@ -872,7 +913,7 @@ def main():
                 if len(externalmember_del) > 0:
                     del_member_args["ipaexternalmember"] = \
                         externalmember_del
-            elif externalmember or external:
+            elif externalmember:
                 ansible_module.fail_json(
                     msg="Cannot add external members to a "
                         "non-external group."

diff --git a/tests/group/test_group.yml b/tests/group/test_group.yml
@@ -1,15 +1,18 @@
 ---
 - name: Test group
   hosts: "{{ ipa_test_host | default('ipaserver') }}"
-  become: true
-  gather_facts: true
+  become: false
+  gather_facts: false
   module_defaults:
     ipauser:
       ipaadmin_password: SomeADMINpassword
       ipaapi_context: "{{ ipa_context | default(omit) }}"
     ipagroup:
       ipaadmin_password: SomeADMINpassword
       ipaapi_context: "{{ ipa_context | default(omit) }}"
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      ipaapi_context: "{{ ipa_context | default(omit) }}"
 
   tasks:
   # setup
@@ -51,6 +54,16 @@
     register: result
     failed_when: not result.changed or result.failed
 
+  - name: Ensure test service HTTP is present
+    ipaservice:
+      name: "{{ 'HTTP/' + fqdn_at_domain }}"
+    notify: Cleanup http service
+
+  - name: Ensure test service LDAP is present
+    ipaservice:
+      name: "{{ 'ldap/' + fqdn_at_domain }}"
+    notify: Cleanup ldap service
+
   # TESTS
 
   - name: Ensure group1 is present
@@ -437,3 +450,16 @@
       state: absent
     register: result
     failed_when: not result.changed or result.failed
+
+  # ansible-lint is complaining on the use of 'when' and requiring
+  # the use of handlers.
+  handlers:
+  - name: Cleanup http service
+    ipaservice:
+      name: "{{ 'HTTP/' + fqdn_at_domain }}"
+      state: absent
+
+  - name: Cleanup ldap service
+    ipaservice:
+      name: "{{ 'ldap/' + fqdn_at_domain }}"
+      state: absent
diff --git a/tests/group/test_group_ad_users.yml b/tests/group/test_group_ad_users.yml
@@ -0,0 +1,73 @@
+---
+- name: Test group AD external members idempotence
+  hosts: ipaserver
+  become: false
+  gather_facts: false
+  module_defaults:
+    ipagroup:
+      ipaadmin_password: SomeADMINpassword
+      ipaapi_context: "{{ ipa_context | default(omit) }}"
+
+  vars:
+    ad_user: "{{ test_ad_user | default('AD\\aduser') }}"
+    alt_user: "{{ test_alt_user | default('[email protected]') }}"
+
+  tasks:
+  - name: Include tasks ../env_freeipa_facts.yml
+    ansible.builtin.include_tasks: ../env_freeipa_facts.yml
+
+  - name: Ensure test group is absent.
+    ipagroup:
+      name: extgroup
+      state: absent
+
+  - name: Execute group tests if trust test environment is supported
+    when: trust_test_is_supported | default(false)
+    block:
+      - name: Ensure external group, with AD users, is present.
+        ipagroup:
+          name: extgroup
+          external: true
+          external_member: "{{ ad_user }}"
+        register: result
+        failed_when: result.failed or not result.changed
+
+      - name: Ensure external group, with AD users, is present, again
+        ipagroup:
+          name: extgroup
+          external: true
+          external_member: "{{ ad_user }}"
+        register: result
+        failed_when: result.failed or result.changed
+
+      - name: Ensure external group, with alternate name AD users, is present
+        ipagroup:
+          name: extgroup
+          external: true
+          external_member: "{{ alt_user }}"
+        register: result
+        failed_when: result.failed or result.changed
+
+      - name: Ensure external_member is absent
+        ipagroup:
+          name: extgroup
+          external_member: "{{ ad_user }}"
+          action: member
+          state: absent
+        register: result
+        failed_when: result.failed or not result.changed
+
+      - name: Ensure external_member is absent, again
+        ipagroup:
+          name: extgroup
+          external_member: "{{ alt_user }}"
+          action: member
+          state: absent
+        register: result
+        failed_when: result.failed or result.changed
+
+    always:
+      - name: Cleanup environment.
+        ipagroup:
+          name: extgroup
+          state: absent