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 <felix@fontein.de>

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

plugins/modules/system/pamd.py

@@ -274,8 +274,7 @@ RULE_REGEX = re.compile(r"""(?P<rule_type>-?(?:auth|account|session|password))\s
                         (?P<control>\[.*\]|\S*)\s+
                         (?P<path>\S*)\s*
                         (?P<args>.*)\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):
+                rule_path == self.rule_path)
-            return True
-        return False
 
     @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 new_args is not None:
-                if(current_rule.rule_args != new_args):
+                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]:
+            curr_validate = current_line.validate()
-                return 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):
+def parse_module_arguments(module_arguments, return_none=False):
-    # Return empty list if we have no args to parse
+    # If args is None, return empty list by default.
-    if not module_arguments:
+    # But if return_none is True, then return None
-        return []
+    if module_arguments is None:
-    elif isinstance(module_arguments, list) and len(module_arguments) == 1 and not module_arguments[0]:
+        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 \
+            module.fail_json(msg='Unable to create temporary file %s' % temp_file)
-                                    file %s' % temp_file)
 
         module.atomic_move(temp_file.name, os.path.realpath(fname))
 

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:

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