From 7f61e72a2c7fa92ebb3ae483c028d220deb1dc25 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Mon, 24 Jan 2022 17:35:36 -0300 Subject: [PATCH 1/2] ipauser: Fix idempotence issue when using 'preserved'. When trying to ensure 'state: absent' with 'preserved: yes' in ipauser, after the first execution the playbook would fail with "user is already present". Similar idempotence issue would happen when 'state: undelete' was used. This PR fixes both issues, and improve tests for the states where user is preserved, enabled and disabled. The 'find_user' function now uses IPA API 'user_show' instead of 'user_find' so that only the requested user is actually returned. --- plugins/modules/ipauser.py | 69 ++++++++++++++------------------------ tests/user/test_user.yml | 66 ++++++++++++++++++++++++++++++++++++ tests/user/test_users.yml | 62 ++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 44 deletions(-) diff --git a/plugins/modules/ipauser.py b/plugins/modules/ipauser.py index a1146fb6b..d11d15b9d 100644 --- a/plugins/modules/ipauser.py +++ b/plugins/modules/ipauser.py @@ -474,41 +474,31 @@ from ansible.module_utils.ansible_freeipa_module import \ IPAAnsibleModule, compare_args_ipa, gen_add_del_lists, date_format, \ - encode_certificate, load_cert_from_str, DN_x500_text, to_text + encode_certificate, load_cert_from_str, DN_x500_text, to_text, \ + ipalib_errors from ansible.module_utils import six if six.PY3: unicode = str -def find_user(module, name, preserved=False): +def find_user(module, name): _args = { "all": True, - "uid": name, } - if preserved: - _args["preserved"] = preserved - - _result = module.ipa_command("user_find", name, _args) - - if len(_result["result"]) > 1: - module.fail_json( - msg="There is more than one user '%s'" % (name)) - elif len(_result["result"]) == 1: - # Transform each principal to a string - _result = _result["result"][0] - if "krbprincipalname" in _result \ - and _result["krbprincipalname"] is not None: - _list = [] - for x in _result["krbprincipalname"]: - _list.append(str(x)) - _result["krbprincipalname"] = _list - certs = _result.get("usercertificate") - if certs is not None: - _result["usercertificate"] = [encode_certificate(x) - for x in certs] - return _result - - return None + + try: + _result = module.ipa_command("user_show", name, _args).get("result") + except ipalib_errors.NotFound: + return None + + # Transform each principal to a string + _result["krbprincipalname"] = [ + to_text(x) for x in (_result.get("krbprincipalname") or []) + ] + _result["usercertificate"] = [ + encode_certificate(x) for x in (_result.get("usercertificate") or []) + ] + return _result def gen_args(first, last, fullname, displayname, initials, homedir, shell, @@ -1085,12 +1075,6 @@ def main(): # Make sure user exists res_find = find_user(ansible_module, name) - # Also search for preserved user if the user could not be found - if res_find is None: - res_find_preserved = find_user(ansible_module, name, - preserved=True) - else: - res_find_preserved = None # Create command if state == "present": @@ -1104,10 +1088,6 @@ def main(): departmentnumber, employeenumber, employeetype, preferredlanguage, noprivate, nomembers) - # Also check preserved users - if res_find is None and res_find_preserved is not None: - res_find = res_find_preserved - if action == "user": # Found the user if res_find is not None: @@ -1310,16 +1290,16 @@ def main(): gen_certmapdata_args(_data)]) elif state == "absent": - # Also check preserved users - if res_find is None and res_find_preserved is not None: - res_find = res_find_preserved - if action == "user": if res_find is not None: args = {} if preserve is not None: args["preserve"] = preserve - commands.append([name, "user_del", args]) + if ( + not res_find.get("preserved", False) + or not args.get("preserve", False) + ): + commands.append([name, "user_del", args]) elif action == "member": if res_find is None: ansible_module.fail_json( @@ -1370,8 +1350,9 @@ def main(): commands.append([name, "user_remove_certmapdata", gen_certmapdata_args(_data)]) elif state == "undeleted": - if res_find_preserved is not None: - commands.append([name, "user_undel", {}]) + if res_find is not None: + if res_find.get("preserved", False): + commands.append([name, "user_undel", {}]) else: raise ValueError("No preserved user '%s'" % name) diff --git a/tests/user/test_user.yml b/tests/user/test_user.yml index d61ae5214..4583fd72a 100644 --- a/tests/user/test_user.yml +++ b/tests/user/test_user.yml @@ -249,6 +249,16 @@ register: result failed_when: not result.changed or result.failed + - name: User pinky absent and preserved, again + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + preserve: yes + state: absent + register: result + failed_when: result.changed or result.failed + - name: User pinky undeleted (preserved before) ipauser: ipaadmin_password: SomeADMINpassword @@ -258,6 +268,15 @@ register: result failed_when: not result.changed or result.failed + - name: User pinky undeleted (preserved before), again + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + state: undeleted + register: result + failed_when: result.changed or result.failed + - name: Users pinky disabled ipauser: ipaadmin_password: SomeADMINpassword @@ -267,6 +286,15 @@ register: result failed_when: not result.changed or result.failed + - name: Users pinky disabled, again + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + state: disabled + register: result + failed_when: result.changed or result.failed + - name: User pinky enabled ipauser: ipaadmin_password: SomeADMINpassword @@ -276,6 +304,44 @@ register: result failed_when: not result.changed or result.failed + - name: User pinky enabled, again + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + state: enabled + register: result + failed_when: result.changed or result.failed + + - name: User pinky absent and preserved for future exclusion. + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + preserve: yes + state: absent + register: result + failed_when: not result.changed or result.failed + + - name: User pinky absent + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + state: absent + register: result + failed_when: not result.changed or result.failed + + - name: User pinky absent and preserved, when already absent + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + preserve: yes + state: absent + register: result + failed_when: result.changed or result.failed + - name: Remove test users ipauser: ipaadmin_password: SomeADMINpassword diff --git a/tests/user/test_users.yml b/tests/user/test_users.yml index 404b86b21..9b795244e 100644 --- a/tests/user/test_users.yml +++ b/tests/user/test_users.yml @@ -369,6 +369,15 @@ register: result failed_when: not result.changed or result.failed + - name: User pinky absent and preserved, again + ipauser: + ipaadmin_password: SomeADMINpassword + name: pinky + preserve: yes + state: absent + register: result + failed_when: result.changed or result.failed + - name: User pinky undeleted (preserved before) ipauser: ipaadmin_password: SomeADMINpassword @@ -377,6 +386,14 @@ register: result failed_when: not result.changed or result.failed + - name: User pinky undeleted (preserved before), again + ipauser: + ipaadmin_password: SomeADMINpassword + name: pinky + state: undeleted + register: result + failed_when: result.changed or result.failed + - name: Users pinky disabled ipauser: ipaadmin_password: SomeADMINpassword @@ -385,6 +402,14 @@ register: result failed_when: not result.changed or result.failed + - name: Users pinky disabled, again + ipauser: + ipaadmin_password: SomeADMINpassword + name: pinky + state: disabled + register: result + failed_when: result.changed or result.failed + - name: User pinky enabled ipauser: ipaadmin_password: SomeADMINpassword @@ -393,6 +418,43 @@ register: result failed_when: not result.changed or result.failed + - name: User pinky enabled, again + ipauser: + ipaadmin_password: SomeADMINpassword + name: pinky + state: enabled + register: result + failed_when: result.changed or result.failed + + - name: User pinky absent and preserved for future exclusion. + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + preserve: yes + state: absent + register: result + failed_when: not result.changed or result.failed + + - name: User pinky absent + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + state: absent + register: result + failed_when: not result.changed or result.failed + + - name: User pinky absent and preserved, when already absent + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: pinky + preserve: yes + state: absent + register: result + failed_when: result.changed or result.failed + - name: Remove test users ipauser: ipaadmin_password: SomeADMINpassword From 401b911171ef7589d719a5432132c3f7e1f7e509 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Wed, 26 Jan 2022 08:42:05 -0300 Subject: [PATCH 2/2] ipauser: Make 'no user' messages consistent. When ensuring states 'undeleted', 'enabled', 'disabled', and 'unlocked' the error messages for an unexistent user were not consistent. This change changes the message for all states to "No user '%s'." --- plugins/modules/ipauser.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/modules/ipauser.py b/plugins/modules/ipauser.py index d11d15b9d..aee71cd22 100644 --- a/plugins/modules/ipauser.py +++ b/plugins/modules/ipauser.py @@ -1354,14 +1354,14 @@ def main(): if res_find.get("preserved", False): commands.append([name, "user_undel", {}]) else: - raise ValueError("No preserved user '%s'" % name) + raise ValueError("No user '%s'" % name) elif state == "enabled": if res_find is not None: if res_find["nsaccountlock"]: commands.append([name, "user_enable", {}]) else: - raise ValueError("No disabled user '%s'" % name) + raise ValueError("No user '%s'" % name) elif state == "disabled": if res_find is not None: @@ -1373,6 +1373,8 @@ def main(): elif state == "unlocked": if res_find is not None: commands.append([name, "user_unlock", {}]) + else: + raise ValueError("No user '%s'" % name) else: ansible_module.fail_json(msg="Unkown state '%s'" % state)