[PM-31159] Full state service re-write - #1029
Merged
Merged
Conversation
Contributor
|
New Issues (34)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1029 +/- ##
==========================================
+ Coverage 6.79% 25.68% +18.89%
==========================================
Files 67 73 +6
Lines 2798 2955 +157
Branches 483 539 +56
==========================================
+ Hits 190 759 +569
+ Misses 2576 2072 -504
- Partials 32 124 +92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eliykat
requested changes
Mar 7, 2026
eliykat
left a comment
eliykat
left a comment
Member
There was a problem hiding this comment.
Thank you for tackling this! I think spending some time getting proper tests on stateService and stateMigrationService will give us some peace of mind that we won't get an influx of tickets on release.
BTreston
commented
Mar 11, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
|
@eliykat this is ready for another look when you have a moment, thanks. |
- State v4 was never properly migrated, handle migration directly from v3 - fix access token not properly migrated - fix environment urls not properly migrated - update tests - fix fresh install (missing state version) being set to v1 -> v5 (current)
eliykat
requested changes
Mar 21, 2026
eliykat
left a comment
eliykat
left a comment
Member
There was a problem hiding this comment.
Great job improving the tests for StateService and StateMigrationService in particular.
Comment on lines
+60
to
+68
| const ldapConfig = { ...account.directoryConfigurations.ldap }; | ||
| if ( | ||
| useSecureStorageForSecrets && | ||
| ldapConfig.password && | ||
| ldapConfig.password !== StoredSecurely | ||
| ) { | ||
| await this.secureStorageService.save(SecureStorageKeys.ldap, ldapConfig.password); | ||
| ldapConfig.password = StoredSecurely; | ||
| } |
Member
There was a problem hiding this comment.
Is this necessary? As long as we're migrating from plain storage to plain storage, and from secure storage to secure storage, I don't think we need to read the useSecureStorageForSecrets value. It'll just maintain wherever they have secrets stored at the moment.
Member
|
I can't see that this comment has been addressed. Is it that the current secure storage prepends the orgId, and we're now dropping that? |
Contributor
Author
Yes, sorry I forgot the address this in a comment. That is correct, they were prepended with the org id and this moved them off of that. |
- Rename state.service.ts to default-state.service.ts - Rename state.service.spec.ts to default-state.service.spec.ts - Clean up state service interface to remove unused StorageOptions - More type saftey for storage keys - Clean up migration logic
eliykat
requested changes
Mar 26, 2026
eliykat
left a comment
eliykat
left a comment
Member
There was a problem hiding this comment.
Only 1 actual change below, then feel free to pass to QA. I can approve when I'm back from OOO next week but that doesn't need to hold up QA.
Comment on lines
+6
to
+9
| get: <T>(key: StorageKey | SecureStorageKey, options?: StorageOptions) => Promise<T>; | ||
| has: (key: StorageKey | SecureStorageKey, options?: StorageOptions) => Promise<boolean>; | ||
| save: (key: StorageKey | SecureStorageKey, obj: any, options?: StorageOptions) => Promise<any>; | ||
| remove: (key: StorageKey | SecureStorageKey, options?: StorageOptions) => Promise<any>; |
Member
There was a problem hiding this comment.
d'oh. Didn't realise both Storage and SecureStorage implemented the same interface, so this doesn't prevent keys from being mixed up between them. oh well :)
Comment on lines
+77
to
+78
| await this.secureStorageService.save(SecureStorageKeys.ldap, ldapConfig.password); | ||
| ldapConfig.password = StoredSecurely; |
Member
There was a problem hiding this comment.
Secure storage keys are migrated below, so I meant we don't have to deal with them at all here.
This section: migrate old plaintext keys -> new plaintext keys
Below: migrate secure storage keys -> secure storage keys
So here I would just do:
const ldapConfig = { ...account.directoryConfigurations.ldap };
await this.set(StorageKeys.directoryLdap, ldapConfig);... and so on.
Let me know if this doesn't work for whatever reason.
… directory config
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31159
📔 Objective
This PR encompasses the totality of the state service re-write. This includes:
state.service.tsjslibjslibservices into the main directory structurejslibservice dependencies (e.g.crypto-service)jslibstate into DC app state (mostly electron related state).jslibcompatibility (stub files)jslibstate migration logic into DC's state migration servicejslibmodels📸 Screenshots