diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index fff10e786f..114935f294 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -147,6 +147,7 @@ def do_export(self, resource_id, resource_dict, parent_dir): should_sign_package = self.code_signer.should_sign_package(resource_id) artifact_extension = "zip" if should_sign_package else None uploaded_url = upload_local_artifacts( + self.RESOURCE_TYPE, resource_id, resource_dict, self.PROPERTY_NAME, @@ -325,7 +326,7 @@ def do_export(self, resource_id, resource_dict, parent_dir): """ artifact_s3_url = upload_local_artifacts( - resource_id, resource_dict, self.PROPERTY_NAME, parent_dir, self.uploader + self.RESOURCE_TYPE, resource_id, resource_dict, self.PROPERTY_NAME, parent_dir, self.uploader ) parsed_url = S3Uploader.parse_s3_url( diff --git a/samcli/lib/package/permissions.py b/samcli/lib/package/permissions.py new file mode 100644 index 0000000000..24da0bdac5 --- /dev/null +++ b/samcli/lib/package/permissions.py @@ -0,0 +1,160 @@ +""" +Classes which will change permissions on a ZipInfo object +""" +import platform +import zipfile + + +class PermissionMapper: + """ + Base Mapper with an empty apply method. + """ + + def apply(self, zip_info: zipfile.ZipInfo): + pass + + +class WindowsFilePermissionPermissionMapper(PermissionMapper): + """ + Windows specific Permission Mapper onto a zipfile.ZipInfo object to + set explicit permissions onto `external_attr` attribute if the given object + is of type `file` + """ + + def __init__(self, permissions: int): + """ + + Parameters + ---------- + permissions: int + Base permissions. + """ + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + """ + + Parameters + ---------- + zip_info: zipfile.ZipInfo + object of zipfile.ZipInfo + + Returns + ------- + zipfile.ZipInfo + modified object with `external_attr` set to member permissions. + + """ + if platform.system().lower() == "windows" and not zip_info.is_dir(): + zip_info.external_attr = self.permissions << 16 + return zip_info + + +class WindowsDirPermissionPermissionMapper(PermissionMapper): + """ + Windows specific Permission Mapper onto a zipfile.ZipInfo object to + set explicit permissions onto `external_attr` attribute if the given object + is of type `directory` + """ + + def __init__(self, permissions: int): + """ + + Parameters + ---------- + permissions: int + Base permissions. + """ + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + """ + + Parameters + ---------- + zip_info: zipfile.ZipInfo + object of zipfile.ZipInfo + + Returns + ------- + zipfile.ZipInfo + modified object with `external_attr` set to member permissions. + + """ + if platform.system().lower() == "windows" and zip_info.is_dir(): + zip_info.external_attr = self.permissions << 16 + return zip_info + + +class AdditiveDirPermissionPermissionMapper(PermissionMapper): + """ + Additive Permission Mapper onto a zipfile.ZipInfo object to + set additive permissions onto `external_attr` attribute if the given object + is of type `directory` + """ + + def __init__(self, permissions: int): + """ + + Parameters + ---------- + permissions: int + Base permissions. + """ + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + """ + + Parameters + ---------- + zip_info: zipfile.ZipInfo + object of zipfile.ZipInfo + + Returns + ------- + zipfile.ZipInfo + modified object with `external_attr` added with member permissions + if conditions are satisfied. + + """ + if zip_info.is_dir(): + zip_info.external_attr |= self.permissions << 16 + return zip_info + + +class AdditiveFilePermissionPermissionMapper(PermissionMapper): + """ + Additive Permission Mapper onto a zipfile.ZipInfo object to + set additive permissions onto `external_attr` attribute if the given object + is of type `file` + """ + + def __init__(self, permissions: int): + """ + + Parameters + ---------- + permissions: int + Base permissions. + """ + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + """ + + Parameters + ---------- + zip_info: zipfile.ZipInfo + object of zipfile.ZipInfo + + Returns + ------- + zipfile.ZipInfo + modified object with `external_attr` added with member permissions + if conditions are satisfied. + + """ + if not zip_info.is_dir(): + zip_info.external_attr |= self.permissions << 16 + return zip_info diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py index 69017ea891..e5116f1db8 100644 --- a/samcli/lib/package/utils.py +++ b/samcli/lib/package/utils.py @@ -1,23 +1,32 @@ """ Utilities involved in Packaging. """ +import functools import logging import os -import platform import re import shutil import tempfile +import time import zipfile import contextlib from contextlib import contextmanager -from typing import Dict, Optional, cast +from typing import Dict, Optional, Callable, cast, List import jmespath from samcli.commands.package.exceptions import ImageNotFoundError, InvalidLocalPathError from samcli.lib.package.ecr_utils import is_ecr_url +from samcli.lib.package.permissions import ( + WindowsFilePermissionPermissionMapper, + WindowsDirPermissionPermissionMapper, + AdditiveFilePermissionPermissionMapper, + AdditiveDirPermissionPermissionMapper, + PermissionMapper, +) from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.utils.hash import dir_checksum +from samcli.lib.utils.resources import LAMBDA_LOCAL_RESOURCES LOG = logging.getLogger(__name__) @@ -119,6 +128,7 @@ def upload_local_image_artifacts(resource_id, resource_dict, property_name, pare def upload_local_artifacts( + resource_type: str, resource_id: str, resource_dict: Dict, property_name: str, @@ -138,6 +148,7 @@ def upload_local_artifacts( If path is already a path to S3 object, this method does nothing. + :param resource_type: Type of the CloudFormation resource :param resource_id: Id of the CloudFormation resource :param resource_dict: Dictionary containing resource definition :param property_name: Property name of CloudFormation resource where this @@ -166,9 +177,14 @@ def upload_local_artifacts( local_path = make_abs_path(parent_dir, local_path) - # Or, pointing to a folder. Zip the folder and upload + # Or, pointing to a folder. Zip the folder and upload (zip_method is changed based on resource type) if is_local_folder(local_path): - return zip_and_upload(local_path, uploader, extension) + return zip_and_upload( + local_path, + uploader, + extension, + zip_method=make_zip_with_lambda_permissions if resource_type in LAMBDA_LOCAL_RESOURCES else make_zip, + ) # Path could be pointing to a file. Upload the file if is_local_file(local_path): @@ -184,13 +200,13 @@ def resource_not_packageable(resource_dict): return False -def zip_and_upload(local_path: str, uploader: S3Uploader, extension: Optional[str]) -> str: - with zip_folder(local_path) as (zip_file, md5_hash): +def zip_and_upload(local_path: str, uploader: S3Uploader, extension: Optional[str], zip_method: Callable) -> str: + with zip_folder(local_path, zip_method=zip_method) as (zip_file, md5_hash): return uploader.upload_with_dedup(zip_file, precomputed_md5=md5_hash, extension=extension) @contextmanager -def zip_folder(folder_path): +def zip_folder(folder_path, zip_method): """ Zip the entire folder and return a file to the zip. Use this inside a "with" statement to cleanup the zipfile after it is used. @@ -199,6 +215,8 @@ def zip_folder(folder_path): ---------- folder_path : str The path of the folder to zip + zip_method : Callable + Callable function that takes in a file name and source_path and zips accordingly. Yields ------ @@ -210,7 +228,7 @@ def zip_folder(folder_path): md5hash = dir_checksum(folder_path, followlinks=True) filename = os.path.join(tempfile.gettempdir(), "data-" + md5hash) - zipfile_name = make_zip(filename, folder_path) + zipfile_name = zip_method(filename, folder_path) try: yield zipfile_name, md5hash finally: @@ -218,7 +236,7 @@ def zip_folder(folder_path): os.remove(zipfile_name) -def make_zip(file_name, source_root): +def make_zip_with_permissions(file_name, source_root, permission_mappers: List[PermissionMapper]): """ Create a zip file from the source directory @@ -228,11 +246,15 @@ def make_zip(file_name, source_root): The basename of the zip file, without .zip source_root : str The path to the source directory + permission_mappers : list + permission objects that need to match an interface such that they have an apply method + which takes in the external attributes of a zipfile.Zipinfo object Returns ------- str The name of the zip file, including .zip extension """ + permission_mappers = permission_mappers or [] zipfile_name = "{0}.zip".format(file_name) source_root = os.path.abspath(source_root) compression_type = zipfile.ZIP_DEFLATED @@ -242,28 +264,54 @@ def make_zip(file_name, source_root): for filename in files: full_path = os.path.join(root, filename) relative_path = os.path.relpath(full_path, source_root) - if platform.system().lower() == "windows": - with open(full_path, "rb") as data: - file_bytes = data.read() - info = zipfile.ZipInfo(relative_path) - # Clear external attr set for Windows - info.external_attr = 0 - # Set external attr with Unix 0755 permission - # Originally set to 0005 in the discussion below - # https://github.com/aws/aws-sam-cli/pull/2193#discussion_r513110608 - # Changed to 0755 due to a regression in https://github.com/aws/aws-sam-cli/issues/2344 - # Mimicking Unix permission bits and recommanded permission bits - # in the Lambda Trouble Shooting Docs - info.external_attr = 0o100755 << 16 + with open(full_path, "rb") as data: + file_bytes = data.read() + info = zipfile.ZipInfo(relative_path) + # Context: Nov 2020 + # Set external attr with Unix 0755 permission + # Originally set to 0005 in the discussion below + # https://github.com/aws/aws-sam-cli/pull/2193#discussion_r513110608 + # Changed to 0755 due to a regression in https://github.com/aws/aws-sam-cli/issues/2344 + # Final PR: https://github.com/aws/aws-sam-cli/pull/2356/files + if permission_mappers: # Set host OS to Unix info.create_system = 3 + # Set current permission of the file/dir to ZipInfo's external_attr + info.external_attr = os.stat(full_path).st_mode << 16 + for permission_mapper in permission_mappers: + info = permission_mapper.apply(info) + # Set current time to be the last time the zip content was modified. + info.date_time = time.localtime()[0:6] zf.writestr(info, file_bytes, compress_type=compression_type) - else: - zf.write(full_path, relative_path) + else: + zf.write(full_path, relative_path) return zipfile_name +make_zip = functools.partial( + make_zip_with_permissions, + permission_mappers=[ + WindowsFilePermissionPermissionMapper(permissions=0o100755), + WindowsDirPermissionPermissionMapper(permissions=0o100755), + ], +) +# Context: Jan 2023 +# NOTE(sriram-mv): Modify permissions regardless of the Operating system +# to add 111 for directories and 444 for files in addition to existing permissions. +# No overriding explicit permissions are set. +# Extended Attributes are preserved. +make_zip_with_lambda_permissions = functools.partial( + make_zip_with_permissions, + permission_mappers=[ + WindowsFilePermissionPermissionMapper(permissions=0o100755), + WindowsDirPermissionPermissionMapper(permissions=0o100755), + AdditiveFilePermissionPermissionMapper(permissions=0o100444), + AdditiveDirPermissionPermissionMapper(permissions=0o100111), + ], +) + + def copy_to_temp_dir(filepath): tmp_dir = tempfile.mkdtemp() dst = os.path.join(tmp_dir, os.path.basename(filepath)) diff --git a/samcli/lib/sync/flows/auto_dependency_layer_sync_flow.py b/samcli/lib/sync/flows/auto_dependency_layer_sync_flow.py index 5151c6e9fe..2154864623 100644 --- a/samcli/lib/sync/flows/auto_dependency_layer_sync_flow.py +++ b/samcli/lib/sync/flows/auto_dependency_layer_sync_flow.py @@ -11,7 +11,7 @@ from samcli.lib.bootstrap.nested_stack.nested_stack_builder import NestedStackBuilder from samcli.lib.bootstrap.nested_stack.nested_stack_manager import NestedStackManager from samcli.lib.build.build_graph import BuildGraph -from samcli.lib.package.utils import make_zip +from samcli.lib.package.utils import make_zip_with_lambda_permissions from samcli.lib.providers.provider import Function, Stack from samcli.lib.providers.sam_function_provider import SamFunctionProvider from samcli.lib.sync.exceptions import ( @@ -88,7 +88,7 @@ def gather_resources(self) -> None: self._get_compatible_runtimes()[0], ) zip_file_path = os.path.join(tempfile.gettempdir(), "data-" + uuid.uuid4().hex) - self._zip_file = make_zip(zip_file_path, self._artifact_folder) + self._zip_file = make_zip_with_lambda_permissions(zip_file_path, self._artifact_folder) self._local_sha = file_checksum(cast(str, self._zip_file), hashlib.sha256()) def _get_dependent_functions(self) -> List[Function]: diff --git a/samcli/lib/sync/flows/layer_sync_flow.py b/samcli/lib/sync/flows/layer_sync_flow.py index ee9bcfa6ce..03126ec02b 100644 --- a/samcli/lib/sync/flows/layer_sync_flow.py +++ b/samcli/lib/sync/flows/layer_sync_flow.py @@ -12,7 +12,7 @@ from contextlib import ExitStack from samcli.lib.build.app_builder import ApplicationBuilder -from samcli.lib.package.utils import make_zip +from samcli.lib.package.utils import make_zip_with_lambda_permissions from samcli.lib.providers.provider import ResourceIdentifier, Stack, get_resource_by_id, Function, LayerVersion from samcli.lib.providers.sam_function_provider import SamFunctionProvider from samcli.lib.sync.exceptions import MissingPhysicalResourceError, NoLayerVersionsFoundError @@ -248,7 +248,7 @@ def gather_resources(self) -> None: self._artifact_folder = builder.build().artifacts.get(self._layer_identifier) zip_file_path = os.path.join(tempfile.gettempdir(), f"data-{uuid.uuid4().hex}") - self._zip_file = make_zip(zip_file_path, self._artifact_folder) + self._zip_file = make_zip_with_lambda_permissions(zip_file_path, self._artifact_folder) LOG.debug("%sCreated artifact ZIP file: %s", self.log_prefix, self._zip_file) self._local_sha = file_checksum(cast(str, self._zip_file), hashlib.sha256()) @@ -278,7 +278,7 @@ class LayerSyncFlowSkipBuildDirectory(LayerSyncFlow): def gather_resources(self) -> None: zip_file_path = os.path.join(tempfile.gettempdir(), f"data-{uuid.uuid4().hex}") - self._zip_file = make_zip(zip_file_path, self._layer.codeuri) + self._zip_file = make_zip_with_lambda_permissions(zip_file_path, self._layer.codeuri) LOG.debug("%sCreated artifact ZIP file: %s", self.log_prefix, self._zip_file) self._local_sha = file_checksum(cast(str, self._zip_file), hashlib.sha256()) diff --git a/samcli/lib/sync/flows/zip_function_sync_flow.py b/samcli/lib/sync/flows/zip_function_sync_flow.py index e85aa78aa7..6d5d0c4767 100644 --- a/samcli/lib/sync/flows/zip_function_sync_flow.py +++ b/samcli/lib/sync/flows/zip_function_sync_flow.py @@ -16,7 +16,7 @@ from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.utils.colors import Colored from samcli.lib.utils.hash import file_checksum -from samcli.lib.package.utils import make_zip +from samcli.lib.package.utils import make_zip_with_lambda_permissions from samcli.lib.build.app_builder import ApplicationBuilder from samcli.lib.sync.sync_flow import ResourceAPICall, ApiCallTypes @@ -104,7 +104,7 @@ def gather_resources(self) -> None: self._artifact_folder = build_result.artifacts.get(self._function_identifier) zip_file_path = os.path.join(tempfile.gettempdir(), "data-" + uuid.uuid4().hex) - self._zip_file = make_zip(zip_file_path, self._artifact_folder) + self._zip_file = make_zip_with_lambda_permissions(zip_file_path, self._artifact_folder) LOG.debug("%sCreated artifact ZIP file: %s", self.log_prefix, self._zip_file) self._local_sha = file_checksum(cast(str, self._zip_file), hashlib.sha256()) diff --git a/samcli/lib/utils/resources.py b/samcli/lib/utils/resources.py index aed3adc244..731e0829c4 100644 --- a/samcli/lib/utils/resources.py +++ b/samcli/lib/utils/resources.py @@ -96,6 +96,13 @@ AWS_CLOUDFORMATION_STACK: "TemplateURL", } +LAMBDA_LOCAL_RESOURCES = [ + AWS_LAMBDA_FUNCTION, + AWS_LAMBDA_LAYERVERSION, + AWS_SERVERLESS_FUNCTION, + AWS_SERVERLESS_LAYERVERSION, +] + AWS_LAMBDA_FUNCTION_URL = "AWS::Lambda::Url" diff --git a/tests/integration/sync/test_sync_adl.py b/tests/integration/sync/test_sync_adl.py index 5910fd5d70..ba7ac3d8e5 100644 --- a/tests/integration/sync/test_sync_adl.py +++ b/tests/integration/sync/test_sync_adl.py @@ -133,7 +133,7 @@ def test_sync_watch_code(self): ) read_until_string( self.watch_process, - "\x1b[32mFinished syncing Function Layer Reference Sync HelloWorldFunction.\x1b[0m\n", + "\x1b[32mFinished syncing Layer HelloWorldFunction", timeout=60, ) lambda_response = json.loads(self._get_lambda_response(lambda_functions[0])) @@ -147,7 +147,7 @@ def test_sync_watch_code(self): ) read_until_string( self.watch_process, - "\x1b[32mFinished syncing Function Layer Reference Sync HelloWorldFunction.\x1b[0m\n", + "\x1b[32mFinished syncing Layer HelloWorldFunction", timeout=60, ) self._confirm_lambda_error(lambda_functions[0]) diff --git a/tests/integration/sync/test_sync_watch.py b/tests/integration/sync/test_sync_watch.py index f0f7926cbf..901b1d74bd 100644 --- a/tests/integration/sync/test_sync_watch.py +++ b/tests/integration/sync/test_sync_watch.py @@ -119,6 +119,7 @@ def _setup_verify_infra(self): s3_prefix=self.s3_prefix, kms_key_id=self.kms_key, tags="integ=true clarity=yes foo_bar=baz", + debug=True, ) self.watch_process = start_persistent_process(sync_command_list, cwd=self.test_dir) read_until_string(self.watch_process, "Enter Y to proceed with the command, or enter N to cancel:\n") diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 4e9d6c977d..b64b447243 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1,4 +1,6 @@ +import functools import json +import platform import tempfile import os import string @@ -11,10 +13,17 @@ from unittest.mock import patch, Mock, MagicMock from samcli.commands.package.exceptions import ExportFailedError +from samcli.lib.package.permissions import ( + WindowsFilePermissionPermissionMapper, + WindowsDirPermissionPermissionMapper, + AdditiveFilePermissionPermissionMapper, + AdditiveDirPermissionPermissionMapper, +) from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.package.uploaders import Destination -from samcli.lib.package.utils import zip_folder, make_zip +from samcli.lib.package.utils import zip_folder, make_zip, make_zip_with_lambda_permissions, make_zip_with_permissions from samcli.lib.utils.packagetype import ZIP, IMAGE +from samcli.lib.utils.resources import LAMBDA_LOCAL_RESOURCES, RESOURCES_WITH_LOCAL_PATHS from tests.testing_utils import FileCreator from samcli.commands.package import exceptions from samcli.lib.package.artifact_exporter import ( @@ -165,11 +174,22 @@ def _helper_verify_export_resources( LambdaLayerVersionResource, ): upload_local_artifacts_mock.assert_called_once_with( - resource_id, resource_dict, test_class.PROPERTY_NAME, parent_dir, s3_uploader_mock + test_class.RESOURCE_TYPE, + resource_id, + resource_dict, + test_class.PROPERTY_NAME, + parent_dir, + s3_uploader_mock, ) else: upload_local_artifacts_mock.assert_called_once_with( - resource_id, resource_dict, test_class.PROPERTY_NAME, parent_dir, s3_uploader_mock, None + test_class.RESOURCE_TYPE, + resource_id, + resource_dict, + test_class.PROPERTY_NAME, + parent_dir, + s3_uploader_mock, + None, ) code_signer_mock.sign_package.assert_not_called() if "." in test_class.PROPERTY_NAME: @@ -273,6 +293,7 @@ def test_upload_local_artifacts_local_file(self, zip_and_upload_mock): # Verifies that we package local artifacts appropriately property_name = "property" resource_id = "resource_id" + resource_type = "resource_type" expected_s3_url = "s3://foo/bar?versionId=baz" self.s3_uploader_mock.upload_with_dedup.return_value = expected_s3_url @@ -284,7 +305,7 @@ def test_upload_local_artifacts_local_file(self, zip_and_upload_mock): resource_dict = {property_name: artifact_path} result = upload_local_artifacts( - resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock + resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock ) self.assertEqual(result, expected_s3_url) @@ -301,6 +322,7 @@ def test_upload_local_artifacts_local_file_abs_path(self, zip_and_upload_mock): # Verifies that we package local artifacts appropriately property_name = "property" resource_id = "resource_id" + resource_type = "resource_type" expected_s3_url = "s3://foo/bar?versionId=baz" self.s3_uploader_mock.upload_with_dedup.return_value = expected_s3_url @@ -311,7 +333,7 @@ def test_upload_local_artifacts_local_file_abs_path(self, zip_and_upload_mock): resource_dict = {property_name: artifact_path} result = upload_local_artifacts( - resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock + resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock ) self.assertEqual(result, expected_s3_url) @@ -322,6 +344,7 @@ def test_upload_local_artifacts_local_file_abs_path(self, zip_and_upload_mock): def test_upload_local_artifacts_local_folder(self, zip_and_upload_mock): property_name = "property" resource_id = "resource_id" + resource_type = "resource_type" expected_s3_url = "s3://foo/bar?versionId=baz" zip_and_upload_mock.return_value = expected_s3_url @@ -332,17 +355,85 @@ def test_upload_local_artifacts_local_folder(self, zip_and_upload_mock): parent_dir = tempfile.gettempdir() resource_dict = {property_name: artifact_path} - result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, Mock()) + result = upload_local_artifacts( + resource_type, resource_id, resource_dict, property_name, parent_dir, Mock() + ) self.assertEqual(result, expected_s3_url) absolute_artifact_path = make_abs_path(parent_dir, artifact_path) - zip_and_upload_mock.assert_called_once_with(absolute_artifact_path, mock.ANY, None) + zip_and_upload_mock.assert_called_once_with(absolute_artifact_path, mock.ANY, None, zip_method=make_zip) + + @patch("samcli.lib.package.utils.zip_and_upload") + def test_upload_local_artifacts_local_folder_lambda_resources(self, zip_and_upload_mock): + for resource_type in LAMBDA_LOCAL_RESOURCES: + property_name = "property" + resource_id = "resource_id" + expected_s3_url = "s3://foo/bar?versionId=baz" + + zip_and_upload_mock.return_value = expected_s3_url + # Artifact path is a Directory + with self.make_temp_dir() as artifact_path: + # Artifact is a file in the temporary directory + parent_dir = tempfile.gettempdir() + resource_dict = {property_name: artifact_path} + + result = upload_local_artifacts( + resource_type, resource_id, resource_dict, property_name, parent_dir, Mock() + ) + self.assertEqual(result, expected_s3_url) + + absolute_artifact_path = make_abs_path(parent_dir, artifact_path) + # zip_method will NOT be the generalized zip_method `make_zip` + + with self.assertRaises(AssertionError): + zip_and_upload_mock.assert_called_once_with( + absolute_artifact_path, mock.ANY, None, zip_method=make_zip + ) + + # zip_method will be lambda specific. + zip_and_upload_mock.assert_called_once_with( + absolute_artifact_path, mock.ANY, None, zip_method=make_zip_with_lambda_permissions + ) + zip_and_upload_mock.reset_mock() + + @patch("samcli.lib.package.utils.zip_and_upload") + def test_upload_local_artifacts_local_folder_non_lambda_resources(self, zip_and_upload_mock): + non_lambda_resources = RESOURCES_WITH_LOCAL_PATHS.keys() - LAMBDA_LOCAL_RESOURCES + for resource_type in non_lambda_resources: + property_name = "property" + resource_id = "resource_id" + expected_s3_url = "s3://foo/bar?versionId=baz" + + zip_and_upload_mock.return_value = expected_s3_url + # Artifact path is a Directory + with self.make_temp_dir() as artifact_path: + # Artifact is a file in the temporary directory + parent_dir = tempfile.gettempdir() + resource_dict = {property_name: artifact_path} + + result = upload_local_artifacts( + resource_type, resource_id, resource_dict, property_name, parent_dir, Mock() + ) + self.assertEqual(result, expected_s3_url) + + absolute_artifact_path = make_abs_path(parent_dir, artifact_path) + + # zip_method will NOT be the specialized zip_method `make_zip_with_lambda_permissions` + with self.assertRaises(AssertionError): + zip_and_upload_mock.assert_called_once_with( + absolute_artifact_path, mock.ANY, None, zip_method=make_zip_with_lambda_permissions + ) + + # zip_method will be the generalized zip_method `make_zip` + zip_and_upload_mock.assert_called_once_with(absolute_artifact_path, mock.ANY, None, zip_method=make_zip) + zip_and_upload_mock.reset_mock() @patch("samcli.lib.package.utils.zip_and_upload") def test_upload_local_artifacts_no_path(self, zip_and_upload_mock): property_name = "property" resource_id = "resource_id" + resource_type = "resource_type" expected_s3_url = "s3://foo/bar?versionId=baz" zip_and_upload_mock.return_value = expected_s3_url @@ -351,23 +442,28 @@ def test_upload_local_artifacts_no_path(self, zip_and_upload_mock): resource_dict = {} parent_dir = tempfile.gettempdir() - result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock) + result = upload_local_artifacts( + resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock + ) self.assertEqual(result, expected_s3_url) - zip_and_upload_mock.assert_called_once_with(parent_dir, mock.ANY, None) + zip_and_upload_mock.assert_called_once_with(parent_dir, mock.ANY, None, zip_method=make_zip) self.s3_uploader_mock.upload_with_dedup.assert_not_called() @patch("samcli.lib.package.utils.zip_and_upload") def test_upload_local_artifacts_s3_url(self, zip_and_upload_mock): property_name = "property" resource_id = "resource_id" + resource_type = "resource_type" object_s3_url = "s3://foo/bar?versionId=baz" # If URL is already S3 URL, this will be returned without zip/upload resource_dict = {property_name: object_s3_url} parent_dir = tempfile.gettempdir() - result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock) + result = upload_local_artifacts( + resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock + ) self.assertEqual(result, object_s3_url) zip_and_upload_mock.assert_not_called() @@ -377,17 +473,22 @@ def test_upload_local_artifacts_s3_url(self, zip_and_upload_mock): def test_upload_local_artifacts_invalid_value(self, zip_and_upload_mock): property_name = "property" resource_id = "resource_id" + resource_type = "resource_type" parent_dir = tempfile.gettempdir() with self.assertRaises(exceptions.InvalidLocalPathError): non_existent_file = "some_random_filename" resource_dict = {property_name: non_existent_file} - upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock) + upload_local_artifacts( + resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock + ) with self.assertRaises(exceptions.InvalidLocalPathError): non_existent_file = ["invalid datatype"] resource_dict = {property_name: non_existent_file} - upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock) + upload_local_artifacts( + resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock + ) zip_and_upload_mock.assert_not_called() self.s3_uploader_mock.upload_with_dedup.assert_not_called() @@ -398,7 +499,7 @@ def test_zip_folder(self, make_zip_mock): make_zip_mock.return_value = zip_file_name with self.make_temp_dir() as dirname: - with zip_folder(dirname) as actual_zip_file_name: + with zip_folder(dirname, zip_method=make_zip_mock) as actual_zip_file_name: self.assertEqual(actual_zip_file_name, (zip_file_name, mock.ANY)) make_zip_mock.assert_called_once_with(mock.ANY, dirname) @@ -423,7 +524,13 @@ class MockResource(ResourceZip): resource.export(resource_id, resource_dict, parent_dir) upload_local_artifacts_mock.assert_called_once_with( - resource_id, resource_dict, resource.PROPERTY_NAME, parent_dir, self.s3_uploader_mock, None + resource.RESOURCE_TYPE, + resource_id, + resource_dict, + resource.PROPERTY_NAME, + parent_dir, + self.s3_uploader_mock, + None, ) self.assertEqual(resource_dict[resource.PROPERTY_NAME], s3_url) @@ -580,7 +687,7 @@ class MockResource(ResourceZip): resource.export(resource_id, resource_dict, parent_dir) - zip_and_upload_mock.assert_called_once_with(tmp_dir, mock.ANY, None) + zip_and_upload_mock.assert_called_once_with(tmp_dir, mock.ANY, None, zip_method=make_zip) rmtree_mock.assert_called_once_with(tmp_dir) is_zipfile_mock.assert_called_once_with(original_path) self.code_signer_mock.should_sign_package.assert_called_once_with(resource_id) @@ -698,7 +805,13 @@ class MockResource(ResourceZip): resource_dict = {} resource.export(resource_id, resource_dict, parent_dir) upload_local_artifacts_mock.assert_called_once_with( - resource_id, resource_dict, resource.PROPERTY_NAME, parent_dir, self.s3_uploader_mock, None + resource.RESOURCE_TYPE, + resource_id, + resource_dict, + resource.PROPERTY_NAME, + parent_dir, + self.s3_uploader_mock, + None, ) self.code_signer_mock.should_sign_package.assert_called_once_with(resource_id) self.code_signer_mock.sign_package.assert_not_called() @@ -793,7 +906,12 @@ class MockResource(ResourceWithS3UrlDict): resource.export(resource_id, resource_dict, parent_dir) upload_local_artifacts_mock.assert_called_once_with( - resource_id, resource_dict, resource.PROPERTY_NAME, parent_dir, self.s3_uploader_mock + resource.RESOURCE_TYPE, + resource_id, + resource_dict, + resource.PROPERTY_NAME, + parent_dir, + self.s3_uploader_mock, ) self.assertEqual( @@ -1574,7 +1692,7 @@ def test_template_export_path_be_folder(self): Template(template_path, os.path.relpath(dirname), self.uploaders_mock, self.code_signer_mock) - def test_make_zip(self): + def test_make_zip_keep_permissions_as_is(self): test_file_creator = FileCreator() test_file_creator.append_file( "index.js", "exports handler = (event, context, callback) => {callback(null, event);}" @@ -1582,6 +1700,9 @@ def test_make_zip(self): dirname = test_file_creator.rootdir + file_permissions = os.stat(test_file_creator.full_path("index.js")).st_mode + dir_permissions = os.stat(test_file_creator.rootdir).st_mode + expected_files = {"index.js"} random_name = "".join(random.choice(string.ascii_letters) for _ in range(10)) @@ -1594,8 +1715,15 @@ def test_make_zip(self): test_zip_file = zipfile.ZipFile(zipfile_name, "r") with closing(test_zip_file) as zf: files_in_zip = set() + external_attr_mask = 65535 << 16 for info in zf.infolist(): files_in_zip.add(info.filename) + permission_bits = (info.external_attr & external_attr_mask) >> 16 + if platform.system().lower() != "windows": + if info.is_dir(): + self.assertEqual(permission_bits, dir_permissions) + else: + self.assertEqual(permission_bits, file_permissions) self.assertEqual(files_in_zip, expected_files) @@ -1607,6 +1735,16 @@ def test_make_zip(self): @patch("platform.system") def test_make_zip_windows(self, mock_system): mock_system.return_value = "Windows" + # Redefining `make_zip` as is in local scope so that arguments passed to functools partial are re-loaded. + windows_make_zip = functools.partial( + make_zip_with_permissions, + permission_mappers=[ + WindowsFilePermissionPermissionMapper(permissions=0o100755), + WindowsDirPermissionPermissionMapper(permissions=0o100755), + AdditiveFilePermissionPermissionMapper(permissions=0o100444), + AdditiveDirPermissionPermissionMapper(permissions=0o100111), + ], + ) test_file_creator = FileCreator() test_file_creator.append_file( @@ -1622,7 +1760,7 @@ def test_make_zip_windows(self, mock_system): zipfile_name = None try: - zipfile_name = make_zip(outfile, dirname) + zipfile_name = windows_make_zip(outfile, dirname) test_zip_file = zipfile.ZipFile(zipfile_name, "r") with closing(test_zip_file) as zf: @@ -1640,6 +1778,50 @@ def test_make_zip_windows(self, mock_system): os.remove(zipfile_name) test_file_creator.remove_all() + def test_make_zip_lambda_resources(self): + + test_file_creator = FileCreator() + test_file_creator.append_file( + "index.js", "exports handler = (event, context, callback) => {callback(null, event);}" + ) + + dirname = test_file_creator.rootdir + file_permissions = os.stat(test_file_creator.full_path("index.js")).st_mode + dir_permissions = os.stat(test_file_creator.rootdir).st_mode + + expected_files = {"index.js"} + + random_name = "".join(random.choice(string.ascii_letters) for _ in range(10)) + outfile = os.path.join(tempfile.gettempdir(), random_name) + + zipfile_name = None + try: + zipfile_name = make_zip_with_lambda_permissions(outfile, dirname) + + test_zip_file = zipfile.ZipFile(zipfile_name, "r") + with closing(test_zip_file) as zf: + files_in_zip = set() + external_attr_mask = 65535 << 16 + for info in zf.infolist(): + files_in_zip.add(info.filename) + permission_bits = (info.external_attr & external_attr_mask) >> 16 + if not platform.system().lower() == "windows": + if info.is_dir(): + permission_difference = permission_bits ^ dir_permissions + self.assertTrue(permission_difference <= 0o100111) + else: + permission_difference = permission_bits ^ file_permissions + self.assertTrue(permission_difference <= 0o100444) + else: + self.assertEqual(permission_bits, 0o100755) + + self.assertEqual(files_in_zip, expected_files) + + finally: + if zipfile_name: + os.remove(zipfile_name) + test_file_creator.remove_all() + @patch("shutil.copyfile") @patch("tempfile.mkdtemp") def test_copy_to_temp_dir(self, mkdtemp_mock, copyfile_mock): diff --git a/tests/unit/lib/package/test_permissions.py b/tests/unit/lib/package/test_permissions.py new file mode 100644 index 0000000000..ab8c26b13a --- /dev/null +++ b/tests/unit/lib/package/test_permissions.py @@ -0,0 +1,71 @@ +import zipfile + +from unittest import TestCase +from unittest.mock import patch + +from parameterized import parameterized + +from samcli.lib.package.permissions import ( + WindowsFilePermissionPermissionMapper, + WindowsDirPermissionPermissionMapper, + AdditiveFilePermissionPermissionMapper, + AdditiveDirPermissionPermissionMapper, +) + + +class TestPermissions(TestCase): + @patch("platform.system") + def test_file_permission_mapper_linux(self, mock_system): + mock_system.return_value = "Linux" + # permissions set are ignored. + mapper = WindowsFilePermissionPermissionMapper(permissions=0o100777) + zi = zipfile.ZipInfo() + self.assertEqual(mapper.apply(zi), zi) + # return as-is + self.assertNotEqual(zi.external_attr, mapper.permissions << 16) + + @patch("platform.system") + def test_dir_permission_mapper_linux(self, mock_system): + mock_system.return_value = "Linux" + # permissions set are ignored. + mapper = WindowsDirPermissionPermissionMapper(permissions=0o100777) + zi = zipfile.ZipInfo(filename="dir/") + self.assertEqual(mapper.apply(zi), zi) + # return as-is + self.assertNotEqual(zi.external_attr, mapper.permissions << 16) + + @patch("platform.system") + def test_file_permission_mapper_windows(self, mock_system): + mock_system.return_value = "Windows" + mapper = WindowsFilePermissionPermissionMapper(permissions=0o100644) + zi = zipfile.ZipInfo() + self.assertEqual(mapper.apply(zi).external_attr, mapper.permissions << 16) + + @patch("platform.system") + def test_dir_permission_mapper_windows(self, mock_system): + mock_system.return_value = "Windows" + mapper = WindowsDirPermissionPermissionMapper(permissions=0o100755) + zi = zipfile.ZipInfo(filename="dir/") + self.assertEqual(mapper.apply(zi).external_attr, mapper.permissions << 16) + + @parameterized.expand( + [ + (AdditiveFilePermissionPermissionMapper, "file", 0o100000, 0o100444, 0o100444), + (AdditiveFilePermissionPermissionMapper, "file", 0o100111, 0o100444, 0o100555), + (AdditiveFilePermissionPermissionMapper, "file", 0o100444, 0o100444, 0o100444), + (AdditiveFilePermissionPermissionMapper, "file", 0o100644, 0o100444, 0o100644), + (AdditiveFilePermissionPermissionMapper, "file", 0o100777, 0o100444, 0o100777), + (AdditiveDirPermissionPermissionMapper, "dir/", 0o100000, 0o100111, 0o100111), + (AdditiveDirPermissionPermissionMapper, "dir/", 0o100111, 0o100444, 0o100555), + (AdditiveDirPermissionPermissionMapper, "dir/", 0o100111, 0o100111, 0o100111), + (AdditiveDirPermissionPermissionMapper, "dir/", 0o100644, 0o100111, 0o100755), + (AdditiveDirPermissionPermissionMapper, "dir/", 0o100777, 0o100111, 0o100777), + ] + ) + def test_additive_permissions( + self, _mapper, filename, current_permissions, additive_permissions, resultant_permissions + ): + mapper = _mapper(permissions=additive_permissions) + zi = zipfile.ZipInfo(filename=filename) + zi.external_attr = current_permissions << 16 + self.assertEqual(mapper.apply(zi).external_attr, resultant_permissions << 16) diff --git a/tests/unit/lib/sync/flows/test_auto_dependency_layer_sync_flow.py b/tests/unit/lib/sync/flows/test_auto_dependency_layer_sync_flow.py index 6e663327b4..034e9b389c 100644 --- a/tests/unit/lib/sync/flows/test_auto_dependency_layer_sync_flow.py +++ b/tests/unit/lib/sync/flows/test_auto_dependency_layer_sync_flow.py @@ -79,7 +79,7 @@ def test_gather_resources_fail_when_no_runtime_defined_for_function(self, patche @patch("samcli.lib.sync.flows.auto_dependency_layer_sync_flow.uuid") @patch("samcli.lib.sync.flows.auto_dependency_layer_sync_flow.file_checksum") - @patch("samcli.lib.sync.flows.auto_dependency_layer_sync_flow.make_zip") + @patch("samcli.lib.sync.flows.auto_dependency_layer_sync_flow.make_zip_with_lambda_permissions") @patch("samcli.lib.sync.flows.auto_dependency_layer_sync_flow.tempfile") @patch("samcli.lib.sync.flows.auto_dependency_layer_sync_flow.NestedStackManager") def test_gather_resources( diff --git a/tests/unit/lib/sync/flows/test_layer_sync_flow.py b/tests/unit/lib/sync/flows/test_layer_sync_flow.py index 9d8ee03a8c..19214a4328 100644 --- a/tests/unit/lib/sync/flows/test_layer_sync_flow.py +++ b/tests/unit/lib/sync/flows/test_layer_sync_flow.py @@ -64,7 +64,7 @@ def test_setup_with_unknown_layer(self): @patch("samcli.lib.sync.flows.layer_sync_flow.ApplicationBuilder") @patch("samcli.lib.sync.flows.layer_sync_flow.tempfile") - @patch("samcli.lib.sync.flows.layer_sync_flow.make_zip") + @patch("samcli.lib.sync.flows.layer_sync_flow.make_zip_with_lambda_permissions") @patch("samcli.lib.sync.flows.layer_sync_flow.file_checksum") @patch("samcli.lib.sync.flows.layer_sync_flow.os") @patch("samcli.lib.sync.flows.layer_sync_flow.rmtree_if_exists") @@ -449,7 +449,7 @@ def test_gather_dependencies(self): class TestLayerSyncFlowSkipBuild(TestCase): - @patch("samcli.lib.sync.flows.layer_sync_flow.make_zip") + @patch("samcli.lib.sync.flows.layer_sync_flow.make_zip_with_lambda_permissions") @patch("samcli.lib.sync.flows.layer_sync_flow.file_checksum") def test_gather_resources_for_skip_build_directory(self, mock_checksum, mock_make_zip): layer_sync_flow = LayerSyncFlowSkipBuildDirectory("LayerA", Mock(), Mock(), Mock(), {}, []) diff --git a/tests/unit/lib/sync/flows/test_zip_function_sync_flow.py b/tests/unit/lib/sync/flows/test_zip_function_sync_flow.py index 07fea55cc6..bedaaf7436 100644 --- a/tests/unit/lib/sync/flows/test_zip_function_sync_flow.py +++ b/tests/unit/lib/sync/flows/test_zip_function_sync_flow.py @@ -34,7 +34,7 @@ def test_set_up(self, session_mock, client_provider_mock): @patch("samcli.lib.sync.flows.zip_function_sync_flow.hashlib.sha256") @patch("samcli.lib.sync.flows.zip_function_sync_flow.uuid.uuid4") @patch("samcli.lib.sync.flows.zip_function_sync_flow.file_checksum") - @patch("samcli.lib.sync.flows.zip_function_sync_flow.make_zip") + @patch("samcli.lib.sync.flows.zip_function_sync_flow.make_zip_with_lambda_permissions") @patch("samcli.lib.sync.flows.zip_function_sync_flow.tempfile.gettempdir") @patch("samcli.lib.sync.flows.zip_function_sync_flow.ApplicationBuilder") @patch("samcli.lib.sync.flows.zip_function_sync_flow.rmtree_if_exists")