From 0ae4dac65a6f78dac4fcfce6d1074074eeac2f4e Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Thu, 11 Jun 2020 15:18:59 -0400 Subject: [PATCH] Fix copy module file perms with remote_src (#69993) When using 'remote_src: yes' and 'mode: preserve', the code handling the file modes has to be handled on the remote node because it's the one that has access to the source files. This means that the copy module itself must handle this, rather than the copy action plugin (which is where all that logic exists). The copy module handles this when we copy a single file over. But when it is a directory as the src parameter value, the mode of the files beneath it are not considered. Subdirectories are copied with shutil.copytree() which will preserve permissions automatically. Individual files are copied with shutil.copyfile() which does NOT preserve permissions. We need to add some calls to shutil.copymode() to correct that. Note: This *always* retains individial file permissions. Specifying a 'mode' other than 'preserve' when giving a source directory for the 'src' param does not make sense so will be ignored in that case only. Fixes #69783 * Add changelog and test --- .../fragments/69993-copy-remote-src-perms.yml | 2 + lib/ansible/modules/copy.py | 8 +++ test/integration/targets/copy/tasks/tests.yml | 70 +++++++++++++++++++ 3 files changed, 80 insertions(+) create mode 100644 changelogs/fragments/69993-copy-remote-src-perms.yml diff --git a/changelogs/fragments/69993-copy-remote-src-perms.yml b/changelogs/fragments/69993-copy-remote-src-perms.yml new file mode 100644 index 00000000000..b57e70b04e3 --- /dev/null +++ b/changelogs/fragments/69993-copy-remote-src-perms.yml @@ -0,0 +1,2 @@ +bugfixes: + - copy - Fix copy modes when using remote_src=yes and src is a directory with trailing slash. diff --git a/lib/ansible/modules/copy.py b/lib/ansible/modules/copy.py index add01b056d1..7f7844a217b 100644 --- a/lib/ansible/modules/copy.py +++ b/lib/ansible/modules/copy.py @@ -395,6 +395,8 @@ def chown_recursive(path, module): def copy_diff_files(src, dest, module): + """Copy files that are different between `src` directory and `dest` directory.""" + changed = False owner = module.params['owner'] group = module.params['group'] @@ -413,6 +415,7 @@ def copy_diff_files(src, dest, module): os.symlink(linkto, b_dest_item_path) 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) @@ -423,6 +426,8 @@ def copy_diff_files(src, dest, module): def copy_left_only(src, dest, module): + """Copy files that exist in `src` directory only to the `dest` directory.""" + changed = False owner = module.params['owner'] group = module.params['group'] @@ -458,6 +463,8 @@ def copy_left_only(src, dest, module): if not os.path.islink(b_src_item_path) and os.path.isfile(b_src_item_path): 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: @@ -718,6 +725,7 @@ def main(): else: changed = False + # If neither have checksums, both src and dest are directories. if checksum_src is None and checksum_dest is None: if remote_src and os.path.isdir(module.params['src']): b_src = to_bytes(module.params['src'], errors='surrogate_or_strict') diff --git a/test/integration/targets/copy/tasks/tests.yml b/test/integration/targets/copy/tasks/tests.yml index 8e47e4755ef..be9553179e2 100644 --- a/test/integration/targets/copy/tasks/tests.yml +++ b/test/integration/targets/copy/tasks/tests.yml @@ -2189,3 +2189,73 @@ - "stat_remote_dir_src_link_file12_before.stat.gr_name == stat_remote_dir_src_link_file12_after.stat.gr_name" - "stat_remote_dir_src_link_file12_before.stat.path == stat_remote_dir_src_link_file12_after.stat.path" - "stat_remote_dir_src_link_file12_before.stat.mode == stat_remote_dir_src_link_file12_after.stat.mode" + +# Test for issue 69783: copy with remote_src=yes and src='dir/' preserves all permissions +- block: + - name: Create directory structure + file: + path: "{{ local_temp_dir }}/test69783/{{ item }}" + state: directory + loop: + - "src/dir" + - "dest" + + - name: Create source file structure + file: + path: "{{ local_temp_dir }}/test69783/src/{{ item.name }}" + state: touch + mode: "{{ item.mode }}" + loop: + - { name: 'readwrite', mode: '0644' } + - { name: 'executable', mode: '0755' } + - { name: 'readonly', mode: '0444' } + - { name: 'dir/readwrite', mode: '0644' } + - { name: 'dir/executable', mode: '0755' } + - { name: 'dir/readonly', mode: '0444' } + + - name: Recursive remote copy with preserve + copy: + src: "{{ local_temp_dir }}/test69783/src/" + dest: "{{ local_temp_dir }}/test69783/dest/" + remote_src: yes + mode: preserve + + - name: Stat dest 'readwrite' file + stat: + path: "{{ local_temp_dir}}/test69783/dest/readwrite" + register: dest_readwrite_stat + + - name: Stat dest 'executable' file + stat: + path: "{{ local_temp_dir}}/test69783/dest/executable" + register: dest_executable_stat + + - name: Stat dest 'readonly' file + stat: + path: "{{ local_temp_dir}}/test69783/dest/readonly" + register: dest_readonly_stat + + - name: Stat dest 'dir/readwrite' file + stat: + path: "{{ local_temp_dir}}/test69783/dest/dir/readwrite" + register: dest_dir_readwrite_stat + + - name: Stat dest 'dir/executable' file + stat: + path: "{{ local_temp_dir}}/test69783/dest/dir/executable" + register: dest_dir_executable_stat + + - name: Stat dest 'dir/readonly' file + stat: + path: "{{ local_temp_dir}}/test69783/dest/dir/readonly" + register: dest_dir_readonly_stat + + - name: Assert modes are preserved + assert: + that: + - "dest_readwrite_stat.stat.mode == '0644'" + - "dest_executable_stat.stat.mode == '0755'" + - "dest_readonly_stat.stat.mode == '0444'" + - "dest_dir_readwrite_stat.stat.mode == '0644'" + - "dest_dir_executable_stat.stat.mode == '0755'" + - "dest_dir_readonly_stat.stat.mode == '0444'"