Update logic for handling the pin protected user key #6169
Conversation
| storeUserAutoUnlockKey(userId = userId, userAutoUnlockKey = null) | ||
| storePinProtectedUserKey(userId = userId, pinProtectedUserKey = null) | ||
| storePinProtectedUserKeyEnvelope(userId = userId, pinProtectedUserKeyEnvelope = null) | ||
| storeEncryptedPin(userId = userId, encryptedPin = null) |
There was a problem hiding this comment.
This data is never intended to be removed when loging out of the app (hard or soft logout).
Given this requirement, I am simplifying the UserLogoutManager and handling it here.
| // * PinProtectedUserKeyEnvelop | ||
| // * DeviceKey | ||
| // * PendingAuthRequest | ||
| // * OnboardingStatus |
There was a problem hiding this comment.
Updating the list of properties that are never cleared.
| } | ||
| authDiskSource.storePinProtectedUserKeyEnvelope( | ||
| userId = userId, | ||
| pinProtectedUserKeyEnvelope = pinProtectedUserKeyEnvelope, |
There was a problem hiding this comment.
We simply never clear this data, so there is no need to have this logic present to preserve the data.
| authDiskSource.storeAccountTokens( | ||
| userId = userId, | ||
| accountTokens = null, | ||
| ) |
There was a problem hiding this comment.
The tokens will be cleared as part of clearData(userId = userId) call below.
david-livefront
requested review from
a team and
brian-livefront
as code owners
|
Claude finished @david-livefront's task —— View job Code Review CompleteOverall Assessment: APPROVE This is a clean refactoring that extracts PIN-protected user key handling into a dedicated manager class. The change follows established architectural patterns, reduces code duplication, and improves separation of concerns. SummaryChange Type: Refactoring (standard review depth) Key Improvements:
Findings
💭 Finding 2: Test naming was questioned in previous review but names are actually correct
Architecture Validation✅ MVVM + Manager Pattern: New manager follows established patterns in docs/ARCHITECTURE.md Test Coverage✅ Comprehensive: All code paths tested (early returns, success, failure, in-memory vs disk) Coverage includes:
Security Assessment✅ Zero-knowledge architecture preserved No security concerns identified. Code Quality✅ Follows STYLE_AND_BEST_PRACTICES.md Recommendation: APPROVE pending typo fix in AuthDiskSourceImpl.kt:166 This refactoring improves code organization and maintainability while preserving existing behavior and security properties. |
david-livefront
force-pushed
the
pin-migration-update
branch
from
e1426e9 to
98a0f47
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
app/src/main/kotlin/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSourceImpl.kt
Outdated
Show resolved
Hide resolved
| ) | ||
| } | ||
|
|
||
| private fun storePinData( |
There was a problem hiding this comment.
💭 Consider adding KDoc to explain the storePinData behavior, particularly:
- The relationship between
inMemoryOnlyparameter and storage calls - Why
pinProtectedUserKeyis always cleared at disk level (lines 91-95)
Suggested KDoc
/**
* Stores PIN-related data to disk after an enrollment operation.
*
* @param userId The user ID for which to store PIN data.
* @param encryptedPin The new encrypted PIN value, or null to clear it.
* @param pinProtectedUserKeyEnvelope The new PIN-protected user key envelope, or null to clear it.
* @param inMemoryOnly If true, the PIN-protected user key envelope is stored only in memory.
* If false, it is persisted to disk.
*
* Note: This always clears the legacy [pinProtectedUserKey] at the disk level to ensure proper
* migration to the envelope-based approach.
*/
private fun storePinData(While private functions don't require KDoc per project guidelines, this function has non-obvious behavior that would benefit from documentation.
|
Great job! No new security vulnerabilities introduced in this pull request |
david-livefront
force-pushed
the
pin-migration-update
branch
from
98a0f47 to
c5f4eb5
Compare
app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/PinProtectedUserKeyManagerTest.kt
Show resolved
Hide resolved
app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/PinProtectedUserKeyManagerTest.kt
Show resolved
Hide resolved
david-livefront
force-pushed
the
pin-migration-update
branch
from
c5f4eb5 to
535deae
Compare
david-livefront
force-pushed
the
pin-migration-update
branch
from
535deae to
71a987c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6169 +/- ##
==========================================
+ Coverage 84.97% 85.00% +0.03%
==========================================
Files 735 724 -11
Lines 53087 52848 -239
Branches 7678 7678
==========================================
- Hits 45113 44926 -187
+ Misses 5291 5246 -45
+ Partials 2683 2676 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
TBD
📔 Objective
This PR updates the way we handle the pin protected user key and it migration to the pin protected user key envelope.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes