From 0757bfee0a1d5ddb4b619ba9ae61d787fd380558 Mon Sep 17 00:00:00 2001
From: Rafael Guterres Jeffman <rjeffman@redhat.com>
Date: Tue, 26 Oct 2021 11:37:07 -0300
Subject: [PATCH] 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 | 95 +++++++++++-------------------
tests/privilege/test_privilege.yml | 50 ++++++++++++++++
2 files changed, 84 insertions(+), 61 deletions(-)
diff --git a/plugins/modules/ipaprivilege.py b/plugins/modules/ipaprivilege.py
index d38c9a5a..6856b1a6 100644
--- a/plugins/modules/ipaprivilege.py
+++ b/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 = {
- 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("memberof_permission")
- )
-
- # Add members
- if len(permission_add) > 0:
- commands.append([name, "privilege_add_permission",
- {
- "permission": permission_add,
- }])
- # Remove members
- if len(permission_del) > 0:
- commands.append([
- name,
- "privilege_remove_permission",
- {"permission": permission_del}
- ])
+ # Generate addition and removal lists
+ permission_add, permission_del = gen_add_del_lists(
+ permission, res_find.get("memberof_permission")
+ )
+
+ # Add members
+ if len(permission_add) > 0:
+ commands.append([name, "privilege_add_permission",
+ {
+ "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": permission}])
+ permission = gen_add_list(
+ 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": permission,
- }])
+ permission = gen_intersection_list(
+ permission, res_find.get("memberof_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
diff --git a/tests/privilege/test_privilege.yml b/tests/privilege/test_privilege.yml
index 71c08d16..bfb46729 100644
--- a/tests/privilege/test_privilege.yml
+++ b/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
--
GitLab