ipaprivilege: Fix module execution in check_mode.

This patch removes the custom command result handler in favor of the
IPAAnsibleModule default member handler and fixes creation of add and
del lists of permissions, fixing the behavior of the moudule execution
when 'check_mode: yes'.

plugins/modules/ipaprivilege.py

@@ -108,7 +108,8 @@ RETURN = """
 
 
 from ansible.module_utils.ansible_freeipa_module import \
-    IPAAnsibleModule, compare_args_ipa, gen_add_del_lists
+    IPAAnsibleModule, compare_args_ipa, gen_add_del_lists, gen_add_list, \
+    gen_intersection_list
 import six
 
 if six.PY3:
@@ -126,22 +127,6 @@ def find_privilege(module, name):
         return _result["result"]
 
 
-# pylint: disable=unused-argument
-def result_handler(module, result, command, name, args, errors):
-    # Get all errors
-    # All "already a member" and "not a member" failures in the
-    # result are ignored. All others are reported.
-    for failed_item in result.get("failed", []):
-        failed = result["failed"][failed_item]
-        for member_type in failed:
-            for member, failure in failed[member_type]:
-                if "already a member" in failure \
-                   or "not a member" in failure:
-                    continue
-                errors.append("%s: %s %s: %s" % (
-                    command, member_type, member, failure))
-
-
 def main():
     ansible_module = IPAAnsibleModule(
         argument_spec=dict(
@@ -230,48 +215,31 @@ def main():
                 if action == "privilege":
                     # Found the privilege
                     if res_find is not None:
-                        res_cmp = {
+                        cmp = {"description": res_find.get("description")}
-                            k: v for k, v in res_find.items()
+                        if not compare_args_ipa(ansible_module, args, cmp):
-                            if k not in [
-                                "objectclass", "cn", "dn",
-                                "memberof_permisssion"
-                            ]
-                        }
-                        # For all settings is args, check if there are
-                        # different settings in the find result.
-                        # If yes: modify
-                        if args and not compare_args_ipa(ansible_module, args,
-                                                         res_cmp):
                             commands.append([name, "privilege_mod", args])
                     else:
                         commands.append([name, "privilege_add", args])
                         res_find = {}
 
-                    member_args = {}
+                    # Generate addition and removal lists
-                    if permission:
+                    permission_add, permission_del = gen_add_del_lists(
-                        member_args['permission'] = permission
+                        permission, res_find.get("memberof_permission")
+                    )
 
-                    if not compare_args_ipa(ansible_module, member_args,
+                    # Add members
-                                            res_find):
+                    if len(permission_add) > 0:
-
+                        commands.append([name, "privilege_add_permission",
-                        # Generate addition and removal lists
+                                         {
-                        permission_add, permission_del = gen_add_del_lists(
+                                             "permission": permission_add,
-                            permission, res_find.get("memberof_permission")
+                                         }])
-                        )
+                    # Remove members
-
+                    if len(permission_del) > 0:
-                        # Add members
+                        commands.append([
-                        if len(permission_add) > 0:
+                            name,
-                            commands.append([name, "privilege_add_permission",
+                            "privilege_remove_permission",
-                                             {
+                            {"permission": permission_del}
-                                                 "permission": permission_add,
+                        ])
-                                             }])
-                        # Remove members
-                        if len(permission_del) > 0:
-                            commands.append([
-                                name,
-                                "privilege_remove_permission",
-                                {"permission": permission_del}
-                            ])
 
                 elif action == "member":
                     if res_find is None:
@@ -281,8 +249,11 @@ def main():
                     if permission is None:
                         ansible_module.fail_json(msg="No permission given")
 
-                    commands.append([name, "privilege_add_permission",
+                    permission = gen_add_list(
-                                     {"permission": permission}])
+                        permission, res_find.get("memberof_permission"))
+                    if permission:
+                        commands.append([name, "privilege_add_permission",
+                                         {"permission": permission}])
 
             elif state == "absent":
                 if action == "privilege":
@@ -297,10 +268,11 @@ def main():
                     if permission is None:
                         ansible_module.fail_json(msg="No permission given")
 
-                    commands.append([name, "privilege_remove_permission",
+                    permission = gen_intersection_list(
-                                     {
+                        permission, res_find.get("memberof_permission"))
-                                         "permission": permission,
+                    if permission:
-                                     }])
+                        commands.append([name, "privilege_remove_permission",
+                                         {"permission": permission}])
 
             elif state == "renamed":
                 if not rename:
@@ -319,7 +291,8 @@ def main():
 
         # Execute commands
 
-        changed = ansible_module.execute_ipa_commands(commands, result_handler)
+        changed = ansible_module.execute_ipa_commands(
+            commands, fail_on_member_errors=True)
 
     # Done
 

tests/privilege/test_privilege.yml

@@ -47,6 +47,20 @@
     register: result
     failed_when: not result.changed or result.failed
 
+  - name: Check if adding member permissions would cause a change (issue 669).
+    ipaprivilege:
+      ipaadmin_password: SomeADMINpassword
+      ipaapi_context: "{{ ipa_context | default(omit) }}"
+      name: Broad Privilege
+      permission:
+      - "Write IPA Configuration"
+      - "System: Write DNS Configuration"
+      - "System: Update DNS Entries"
+      action: member
+    check_mode: yes
+    register: result
+    failed_when: not result.changed or result.failed
+
   - name: Ensure privilege Broad Privilege has permissions
     ipaprivilege:
       ipaadmin_password: SomeADMINpassword
@@ -73,6 +87,20 @@
     register: result
     failed_when: result.changed or result.failed
 
+  - name: Check if existing member permissions would not cause a change (issue 669).
+    ipaprivilege:
+      ipaadmin_password: SomeADMINpassword
+      ipaapi_context: "{{ ipa_context | default(omit) }}"
+      name: Broad Privilege
+      permission:
+      - "Write IPA Configuration"
+      - "System: Write DNS Configuration"
+      - "System: Update DNS Entries"
+      action: member
+    check_mode: yes
+    register: result
+    failed_when: result.changed or result.failed
+
   - name: Ensure privilege Broad Privilege member permission "Write IPA Configuration" is absent
     ipaprivilege:
       ipaadmin_password: SomeADMINpassword
@@ -161,6 +189,17 @@
       name: Broad Privilege
       state: absent
 
+  - name: Check if creating privilege Broad Privilege with permission would issue a change. (issue 669)
+    ipaprivilege:
+      ipaadmin_password: SomeADMINpassword
+      ipaapi_context: "{{ ipa_context | default(omit) }}"
+      name: Broad Privilege
+      permission:
+      - "Write IPA Configuration"
+    check_mode: yes
+    register: result
+    failed_when: not result.changed or result.failed
+
   - name: Ensure privilege Broad Privilege is created with permission. (issue 529)
     ipaprivilege:
       ipaadmin_password: SomeADMINpassword
@@ -181,6 +220,17 @@
     register: result
     failed_when: result.changed or result.failed
 
+  - name: Check if exisintg privilege Broad Privilege with permission would not issue a change. (issue 669)
+    ipaprivilege:
+      ipaadmin_password: SomeADMINpassword
+      ipaapi_context: "{{ ipa_context | default(omit) }}"
+      name: Broad Privilege
+      permission:
+      - "Write IPA Configuration"
+    check_mode: yes
+    register: result
+    failed_when: result.changed or result.failed
+
   # CLEANUP TEST ITEMS
 
   - name: Ensure privilege testing privileges are absent