Merge pull request #780 from rjeffman/module_utils_empty_strings_and_inexistent_attributes

module_utils: Fix comparison of elements not in IPA object.

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):

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
 

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

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

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