Changes to support feedback from @thgoebel, fmd-foss maintainer - #9
Conversation
…ort PINs (future will be 16+)
There was a problem hiding this comment.
Pull Request Overview
This PR adds password-free authentication artifacts support, updates device wipe to require a PIN, renames photo-related methods for consistency, adds Location.from_json() factory method, and removes deprecated get_device_stats functionality. Version bumped to 2.0.4.
- Password-free resume functionality with artifact export/import and hash-based reauthentication
- Device wipe now requires alphanumeric ASCII PIN parameter with validation
- Photo method naming updated from
*_photo()to*_picture()with deprecation warnings for old names
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| fmd_api/client.py | Adds resume(), export_auth_artifacts(), from_auth_artifacts(), drop_password() methods and hash-based reauth; removes get_device_stats() |
| fmd_api/device.py | Renames photo methods (with deprecation for old names), adds PIN validation to wipe(), adds message support to lock(), uses Location.from_json() |
| fmd_api/models.py | Adds Location.from_json() factory method for parsing JSON strings/dicts |
| fmd_api/_version.py | Version bump to 2.0.4 |
| pyproject.toml | Version bump to 2.0.4 |
| README.md | Updates documentation for new features, method renames, and wipe PIN requirement |
| docs/MIGRATE_FROM_V1.md | Updates migration guide for renamed methods and wipe PIN requirement |
| docs/PROPOSAL.md | Updates method names in proposal documentation |
| docs/AUTH_ARTIFACTS_DESIGN.md | New design document for password-free authentication |
| tests/unit/test_client.py | Removes test for deleted get_device_stats() |
| tests/unit/test_device.py | Updates tests for renamed methods and wipe PIN requirement |
| tests/unit/test_resume.py | New tests for artifact-based resume and hash-based reauth |
| tests/unit/test_models.py | New tests for Location.from_json() |
| tests/unit/test_lock_message.py | New tests for lock message sanitization |
| tests/unit/test_drop_password.py | New tests for drop_password functionality |
| tests/unit/test_device_wipe_validation.py | New tests for wipe PIN validation |
| tests/functional/test_device.py | Updates method names in functional tests |
| tests/functional/test_commands.py | Removes stats command, adds lock message support |
Comments suppressed due to low confidence (1)
fmd_api/device.py:210
- This statement is unreachable.
raise OperationError("PIN cannot be empty")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| inst.private_key = cast(RSAPrivateKey, serialization.load_pem_private_key(pk_bytes, password=None)) | ||
| except ValueError: | ||
| inst.private_key = cast(RSAPrivateKey, serialization.load_der_private_key(pk_bytes, password=None)) |
There was a problem hiding this comment.
[nitpick] Bare exception handling may silently swallow unexpected errors. The try-except block on lines 267-270 catches only ValueError when loading PEM, then attempts DER. However, if the DER loading also fails with ValueError, the exception will propagate. If either operation fails with a different exception type (e.g., TypeError, cryptography-specific errors), it will also propagate. Consider adding explicit error handling or documentation about expected failure modes to make the behavior more predictable.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merge pull request #9 from devinslick/ThoreFeedback
This release intents on addressing feedback from @thgoebel, who graciously provided his feedback on this project.
This release addresses 6 main areas:
See more details in the issue (#8).