ensure unsafe writes fallback (#70722) (#73144)

* Ensure we actually fallback to unsafe_writes when set to true

 add integration test
 add fix for get_url not passing the parameter from args

(cherry picked from commit 932ba36160)

* Added clog missing for issue 70722 (#73175)

(cherry picked from commit d6670da1d7)

changelogs/fragments/unsafe_writes_fix.yml

@@ -0,0 +1,4 @@
+bugfixes:
+  - Restored unsafe_writes functionality which was being skipped.
+  - Added unsafe_writes test.
+  - Enabled unsafe_writes for get_url which was ignoring the paramter.

lib/ansible/module_utils/basic.py

@@ -2375,8 +2375,7 @@ class AnsibleModule(object):
             if e.errno not in [errno.EPERM, errno.EXDEV, errno.EACCES, errno.ETXTBSY, errno.EBUSY]:
                 # only try workarounds for errno 18 (cross device), 1 (not permitted),  13 (permission denied)
                 # and 26 (text file busy) which happens on vagrant synced folders and other 'exotic' non posix file systems
-                self.fail_json(msg='Could not replace file: %s to %s: %s' % (src, dest, to_native(e)),
+                self.fail_json(msg='Could not replace file: %s to %s: %s' % (src, dest, to_native(e)), exception=traceback.format_exc())
-                               exception=traceback.format_exc())
             else:
                 # Use bytes here.  In the shippable CI, this fails with
                 # a UnicodeError with surrogateescape'd strings for an unknown
@@ -2386,14 +2385,13 @@ class AnsibleModule(object):
                 error_msg = None
                 tmp_dest_name = None
                 try:
-                    tmp_dest_fd, tmp_dest_name = tempfile.mkstemp(prefix=b'.ansible_tmp',
+                    tmp_dest_fd, tmp_dest_name = tempfile.mkstemp(prefix=b'.ansible_tmp', dir=b_dest_dir, suffix=b_suffix)
-                                                                  dir=b_dest_dir, suffix=b_suffix)
                 except (OSError, IOError) as e:
                     error_msg = 'The destination directory (%s) is not writable by the current user. Error was: %s' % (os.path.dirname(dest), to_native(e))
                 except TypeError:
                     # We expect that this is happening because python3.4.x and
-                    # below can't handle byte strings in mkstemp().  Traceback
+                    # below can't handle byte strings in mkstemp().
-                    # would end in something like:
+                    # Traceback would end in something like:
                     #     file = _os.path.join(dir, pre + name + suf)
                     # TypeError: can't concat bytes to str
                     error_msg = ('Failed creating tmp file for atomic move.  This usually happens when using Python3 less than Python3.5. '
@@ -2437,11 +2435,12 @@ class AnsibleModule(object):
                                     self._unsafe_writes(b_tmp_dest_name, b_dest)
                                 else:
                                     self.fail_json(msg='Unable to make %s into to %s, failed final rename from %s: %s' %
-                                                       (src, dest, b_tmp_dest_name, to_native(e)),
+                                                       (src, dest, b_tmp_dest_name, to_native(e)), exception=traceback.format_exc())
-                                                   exception=traceback.format_exc())
                         except (shutil.Error, OSError, IOError) as e:
-                            self.fail_json(msg='Failed to replace file: %s to %s: %s' % (src, dest, to_native(e)),
+                            if unsafe_writes:
-                                           exception=traceback.format_exc())
+                                self._unsafe_writes(b_src, b_dest)
+                            else:
+                                self.fail_json(msg='Failed to replace file: %s to %s: %s' % (src, dest, to_native(e)), exception=traceback.format_exc())
                     finally:
                         self.cleanup(b_tmp_dest_name)
 

lib/ansible/modules/get_url.py

@@ -610,7 +610,7 @@ def main():
             if backup:
                 if os.path.exists(dest):
                     backup_file = module.backup_local(dest)
-            module.atomic_move(tmpsrc, dest)
+            module.atomic_move(tmpsrc, dest, unsafe_writes=module.params['unsafe_writes'])
         except Exception as e:
             if os.path.exists(tmpsrc):
                 os.remove(tmpsrc)

test/integration/targets/unsafe_writes/aliases

@@ -0,0 +1,6 @@
+needs/root
+skip/freebsd
+skip/osx
+skip/macos
+skip/aix
+shippable/posix/group3

test/integration/targets/unsafe_writes/basic.yml

@@ -0,0 +1,53 @@
+- hosts: testhost
+  gather_facts: false
+  vars:
+    testudir: '{{output_dir}}/unsafe_writes_test'
+    testufile: '{{testudir}}/unreplacablefile.txt'
+  tasks:
+    - name: test unsafe_writes on immutable dir (file cannot be atomically replaced)
+      block:
+      - name: create target dir
+        file: path={{testudir}} state=directory
+      - name: setup test file
+        copy: content=ORIGINAL dest={{testufile}}
+      - name: make target dir immutable (cannot write to file w/o unsafe_writes)
+        file: path={{testudir}} state=directory attributes="+i"
+        become: yes
+        ignore_errors: true
+        register: madeimmutable
+
+      - name: only run if immutable dir command worked, some of our test systems don't allow for it
+        when: madeimmutable is success
+        block:
+        - name: test this is actually immmutable working as we expect
+          file: path={{testufile}} state=absent
+          register: breakimmutable
+          ignore_errors: True
+
+        - name: only run if reallyh immutable dir
+          when: breakimmutable is failed
+          block:
+          - name: test overwriting file w/o unsafe
+            copy: content=NEW dest={{testufile}} unsafe_writes=False
+            ignore_errors: true
+            register: copy_without
+
+          - name: ensure we properly failed
+            assert:
+              that:
+                - copy_without is failed
+
+          - name: test overwriting file with unsafe
+            copy: content=NEW dest={{testufile}} unsafe_writes=True
+            register: copy_with
+
+          - name: ensure we properly changed
+            assert:
+              that:
+                - copy_with is changed
+
+      always:
+      - name: remove immutable flag from dir to prevent issues with cleanup
+        file: path={{testudir}} state=directory attributes="-i"
+        ignore_errors: true
+        become: yes

test/integration/targets/unsafe_writes/runme.sh

@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+set -eux
+
+ansible-playbook basic.yml -i ../../inventory -e "output_dir=${OUTPUT_DIR}" "$@"

test/units/module_utils/basic/test_atomic_move.py

@@ -23,6 +23,7 @@ def atomic_am(am, mocker):
     am.selinux_context = mocker.MagicMock()
     am.selinux_default_context = mocker.MagicMock()
     am.set_context_if_different = mocker.MagicMock()
+    am._unsafe_writes = mocker.MagicMock()
 
     yield am