Merge pull request #670 from rjeffman/ipapermission_fixes

ipaprivilege: Fix permissions handling.

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,33 +215,17 @@ def main():
                 if action == "privilege":
                     # Found the privilege
                     if res_find is not None:
-                        res_cmp = {
-                            k: v for k, v in res_find.items()
-                            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):
+                        cmp = {"description": res_find.get("description")}
+                        if not compare_args_ipa(ansible_module, args, cmp):
                             commands.append([name, "privilege_mod", args])
                     else:
                         commands.append([name, "privilege_add", args])
                         res_find = {}
 
-                    member_args = {}
-                    if permission:
-                        member_args['permission'] = permission
-
-                    if not compare_args_ipa(ansible_module, member_args,
-                                            res_find):
-
                     # Generate addition and removal lists
                     permission_add, permission_del = gen_add_del_lists(
-                                permission, res_find.get("member_permission"))
+                        permission, res_find.get("memberof_permission")
+                    )
 
                     # Add members
                     if len(permission_add) > 0:
@@ -280,6 +249,9 @@ def main():
                     if permission is None:
                         ansible_module.fail_json(msg="No permission given")
 
+                    permission = gen_add_list(
+                        permission, res_find.get("memberof_permission"))
+                    if permission:
                         commands.append([name, "privilege_add_permission",
                                          {"permission": permission}])
 
@@ -296,10 +268,11 @@ def main():
                     if permission is None:
                         ansible_module.fail_json(msg="No permission given")
 
+                    permission = gen_intersection_list(
+                        permission, res_find.get("memberof_permission"))
+                    if permission:
                         commands.append([name, "privilege_remove_permission",
-                                     {
-                                         "permission": permission,
-                                     }])
+                                         {"permission": permission}])
 
             elif state == "renamed":
                 if not rename:
@@ -318,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