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 190f585a2..1c40fa2ca 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 @@ def ipa_connect(self, context=None): 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 @@ def params_get(self, name, allow_empty_string=False): ---------- 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 @@ def params_get_lowercase(self, name, allow_empty_string=False): ---------- 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 a32376cc4..b94b3c726 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 e5e2f4652..8daf1c42c 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 d0f560989..533eed36a 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 dcea92f46..03bf8af72 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 1c40fa2ca..d64c9e31b 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 @@ def params_get_lowercase(self, name, allow_empty_list_item=False): """ 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 a71a88dd2..bd0392181 100644 --- a/plugins/modules/ipapwpolicy.py +++ b/plugins/modules/ipapwpolicy.py @@ -153,7 +153,7 @@ """ 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 d83955bce..6714265f9 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 b94b3c726..da57e7cc3 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 576725165..4432596c6 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