From f4c9e2871582ee5e37f5d51b36ff5861c310b961 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Mon, 18 Sep 2023 11:27:46 -0300 Subject: [PATCH 1/5] Rename parameter 'allow_empty_string' to 'allow_empty_list_item' The parameter 'allow_empty_string' in 'module_params_get' is used to allow an item in a list to be an empty string. The problem is that the naming is misleading, as it is checking a list item rather than a string. This patch rename the parameter to 'allow_empty_list_item' so that it more clearly refers to list itens instead of standalone strings, and do not collide with future parameters that may test for empty strings which are not part of lists. --- .../module_utils/ansible_freeipa_module.py | 25 +++++++++---------- plugins/modules/ipaconfig.py | 4 +-- plugins/modules/ipahost.py | 7 +++--- plugins/modules/ipaservice.py | 6 +++-- plugins/modules/ipauser.py | 4 +-- 5 files changed, 24 insertions(+), 22 deletions(-) diff --git a/plugins/module_utils/ansible_freeipa_module.py b/plugins/module_utils/ansible_freeipa_module.py index 190f585a..1c40fa2c 100644 --- a/plugins/module_utils/ansible_freeipa_module.py +++ b/plugins/module_utils/ansible_freeipa_module.py @@ -470,12 +470,11 @@ def _afm_convert(value): return value -def module_params_get(module, name, allow_empty_string=False): +def module_params_get(module, name, allow_empty_list_item=False): value = _afm_convert(module.params.get(name)) - # Fail on empty strings in the list or if allow_empty_string is True - # if there is another entry in the list together with the empty - # string. + # Fail on empty strings in the list or if allow_empty_list_item is True + # if there is another entry in the list together with the empty string. # Due to an issue in Ansible it is possible to use the empty string # "" for lists with choices, even if the empty list is not part of # the choices. @@ -483,7 +482,7 @@ def module_params_get(module, name, allow_empty_string=False): if isinstance(value, list): for val in value: if isinstance(val, (str, unicode)) and not val: - if not allow_empty_string: + if not allow_empty_list_item: module.fail_json( msg="Parameter '%s' contains an empty string" % name) @@ -495,8 +494,8 @@ def module_params_get(module, name, allow_empty_string=False): return value -def module_params_get_lowercase(module, name, allow_empty_string=False): - value = module_params_get(module, name, allow_empty_string) +def module_params_get_lowercase(module, name, allow_empty_list_item=False): + value = module_params_get(module, name, allow_empty_list_item) if isinstance(value, list): value = [v.lower() for v in value] if isinstance(value, (str, unicode)): @@ -1051,7 +1050,7 @@ class IPAAnsibleModule(AnsibleModule): finally: temp_kdestroy(ccache_dir, ccache_name) - def params_get(self, name, allow_empty_string=False): + def params_get(self, name, allow_empty_list_item=False): """ Retrieve value set for module parameter. @@ -1059,13 +1058,13 @@ class IPAAnsibleModule(AnsibleModule): ---------- name: string The name of the parameter to retrieve. - allow_empty_string: bool + allow_empty_list_item: bool The parameter allowes to have empty strings in a list """ - return module_params_get(self, name, allow_empty_string) + return module_params_get(self, name, allow_empty_list_item) - def params_get_lowercase(self, name, allow_empty_string=False): + def params_get_lowercase(self, name, allow_empty_list_item=False): """ Retrieve value set for module parameter as lowercase, if not None. @@ -1073,11 +1072,11 @@ class IPAAnsibleModule(AnsibleModule): ---------- name: string The name of the parameter to retrieve. - allow_empty_string: bool + allow_empty_list_item: bool The parameter allowes to have empty strings in a list """ - return module_params_get_lowercase(self, name, allow_empty_string) + return module_params_get_lowercase(self, name, allow_empty_list_item) def params_fail_used_invalid(self, invalid_params, state, action=None): """ diff --git a/plugins/modules/ipaconfig.py b/plugins/modules/ipaconfig.py index a32376cc..b94b3c72 100644 --- a/plugins/modules/ipaconfig.py +++ b/plugins/modules/ipaconfig.py @@ -470,13 +470,13 @@ def main(): "netbios_name": "netbios_name", "add_sids": "add_sids", } - allow_empty_string = ["pac_type", "user_auth_type", "configstring"] reverse_field_map = {v: k for k, v in field_map.items()} + allow_empty_list_item = ["pac_type", "user_auth_type", "configstring"] params = {} for x in field_map: val = ansible_module.params_get( - x, allow_empty_string=x in allow_empty_string) + x, allow_empty_list_item=(x in allow_empty_list_item)) if val is not None: params[field_map.get(x, x)] = val diff --git a/plugins/modules/ipahost.py b/plugins/modules/ipahost.py index e5e2f465..8daf1c42 100644 --- a/plugins/modules/ipahost.py +++ b/plugins/modules/ipahost.py @@ -876,10 +876,11 @@ def main(): allow_retrieve_keytab_hostgroup = ansible_module.params_get( "allow_retrieve_keytab_hostgroup") mac_address = ansible_module.params_get("mac_address") - sshpubkey = ansible_module.params_get("sshpubkey", - allow_empty_string=True) + sshpubkey = ansible_module.params_get( + "sshpubkey", allow_empty_list_item=True) userclass = ansible_module.params_get("userclass") - auth_ind = ansible_module.params_get("auth_ind", allow_empty_string=True) + auth_ind = ansible_module.params_get( + "auth_ind", allow_empty_list_item=True) requires_pre_auth = ansible_module.params_get("requires_pre_auth") ok_as_delegate = ansible_module.params_get("ok_as_delegate") ok_to_auth_as_delegate = ansible_module.params_get( diff --git a/plugins/modules/ipaservice.py b/plugins/modules/ipaservice.py index d0f56098..533eed36 100644 --- a/plugins/modules/ipaservice.py +++ b/plugins/modules/ipaservice.py @@ -607,8 +607,10 @@ def main(): # white space also. if certificate is not None: certificate = [cert.strip() for cert in certificate] - pac_type = ansible_module.params_get("pac_type", allow_empty_string=True) - auth_ind = ansible_module.params_get("auth_ind", allow_empty_string=True) + pac_type = ansible_module.params_get( + "pac_type", allow_empty_list_item=True) + auth_ind = ansible_module.params_get( + "auth_ind", allow_empty_list_item=True) skip_host_check = ansible_module.params_get("skip_host_check") force = ansible_module.params_get("force") requires_pre_auth = ansible_module.params_get("requires_pre_auth") diff --git a/plugins/modules/ipauser.py b/plugins/modules/ipauser.py index dcea92f4..03bf8af7 100644 --- a/plugins/modules/ipauser.py +++ b/plugins/modules/ipauser.py @@ -1185,9 +1185,9 @@ def main(): manager = ansible_module.params_get("manager") carlicense = ansible_module.params_get("carlicense") sshpubkey = ansible_module.params_get("sshpubkey", - allow_empty_string=True) + allow_empty_list_item=True) userauthtype = ansible_module.params_get("userauthtype", - allow_empty_string=True) + allow_empty_list_item=True) userclass = ansible_module.params_get("userclass") radius = ansible_module.params_get("radius") radiususer = ansible_module.params_get("radiususer") From e55a41ca0cf37e4cc65b282bf2842426a89d79ff Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Sun, 17 Sep 2023 22:28:10 -0300 Subject: [PATCH 2/5] ansible_freeipa_module: Ensure data type when retrieving parameter Some parameters, in modules, have a specific data type, but allow the use of an empty string to clear the parameter. By providing a method to retrieve the parameter with the correct data type, or optionally an empty string, allows for consistency of parameter handling between different modules. --- .../module_utils/ansible_freeipa_module.py | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/plugins/module_utils/ansible_freeipa_module.py b/plugins/module_utils/ansible_freeipa_module.py index 1c40fa2c..d64c9e31 100644 --- a/plugins/module_utils/ansible_freeipa_module.py +++ b/plugins/module_utils/ansible_freeipa_module.py @@ -503,6 +503,48 @@ def module_params_get_lowercase(module, name, allow_empty_list_item=False): return value +def module_params_get_with_type_cast( + module, name, datatype, allow_empty=False +): + """ + Retrieve value set for module parameter as a specific data type. + + Parameters + ---------- + module: AnsibleModule + The module from where to get the parameter value from. + name: string + The name of the parameter to retrieve. + datatype: type + The type to convert the value to, if value is not empty. + allow_empty: bool + Allow an empty string for non list parameters or a list + containing (only) an empty string item. This is used for + resetting parameters to the default value. + + """ + value = module_params_get(module, name, allow_empty) + if not allow_empty and value == "": + module.fail_json( + msg="Argument '%s' must not be an empty string" % (name,) + ) + if value is not None and value != "": + try: + if datatype is bool: + # We let Ansible handle bool values + value = boolean(value) + else: + value = datatype(value) + except ValueError: + module.fail_json( + msg="Invalid value '%s' for argument '%s'" % (value, name) + ) + except TypeError as terr: + # If Ansible fails to parse a boolean, it will raise TypeError + module.fail_json(msg="Param '%s': %s" % (name, str(terr))) + return value + + def api_get_domain(): return api.env.domain @@ -1078,6 +1120,29 @@ class IPAAnsibleModule(AnsibleModule): """ return module_params_get_lowercase(self, name, allow_empty_list_item) + def params_get_with_type_cast( + self, name, datatype, allow_empty=True + ): + """ + Retrieve value set for module parameter as a specific data type. + + Parameters + ---------- + name: string + The name of the parameter to retrieve. + datatype: type + The type to convert the value to, if not empty. + datatype: type + The type to convert the value to, if value is not empty. + allow_empty: bool + Allow an empty string for non list parameters or a list + containing (only) an empty string item. This is used for + resetting parameters to the default value. + + """ + return module_params_get_with_type_cast( + self, name, datatype, allow_empty) + def params_fail_used_invalid(self, invalid_params, state, action=None): """ Fail module execution if one of the invalid parameters is not None. From 92d579be418fc923bb1b3b42356bf0c52e2b1884 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Sun, 17 Sep 2023 22:32:35 -0300 Subject: [PATCH 3/5] ipapwpolicy: Use modules.params_get_type Use the commom parameter type handling method for parameters that accept a value or an empty string. --- plugins/modules/ipapwpolicy.py | 79 +++++++++++++--------------------- 1 file changed, 29 insertions(+), 50 deletions(-) diff --git a/plugins/modules/ipapwpolicy.py b/plugins/modules/ipapwpolicy.py index a71a88dd..bd039218 100644 --- a/plugins/modules/ipapwpolicy.py +++ b/plugins/modules/ipapwpolicy.py @@ -153,7 +153,7 @@ RETURN = """ """ from ansible.module_utils.ansible_freeipa_module import \ - IPAAnsibleModule, compare_args_ipa, boolean + IPAAnsibleModule, compare_args_ipa def find_pwpolicy(module, name): @@ -294,20 +294,34 @@ def main(): names = ansible_module.params_get("name") # present - maxlife = ansible_module.params_get("maxlife") - minlife = ansible_module.params_get("minlife") - history = ansible_module.params_get("history") - minclasses = ansible_module.params_get("minclasses") - minlength = ansible_module.params_get("minlength") - priority = ansible_module.params_get("priority") - maxfail = ansible_module.params_get("maxfail") - failinterval = ansible_module.params_get("failinterval") - lockouttime = ansible_module.params_get("lockouttime") - maxrepeat = ansible_module.params_get("maxrepeat") - maxsequence = ansible_module.params_get("maxsequence") - dictcheck = ansible_module.params_get("dictcheck") - usercheck = ansible_module.params_get("usercheck") - gracelimit = ansible_module.params_get("gracelimit") + maxlife = ansible_module.params_get_with_type_cast( + "maxlife", int, allow_empty=True) + minlife = ansible_module.params_get_with_type_cast( + "minlife", int, allow_empty=True) + history = ansible_module.params_get_with_type_cast( + "history", int, allow_empty=True) + minclasses = ansible_module.params_get_with_type_cast( + "minclasses", int, allow_empty=True) + minlength = ansible_module.params_get_with_type_cast( + "minlength", int, allow_empty=True) + priority = ansible_module.params_get_with_type_cast( + "priority", int, allow_empty=True) + maxfail = ansible_module.params_get_with_type_cast( + "maxfail", int, allow_empty=True) + failinterval = ansible_module.params_get_with_type_cast( + "failinterval", int, allow_empty=True) + lockouttime = ansible_module.params_get_with_type_cast( + "lockouttime", int, allow_empty=True) + maxrepeat = ansible_module.params_get_with_type_cast( + "maxrepeat", int, allow_empty=True) + maxsequence = ansible_module.params_get_with_type_cast( + "maxsequence", int, allow_empty=True) + dictcheck = ansible_module.params_get_with_type_cast( + "dictcheck", bool, allow_empty=True) + usercheck = ansible_module.params_get_with_type_cast( + "usercheck", bool, allow_empty=True) + gracelimit = ansible_module.params_get_with_type_cast( + "gracelimit", int, allow_empty=True) # state state = ansible_module.params_get("state") @@ -336,41 +350,6 @@ def main(): ansible_module.params_fail_used_invalid(invalid, state) - # Ensure parameter values are valid and have proper type. - def int_or_empty_param(value, param): - if value is not None and value != "": - try: - value = int(value) - except ValueError: - ansible_module.fail_json( - msg="Invalid value '%s' for argument '%s'" % (value, param) - ) - return value - - maxlife = int_or_empty_param(maxlife, "maxlife") - minlife = int_or_empty_param(minlife, "minlife") - history = int_or_empty_param(history, "history") - minclasses = int_or_empty_param(minclasses, "minclasses") - minlength = int_or_empty_param(minlength, "minlength") - priority = int_or_empty_param(priority, "priority") - maxfail = int_or_empty_param(maxfail, "maxfail") - failinterval = int_or_empty_param(failinterval, "failinterval") - lockouttime = int_or_empty_param(lockouttime, "lockouttime") - maxrepeat = int_or_empty_param(maxrepeat, "maxrepeat") - maxsequence = int_or_empty_param(maxsequence, "maxsequence") - gracelimit = int_or_empty_param(gracelimit, "gracelimit") - - def bool_or_empty_param(value, param): # pylint: disable=R1710 - if value is None or value == "": - return value - try: - return boolean(value) - except TypeError as terr: - ansible_module.fail_json(msg="Param '%s': %s" % (param, str(terr))) - - dictcheck = bool_or_empty_param(dictcheck, "dictcheck") - usercheck = bool_or_empty_param(usercheck, "usercheck") - # Ensure gracelimit has proper limit. if gracelimit: if gracelimit < -1: From bc694b722c48b591e87b68dc12c65793bfc6eb2c Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Sun, 17 Sep 2023 22:34:48 -0300 Subject: [PATCH 4/5] idoverideuser: Use module.params_get_type Use the commom parameter type handling method for parameters that accept a value or an empty string. --- plugins/modules/ipaidoverrideuser.py | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/plugins/modules/ipaidoverrideuser.py b/plugins/modules/ipaidoverrideuser.py index d83955bc..6714265f 100644 --- a/plugins/modules/ipaidoverrideuser.py +++ b/plugins/modules/ipaidoverrideuser.py @@ -439,9 +439,9 @@ def main(): # present description = ansible_module.params_get("description") name = ansible_module.params_get("name") - uid = ansible_module.params_get("uid") + uid = ansible_module.params_get_with_type_cast("uid", int) gecos = ansible_module.params_get("gecos") - gidnumber = ansible_module.params_get("gidnumber") + gidnumber = ansible_module.params_get_with_type_cast("gidnumber", int) homedir = ansible_module.params_get("homedir") shell = ansible_module.params_get("shell") sshpubkey = ansible_module.params_get("sshpubkey") @@ -479,20 +479,6 @@ def main(): ansible_module.params_fail_used_invalid(invalid, state, action) - # Ensure parameter values are valid and have proper type. - def int_or_empty_param(value, param): - if value is not None and value != "": - try: - value = int(value) - except ValueError: - ansible_module.fail_json( - msg="Invalid value '%s' for argument '%s'" % (value, param) - ) - return value - - uid = int_or_empty_param(uid, "uid") - gidnumber = int_or_empty_param(gidnumber, "gidnumber") - if certificate is not None: certificate = [cert.strip() for cert in certificate] From 34973c04c608104b355939aff2ed663f0f3e0bd3 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Mon, 18 Sep 2023 12:44:45 -0300 Subject: [PATCH 5/5] idoveridegroup: Use module.params_get_type Use the commom parameter type handling method for parameters that accept a value or an empty string. --- plugins/modules/ipaconfig.py | 2 +- plugins/modules/ipaidoverridegroup.py | 15 +-------------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/plugins/modules/ipaconfig.py b/plugins/modules/ipaconfig.py index b94b3c72..da57e7cc 100644 --- a/plugins/modules/ipaconfig.py +++ b/plugins/modules/ipaconfig.py @@ -476,7 +476,7 @@ def main(): params = {} for x in field_map: val = ansible_module.params_get( - x, allow_empty_list_item=(x in allow_empty_list_item)) + x, allow_empty_list_item=x in allow_empty_list_item) if val is not None: params[field_map.get(x, x)] = val diff --git a/plugins/modules/ipaidoverridegroup.py b/plugins/modules/ipaidoverridegroup.py index 57672516..4432596c 100644 --- a/plugins/modules/ipaidoverridegroup.py +++ b/plugins/modules/ipaidoverridegroup.py @@ -243,7 +243,7 @@ def main(): # present description = ansible_module.params_get("description") name = ansible_module.params_get("name") - gid = ansible_module.params_get("gid") + gid = ansible_module.params_get_with_type_cast("gid", int) # runtime flags fallback_to_ldap = ansible_module.params_get("fallback_to_ldap") @@ -271,19 +271,6 @@ def main(): ansible_module.params_fail_used_invalid(invalid, state) - # Ensure parameter values are valid and have proper type. - def int_or_empty_param(value, param): - if value is not None and value != "": - try: - value = int(value) - except ValueError: - ansible_module.fail_json( - msg="Invalid value '%s' for argument '%s'" % (value, param) - ) - return value - - gid = int_or_empty_param(gid, "gid") - # Init changed = False