diff --git a/supervisor/backups/backup.py b/supervisor/backups/backup.py index 2e379df6e..49d67a845 100644 --- a/supervisor/backups/backup.py +++ b/supervisor/backups/backup.py @@ -366,14 +366,14 @@ class Backup(JobGroup): backend=default_backend(), ) - async def validate_password(self, location: str | None) -> bool: - """Validate backup password. + async def validate_backup(self, location: str | None) -> None: + """Validate backup. - Returns false only when the password is known to be wrong. + Checks if we can access the backup file and decrypt if necessary. """ backup_file: Path = self.all_locations[location][ATTR_PATH] - def _validate_file() -> bool: + def _validate_file() -> None: ending = f".tar{'.gz' if self.compressed else ''}" with tarfile.open(backup_file, "r:") as backup: @@ -386,8 +386,8 @@ class Backup(JobGroup): None, ) if not test_tar_name: - _LOGGER.warning("No tar file found to validate password with") - return True + # From Supervisor perspective, a metadata only backup only is valid. + return test_tar_file = backup.extractfile(test_tar_name) try: @@ -399,16 +399,14 @@ class Backup(JobGroup): fileobj=test_tar_file, ): # If we can read the tar file, the password is correct - return True - except tarfile.ReadError: - _LOGGER.debug("Invalid password") - return False - except Exception: # pylint: disable=broad-exception-caught - _LOGGER.exception("Unexpected error validating password") - return True + return + except tarfile.ReadError as ex: + raise BackupInvalidError( + f"Invalid password for backup {backup.slug}", _LOGGER.error + ) from ex try: - return await self.sys_run_in_executor(_validate_file) + await self.sys_run_in_executor(_validate_file) except FileNotFoundError as err: self.sys_create_task(self.sys_backups.reload(location)) raise BackupFileNotFoundError( diff --git a/supervisor/backups/manager.py b/supervisor/backups/manager.py index 87e7b2de5..14f45dcc2 100644 --- a/supervisor/backups/manager.py +++ b/supervisor/backups/manager.py @@ -717,7 +717,7 @@ class BackupManager(FileConfiguration, JobGroup): _job_override__cleanup=False ) - async def _set_location_password( + async def _validate_backup_location( self, backup: Backup, password: str | None = None, @@ -732,13 +732,11 @@ class BackupManager(FileConfiguration, JobGroup): location = location if location != DEFAULT else backup.location if backup.all_locations[location][ATTR_PROTECTED]: backup.set_password(password) - if not await backup.validate_password(location): - raise BackupInvalidError( - f"Invalid password for backup {backup.slug}", _LOGGER.error - ) else: backup.set_password(None) + await backup.validate_backup(location) + @Job( name=JOB_FULL_RESTORE, conditions=[ @@ -767,7 +765,7 @@ class BackupManager(FileConfiguration, JobGroup): f"{backup.slug} is only a partial backup!", _LOGGER.error ) - await self._set_location_password(backup, password, location) + await self._validate_backup_location(backup, password, location) if backup.supervisor_version > self.sys_supervisor.version: raise BackupInvalidError( @@ -832,7 +830,7 @@ class BackupManager(FileConfiguration, JobGroup): folder_list.remove(FOLDER_HOMEASSISTANT) homeassistant = True - await self._set_location_password(backup, password, location) + await self._validate_backup_location(backup, password, location) if backup.homeassistant is None and homeassistant: raise BackupInvalidError( diff --git a/tests/api/test_backups.py b/tests/api/test_backups.py index 45ddca10a..39fc8758b 100644 --- a/tests/api/test_backups.py +++ b/tests/api/test_backups.py @@ -16,7 +16,11 @@ from supervisor.backups.backup import Backup from supervisor.const import CoreState from supervisor.coresys import CoreSys from supervisor.docker.manager import DockerAPI -from supervisor.exceptions import AddonsError, HomeAssistantBackupError +from supervisor.exceptions import ( + AddonsError, + BackupInvalidError, + HomeAssistantBackupError, +) from supervisor.homeassistant.core import HomeAssistantCore from supervisor.homeassistant.module import HomeAssistant from supervisor.homeassistant.websocket import HomeAssistantWebSocket @@ -466,6 +470,7 @@ async def test_restore_immediate_errors( assert "only a partial backup" in (await resp.json())["message"] with ( + patch.object(Backup, "validate_backup"), patch.object( Backup, "supervisor_version", @@ -488,7 +493,11 @@ async def test_restore_immediate_errors( patch.object( Backup, "all_locations", new={None: {"path": None, "protected": True}} ), - patch.object(Backup, "validate_password", return_value=False), + patch.object( + Backup, + "validate_backup", + side_effect=BackupInvalidError("Invalid password"), + ), ): resp = await api_client.post( f"/backups/{mock_partial_backup.slug}/restore/partial", @@ -497,7 +506,10 @@ async def test_restore_immediate_errors( assert resp.status == 400 assert "Invalid password" in (await resp.json())["message"] - with patch.object(Backup, "homeassistant", new=PropertyMock(return_value=None)): + with ( + patch.object(Backup, "validate_backup"), + patch.object(Backup, "homeassistant", new=PropertyMock(return_value=None)), + ): resp = await api_client.post( f"/backups/{mock_partial_backup.slug}/restore/partial", json={"background": True, "homeassistant": True}, @@ -944,7 +956,7 @@ async def test_restore_backup_from_location( body = await resp.json() assert ( body["message"] - == f"Cannot open backup at {backup_local_path.as_posix()}, file does not exist!" + == f"Cannot validate backup at {backup_local_path.as_posix()}, file does not exist!" ) resp = await api_client.post( diff --git a/tests/backups/conftest.py b/tests/backups/conftest.py index 450b7a9e1..7fa2910f5 100644 --- a/tests/backups/conftest.py +++ b/tests/backups/conftest.py @@ -42,6 +42,7 @@ def partial_backup_mock(backup_mock): backup_instance.supervisor_version = "9999.09.9.dev9999" backup_instance.location = None backup_instance.all_locations = {None: {"protected": False}} + backup_instance.validate_backup = AsyncMock() yield backup_mock @@ -55,6 +56,7 @@ def full_backup_mock(backup_mock): backup_instance.supervisor_version = "9999.09.9.dev9999" backup_instance.location = None backup_instance.all_locations = {None: {"protected": False}} + backup_instance.validate_backup = AsyncMock() yield backup_mock diff --git a/tests/backups/test_backup.py b/tests/backups/test_backup.py index 93dca5200..d2098cc4a 100644 --- a/tests/backups/test_backup.py +++ b/tests/backups/test_backup.py @@ -11,7 +11,7 @@ import pytest from supervisor.backups.backup import Backup from supervisor.backups.const import BackupType from supervisor.coresys import CoreSys -from supervisor.exceptions import BackupFileNotFoundError +from supervisor.exceptions import BackupFileNotFoundError, BackupInvalidError from tests.common import get_fixture_path @@ -79,22 +79,21 @@ async def test_consolidate( @pytest.mark.asyncio @pytest.mark.parametrize( - "tarfile_side_effect, securetar_side_effect, expected_result, expect_exception", + "tarfile_side_effect, securetar_side_effect, expected_exception", [ - (None, None, True, False), # Successful validation - (FileNotFoundError, None, None, True), # File not found - (None, tarfile.ReadError, False, False), # Invalid password + (None, None, None), # Successful validation + (FileNotFoundError, None, BackupFileNotFoundError), # File not found + (None, tarfile.ReadError, BackupInvalidError), # Invalid password ], ) -async def test_validate_password( +async def test_validate_backup( coresys: CoreSys, tmp_path: Path, tarfile_side_effect, securetar_side_effect, - expected_result, - expect_exception, + expected_exception, ): - """Parameterized test for validate_password.""" + """Parameterized test for validate_backup.""" enc_tar = Path(copy(get_fixture_path("backup_example_enc.tar"), tmp_path)) enc_backup = Backup(coresys, enc_tar, "test", None) await enc_backup.load() @@ -120,9 +119,8 @@ async def test_validate_password( MagicMock(side_effect=securetar_side_effect), ), ): - if expect_exception: - with pytest.raises(BackupFileNotFoundError): - await enc_backup.validate_password(None) + if expected_exception: + with pytest.raises(expected_exception): + await enc_backup.validate_backup(None) else: - result = await enc_backup.validate_password(None) - assert result == expected_result + await enc_backup.validate_backup(None) diff --git a/tests/backups/test_manager.py b/tests/backups/test_manager.py index 9be702d7f..b11b8bfba 100644 --- a/tests/backups/test_manager.py +++ b/tests/backups/test_manager.py @@ -344,7 +344,7 @@ async def test_fail_invalid_full_backup( backup_instance = full_backup_mock.return_value backup_instance.all_locations[None]["protected"] = True - backup_instance.validate_password = AsyncMock(return_value=False) + backup_instance.validate_backup.side_effect = BackupInvalidError() with pytest.raises(BackupInvalidError): await manager.do_restore_full(backup_instance) @@ -373,7 +373,7 @@ async def test_fail_invalid_partial_backup( backup_instance = partial_backup_mock.return_value backup_instance.all_locations[None]["protected"] = True - backup_instance.validate_password = AsyncMock(return_value=False) + backup_instance.validate_backup.side_effect = BackupInvalidError() with pytest.raises(BackupInvalidError): await manager.do_restore_partial(backup_instance)