Merge 756c695d67 into 6aa8192131

changelogs/fragments/copy-module-fix-recursive-mode.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - copy - fix setting ``mode`` and ``directory_mode`` recursively when copying a remote directory (https://github.com/ansible/ansible/issues/81126).

lib/ansible/module_utils/basic.py

@@ -683,9 +683,6 @@ class AnsibleModule(object):
         if not self.selinux_enabled():
             return changed
 
-        if self.check_file_absent_if_check_mode(path):
-            return True
-
         cur_context = self.selinux_context(path)
         new_context = list(cur_context)
         # Iterate over the current context instead of the

lib/ansible/modules/copy.py

@@ -82,10 +82,10 @@ options:
       See CVE-2020-1736 for further details.
   directory_mode:
     description:
-    - Set the access permissions of newly created directories to the given mode.
-      Permissions on existing directories do not change.
-    - See O(mode) for the syntax of accepted values.
-    - The target system's defaults determine permissions when this parameter is not set.
+    - Set the access permissions of all directories copied from the O(src) to the given mode.
+      When O(dest) ends with a trailing slash, any intermediate destination directories created also use this mode.
+    - See O(mode) for the syntax of accepted values (V(preserve) is not supported).
+    - The target system's defaults determine permissions for newly created directories when this parameter is not set.
     type: raw
     version_added: '1.5'
   remote_src:
@@ -287,10 +287,8 @@ state:
 
 import errno
 import filecmp
-import grp
 import os
 import os.path
-import pwd
 import shutil
 import stat
 import tempfile
@@ -323,81 +321,25 @@ def split_pre_existing_dir(dirname):
     return (pre_existing_dir, new_directory_list)
 
 
-def adjust_recursive_directory_permissions(pre_existing_dir, new_directory_list, module, directory_args, changed):
-    '''
-    Walk the new directories list and make sure that permissions are as we would expect
-    '''
-
-    if new_directory_list:
-        working_dir = os.path.join(pre_existing_dir, new_directory_list.pop(0))
-        directory_args['path'] = working_dir
-        changed = module.set_fs_attributes_if_different(directory_args, changed)
-        changed = adjust_recursive_directory_permissions(working_dir, new_directory_list, module, directory_args, changed)
-    return changed
-
-
-def chown_recursive(path, module):
+def walk_path_and_apply_fs_attributes(path, module, file_args, directory_args):
     changed = False
-    owner = module.params['owner']
-    group = module.params['group']
-
-    if owner is not None:
-        if not module.check_mode:
-            for dirpath, dirnames, filenames in os.walk(path):
-                owner_changed = module.set_owner_if_different(dirpath, owner, False)
-                if owner_changed is True:
-                    changed = owner_changed
-                for dir in [os.path.join(dirpath, d) for d in dirnames]:
-                    owner_changed = module.set_owner_if_different(dir, owner, False)
-                    if owner_changed is True:
-                        changed = owner_changed
-                for file in [os.path.join(dirpath, f) for f in filenames]:
-                    owner_changed = module.set_owner_if_different(file, owner, False)
-                    if owner_changed is True:
-                        changed = owner_changed
-        else:
-            uid = pwd.getpwnam(owner).pw_uid
-            for dirpath, dirnames, filenames in os.walk(path):
-                owner_changed = (os.stat(dirpath).st_uid != uid)
-                if owner_changed is True:
-                    changed = owner_changed
-                for dir in [os.path.join(dirpath, d) for d in dirnames]:
-                    owner_changed = (os.stat(dir).st_uid != uid)
-                    if owner_changed is True:
-                        changed = owner_changed
-                for file in [os.path.join(dirpath, f) for f in filenames]:
-                    owner_changed = (os.stat(file).st_uid != uid)
-                    if owner_changed is True:
-                        changed = owner_changed
-    if group is not None:
-        if not module.check_mode:
-            for dirpath, dirnames, filenames in os.walk(path):
-                group_changed = module.set_group_if_different(dirpath, group, False)
-                if group_changed is True:
-                    changed = group_changed
-                for dir in [os.path.join(dirpath, d) for d in dirnames]:
-                    group_changed = module.set_group_if_different(dir, group, False)
-                    if group_changed is True:
-                        changed = group_changed
-                for file in [os.path.join(dirpath, f) for f in filenames]:
-                    group_changed = module.set_group_if_different(file, group, False)
-                    if group_changed is True:
-                        changed = group_changed
-        else:
-            gid = grp.getgrnam(group).gr_gid
-            for dirpath, dirnames, filenames in os.walk(path):
-                group_changed = (os.stat(dirpath).st_gid != gid)
-                if group_changed is True:
-                    changed = group_changed
-                for dir in [os.path.join(dirpath, d) for d in dirnames]:
-                    group_changed = (os.stat(dir).st_gid != gid)
-                    if group_changed is True:
-                        changed = group_changed
-                for file in [os.path.join(dirpath, f) for f in filenames]:
-                    group_changed = (os.stat(file).st_gid != gid)
-                    if group_changed is True:
-                        changed = group_changed
 
+    if file_args['mode'] == 'preserve':
+        # preserve is handled in copy_diff_files and copy_left_only where we have the src file
+        file_args['mode'] = None
+
+    if os.path.isfile(path):
+        file_args['path'] = path
+        changed |= module.set_fs_attributes_if_different(file_args, changed)
+        return changed
+
+    for dirpath, dirnames, filenames in os.walk(path):
+        for _dir in [dirpath] + [os.path.join(dirpath, d) for d in dirnames]:
+            directory_args['path'] = _dir
+            changed |= module.set_fs_attributes_if_different(directory_args, changed)
+        for _file in [os.path.join(dirpath, f) for f in filenames]:
+            file_args['path'] = _file
+            changed |= module.set_fs_attributes_if_different(file_args, changed)
     return changed
 
 
@@ -423,11 +365,6 @@ def copy_diff_files(src, dest, module):
             else:
                 shutil.copyfile(b_src_item_path, b_dest_item_path)
                 shutil.copymode(b_src_item_path, b_dest_item_path)
-
-            if owner is not None:
-                module.set_owner_if_different(b_dest_item_path, owner, False)
-            if group is not None:
-                module.set_group_if_different(b_dest_item_path, group, False)
             changed = True
     return changed
 
@@ -451,7 +388,6 @@ def copy_left_only(src, dest, module):
 
             if os.path.islink(b_src_item_path) and os.path.isdir(b_src_item_path) and local_follow is True:
                 shutil.copytree(b_src_item_path, b_dest_item_path, symlinks=not local_follow)
-                chown_recursive(b_dest_item_path, module)
 
             if os.path.islink(b_src_item_path) and os.path.isdir(b_src_item_path) and local_follow is False:
                 linkto = os.readlink(b_src_item_path)
@@ -459,10 +395,6 @@ def copy_left_only(src, dest, module):
 
             if os.path.islink(b_src_item_path) and os.path.isfile(b_src_item_path) and local_follow is True:
                 shutil.copyfile(b_src_item_path, b_dest_item_path)
-                if owner is not None:
-                    module.set_owner_if_different(b_dest_item_path, owner, False)
-                if group is not None:
-                    module.set_group_if_different(b_dest_item_path, group, False)
 
             if os.path.islink(b_src_item_path) and os.path.isfile(b_src_item_path) and local_follow is False:
                 linkto = os.readlink(b_src_item_path)
@@ -472,14 +404,8 @@ def copy_left_only(src, dest, module):
                 shutil.copyfile(b_src_item_path, b_dest_item_path)
                 shutil.copymode(b_src_item_path, b_dest_item_path)
 
-                if owner is not None:
-                    module.set_owner_if_different(b_dest_item_path, owner, False)
-                if group is not None:
-                    module.set_group_if_different(b_dest_item_path, group, False)
-
             if not os.path.islink(b_src_item_path) and os.path.isdir(b_src_item_path):
                 shutil.copytree(b_src_item_path, b_dest_item_path, symlinks=not local_follow)
-                chown_recursive(b_dest_item_path, module)
 
             changed = True
     return changed
@@ -538,7 +464,6 @@ def main():
     validate = module.params.get('validate', None)
     follow = module.params['follow']
     local_follow = module.params['local_follow']
-    mode = module.params['mode']
     owner = module.params['owner']
     group = module.params['group']
     remote_src = module.params['remote_src']
@@ -552,8 +477,9 @@ def main():
     # Preserve is usually handled in the action plugin but mode + remote_src has to be done on the
     # remote host
     if module.params['mode'] == 'preserve':
-        module.params['mode'] = '0%03o' % stat.S_IMODE(os.stat(b_src).st_mode)
-    mode = module.params['mode']
+        mode = '0%03o' % stat.S_IMODE(os.stat(b_src).st_mode)
+    else:
+        mode = module.params['mode']
 
     changed = False
 
@@ -582,6 +508,7 @@ def main():
         )
 
     # Special handling for recursive copy - create intermediate dirs
