E208: Corrected file mode checking (#1030)

lib/ansiblelint/rules/MissingFilePermissionsRule.py

@@ -19,45 +19,60 @@
 # THE SOFTWARE.
 from ansiblelint.rules import AnsibleLintRule
 
+# Despite documentation mentioning 'preserve' only these modules support it:
+_modules_with_preserve = (
+    'copy',
+    'template',
+)
+
 
 class MissingFilePermissionsRule(AnsibleLintRule):
     id = "208"
-    shortdesc = 'File permissions not mentioned'
+    shortdesc = 'File permissions unset or incorrect'
     description = (
-        "Missing mode parameter can cause unexpected file permissions based "
-        "on version of Ansible being used. Be explicit, or if you still "
-        "want the default behavior you can use ``mode: preserve`` to avoid "
-        "hitting this rule. See "
-        "https://github.com/ansible/ansible/issues/71200"
+        "Missing or unsupported mode parameter can cause unexpected file "
+        "permissions based "
+        "on version of Ansible being used. Be explicit, like ``mode: 0644`` to "
+        "avoid hitting this rule. Special ``preserve`` value is accepted "
+        f"only by {', '.join(_modules_with_preserve)} modules. "
+        "See https://github.com/ansible/ansible/issues/71200"
     )
     severity = 'VERY_HIGH'
     tags = ['unpredictability', 'experimental']
     version_added = 'v4.3.0'
 
-    _modules = (
-        'assemble',
+    _modules = {
         'archive',
-        'copy',
+        'assemble',
+        'copy',  # supports preserve
         'file',
-        'template',
+        'replace',  # implicit preserve behavior but mode: preserve is invalid
+        'template',  # supports preserve
         'unarchive',
-    )
+    }
 
-    _modules_with_create = (
-        'blockinfile',
-        'ini_file',
-        'lineinfile'
-    )
+    _modules_with_create = {
+        'blockinfile': False,
+        'htpasswd': True,
+        'ini_file': True,
+        'lineinfile': False,
+    }
 
     def matchtask(self, file, task):
-        if task["action"]["__ansible_module__"] not in self._modules and \
-                task["action"]["__ansible_module__"] not in self._modules_with_create:
-            return False
+        module = task["action"]["__ansible_module__"]
+        mode = task['action'].get('mode', None)
 
-        if task["action"]["__ansible_module__"] in self._modules_with_create and \
-                not task["action"].get("create", False):
+        if module not in self._modules and \
+                module not in self._modules_with_create:
             return False
 
+        if mode == 'preserve' and module not in _modules_with_preserve:
+            return True
+
+        if module in self._modules_with_create:
+            create = task["action"].get("create", self._modules_with_create[module])
+            return create and mode is None
+
         # A file that doesn't exist cannot have a mode
         if task['action'].get('state', None) == "absent":
             return False
@@ -67,9 +82,14 @@ def matchtask(self, file, task):
             return False
 
         # The file module does not create anything when state==file (default)
-        if task["action"]["__ansible_module__"] == "file" and \
+        if module == "file" and \
                 task['action'].get('state', 'file') == 'file':
             return False
 
-        mode = task['action'].get('mode', None)
+        # replace module is the only one that has a valid default preserve
+        # behavior, but we want to trigger rule if user used incorrect
+        # documentation and put 'preserve', which is not supported.
+        if module == 'replace' and mode is None:
+            return False
+
         return mode is None

test/TestMissingFilePermissionsRule.py

@@ -26,10 +26,6 @@
 ---
 - hosts: hosts
   tasks:
-    - name: permissions not missing and string
-      file:
-        path: foo
-        mode: preserve
     - name: permissions not missing and numeric
       file:
         path: foo
@@ -49,13 +45,21 @@
     - name: file edit when create is false
       lineinfile:
         path: foo
+        create: false
         line: some content here
+    - name: replace should not require mode
+      replace:
+        path: foo
 '''
 
 FAIL_TASKS = '''
 ---
 - hosts: hosts
   tasks:
+    - name: file does not allow preserve value for mode
+      file:
+        path: foo
+        mode: preserve
     - name: permissions missing and might create file
       file:
         path: foo
@@ -64,10 +68,24 @@
       file:
         path: foo
         state: directory
-    - name: permissions needed if create is possible
+    - name: permissions needed if create is used
       ini_file:
         path: foo
         create: true
+    - name: lineinfile when create is true
+      lineinfile:
+        path: foo
+        create: true
+        line: some content here
+    - name: replace does not allow preserve mode
+      replace:
+        path: foo
+        mode: preserve
+    - name: ini_file does not accept preserve mode
+      ini_file:
+        path: foo
+        create: true
+        mode: preserve
 '''
 
 
@@ -82,4 +100,11 @@ def test_success(rule_runner):
 def test_fail(rule_runner):
     """Validate that missing mode triggers the rule."""
     results = rule_runner.run_playbook(FAIL_TASKS)
-    assert len(results) == 3
+    assert len(results) == 7
+    assert results[0].linenumber == 5
+    assert results[1].linenumber == 9
+    assert results[2].linenumber == 13
+    assert results[3].linenumber == 17
+    assert results[4].linenumber == 21
+    assert results[5].linenumber == 26
+    assert results[6].linenumber == 30

test/nomatchestest.yml

@@ -6,4 +6,4 @@
       action: debug msg="Hello!"
 
     - name: this should be fine too
-      action: file state=touch dest=./wherever mode=preserve
+      action: file state=touch dest=./wherever mode=0600

test/unicode.yml

@@ -6,4 +6,4 @@
 
   tasks:
   - name: bonjour, ça va?
-    file: state=touch dest=/tmp/naïve.yml mode=preserve
+    file: state=touch dest=/tmp/naïve.yml mode=0600