Always validate Backup before restoring (#5632)

* Validate Backup always before restoring

Since #5519 we check the encryption password early in restore case.
This has the side effect that we check the file existance early too.
However, in the non-encryption case, the file is not checked early.

This PR changes the behavior to always validate the backup file before
restoring, ensuring both encryption and non-encryption cases are
handled consistently.

In particular, the last case of test_restore_immediate_errors actually
validates that behavior. That test should actually have failed so far.
But it seems that because we validate the backup shortly after freeze
anyways, the exception still got raised early enough.

A simply `await asyncio.sleep(10)` right after the freeze makes the
test case fail. With this change, the test works consistently.

* Address pylint

* Fix backup_manager tests

* Drop warning message

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(

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
 
 

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)

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)