State management

[dani-garcia]

Type of change

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

Objective

Implemented a state management solution, including persisting to a file (or LocalStorage for WASM). This state solution includes a per account state and a global state.

Code changes

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

• state.rs: Contains all the state handling code, and is mostly 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.
• domain.rs: Contains the domain structs with all the data encrypted as we get it from the server, plus the functions to convert the response to the sync response into the domain.
• view.rs: Contains the view structs, and implements the Encryptable and Decryptable traits on them to convert them from/to the domain model.

The Client has been modified to move most of it's state into the new state management solution, the only thing left are the decrypted keys now. 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 only a small part of the models are represented in code, just enough to test that it works correctly. Currently missing are organisations, collections, policies, sends, and most of the cipher and profile data. I might be missing more as well. To complete those we have to follow these steps:

• Create an empty struct in domain.rs(if it exists you can skip to the next step). Make sure the #[derive] and #[serde] annotations are present at the top of the struct. Next, add a field using your new struct to the existing Data struct (just like Ciphers and Folders are added). At the moment we are using HashMaps for Ciphers and Folders, you can use a HashMap as well or a Vec, whichever is more appropriate.
• Add all the fields to the struct (check SyncResponseModel to see what data we get from the server). Make sure all the names are snake_case, and use types as appropriate (don't use Optional if the value is never null, use Uuid and DateTime instead of String for Guids and dates, use CipherString instead of String for all the encrypted strings)
• At this point you should get a compile error that one of the convert_* functions at the bottom is missing the field you added to Data before, you can add this field now and assign it a value extracted from the parameters.
• In view.rs, create a <Model>View struct, with the same structure and contents than the one in domain.rs, the only difference is all the CipherString's need to be changed to String's, as the view contains the decrypted data.
• Implement Decryptable and Encryptable for your struct in view.rs, based on the existing implementations for Cipher and Folder

Before you submit

• Please add unit tests where it makes sense to do so (encouraged but not required)

[MGibson1]

Would it be possible to split out the automated openapi gen stuff to a different PR?

If the reset of this is needed to enable it, I'd be OK opening a PR against an api update branch, but some visual distinction between manual changes vs automated would be super helpful

[MGibson1]

I haven't finished by any means, but wanted to get a few thoughts to you quickly

[MGibson1]

Should probably log rather than eprintln

Suggested change

	Err(e) => {
	eprintln!("Error parsing Bitwarden client settings: {e:?}");
	}
	Err(e) => {
	log::error!("Error parsing Bitwarden client settings: {e:?}");
	}

[dani-garcia]

We don't have the log crate added at the moment to the bitwarden-json crate, but that makes sense to change, yes

[MGibson1]

Global and account-level state exist due to the structured nature of state today and the fact that clients can manage multiple accounts simultaneously.

I'm not sure we ever want the SDK to manage multiple accounts. I imagine multiple accounts in a visualizing client application would just be through multiple SDK client instances.

If that's the case, does it make sense to get rid of the whole concept and allow the respective calls to store handle scoping by the storage key?

@Hinton what are your thoughts here?

[dani-garcia]

The idea is to have the sdk account state manage a single defined account for each SDK client, yes, so the view apps as you mention would have to keep multiple sdk client instances.

The global state was created for the visualization apps to store some settings that aren't account specific in the same place as the rest of the account-specific state.The SDK doesn't use the global state for anything at the moment, so if we are okay with the clients keeping their global and non-sdk config somewhere else, we can remove the global support in the SDK.

[MGibson1]

I think the global items would still be managed by the SDK, it's just that the fact that they are global doesn't need to be encompassed in our structs at all.

In fact, I'm not so sure representing our state in structs is a good idea in general. It feels like it'll push up towards the issue described in the uml below.

I was more imagining a hashmap and helper functions for other structs to persist data as they need

[Hinton]

I had to dig into what global state we have. I think almost all of it falls under areas the SDK shouldn't be responsible for as it's strictly "application" specific logic. That's not to say that we wouldn't benefit from avoiding duplicating it somehow.

The only thing that might be useful for the sdk is the environment urls, but at the same time those would be used when you login to a new account i.e. it's the applications responsibility.

I would like to investigate building a "higher level SDK" that would be aware of multiple accounts and responsible for switching between them (with whatever security enforcement we have) as a way to reduce code duplication and ensure consistent and safe behaviors. But for now we can delegate that responsibility to the application layer.

[MGibson1]

I think it's going to be a mistake to keep our state too structured. Especially if we do things like the Data struct, where data for all of our ciphers and folders are stored.

We want to be able to get overlapping update requests without data loss

[dani-garcia]

Yeah, that sounds fair enough, I pretty much just copied the structure we had now as a starting point, but there's no reason for it to have that Data separation

[Hinton]

We covered this slightly in this ADR, probably worth a read, I think the main points would still apply in the SDK. https://contributing.bitwarden.com/architecture/adr/refactor-state-service

[dani-garcia]

Would it be possible to split out the automated openapi gen stuff to a different PR?

If the reset of this is needed to enable it, I'd be OK opening a PR against an api update branch, but some visual distinction between manual changes vs automated would be super helpful

It's in separated commits, the ones called "Update bindings to support uuids" and "Update JSON schemas" are automatically generated while the rest are manual changes.

The GitHub review interface lets you select a range of commits to review instead, the manual changes for this PR are here: https://github.com/bitwarden/sdk/pull/48/files/da088e07d7419b09adf9eaea619b5275b49f4fbc..a2f4275b4033d67b95c2baa824c65483d5d63fa0

Let me know if that helps, otherwise I can look into splitting into two PR's.

[dani-garcia]

This is replaced by #64