From edd7b84285dd944f8c3e736928ef6a56a563748b Mon Sep 17 00:00:00 2001 From: Alexei Znamensky <103110+russoz@users.noreply.github.com> Date: Tue, 31 Aug 2021 22:34:57 +1200 Subject: [PATCH] pamd - fixed issue+minor refactorings (#3285) * pamd - fixed issue+minor refactorings * added changelog fragment * added unit test suggested in issue * Update tests/integration/targets/pamd/tasks/main.yml * fixed per PR + additional adjustment Co-authored-by: Felix Fontein --- .../3285-pamd-updated-with-empty-args.yaml | 4 ++ plugins/modules/system/pamd.py | 46 +++++++++---------- tests/integration/targets/pamd/tasks/main.yml | 31 ++++++++++--- .../unit/plugins/modules/system/test_pamd.py | 8 ++++ 4 files changed, 59 insertions(+), 30 deletions(-) create mode 100644 changelogs/fragments/3285-pamd-updated-with-empty-args.yaml diff --git a/changelogs/fragments/3285-pamd-updated-with-empty-args.yaml b/changelogs/fragments/3285-pamd-updated-with-empty-args.yaml new file mode 100644 index 0000000000..1c176dfdc3 --- /dev/null +++ b/changelogs/fragments/3285-pamd-updated-with-empty-args.yaml @@ -0,0 +1,4 @@ +bugfixes: + - pamd - code for ``state=updated`` when dealing with the pam module arguments, made no distinction between ``None`` and an empty list (https://github.com/ansible-collections/community.general/issues/3260). +minor_changes: + - pamd - minor refactorings (https://github.com/ansible-collections/community.general/pull/3285). diff --git a/plugins/modules/system/pamd.py b/plugins/modules/system/pamd.py index 738a23ee43..dda504974d 100644 --- a/plugins/modules/system/pamd.py +++ b/plugins/modules/system/pamd.py @@ -274,8 +274,7 @@ RULE_REGEX = re.compile(r"""(?P-?(?:auth|account|session|password))\s (?P\[.*\]|\S*)\s+ (?P\S*)\s* (?P.*)\s*""", re.X) - -RULE_ARG_REGEX = re.compile(r"""(\[.*\]|\S*)""") +RULE_ARG_REGEX = re.compile(r"(\[.*\]|\S*)") VALID_TYPES = ['account', '-account', 'auth', '-auth', 'password', '-password', 'session', '-session'] @@ -358,11 +357,9 @@ class PamdRule(PamdLine): # Method to check if a rule matches the type, control and path. def matches(self, rule_type, rule_control, rule_path, rule_args=None): - if (rule_type == self.rule_type and + return (rule_type == self.rule_type and rule_control == self.rule_control and - rule_path == self.rule_path): - return True - return False + rule_path == self.rule_path) @classmethod def rule_from_string(cls, line): @@ -507,25 +504,25 @@ class PamdService(object): # Get a list of rules we want to change rules_to_find = self.get(rule_type, rule_control, rule_path) - new_args = parse_module_arguments(new_args) + new_args = parse_module_arguments(new_args, return_none=True) changes = 0 for current_rule in rules_to_find: rule_changed = False if new_type: - if(current_rule.rule_type != new_type): + if current_rule.rule_type != new_type: rule_changed = True current_rule.rule_type = new_type if new_control: - if(current_rule.rule_control != new_control): + if current_rule.rule_control != new_control: rule_changed = True current_rule.rule_control = new_control if new_path: - if(current_rule.rule_path != new_path): + if current_rule.rule_path != new_path: rule_changed = True current_rule.rule_path = new_path - if new_args: - if(current_rule.rule_args != new_args): + if new_args is not None: + if current_rule.rule_args != new_args: rule_changed = True current_rule.rule_args = new_args @@ -724,8 +721,9 @@ class PamdService(object): current_line = self._head while current_line is not None: - if not current_line.validate()[0]: - return current_line.validate() + curr_validate = current_line.validate() + if not curr_validate[0]: + return curr_validate current_line = current_line.next return True, "Module is valid" @@ -750,22 +748,25 @@ class PamdService(object): return '\n'.join(lines) + '\n' -def parse_module_arguments(module_arguments): - # Return empty list if we have no args to parse - if not module_arguments: - return [] - elif isinstance(module_arguments, list) and len(module_arguments) == 1 and not module_arguments[0]: +def parse_module_arguments(module_arguments, return_none=False): + # If args is None, return empty list by default. + # But if return_none is True, then return None + if module_arguments is None: + return None if return_none else [] + if isinstance(module_arguments, list) and len(module_arguments) == 1 and not module_arguments[0]: return [] if not isinstance(module_arguments, list): module_arguments = [module_arguments] - parsed_args = list() + # From this point on, module_arguments is guaranteed to be a list, empty or not + parsed_args = [] + re_clear_spaces = re.compile(r"\s*=\s*") for arg in module_arguments: for item in filter(None, RULE_ARG_REGEX.findall(arg)): if not item.startswith("["): - re.sub("\\s*=\\s*", "=", item) + re_clear_spaces.sub("=", item) parsed_args.append(item) return parsed_args @@ -861,8 +862,7 @@ def main(): fd.write(str(service)) except IOError: - module.fail_json(msg='Unable to create temporary \ - file %s' % temp_file) + module.fail_json(msg='Unable to create temporary file %s' % temp_file) module.atomic_move(temp_file.name, os.path.realpath(fname)) diff --git a/tests/integration/targets/pamd/tasks/main.yml b/tests/integration/targets/pamd/tasks/main.yml index 3e0fb4ee32..3835ff9db0 100644 --- a/tests/integration/targets/pamd/tasks/main.yml +++ b/tests/integration/targets/pamd/tasks/main.yml @@ -5,11 +5,10 @@ set_fact: test_pamd_file: "/tmp/pamd_file" -- name: Copy temporary pam.d file +- name: Create temporary pam.d file copy: content: "session required pam_lastlog.so silent showfailed" dest: "{{ test_pamd_file }}" - - name: Test working on a single-line file works (2925) community.general.pamd: path: /tmp @@ -20,17 +19,37 @@ module_arguments: silent state: args_absent register: pamd_file_output - - name: Check if changes made assert: that: - pamd_file_output is changed -- name: Copy temporary pam.d file +- name: Test removing all arguments from an entry (3260) + community.general.pamd: + path: /tmp + name: pamd_file + type: session + control: required + module_path: pam_lastlog.so + module_arguments: "" + state: updated + register: pamd_file_output_noargs +- name: Read back the file (3260) + slurp: + src: "{{ test_pamd_file }}" + register: pamd_file_slurp_noargs +- name: Check if changes made (3260) + vars: + line_array: "{{ (pamd_file_slurp_noargs.content|b64decode).split('\n')[2].split() }}" + assert: + that: + - pamd_file_output_noargs is changed + - line_array == ['session', 'required', 'pam_lastlog.so'] + +- name: Create temporary pam.d file copy: content: "" dest: "{{ test_pamd_file }}" - # This test merely demonstrates that, as-is, module will not perform any changes on an empty file # All the existing values for "state" will first search for a rule matching type, control, module_path # and will not perform any change whatsoever if no existing rules match. @@ -43,12 +62,10 @@ module_path: pam_lastlog.so module_arguments: silent register: pamd_file_output_empty - - name: Read back the file slurp: src: "{{ test_pamd_file }}" register: pamd_file_slurp - - name: Check if changes made assert: that: diff --git a/tests/unit/plugins/modules/system/test_pamd.py b/tests/unit/plugins/modules/system/test_pamd.py index e7a6883564..19c9d7352a 100644 --- a/tests/unit/plugins/modules/system/test_pamd.py +++ b/tests/unit/plugins/modules/system/test_pamd.py @@ -218,6 +218,14 @@ auth required pam_deny.so test_rule = PamdRule('auth', 'sufficient', 'pam_unix.so', 'nullok try_first_pass') self.assertNotIn(str(test_rule), str(self.pamd)) + def test_update_rule_remove_module_args(self): + self.assertTrue(self.pamd.update_rule('auth', 'sufficient', 'pam_unix.so', new_args='')) + test_rule = PamdRule('auth', 'sufficient', 'pam_unix.so', '') + self.assertIn(str(test_rule), str(self.pamd)) + + test_rule = PamdRule('auth', 'sufficient', 'pam_unix.so', 'nullok try_first_pass') + self.assertNotIn(str(test_rule), str(self.pamd)) + def test_update_first_three(self): self.assertTrue(self.pamd.update_rule('auth', 'required', 'pam_env.so', new_type='one', new_control='two', new_path='three'))