From ebc91a9b93bbf53f43ebe23f5950e9459ce719d7 Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Tue, 6 Oct 2020 10:25:03 -0500 Subject: [PATCH] [unarchive] work on older RHEL with group: (#72098) Change: - No longer fail due to old Fedora/RHEL and our failure to try to cast gids to integers before trying to pass them to getgrgid() before trying to use them. - Add tests for user/mode for various unarchive formats. Test Plan: - New integration tests, ran against centos6 container Tickets: - Fixes #71903 --- .../fragments/71903-unarchive-gid-cast.yml | 2 + lib/ansible/modules/unarchive.py | 8 +- .../unarchive/tasks/test_owner_group.yml | 162 ++++++++++++++++++ .../targets/unarchive/tasks/test_tar.yml | 7 + .../targets/unarchive/tasks/test_tar_gz.yml | 7 + .../targets/unarchive/tasks/test_zip.yml | 12 ++ 6 files changed, 195 insertions(+), 3 deletions(-) create mode 100644 changelogs/fragments/71903-unarchive-gid-cast.yml create mode 100644 test/integration/targets/unarchive/tasks/test_owner_group.yml diff --git a/changelogs/fragments/71903-unarchive-gid-cast.yml b/changelogs/fragments/71903-unarchive-gid-cast.yml new file mode 100644 index 00000000000..89a99d02d13 --- /dev/null +++ b/changelogs/fragments/71903-unarchive-gid-cast.yml @@ -0,0 +1,2 @@ +bugfixes: + - unarchive - ``zip`` unarchive no longer errors on RHEL/CentOS 6 and old Fedora when attempting to use a numeric gid (https://github.com/ansible/ansible/issues/71903). diff --git a/lib/ansible/modules/unarchive.py b/lib/ansible/modules/unarchive.py index f6591ed5d41..550ca3a00c4 100644 --- a/lib/ansible/modules/unarchive.py +++ b/lib/ansible/modules/unarchive.py @@ -332,8 +332,8 @@ class ZipArchive(object): tpw = pwd.getpwnam(self.file_args['owner']) except KeyError: try: - tpw = pwd.getpwuid(self.file_args['owner']) - except (TypeError, KeyError): + tpw = pwd.getpwuid(int(self.file_args['owner'])) + except (TypeError, KeyError, ValueError): tpw = pwd.getpwuid(run_uid) fut_owner = tpw.pw_name fut_uid = tpw.pw_uid @@ -351,7 +351,9 @@ class ZipArchive(object): tgr = grp.getgrnam(self.file_args['group']) except (ValueError, KeyError): try: - tgr = grp.getgrgid(self.file_args['group']) + # no need to check isdigit() explicitly here, if we fail to + # parse, the ValueError will be caught. + tgr = grp.getgrgid(int(self.file_args['group'])) except (KeyError, ValueError, OverflowError): tgr = grp.getgrgid(run_gid) fut_group = tgr.gr_name diff --git a/test/integration/targets/unarchive/tasks/test_owner_group.yml b/test/integration/targets/unarchive/tasks/test_owner_group.yml new file mode 100644 index 00000000000..95f1457e66a --- /dev/null +++ b/test/integration/targets/unarchive/tasks/test_owner_group.yml @@ -0,0 +1,162 @@ +- block: + - name: Create a group to chown to + group: + name: testgroup + register: testgroup + + - name: Create a user to chown to + user: + name: testuser + groups: + - testgroup + register: testuser + + - set_fact: + outdir: '{{remote_tmp_dir}}/test-unarchive-{{ ext | replace(".", "_") }}' + + - debug: + msg: Username test + + # username + - name: create our unarchive destinations + file: + path: '{{outdir}}' + state: directory + + - name: unarchive a file + unarchive: + src: '{{remote_tmp_dir}}/{{archive}}' + dest: '{{outdir}}' + list_files: True + remote_src: yes + owner: testuser + + - name: stat an output file + stat: + path: '{{outdir}}/{{testfile}}' + register: stat + + - name: verify that the file has the right owner + assert: + that: + - stat.stat.exists + - stat.stat.pw_name == testuser.name + - stat.stat.uid == testuser.uid + + - name: nuke destination + file: + path: '{{outdir}}' + state: absent + + - debug: + msg: uid test + + # uid + - name: create our unarchive destinations + file: + path: '{{outdir}}' + state: directory + + - name: unarchive a file + unarchive: + src: '{{remote_tmp_dir}}/{{archive}}' + dest: '{{outdir}}' + list_files: True + remote_src: yes + owner: '{{ testuser.uid }}' + + - name: stat an output file + stat: + path: '{{outdir}}/{{testfile}}' + register: stat + + - name: verify that the file has the right owner + assert: + that: + - stat.stat.exists + - stat.stat.pw_name == testuser.name + - stat.stat.uid == testuser.uid + + - name: nuke destination + file: + path: '{{outdir}}' + state: absent + + - debug: + msg: groupname test + + # groupname + - name: create our unarchive destinations + file: + path: '{{outdir}}' + state: directory + + - name: unarchive a file + unarchive: + src: '{{remote_tmp_dir}}/{{archive}}' + dest: '{{outdir}}' + list_files: True + remote_src: yes + group: testgroup + + - name: stat an output file + stat: + path: '{{outdir}}/{{testfile}}' + register: stat + + - name: verify that the file has the right owner + assert: + that: + - stat.stat.exists + - stat.stat.gr_name == testgroup.name + - stat.stat.gid == testgroup.gid + + - name: nuke destination + file: + path: '{{outdir}}' + state: absent + + - debug: + msg: gid test + + # gid + - name: create our unarchive destinations + file: + path: '{{outdir}}' + state: directory + + - name: unarchive a file + unarchive: + src: '{{remote_tmp_dir}}/{{archive}}' + dest: '{{outdir}}' + list_files: True + remote_src: yes + group: '{{ testgroup.gid }}' + + - name: stat an output file + stat: + path: '{{outdir}}/{{testfile}}' + register: stat + + - name: verify that the file has the right owner + assert: + that: + - stat.stat.exists + - stat.stat.gr_name == testgroup.name + - stat.stat.gid == testgroup.gid + + - name: nuke destination + file: + path: '{{outdir}}' + state: absent + + always: + - name: Remove testuser + user: + name: testuser + state: absent + + - name: Remove testgroup + group: + name: testgroup + state: absent diff --git a/test/integration/targets/unarchive/tasks/test_tar.yml b/test/integration/targets/unarchive/tasks/test_tar.yml index 09105c600e8..0a020410001 100644 --- a/test/integration/targets/unarchive/tasks/test_tar.yml +++ b/test/integration/targets/unarchive/tasks/test_tar.yml @@ -24,3 +24,10 @@ file: path: '{{remote_tmp_dir}}/test-unarchive-tar' state: absent + +- name: test owner/group perms + include_tasks: test_owner_group.yml + vars: + ext: tar + archive: test-unarchive.tar + testfile: foo-unarchive.txt diff --git a/test/integration/targets/unarchive/tasks/test_tar_gz.yml b/test/integration/targets/unarchive/tasks/test_tar_gz.yml index ac9e9a15499..e88f77b6a2c 100644 --- a/test/integration/targets/unarchive/tasks/test_tar_gz.yml +++ b/test/integration/targets/unarchive/tasks/test_tar_gz.yml @@ -26,3 +26,10 @@ file: path: '{{remote_tmp_dir}}/test-unarchive-tar-gz' state: absent + +- name: test owner/group perms + include_tasks: test_owner_group.yml + vars: + ext: tar.gz + archive: test-unarchive.tar.gz + testfile: foo-unarchive.txt diff --git a/test/integration/targets/unarchive/tasks/test_zip.yml b/test/integration/targets/unarchive/tasks/test_zip.yml index aae57d8ec90..cf03946fcdf 100644 --- a/test/integration/targets/unarchive/tasks/test_zip.yml +++ b/test/integration/targets/unarchive/tasks/test_zip.yml @@ -43,3 +43,15 @@ assert: that: - "unarchive03b.changed == false" + +- name: nuke zip destination + file: + path: '{{remote_tmp_dir}}/test-unarchive-zip' + state: absent + +- name: test owner/group perms + include_tasks: test_owner_group.yml + vars: + ext: zip + archive: test-unarchive.zip + testfile: foo-unarchive.txt