[PM-27289] Remove password unlock method #2155
Conversation
|
Claude Code is working… I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } else { | ||
| .password(password: password, userKey: encUserKey) | ||
| guard let masterPasswordUnlock = account.profile.userDecryptionOptions?.masterPasswordUnlock else { | ||
| throw AuthError.missingUserDecryptionOptions |
There was a problem hiding this comment.
💭 Consider a more specific error type
While AuthError.missingUserDecryptionOptions is functionally correct, it's also used in other unlock scenarios (device key, etc.). For better error diagnostics and user messaging, consider adding a more specific error case:
| throw AuthError.missingUserDecryptionOptions | |
| guard let masterPasswordUnlock = account.profile.userDecryptionOptions?.masterPasswordUnlock else { | |
| throw AuthError.missingMasterPasswordUnlockData |
This would make it easier to distinguish between different missing decryption option scenarios when debugging or logging.
There was a problem hiding this comment.
@andrebispo5 I think I agree with Claude here where we could add more explicit errors. Perhaps just an associated property to the enum' value AuthError.missingUserDecryptionOptions that explains which thing is missing. As in this case it could be that account.profile.userDecryptionOptions is nil or it could be that account.profile.userDecryptionOptions.masterPasswordUnlock is nil.
Also we'd need to update the docs on AuthError.missingUserDecryptionOptions as right now it says:
/// The device key from trusting the device is missing.
case missingUserDecryptionOptionswhich doesn't align with this scenario.
There was a problem hiding this comment.
Are there any concerns that there could still be users that don't have masterPasswordUnlock data? Either self-hosted users or users which haven't synced in a while? In this case the user is essentially locked out after the password unlock option is removed, right? They'd have to log out and log back in to recover? Maybe a product question, but I wonder if we'd want a specific alert to let the user know that they need to logout and log back in?
| "Master Password Unlock" | ||
| case .password: | ||
| "Password" | ||
| "Password (Legacy - Deprecated)" |
There was a problem hiding this comment.
💭 Deprecation strategy
Good to mark this as "Legacy - Deprecated". Since this PR removes .password usage from the vault unlock flow but the enum case remains in the SDK, consider:
- Adding a code comment documenting when/if this case can be fully removed
- If it must remain for SDK compatibility, document which scenarios still require it
- If removal is planned, consider adding a
@availabledeprecation attribute when Swift's SDK supports it
This helps future maintainers understand the deprecation timeline.
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2155 +/- ##
==========================================
+ Coverage 85.38% 85.40% +0.01%
==========================================
Files 1726 1727 +1
Lines 145621 145770 +149
==========================================
+ Hits 124343 124493 +150
+ Misses 21278 21277 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27289
📔 Objective
This pull request refactors the vault unlock logic to fully transition away from the legacy password-based unlock method, ensuring that vault unlocking now always uses the more secure master password unlock flow. The changes update both implementation and tests.
Authentication Logic Refactor
unlockVaultWithPassword(password:)method now exclusively uses the.masterPasswordUnlockmethod, removing support for the legacy.passwordunlock flow. IfmasterPasswordUnlockdata is missing, an explicit error is thrown.⏰ 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