+    created_intermediate_dir = None
     if dest.endswith(os.sep):
         if _original_basename:
             dest = os.path.join(dest, _original_basename)
@@ -599,13 +526,7 @@ def main():
                 module.exit_json(msg='dest directory %s would be created' % dirname, changed=True, src=src)
             os.makedirs(b_dirname)
             changed = True
-            directory_args = module.load_file_common_arguments(module.params)
-            directory_mode = module.params["directory_mode"]
-            if directory_mode is not None:
-                directory_args['mode'] = directory_mode
-            else:
-                directory_args['mode'] = None
-            adjust_recursive_directory_permissions(pre_existing_dir, new_directory_list, module, directory_args, changed)
+            created_intermediate_dir = os.path.join(pre_existing_dir, new_directory_list[0]) if new_directory_list else dirname
 
     if os.path.isdir(b_dest):
         basename = os.path.basename(src)
@@ -653,12 +574,9 @@ def main():
                 if validate:
                     # if we have a mode, make sure we set it on the temporary
                     # file source as some validations may require it
-                    if mode is not None:
-                        module.set_mode_if_different(src, mode, False)
-                    if owner is not None:
-                        module.set_owner_if_different(src, owner, False)
-                    if group is not None:
-                        module.set_group_if_different(src, group, False)
+                    module.set_mode_if_different(src, mode, False)
+                    module.set_owner_if_different(src, owner, False)
+                    module.set_group_if_different(src, group, False)
                     if "%s" not in validate:
                         module.fail_json(msg="validate must contain %%s: %s" % (validate))
                     (rc, out, err) = module.run_command(validate % src)
