From 2a47b6a373c01fd97025f8a32a17d9359b23e328 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Wed, 8 Nov 2023 20:09:30 +0100 Subject: [PATCH 01/25] implement purge_extract_dir option for Client to prevent accidental deletion directories with unrelated files, we require the extract_dir name to match a specified name --- src/tufup/client.py | 24 +++++++++++++++++++++--- src/tufup/utils/platform_specific.py | 2 +- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/tufup/client.py b/src/tufup/client.py index 5ccebd2..b16d036 100644 --- a/src/tufup/client.py +++ b/src/tufup/client.py @@ -13,11 +13,14 @@ import tuf.ngclient from tufup.common import TargetMeta +from tufup.utils import remove_path from tufup.utils.platform_specific import install_update logger = logging.getLogger(__name__) -DEFAULT_EXTRACT_DIR = pathlib.Path(tempfile.gettempdir()) / 'tufup' +# dir name must be unequivocally linked to tufup, as dir contents are removed +EXTRACT_DIR_NAME = 'tufup_temporary_extract_dir' +DEFAULT_EXTRACT_DIR = pathlib.Path(tempfile.gettempdir()) / EXTRACT_DIR_NAME class Client(tuf.ngclient.Updater): @@ -33,6 +36,7 @@ def __init__( extract_dir: Optional[pathlib.Path] = None, refresh_required: bool = False, session_auth: Optional[Dict[str, Union[Tuple[str, str], AuthBase]]] = None, + purge_extract_dir: bool = False, **kwargs, ): # tuf.ngclient.Updater.__init__ loads local root metadata automatically @@ -46,6 +50,7 @@ def __init__( ) self.app_install_dir = app_install_dir self.extract_dir = extract_dir + self.purge_extract_dir = purge_extract_dir self.refresh_required = refresh_required self.current_archive = TargetMeta(name=app_name, version=current_version) self.current_archive_local_path = target_dir / self.current_archive.path @@ -257,12 +262,25 @@ def _apply_updates( self.new_archive_info.verify_length_and_hashes(data=archive_bytes) # write the patched new archive self.new_archive_local_path.write_bytes(archive_bytes) - # extract archive to temporary directory + # extract archive to (temporary) directory if self.extract_dir is None: self.extract_dir = DEFAULT_EXTRACT_DIR self.extract_dir.mkdir(exist_ok=True) logger.debug(f'default extract dir created: {self.extract_dir}') - # extract + # remove any files/folders present in the extract_dir (recursively), + # if requested, but ony if the extract_dir is properly "namespaced" (this is + # intended to prevent accidental deletion of unrelated files from a + # user-specified extract_dir) + if self.purge_extract_dir: + if self.extract_dir.name == EXTRACT_DIR_NAME: + for path_item in self.extract_dir.iterdir(): + remove_path(path=path_item) + else: + logger.warning( + f'cannot clean extract_dir {self.extract_dir}: ' + f'name does not match "{EXTRACT_DIR_NAME}"' + ) + # extract the archive into the extract_dir shutil.unpack_archive( filename=self.new_archive_local_path, extract_dir=self.extract_dir ) diff --git a/src/tufup/utils/platform_specific.py b/src/tufup/utils/platform_specific.py index 096519c..323989a 100644 --- a/src/tufup/utils/platform_specific.py +++ b/src/tufup/utils/platform_specific.py @@ -128,7 +128,7 @@ def _install_update_win( batch_template_extra_kwargs: Optional[dict] = None, log_file_name: Optional[str] = None, robocopy_options_override: Optional[List[str]] = None, - process_creation_flags = None, + process_creation_flags=None, ): """ Create a batch script that moves files from src to dst, then run the From 162060c5c7c4a6f4ad136f41789f145e0a16d019 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Wed, 8 Nov 2023 22:49:05 +0100 Subject: [PATCH 02/25] add separate Client._extract_archive() method with purge.manifest including rudimentary test --- src/tufup/client.py | 71 ++++++++++++++++++++++++++++---------------- tests/test_client.py | 64 ++++++++++++++++++++++++++++----------- 2 files changed, 91 insertions(+), 44 deletions(-) diff --git a/src/tufup/client.py b/src/tufup/client.py index b16d036..ce274f6 100644 --- a/src/tufup/client.py +++ b/src/tufup/client.py @@ -1,8 +1,10 @@ import bsdiff4 +import json import logging import pathlib import shutil import sys +import tarfile import tempfile from typing import Callable, Dict, Iterator, List, Optional, Tuple, Union from urllib import parse @@ -19,7 +21,8 @@ logger = logging.getLogger(__name__) # dir name must be unequivocally linked to tufup, as dir contents are removed -EXTRACT_DIR_NAME = 'tufup_temporary_extract_dir' +EXTRACT_DIR_PURGE_MANIFEST_NAME = 'purge.manifest' +EXTRACT_DIR_NAME = 'tufup_extract_dir' DEFAULT_EXTRACT_DIR = pathlib.Path(tempfile.gettempdir()) / EXTRACT_DIR_NAME @@ -262,30 +265,9 @@ def _apply_updates( self.new_archive_info.verify_length_and_hashes(data=archive_bytes) # write the patched new archive self.new_archive_local_path.write_bytes(archive_bytes) - # extract archive to (temporary) directory - if self.extract_dir is None: - self.extract_dir = DEFAULT_EXTRACT_DIR - self.extract_dir.mkdir(exist_ok=True) - logger.debug(f'default extract dir created: {self.extract_dir}') - # remove any files/folders present in the extract_dir (recursively), - # if requested, but ony if the extract_dir is properly "namespaced" (this is - # intended to prevent accidental deletion of unrelated files from a - # user-specified extract_dir) - if self.purge_extract_dir: - if self.extract_dir.name == EXTRACT_DIR_NAME: - for path_item in self.extract_dir.iterdir(): - remove_path(path=path_item) - else: - logger.warning( - f'cannot clean extract_dir {self.extract_dir}: ' - f'name does not match "{EXTRACT_DIR_NAME}"' - ) - # extract the archive into the extract_dir - shutil.unpack_archive( - filename=self.new_archive_local_path, extract_dir=self.extract_dir - ) - logger.debug(f'files extracted to {self.extract_dir}') - # install + # extract archive to "temporary" directory + self._extract_archive() + # install, i.e. move extracted files from "temporary" dir to final "install" dir confirmation_message = f'Install update in {self.app_install_dir}? [y]/n' if skip_confirmation or input(confirmation_message) in ['y', '']: # start a script that moves the extracted files to the app install @@ -297,7 +279,44 @@ def _apply_updates( ) else: logger.warning('Installation aborted.') - # todo: clean up deprecated local archive + + def _extract_archive(self): + # ensure extract_dir exists + if self.extract_dir is None: + self.extract_dir = DEFAULT_EXTRACT_DIR + logger.debug(f'using default extract_dir: {self.extract_dir}') + self.extract_dir.mkdir(exist_ok=True) + # purge all files/folders present in the extract_dir, but only if they are + # listed in the previous archive's purge manifest (this should prevent + # accidental removal of files unrelated to tufup) + purge_manifest_path = self.extract_dir / EXTRACT_DIR_PURGE_MANIFEST_NAME + if self.purge_extract_dir: + if purge_manifest_path.exists(): + purge_manifest = json.loads(purge_manifest_path.read_text()) + for path_item in self.extract_dir.iterdir(): + if path_item.name in purge_manifest: + remove_path(path=path_item) + else: + logger.warning( + f'{path_item} not in {EXTRACT_DIR_PURGE_MANIFEST_NAME}' + ) + logger.info(f'extract_dir purged: {self.extract_dir}') + else: + logger.warning( + f'cannot purge: {EXTRACT_DIR_PURGE_MANIFEST_NAME} not found' + ) + # extract the new archive into the "temporary" extract_dir + shutil.unpack_archive( + filename=self.new_archive_local_path, extract_dir=self.extract_dir + ) + logger.debug(f'files extracted to {self.extract_dir}') + # create/overwrite purge manifest in extract_dir + with tarfile.open(self.new_archive_local_path) as tar: + new_purge_manifest = [ + pathlib.Path(t.name).name for t in tar if t.name != '.' + ] + purge_manifest_path.write_text(json.dumps(new_purge_manifest)) + logger.debug(f'purge manifest created/updated: {purge_manifest_path}') class AuthRequestsFetcher(tuf.ngclient.RequestsFetcher): diff --git a/tests/test_client.py b/tests/test_client.py index 482652c..9fcd6dd 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -11,7 +11,7 @@ from tuf.ngclient import TargetFile from tests import TempDirTestCase, TEST_REPO_DIR -from tufup.client import AuthRequestsFetcher, Client +from tufup.client import AuthRequestsFetcher, Client, EXTRACT_DIR_PURGE_MANIFEST_NAME from tufup.common import TargetMeta ROOT_FILENAME = 'root.json' @@ -77,6 +77,25 @@ def get_refreshed_client(self): ) return client + def populate_refreshed_client(self, client): + # directly use target files from test repo as downloaded files + client.downloaded_target_files = { + target_meta: TEST_REPO_DIR / 'targets' / str(target_meta) + for target_meta in client.trusted_target_metas + if target_meta.is_patch and str(target_meta.version) in ['2.0', '3.0rc0'] + } + # specify new archive (normally done in _check_updates) + archives = [ + tp + for tp in client.trusted_target_metas + if tp.is_archive and str(tp.version) == '3.0rc0' + ] + client.new_archive_info = client.get_targetinfo(archives[-1]) + client.new_archive_local_path = pathlib.Path( + client.target_dir, client.new_archive_info.path + ) + return client + def test_init_no_metadata(self): # cannot initialize without root metadata file (self.metadata_dir / ROOT_FILENAME).unlink() @@ -185,23 +204,7 @@ def test__download_updates(self): self.assertEqual(downloaded_path, str(local_path)) def test__apply_updates(self): - client = self.get_refreshed_client() - # directly use target files from test repo as downloaded files - client.downloaded_target_files = { - target_meta: TEST_REPO_DIR / 'targets' / str(target_meta) - for target_meta in client.trusted_target_metas - if target_meta.is_patch and str(target_meta.version) in ['2.0', '3.0rc0'] - } - # specify new archive (normally done in _check_updates) - archives = [ - tp - for tp in client.trusted_target_metas - if tp.is_archive and str(tp.version) == '3.0rc0' - ] - client.new_archive_info = client.get_targetinfo(archives[-1]) - client.new_archive_local_path = pathlib.Path( - client.target_dir, client.new_archive_info.path - ) + client = self.populate_refreshed_client(client=self.get_refreshed_client()) # test confirmation mock_install = Mock() with patch('builtins.input', Mock(return_value='y')): @@ -213,6 +216,31 @@ def test__apply_updates(self): client._apply_updates(install=mock_install, skip_confirmation=True) mock_install.assert_called() + def test__extract_archive_with_purge(self): + temp_extract_dir = self.temp_dir_path / 'tufup-extract-dir' + temp_extract_dir.mkdir() + self.client_kwargs['extract_dir'] = temp_extract_dir + self.client_kwargs['purge_extract_dir'] = True + client = self.populate_refreshed_client(client=self.get_refreshed_client()) + # ensure new archive exists in targets dir(dummy) + shutil.copy( + src=TEST_REPO_DIR / 'targets' / client.new_archive_local_path.name, + dst=client.new_archive_local_path, + ) + # test extract without purge manifest + client._extract_archive() + self.assertTrue(any(client.extract_dir.iterdir())) + # purge manifest should now exist + purge_manifest_path = client.extract_dir / EXTRACT_DIR_PURGE_MANIFEST_NAME + purge_manifest_content = purge_manifest_path.read_text() + print(purge_manifest_content) + self.assertTrue(purge_manifest_content) + # test extract with purge manifest + with self.assertLogs(level='DEBUG') as logs: + client._extract_archive() + print(logs.output) + self.assertTrue(any('removed file' in msg.lower() for msg in logs.output)) + class AuthRequestsFetcherTests(unittest.TestCase): def setUp(self) -> None: From cc174be9c3d667abf645a57d8c5e7dbbfa410254 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Thu, 9 Nov 2023 10:15:11 +0100 Subject: [PATCH 03/25] include purge.manifest itself in purge, and improve test --- src/tufup/client.py | 4 ++-- tests/test_client.py | 26 +++++++++++++++++--------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/tufup/client.py b/src/tufup/client.py index ce274f6..85539f4 100644 --- a/src/tufup/client.py +++ b/src/tufup/client.py @@ -312,11 +312,11 @@ def _extract_archive(self): logger.debug(f'files extracted to {self.extract_dir}') # create/overwrite purge manifest in extract_dir with tarfile.open(self.new_archive_local_path) as tar: - new_purge_manifest = [ + new_purge_manifest = [EXTRACT_DIR_PURGE_MANIFEST_NAME] + [ pathlib.Path(t.name).name for t in tar if t.name != '.' ] purge_manifest_path.write_text(json.dumps(new_purge_manifest)) - logger.debug(f'purge manifest created/updated: {purge_manifest_path}') + logger.debug(f'purge manifest created: {purge_manifest_path}') class AuthRequestsFetcher(tuf.ngclient.RequestsFetcher): diff --git a/tests/test_client.py b/tests/test_client.py index 9fcd6dd..f8190a7 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,3 +1,4 @@ +import json import logging import os import pathlib @@ -53,7 +54,7 @@ def setUp(self) -> None: ) def mock_download_metadata( - self, rolename: str, length: int, version: Optional[int] = None + self, rolename: str, length: int, version: Optional[int] = None ) -> bytes: if rolename == 'root': # indicate current root is newest version @@ -141,7 +142,7 @@ def test_download_and_apply_update(self): mock_apply = Mock(return_value=True) mock_install = Mock() with patch.multiple( - Client, _download_updates=mock_download, _apply_updates=mock_apply + Client, _download_updates=mock_download, _apply_updates=mock_apply ): client = self.get_refreshed_client() client.new_targets = {'dummy': None} @@ -192,9 +193,9 @@ def test__download_updates(self): client.new_targets = {Mock(): Mock()} for cached_path, downloaded_path in [('cached', None), (None, 'downloaded')]: with patch.multiple( - client, - find_cached_target=Mock(return_value=cached_path), - download_target=Mock(return_value=downloaded_path), + client, + find_cached_target=Mock(return_value=cached_path), + download_target=Mock(return_value=downloaded_path), ): self.assertTrue(client._download_updates(progress_hook=None)) local_path = next(iter(client.downloaded_target_files.values())) @@ -232,14 +233,21 @@ def test__extract_archive_with_purge(self): self.assertTrue(any(client.extract_dir.iterdir())) # purge manifest should now exist purge_manifest_path = client.extract_dir / EXTRACT_DIR_PURGE_MANIFEST_NAME - purge_manifest_content = purge_manifest_path.read_text() + purge_manifest_content = json.loads(purge_manifest_path.read_text()) print(purge_manifest_content) - self.assertTrue(purge_manifest_content) + self.assertEqual( + {'1.root.json', 'dummy.exe', EXTRACT_DIR_PURGE_MANIFEST_NAME}, + set(purge_manifest_content), + ) # test extract with purge manifest with self.assertLogs(level='DEBUG') as logs: client._extract_archive() - print(logs.output) - self.assertTrue(any('removed file' in msg.lower() for msg in logs.output)) + for item in logs.output: + print(item) + self.assertEqual( + len(purge_manifest_content), + sum('removed file' in msg.lower() for msg in logs.output), + ) class AuthRequestsFetcherTests(unittest.TestCase): From 1358fd235c681804ef29398f7a1139097380cedc Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Thu, 9 Nov 2023 16:53:39 +0100 Subject: [PATCH 04/25] note about test data --- tests/data/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/data/README.md b/tests/data/README.md index 1fb1581..f9fc49a 100644 --- a/tests/data/README.md +++ b/tests/data/README.md @@ -1,3 +1,5 @@ Large parts of the test data were copied verbatim from the `python-tuf` [repository_data][1] folder. +The repository data were created using `examples/repo_workflow_example.py`. + [1]: https://github.com/theupdateframework/python-tuf/tree/develop/tests/repository_data From 070d8c7a25d759c7e568c8a9e58c30953b52ed0b Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Thu, 9 Nov 2023 22:39:02 +0100 Subject: [PATCH 05/25] move purge manifest into separate PurgeManifest class, with proper tests work in progress... --- src/tufup/client.py | 101 ++++++++++++++++++++-------- src/tufup/repo/__init__.py | 2 +- tests/test_client.py | 134 ++++++++++++++++++++++++++++++------- 3 files changed, 184 insertions(+), 53 deletions(-) diff --git a/src/tufup/client.py b/src/tufup/client.py index 85539f4..83df1da 100644 --- a/src/tufup/client.py +++ b/src/tufup/client.py @@ -21,7 +21,6 @@ logger = logging.getLogger(__name__) # dir name must be unequivocally linked to tufup, as dir contents are removed -EXTRACT_DIR_PURGE_MANIFEST_NAME = 'purge.manifest' EXTRACT_DIR_NAME = 'tufup_extract_dir' DEFAULT_EXTRACT_DIR = pathlib.Path(tempfile.gettempdir()) / EXTRACT_DIR_NAME @@ -286,37 +285,16 @@ def _extract_archive(self): self.extract_dir = DEFAULT_EXTRACT_DIR logger.debug(f'using default extract_dir: {self.extract_dir}') self.extract_dir.mkdir(exist_ok=True) - # purge all files/folders present in the extract_dir, but only if they are - # listed in the previous archive's purge manifest (this should prevent - # accidental removal of files unrelated to tufup) - purge_manifest_path = self.extract_dir / EXTRACT_DIR_PURGE_MANIFEST_NAME - if self.purge_extract_dir: - if purge_manifest_path.exists(): - purge_manifest = json.loads(purge_manifest_path.read_text()) - for path_item in self.extract_dir.iterdir(): - if path_item.name in purge_manifest: - remove_path(path=path_item) - else: - logger.warning( - f'{path_item} not in {EXTRACT_DIR_PURGE_MANIFEST_NAME}' - ) - logger.info(f'extract_dir purged: {self.extract_dir}') - else: - logger.warning( - f'cannot purge: {EXTRACT_DIR_PURGE_MANIFEST_NAME} not found' - ) - # extract the new archive into the "temporary" extract_dir + # purge extract_dir + purge_manifest = PurgeManifest(dir_to_purge=self.extract_dir) + purge_manifest.purge() + # extract the new archive into the extract_dir shutil.unpack_archive( filename=self.new_archive_local_path, extract_dir=self.extract_dir ) logger.debug(f'files extracted to {self.extract_dir}') - # create/overwrite purge manifest in extract_dir - with tarfile.open(self.new_archive_local_path) as tar: - new_purge_manifest = [EXTRACT_DIR_PURGE_MANIFEST_NAME] + [ - pathlib.Path(t.name).name for t in tar if t.name != '.' - ] - purge_manifest_path.write_text(json.dumps(new_purge_manifest)) - logger.debug(f'purge manifest created: {purge_manifest_path}') + # create new purge manifest in extract_dir + purge_manifest.create_from_archive(archive=self.new_archive_local_path) class AuthRequestsFetcher(tuf.ngclient.RequestsFetcher): @@ -393,3 +371,70 @@ def _chunks(self, response: 'requests.Response') -> Iterator[bytes]: for data in super()._chunks(response=response): self._progress(bytes_new=len(data)) yield data + + +class PurgeManifest(object): + _filename = 'purge.manifest' + + def __init__(self, dir_to_purge: Union[pathlib.Path, str]): + """ + a purge manifest is used to remove files and subdirectories from the + specified directory (`dir_to_purge`) in a safe manner, i.e. minimizing the + risk of accidental removal of pre-existing files/dirs that are unrelated to + the present application + + this is achieved by removing only those items listed in a purge.manifest file + in the specified directory + + if there is no purge.manifest file, nothing is removed + + the purge.manifest file should be created (or updated) whenever files are + added to the `dir_to_purge` + + the purge manifest can be used to clear the extract_dir or the + app_install_dir, for example + """ + self.dir_to_purge = pathlib.Path(dir_to_purge) + + @property + def file_path(self) -> pathlib.Path: + return self.dir_to_purge / self._filename + + def read_from_file(self) -> Optional[List[str]]: + if not self.file_path.exists(): + logger.warning(f'cannot read manifest: {self.file_path} not found') + return + return json.loads(self.file_path.read_text()) + + def write_to_file(self, manifest: List[str]): + self.file_path.write_text(json.dumps(manifest)) + logger.debug(f'purge manifest created: {self.file_path}') + + def purge(self) -> None: + """ + remove all files/folders listed in the purge manifest from the specified + directory (`dir_to_purge`) + """ + manifest = self.read_from_file() + if manifest: + for path in self.dir_to_purge.iterdir(): + relative_path = path.relative_to(self.dir_to_purge) + if str(relative_path) in manifest: + remove_path(path=path) + else: + logger.warning(f'{path} remains: not in {self._filename}') + logger.info(f'directory purged: {self.dir_to_purge}') + + def create_from_archive(self, archive: Union[pathlib.Path, str]) -> None: + """create/overwrite purge manifest in extract_dir""" + # note that `shutil.make_archive` puts everything in a dir called "." (for + # cwd) and prefixes items with "./" (if this ever poses a problem, we could + # switch to using `tarfile.TarFile.add()` with the `arcname` argument) see + # e.g https://stackoverflow.com/a/70081512 + with tarfile.open(archive) as tar: + manifest = [self._filename] + [ + tarinfo.name[2:] if tarinfo.name.startswith('./') else tarinfo.name + for tarinfo in tar + if tarinfo.name != '.' + ] + self.write_to_file(manifest=manifest) diff --git a/src/tufup/repo/__init__.py b/src/tufup/repo/__init__.py index 30b8890..cf49e9a 100644 --- a/src/tufup/repo/__init__.py +++ b/src/tufup/repo/__init__.py @@ -97,7 +97,7 @@ def make_gztar_archive( if input(f'Found existing archive: {archive_path}.\nOverwrite? [n]/y') != 'y': print('Using existing archive.') return TargetMeta(archive_path) - # make archive + # make archive (see https://stackoverflow.com/a/70081512 about "." directory) base_name = str(dst_dir / archive_filename.replace(SUFFIX_ARCHIVE, '')) archive_path_str = shutil.make_archive( base_name=base_name, # archive file path, no suffix diff --git a/tests/test_client.py b/tests/test_client.py index f8190a7..0a85763 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -3,6 +3,8 @@ import os import pathlib import shutil +import subprocess +import tarfile from typing import Optional import unittest from unittest.mock import Mock, patch @@ -12,8 +14,9 @@ from tuf.ngclient import TargetFile from tests import TempDirTestCase, TEST_REPO_DIR -from tufup.client import AuthRequestsFetcher, Client, EXTRACT_DIR_PURGE_MANIFEST_NAME +from tufup.client import AuthRequestsFetcher, Client, PurgeManifest from tufup.common import TargetMeta +from tufup.utils.platform_specific import ON_WINDOWS ROOT_FILENAME = 'root.json' TARGETS_FILENAME = 'targets.json' @@ -54,7 +57,7 @@ def setUp(self) -> None: ) def mock_download_metadata( - self, rolename: str, length: int, version: Optional[int] = None + self, rolename: str, length: int, version: Optional[int] = None ) -> bytes: if rolename == 'root': # indicate current root is newest version @@ -142,7 +145,7 @@ def test_download_and_apply_update(self): mock_apply = Mock(return_value=True) mock_install = Mock() with patch.multiple( - Client, _download_updates=mock_download, _apply_updates=mock_apply + Client, _download_updates=mock_download, _apply_updates=mock_apply ): client = self.get_refreshed_client() client.new_targets = {'dummy': None} @@ -193,9 +196,9 @@ def test__download_updates(self): client.new_targets = {Mock(): Mock()} for cached_path, downloaded_path in [('cached', None), (None, 'downloaded')]: with patch.multiple( - client, - find_cached_target=Mock(return_value=cached_path), - download_target=Mock(return_value=downloaded_path), + client, + find_cached_target=Mock(return_value=cached_path), + download_target=Mock(return_value=downloaded_path), ): self.assertTrue(client._download_updates(progress_hook=None)) local_path = next(iter(client.downloaded_target_files.values())) @@ -228,26 +231,12 @@ def test__extract_archive_with_purge(self): src=TEST_REPO_DIR / 'targets' / client.new_archive_local_path.name, dst=client.new_archive_local_path, ) - # test extract without purge manifest + # test extract client._extract_archive() self.assertTrue(any(client.extract_dir.iterdir())) - # purge manifest should now exist - purge_manifest_path = client.extract_dir / EXTRACT_DIR_PURGE_MANIFEST_NAME - purge_manifest_content = json.loads(purge_manifest_path.read_text()) - print(purge_manifest_content) - self.assertEqual( - {'1.root.json', 'dummy.exe', EXTRACT_DIR_PURGE_MANIFEST_NAME}, - set(purge_manifest_content), - ) - # test extract with purge manifest - with self.assertLogs(level='DEBUG') as logs: - client._extract_archive() - for item in logs.output: - print(item) - self.assertEqual( - len(purge_manifest_content), - sum('removed file' in msg.lower() for msg in logs.output), - ) + # a purge manifest file should now exist + purge_manifest = PurgeManifest(dir_to_purge=client.extract_dir) + self.assertTrue(purge_manifest.file_path.exists()) class AuthRequestsFetcherTests(unittest.TestCase): @@ -344,3 +333,100 @@ def mock_iter_content(*args): for __ in fetcher._chunks(response=mock_response): pass self.assertEqual(chunk_count, mock_hook.call_count) + + +class PurgeManifestTests(TempDirTestCase): + def test_init(self): + self.assertTrue(PurgeManifest(dir_to_purge='some/path')) + + def test_file_path_property(self): + self.assertTrue(PurgeManifest(dir_to_purge='some/path').file_path) + + def test_read_from_file(self): + purge_manifest = PurgeManifest(dir_to_purge=self.temp_dir_path) + self.assertIsNone(purge_manifest.read_from_file()) + # write dummy manifest file + purge_manifest.file_path.write_text('[]') + # test + self.assertEqual([], purge_manifest.read_from_file()) + + def test_write_to_file(self): + purge_manifest = PurgeManifest(dir_to_purge=self.temp_dir_path) + manifest = ['some.file'] + purge_manifest.write_to_file(manifest=manifest) + self.assertEqual(manifest, json.loads(purge_manifest.file_path.read_text())) + + def test_purge_no_manifest_file(self): + # prepare + dir_to_purge = self.temp_dir_path + dummy_file_path = dir_to_purge.joinpath('dummy.file') + dummy_file_path.touch() + purge_manifest = PurgeManifest(dir_to_purge=dir_to_purge) + # test + purge_manifest.purge() + self.assertTrue(dummy_file_path.exists()) + + def test_purge(self): + # create dummy files + dir_to_purge = self.temp_dir_path + subdir = dir_to_purge / 'subdir' + subdir.mkdir() + items_to_purge = [dir_to_purge / 'some.dummy', subdir / 'other.dummy', subdir] + items_to_keep = [dir_to_purge / 'file.to.keep'] + for item in items_to_purge + items_to_keep: + if not item.exists(): + item.touch() + # write manifest file manually (when writing the manifest from a .tar.gz + # archive, each dir and each item in that dir is listed, recursively, + # so we also need to include both subdir and the items inside subdir here) + purge_manifest = PurgeManifest(dir_to_purge=dir_to_purge) + manifest = [ + str(item.relative_to(dir_to_purge)) + for item in items_to_purge + [purge_manifest.file_path] + ] + purge_manifest.file_path.write_text(json.dumps(manifest)) + # test + purge_manifest.purge() + for path in items_to_purge: + self.assertFalse(path.exists()) + for path in items_to_keep: + self.assertTrue(path.exists()) + + def test_create_from_archive(self): + # create dummy files, including some hidden + dir_to_purge = self.temp_dir_path + hidden_subdir = dir_to_purge / '.hidden' + hidden_subdir.mkdir() + if ON_WINDOWS: + # https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/attrib + subprocess.run(['attrib', '+h', str(hidden_subdir)], check=True) + dummy_file_in_hidden_subdir = hidden_subdir / 'other.dummy' + dummy_file_in_hidden_subdir.touch() + dummy_file = dir_to_purge / 'some.dummy' + dummy_file.touch() + # create dummy archive + archive_path = self.temp_dir_path / 'dummy_archive.tar.gz' + # todo: should create tar using shutil.make_archive, like in make_gztar_archive, to test handling of "." and "./" + with tarfile.open(archive_path, mode='w:gz') as tar: + for path in [dummy_file, hidden_subdir]: + tar.add( + name=path, arcname=path.relative_to(dir_to_purge), recursive=True + ) + # test + purge_manifest = PurgeManifest(dir_to_purge=dir_to_purge) + purge_manifest.create_from_archive(archive=archive_path) + self.assertTrue(purge_manifest.file_path.exists()) + # load manifest data manually (normally we would use read_from_file) + manifest = json.loads(purge_manifest.file_path.read_text()) + self.assertEqual( + { + str(item.relative_to(dir_to_purge)) + for item in [ + purge_manifest.file_path, + hidden_subdir, + dummy_file, + dummy_file_in_hidden_subdir, + ] + }, + set(manifest), + ) From 39f7d1d573b8ae5cbc829259b1e3852a12ca548c Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Fri, 10 Nov 2023 09:28:34 +0100 Subject: [PATCH 06/25] add test to clarify difference between shutil and tarfile archives --- tests/test_repo.py | 67 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/test_repo.py b/tests/test_repo.py index 4ed6170..3bd605c 100644 --- a/tests/test_repo.py +++ b/tests/test_repo.py @@ -3,6 +3,8 @@ import json import logging import pathlib +import shutil +import tarfile from tempfile import TemporaryDirectory from time import sleep from unittest.mock import Mock, patch @@ -892,3 +894,68 @@ def test_publish_changes(self): # verify change in config config_from_disk = repo.load_config() self.assertIn(config_change, config_from_disk['encrypted_keys']) + + +class ArchivingTests(TempDirTestCase): + def test_equivalence_shutil_vs_tarfile(self): + """ + compares archives created using the high-level shutil.make_archive with those + created using the low-level tarfile approach, to test their equivalence + """ + def make_archive_using_shutil(src_dir, dst_dir, filename): + """ this is a simplified version of repo.make_gztar_archive """ + archive_path = dst_dir / filename + archive_path_str = shutil.make_archive( + base_name=str(archive_path).replace('.tar.gz', ''), # no suffix + root_dir=str(src_dir), # paths in archive will be relative to root_dir + format='gztar', + ) + assert str(archive_path.resolve()) == archive_path_str + return archive_path + + def make_archive_using_tarfile(src_dir, dst_dir, filename): + archive_path = dst_dir / filename + with tarfile.open(archive_path, mode='w:gz') as tar: + for item in src_dir.iterdir(): + tar.add(item, arcname=item.relative_to(src_dir)) + return archive_path + + def list_archive_content(archive_path): + # alternatively we could run `tar -f --list` using subprocess + # but on windows we would need to do that via powershell + with tarfile.open(archive_path) as tar: + content = [item.name for item in tar] + return content + + # create dummy content + dst_dir = self.temp_dir_path + src_dir = self.temp_dir_path / 'source' + src_dir.mkdir() + dummy_file_name = 'dummy.file' + dummy_file = src_dir / dummy_file_name + dummy_file.touch() + # create archive using shutil + shutil_archive = make_archive_using_shutil( + src_dir=src_dir, dst_dir=dst_dir, filename='shutil_archive.tar.gz' + ) + tarfile_archive = make_archive_using_tarfile( + src_dir=src_dir, dst_dir=dst_dir, filename='tarfile_archive.tar.gz' + ) + # make sure the archives were created in the correct location + for archive in [shutil_archive, tarfile_archive]: + self.assertIn(archive, list(self.temp_dir_path.iterdir())) + # list archive content using tarfile (this shows the difference: shutil + # places everything in a subdirectory called ".") + for archive, expected in [ + (shutil_archive, ['.', './' + dummy_file_name]), + (tarfile_archive, [dummy_file_name]), + ]: + self.assertEqual(expected, list_archive_content(archive)) + # verify that the "." dir from shutil does not pose a problem upon extraction + output_dir = self.temp_dir_path / 'output' + output_dir.mkdir() + with tarfile.open(shutil_archive) as tar: + tar.extractall(path=output_dir) + output_dir_items = list(output_dir.iterdir()) + self.assertEqual(1, len(output_dir_items)) + self.assertEqual(dummy_file_name, output_dir_items[0].name) From 1193838c4d984b206236eddaed5dc4136e44bc58 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Fri, 10 Nov 2023 09:32:37 +0100 Subject: [PATCH 07/25] add note --- tests/test_repo.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_repo.py b/tests/test_repo.py index 3bd605c..ed00a69 100644 --- a/tests/test_repo.py +++ b/tests/test_repo.py @@ -901,6 +901,11 @@ def test_equivalence_shutil_vs_tarfile(self): """ compares archives created using the high-level shutil.make_archive with those created using the low-level tarfile approach, to test their equivalence + + the reason is that shutil puts all content inside a "." directory in the + archive, see e.g. [1] + + [1]: https://stackoverflow.com/a/70081512 """ def make_archive_using_shutil(src_dir, dst_dir, filename): """ this is a simplified version of repo.make_gztar_archive """ From b42657159e6ff60bdafefda352ec394527cb5724 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Fri, 10 Nov 2023 12:54:44 +0100 Subject: [PATCH 08/25] fix create_from_archive test on windows --- tests/test_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client.py b/tests/test_client.py index 0a85763..12fc557 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -420,7 +420,7 @@ def test_create_from_archive(self): manifest = json.loads(purge_manifest.file_path.read_text()) self.assertEqual( { - str(item.relative_to(dir_to_purge)) + item.relative_to(dir_to_purge).as_posix() for item in [ purge_manifest.file_path, hidden_subdir, From c94ca891afdcedffb0a10cda47e7959ff85208c3 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Fri, 10 Nov 2023 13:03:24 +0100 Subject: [PATCH 09/25] remove temp assertion from archive test --- tests/test_repo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_repo.py b/tests/test_repo.py index ed00a69..b11face 100644 --- a/tests/test_repo.py +++ b/tests/test_repo.py @@ -915,7 +915,7 @@ def make_archive_using_shutil(src_dir, dst_dir, filename): root_dir=str(src_dir), # paths in archive will be relative to root_dir format='gztar', ) - assert str(archive_path.resolve()) == archive_path_str + self.assertEqual(archive_path.resolve().to_posix(), archive_path_str) return archive_path def make_archive_using_tarfile(src_dir, dst_dir, filename): From 28e1a793800b54493e8ced483e95aff4141a2cc6 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Fri, 10 Nov 2023 13:05:51 +0100 Subject: [PATCH 10/25] fix typo... --- tests/test_repo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_repo.py b/tests/test_repo.py index b11face..1cb99fa 100644 --- a/tests/test_repo.py +++ b/tests/test_repo.py @@ -915,7 +915,7 @@ def make_archive_using_shutil(src_dir, dst_dir, filename): root_dir=str(src_dir), # paths in archive will be relative to root_dir format='gztar', ) - self.assertEqual(archive_path.resolve().to_posix(), archive_path_str) + self.assertEqual(archive_path.resolve().as_posix(), archive_path_str) return archive_path def make_archive_using_tarfile(src_dir, dst_dir, filename): From 263a54044f70e4608ec197822b93daf81c1c1b5b Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Fri, 10 Nov 2023 13:07:53 +0100 Subject: [PATCH 11/25] compare paths instead of strings, to prevent platform-dependent issue --- tests/test_repo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_repo.py b/tests/test_repo.py index 1cb99fa..ef9b477 100644 --- a/tests/test_repo.py +++ b/tests/test_repo.py @@ -915,7 +915,7 @@ def make_archive_using_shutil(src_dir, dst_dir, filename): root_dir=str(src_dir), # paths in archive will be relative to root_dir format='gztar', ) - self.assertEqual(archive_path.resolve().as_posix(), archive_path_str) + self.assertEqual(archive_path.resolve(), pathlib.Path(archive_path_str)) return archive_path def make_archive_using_tarfile(src_dir, dst_dir, filename): From 38428b6729837cd83f3b75b73f26ea0a65add79c Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Fri, 10 Nov 2023 13:20:16 +0100 Subject: [PATCH 12/25] resolve paths to fix alias issue in github actions --- tests/test_repo.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_repo.py b/tests/test_repo.py index ef9b477..fd644ad 100644 --- a/tests/test_repo.py +++ b/tests/test_repo.py @@ -907,15 +907,18 @@ def test_equivalence_shutil_vs_tarfile(self): [1]: https://stackoverflow.com/a/70081512 """ + def make_archive_using_shutil(src_dir, dst_dir, filename): - """ this is a simplified version of repo.make_gztar_archive """ + """this is a simplified version of repo.make_gztar_archive""" archive_path = dst_dir / filename archive_path_str = shutil.make_archive( base_name=str(archive_path).replace('.tar.gz', ''), # no suffix root_dir=str(src_dir), # paths in archive will be relative to root_dir format='gztar', ) - self.assertEqual(archive_path.resolve(), pathlib.Path(archive_path_str)) + self.assertEqual( + archive_path.resolve(), pathlib.Path(archive_path_str).resolve() + ) return archive_path def make_archive_using_tarfile(src_dir, dst_dir, filename): From bdff33b85fb5d0a8edaef0c77aa8f5a2a14a0138 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Mon, 13 Nov 2023 22:05:03 +0100 Subject: [PATCH 13/25] issue56: purge readonly files as well --- src/tufup/client.py | 2 +- src/tufup/utils/__init__.py | 25 ++++++++++++++++++++++--- tests/test_client.py | 8 +++++++- tests/test_utils.py | 25 +++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/tufup/client.py b/src/tufup/client.py index 83df1da..94d507e 100644 --- a/src/tufup/client.py +++ b/src/tufup/client.py @@ -420,7 +420,7 @@ def purge(self) -> None: for path in self.dir_to_purge.iterdir(): relative_path = path.relative_to(self.dir_to_purge) if str(relative_path) in manifest: - remove_path(path=path) + remove_path(path=path, override_readonly=True) else: logger.warning(f'{path} remains: not in {self._filename}') logger.info(f'directory purged: {self.dir_to_purge}') diff --git a/src/tufup/utils/__init__.py b/src/tufup/utils/__init__.py index b63ccd9..9d320c5 100644 --- a/src/tufup/utils/__init__.py +++ b/src/tufup/utils/__init__.py @@ -1,6 +1,8 @@ import logging +import os import pathlib import shutil +import stat import sys from typing import List, Optional, Union @@ -9,7 +11,7 @@ _INPUT_SEPARATOR = ' ' -def remove_path(path: Union[pathlib.Path, str]) -> bool: +def remove_path(path: Union[pathlib.Path, str], override_readonly=False) -> bool: """ Recursively remove directory or file at specified path. @@ -18,14 +20,31 @@ def remove_path(path: Union[pathlib.Path, str]) -> bool: for path in my_dir_path.iterdir(): remove_path(path) """ + def remove_readonly(func, path_, _): + """ + clear the readonly bit and reattempt the removal + + copied from https://docs.python.org/3/library/shutil.html#rmtree-example + """ + os.chmod(path_, stat.S_IWRITE) + func(path_) + # enforce pathlib.Path path = pathlib.Path(path) try: + on_error = remove_readonly if override_readonly else None if path.is_dir(): - shutil.rmtree(path=path) + shutil.rmtree(path=path, onerror=on_error) utils_logger.debug(f'Removed directory {path}') elif path.is_file(): - path.unlink() + try: + path.unlink() + except Exception as e: + if override_readonly: + path.chmod(stat.S_IWRITE) + path.unlink() + else: + raise e utils_logger.debug(f'Removed file {path}') except Exception as e: utils_logger.error(f'Failed to remove {path}: {e}') diff --git a/tests/test_client.py b/tests/test_client.py index 12fc557..0ed7411 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -371,11 +371,17 @@ def test_purge(self): dir_to_purge = self.temp_dir_path subdir = dir_to_purge / 'subdir' subdir.mkdir() - items_to_purge = [dir_to_purge / 'some.dummy', subdir / 'other.dummy', subdir] + readonly_file = dir_to_purge / 'readonly.dummy' + items_to_purge = [ + readonly_file, dir_to_purge / 'some.dummy', subdir / 'other.dummy', subdir + ] items_to_keep = [dir_to_purge / 'file.to.keep'] for item in items_to_purge + items_to_keep: if not item.exists(): item.touch() + # make sure readonly file is readonly + readonly_file.chmod(0o444) # could also use touch(mode=0o444) + self.assertFalse(os.access(readonly_file, os.W_OK)) # write manifest file manually (when writing the manifest from a .tar.gz # archive, each dir and each item in that dir is listed, recursively, # so we also need to include both subdir and the items inside subdir here) diff --git a/tests/test_utils.py b/tests/test_utils.py index 64e73c0..5171eae 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,10 +1,14 @@ +import os import pathlib import unittest from unittest.mock import Mock, patch import tufup.utils +from tufup.utils.platform_specific import ON_WINDOWS from tests import TempDirTestCase +READ_ONLY = 0o444 + class RemovePathTests(TempDirTestCase): def test_remove_path(self): @@ -23,6 +27,27 @@ def test_remove_path(self): self.assertTrue(tufup.utils.remove_path(path=arg_type(dir_path))) self.assertFalse(dir_path.exists()) + def test_remove_path_readonly_file(self): + # prepare + dir_path = self.temp_dir_path / 'dir' + file_path = dir_path / 'dummy.file' + dir_path.mkdir() + file_path.touch(mode=READ_ONLY) + # test + self.assertFalse(tufup.utils.remove_path(dir_path)) + self.assertTrue(tufup.utils.remove_path(dir_path, override_readonly=True)) + + def test_remove_path_readonly_dir(self): + # prepare + dir_path = self.temp_dir_path / 'dir' + file_path = dir_path / 'dummy.file' + dir_path.mkdir(mode=READ_ONLY) + file_path.touch() + # test (windows doesn't really do readonly dirs) + self.assertEqual(ON_WINDOWS, os.access(dir_path, os.W_OK)) + self.assertEqual(ON_WINDOWS, tufup.utils.remove_path(dir_path)) + self.assertTrue(tufup.utils.remove_path(dir_path, override_readonly=True)) + class InputTests(unittest.TestCase): def test_input_bool(self): From 0c9ec5dee7fb075f69295f06ede62d1dc2791b35 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Mon, 13 Nov 2023 22:21:39 +0100 Subject: [PATCH 14/25] issue56: make dir readonly after creating file, in test --- tests/test_utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_utils.py b/tests/test_utils.py index 5171eae..d022abc 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -41,8 +41,9 @@ def test_remove_path_readonly_dir(self): # prepare dir_path = self.temp_dir_path / 'dir' file_path = dir_path / 'dummy.file' - dir_path.mkdir(mode=READ_ONLY) + dir_path.mkdir() file_path.touch() + dir_path.chmod(READ_ONLY) # test (windows doesn't really do readonly dirs) self.assertEqual(ON_WINDOWS, os.access(dir_path, os.W_OK)) self.assertEqual(ON_WINDOWS, tufup.utils.remove_path(dir_path)) From c5e76ceb5a5e12dbd25a129a4b817bc2677913bf Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Mon, 13 Nov 2023 22:37:35 +0100 Subject: [PATCH 15/25] issue56: fix remove_readonly_dir test on windows --- tests/test_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_utils.py b/tests/test_utils.py index d022abc..10fc800 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -46,7 +46,7 @@ def test_remove_path_readonly_dir(self): dir_path.chmod(READ_ONLY) # test (windows doesn't really do readonly dirs) self.assertEqual(ON_WINDOWS, os.access(dir_path, os.W_OK)) - self.assertEqual(ON_WINDOWS, tufup.utils.remove_path(dir_path)) + self.assertFalse(tufup.utils.remove_path(dir_path)) self.assertTrue(tufup.utils.remove_path(dir_path, override_readonly=True)) From b4abdf7a376d48f1ebe0b216dc6a5cb5ed1f36b3 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 16:16:46 +0100 Subject: [PATCH 16/25] clean up remove_path() --- src/tufup/utils/__init__.py | 15 ++++++++------- tests/test_utils.py | 13 +++++-------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/tufup/utils/__init__.py b/src/tufup/utils/__init__.py index 9d320c5..e439ba9 100644 --- a/src/tufup/utils/__init__.py +++ b/src/tufup/utils/__init__.py @@ -11,7 +11,7 @@ _INPUT_SEPARATOR = ' ' -def remove_path(path: Union[pathlib.Path, str], override_readonly=False) -> bool: +def remove_path(path: Union[pathlib.Path, str], remove_readonly=False) -> bool: """ Recursively remove directory or file at specified path. @@ -20,27 +20,28 @@ def remove_path(path: Union[pathlib.Path, str], override_readonly=False) -> bool for path in my_dir_path.iterdir(): remove_path(path) """ - def remove_readonly(func, path_, _): + def _remove_readonly(failed_function, failed_path, _): """ clear the readonly bit and reattempt the removal copied from https://docs.python.org/3/library/shutil.html#rmtree-example """ - os.chmod(path_, stat.S_IWRITE) - func(path_) + os.chmod(failed_path, stat.S_IWRITE) + failed_function(failed_path) # enforce pathlib.Path path = pathlib.Path(path) try: - on_error = remove_readonly if override_readonly else None if path.is_dir(): - shutil.rmtree(path=path, onerror=on_error) + shutil.rmtree( + path=path, onerror=_remove_readonly if remove_readonly else None + ) utils_logger.debug(f'Removed directory {path}') elif path.is_file(): try: path.unlink() except Exception as e: - if override_readonly: + if remove_readonly: path.chmod(stat.S_IWRITE) path.unlink() else: diff --git a/tests/test_utils.py b/tests/test_utils.py index 10fc800..134ad5f 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -7,8 +7,6 @@ from tufup.utils.platform_specific import ON_WINDOWS from tests import TempDirTestCase -READ_ONLY = 0o444 - class RemovePathTests(TempDirTestCase): def test_remove_path(self): @@ -32,10 +30,10 @@ def test_remove_path_readonly_file(self): dir_path = self.temp_dir_path / 'dir' file_path = dir_path / 'dummy.file' dir_path.mkdir() - file_path.touch(mode=READ_ONLY) + file_path.touch(mode=0o444) # test - self.assertFalse(tufup.utils.remove_path(dir_path)) - self.assertTrue(tufup.utils.remove_path(dir_path, override_readonly=True)) + self.assertEqual(not ON_WINDOWS, tufup.utils.remove_path(dir_path)) + self.assertTrue(tufup.utils.remove_path(dir_path, remove_readonly=True)) def test_remove_path_readonly_dir(self): # prepare @@ -43,11 +41,10 @@ def test_remove_path_readonly_dir(self): file_path = dir_path / 'dummy.file' dir_path.mkdir() file_path.touch() - dir_path.chmod(READ_ONLY) + dir_path.chmod(0o555) # test (windows doesn't really do readonly dirs) - self.assertEqual(ON_WINDOWS, os.access(dir_path, os.W_OK)) self.assertFalse(tufup.utils.remove_path(dir_path)) - self.assertTrue(tufup.utils.remove_path(dir_path, override_readonly=True)) + self.assertTrue(tufup.utils.remove_path(dir_path, remove_readonly=True)) class InputTests(unittest.TestCase): From 8f1c649c9ca658dacc2a4811fac8df0183cb4372 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 17:21:09 +0100 Subject: [PATCH 17/25] crude override of readonly paths in remove_path and fix tests for linux/windows --- src/tufup/utils/__init__.py | 22 +++++----------------- tests/test_utils.py | 37 ++++++++++++++++++++++++++++--------- 2 files changed, 33 insertions(+), 26 deletions(-) diff --git a/src/tufup/utils/__init__.py b/src/tufup/utils/__init__.py index e439ba9..1bc63b4 100644 --- a/src/tufup/utils/__init__.py +++ b/src/tufup/utils/__init__.py @@ -20,32 +20,20 @@ def remove_path(path: Union[pathlib.Path, str], remove_readonly=False) -> bool: for path in my_dir_path.iterdir(): remove_path(path) """ - def _remove_readonly(failed_function, failed_path, _): - """ - clear the readonly bit and reattempt the removal - - copied from https://docs.python.org/3/library/shutil.html#rmtree-example - """ - os.chmod(failed_path, stat.S_IWRITE) - failed_function(failed_path) # enforce pathlib.Path path = pathlib.Path(path) try: + if remove_readonly: + # override any read-only permissions + path.chmod(0o777) if path.is_dir(): shutil.rmtree( - path=path, onerror=_remove_readonly if remove_readonly else None + path=path ) utils_logger.debug(f'Removed directory {path}') elif path.is_file(): - try: - path.unlink() - except Exception as e: - if remove_readonly: - path.chmod(stat.S_IWRITE) - path.unlink() - else: - raise e + path.unlink() utils_logger.debug(f'Removed file {path}') except Exception as e: utils_logger.error(f'Failed to remove {path}: {e}') diff --git a/tests/test_utils.py b/tests/test_utils.py index 134ad5f..842f3b2 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -25,24 +25,43 @@ def test_remove_path(self): self.assertTrue(tufup.utils.remove_path(path=arg_type(dir_path))) self.assertFalse(dir_path.exists()) - def test_remove_path_readonly_file(self): - # prepare + def test_remove_dir_for_readonly_file(self): + """ + on linux: a readonly file does not prevent deletion of the file, nor does it + prevent deletion of the parent directory + + on windows: a readonly file prevents both file deletion and deletion of the + parent directory + """ + # prepare readonly file dir_path = self.temp_dir_path / 'dir' - file_path = dir_path / 'dummy.file' + file_path = dir_path / 'readonly.file' dir_path.mkdir() file_path.touch(mode=0o444) # test self.assertEqual(not ON_WINDOWS, tufup.utils.remove_path(dir_path)) - self.assertTrue(tufup.utils.remove_path(dir_path, remove_readonly=True)) + if ON_WINDOWS: + self.assertTrue(tufup.utils.remove_path(dir_path, remove_readonly=True)) - def test_remove_path_readonly_dir(self): - # prepare - dir_path = self.temp_dir_path / 'dir' + + def test_remove_dir_for_readonly_dir(self): + """ + on linux: a readonly directory prevents both file deletion and deletion of + the directory itself + + on windows: a "readonly" directory does *not* prevent file deletion, but it + does prevent deletion of the directory, even though some sources suggest "the + Read-only attribute for a folder is typically ignored by Windows" [1] + + [1]: https://support.microsoft.com/en-gb/topic/you-cannot-view-or-change-the-read-only-or-the-system-attributes-of-folders-in-windows-server-2003-in-windows-xp-in-windows-vista-or-in-windows-7-55bd5ec5-d19e-6173-0df1-8f5b49247165 + """ + # prepare readonly directory + dir_path = self.temp_dir_path / 'readonly_dir' file_path = dir_path / 'dummy.file' dir_path.mkdir() file_path.touch() - dir_path.chmod(0o555) - # test (windows doesn't really do readonly dirs) + dir_path.chmod(0o555) # dir must have execution permission + # test self.assertFalse(tufup.utils.remove_path(dir_path)) self.assertTrue(tufup.utils.remove_path(dir_path, remove_readonly=True)) From 7be4973572b01de728b2488d63055a830f03ba9b Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 17:39:46 +0100 Subject: [PATCH 18/25] test file and dir removal separately for remove_path --- src/tufup/utils/__init__.py | 9 +++---- tests/test_utils.py | 54 +++++++++++++++++++++++++++++-------- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/src/tufup/utils/__init__.py b/src/tufup/utils/__init__.py index 1bc63b4..7144013 100644 --- a/src/tufup/utils/__init__.py +++ b/src/tufup/utils/__init__.py @@ -20,17 +20,16 @@ def remove_path(path: Union[pathlib.Path, str], remove_readonly=False) -> bool: for path in my_dir_path.iterdir(): remove_path(path) """ - # enforce pathlib.Path path = pathlib.Path(path) try: if remove_readonly: - # override any read-only permissions + # override any read-only permissions (note this will fail when dealing + # with a file in a readonly directory on linux, but the alternative would + # be to (temporarily) change permissions on the parent directory) path.chmod(0o777) if path.is_dir(): - shutil.rmtree( - path=path - ) + shutil.rmtree(path=path) utils_logger.debug(f'Removed directory {path}') elif path.is_file(): path.unlink() diff --git a/tests/test_utils.py b/tests/test_utils.py index 842f3b2..84f56d1 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -25,13 +25,11 @@ def test_remove_path(self): self.assertTrue(tufup.utils.remove_path(path=arg_type(dir_path))) self.assertFalse(dir_path.exists()) - def test_remove_dir_for_readonly_file(self): + def test_remove_dir_containing_readonly_file(self): """ - on linux: a readonly file does not prevent deletion of the file, nor does it - prevent deletion of the parent directory + on linux: a readonly file does not prevent deletion of the parent directory - on windows: a readonly file prevents both file deletion and deletion of the - parent directory + on windows: a readonly file prevents deletion of the parent directory """ # prepare readonly file dir_path = self.temp_dir_path / 'dir' @@ -43,15 +41,30 @@ def test_remove_dir_for_readonly_file(self): if ON_WINDOWS: self.assertTrue(tufup.utils.remove_path(dir_path, remove_readonly=True)) + def test_remove_readonly_file(self): + """ + on linux: a readonly file does not prevent deletion of the file + + on windows: a readonly file prevents file deletion + """ + # prepare readonly file + dir_path = self.temp_dir_path / 'dir' + file_path = dir_path / 'readonly.file' + dir_path.mkdir() + file_path.touch(mode=0o444) + # test + self.assertEqual(not ON_WINDOWS, tufup.utils.remove_path(file_path)) + if ON_WINDOWS: + self.assertTrue(tufup.utils.remove_path(file_path, remove_readonly=True)) + - def test_remove_dir_for_readonly_dir(self): + def test_remove_readonly_dir(self): """ - on linux: a readonly directory prevents both file deletion and deletion of - the directory itself + on linux: a readonly directory prevents deletion of the directory itself - on windows: a "readonly" directory does *not* prevent file deletion, but it - does prevent deletion of the directory, even though some sources suggest "the - Read-only attribute for a folder is typically ignored by Windows" [1] + on windows: a "readonly" directory prevents deletion of the directory, + even though some sources suggest "the Read-only attribute for a folder is + typically ignored by Windows" [1] [1]: https://support.microsoft.com/en-gb/topic/you-cannot-view-or-change-the-read-only-or-the-system-attributes-of-folders-in-windows-server-2003-in-windows-xp-in-windows-vista-or-in-windows-7-55bd5ec5-d19e-6173-0df1-8f5b49247165 """ @@ -65,6 +78,25 @@ def test_remove_dir_for_readonly_dir(self): self.assertFalse(tufup.utils.remove_path(dir_path)) self.assertTrue(tufup.utils.remove_path(dir_path, remove_readonly=True)) + def test_remove_file_from_readonly_dir(self): + """ + on linux: a readonly directory prevents deletion of the files in the directory + + on windows: a readonly directory does not prevent file deletion + """ + # prepare readonly directory + dir_path = self.temp_dir_path / 'readonly_dir' + file_path = dir_path / 'dummy.file' + dir_path.mkdir() + file_path.touch() + dir_path.chmod(0o555) # dir must have execution permission + # test + self.assertEqual(ON_WINDOWS, tufup.utils.remove_path(file_path)) + if not ON_WINDOWS: + # for this to work, remove_path would have to change permissions on the + # file's parent directory, which could lead to unwelcome surprises... + self.assertFalse(tufup.utils.remove_path(file_path, remove_readonly=True)) + class InputTests(unittest.TestCase): def test_input_bool(self): From 7d4c526ed2dd066f9787fb4efca1ab2b30909df8 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 17:40:57 +0100 Subject: [PATCH 19/25] fix arguments --- src/tufup/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tufup/client.py b/src/tufup/client.py index 94d507e..eda4b62 100644 --- a/src/tufup/client.py +++ b/src/tufup/client.py @@ -420,7 +420,7 @@ def purge(self) -> None: for path in self.dir_to_purge.iterdir(): relative_path = path.relative_to(self.dir_to_purge) if str(relative_path) in manifest: - remove_path(path=path, override_readonly=True) + remove_path(path=path, remove_readonly=True) else: logger.warning(f'{path} remains: not in {self._filename}') logger.info(f'directory purged: {self.dir_to_purge}') From 20ffbe9f2801566a4e25f2065ffd3a6c6d43d34c Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 17:41:23 +0100 Subject: [PATCH 20/25] ruff format --- tests/test_client.py | 5 ++++- tests/test_utils.py | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_client.py b/tests/test_client.py index 0ed7411..73fea3f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -373,7 +373,10 @@ def test_purge(self): subdir.mkdir() readonly_file = dir_to_purge / 'readonly.dummy' items_to_purge = [ - readonly_file, dir_to_purge / 'some.dummy', subdir / 'other.dummy', subdir + readonly_file, + dir_to_purge / 'some.dummy', + subdir / 'other.dummy', + subdir, ] items_to_keep = [dir_to_purge / 'file.to.keep'] for item in items_to_purge + items_to_keep: diff --git a/tests/test_utils.py b/tests/test_utils.py index 84f56d1..18fbb72 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -57,7 +57,6 @@ def test_remove_readonly_file(self): if ON_WINDOWS: self.assertTrue(tufup.utils.remove_path(file_path, remove_readonly=True)) - def test_remove_readonly_dir(self): """ on linux: a readonly directory prevents deletion of the directory itself From 392ffc1b27a9af3bd03e8df788350e9a4de207b3 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 18:12:01 +0100 Subject: [PATCH 21/25] issue56: fix case remove_dir_containing_readonly_file on windows --- src/tufup/utils/__init__.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/tufup/utils/__init__.py b/src/tufup/utils/__init__.py index 7144013..60bcec0 100644 --- a/src/tufup/utils/__init__.py +++ b/src/tufup/utils/__init__.py @@ -20,6 +20,15 @@ def remove_path(path: Union[pathlib.Path, str], remove_readonly=False) -> bool: for path in my_dir_path.iterdir(): remove_path(path) """ + def _remove_readonly(failed_function, failed_path, _): + """ + clear the readonly bit and reattempt the removal + + copied from https://docs.python.org/3/library/shutil.html#rmtree-example + """ + os.chmod(failed_path, 0o777) + failed_function(failed_path) + # enforce pathlib.Path path = pathlib.Path(path) try: @@ -29,7 +38,9 @@ def remove_path(path: Union[pathlib.Path, str], remove_readonly=False) -> bool: # be to (temporarily) change permissions on the parent directory) path.chmod(0o777) if path.is_dir(): - shutil.rmtree(path=path) + shutil.rmtree( + path=path, onerror=_remove_readonly if remove_readonly else None + ) utils_logger.debug(f'Removed directory {path}') elif path.is_file(): path.unlink() From f2ed21eb32a861a53afdd6aeef2fe600b4e3fbfa Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 19:16:02 +0100 Subject: [PATCH 22/25] remove unused imports --- src/tufup/utils/__init__.py | 2 -- tests/test_utils.py | 1 - 2 files changed, 3 deletions(-) diff --git a/src/tufup/utils/__init__.py b/src/tufup/utils/__init__.py index 60bcec0..6a94055 100644 --- a/src/tufup/utils/__init__.py +++ b/src/tufup/utils/__init__.py @@ -1,8 +1,6 @@ import logging -import os import pathlib import shutil -import stat import sys from typing import List, Optional, Union diff --git a/tests/test_utils.py b/tests/test_utils.py index 18fbb72..6fef185 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,4 +1,3 @@ -import os import pathlib import unittest from unittest.mock import Mock, patch From cc793f1f962f499a44c5d354d08c7022431edf74 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Tue, 14 Nov 2023 19:19:56 +0100 Subject: [PATCH 23/25] ... and import os again it's getting late... --- src/tufup/utils/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tufup/utils/__init__.py b/src/tufup/utils/__init__.py index 6a94055..10c0437 100644 --- a/src/tufup/utils/__init__.py +++ b/src/tufup/utils/__init__.py @@ -1,4 +1,5 @@ import logging +import os import pathlib import shutil import sys From 34f7e78417d0b203b563d41f42ed7c7fa4bed062 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Wed, 15 Nov 2023 10:22:47 +0100 Subject: [PATCH 24/25] make sure extract_dir purge is actually optional --- src/tufup/client.py | 3 ++- tests/test_client.py | 25 +++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/tufup/client.py b/src/tufup/client.py index eda4b62..3de539a 100644 --- a/src/tufup/client.py +++ b/src/tufup/client.py @@ -287,7 +287,8 @@ def _extract_archive(self): self.extract_dir.mkdir(exist_ok=True) # purge extract_dir purge_manifest = PurgeManifest(dir_to_purge=self.extract_dir) - purge_manifest.purge() + if self.purge_extract_dir: + purge_manifest.purge() # extract the new archive into the extract_dir shutil.unpack_archive( filename=self.new_archive_local_path, extract_dir=self.extract_dir diff --git a/tests/test_client.py b/tests/test_client.py index 73fea3f..b5a7c38 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -14,7 +14,8 @@ from tuf.ngclient import TargetFile from tests import TempDirTestCase, TEST_REPO_DIR -from tufup.client import AuthRequestsFetcher, Client, PurgeManifest +import tufup.client # for patch +from tufup.client import AuthRequestsFetcher, Client, EXTRACT_DIR_NAME, PurgeManifest from tufup.common import TargetMeta from tufup.utils.platform_specific import ON_WINDOWS @@ -221,7 +222,7 @@ def test__apply_updates(self): mock_install.assert_called() def test__extract_archive_with_purge(self): - temp_extract_dir = self.temp_dir_path / 'tufup-extract-dir' + temp_extract_dir = self.temp_dir_path / EXTRACT_DIR_NAME temp_extract_dir.mkdir() self.client_kwargs['extract_dir'] = temp_extract_dir self.client_kwargs['purge_extract_dir'] = True @@ -238,6 +239,26 @@ def test__extract_archive_with_purge(self): purge_manifest = PurgeManifest(dir_to_purge=client.extract_dir) self.assertTrue(purge_manifest.file_path.exists()) + def test__extract_archive_without_purge(self): + temp_extract_dir = self.temp_dir_path / EXTRACT_DIR_NAME + temp_extract_dir.mkdir() + self.client_kwargs['extract_dir'] = temp_extract_dir + self.client_kwargs['purge_extract_dir'] = False + client = self.populate_refreshed_client(client=self.get_refreshed_client()) + # ensure new archive exists in targets dir(dummy) + shutil.copy( + src=TEST_REPO_DIR / 'targets' / client.new_archive_local_path.name, + dst=client.new_archive_local_path, + ) + # test extract + with patch.object(tufup.client.PurgeManifest, 'purge') as mock_purge: + client._extract_archive() + # purge must not be called + self.assertFalse(mock_purge.called) + # a purge manifest file should now exist + purge_manifest = PurgeManifest(dir_to_purge=client.extract_dir) + self.assertTrue(purge_manifest.file_path.exists()) + class AuthRequestsFetcherTests(unittest.TestCase): def setUp(self) -> None: From b378ff58e9f8682d9a1f9687e4320318eeae0db5 Mon Sep 17 00:00:00 2001 From: dennisvang <29799340+dennisvang@users.noreply.github.com> Date: Wed, 15 Nov 2023 15:35:32 +0100 Subject: [PATCH 25/25] add .ruff_cache folder to gitignore, for clarity --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 2333e55..49b3b87 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,7 @@ examples/client/cache/ examples/client/programs/ examples/repo/keystore/ examples/repo/repository/ +.ruff_cache/ # files .tufup-repo-config