Add State Provider Framework #6640

justindbaur · 2023-10-19T20:05:31Z

Type of change

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

Objective

PM-4342 PM-4343

This adds a framework for how a domain can own their own state and gain observable access to the state that you own, both globally and for a user. There is no implementation of this framework in this PR, a PR with an implementation will follow the merging of this.

Code changes

`StateDefinition`

This is an object that will be created in a common file that is owned by platform and all creations of this type outside of there is strictly prohibited. It encapsulates the rule of the name for your state (most commonly a domain ex. FolderService, CipherService.) and where this state should be stored. The name and location combination of this state should be unique across all other uses of StateDefinitions yet should remain human readable (not a uuid). This is why creations of this type will be required to be in a single file owned by the platform team, and tests will be created to help ensure this.

`KeyDefinition`

This is a type that builds upon the concept of StateDefinition (in fact the creation of a KeyDefinition requires a `StateDefinition) by sub-dividing a state into smaller chunks of information that a state or domain will care about. The creation of this object is also limited to a single file that is owned by platform. The keys within a state must also remain unique. This type also ensures that you give it logic to convert the JSON form of your data into the data you expect to actually be there. This is because state is often round tripped through it's json form and it is not able to be converted back unless specific logic is provided.

This type also contains several helper methods for creating your keys in popular configurations like arrays or records.

`GlobalState`

This class provides you with 2 API's. One is an update method that takes a callback for manipulating your state. The second API is a state$ observable that you may subscribe to that will provide you with emissions any time your state is updated. This may be from your own usage of the update method or this may be from the usage of the update method in a different context than you.

`GlobalStateProvider`

A simple class that gets your GlobalState implementation for the given KeyDefinition.

`UserState`

This is a similar concept to GlobalState but for state about scoped to users. It has a similar update method for updating the state of the currently active user and an observable for subscribing to the data from the current user. It also gives you the ability to get your state directly from it's storage and the ability to update state for a specified user via their id.

Screenshots

Before you submit

Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team
Ensure that all UI additions follow WCAG AA requirements

Add a class for encapsulation information about state this will often be for a domain but creations of this will exist outside of a specific domain, hence just the name State.

This adds a type that extends state definition into another sub-key and forces creators to define the data that will be stored and how to read the data that they expect to be stored.

Adds to function to help building keys for both keys scoped to a specific user and for keys scoped to global storage. Co-authored-by: Matt Gibson <[email protected]>

Original commit by Matt: 823d954 Co-authored-by: Matt Gibson <[email protected]>

Create a helper that creats an Observable from a chrome event and removes the listener when the subscription is completed.

Use fromChromeEvent to create an observable from chrome event and map that into our expected shape.

Co-authored-by: Matt Gibson <[email protected]>

bitwarden-bot · 2023-10-19T21:28:49Z

Checkmarx One – Scan Summary & Details – a7897a6e-0a4a-4696-ac12-eb5bf65cb1fd

No New Or Fixed Issues Found

MGibson1

Early review just because I think you'll the very first comment I had 😄

apps/browser/src/platform/browser/from-chrome-event.ts

Rework fromChromeEvent so we have to lie to TS less and remove unneeded generics. I did this by caring less about the function and more about the parameters only. Co-authored-by: Matt Gibson <[email protected]>

MGibson1

I think there's a bit more polish to do before this is ready

apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts

libs/common/src/platform/abstractions/storage.service.ts

libs/common/src/platform/misc/key-builders.ts

libs/common/src/platform/state/derived-state-definition.ts

libs/common/src/platform/state/implementations/default-user-state.ts

MGibson1 · 2023-10-20T02:10:08Z

libs/common/src/platform/state/implementations/default-user-state.ts

+      const accountChangeSubscription = activeAccountData$.subscribe();
+      const storageUpdateSubscription = storageUpdates$.subscribe((data) => {


💭 Looking at this again, it seems fragile to have two separate watchers be responsible for the current value of a single observable. I might be being paranoid here, though

libs/common/src/platform/state/implementations/default-user-state.ts

libs/common/src/platform/misc/key-builders.ts

Use different key format

…warden/clients into add-state-provider-framework

…framework

apps/browser/src/platform/browser/browser-api.ts

apps/cli/src/platform/services/node-env-secure-storage.service.ts

libs/common/src/platform/misc/key-builders.ts

libs/common/src/platform/state/key-definition.ts

libs/common/src/platform/state/derived-state-definition.ts

libs/common/src/platform/state/implementations/default-global-state.ts

libs/common/src/platform/state/implementations/default-user-state.provider.ts

Co-authored-by: Oscar Hinton <[email protected]>

shane-melton

Looks good for Vault! Thanks for creating the ticket!

* Add StateDefinition Add a class for encapsulation information about state this will often be for a domain but creations of this will exist outside of a specific domain, hence just the name State. * Add KeyDefinition This adds a type that extends state definition into another sub-key and forces creators to define the data that will be stored and how to read the data that they expect to be stored. * Add key-builders helper functions Adds to function to help building keys for both keys scoped to a specific user and for keys scoped to global storage. Co-authored-by: Matt Gibson <[email protected]> * Add updates$ stream to existing storageServices Original commit by Matt: 823d954 Co-authored-by: Matt Gibson <[email protected]> * Add fromChromeEvent helper Create a helper that creats an Observable from a chrome event and removes the listener when the subscription is completed. * Implement `updates$` property for chrome storage Use fromChromeEvent to create an observable from chrome event and map that into our expected shape. * Add GlobalState Abstractions * Add UserState Abstractions * Add Default Implementations of User/Global state Co-authored-by: Matt Gibson <[email protected]> * Add Barrel File for state Co-authored-by: Matt Gibson <[email protected]> * Fix ChromeStorageServices * Rework fromChromeEvent Rework fromChromeEvent so we have to lie to TS less and remove unneeded generics. I did this by caring less about the function and more about the parameters only. Co-authored-by: Matt Gibson <[email protected]> * Fix UserStateProvider Test * Add Inner Mock & Assert Calls * Update Tests to use new keys Use different key format * Prefer returns over mutations in update * Update Tests * Address PR Feedback * Be stricter with userId parameter * Add Better Way To Determine if it was a remove * Fix Web & Browser Storage Services * Fix Desktop & CLI Storage Services * Fix Test Storage Service * Use createKey Helper * Prefer implement to extending * Determine storage location in providers * Export default providers publicly * Fix user state tests * Name tests * Fix CLI * Prefer Implement In Chrome Storage * Remove Secure Storage Option Also throw an exception for subscribes to the secure storage observable. * Update apps/browser/src/platform/browser/from-chrome-event.ts Co-authored-by: Oscar Hinton <[email protected]> * Enforce state module barrel file * Fix Linting Error * Allow state module import from other modules * Globally Unregister fromChromeEvent Listeners Changed fromChromeEvent to add its listeners through the BrowserApi, so that they will be unregistered when safari closes. * Test default global state * Use Proper Casing in Parameter * Address Feedback * Update libs/common/src/platform/state/key-definition.ts Co-authored-by: Oscar Hinton <[email protected]> * Add `buildCacheKey` Method * Fix lint errors * Add Comment Co-authored-by: Oscar Hinton <[email protected]> * Use Generic in callback parameter * Refactor Out DerivedStateDefinition * Persist Listener Return Type * Add Ticket Link --------- Co-authored-by: Matt Gibson <[email protected]> Co-authored-by: Matt Gibson <[email protected]> Co-authored-by: Oscar Hinton <[email protected]>

justindbaur and others added 11 commits October 19, 2023 15:46

Add StateDefinition

b774358

Add a class for encapsulation information about state this will often be for a domain but creations of this will exist outside of a specific domain, hence just the name State.

Add KeyDefinition

ac6d7a3

This adds a type that extends state definition into another sub-key and forces creators to define the data that will be stored and how to read the data that they expect to be stored.

Add key-builders helper functions

624f2ac

Adds to function to help building keys for both keys scoped to a specific user and for keys scoped to global storage. Co-authored-by: Matt Gibson <[email protected]>

Add updates$ stream to existing storageServices

af2a300

Original commit by Matt: 823d954 Co-authored-by: Matt Gibson <[email protected]>

Add fromChromeEvent helper

9f6cd1d

Create a helper that creats an Observable from a chrome event and removes the listener when the subscription is completed.

Implement updates$ property for chrome storage

4d31608

Use fromChromeEvent to create an observable from chrome event and map that into our expected shape.

Add GlobalState Abstractions

32a6bd8

Add UserState Abstractions

518ca5b

Add Default Implementations of User/Global state

c07ed17

Co-authored-by: Matt Gibson <[email protected]>

Add Barrel File for state

0939971

Co-authored-by: Matt Gibson <[email protected]>

Fix ChromeStorageServices

38b8ffd

github-actions bot added the needs-qa Marks a PR as requiring QA approval label Oct 19, 2023

MGibson1 self-assigned this Oct 19, 2023

MGibson1 reviewed Oct 19, 2023

View reviewed changes

apps/browser/src/platform/browser/from-chrome-event.ts Outdated Show resolved Hide resolved

apps/browser/src/platform/browser/from-chrome-event.ts Outdated Show resolved Hide resolved

Rework fromChromeEvent

64026d3

Rework fromChromeEvent so we have to lie to TS less and remove unneeded generics. I did this by caring less about the function and more about the parameters only. Co-authored-by: Matt Gibson <[email protected]>

justindbaur force-pushed the add-state-provider-framework branch from c440182 to 64026d3 Compare October 20, 2023 01:30

MGibson1 requested changes Oct 20, 2023

View reviewed changes

MGibson1 previously requested changes Oct 20, 2023

View reviewed changes

libs/common/src/platform/misc/key-builders.ts Outdated Show resolved Hide resolved

libs/common/src/platform/misc/key-builders.ts Outdated Show resolved Hide resolved

justindbaur and others added 11 commits October 23, 2023 14:23

Fix UserStateProvider Test

51d2525

Add Inner Mock & Assert Calls

958d85f

Update Tests to use new keys

2ae4805

Use different key format

Prefer returns over mutations in update

6912ea7

Update Tests

bb8a510

Merge branch 'add-state-provider-framework' of https://github.com/bit…

eac8c75

…warden/clients into add-state-provider-framework

Address PR Feedback

ce0700a

Be stricter with userId parameter

9debaf1

Add Better Way To Determine if it was a remove

c9bb3b8

Fix Web & Browser Storage Services

b6a3d78

Merge remote-tracking branch 'origin/master' into add-state-provider-…

5514274

…framework

Use Proper Casing in Parameter

bf3e516

Hinton reviewed Nov 3, 2023

View reviewed changes

justindbaur and others added 4 commits November 3, 2023 13:10

Address Feedback

bd67d40

Update libs/common/src/platform/state/key-definition.ts

9ca81cf

Co-authored-by: Oscar Hinton <[email protected]>

Add buildCacheKey Method

d15a227

Fix lint errors

ba7888c

justindbaur requested a review from a team as a code owner November 3, 2023 18:49

justindbaur requested a review from shane-melton November 3, 2023 18:49

justindbaur and others added 4 commits November 6, 2023 10:55

Add Comment

b35fd23

Co-authored-by: Oscar Hinton <[email protected]>

Merge branch 'master' into add-state-provider-framework

f02113a

Use Generic in callback parameter

d3322c4

Refactor Out DerivedStateDefinition

9d94bc9

MGibson1 previously approved these changes Nov 7, 2023

View reviewed changes

Persist Listener Return Type

5173350

justindbaur dismissed MGibson1’s stale review via 5173350 November 7, 2023 14:37

MGibson1 approved these changes Nov 7, 2023

View reviewed changes

MGibson1 previously approved these changes Nov 7, 2023

View reviewed changes

Add Ticket Link

dc4575b

justindbaur dismissed MGibson1’s stale review via dc4575b November 7, 2023 17:10

shane-melton approved these changes Nov 7, 2023

View reviewed changes

MGibson1 approved these changes Nov 8, 2023

View reviewed changes

Merge branch 'master' into add-state-provider-framework

59012e9

justindbaur removed the needs-qa Marks a PR as requiring QA approval label Nov 9, 2023

justindbaur merged commit e1b5b83 into master Nov 9, 2023
59 of 60 checks passed

justindbaur deleted the add-state-provider-framework branch November 9, 2023 22:06

MGibson1 mentioned this pull request Nov 10, 2023

Ps/pm 2910/state framework improvements #6860

Merged

rr-bw mentioned this pull request Nov 16, 2023

[PM-194] Browser Account Switcher UI #6772

Merged

justindbaur mentioned this pull request Dec 5, 2023

Add State Provider Framework Deep Dive bitwarden/contributing-docs#248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add State Provider Framework #6640

Add State Provider Framework #6640

justindbaur commented Oct 19, 2023 •

edited by MGibson1

Loading

bitwarden-bot commented Oct 19, 2023 •

edited

Loading

MGibson1 left a comment

MGibson1 left a comment

MGibson1 Oct 20, 2023

shane-melton left a comment

		const accountChangeSubscription = activeAccountData$.subscribe();
		const storageUpdateSubscription = storageUpdates$.subscribe((data) => {

Add State Provider Framework #6640

Add State Provider Framework #6640

Conversation

justindbaur commented Oct 19, 2023 • edited by MGibson1 Loading

Type of change

Objective

Code changes

StateDefinition

KeyDefinition

GlobalState

GlobalStateProvider

UserState

Screenshots

Before you submit

bitwarden-bot commented Oct 19, 2023 • edited Loading

No New Or Fixed Issues Found

MGibson1 left a comment

Choose a reason for hiding this comment

MGibson1 left a comment

Choose a reason for hiding this comment

MGibson1 Oct 20, 2023

Choose a reason for hiding this comment

shane-melton left a comment

Choose a reason for hiding this comment

justindbaur commented Oct 19, 2023 •

edited by MGibson1

Loading

`StateDefinition`

`KeyDefinition`

`GlobalState`

`GlobalStateProvider`

`UserState`

bitwarden-bot commented Oct 19, 2023 •

edited

Loading