From 3cb678ad3274620de549ec16f2b8759f59a8291c Mon Sep 17 00:00:00 2001 From: Ilija Lazoroski Date: Mon, 20 Jun 2022 14:55:26 +0200 Subject: [PATCH] Island: Raise distinct errors when openning a file IFileRepository now distincts between file not found and a file that could not be retrieved --- .../repository/file_storage/i_file_repository.py | 3 ++- .../file_storage/local_storage_file_repository.py | 12 +++++++----- .../test_local_storage_file_repository.py | 14 +++++++++++++- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py index 2531fd711..5069b36d7 100644 --- a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py @@ -30,7 +30,8 @@ class IFileRepository(metaclass=abc.ABCMeta): :param unsafe_file_name: An unsanitized file name that identifies the file to be opened :return: A file-like object providing access to the file's contents - :raises FileRetrievalError: if the file cannot be opened + :raises FileRetrievalError: if the file cannot be retrieved + :raises FileNotFoundError: if the file cannot be found """ pass diff --git a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py index 4202496ad..a23b25228 100644 --- a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py @@ -1,3 +1,4 @@ +import errno import logging import shutil from pathlib import Path @@ -44,11 +45,12 @@ class LocalStorageFileRepository(IFileRepository): logger.debug(f"Opening {safe_file_path}") return open(safe_file_path, "rb") except OSError as err: - # TODO: The interface should make a destinction between file not found and an error when - # retrieving a file that should exist. The built-in `FileNotFoundError` is not - # sufficient because it inherits from `OSError` and the interface does not - # guarantee that the file is stored on the local file system. - raise FileRetrievalError(f"Failed to retrieve file {safe_file_path}: {err}") from err + if err.errno == errno.ENOENT: + raise FileNotFoundError(f"No file {safe_file_path} found: {err}") from err + else: + raise FileRetrievalError( + f"Failed to retrieve file {safe_file_path}: {err}" + ) from err def delete_file(self, unsafe_file_name: str): safe_file_path = self._get_safe_file_path(unsafe_file_name) diff --git a/monkey/tests/unit_tests/monkey_island/cc/repository/test_local_storage_file_repository.py b/monkey/tests/unit_tests/monkey_island/cc/repository/test_local_storage_file_repository.py index cf506ffd0..804b03c79 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/repository/test_local_storage_file_repository.py +++ b/monkey/tests/unit_tests/monkey_island/cc/repository/test_local_storage_file_repository.py @@ -1,5 +1,7 @@ +import errno import io from pathlib import Path +from unittest.mock import Mock, patch import pytest from tests.monkey_island.utils import assert_linux_permissions, assert_windows_permissions @@ -131,5 +133,15 @@ def test_remove_nonexistant_file(tmp_path): def test_open_nonexistant_file(tmp_path): fss = LocalStorageFileRepository(tmp_path) - with pytest.raises(FileRetrievalError): + with pytest.raises(FileNotFoundError): fss.open_file("nonexistant_file.txt") + + +def test_open_locked_file(tmp_path, monkeypatch): + fss = LocalStorageFileRepository(tmp_path) + + with patch( + "builtins.open", Mock(side_effect=OSError(errno.EIO, "File could not be retrieved")) + ): + with pytest.raises(FileRetrievalError): + fss.open_file("locked_file.txt")