diff --git a/supervisor/api/backups.py b/supervisor/api/backups.py index 8abda03c4..e4d7c51e8 100644 --- a/supervisor/api/backups.py +++ b/supervisor/api/backups.py @@ -457,7 +457,7 @@ class APIBackups(CoreSysAttributes): else: self._validate_cloud_backup_location(request, backup.location) - return self.sys_backups.remove(backup, locations=locations) + self.sys_backups.remove(backup, locations=locations) @api_process async def download(self, request: web.Request): diff --git a/supervisor/api/utils.py b/supervisor/api/utils.py index 4bfe4af3e..1dc4b2781 100644 --- a/supervisor/api/utils.py +++ b/supervisor/api/utils.py @@ -22,7 +22,7 @@ from ..const import ( RESULT_OK, ) from ..coresys import CoreSys -from ..exceptions import APIError, APIForbidden, DockerAPIError, HassioError +from ..exceptions import APIError, BackupFileNotFoundError, DockerAPIError, HassioError from ..utils import check_exception_chain, get_message_from_exception_chain from ..utils.json import json_dumps, json_loads as json_loads_util from ..utils.log_format import format_message @@ -62,8 +62,12 @@ def api_process(method): """Return API information.""" try: answer = await method(api, *args, **kwargs) - except (APIError, APIForbidden, HassioError) as err: - return api_return_error(error=err) + except BackupFileNotFoundError as err: + return api_return_error(err, status=404) + except APIError as err: + return api_return_error(err, status=err.status, job_id=err.job_id) + except HassioError as err: + return api_return_error(err) if isinstance(answer, (dict, list)): return api_return_ok(data=answer) @@ -102,6 +106,13 @@ def api_process_raw(content, *, error_type=None): """Return api information.""" try: msg_data = await method(api, *args, **kwargs) + except APIError as err: + return api_return_error( + err, + error_type=error_type or const.CONTENT_TYPE_BINARY, + status=err.status, + job_id=err.job_id, + ) except HassioError as err: return api_return_error( err, error_type=error_type or const.CONTENT_TYPE_BINARY @@ -121,6 +132,8 @@ def api_return_error( error: Exception | None = None, message: str | None = None, error_type: str | None = None, + status: int = 400, + job_id: str | None = None, ) -> web.Response: """Return an API error message.""" if error and not message: @@ -130,10 +143,6 @@ def api_return_error( if not message: message = "Unknown error, see supervisor" - status = 400 - if is_api_error := isinstance(error, APIError): - status = error.status - match error_type: case const.CONTENT_TYPE_TEXT: return web.Response(body=message, content_type=error_type, status=status) @@ -146,8 +155,8 @@ def api_return_error( JSON_RESULT: RESULT_ERROR, JSON_MESSAGE: message, } - if is_api_error and error.job_id: - result[JSON_JOB_ID] = error.job_id + if job_id: + result[JSON_JOB_ID] = job_id return web.json_response( result, diff --git a/supervisor/backups/backup.py b/supervisor/backups/backup.py index 4e6fc6eb2..334bd6906 100644 --- a/supervisor/backups/backup.py +++ b/supervisor/backups/backup.py @@ -52,7 +52,12 @@ from ..const import ( CRYPTO_AES128, ) from ..coresys import CoreSys -from ..exceptions import AddonsError, BackupError, BackupInvalidError +from ..exceptions import ( + AddonsError, + BackupError, + BackupFileNotFoundError, + BackupInvalidError, +) from ..jobs.const import JOB_GROUP_BACKUP from ..jobs.decorator import Job from ..jobs.job_group import JobGroup @@ -513,7 +518,7 @@ class Backup(JobGroup): else self.all_locations[location][ATTR_PATH] ) if not backup_tarfile.is_file(): - raise BackupError( + raise BackupFileNotFoundError( f"Cannot open backup at {backup_tarfile.as_posix()}, file does not exist!", _LOGGER.error, ) diff --git a/supervisor/backups/manager.py b/supervisor/backups/manager.py index fd95bcd05..c581cc061 100644 --- a/supervisor/backups/manager.py +++ b/supervisor/backups/manager.py @@ -22,6 +22,7 @@ from ..dbus.const import UnitActiveState from ..exceptions import ( BackupDataDiskBadMessageError, BackupError, + BackupFileNotFoundError, BackupInvalidError, BackupJobError, BackupMountDownError, @@ -279,7 +280,7 @@ class BackupManager(FileConfiguration, JobGroup): self, backup: Backup, locations: list[LOCATION_TYPE] | None = None, - ) -> bool: + ): """Remove a backup.""" targets = ( [ @@ -292,24 +293,28 @@ class BackupManager(FileConfiguration, JobGroup): else list(backup.all_locations.keys()) ) for location in targets: + backup_tarfile = backup.all_locations[location][ATTR_PATH] try: - backup.all_locations[location][ATTR_PATH].unlink() + backup_tarfile.unlink() del backup.all_locations[location] + except FileNotFoundError as err: + raise BackupFileNotFoundError( + f"Cannot delete backup at {backup_tarfile.as_posix()}, file does not exist!", + _LOGGER.error, + ) from err except OSError as err: + msg = f"Could delete backup at {backup_tarfile.as_posix()}: {err!s}" if err.errno == errno.EBADMSG and location in { None, LOCATION_CLOUD_BACKUP, }: self.sys_resolution.unhealthy = UnhealthyReason.OSERROR_BAD_MESSAGE - _LOGGER.error("Can't remove backup %s: %s", backup.slug, err) - return False + raise BackupError(msg, _LOGGER.error) from err # If backup has been removed from all locations, remove it from cache if not backup.all_locations: del self._backups[backup.slug] - return True - async def _copy_to_additional_locations( self, backup: Backup, diff --git a/supervisor/exceptions.py b/supervisor/exceptions.py index 550bf26d0..f5a851f28 100644 --- a/supervisor/exceptions.py +++ b/supervisor/exceptions.py @@ -659,6 +659,10 @@ class BackupJobError(BackupError, JobException): """Raise on Backup job error.""" +class BackupFileNotFoundError(BackupError): + """Raise if the backup file hasn't been found.""" + + # Security diff --git a/supervisor/misc/tasks.py b/supervisor/misc/tasks.py index c87416389..37af8a9de 100644 --- a/supervisor/misc/tasks.py +++ b/supervisor/misc/tasks.py @@ -9,7 +9,12 @@ from ..addons.const import ADDON_UPDATE_CONDITIONS from ..backups.const import LOCATION_CLOUD_BACKUP from ..const import AddonState from ..coresys import CoreSysAttributes -from ..exceptions import AddonsError, HomeAssistantError, ObserverError +from ..exceptions import ( + AddonsError, + BackupFileNotFoundError, + HomeAssistantError, + ObserverError, +) from ..homeassistant.const import LANDINGPAGE from ..jobs.decorator import Job, JobCondition, JobExecutionLimit from ..plugins.const import PLUGIN_UPDATE_CONDITIONS @@ -364,4 +369,7 @@ class Tasks(CoreSysAttributes): and datetime.fromisoformat(backup.date) < utcnow() - OLD_BACKUP_THRESHOLD ] for backup in old_backups: - self.sys_backups.remove(backup, [LOCATION_CLOUD_BACKUP]) + try: + self.sys_backups.remove(backup, [LOCATION_CLOUD_BACKUP]) + except BackupFileNotFoundError as err: + _LOGGER.debug("Can't remove backup %s: %s", backup.slug, err) diff --git a/supervisor/resolution/fixups/system_clear_full_backup.py b/supervisor/resolution/fixups/system_clear_full_backup.py index 3515a2a58..6d09989de 100644 --- a/supervisor/resolution/fixups/system_clear_full_backup.py +++ b/supervisor/resolution/fixups/system_clear_full_backup.py @@ -2,6 +2,8 @@ import logging +from supervisor.exceptions import BackupFileNotFoundError + from ...backups.const import BackupType from ...coresys import CoreSys from ..const import MINIMUM_FULL_BACKUPS, ContextType, IssueType, SuggestionType @@ -31,7 +33,10 @@ class FixupSystemClearFullBackup(FixupBase): for backup in sorted(full_backups, key=lambda x: x.date)[ : -1 * MINIMUM_FULL_BACKUPS ]: - self.sys_backups.remove(backup) + try: + self.sys_backups.remove(backup) + except BackupFileNotFoundError as err: + _LOGGER.debug("Can't remove backup %s: %s", backup.slug, err) @property def suggestion(self) -> SuggestionType: diff --git a/tests/api/test_backups.py b/tests/api/test_backups.py index f3a17e4ab..9a9021dc9 100644 --- a/tests/api/test_backups.py +++ b/tests/api/test_backups.py @@ -503,6 +503,13 @@ async def test_restore_immediate_errors( assert resp.status == 400 assert "No Home Assistant" in (await resp.json())["message"] + resp = await api_client.post( + f"/backups/{mock_partial_backup.slug}/restore/partial", + json={"background": True, "folders": ["ssl"]}, + ) + assert resp.status == 404 + assert "file does not exist" in (await resp.json())["message"] + @pytest.mark.parametrize( ("folder", "location"), [("backup", None), ("core/backup", ".cloud_backup")] @@ -808,6 +815,28 @@ async def test_remove_backup_from_location(api_client: TestClient, coresys: Core assert backup.all_locations == {None: {"path": location_1, "protected": False}} +@pytest.mark.usefixtures("tmp_supervisor_data") +async def test_remove_backup_file_not_found(api_client: TestClient, coresys: CoreSys): + """Test removing a backup from one location of multiple.""" + backup_file = get_fixture_path("backup_example.tar") + location = Path(copy(backup_file, coresys.config.path_backup)) + + await coresys.backups.reload() + assert (backup := coresys.backups.get("7fed74c8")) + assert backup.all_locations == { + None: {"path": location, "protected": False}, + } + + location.unlink() + resp = await api_client.delete("/backups/7fed74c8") + assert resp.status == 404 + body = await resp.json() + assert ( + body["message"] + == f"Cannot delete backup at {str(location)}, file does not exist!" + ) + + @pytest.mark.parametrize("local_location", ["", ".local"]) async def test_download_backup_from_location( api_client: TestClient, @@ -909,7 +938,7 @@ async def test_restore_backup_from_location( f"/backups/{backup.slug}/restore/partial", json={"location": local_location, "folders": ["share"]}, ) - assert resp.status == 400 + assert resp.status == 404 body = await resp.json() assert ( body["message"] diff --git a/tests/backups/test_manager.py b/tests/backups/test_manager.py index cbba2747d..b36fc2ecc 100644 --- a/tests/backups/test_manager.py +++ b/tests/backups/test_manager.py @@ -25,6 +25,7 @@ from supervisor.docker.homeassistant import DockerHomeAssistant from supervisor.docker.monitor import DockerContainerStateEvent from supervisor.exceptions import ( BackupError, + BackupFileNotFoundError, BackupInvalidError, BackupJobError, BackupMountDownError, @@ -1769,11 +1770,13 @@ async def test_backup_remove_error( tar_file_mock.unlink.side_effect = (err := OSError()) err.errno = errno.EBUSY - assert coresys.backups.remove(backup) is False + with pytest.raises(BackupError): + coresys.backups.remove(backup) assert coresys.core.healthy is True err.errno = errno.EBADMSG - assert coresys.backups.remove(backup) is False + with pytest.raises(BackupError): + coresys.backups.remove(backup) assert coresys.core.healthy is healthy_expected @@ -2072,3 +2075,25 @@ async def test_addon_backup_excludes(coresys: CoreSys, install_addon_example: Ad assert not test2.exists() assert test_dir.is_dir() assert test3.exists() + + +@pytest.mark.usefixtures("tmp_supervisor_data", "path_extern") +async def test_remove_non_existing_backup_raises( + coresys: CoreSys, +): + """Test removing a backup error.""" + location: LOCATION_TYPE = None + backup_base_path = coresys.backups._get_base_path(location) # pylint: disable=protected-access + backup_base_path.mkdir(exist_ok=True) + copy(get_fixture_path("backup_example.tar"), backup_base_path) + + await coresys.backups.reload(location=location, filename="backup_example.tar") + assert (backup := coresys.backups.get("7fed74c8")) + + assert None in backup.all_locations + backup.all_locations[None]["path"] = (tar_file_mock := MagicMock()) + tar_file_mock.unlink.side_effect = (err := FileNotFoundError()) + err.errno = errno.ENOENT + + with pytest.raises(BackupFileNotFoundError): + coresys.backups.remove(backup)