[PM-24051] In identity and sync response & decryption options, add MasterPasswordUnlockDataResponse in response model #376

mzieniukbw · 2025-08-05T09:21:48Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-24051

📔 Objective

Adds MasterPasswordUnlock and UserDecryption support with response parsing into the identity token response.
Includes Uniffi and WASM support.
Tested response parsing in client's (via WASM).

⏰ 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

👍 (:+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

…tions

github-actions · 2025-08-05T09:27:25Z

Checkmarx One – Scan Summary & Details – 1a65f52b-1474-417e-abb6-86d5d05ab37f

Great job! No new security vulnerabilities introduced in this pull request

codecov · 2025-08-05T09:36:59Z

Codecov Report

❌ Patch coverage is 95.07246% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.36%. Comparing base (ae0cae2) to head (93a3d0c).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...asm-internal/src/key_management/user_decryption.rs	0.00%	10 Missing ⚠️
...twarden-core/src/key_management/master_password.rs	98.02%	5 Missing ⚠️
...twarden-core/src/key_management/user_decryption.rs	97.53%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
+ Coverage   73.95%   74.36%   +0.41%     
==========================================
  Files         252      256       +4     
  Lines       21574    22126     +552     
==========================================
+ Hits        15955    16454     +499     
- Misses       5619     5672      +53

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

quexten

Some initial comments, some of these are required, some are idiomatic (but probably required since the quality bar for SDK seems to be set much higher).

I'd like a review from someone with more rust experience such as @Hinton or @dani-garcia too. I believe specifically the request parsing is something that has not existed in the SDK so far, and there may be a better way to do this that we're unaware of.

crates/bitwarden-core/src/key_management/master_password.rs

quexten · 2025-08-06T08:14:02Z

crates/bitwarden-core/src/key_management/master_password.rs

+pub struct MasterPasswordUnlockData {
+    pub kdf: Kdf,
+    pub master_key_wrapped_user_key: EncString,
+    pub salt: String,


We have an opaque string type MasterPasswordSalt in clients: https://github.com/bitwarden/clients/blob/61cd0c4f51d9fe390c93f61f2f55076de33b57cf/libs/common/src/key-management/master-password/types/master-password.types.ts#L15.

Ideally we would expose this from rust as we do for EncString

sdk-internal/crates/bitwarden-crypto/src/enc_string/symmetric.rs

Line 18 in 4813492

export type EncString = Tagged<string, "EncString">;

and replace the TS version with the SDK version so that we have type safety across the FFI boundary.

This one is optional, if we don't do it here it is tech debt we should record.

From what i understand, in cases EncString, we have an equivalent type in Rust SDK, that is different during Serialization: EncString becomed String type, hence why we export it in TS as string.

This is different here, since we don't have equivalent in our Rust code.
If we want one, then i think either of the two will be ok:

#[cfg_attr(feature = "wasm", tsify::declare(tsify::Tsify))] pub type MasterPasswordSalt = String;

Or change the salt field to have different type in TS.

#[cfg_attr( feature = "wasm", tsify(type = "MasterPasswordSalt") )] pub salt: String,

I think my main concern here is that we want the tagged type, so that the typesafety stretches to TS.

A bit more verbose than I'd like, but this does the job, and is the same approach I use for the new PinProtectedKeyEnvelope (PasswordProtectedkeyEnvelope):

#[derive(Serialize, Deserialize)] #[serde(transparent)] #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] pub struct MasterPasswordSalt( #[cfg_attr( feature = "wasm", tsify(type = r#"Tagged<string, "MasterPasswordSalt">"#) )] String, );

But again, we can do that later if we don't want to do in in this PR, I don't want to scope creep it.

quexten · 2025-08-06T08:15:42Z

crates/bitwarden-core/src/key_management/master_password.rs

@@ -0,0 +1,353 @@
+#![allow(missing_docs)]


Please remove this and add comments. We require comments on everything that is publicly exposed.

crates/bitwarden-core/src/key_management/master_password.rs

crates/bitwarden-core/src/key_management/user_decryption.rs

crates/bitwarden-wasm-internal/src/key_management/master_password.rs

crates/bitwarden-core/src/key_management/master_password.rs

crates/bitwarden-core/src/key_management/user_decryption.rs

crates/bitwarden-core/src/key_management/master_password.rs

sonarqubecloud · 2025-08-08T09:46:50Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

quexten

Looking pretty nice now! Some more smaller items that improve usability by teams unfamiliar with the code (docs / error names)

quexten · 2025-08-08T14:14:25Z

crates/bitwarden-core/src/key_management/master_password.rs

+#[allow(missing_docs)]
+#[bitwarden_error(flat)]
+#[derive(Debug, thiserror::Error)]
+pub enum MasterPasswordError {
+    #[error(transparent)]
+    Crypto(#[from] CryptoError),
+    #[error(transparent)]
+    MissingField(#[from] MissingFieldError),
+}


Suggested change

#[allow(missing_docs)]

#[bitwarden_error(flat)]

#[derive(Debug, thiserror::Error)]

pub enum MasterPasswordError {

#[error(transparent)]

Crypto(#[from] CryptoError),

#[error(transparent)]

MissingField(#[from] MissingFieldError),

}

#[bitwarden_error(flat)]

#[derive(Debug, thiserror::Error)]

pub enum MasterPasswordError {

/// The wrapped encryption key could not be parsed

#[error(transparent)]

Crypto(#[from] CryptoError),

/// The KDF data is malformed

#[error(transparent)]

MissingField(#[from] MissingFieldError),

}

I would actually suggest making these errors more descriptive. If I understand the rest of the file correctly, missingfield happens exactly when the KDF is broken, and Crypto happens exactly when the encstring parsing fails, so you could just skip wrapping the errors and make it more clear to the caller what actually failed:

Suggested change

#[allow(missing_docs)]

#[bitwarden_error(flat)]

#[derive(Debug, thiserror::Error)]

pub enum MasterPasswordError {

#[error(transparent)]

Crypto(#[from] CryptoError),

#[error(transparent)]

MissingField(#[from] MissingFieldError),

}

#[bitwarden_error(flat)]

#[derive(Debug, thiserror::Error)]

pub enum MasterPasswordError {

/// The wrapped encryption key could not be parsed because the encstring is malformed

EncryptionKeyMalformed,

/// The KDF data could not be parsed, because it is missing values or has an invalid value

KdfMalformed

}

quexten · 2025-08-08T14:14:48Z

crates/bitwarden-core/src/key_management/master_password.rs

+    type Error = MasterPasswordError;
+
+    fn try_from(response: MasterPasswordUnlockResponseModel) -> Result<Self, Self::Error> {
+        let kdf = match response.kdf.kdf_type {


If you do apply the suggestion from above, then this would require mapping to the different error types

quexten · 2025-08-08T14:15:13Z

crates/bitwarden-core/src/key_management/user_decryption.rs

+use crate::key_management::master_password::{MasterPasswordError, MasterPasswordUnlockData};
+
+/// Error for master user decryption related operations.
+#[allow(missing_docs)]


Suggested change

#[allow(missing_docs)]

quexten · 2025-08-08T14:15:26Z

crates/bitwarden-core/src/key_management/user_decryption.rs

+
+/// Represents data required to decrypt user's vault.
+/// Currently, this is only used for master password unlock.
+#[allow(missing_docs)]


Suggested change

#[allow(missing_docs)]

quexten · 2025-08-08T14:17:07Z

crates/bitwarden-wasm-internal/src/key_management/user_decryption.rs

+};
+use wasm_bindgen::prelude::wasm_bindgen;
+
+#[wasm_bindgen]


Suggested change

#[wasm_bindgen]

/// This is a stop-gap solution to allow parsing of response models via the SDK while the full decryption functionality is not yet moved over to the SDK

#[wasm_bindgen]

Hinton · 2025-08-11T11:09:36Z

This tightly couples consumers of the SDK to the server generated bindings which is somewhat dangerous. We've previously addressed this by wrapping requests/responses in a manner that more accurately represents the action from the SDK's perspective. There be dragons here and we should consider this a bit.

Do we expect to do similar things to other request/responses? If not I think we should simply wrap the logic with our own struct, while it's a few lines of boilerplate it reduces the complexity.

Includes Uniffi and WASM support.

There is no UniFFI support here.

Hinton · 2025-08-11T11:16:45Z

crates/bitwarden-wasm-internal/src/key_management/user_decryption.rs

question: Why are we implementing logic in wasm-internal? This would generally belong to our feature crates.

Hinton · 2025-08-11T11:18:25Z

support/openapi-template/Cargo.mustache

@@ -90,3 +110,5 @@ mockall = ["dep:mockall"]
 bon = ["dep:bon"]
 {{/useBonBuilder}}
 {{/reqwestTrait}}
+tsify = { workspace = true, optional = true, features = ["js"], default-features = false }
+wasm-bindgen = { workspace = true, optional = true, features = ["serde-serialize"] }


We'd need some way to configure this in runtime, since this would now also apply to identity.

Hinton · 2025-08-11T11:37:35Z

crates/bitwarden-api-api/src/models/master_password_unlock_response_model.rs

 pub struct MasterPasswordUnlockResponseModel {
    #[serde(rename = "kdf")]
    pub kdf: Box<models::MasterPasswordUnlockKdfResponseModel>,
    #[serde(rename = "masterKeyEncryptedUserKey")]
-    pub master_key_encrypted_user_key: Option<String>,
+    pub master_key_encrypted_user_key: String,


Platform hasn't made any strides into making fields required as it has compatibility implications. This will throw an error should these fields not be available in the past, present or future. I.e. can we guarantee forever that this endpoint in the past and future has and always will include master_key_encrypted_user_key and salt? And if these are missing are we fine with it throwing?

This is related to https://bitwarden.atlassian.net/wiki/spaces/DEV/pages/760971277/Improve+API+bindings.

mzieniukbw added 2 commits August 4, 2025 23:40

Adds MasterPasswordUnlock into identity's response user decryption op…

44773f7

…tions

Adds MasterPasswordUnlock KDF change handling in sync

0712878

mzieniukbw added 4 commits August 5, 2025 11:11

simplification

8ca20a8

clippy fix

28a8423

formatting

b089ea0

no handling, just response parsing

0d35393

mzieniukbw requested review from a team and quexten and removed request for a team August 5, 2025 11:08

mzieniukbw added 4 commits August 5, 2025 12:51

test coverage

a08179d

wasm

b3647f2

wasm unit test coverage

36d3136

wasm unit test coverage

7c8664d

mzieniukbw marked this pull request as ready for review August 5, 2025 14:51

mzieniukbw requested a review from a team as a code owner August 5, 2025 14:51

mzieniukbw requested a review from justindbaur August 5, 2025 14:51

Added UserDecryption data, response model with handling

9c7d50d

quexten requested changes Aug 6, 2025

View reviewed changes

mzieniukbw added 5 commits August 7, 2025 13:05

autogenerated wasm responses, UserDecryption struct, use of TryFrom

70ad9d3

failing unit test

110f4db

lint

ff86adf

lint

fa253a3

KdfType enum duplicate

4a95dc5

mzieniukbw mentioned this pull request Aug 7, 2025

[PM-24051] UserDecryption response model tagged with sdk-wasm in OpenApi generated schema bitwarden/server#6171

Open

mzieniukbw added 3 commits August 8, 2025 09:57

revert identity crate wasm, since there it's not used right now

cd37e31

docs

3bf880f

formatting

93a3d0c

mzieniukbw requested a review from quexten August 8, 2025 09:42

quexten reviewed Aug 8, 2025

View reviewed changes

Hinton reviewed Aug 11, 2025

View reviewed changes

	#[wasm_bindgen]
	/// This is a stop-gap solution to allow parsing of response models via the SDK while the full decryption functionality is not yet moved over to the SDK
	#[wasm_bindgen]

[PM-24051] In identity and sync response & decryption options, add MasterPasswordUnlockDataResponse in response model #376

Are you sure you want to change the base?

[PM-24051] In identity and sync response & decryption options, add MasterPasswordUnlockDataResponse in response model #376

Conversation

mzieniukbw commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

codecov bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

quexten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

quexten Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Aug 8, 2025

Quality Gate passed

Uh oh!

quexten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzieniukbw commented Aug 5, 2025 •

edited

Loading

github-actions bot commented Aug 5, 2025 •

edited

Loading

codecov bot commented Aug 5, 2025 •

edited

Loading

quexten Aug 6, 2025 •

edited

Loading

quexten Aug 8, 2025 •

edited

Loading

Hinton commented Aug 11, 2025 •

edited

Loading

Hinton Aug 11, 2025 •

edited

Loading