From ef6acdceeb09e6698ada4ad9bed9e67e86f3d866 Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Wed, 1 Feb 2023 22:34:43 -0800 Subject: [PATCH 1/6] fix: Package permissions * Additive permissions to add execute on directories and read on files. --- samcli/lib/package/packageable_resources.py | 3 +- samcli/lib/package/permissions.py | 47 ++++ samcli/lib/package/utils.py | 94 ++++++-- .../flows/auto_dependency_layer_sync_flow.py | 4 +- samcli/lib/sync/flows/layer_sync_flow.py | 4 +- .../lib/sync/flows/zip_function_sync_flow.py | 4 +- samcli/lib/utils/resources.py | 7 + tests/integration/sync/test_sync_adl.py | 4 +- tests/integration/sync/test_sync_watch.py | 1 + tests/testing_utils.py | 2 +- .../lib/package/test_artifact_exporter.py | 213 ++++++++++++++++-- .../test_auto_dependency_layer_sync_flow.py | 2 +- .../lib/sync/flows/test_layer_sync_flow.py | 2 +- .../sync/flows/test_zip_function_sync_flow.py | 2 +- 14 files changed, 334 insertions(+), 55 deletions(-) create mode 100644 samcli/lib/package/permissions.py 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..f54ec707f4 --- /dev/null +++ b/samcli/lib/package/permissions.py @@ -0,0 +1,47 @@ +""" +Classes which will change permissions on a ZipInfo object +""" +import platform +import zipfile + + +class FilePermissionMapper: + def __init__(self, permissions: oct): + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + if platform.system().lower() != "windows": + return zip_info + zip_info.external_attr = self.permissions << 16 if not zip_info.is_dir() else zip_info.external_attr + return zip_info + + +class DirPermissionMapper: + def __init__(self, permissions: oct): + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + if platform.system().lower() != "windows": + return zip_info + zip_info.external_attr = self.permissions << 16 if zip_info.is_dir() else zip_info.external_attr + return zip_info + + +class AdditiveDirPermissionMapper: + def __init__(self, permissions: oct): + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + if zip_info.is_dir(): + zip_info.external_attr |= self.permissions << 16 + return zip_info + + +class AdditiveFilePermissionMapper: + def __init__(self, permissions: oct): + self.permissions = permissions + + def apply(self, zip_info: zipfile.ZipInfo): + 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..4d330fbfa5 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 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 ( + FilePermissionMapper, + DirPermissionMapper, + AdditiveFilePermissionMapper, + AdditiveDirPermissionMapper, +) 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): """ Create a zip file from the source directory @@ -228,6 +246,9 @@ 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 @@ -242,28 +263,55 @@ 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 any(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: + if permission_mapper: + 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=[ + FilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + DirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + ], +) +# 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=[ + FilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + DirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + AdditiveFilePermissionMapper(permissions=0o100444), + AdditiveDirPermissionMapper(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 04d58b3867..6c47e22d52 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 ( @@ -85,7 +85,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 ae38e2bb9b..7c72e342cb 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 @@ -236,7 +236,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()) diff --git a/samcli/lib/sync/flows/zip_function_sync_flow.py b/samcli/lib/sync/flows/zip_function_sync_flow.py index 8987a23e56..ea573dfd7b 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 @@ -99,7 +99,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 463bdcbe2f..2dce048419 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/testing_utils.py b/tests/testing_utils.py index d410c6b599..6facc5cf9c 100644 --- a/tests/testing_utils.py +++ b/tests/testing_utils.py @@ -126,7 +126,7 @@ def read_until_string(process: Popen, expected_output: str, timeout: int = 5) -> """ def _compare_output(output, _: List[str]) -> bool: - return bool(output == expected_output) + return bool(expected_output in output) try: read_until(process, _compare_output, timeout) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index c5a473d033..537cc36fb6 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,12 @@ from unittest.mock import patch, Mock, MagicMock from samcli.commands.package.exceptions import ExportFailedError +from samcli.lib.package.permissions import FilePermissionMapper, DirPermissionMapper 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 +169,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 +288,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 +300,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 +317,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 +328,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 +339,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 +350,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 +437,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 +468,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 +494,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 +519,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 +682,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 +800,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 +901,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 +1687,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 +1695,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 +1710,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 +1730,14 @@ 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=[ + FilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + DirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + ], + ) test_file_creator = FileCreator() test_file_creator.append_file( @@ -1622,7 +1753,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 +1771,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/sync/flows/test_auto_dependency_layer_sync_flow.py b/tests/unit/lib/sync/flows/test_auto_dependency_layer_sync_flow.py index bfa0190976..e288a25bce 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 @@ -73,7 +73,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 a186c6f53a..df02eb28da 100644 --- a/tests/unit/lib/sync/flows/test_layer_sync_flow.py +++ b/tests/unit/lib/sync/flows/test_layer_sync_flow.py @@ -62,7 +62,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") 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 8e7365c970..3a8eb2cfe1 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 @@ -33,7 +33,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") From 58a982548938a855a022b6819d8f94cb0f8f15a4 Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Wed, 1 Feb 2023 22:55:11 -0800 Subject: [PATCH 2/6] tests: get unit tests passing --- samcli/lib/package/permissions.py | 8 ++++---- tests/unit/lib/package/test_artifact_exporter.py | 9 ++++++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/samcli/lib/package/permissions.py b/samcli/lib/package/permissions.py index f54ec707f4..579c09d5bb 100644 --- a/samcli/lib/package/permissions.py +++ b/samcli/lib/package/permissions.py @@ -6,7 +6,7 @@ class FilePermissionMapper: - def __init__(self, permissions: oct): + def __init__(self, permissions: int): self.permissions = permissions def apply(self, zip_info: zipfile.ZipInfo): @@ -17,7 +17,7 @@ def apply(self, zip_info: zipfile.ZipInfo): class DirPermissionMapper: - def __init__(self, permissions: oct): + def __init__(self, permissions: int): self.permissions = permissions def apply(self, zip_info: zipfile.ZipInfo): @@ -28,7 +28,7 @@ def apply(self, zip_info: zipfile.ZipInfo): class AdditiveDirPermissionMapper: - def __init__(self, permissions: oct): + def __init__(self, permissions: int): self.permissions = permissions def apply(self, zip_info: zipfile.ZipInfo): @@ -38,7 +38,7 @@ def apply(self, zip_info: zipfile.ZipInfo): class AdditiveFilePermissionMapper: - def __init__(self, permissions: oct): + def __init__(self, permissions: int): self.permissions = permissions def apply(self, zip_info: zipfile.ZipInfo): diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 537cc36fb6..45649f6066 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -13,7 +13,12 @@ from unittest.mock import patch, Mock, MagicMock from samcli.commands.package.exceptions import ExportFailedError -from samcli.lib.package.permissions import FilePermissionMapper, DirPermissionMapper +from samcli.lib.package.permissions import ( + FilePermissionMapper, + DirPermissionMapper, + AdditiveFilePermissionMapper, + AdditiveDirPermissionMapper, +) 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, make_zip_with_lambda_permissions, make_zip_with_permissions @@ -1736,6 +1741,8 @@ def test_make_zip_windows(self, mock_system): permission_mappers=[ FilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, DirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + AdditiveFilePermissionMapper(permissions=0o100444), + AdditiveDirPermissionMapper(permissions=0o100111), ], ) From 55692df10bdccca1ae0bd75b16e267086e23140d Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Thu, 2 Feb 2023 13:22:46 -0800 Subject: [PATCH 3/6] tests: extensive testing on permission-changes. --- samcli/lib/package/permissions.py | 4 +- samcli/lib/package/utils.py | 12 ++-- .../lib/package/test_artifact_exporter.py | 8 +-- tests/unit/lib/package/test_permissions.py | 71 +++++++++++++++++++ 4 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 tests/unit/lib/package/test_permissions.py diff --git a/samcli/lib/package/permissions.py b/samcli/lib/package/permissions.py index 579c09d5bb..871d3b89ac 100644 --- a/samcli/lib/package/permissions.py +++ b/samcli/lib/package/permissions.py @@ -5,7 +5,7 @@ import zipfile -class FilePermissionMapper: +class WindowsFilePermissionMapper: def __init__(self, permissions: int): self.permissions = permissions @@ -16,7 +16,7 @@ def apply(self, zip_info: zipfile.ZipInfo): return zip_info -class DirPermissionMapper: +class WindowsDirPermissionMapper: def __init__(self, permissions: int): self.permissions = permissions diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py index 4d330fbfa5..14dd868339 100644 --- a/samcli/lib/package/utils.py +++ b/samcli/lib/package/utils.py @@ -19,8 +19,8 @@ from samcli.commands.package.exceptions import ImageNotFoundError, InvalidLocalPathError from samcli.lib.package.ecr_utils import is_ecr_url from samcli.lib.package.permissions import ( - FilePermissionMapper, - DirPermissionMapper, + WindowsFilePermissionMapper, + WindowsDirPermissionMapper, AdditiveFilePermissionMapper, AdditiveDirPermissionMapper, ) @@ -292,8 +292,8 @@ def make_zip_with_permissions(file_name, source_root, permission_mappers): make_zip = functools.partial( make_zip_with_permissions, permission_mappers=[ - FilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - DirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + WindowsFilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + WindowsDirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, ], ) # Context: Jan 2023 @@ -304,8 +304,8 @@ def make_zip_with_permissions(file_name, source_root, permission_mappers): make_zip_with_lambda_permissions = functools.partial( make_zip_with_permissions, permission_mappers=[ - FilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - DirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + WindowsFilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + WindowsDirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, AdditiveFilePermissionMapper(permissions=0o100444), AdditiveDirPermissionMapper(permissions=0o100111), ], diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 45649f6066..f1dc1d14da 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -14,8 +14,8 @@ from samcli.commands.package.exceptions import ExportFailedError from samcli.lib.package.permissions import ( - FilePermissionMapper, - DirPermissionMapper, + WindowsFilePermissionMapper, + WindowsDirPermissionMapper, AdditiveFilePermissionMapper, AdditiveDirPermissionMapper, ) @@ -1739,8 +1739,8 @@ def test_make_zip_windows(self, mock_system): windows_make_zip = functools.partial( make_zip_with_permissions, permission_mappers=[ - FilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - DirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + WindowsFilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + WindowsDirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, AdditiveFilePermissionMapper(permissions=0o100444), AdditiveDirPermissionMapper(permissions=0o100111), ], diff --git a/tests/unit/lib/package/test_permissions.py b/tests/unit/lib/package/test_permissions.py new file mode 100644 index 0000000000..70c8af1e19 --- /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 ( + WindowsFilePermissionMapper, + WindowsDirPermissionMapper, + AdditiveFilePermissionMapper, + AdditiveDirPermissionMapper, +) + + +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 = WindowsFilePermissionMapper(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 = WindowsDirPermissionMapper(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 = WindowsFilePermissionMapper(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 = WindowsDirPermissionMapper(permissions=0o100755) + zi = zipfile.ZipInfo(filename="dir/") + self.assertEqual(mapper.apply(zi).external_attr, mapper.permissions << 16) + + @parameterized.expand( + [ + (AdditiveFilePermissionMapper, "file", 0o100000, 0o100444, 0o100444), + (AdditiveFilePermissionMapper, "file", 0o100111, 0o100444, 0o100555), + (AdditiveFilePermissionMapper, "file", 0o100444, 0o100444, 0o100444), + (AdditiveFilePermissionMapper, "file", 0o100644, 0o100444, 0o100644), + (AdditiveFilePermissionMapper, "file", 0o100777, 0o100444, 0o100777), + (AdditiveDirPermissionMapper, "dir/", 0o100000, 0o100111, 0o100111), + (AdditiveDirPermissionMapper, "dir/", 0o100111, 0o100444, 0o100555), + (AdditiveDirPermissionMapper, "dir/", 0o100111, 0o100111, 0o100111), + (AdditiveDirPermissionMapper, "dir/", 0o100644, 0o100111, 0o100755), + (AdditiveDirPermissionMapper, "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) From f7a830c30bbf72ab3c678231d60a26d16922929b Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Thu, 2 Feb 2023 13:34:57 -0800 Subject: [PATCH 4/6] doc: add clear docstrings to `permissions.py` --- samcli/lib/package/permissions.py | 106 ++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/samcli/lib/package/permissions.py b/samcli/lib/package/permissions.py index 871d3b89ac..474c3efb97 100644 --- a/samcli/lib/package/permissions.py +++ b/samcli/lib/package/permissions.py @@ -6,10 +6,36 @@ class WindowsFilePermissionMapper: + """ + 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": return zip_info zip_info.external_attr = self.permissions << 16 if not zip_info.is_dir() else zip_info.external_attr @@ -17,10 +43,36 @@ def apply(self, zip_info: zipfile.ZipInfo): class WindowsDirPermissionMapper: + """ + 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": return zip_info zip_info.external_attr = self.permissions << 16 if zip_info.is_dir() else zip_info.external_attr @@ -28,20 +80,74 @@ def apply(self, zip_info: zipfile.ZipInfo): class AdditiveDirPermissionMapper: + """ + 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 AdditiveFilePermissionMapper: + """ + 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 From c961fb7a8e5152eefcc1bed6e586b4aa9d974f58 Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Sat, 4 Feb 2023 23:08:32 -0800 Subject: [PATCH 5/6] fix: address comments --- samcli/lib/package/permissions.py | 27 ++++++++------ samcli/lib/package/utils.py | 32 ++++++++--------- samcli/lib/sync/flows/layer_sync_flow.py | 2 +- .../lib/package/test_artifact_exporter.py | 16 ++++----- tests/unit/lib/package/test_permissions.py | 36 +++++++++---------- 5 files changed, 60 insertions(+), 53 deletions(-) diff --git a/samcli/lib/package/permissions.py b/samcli/lib/package/permissions.py index 474c3efb97..24da0bdac5 100644 --- a/samcli/lib/package/permissions.py +++ b/samcli/lib/package/permissions.py @@ -5,7 +5,16 @@ import zipfile -class WindowsFilePermissionMapper: +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 @@ -36,13 +45,12 @@ def apply(self, zip_info: zipfile.ZipInfo): modified object with `external_attr` set to member permissions. """ - if platform.system().lower() != "windows": - return zip_info - zip_info.external_attr = self.permissions << 16 if not zip_info.is_dir() else zip_info.external_attr + if platform.system().lower() == "windows" and not zip_info.is_dir(): + zip_info.external_attr = self.permissions << 16 return zip_info -class WindowsDirPermissionMapper: +class WindowsDirPermissionPermissionMapper(PermissionMapper): """ Windows specific Permission Mapper onto a zipfile.ZipInfo object to set explicit permissions onto `external_attr` attribute if the given object @@ -73,13 +81,12 @@ def apply(self, zip_info: zipfile.ZipInfo): modified object with `external_attr` set to member permissions. """ - if platform.system().lower() != "windows": - return zip_info - zip_info.external_attr = self.permissions << 16 if zip_info.is_dir() else zip_info.external_attr + if platform.system().lower() == "windows" and zip_info.is_dir(): + zip_info.external_attr = self.permissions << 16 return zip_info -class AdditiveDirPermissionMapper: +class AdditiveDirPermissionPermissionMapper(PermissionMapper): """ Additive Permission Mapper onto a zipfile.ZipInfo object to set additive permissions onto `external_attr` attribute if the given object @@ -116,7 +123,7 @@ def apply(self, zip_info: zipfile.ZipInfo): return zip_info -class AdditiveFilePermissionMapper: +class AdditiveFilePermissionPermissionMapper(PermissionMapper): """ Additive Permission Mapper onto a zipfile.ZipInfo object to set additive permissions onto `external_attr` attribute if the given object diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py index 14dd868339..e5116f1db8 100644 --- a/samcli/lib/package/utils.py +++ b/samcli/lib/package/utils.py @@ -4,7 +4,6 @@ import functools import logging import os -import platform import re import shutil import tempfile @@ -12,17 +11,18 @@ import zipfile import contextlib from contextlib import contextmanager -from typing import Dict, Optional, Callable, 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 ( - WindowsFilePermissionMapper, - WindowsDirPermissionMapper, - AdditiveFilePermissionMapper, - AdditiveDirPermissionMapper, + WindowsFilePermissionPermissionMapper, + WindowsDirPermissionPermissionMapper, + AdditiveFilePermissionPermissionMapper, + AdditiveDirPermissionPermissionMapper, + PermissionMapper, ) from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.utils.hash import dir_checksum @@ -236,7 +236,7 @@ def zip_folder(folder_path, zip_method): os.remove(zipfile_name) -def make_zip_with_permissions(file_name, source_root, permission_mappers): +def make_zip_with_permissions(file_name, source_root, permission_mappers: List[PermissionMapper]): """ Create a zip file from the source directory @@ -254,6 +254,7 @@ def make_zip_with_permissions(file_name, source_root, permission_mappers): 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 @@ -272,14 +273,13 @@ def make_zip_with_permissions(file_name, source_root, permission_mappers): # 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 any(permission_mappers): + 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: - if permission_mapper: - info = permission_mapper.apply(info) + 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) @@ -292,8 +292,8 @@ def make_zip_with_permissions(file_name, source_root, permission_mappers): make_zip = functools.partial( make_zip_with_permissions, permission_mappers=[ - WindowsFilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - WindowsDirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, + WindowsFilePermissionPermissionMapper(permissions=0o100755), + WindowsDirPermissionPermissionMapper(permissions=0o100755), ], ) # Context: Jan 2023 @@ -304,10 +304,10 @@ def make_zip_with_permissions(file_name, source_root, permission_mappers): make_zip_with_lambda_permissions = functools.partial( make_zip_with_permissions, permission_mappers=[ - WindowsFilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - WindowsDirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - AdditiveFilePermissionMapper(permissions=0o100444), - AdditiveDirPermissionMapper(permissions=0o100111), + WindowsFilePermissionPermissionMapper(permissions=0o100755), + WindowsDirPermissionPermissionMapper(permissions=0o100755), + AdditiveFilePermissionPermissionMapper(permissions=0o100444), + AdditiveDirPermissionPermissionMapper(permissions=0o100111), ], ) diff --git a/samcli/lib/sync/flows/layer_sync_flow.py b/samcli/lib/sync/flows/layer_sync_flow.py index 7c72e342cb..9e2bda2801 100644 --- a/samcli/lib/sync/flows/layer_sync_flow.py +++ b/samcli/lib/sync/flows/layer_sync_flow.py @@ -266,7 +266,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/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index f1dc1d14da..159748dc77 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -14,10 +14,10 @@ from samcli.commands.package.exceptions import ExportFailedError from samcli.lib.package.permissions import ( - WindowsFilePermissionMapper, - WindowsDirPermissionMapper, - AdditiveFilePermissionMapper, - AdditiveDirPermissionMapper, + WindowsFilePermissionPermissionMapper, + WindowsDirPermissionPermissionMapper, + AdditiveFilePermissionPermissionMapper, + AdditiveDirPermissionPermissionMapper, ) from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.package.uploaders import Destination @@ -1739,10 +1739,10 @@ def test_make_zip_windows(self, mock_system): windows_make_zip = functools.partial( make_zip_with_permissions, permission_mappers=[ - WindowsFilePermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - WindowsDirPermissionMapper(permissions=0o100755) if platform.system().lower() == "windows" else None, - AdditiveFilePermissionMapper(permissions=0o100444), - AdditiveDirPermissionMapper(permissions=0o100111), + WindowsFilePermissionPermissionMapper(permissions=0o100755), + WindowsDirPermissionPermissionMapper(permissions=0o100755), + AdditiveFilePermissionPermissionMapper(permissions=0o100444), + AdditiveDirPermissionPermissionMapper(permissions=0o100111), ], ) diff --git a/tests/unit/lib/package/test_permissions.py b/tests/unit/lib/package/test_permissions.py index 70c8af1e19..ab8c26b13a 100644 --- a/tests/unit/lib/package/test_permissions.py +++ b/tests/unit/lib/package/test_permissions.py @@ -6,10 +6,10 @@ from parameterized import parameterized from samcli.lib.package.permissions import ( - WindowsFilePermissionMapper, - WindowsDirPermissionMapper, - AdditiveFilePermissionMapper, - AdditiveDirPermissionMapper, + WindowsFilePermissionPermissionMapper, + WindowsDirPermissionPermissionMapper, + AdditiveFilePermissionPermissionMapper, + AdditiveDirPermissionPermissionMapper, ) @@ -18,7 +18,7 @@ class TestPermissions(TestCase): def test_file_permission_mapper_linux(self, mock_system): mock_system.return_value = "Linux" # permissions set are ignored. - mapper = WindowsFilePermissionMapper(permissions=0o100777) + mapper = WindowsFilePermissionPermissionMapper(permissions=0o100777) zi = zipfile.ZipInfo() self.assertEqual(mapper.apply(zi), zi) # return as-is @@ -28,7 +28,7 @@ def test_file_permission_mapper_linux(self, mock_system): def test_dir_permission_mapper_linux(self, mock_system): mock_system.return_value = "Linux" # permissions set are ignored. - mapper = WindowsDirPermissionMapper(permissions=0o100777) + mapper = WindowsDirPermissionPermissionMapper(permissions=0o100777) zi = zipfile.ZipInfo(filename="dir/") self.assertEqual(mapper.apply(zi), zi) # return as-is @@ -37,29 +37,29 @@ def test_dir_permission_mapper_linux(self, mock_system): @patch("platform.system") def test_file_permission_mapper_windows(self, mock_system): mock_system.return_value = "Windows" - mapper = WindowsFilePermissionMapper(permissions=0o100644) + 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 = WindowsDirPermissionMapper(permissions=0o100755) + mapper = WindowsDirPermissionPermissionMapper(permissions=0o100755) zi = zipfile.ZipInfo(filename="dir/") self.assertEqual(mapper.apply(zi).external_attr, mapper.permissions << 16) @parameterized.expand( [ - (AdditiveFilePermissionMapper, "file", 0o100000, 0o100444, 0o100444), - (AdditiveFilePermissionMapper, "file", 0o100111, 0o100444, 0o100555), - (AdditiveFilePermissionMapper, "file", 0o100444, 0o100444, 0o100444), - (AdditiveFilePermissionMapper, "file", 0o100644, 0o100444, 0o100644), - (AdditiveFilePermissionMapper, "file", 0o100777, 0o100444, 0o100777), - (AdditiveDirPermissionMapper, "dir/", 0o100000, 0o100111, 0o100111), - (AdditiveDirPermissionMapper, "dir/", 0o100111, 0o100444, 0o100555), - (AdditiveDirPermissionMapper, "dir/", 0o100111, 0o100111, 0o100111), - (AdditiveDirPermissionMapper, "dir/", 0o100644, 0o100111, 0o100755), - (AdditiveDirPermissionMapper, "dir/", 0o100777, 0o100111, 0o100777), + (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( From 5c45cfcd60e8af8247ffafeb223a43160d12db68 Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Sun, 5 Feb 2023 14:03:24 -0800 Subject: [PATCH 6/6] tests: set the appropriate mock --- tests/unit/lib/sync/flows/test_layer_sync_flow.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 df02eb28da..52f876f2a7 100644 --- a/tests/unit/lib/sync/flows/test_layer_sync_flow.py +++ b/tests/unit/lib/sync/flows/test_layer_sync_flow.py @@ -437,7 +437,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(), {}, [])