Check password early on backup restore #5519
Conversation
📝 WalkthroughWalkthroughThe changes focus on enhancing the password validation mechanism for backups in the Home Assistant Supervisor. The Changes
Sequence DiagramsequenceDiagram
participant Backup
participant BackupManager
participant BackupFile
Backup->>BackupFile: set_password(password)
Backup->>BackupFile: validate_password()
alt Password Valid
BackupFile-->>Backup: Return True
Backup-->>BackupManager: Proceed with restore
else Password Invalid
BackupFile-->>Backup: Return False
Backup-->>BackupManager: Raise BackupInvalidError
end
The sequence diagram illustrates the new password validation flow, showing how the Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (2)supervisor/backups/backup.py (2)
The refactored
The password validation implementation looks solid with proper error handling and logging. However, since this involves file I/O operations, consider adding a timeout to prevent hanging in case of slow disk operations. async def validate_password(self) -> bool:
"""Validate backup password.
Returns false only when the password is known to be wrong.
"""
- return await self.sys_run_in_executor(_validate_file)
+ try:
+ return await asyncio.wait_for(
+ self.sys_run_in_executor(_validate_file),
+ timeout=30 # 30 seconds timeout
+ )
+ except asyncio.TimeoutError:
+ _LOGGER.warning("Password validation timed out")
+ return True # Default to allowing restore attempt on timeoutLet's verify the usage of this new method: Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supervisor/backups/backup.py (1)
299-305: Clarify the return type in the docstring or type hints.
Although the signature was changed from returning a bool to returning None, consider updating documentation to reflect that it always returns None to avoid confusion for callers.def set_password(self, password: str | None) -> None: - """Set the password for an existing backup.""" + """Set the password for an existing backup (returns None)."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/backups/backup.py(2 hunks)supervisor/backups/manager.py(2 hunks)tests/api/test_backups.py(1 hunks)tests/backups/test_manager.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
supervisor/backups/backup.py
339-339: First line should end with a period
Add period
(D400)
339-339: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
🪛 GitHub Check: Check ruff
supervisor/backups/backup.py
[failure] 339-339: Ruff (D400)
supervisor/backups/backup.py:339:9: D400 First line should end with a period
[failure] 339-339: Ruff (D415)
supervisor/backups/backup.py:339:9: D415 First line should end with a period, question mark, or exclamation point
🪛 GitHub Check: Check pylint
supervisor/backups/backup.py
[warning] 363-363:
W0718: Catching too general exception Exception (broad-exception-caught)
[warning] 340-340:
R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
🔇 Additional comments (5)
tests/api/test_backups.py (1)
481-481: Good coverage for negative password validation scenario.
Ensuring that the test checks for validate_password returning False validates the error-handling logic.
supervisor/backups/manager.py (2)
690-695: Good use of validate_password in full-restore logic.
This shift to a dedicated password-validation method is clear and improves separation of concerns.
760-765: Proactive password validation for partial restore flow.
Well-integrated approach consistently ensures that all partial restore scenarios are checked before proceeding.
tests/backups/test_manager.py (2)
333-333: Ensure that validate_password is tested thoroughly for negative scenarios.
Transitioning from set_password to validate_password is correct. Confirm that all call sites expecting a boolean now await the new method’s return.
362-362: Consistent negative test logic for partial RESTORE.
Changing from set_password to validate_password keeps the test aligned with the new method signature.
agners
force-pushed
the
check-backup-password-early
branch
from
e48f467 to
e1b3aff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
supervisor/backups/backup.py (2)
338-342: Enhance docstring with more details.The docstring should include:
- Information about the validation process
- Return value explanation
- Possible exceptions that could be raised
- """Validate backup password. - - Returns false only when the password is known to be wrong. - """ + """Validate backup password by attempting to read the archive. + + The method attempts to read the backup archive using the provided password. + Returns false only when the password is known to be wrong, true if the + password is correct or validation cannot be performed. + + Returns: + bool: False if password is incorrect, True otherwise + """
374-376: Consider handling specific exceptions.The broad exception catch could mask important issues. Consider catching specific exceptions that might occur during tar file operations.
- except Exception: # noqa: BLE001 + except (tarfile.HeaderError, OSError) as err: _LOGGER.exception("Unexpected error validating password") return True🧰 Tools
🪛 GitHub Check: Check pylint
[warning] 374-374:
W0718: Catching too general exception Exception (broad-exception-caught)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/backups/backup.py(2 hunks)supervisor/backups/manager.py(2 hunks)tests/api/test_backups.py(1 hunks)tests/backups/test_manager.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/api/test_backups.py
- supervisor/backups/manager.py
🧰 Additional context used
🪛 GitHub Check: Check pylint
supervisor/backups/backup.py
[warning] 374-374:
W0718: Catching too general exception Exception (broad-exception-caught)
🔇 Additional comments (4)
supervisor/backups/backup.py (2)
299-305: LGTM! Good separation of concerns.
The changes to set_password improve the design by:
- Separating password setting from validation
- Adding ability to clear password by passing None
- Removing boolean return value since the method only sets state
344-377: LGTM! Robust error handling implementation.
The implementation:
- Properly handles cases where validation cannot be performed
- Uses appropriate logging levels for different scenarios
- Follows defensive programming practices
🧰 Tools
🪛 GitHub Check: Check pylint
[warning] 374-374:
W0718: Catching too general exception Exception (broad-exception-caught)
tests/backups/test_manager.py (2)
333-333: LGTM! Test updated to match new validation logic.
The test correctly verifies that backup restoration fails when password validation returns false.
362-362: LGTM! Consistent test coverage for partial backups.
The test maintains parity with full backup testing, ensuring password validation works the same way for partial backups.
agners
force-pushed
the
check-backup-password-early
branch
from
e1b3aff to
5e4c7a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
supervisor/backups/backup.py (2)
339-342: Enhance docstring clarity.The docstring should clarify that it returns
Truein case of successful validation or when validation cannot be performed (e.g., unexpected errors).- """Validate backup password. - - Returns false only when the password is known to be wrong. - """ + """Validate backup password. + + Returns: + bool: False only when the password is known to be wrong. + True if password is valid or validation cannot be performed. + """
338-378: Add type hints for better code maintainability.The method would benefit from explicit type hints for the internal function.
async def validate_password(self) -> bool: - def _validate_file() -> bool: + def _validate_file() -> bool: ending: str = f".tar{'.gz' if self.compressed else ''}" test_tar_name: str | None test_tar_file: IO[bytes] | None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/backups/backup.py(2 hunks)supervisor/backups/manager.py(2 hunks)tests/api/test_backups.py(1 hunks)tests/backups/test_manager.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/backups/test_manager.py
- tests/api/test_backups.py
- supervisor/backups/manager.py
🔇 Additional comments (2)
supervisor/backups/backup.py (2)
299-305: LGTM! Clean implementation of password handling.
The changes to set_password improve flexibility by allowing password clearing while maintaining clean implementation.
344-377: 🛠️ Refactor suggestion
Add explicit return path and improve error handling.
- The
_validate_filefunction needs an explicit return path for the case when no tar file is found and no exceptions occur. - The broad exception catch could mask important issues.
def _validate_file() -> bool:
ending = f".tar{'.gz' if self.compressed else ''}"
with tarfile.open(self.tarfile, "r:") as backup:
test_tar_name = next(
(
entry.name
for entry in backup.getmembers()
if entry.name.endswith(ending)
),
None,
)
if not test_tar_name:
_LOGGER.warning("No tar file found to validate password with")
return True
test_tar_file = backup.extractfile(test_tar_name)
try:
with SecureTarFile(
ending, # Not used
gzip=self.compressed,
key=self._key,
mode="r",
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
+ except (OSError, IOError) as err:
+ _LOGGER.exception("IO error validating password: %s", err)
+ return True
+ except Exception as err: # pylint: disable=broad-exception-caught
+ _LOGGER.exception("Unexpected error validating password: %s", err)
+ return TrueLikely invalid or redundant comment.
Introduce a validate password method which only peaks into the archive to validate the password before starting the actual restore process. This makes sure that a wrong password returns an error even when restoring the backup in background.
agners
force-pushed
the
check-backup-password-early
branch
from
5e4c7a7 to
56df84a
Compare
Proposed change
Introduce a validate password method which only peaks into the archive to validate the password before starting the actual restore process. This makes sure that a wrong password returns an error even when restoring the backup in background.
This implements what has been added in Core with home-assistant/core#133647.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor