galaxy - preserve mode properly on artifact (#68418)

* galaxy - preserve mode properly on artifact

* Fix py2 encoding issue

* Update lib/ansible/galaxy/collection.py

Co-Authored-By: Matt Clay <matt@mystile.com>

* Use sane defaults instead of sourcing from tarfile

Co-authored-by: Matt Clay <matt@mystile.com>

changelogs/fragments/collection-install-mode.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+- ansible-galaxy collection - Preserve executable bit on build and preserve mode on install from what tar member is set to - https://github.com/ansible/ansible/issues/68415

lib/ansible/galaxy/collection.py

@@ -9,6 +9,7 @@ import json
 import operator
 import os
 import shutil
+import stat
 import sys
 import tarfile
 import tempfile
@@ -925,7 +926,8 @@ def _build_collection_tar(b_collection_path, b_tar_path, collection_manifest, fi
                 b_src_path = os.path.join(b_collection_path, to_bytes(filename, errors='surrogate_or_strict'))
 
                 def reset_stat(tarinfo):
-                    tarinfo.mode = 0o0755 if tarinfo.isdir() else 0o0644
+                    existing_is_exec = tarinfo.mode & stat.S_IXUSR
+                    tarinfo.mode = 0o0755 if existing_is_exec or tarinfo.isdir() else 0o0644
                     tarinfo.uid = tarinfo.gid = 0
                     tarinfo.uname = tarinfo.gname = ''
                     return tarinfo
@@ -1085,10 +1087,18 @@ def _extract_tar_file(tar, filename, b_dest, b_temp_path, expected_hash=None):
         if not os.path.exists(b_parent_dir):
             # Seems like Galaxy does not validate if all file entries have a corresponding dir ftype entry. This check
             # makes sure we create the parent directory even if it wasn't set in the metadata.
-            os.makedirs(b_parent_dir)
+            os.makedirs(b_parent_dir, mode=0o0755)
 
         shutil.move(to_bytes(tmpfile_obj.name, errors='surrogate_or_strict'), b_dest_filepath)
 
+        # Default to rw-r--r-- and only add execute if the tar file has execute.
+        tar_member = tar.getmember(to_native(filename, errors='surrogate_or_strict'))
+        new_mode = 0o644
+        if stat.S_IMODE(tar_member.mode) & stat.S_IXUSR:
+            new_mode |= 0o0111
+
+        os.chmod(b_dest_filepath, new_mode)
+
 
 def _get_tar_file_member(tar, filename):
     n_filename = to_native(filename, errors='surrogate_or_strict')

test/units/cli/test_galaxy.py

@@ -25,6 +25,7 @@ import json
 import os
 import pytest
 import shutil
+import stat
 import tarfile
 import tempfile
 import yaml
@@ -579,6 +580,14 @@ def collection_artifact(collection_skeleton, tmp_path_factory):
     ''' Creates a collection artifact tarball that is ready to be published and installed '''
     output_dir = to_text(tmp_path_factory.mktemp('test-ÅÑŚÌβŁÈ Output'))
 
+    # Create a file with +x in the collection so we can test the permissions
+    execute_path = os.path.join(collection_skeleton, 'runme.sh')
+    with open(execute_path, mode='wb') as fd:
+        fd.write(b"echo hi")
+
+    # S_ISUID should not be present on extraction.
+    os.chmod(execute_path, os.stat(execute_path).st_mode | stat.S_ISUID | stat.S_IEXEC)
+
     # Because we call GalaxyCLI in collection_skeleton we need to reset the singleton back to None so it uses the new
     # args, we reset the original args once it is done.
     orig_cli_args = co.GlobalCLIArgs._Singleton__instance
@@ -644,8 +653,9 @@ def test_collection_build(collection_artifact):
     with tarfile.open(tar_path, mode='r') as tar:
         tar_members = tar.getmembers()
 
-        valid_files = ['MANIFEST.json', 'FILES.json', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md']
-        assert len(tar_members) == 7
+        valid_files = ['MANIFEST.json', 'FILES.json', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md',
+                       'runme.sh']
+        assert len(tar_members) == len(valid_files)
 
         # Verify the uid and gid is 0 and the correct perms are set
         for member in tar_members:
@@ -655,7 +665,7 @@ def test_collection_build(collection_artifact):
             assert member.gname == ''
             assert member.uid == 0
             assert member.uname == ''
-            if member.isdir():
+            if member.isdir() or member.name == 'runme.sh':
                 assert member.mode == 0o0755
             else:
                 assert member.mode == 0o0644
@@ -700,19 +710,20 @@ def test_collection_build(collection_artifact):
         finally:
             files_file.close()
 
-        assert len(files['files']) == 6
+        assert len(files['files']) == 7
         assert files['format'] == 1
         assert len(files.keys()) == 2
 
-        valid_files_entries = ['.', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md']
+        valid_files_entries = ['.', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md', 'runme.sh']
         for file_entry in files['files']:
             assert file_entry['name'] in valid_files_entries
             assert file_entry['format'] == 1
 
-            if file_entry['name'] == 'plugins/README.md':
+            if file_entry['name'] in ['plugins/README.md', 'runme.sh']:
                 assert file_entry['ftype'] == 'file'
                 assert file_entry['chksum_type'] == 'sha256'
-                # Can't test the actual checksum as the html link changes based on the version.
+                # Can't test the actual checksum as the html link changes based on the version or the file contents
+                # don't matter
                 assert file_entry['chksum_sha256'] is not None
             elif file_entry['name'] == 'README.md':
                 assert file_entry['ftype'] == 'file'

test/units/galaxy/test_collection_install.py

@@ -12,6 +12,7 @@ import os
 import pytest
 import re
 import shutil
+import stat
 import tarfile
 import yaml
 
@@ -139,6 +140,12 @@ def collection_artifact(request, tmp_path_factory):
             galaxy_obj.write(to_bytes(yaml.safe_dump(existing_yaml)))
             galaxy_obj.truncate()
 
+    # Create a file with +x in the collection so we can test the permissions
+    execute_path = os.path.join(collection_path, 'runme.sh')
+    with open(execute_path, mode='wb') as fd:
+        fd.write(b"echo hi")
+    os.chmod(execute_path, os.stat(execute_path).st_mode | stat.S_IEXEC)
+
     call_galaxy_cli(['build', collection_path, '--output-path', test_dir])
 
     collection_tar = os.path.join(test_dir, '%s-%s-0.1.0.tar.gz' % (namespace, collection))
@@ -634,7 +641,12 @@ def test_install_collection(collection_artifact, monkeypatch):
 
     actual_files = os.listdir(collection_path)
     actual_files.sort()
-    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles']
+    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles',
+                            b'runme.sh']
+
+    assert stat.S_IMODE(os.stat(os.path.join(collection_path, b'plugins')).st_mode) == 0o0755
+    assert stat.S_IMODE(os.stat(os.path.join(collection_path, b'README.md')).st_mode) == 0o0644
+    assert stat.S_IMODE(os.stat(os.path.join(collection_path, b'runme.sh')).st_mode) == 0o0755
 
     assert mock_display.call_count == 1
     assert mock_display.mock_calls[0][1][0] == "Installing 'ansible_namespace.collection:0.1.0' to '%s'" \
@@ -668,7 +680,8 @@ def test_install_collection_with_download(galaxy_server, collection_artifact, mo
 
     actual_files = os.listdir(collection_path)
     actual_files.sort()
-    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles']
+    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles',
+                            b'runme.sh']
 
     assert mock_display.call_count == 1
     assert mock_display.mock_calls[0][1][0] == "Installing 'ansible_namespace.collection:0.1.0' to '%s'" \
@@ -696,7 +709,8 @@ def test_install_collections_from_tar(collection_artifact, monkeypatch):
 
     actual_files = os.listdir(collection_path)
     actual_files.sort()
-    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles']
+    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles',
+                            b'runme.sh']
 
     with open(os.path.join(collection_path, b'MANIFEST.json'), 'rb') as manifest_obj:
         actual_manifest = json.loads(to_text(manifest_obj.read()))
@@ -728,7 +742,7 @@ def test_install_collections_existing_without_force(collection_artifact, monkeyp
 
     actual_files = os.listdir(collection_path)
     actual_files.sort()
-    assert actual_files == [b'README.md', b'docs', b'galaxy.yml', b'playbooks', b'plugins', b'roles']
+    assert actual_files == [b'README.md', b'docs', b'galaxy.yml', b'playbooks', b'plugins', b'roles', b'runme.sh']
 
     # Filter out the progress cursor display calls.
     display_msgs = [m[1][0] for m in mock_display.mock_calls if 'newline' not in m[2] and len(m[1]) == 1]
@@ -759,7 +773,8 @@ def test_install_collection_with_circular_dependency(collection_artifact, monkey
 
     actual_files = os.listdir(collection_path)
     actual_files.sort()
-    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles']
+    assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles',
+                            b'runme.sh']
 
     with open(os.path.join(collection_path, b'MANIFEST.json'), 'rb') as manifest_obj:
         actual_manifest = json.loads(to_text(manifest_obj.read()))