Use file list, not recursion, in _fixup_perms. (#16924)

Run setfacl/chown/chmod on each temp dir and file.

This fixes temp file permissions handling on platforms such as FreeBSD
which always return success when using find -exec. This is done by
eliminating the use of find when setting up temp files and directories.

Additionally, tests that now pass on FreeBSD have been enabled for CI.

11 files changed, 55 insertions, 76 deletions

diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py
index b4f71b17a9..d6e4ffbe98 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -293,7 +293,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
 
         return remote_path
 
-    def _fixup_perms(self, remote_path, remote_user, execute=True, recursive=True):
+    def _fixup_perms(self, remote_paths, remote_user, execute=True):
         """
         We need the files we upload to be readable (and sometimes executable)
         by the user being sudo'd to but we want to limit other people's access
@@ -319,15 +319,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         if self._connection._shell.SHELL_FAMILY == 'powershell':
             # This won't work on Powershell as-is, so we'll just completely skip until
             # we have a need for it, at which point we'll have to do something different.
-            return remote_path
-
-        if remote_path is None:
-            # Sometimes code calls us naively -- it has a var which could
-            # contain a path to a tmp dir but doesn't know if it needs to
-            # exist or not.  If there's no path, then there's no need for us
-            # to do work
-            display.debug('_fixup_perms called with remote_path==None.  Sure this is correct?')
-            return remote_path
+            return remote_paths
 
         if self._play_context.become and self._play_context.become_user not in ('root', remote_user):
             # Unprivileged user that's different than the ssh user.  Let's get
@@ -340,17 +332,17 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             else:
                 mode = 'rX'
 
-            res = self._remote_set_user_facl(remote_path, self._play_context.become_user, mode, recursive=recursive, sudoable=False)
+            res = self._remote_set_user_facl(remote_paths, self._play_context.become_user, mode)
             if res['rc'] != 0:
                 # File system acls failed; let's try to use chown next
                 # Set executable bit first as on some systems an
                 # unprivileged user can use chown
                 if execute:
-                    res = self._remote_chmod('u+x', remote_path, recursive=recursive)
+                    res = self._remote_chmod(remote_paths, 'u+x')
                     if res['rc'] != 0:
                         raise AnsibleError('Failed to set file mode on remote temporary files (rc: {0}, err: {1})'.format(res['rc'], res['stderr']))
 
-                res = self._remote_chown(remote_path, self._play_context.become_user, recursive=recursive)
+                res = self._remote_chown(remote_paths, self._play_context.become_user)
                 if res['rc'] != 0 and remote_user == 'root':
                     # chown failed even if remove_user is root
                     raise AnsibleError('Failed to change ownership of the temporary files Ansible needs to create despite connecting as root.  Unprivileged become user would be unable to read the file.')
@@ -359,7 +351,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
                         # chown and fs acls failed -- do things this insecure
                         # way only if the user opted in in the config file
                         display.warning('Using world-readable permissions for temporary files Ansible needs to create when becoming an unprivileged user which may be insecure. For information on securing this, see https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user')
-                        res = self._remote_chmod('a+%s' % mode, remote_path, recursive=recursive)
+                        res = self._remote_chmod(remote_paths, 'a+%s' % mode)
                         if res['rc'] != 0:
                             raise AnsibleError('Failed to set file mode on remote files (rc: {0}, err: {1})'.format(res['rc'], res['stderr']))
                     else:
@@ -368,33 +360,33 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             # Can't depend on the file being transferred with execute
             # permissions.  Only need user perms because no become was
             # used here
-            res = self._remote_chmod('u+x', remote_path, recursive=recursive)
+            res = self._remote_chmod(remote_paths, 'u+x')
             if res['rc'] != 0:
                 raise AnsibleError('Failed to set file mode on remote files (rc: {0}, err: {1})'.format(res['rc'], res['stderr']))
 
-        return remote_path
+        return remote_paths
 
-    def _remote_chmod(self, mode, path, recursive=True, sudoable=False):
+    def _remote_chmod(self, paths, mode, sudoable=False):
         '''
         Issue a remote chmod command
         '''
-        cmd = self._connection._shell.chmod(mode, path, recursive=recursive)
+        cmd = self._connection._shell.chmod(paths, mode)
         res = self._low_level_execute_command(cmd, sudoable=sudoable)
         return res
 
-    def _remote_chown(self, path, user, group=None, recursive=True, sudoable=False):
+    def _remote_chown(self, paths, user, sudoable=False):
         '''
         Issue a remote chown command
         '''
-        cmd = self._connection._shell.chown(path, user, group, recursive=recursive)
+        cmd = self._connection._shell.chown(paths, user)
         res = self._low_level_execute_command(cmd, sudoable=sudoable)
         return res
 
-    def _remote_set_user_facl(self, path, user, mode, recursive=True, sudoable=False):
+    def _remote_set_user_facl(self, paths, user, mode, sudoable=False):
         '''
         Issue a remote call to setfacl
         '''
-        cmd = self._connection._shell.set_user_facl(path, user, mode, recursive=recursive)
+        cmd = self._connection._shell.set_user_facl(paths, user, mode)
         res = self._low_level_execute_command(cmd, sudoable=sudoable)
         return res
 
@@ -616,9 +608,17 @@ class ActionBase(with_metaclass(ABCMeta, object)):
 
         environment_string = self._compute_environment_string()
 
+        remote_files = None
+
+        if args_file_path:
+            remote_files = tmp, remote_module_path, args_file_path
+        elif remote_module_path:
+            remote_files = tmp, remote_module_path
+
         # Fix permissions of the tmp path and tmp files.  This should be
         # called after all files have been transferred.
-        self._fixup_perms(tmp, remote_user, recursive=True)
+        if remote_files:
+            self._fixup_perms(remote_files, remote_user)
 
         cmd = ""
         in_data = None
diff --git a/lib/ansible/plugins/action/assemble.py b/lib/ansible/plugins/action/assemble.py
index 0898465a03..ed842f729c 100644
--- a/lib/ansible/plugins/action/assemble.py
+++ b/lib/ansible/plugins/action/assemble.py
@@ -159,7 +159,7 @@ class ActionModule(ActionBase):
             xfered = self._transfer_file(path, remote_path)
 
             # fix file permissions when the copy is done as a different user
-            self._fixup_perms(tmp, remote_user, recursive=True)
+            self._fixup_perms((tmp, remote_path), remote_user)
 
             new_module_args.update( dict( src=xfered,))
 
diff --git a/lib/ansible/plugins/action/async.py b/lib/ansible/plugins/action/async.py
index 1ea099437d..ccfebb951a 100644
--- a/lib/ansible/plugins/action/async.py
+++ b/lib/ansible/plugins/action/async.py
@@ -73,17 +73,13 @@ class ActionModule(ActionBase):
                 args_data += '%s="%s" ' % (k, pipes.quote(to_unicode(v)))
             argsfile = self._transfer_data(self._connection._shell.join_path(tmp, 'arguments'), args_data)
 
-        self._fixup_perms(tmp, remote_user, execute=True, recursive=True)
-        # Only the following two files need to be executable but we'd have to
-        # make three remote calls if we wanted to just set them executable.
-        # There's not really a problem with marking too many of the temp files
-        # executable so we go ahead and mark them all as executable in the
-        # line above (the line above is needed in any case [although
-        # execute=False is okay if we uncomment the lines below] so that all
-        # the files are readable in case the remote_user and become_user are
-        # different and both unprivileged)
-        #self._fixup_perms(remote_module_path, remote_user, execute=True, recursive=False)
-        #self._fixup_perms(async_module_path, remote_user, execute=True, recursive=False)
+        remote_paths = tmp, remote_module_path, async_module_path
+
+        # argsfile doesn't need to be executable, but this saves an extra call to the remote host
+        if argsfile:
+            remote_paths += argsfile,
+
+        self._fixup_perms(remote_paths, remote_user, execute=True)
 
         async_limit = self._task.async
         async_jid   = str(random.randint(0, 999999999999))
diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py
index 43a5c6633d..95dc4ade29 100644
--- a/lib/ansible/plugins/action/copy.py
+++ b/lib/ansible/plugins/action/copy.py
@@ -213,8 +213,10 @@ class ActionModule(ActionBase):
                 # Define a remote directory that we will copy the file to.
                 tmp_src = self._connection._shell.join_path(tmp, 'source')
 
+                remote_path = None
+
                 if not raw:
-                    self._transfer_file(source_full, tmp_src)
+                    remote_path = self._transfer_file(source_full, tmp_src)
                 else:
                     self._transfer_file(source_full, dest_file)
 
@@ -223,7 +225,8 @@ class ActionModule(ActionBase):
                 self._loader.cleanup_tmp_file(source_full)
 
                 # fix file permissions when the copy is done as a different user
-                self._fixup_perms(tmp, remote_user, recursive=True)
+                if remote_path:
+                    self._fixup_perms((tmp, remote_path), remote_user)
 
                 if raw:
                     # Continue to next iteration if raw is defined.
diff --git a/lib/ansible/plugins/action/patch.py b/lib/ansible/plugins/action/patch.py
index 7f388fce8f..bd5eb92f30 100644
--- a/lib/ansible/plugins/action/patch.py
+++ b/lib/ansible/plugins/action/patch.py
@@ -63,7 +63,7 @@ class ActionModule(ActionBase):
         tmp_src = self._connection._shell.join_path(tmp, os.path.basename(src))
         self._transfer_file(src, tmp_src)
 
-        self._fixup_perms(tmp, remote_user, recursive=True)
+        self._fixup_perms((tmp, tmp_src), remote_user)
 
         new_module_args = self._task.args.copy()
         new_module_args.update(
diff --git a/lib/ansible/plugins/action/script.py b/lib/ansible/plugins/action/script.py
index 99a8f978cf..7b280ea542 100644
--- a/lib/ansible/plugins/action/script.py
+++ b/lib/ansible/plugins/action/script.py
@@ -81,7 +81,7 @@ class ActionModule(ActionBase):
         self._transfer_file(source, tmp_src)
 
         # set file permissions, more permissive when the copy is done as a different user
-        self._fixup_perms(tmp, remote_user, execute=True, recursive=True)
+        self._fixup_perms((tmp, tmp_src), remote_user, execute=True)
 
         # add preparation steps to one ssh roundtrip executing the script
         env_string = self._compute_environment_string()
diff --git a/lib/ansible/plugins/action/template.py b/lib/ansible/plugins/action/template.py
index 88ccdcd2a5..760db83ba8 100644
--- a/lib/ansible/plugins/action/template.py
+++ b/lib/ansible/plugins/action/template.py
@@ -166,7 +166,7 @@ class ActionModule(ActionBase):
                 xfered = self._transfer_data(self._connection._shell.join_path(tmp, 'source'), resultant)
 
                 # fix file permissions when the copy is done as a different user
-                self._fixup_perms(tmp, remote_user, recursive=True)
+                self._fixup_perms((tmp, xfered), remote_user)
 
                 # run the copy module
                 new_module_args.update(
diff --git a/lib/ansible/plugins/action/unarchive.py b/lib/ansible/plugins/action/unarchive.py
index f558b35e94..a160ad83d2 100644
--- a/lib/ansible/plugins/action/unarchive.py
+++ b/lib/ansible/plugins/action/unarchive.py
@@ -97,7 +97,7 @@ class ActionModule(ActionBase):
 
         if copy:
             # fix file permissions when the copy is done as a different user
-            self._fixup_perms(tmp, remote_user, recursive=True)
+            self._fixup_perms((tmp, tmp_src), remote_user)
             # Build temporary module_args.
             new_module_args = self._task.args.copy()
             new_module_args.update(
diff --git a/lib/ansible/plugins/shell/__init__.py b/lib/ansible/plugins/shell/__init__.py
index 9bd92562b5..44af0abb74 100644
--- a/lib/ansible/plugins/shell/__init__.py
+++ b/lib/ansible/plugins/shell/__init__.py
@@ -57,45 +57,25 @@ class ShellBase(object):
     def path_has_trailing_slash(self, path):
         return path.endswith('/')
 
-    def chmod(self, mode, path, recursive=True):
-        path = pipes.quote(path)
-        cmd = ['chmod']
-
-        if recursive:
-            cmd.append('-R') # many chmods require -R before file list
-
-        cmd.extend([mode, path])
+    def chmod(self, paths, mode):
+        cmd = ['chmod', mode]
+        cmd.extend(paths)
+        cmd = [pipes.quote(c) for c in cmd]
 
         return ' '.join(cmd)
 
-    def chown(self, path, user, group=None, recursive=True):
-        path = pipes.quote(path)
-        user = pipes.quote(user)
-
-        cmd = ['chown']
-
-        if recursive:
-            cmd.append('-R') # many chowns require -R before file list
-
-        if group is None:
-            cmd.extend([user, path])
-        else:
-            group = pipes.quote(group)
-            cmd.extend(['%s:%s' % (user, group), path])
+    def chown(self, paths, user):
+        cmd = ['chown', user]
+        cmd.extend(paths)
+        cmd = [pipes.quote(c) for c in cmd]
 
         return ' '.join(cmd)
 
-    def set_user_facl(self, path, user, mode, recursive=True):
+    def set_user_facl(self, paths, user, mode):
         """Only sets acls for users as that's really all we need"""
-        path = pipes.quote(path)
-        mode = pipes.quote(mode)
-        user = pipes.quote(user)
-
         cmd = ['setfacl', '-m', 'u:%s:%s' % (user, mode)]
-        if recursive:
-            cmd = ['find', path, '-exec'] + cmd + ["'{}'", "'+'"]
-        else:
-            cmd.append(path)
+        cmd.extend(paths)
+        cmd = [pipes.quote(c) for c in cmd]
 
         return ' '.join(cmd)
 
diff --git a/lib/ansible/plugins/shell/powershell.py b/lib/ansible/plugins/shell/powershell.py
index 505b2e01da..1e5c71cf49 100644
--- a/lib/ansible/plugins/shell/powershell.py
+++ b/lib/ansible/plugins/shell/powershell.py
@@ -68,13 +68,13 @@ class ShellModule(object):
         path = self._unquote(path)
         return path.endswith('/') or path.endswith('\\')
 
-    def chmod(self, mode, path, recursive=True):
+    def chmod(self, paths, mode):
         raise NotImplementedError('chmod is not implemented for Powershell')
 
-    def chown(self, path, user, group=None, recursive=True):
+    def chown(self, paths, user):
         raise NotImplementedError('chown is not implemented for Powershell')
 
-    def set_user_facl(self, path, user, mode, recursive=True):
+    def set_user_facl(self, paths, user, mode):
         raise NotImplementedError('set_user_facl is not implemented for Powershell')
 
     def remove(self, path, recurse=False):
diff --git a/test/utils/shippable/remote-integration.sh b/test/utils/shippable/remote-integration.sh
index cf3f6ce729..8b523f225f 100644
--- a/test/utils/shippable/remote-integration.sh
+++ b/test/utils/shippable/remote-integration.sh
@@ -15,7 +15,7 @@ test_flags="${TEST_FLAGS:-}"
 force_color="${FORCE_COLOR:-1}"
 
 # FIXME: these tests fail
-skip_tags='test_copy,test_template,test_unarchive,test_command_shell,test_sudo,test_become,test_service,test_postgresql,test_mysql_db,test_mysql_user,test_mysql_variables,test_uri,test_get_url'
+skip_tags='test_copy,test_template,test_unarchive,test_command_shell,test_service,test_postgresql,test_mysql_db,test_mysql_user,test_mysql_variables,test_uri,test_get_url'
 
 cd ~/