[PM1724] State Management v2 #64

dani-garcia · 2023-06-08T12:51:05Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Version two of the state service implementation, now supporting more granular changes (instead of just overwriting the whole profile) and using file locks as well.

Also implemented state services with different views over the global state to isolate the changes a bit better, this is put to the test on a separate PR (#65) handling folder creation/update/deletion. Example:

// Create a new folder
client
    .get_state_service(FOLDERS_SERVICE)
    // This applies the change in the callback to the state in a synchronised way to avoid overwriting other changes
    .modify(move |folders| { // `folders` here is a mutable reference to the Folders HashMap
        folders.insert(id, Folder { id, name, revision_date });
        Ok(())
    })
    .await?;

Code changes

The state code is stored inside crates/bitwarden/src/state/, inside there you can find the following:

state.rs: Contains all the state handling code, and is mostly meant to be generic over state contents and storage medium, to implement another storage type, like secure storage, biometric storage, etc we'd just need to implement the trait StateStorageMedium. Currently implemented are file based storage for non-WASM and LocalStorage for WASM.
state_service.rs: Contains the implementation for the different state "views" that will allow the services to access only the part of the state that they require. Check the separate Folders PR (Folders state service using Typestate pattern #65) for an example on how it's used for the Folders service, there's also src/client/[auth,keys,profile].rs, which handle some parts of the state that are used internally.
The sync function has the response type removed, the response is processed internally by the state handling code, and clients should access the data through there, instead of through the raw sync response.
Client has been modified to move most of it's internal state into the new state management solution. This has caused some functions to need to become async, to handle the synchronised access to the state.
The client now also has methods to access the state, plus a session_login method to restore the client from a session persisted to disk. At the moment, session_login also forces a vault unlock, though in the future we probably want to create separate lock and unlock functionality.

# Conflicts: # crates/bitwarden-json/src/client.rs # crates/bitwarden-json/src/command.rs # crates/bitwarden-napi/src-ts/bitwarden_client/index.ts # crates/bitwarden-napi/src-ts/bitwarden_client/schemas.ts # crates/bitwarden/src/auth/commands/login.rs # crates/bitwarden/src/client/auth_settings.rs # crates/bitwarden/src/client/client.rs # crates/bitwarden/src/client/mod.rs # crates/bitwarden/src/commands/mod.rs # crates/bitwarden/src/commands/secrets.rs # crates/bitwarden/src/commands/sync.rs # crates/bitwarden/src/crypto.rs # crates/bitwarden/src/platform/empty_request.rs # crates/bitwarden/src/platform/folders_request.rs # crates/bitwarden/src/platform/sync.rs # crates/bitwarden/src/sdk/mod.rs # crates/bitwarden/src/sdk/request/mod.rs # crates/bitwarden/src/sdk/response/mod.rs # crates/bitwarden/src/sdk/response/secrets_response.rs # crates/bitwarden/src/util.rs # crates/sdk-schemas/src/main.rs # languages/csharp/schemas.cs # languages/js_webassembly/bitwarden_client/schemas.ts # languages/python/BitwardenClient/schemas.py # support/schemas/bitwarden/client/ClientSettings.json

crates/bitwarden-json/src/command.rs

crates/bitwarden/src/client/auth_settings.rs

crates/bws/src/main.rs

crates/bitwarden-json/src/command.rs

crates/bitwarden-json/src/response.rs

# Conflicts: # crates/bitwarden-json/src/command.rs # crates/bitwarden-napi/src-ts/bitwarden_client/schemas.ts # languages/csharp/schemas.cs # languages/js_webassembly/bitwarden_client/schemas.ts # languages/python/BitwardenClient/schemas.py # support/schemas/bitwarden_json/Command.json

Hinton · 2023-07-03T14:06:19Z

crates/bitwarden/src/state/domain.rs

Not sure if I like the filename of this. Ideally different models should live in different files. I also don't necessarily associate domain with state.

Yeah, I don't think it's a great place for them, I've renamed the file to models which I think is somewhat better, and I'll see where else we can move those models to, the only remaining ones are the Encryption Keys, Profile and Auth tokens, so I guess the best place to put them would be auth and platform? Or maybe client, next to AuthSettings and EncryptionSettings?

Hinton · 2023-07-03T14:07:27Z

crates/bitwarden/src/state/domain.rs

+    pub profile: Option<Profile>,
+
+    pub ciphers: HashMap<Uuid, Cipher>,
+    pub folders: HashMap<Uuid, Folder>,


I would like to have a generic way to assign data to an account, so that we don't necessarily fill this with 20 hashmaps, and everyone has to constantly modify it.

That would also allow the consumers to store data without needing to maintain a separate system for it.

This struct wasn't in use in the last revision of the PR, it remained there from when the state was handled as a single block. I've deleted it now.

I think what you want to do can be done with client .get_state_service(FOLDERS_SERVICE), by defining any service, it's just a type and a namespace. It's not exposed outside of the SDK but it can be done, we'd just need to avoid conflicting namespaces.

crates/bitwarden/src/state/domain.rs

…te services in them

bitwarden-bot · 2023-07-10T17:18:16Z

Checkmarx One – Scan Summary & Details – 1bb9fef7-d521-4aab-9c17-2a9cef17de92

No New Or Fixed Issues Found

# Conflicts: # Cargo.lock # crates/bitwarden/src/auth/commands/login.rs # crates/bitwarden/src/platform/get_user_api_key.rs

# Conflicts: # crates/bitwarden-napi/src-ts/bitwarden_client/schemas.ts # crates/sdk-schemas/Cargo.toml # languages/csharp/schemas.cs # languages/js_webassembly/bitwarden_client/schemas.ts # languages/python/BitwardenClient/schemas.py # support/schemas/bitwarden/client/ClientSettings.json # support/schemas/bitwarden_json/Command.json

# Conflicts: # Cargo.lock # crates/bitwarden-json/Cargo.toml # crates/bitwarden/Cargo.toml

crates/bitwarden-json/Cargo.toml

Hinton · 2023-07-17T13:50:36Z

We have 36 warnings building this. I think we need to put more things behind flags.

dani-garcia · 2023-07-17T14:01:51Z

A few of those warnings are on the master branch, I'll make a new PR to fix them

# Conflicts: # crates/bitwarden/src/auth/commands/login.rs # crates/bitwarden/src/client/client.rs # crates/bitwarden/src/client/mod.rs

Hinton

Did an initial review.

There are two architectural changes in this PR.

Introducing state_services which are loosely stored on the client object.
State persistent mechanism.

I will need to do some deeper digging into this to properly understand everything.

crates/bitwarden-json/src/client.rs

Hinton · 2023-07-17T20:14:16Z

crates/bitwarden/src/state/state.rs

@@ -0,0 +1,293 @@
+use std::{collections::HashMap, fmt::Debug, path::PathBuf};
+
+use async_lock::Mutex;


Could we use tokio instead? It's already a dependency and would let us remove async-lock.

We don't actually use tokio on the bitwarden library crate, for compatibility reasons with WASM

crates/bitwarden/src/crypto.rs

Hinton · 2023-07-18T08:48:52Z

crates/bitwarden/src/auth/commands/login.rs

+    let Some(expires) = auth.token_expiration else {return Err(Error::VaultLocked)};
+    let Some(login_method) = auth.login_method else {return Err(Error::VaultLocked)};


This error doesn't really make sense.

This is removed with the set_access_token change mentioned below.

Hinton · 2023-07-18T08:50:47Z

crates/bitwarden/src/auth/commands/login.rs

+
+    let Some(expires) = auth.token_expiration else {return Err(Error::VaultLocked)};
+    let Some(login_method) = auth.login_method else {return Err(Error::VaultLocked)};
+    let expires_seconds = (expires.timestamp() - chrono::Utc::now().timestamp()) as u64;


I wonder if we should have two set_tokens methods, one that accepts a DateTime, and one that accepts seconds in the future.

Just realised that calling set_tokens here didn't make sense. set_tokens both sets the access token for the API clients, and also stores the access and refresh tokens and their expiration in the state.

In this case we are only interested in the first part, it doesn't make sense here to save it in the state when we just read it from state, so I've separated that part to a new set_access_token, which means we don't have to use the expiration here at all.

Hinton · 2023-07-18T10:45:24Z

crates/bitwarden/src/client/client.rs

@@ -86,6 +84,8 @@ impl Client {
            .build()
            .unwrap();

+        let state = State::load_state(&settings);


Do you know how long time it takes to load the state of a large account?

This doesn't actually load and parse the file yet, it's just initializing the state with the correct path and everything. Probably worth renaming to initialize_state.

The file would be loaded in the current session_login, by the line client.state.load_account(input.user_id).await?;.

I'll do some testing, but I expect it to be plenty fast.

Hinton · 2023-07-18T10:46:17Z

crates/bitwarden/src/client/client.rs

+    pub(crate) async fn get_auth_settings(&self) -> Option<AuthSettings> {
+        Auth::get(self).await.kdf
    }


This looks odd, kdf shouldn't be auth settings?

Yeah, I feel like it should be kdf_settings and the struct renamed to KdfSettings. It only holds the email and KDF parameters, it's only tangentially related to authentication. What do you think?

Hinton · 2023-07-18T11:41:09Z

crates/bitwarden/src/state/state.rs

+    }
+}
+
+fn load_medium(path: &Option<String>) -> Box<dyn StateStorageMedium> {


Is medium some terminology?

I've used storage medium to refer to the different storage backends, in a similar way than CD's or DVD's would be different storage media.

It doesn't really add anything to the name though, we can just rename the StateStorageMedium trait to Storage plainly, and rename load_medium to initialize_storage (Because it's not actually loading anything yet).

Hinton · 2023-07-18T11:41:42Z

crates/bitwarden/src/state/state.rs

+#[cfg(not(target_arch = "wasm32"))]
+#[derive(Debug)]
+struct FileStateStorageMedium {
+    path: PathBuf,
+}


We should probably move out wasm code to it's own file to easier conditionally compile it.

Hinton · 2023-07-18T11:42:22Z

crates/bitwarden/src/state/state.rs

+            Box::new(FileStateStorageMedium::new(path.into()))
+        }
+    } else {
+        Box::new(InMemoryStateStorageMedium::new())


Why do we need a in memory state?

This is meant to handle two cases:

The SDK doesn't have configured the storage path, like for secrets manager use, or for a one-off operation. In this case we still use the state to store authentication tokens, etc. But they are not persisted to disk.

During login, we don't have the users UUID until after a sync is done, so we don't create the persistent storage until then, we use the in memory storage as a temporary place until we can dump everything in the persistent storage

…te files, some renames

…y store them in state

dani-garcia added 2 commits June 8, 2023 14:39

SDK state management V2

36b6721

Update schemas

b1a7e2e

dani-garcia requested review from Hinton and MGibson1 June 8, 2023 12:51

dani-garcia mentioned this pull request Jun 8, 2023

State management #48

Closed

dani-garcia changed the title ~~Pm 1724 state management v2~~ [PM1724] State Management v2 Jun 8, 2023

Don't include tokio or fs4 in wasm builds

69c6ce9

MGibson1 force-pushed the PM-1724-state-management-v2 branch from bf8c233 to b1a7e2e Compare June 9, 2023 01:08

dani-garcia added 3 commits June 14, 2023 13:15

Remove unused SyncResponse

1f82459

Initialize organization crypto when restoring session from state

9027f6d

dani-garcia requested review from Hinton and removed request for Hinton June 28, 2023 18:17

Update schemas

9d40999

Hinton reviewed Jun 29, 2023

View reviewed changes

crates/bitwarden-json/src/command.rs Outdated Show resolved Hide resolved

Hinton reviewed Jun 29, 2023

View reviewed changes

crates/bitwarden/src/client/auth_settings.rs Outdated Show resolved Hide resolved

Hinton reviewed Jun 29, 2023

View reviewed changes

crates/bws/src/main.rs Outdated Show resolved Hide resolved

Hinton reviewed Jun 29, 2023

View reviewed changes

crates/bitwarden-json/src/command.rs Outdated Show resolved Hide resolved

crates/bitwarden-json/src/response.rs Outdated Show resolved Hide resolved

dani-garcia added 4 commits June 30, 2023 11:18

Remove some unrelated changes

f0dfff4

Remove most of the KDF changes from here

17d0810

Domain doesn't need to implement JSON schema

cbeb4fc

Hinton mentioned this pull request Jul 3, 2023

Extract encrypt/decrypt refactor from state PR #85

Merged

Hinton reviewed Jul 3, 2023

View reviewed changes

dani-garcia added 6 commits July 4, 2023 16:14

Remove unused model structs and folder code, rename domain.rs

9094323

Merge branch 'master' into PM-1724-state-management-v2

9a99eb2

Update schemas

1f9edd2

Extracted decryptable/encryptable

eb0281c

Split models.rs into auth/keys/profile files, and encapsulate the sta…

1fd9cca

…te services in them

Merge branch 'master' into PM-1724-state-management-v2

4915eb7

dani-garcia added 4 commits July 12, 2023 14:36

Merge branch 'master' into PM-1724-state-management-v2

4064d3b

# Conflicts: # Cargo.lock # crates/bitwarden/src/auth/commands/login.rs # crates/bitwarden/src/platform/get_user_api_key.rs

Run prettier

e893acd

Merge branch 'master' into PM-1724-state-management-v2

9345ce8

# Conflicts: # Cargo.lock # crates/bitwarden-json/Cargo.toml # crates/bitwarden/Cargo.toml

Hinton reviewed Jul 17, 2023

View reviewed changes

crates/bitwarden-json/Cargo.toml Show resolved Hide resolved

Merge branch 'master' into PM-1724-state-management-v2

2dd6e61

# Conflicts: # crates/bitwarden/src/auth/commands/login.rs # crates/bitwarden/src/client/client.rs # crates/bitwarden/src/client/mod.rs

dani-garcia requested a review from Hinton July 17, 2023 16:09

Hinton reviewed Jul 18, 2023

View reviewed changes

dani-garcia added 3 commits July 18, 2023 16:30

Remove some unnecessary pub's, move storage implementations to separa…

22c4a74

…te files, some renames

Rename functions to unlock

1d43f45

Just set the access token on unlock, no reason to read and immediatel…

530ae5b

…y store them in state

dani-garcia closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM1724] State Management v2 #64

