pamd: fix issue with trailing commas in module_arguments

lib/ansible/modules/system/pamd.py

@@ -1,9 +1,10 @@
 #!/usr/bin/python
 # -*- coding: utf-8 -*-
-# (c) 2016, Kenneth D. Evensen <kevensen@redhat.com>
+# (c) 2017, Kenneth D. Evensen <kevensen@redhat.com>
+
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
-from __future__ import absolute_import, division, print_function
+from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
 
@@ -62,7 +63,8 @@ options:
       'args_present' any args listed in module_arguments are added if
       missing from the existing rule.  Furthermore, if the module argument
       takes a value denoted by '=', the value will be changed to that specified
-      in module_arguments.
+      in module_arguments.  Note that module_arguments is a list.  Please see
+      the examples for usage.
   state:
     default: updated
     choices:
@@ -157,7 +159,7 @@ EXAMPLES = """
     name: system-auth
     type: session control='[success=1 default=ignore]'
     module_path: pam_succeed_if.so
-    module_arguments: 'crond quiet'
+    module_arguments: crond,quiet
     state: args_absent
 
 - name: Ensure specific arguments are present in a rule
@@ -166,7 +168,28 @@ EXAMPLES = """
     type: session
     control: '[success=1 default=ignore]'
     module_path: pam_succeed_if.so
-    module_arguments: 'crond quiet'
+    module_arguments: crond,quiet
+    state: args_present
+
+- name: Ensure specific arguments are present in a rule (alternative)
+  pamd:
+    name: system-auth
+    type: session
+    control: '[success=1 default=ignore]'
+    module_path: pam_succeed_if.so
+    module_arguments:
+    - crond
+    - quiet
+    state: args_present
+
+- name: Module arguments requiring commas must be listed as a Yaml list
+  pamd:
+    name: special-module
+    type: account
+    control: required
+    module_path: pam_access.so
+    module_arguments:
+    - listsep=,
     state: args_present
 
 - name: Update specific argument value in a rule
@@ -219,6 +242,7 @@ dest:
 ...
 '''
 
+
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.pycompat24 import get_exception
 import os
@@ -259,18 +283,18 @@ class PamdRule(object):
 
         if '[' in stringline:
             pattern = re.compile(
-                r"""([\-A-Za-z0-9_]+)\s*        # Rule Type
-                    \[([A-Za-z0-9_=\s]+)\]\s*   # Rule Control
-                    ([A-Za-z0-9_\.]+)\s*        # Rule Path
-                    ([A-Za-z0-9_=<>\-\s]*)""",  # Rule Args
+                r"""([\-A-Za-z0-9_]+)\s*         # Rule Type
+                    \[([A-Za-z0-9_=\s]+)\]\s*    # Rule Control
+                    ([A-Za-z0-9_\-\.]+)\s*         # Rule Path
+                    ([A-Za-z0-9,_=<>\-\s]*)""",  # Rule Args
                 re.X)
             complicated = True
         else:
             pattern = re.compile(
                 r"""([\-A-Za-z0-9_]+)\s*        # Rule Type
                     ([A-Za-z0-9_]+)\s*          # Rule Control
-                    ([A-Za-z0-9_\.]+)\s*        # Rule Path
-                    ([A-Za-z0-9_=<>\-\s]*)""",  # Rule Args
+                    ([A-Za-z0-9_\-\.]+)\s*        # Rule Path
+                    ([A-Za-z0-9,_=<>\-\s]*)""",  # Rule Args
                 re.X)
 
         result = pattern.match(stringline)

test/units/modules/system/test_pamd.py

@@ -52,39 +52,47 @@ class PamdRuleTestCase(unittest.TestCase):
         self.assertEqual(rule, module_string.rstrip())
         self.assertEqual('uid >= 1025 quiet_success', module.get_module_args_as_string())
 
+    def test_trailing_comma(self):
+        rule = "account required pam_access.so listsep=,"
+        module = PamdRule.rulefromstring(stringline=rule)
+        module_string = re.sub(' +', ' ', str(module).replace('\t', ' '))
+        self.assertEqual(rule, module_string.rstrip())
+
 
 class PamdServiceTestCase(unittest.TestCase):
     def setUp(self):
         self.system_auth_string = """#%PAM-1.0
 # This file is auto-generated.
 # User changes will be destroyed the next time authconfig is run.
-auth      	required	pam_env.so
-auth      	sufficient	pam_unix.so nullok try_first_pass
-auth      	requisite	pam_succeed_if.so uid
-auth      	required	pam_deny.so
-
-account   	required	pam_unix.so
-account   	sufficient	pam_localuser.so
-account   	sufficient	pam_succeed_if.so uid
-account   	required	pam_permit.so
-
-password  	requisite	pam_pwquality.so try_first_pass local_users_only retry=3 authtok_type=
-password  	sufficient	pam_unix.so sha512 shadow nullok try_first_pass use_authtok
-password  	required	pam_deny.so
-
-session   	optional	pam_keyinit.so revoke
-session   	required	pam_limits.so
--session  	optional	pam_systemd.so
-session   	[success=1 default=ignore]	pam_succeed_if.so service in crond quiet use_uid
-session   	[success=1 test=me default=ignore]	pam_succeed_if.so service in crond quiet use_uid
-session   	required	pam_unix.so"""
+auth      \trequired\tpam_env.so
+auth      \tsufficient\tpam_unix.so nullok try_first_pass
+auth      \trequisite\tpam_succeed_if.so uid
+auth      \trequired\tpam_deny.so
+
+account   \trequired\tpam_unix.so
+account   \tsufficient\tpam_localuser.so
+account   \tsufficient\tpam_succeed_if.so uid
+account   \trequired\tpam_permit.so
+account   \trequired\tpam_access.so listsep=,
+session   \tinclude\tsystem-auth
+
+password  \trequisite\tpam_pwquality.so try_first_pass local_users_only retry=3 authtok_type=
+password  \tsufficient\tpam_unix.so sha512 shadow nullok try_first_pass use_authtok
+password  \trequired\tpam_deny.so
+
+session   \toptional\tpam_keyinit.so revoke
+session   \trequired\tpam_limits.so
+-session  \toptional\tpam_systemd.so
+session   \t[success=1 default=ignore]\tpam_succeed_if.so service in crond quiet use_uid
+session   \t[success=1 test=me default=ignore]\tpam_succeed_if.so service in crond quiet use_uid
+session   \trequired\tpam_unix.so"""
 
         self.pamd = PamdService()
         self.pamd.load_rules_from_string(self.system_auth_string)
 
     def test_load_rule_from_string(self):
 
-        self.assertEqual(self.system_auth_string, str(self.pamd))
+        self.assertEqual(self.system_auth_string.rstrip().replace("\n\n", "\n"), str(self.pamd).rstrip().replace("\n\n", "\n"))
 
     def test_update_rule_type(self):
         old_rule = PamdRule.rulefromstring('auth      	required	pam_env.so')
@@ -201,3 +209,10 @@ session   	required	pam_unix.so"""
         old_rule = PamdRule.rulefromstring('account   	required	pam_unix.so')
         remove_rule(self.pamd, old_rule)
         self.assertNotIn(str(old_rule).rstrip(), str(self.pamd))
+
+    def test_add_module_args_with_string(self):
+        old_rule = PamdRule.rulefromstring("account     required    pam_access.so")
+        new_rule = PamdRule.rulefromstring("account     required    pam_access.so listsep=,")
+        args_to_add = ['listsep=,']
+        add_module_arguments(self.pamd, old_rule, args_to_add)
+        self.assertIn(str(new_rule).rstrip(), str(self.pamd))