@@ -696,8 +614,7 @@ def main():
                 diff_files_changed = copy_diff_files(b_src, b_dest, module)
                 left_only_changed = copy_left_only(b_src, b_dest, module)
                 common_dirs_changed = copy_common_dirs(b_src, b_dest, module)
-                owner_group_changed = chown_recursive(b_dest, module)
-                if diff_files_changed or left_only_changed or common_dirs_changed or owner_group_changed:
+                if diff_files_changed or left_only_changed or common_dirs_changed:
                     changed = True
 
             if src.endswith(os.path.sep) and not os.path.exists(module.params['dest']):
@@ -706,7 +623,6 @@ def main():
                 b_src = to_bytes(os.path.join(module.params['src'], ""), errors='surrogate_or_strict')
                 if not module.check_mode:
                     shutil.copytree(b_src, b_dest, symlinks=not local_follow)
-                chown_recursive(dest, module)
                 changed = True
 
             if not src.endswith(os.path.sep) and os.path.isdir(module.params['dest']):
@@ -716,15 +632,13 @@ def main():
                 if not module.check_mode and not os.path.exists(b_dest):
                     shutil.copytree(b_src, b_dest, symlinks=not local_follow)
                     changed = True
-                    chown_recursive(dest, module)
                 if module.check_mode and not os.path.exists(b_dest):
                     changed = True
                 if os.path.exists(b_dest):
                     diff_files_changed = copy_diff_files(b_src, b_dest, module)
                     left_only_changed = copy_left_only(b_src, b_dest, module)
                     common_dirs_changed = copy_common_dirs(b_src, b_dest, module)
-                    owner_group_changed = chown_recursive(b_dest, module)
-                    if diff_files_changed or left_only_changed or common_dirs_changed or owner_group_changed:
+                    if diff_files_changed or left_only_changed or common_dirs_changed:
                         changed = True
 
             if not src.endswith(os.path.sep) and not os.path.exists(module.params['dest']):
@@ -737,7 +651,6 @@ def main():
                     diff_files_changed = copy_diff_files(b_src, b_dest, module)
                     left_only_changed = copy_left_only(b_src, b_dest, module)
                     common_dirs_changed = copy_common_dirs(b_src, b_dest, module)