[PM1724] State Management v2 #64

dani-garcia commented Jun 8, 2023 •

edited

Loading

Hinton Jul 3, 2023 •

edited

Loading

dani-garcia Jul 4, 2023

Hinton Jul 3, 2023

dani-garcia Jul 4, 2023

bitwarden-bot commented Jul 10, 2023 •

edited

Loading

Hinton commented Jul 17, 2023

dani-garcia commented Jul 17, 2023

Hinton left a comment

Hinton Jul 17, 2023

dani-garcia Jul 18, 2023

Hinton Jul 18, 2023

dani-garcia Jul 18, 2023

Hinton Jul 18, 2023

dani-garcia Jul 18, 2023

Hinton Jul 18, 2023

dani-garcia Jul 18, 2023

Hinton Jul 18, 2023

dani-garcia Jul 18, 2023

Hinton Jul 18, 2023

dani-garcia Jul 18, 2023 •

edited

Loading

Hinton Jul 18, 2023

Hinton Jul 18, 2023

dani-garcia Jul 18, 2023

		@@ -0,0 +1,293 @@
		use std::{collections::HashMap, fmt::Debug, path::PathBuf};

		use async_lock::Mutex;

		let Some(expires) = auth.token_expiration else {return Err(Error::VaultLocked)};
		let Some(login_method) = auth.login_method else {return Err(Error::VaultLocked)};

[PM1724] State Management v2 #64

[PM1724] State Management v2 #64

Conversation

dani-garcia commented Jun 8, 2023 • edited Loading

Type of change

Objective

Code changes

Hinton Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwarden-bot commented Jul 10, 2023 • edited Loading

No New Or Fixed Issues Found

Hinton commented Jul 17, 2023

dani-garcia commented Jul 17, 2023

Hinton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia commented Jun 8, 2023 •

edited

Loading

Hinton Jul 3, 2023 •

edited

Loading

bitwarden-bot commented Jul 10, 2023 •

edited

Loading

dani-garcia Jul 18, 2023 •

edited

Loading