-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-27281] Support v2 account encryption on JIT master password signups #6777
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-27281] Support v2 account encryption on JIT master password signups #6777
Conversation
Codecov Reportโ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6777 +/- ##
==========================================
+ Coverage 58.83% 59.00% +0.17%
==========================================
Files 1925 1933 +8
Lines 85355 85756 +401
Branches 7652 7681 +29
==========================================
+ Hits 50217 50604 +387
- Misses 33269 33283 +14
Partials 1869 1869 โ View full report in Codecov by Sentry. ๐ New features to boost your workflow:
|
|
New Issues (5)Checkmarx found the following issues in this Pull Request
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs
Outdated
Show resolved
Hide resolved
|
Adding test coverage, turning to draft for now |
quexten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you moved this to draft; Adding the comments I had so far.
src/Core/Auth/UserFeatures/UserMasterPassword/Interfaces/ISetInitialMasterPasswordCommandV1.cs
Show resolved
Hide resolved
src/Core/Auth/UserFeatures/TdeOnboardingPassword/Interfaces/ITdeOnboardingPasswordCommand.cs
Outdated
Show resolved
Hide resolved
|
Test coverage added, redy to review |
mkincaid-bw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor things.
util/Migrator/DbScripts/2026-01-02_00_User_UpdateMasterPassword.sql
Outdated
Show resolved
Hide resolved
util/Migrator/DbScripts/2026-01-02_00_User_UpdateMasterPassword.sql
Outdated
Show resolved
Hide resolved
ike-kottlowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments mainly around test coverage and possible improvements therein.
src/Core/Auth/UserFeatures/TdeOnboardingPassword/Interfaces/ITdeOnboardingPasswordCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommandV1.cs
Show resolved
Hide resolved
quexten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! I don't have much to comment on this / concerns were addressed or are addressed at some point in the future in other work.
mkincaid-bw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ike-kottlowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent test coverage, thank you! Just a non-blocking ๐จ comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐จ : We try to not name things V1/Vn because of the ambiguity that comes with it. I think something akin to SetInitialMasterPasswordWithSymetricKeyEncryptionCommand - really rolls off the tongue ๐ซ - is not ambiguous.
The current SetInitialMasterPasswordCommand could be SetInitialMasterPasswordWithCborEncryptionCommand. The usage can be denoted with comments in code pointing to the preferred when creating a password, so developers know which command to prefer.
This is not a blocker, but I think removes ambiguity from the code.
## ๐๏ธ Tracking https://bitwarden.atlassian.net/browse/PM-27277 ## ๐ Objective Encryption V2 for SSO JIT master password account registration. Changed the default Kdf for Encryption V2 accounts, which uses Argon2Id with more iOS mobile friendly parameters (less memory, but more iterations). Refactored the Kdf default functions, so we only expose `Kdf::default_pbkdf2` or `Kdf::default_argon2id`. Requires bitwarden/server#6777, for the autogenerated models, which is manually committed in `bitwarden-api-api` crate temporarily, until the server PR is done. Updated WASM build script only to format autogenerated TS files, since the JS file got too big for `npm prettier` to handle and is not necessary for non-TS files. ## ๐จ Breaking Changes None, new functionality. ## โฐ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## ๐ฆฎ Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - ๐ (`:+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


๐๏ธ Tracking
https://bitwarden.atlassian.net/browse/PM-27281
๐ Objective
Updated the server API
POST /account/set-passwordto support User Crypto V2 for JIT (Just-In-Time) initial master password registration flow.This endpoint is used for two SSO user flows.
manage account recoverypermission - user will be asked to set master password upon new login. Account keys are already setup. Since there are slightly different requirements on the validation and crypto initialisation logic, i decided to separate this flow into it's separate command.Other changes:
V1suffix intoSetInitialMasterPasswordCommandV1, will be easier to remove later.UserRepository.UpdateUserDataAsyncfunction for the TDE decryption MP onboarding user flow, since they already have the account keys and only need MP set. Since we already haveSetMasterPasswordthat is used for both flows (MP and TDE decryption) and this function returns a "task" (does not execute against database), it make sense to re-use it. This function is technically a "run multiple tasks in one transaction" orchestrator, which we might want to do going forward anyway for complex database operations, that needs multiple steps done within one transaction and requires minimal changes to the repository.๐ธ Screenshots
โฐ 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