From f2865efb1a81a075fb255be8eb2385a98afbafa2 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Tue, 22 Feb 2022 18:00:52 -0300 Subject: [PATCH 1/3] module_utils: Fix comparison of elements not in IPA object. This change modifies the comparison of the retrieved IPA object and the provided arguments on ansible_freeipa_module.compare_args_ipa when the provider argument is an empty string. If an attribute is not available in 'ipa', its value is considered to be a list with an empty string (['']), possibly forcing the conversion of the 'args' attribute to a list for comparison. This allows, for example, the usage of empty strings which should compare as equals to inexistent attributes (None), as is done in IPA API. --- .../module_utils/ansible_freeipa_module.py | 131 +++++++++++------- 1 file changed, 79 insertions(+), 52 deletions(-) diff --git a/plugins/module_utils/ansible_freeipa_module.py b/plugins/module_utils/ansible_freeipa_module.py index 17d02d0e..88d12116 100644 --- a/plugins/module_utils/ansible_freeipa_module.py +++ b/plugins/module_utils/ansible_freeipa_module.py @@ -310,15 +310,49 @@ else: raise ValueError("Invalid date '%s'" % value) def compare_args_ipa(module, args, ipa, ignore=None): # noqa - """Compare IPA obj attrs with the command args. + """Compare IPA object attributes against command arguments. - This function compares IPA objects attributes with the args the - module is intending to use to call a command. ignore can be a list - of attributes, that should be ignored in the comparison. - This is useful to know if a call to IPA server will be needed or not. - In order to compare we have to perform slight changes in data formats. + This function compares 'ipa' attributes with the 'args' the module + is intending to use as parameters to an IPA API command. A list of + attribute names that should be ignored during comparison may be + provided. - Returns True if they are the same and False otherwise. + The comparison will be performed on every attribute provided in + 'args'. If the attribute in 'args' or 'ipa' is not a scalar value + (including strings) the comparison will be performed as if the + attribute is a set of values, so duplicate values will count as a + single one. If both values are scalar values, then a direct + comparison is performed. + + If an attribute is not available in 'ipa', its value is considered + to be a list with an empty string (['']), possibly forcing the + conversion of the 'args' attribute to a list for comparison. This + allows, for example, the usage of empty strings which should compare + as equals to inexistent attributes (None), as is done in IPA API. + + This function is mostly useful to evaluate the need of a call to + IPA server when provided arguments are equivalent to the existing + values for a given IPA object. + + Parameters + ---------- + module: AnsibleModule + The AnsibleModule used to log debug messages. + + args: dict + The set of attributes provided by the playbook task. + + ipa: dict + The set of attributes from the IPA object retrieved. + + ignore: list + An optional list of attribute names that should be ignored and + not evaluated. + + Return + ------ + True is returned if all attribute values in 'args' are + equivalent to the corresponding attribute value in 'ipa'. """ base_debug_msg = "Ansible arguments and IPA commands differed. " @@ -338,52 +372,45 @@ else: filtered_args = [key for key in args if key not in ignore] for key in filtered_args: - if key not in ipa: # pylint: disable=no-else-return - module.debug( - base_debug_msg + "Command key not present in IPA: %s" % key - ) - return False + arg = args[key] + ipa_arg = ipa.get(key, [""]) + # If ipa_arg is a list and arg is not, replace arg + # with list containing arg. Most args in a find result + # are lists, but not all. + if isinstance(ipa_arg, (list, tuple)): + if not isinstance(arg, list): + arg = [arg] + if len(ipa_arg) != len(arg): + module.debug( + base_debug_msg + + "List length doesn't match for key %s: %d %d" + % (key, len(arg), len(ipa_arg),) + ) + return False + # ensure list elements types are the same. + if not ( + isinstance(ipa_arg[0], type(arg[0])) + or isinstance(arg[0], type(ipa_arg[0])) + ): + arg = [to_text(_arg) for _arg in arg] + try: + arg_set = set(arg) + ipa_arg_set = set(ipa_arg) + except TypeError: + if arg != ipa_arg: + module.debug( + base_debug_msg + + "Different values: %s %s" % (arg, ipa_arg) + ) + return False else: - arg = args[key] - ipa_arg = ipa[key] - # If ipa_arg is a list and arg is not, replace arg - # with list containing arg. Most args in a find result - # are lists, but not all. - if isinstance(ipa_arg, tuple): - ipa_arg = list(ipa_arg) - if isinstance(ipa_arg, list): - if not isinstance(arg, list): - arg = [arg] - if len(ipa_arg) != len(arg): - module.debug( - base_debug_msg - + "List length doesn't match for key %s: %d %d" - % (key, len(arg), len(ipa_arg),) - ) - return False - if isinstance(ipa_arg[0], str) and isinstance(arg[0], int): - arg = [to_text(_arg) for _arg in arg] - if isinstance(ipa_arg[0], unicode) \ - and isinstance(arg[0], int): - arg = [to_text(_arg) for _arg in arg] - try: - arg_set = set(arg) - ipa_arg_set = set(ipa_arg) - except TypeError: - if arg != ipa_arg: - module.debug( - base_debug_msg - + "Different values: %s %s" % (arg, ipa_arg) - ) - return False - else: - if arg_set != ipa_arg_set: - module.debug( - base_debug_msg - + "Different set content: %s %s" - % (arg_set, ipa_arg_set,) - ) - return False + if arg_set != ipa_arg_set: + module.debug( + base_debug_msg + + "Different set content: %s %s" + % (arg_set, ipa_arg_set,) + ) + return False return True def _afm_convert(value): From 70f4b7d6460f8fb9af614f7c99f065b5564d134d Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Wed, 23 Feb 2022 18:08:22 -0300 Subject: [PATCH 2/3] ipauser: Refactor module due to fix on arguments comparison. Due to a change in 'ansible_freeipa_module.compare_args_ipa', playbook parameters using empty strings are correctly evaluated, and do not need to be removed before comparison is performed. A new test playbook, with tests for clearing attributes with an empty string ("") is available at: tests/user/test_user_empty_lists.yml --- plugins/modules/ipauser.py | 14 ------ tests/user/test_user_empty_lists.yml | 71 ++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 14 deletions(-) create mode 100644 tests/user/test_user_empty_lists.yml diff --git a/plugins/modules/ipauser.py b/plugins/modules/ipauser.py index fb0fbc47..739b27b5 100644 --- a/plugins/modules/ipauser.py +++ b/plugins/modules/ipauser.py @@ -1103,20 +1103,6 @@ def main(): if "noprivate" in args: del args["noprivate"] - # Ignore sshpubkey if it is empty (for resetting) - # and not set in for the user - if "ipasshpubkey" not in res_find and \ - "ipasshpubkey" in args and \ - args["ipasshpubkey"] == ['']: - del args["ipasshpubkey"] - - # Ignore userauthtype if it is empty (for resetting) - # and not set in for the user - if "ipauserauthtype" not in res_find and \ - "ipauserauthtype" in args and \ - args["ipauserauthtype"] == ['']: - del args["ipauserauthtype"] - # For all settings is args, check if there are # different settings in the find result. # If yes: modify diff --git a/tests/user/test_user_empty_lists.yml b/tests/user/test_user_empty_lists.yml new file mode 100644 index 00000000..45bb829a --- /dev/null +++ b/tests/user/test_user_empty_lists.yml @@ -0,0 +1,71 @@ +--- +- name: Test users + hosts: "{{ ipa_test_host | default('ipaserver') }}" + become: no + gather_facts: no + + tasks: + # SETUP + - name: Remove test users + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: testuser + state: absent + + - name: Ensure user testuser is present + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: testuser + first: test + last: user + userauthtype: password,radius,otp + sshpubkey: + # yamllint disable-line rule:line-length + - ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQCqmVDpEX5gnSjKuv97AyzOhaUMMKz8ahOA3GY77tVC4o68KNgMCmDSEG1/kOIaElngNLaCha3p/2iAcU9Bi1tLKUlm2bbO5NHNwHfRxY/3cJtq+/7D1vxJzqThYwI4F9vr1WxyY2+mMTv3pXbfAJoR8Mu06XaEY5PDetlDKjHLuNWF+/O7ZU8PsULTa1dJZFrtXeFpmUoLoGxQBvlrlcPI1zDciCSU24t27Zan5Py2l5QchyI7yhCyMM77KDtj5+AFVpmkb9+zq50rYJAyFVeyUvwjzErvQrKJzYpA0NyBp7vskWbt36M16/M/LxEK7HA6mkcakO3ESWx5MT1LAjvdlnxbWG3787MxweHXuB8CZU+9bZPFBaJ+VQtOfJ7I8eH0S16moPC4ak8FlcFvOH8ERDPWLFDqfy09yaZ7bVIF0//5ZI7Nf3YDe3S7GrBX5ieYuECyP6UNkTx9BRsAQeVvXEc6otzB7iCSnYBMGUGzCqeigoAWaVQUONsSR3Uatks= pinky@ipaserver.el81.local # noqa 204 + + # TESTS - action: user + - name: Ensure user testuser present with empty sshpubkey + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: testuser + sshpubkey: "" + register: result + failed_when: not result.changed or result.failed + + - name: Ensure user testuser present with empty sshpubkey, again + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: testuser + sshpubkey: "" + register: result + failed_when: result.changed or result.failed + + - name: Ensure user testuser present with empty userauthtype + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: testuser + userauthtype: "" + register: result + failed_when: not result.changed or result.failed + + - name: Ensure user testuser present with empty userauthtype, again + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: testuser + userauthtype: "" + register: result + failed_when: result.changed or result.failed + + # CLEANUP + - name: Remove test users + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + name: testuser + state: absent From a83bab9425cd1e4a3ce7e635ab1eb495eed920d7 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Tue, 22 Feb 2022 18:09:58 -0300 Subject: [PATCH 3/3] ipaautomountmap: Allows clearing description attribute with "". This change allows clearing automountmap 'description' attribute by passing an empty string ("") as the playbook parameter. New test cases were added to check this behavior. --- plugins/modules/ipaautomountmap.py | 3 ++- tests/automount/test_automountmap.yml | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/plugins/modules/ipaautomountmap.py b/plugins/modules/ipaautomountmap.py index 79b341b9..9cca0a98 100644 --- a/plugins/modules/ipaautomountmap.py +++ b/plugins/modules/ipaautomountmap.py @@ -126,7 +126,8 @@ class AutomountMap(IPAAnsibleModule): _args = {} if mapname: _args["automountmapname"] = mapname - if desc: + # An empty string is valid and will clear the attribute. + if desc is not None: _args["description"] = desc return _args diff --git a/tests/automount/test_automountmap.yml b/tests/automount/test_automountmap.yml index da542e89..377b3a82 100644 --- a/tests/automount/test_automountmap.yml +++ b/tests/automount/test_automountmap.yml @@ -71,6 +71,24 @@ register: result failed_when: result.failed or result.changed + - name: ensure map TestMap has an empty description + ipaautomountmap: + ipaadmin_password: SomeADMINpassword + name: TestMap + location: TestLocation + desc: "" + register: result + failed_when: result.failed or not result.changed + + - name: ensure map TestMap has an empty description, again + ipaautomountmap: + ipaadmin_password: SomeADMINpassword + name: TestMap + location: TestLocation + desc: "" + register: result + failed_when: result.failed or result.changed + - name: ensure map TestMap is removed ipaautomountmap: ipaadmin_password: SomeADMINpassword