[PM-24051] Init crypto and update kdf with MasterPasswordUnlock #399
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ use crate::{ | |
| JwtToken, | ||
| }, | ||
| client::{internal::UserKeyState, LoginMethod, UserLoginMethod}, | ||
| key_management::UserDecryptionData, | ||
| require, Client, | ||
| }; | ||
|
|
||
|
|
@@ -32,34 +33,54 @@ pub(crate) async fn login_api_key( | |
| let kdf = client.auth().prelogin(email.clone()).await?; | ||
|
|
||
| client.internal.set_tokens( | ||
| r.access_token.to_owned(), | ||
| r.refresh_token.to_owned(), | ||
|
Comment on lines
+36
to
+37
There was a problem hiding this comment. 💭 not a rust expert It is weird to me that we are keeping From the rust docs https://doc.rust-lang.org/std/borrow/trait.ToOwned.html
It sounds like we should prefer clone when going from There was a problem hiding this comment. Agree on this. In this case we are not even going from borrowed data to owned data, so semantically it is confusing saying "to owned" (implying we are converting to an owned object (by cloning usually). We're going from String to String (T to T), so we should clone. |
||
| r.expires_in, | ||
| ); | ||
|
|
||
| let private_key = r.private_key.as_deref(); | ||
|
There was a problem hiding this comment. Ref the other comment about TryFrom<Option<&str>> |
||
| let private_key: EncString = require!(private_key).parse()?; | ||
|
|
||
| let user_key_state = UserKeyState { | ||
| private_key, | ||
| signing_key: None, | ||
|
There was a problem hiding this comment. (Not in this PR) looks like we don't have support for parsing the new keys from the token response here yet. We should make a follow-up ticket somewhere to fix this. |
||
| security_state: None, | ||
| }; | ||
|
|
||
| if let Some(master_password_unlock) = r | ||
| .user_decryption_options | ||
|
There was a problem hiding this comment. Suggestion (non-blocking): We have this long chain quite a bit. It may be nice to just implement "master_password_unlock() -> Option" on the decryptionData. Then, you can rewrite as (untested): |
||
| .as_ref() | ||
| .map(UserDecryptionData::try_from) | ||
| .transpose()? | ||
| .and_then(|user_decryption| user_decryption.master_password_unlock) | ||
| { | ||
| client | ||
| .internal | ||
| .initialize_user_crypto_master_password_unlock( | ||
| input.password.clone(), | ||
| master_password_unlock, | ||
| user_key_state, | ||
| )?; | ||
| } else { | ||
|
Comment on lines
+50
to
+64
There was a problem hiding this comment. ⛏️ |
||
| let user_key = r.key.as_deref(); | ||
| let user_key: EncString = require!(user_key).parse()?; | ||
|
There was a problem hiding this comment. Ref the comment about TryFrom<Option<&str>> |
||
| let master_key = MasterKey::derive(&input.password, &email, &kdf)?; | ||
|
|
||
| client.internal.initialize_user_crypto_master_key( | ||
| master_key, | ||
| user_key, | ||
| user_key_state, | ||
| )?; | ||
| } | ||
|
|
||
| client | ||
| .internal | ||
| .set_login_method(LoginMethod::User(UserLoginMethod::ApiKey { | ||
| client_id: input.client_id.clone(), | ||
| client_secret: input.client_secret.clone(), | ||
| email, | ||
| kdf, | ||
| })); | ||
| } | ||
|
|
||
| Ok(ApiKeyLoginResponse::process_response(response)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,20 +89,11 @@ pub(crate) async fn complete_auth_request( | |
| .await?; | ||
|
|
||
| if let IdentityTokenResponse::Authenticated(r) = response { | ||
| client.internal.set_tokens( | ||
| r.access_token.clone(), | ||
| r.refresh_token.clone(), | ||
| r.expires_in, | ||
| ); | ||
|
|
||
| let method = match res.master_password_hash { | ||
|
There was a problem hiding this comment. We should probably just drop this code. Support for this auth method on the approving side has been removed 3 months ago: bitwarden/clients#14236 (not this PR) |
||
| Some(_) => AuthRequestMethod::MasterKey { | ||
|
|
@@ -118,8 +109,8 @@ pub(crate) async fn complete_auth_request( | |
| .crypto() | ||
| .initialize_user_crypto(InitUserCryptoRequest { | ||
| user_id: None, | ||
| kdf_params: Kdf::default(), | ||
| email: auth_req.email.to_owned(), | ||
| private_key: require!(r.private_key).parse()?, | ||
| signing_key: None, | ||
| security_state: None, | ||
|
|
@@ -130,6 +121,14 @@ pub(crate) async fn complete_auth_request( | |
| }) | ||
| .await?; | ||
|
|
||
| client | ||
| .internal | ||
| .set_login_method(LoginMethod::User(UserLoginMethod::Username { | ||
| client_id: "web".to_owned(), | ||
| email: auth_req.email.to_owned(), | ||
| kdf: Kdf::default(), | ||
| })); | ||
|
Comment on lines
+124
to
+130
There was a problem hiding this comment. ❓ Looking to clarify. So this will be a change in behavior? Previously this would get overridden by the With the difference being Reading through the old flow previously kdf was getting set to default and emails match up. Odd for initing crypto to have that side effect but I don't understand all the flows. |
||
|
|
||
| Ok(()) | ||
| } else { | ||
| Err(LoginError::AuthenticationFailed) | ||
|
|
||
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.
💭 question:
initialize_crypto_single_org_keydidn't have the side effect of setting the login method? Is this change just to have consistency with the others? I'm a little hesitant because it's SM.