-                    owner_group_changed = chown_recursive(b_dest, module)
                 if module.check_mode and not os.path.exists(b_dest):
                     changed = True
 
@@ -747,8 +660,12 @@ def main():
     if backup_file:
         res_args['backup_file'] = backup_file
 
-    file_args = module.load_file_common_arguments(module.params, path=dest)
-    res_args['changed'] = module.set_fs_attributes_if_different(file_args, res_args['changed'])
+    file_args = module.load_file_common_arguments(module.params, path=created_intermediate_dir or dest)
+    if file_args['mode'] == 'preserve' and os.path.isfile(dest):
+        file_args['mode'] = mode
+    directory_args = file_args.copy()
+    directory_args['mode'] = module.params['directory_mode']
+    res_args['changed'] |= walk_path_and_apply_fs_attributes(file_args['path'], module, file_args, directory_args)
 
     module.exit_json(**res_args)
 

test/integration/targets/copy/tasks/tests.yml

@@ -1670,6 +1670,7 @@
     file:
       path: '{{ remote_dir }}/testcase1_local_follow_true'
       state: directory
+      mode: '0555'
 
   - name: execute - Copy the directory on remote with local_follow True
     copy:
@@ -1677,6 +1678,8 @@
       src: '{{ remote_dir }}/remote_dir_src/'
       dest: '{{ remote_dir }}/testcase1_local_follow_true'
       local_follow: True
+      mode: '0555'
+      directory_mode: '0555'
     register: testcase1
 
   - name: gather - Stat the testcase1_local_follow_true
@@ -1705,15 +1708,56 @@
       that:
       - testcase1 is changed
       - "stat_testcase1_local_follow_true.stat.isdir"
+      - "stat_testcase1_local_follow_true.stat.mode == '0555'"
       - "stat_testcase1_local_follow_true_subdir.stat.isdir"
+      - "stat_testcase1_local_follow_true_subdir.stat.mode == '0555'"
       - "stat_testcase1_local_follow_true_file1.stat.exists"
+      - "stat_testcase1_local_follow_true_file1.stat.mode == '0555'"
       - "stat_remote_dir_src_file1_before.stat.checksum == stat_testcase1_local_follow_true_file1.stat.checksum"
       - "stat_testcase1_local_follow_true_subdir_file12.stat.exists"
       - "stat_remote_dir_src_subdir_file12_before.stat.checksum == stat_testcase1_local_follow_true_subdir_file12.stat.checksum"
       - "stat_testcase1_local_follow_true_link_file12.stat.exists"
       - "not stat_testcase1_local_follow_true_link_file12.stat.islnk"
+      - "stat_testcase1_local_follow_true_link_file12.stat.mode == '0555'"
       - "stat_remote_dir_src_subdir_file12_before.stat.checksum == stat_testcase1_local_follow_true_link_file12.stat.checksum"
 
+  - name: execute - Copy the directory on remote with local_follow True again
+    copy:
+      remote_src: True
+      src: '{{ remote_dir }}/remote_dir_src/'
+      dest: '{{ remote_dir }}/testcase1_local_follow_true'
+      local_follow: True
+      mode: '0555'
+      directory_mode: '0555'
+    register: testcase1_idempotent
+
+  - name: execute - Copy the directory on remote with local_follow True and modified mode
+    copy:
+      remote_src: True
+      src: '{{ remote_dir }}/remote_dir_src/'
+      dest: '{{ remote_dir }}/testcase1_local_follow_true'
+      local_follow: True
+      mode: '0547'
+      directory_mode: '0547'
+    register: testcase1_modified_mode
+
+  - name: gather - stat modified testcase1 files
+    stat:
+      path: '{{ remote_dir }}/{{ item }}'
+    register: testcase1_modified_stat
+    loop:
+      - testcase1_local_follow_true
+      - testcase1_local_follow_true/subdir
+      - testcase1_local_follow_true/file1
+      - testcase1_local_follow_true/subdir/file12
+
+  - name: assert - remote_dir_src has copied with local_follow True.
+    assert:
+      that:
+      - testcase1_idempotent is not changed
+      - testcase1_modified_mode is changed
+      - testcase1_modified_stat.results | map(attribute='stat') | map(attribute='mode') | unique == ['0547']
+
 ### local_follow: False
   - name: execute - Create a test dest dir
     file: