[PM-12049] Remove usage of ActiveUserState from folder service #11880

gbubemismith · 2024-11-06T15:52:26Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-12049

📔 Objective

ActiveUserState is being deprecated. Remove usage from folderService and require userId to be passed.

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

…tate Added extra test cases for encrypted folder and decrypted folders Updated derived state to use decrypt with key

codecov · 2024-11-07T01:53:18Z

Codecov Report

Attention: Patch coverage is 40.95745% with 111 lines in your changes missing coverage. Please review.

Project coverage is 33.51%. Comparing base (ca839b3) to head (12a8501).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
libs/common/src/platform/sync/core-sync.service.ts	0.00%	10 Missing ⚠️
.../src/vault/components/folder-add-edit.component.ts	0.00%	9 Missing ⚠️
...wser/src/vault/popup/settings/folders.component.ts	0.00%	7 Missing ⚠️
apps/cli/src/vault/create.command.ts	0.00%	7 Missing ⚠️
...ault/vault-filter/services/vault-filter.service.ts	0.00%	7 Missing ⚠️
...common/src/vault/services/folder/folder.service.ts	66.66%	7 Missing ⚠️
apps/cli/src/commands/edit.command.ts	0.00%	6 Missing ⚠️
apps/cli/src/commands/get.command.ts	0.00%	6 Missing ⚠️
apps/cli/src/vault/delete.command.ts	0.00%	6 Missing ⚠️
...ogs/bulk-move-dialog/bulk-move-dialog.component.ts	0.00%	6 Missing ⚠️
... and 16 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11880      +/-   ##
==========================================
- Coverage   33.52%   33.51%   -0.01%     
==========================================
  Files        2859     2859              
  Lines       89434    89489      +55     
  Branches    17022    17033      +11     
==========================================
+ Hits        29979    29994      +15     
- Misses      57094    57129      +35     
- Partials     2361     2366       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

JS Bundle Analysis - Avoid shipping oversized bundles

gbubemismith · 2024-11-08T17:49:42Z

libs/common/src/vault/services/key-state/folder.state.ts

      return [];
    }
+
+    const decryptFolderPromises = folders.map((f) => f.decryptWithKey(userKey, encryptService));


ℹ️ @bitwarden/team-platform-dev changed folder decryption to use decryptWithKey instead of the decrypt method.

gbubemismith · 2024-11-08T17:51:33Z

libs/common/src/vault/services/folder/folder.service.ts

+
+  folders$(userId$: Observable<UserId>): Observable<Folder[]> {
+    return userId$.pipe(
+      takeWhile((userId) => userId != null),


ℹ️ takeWhile operator to stop folder state observations when userId becomes invalid (during logout)

gbubemismith · 2024-11-08T17:54:08Z

libs/common/src/vault/services/folder/folder.service.ts

-    this.folderViews$ = this.decryptedFoldersState.state$;
+  folderViews$(userId$: Observable<UserId>): Observable<FolderView[]> {
+    return userId$.pipe(
+      takeWhile((userId) => userId != null),


ℹ️ takeWhile operator to stop folder state observations when userId becomes invalid (during logout)

libs/importer/src/components/import.component.ts

shane-melton

Nice work on this so far!

I mostly just left a couple thoughts/comments on my concern with mixing Observable<UserId> and UserId arguments to the FolderService. They're not hard change requests, but I think it something we should consider / discuss further.

Also, I do think we'll need to introduce the same UserId requirement to the FolderApiService to ensure we are always using the same userId throughout a submission.

shane-melton · 2024-11-14T23:48:13Z

libs/common/src/vault/services/folder/folder-api.service.ts

💭 We may need to update these methods to also accept a userId to ensure the userId is consistent in the calling location and within the api service.

For instance, the AddEditFolderDialog component has the following snippet in its save() method:

const activeUserId = await firstValueFrom(this.accountService.activeAccount$); const userKey = await this.keyService.getUserKeyWithLegacySupport(activeUserId.id); const folder = await this.folderService.encrypt(this.folder, userKey); // The activeAccount$ could have potentially changed during encryption, and the API service would use the updated userId await this.folderApiService.save(folder);

The CLI edit and create commands have similar saving behavior.

shane-melton · 2024-11-14T23:56:06Z

.../tools/export/vault-export/vault-export-core/src/services/individual-vault-export.service.ts

@@ -62,7 +64,7 @@ export class IndividualVaultExportService
    const promises = [];

    promises.push(
-      this.folderService.getAllDecryptedFromState().then((folders) => {
+      firstValueFrom(this.folderService.folderViews$(this.activeUserId$)).then((folders) => {


💭 Here's an example of where we would want folderViews$ to use a single UserId instead of an Observable<UserId>.

The cipherService.getAllDecrypted() call below will eventually be updated to a cipherViews$(userId) method. Now, if both ...Views$ observables take their own activeUserId$ observable as an input, there is a chance the activeUserId$ could emit in-between the calls to folderViews$() and cipherViews$(). If this were to happen, the folders and ciphers would belong to different users.

If we instead, set const activeUserId = await firstvalueFrom(this.activeUserId$) at the top of the method and pass it into each ...Views$(userId) from both services we can ensure the same user is being referenced by both.

shane-melton · 2024-11-15T00:05:45Z

libs/common/src/vault/services/folder/folder.service.ts


-    this.folderViews$ = this.decryptedFoldersState.state$;
+  folderViews$(userId$: Observable<UserId>): Observable<FolderView[]> {


💭 I'm hesitant to update this interface to accept an Observable<UserId>. There is a chance users of the service could be combining the results of folderViews$ with other service observables. For example:

combineLatest([ folderService.folderViews$(accountService.activeUserId$), cipherService.cipherViews$(accountService.activeUserId$) ])

If this happens and activeUserId$ emits, it could lead to folderView$() emitting before cipherViews$() (because folders are typically fewer/smaller so faster to decrypt than ciphers). If that happens then the combineLatest will emit with [newUserIdFolderViews, oldUserIdCipherViews] and the consumer could be inadvertently mixing user data.

By requiring a specific userId we help protect consumers / encourage them to ensure they have an unchanging userId for all of their services.

Open to other's thoughts on this. There was a related conversation in Slack here and a good StackBlitz example of the potential issue from Justin.

shane-melton · 2024-11-15T00:08:35Z

libs/common/src/vault/services/folder/folder.service.ts

@@ -58,29 +63,29 @@ export class FolderService implements InternalFolderServiceAbstraction {
    return folder;
  }

-  async get(id: string): Promise<Folder> {
-    const folders = await firstValueFrom(this.folders$);
+  async get(id: string, userId$: Observable<UserId>): Promise<Folder> {


💭 🎨 We're immediately taking the firstValueFrom this userId$ observable (via this.folders$()). Imo it would be more clear to simply accept an unchanging UserId.

shane-melton · 2024-11-15T00:11:28Z

libs/common/src/vault/services/folder/folder.service.ts

  }

-  async upsert(folderData: FolderData | FolderData[]): Promise<void> {
-    await this.encryptedFoldersState.update((folders) => {
+  async upsert(folderData: FolderData | FolderData[], userId: UserId): Promise<void> {


💭 This makes sense to me. It implies that the user of the service knows they're updating a specific user's folder and not just the latest from the activeUserId$ stream. But, now it seems kind of odd that we have some methods taking UserId and others taking Observable<UserId>. It makes me more inclined to be consistent and use UserId everywhere. What do you think?

shane-melton · 2024-11-15T00:20:37Z

apps/browser/src/vault/popup/settings/folders-v2.component.ts

  ) {
-    this.folders$ = this.folderService.folderViews$.pipe(
+    this.folders$ = this.folderService.folderViews$(this.activeUserId$).pipe(


💭 I'm not sure what we gain by making the folderViews$ reactive to / depend on the activeUserId$.

If the acitveUserId$ changes, we're almost certainly navigating away to another page (lock/logout) and the view will be destroyed very soon after. To avoid needing to await a firstValueFrom(this.activeUserId$) we could pipe/switchMap to the folderService.folderViews(activeUserId) instead.

gbubemismith · 2024-11-19T18:41:23Z

Nice work on this so far!

I mostly just left a couple thoughts/comments on my concern with mixing Observable<UserId> and UserId arguments to the FolderService. They're not hard change requests, but I think it something we should consider / discuss further.

Also, I do think we'll need to introduce the same UserId requirement to the FolderApiService to ensure we are always using the same userId throughout a submission.

Yeah I agree with simplifying it to a single user id. The simpler userId approach makes things more predictable 👍🏼

gbubemismith · 2024-11-19T18:46:30Z

libs/importer/src/components/import.component.ts

@@ -256,7 +259,10 @@ export class ImportComponent implements OnInit, OnDestroy, AfterViewInit {

  private handleImportInit() {
    // Filter out the no folder-item from folderViews$
-    this.folders$ = this.folderService.folderViews$.pipe(
+    this.folders$ = this.activeUserId$.pipe(


@audreyality, does folders$ need to react to user changes in this component or we could simplify since the component would be destroyed during user change

const activeUserId = await firstValueFrom(this.activeUserId$); this.folders$ = this.folderService.folderViews(userId) .pipe(map(folders => folders.filter(f => f.id != null)));

There's no guarantee that a component will be destroyed when the user changes; that's just how it works right now. I'd like to keep the complexity since observables are more resilient to change than promises.

In particular, many of our components are going to stop querying the active user and instead be handed a specific userId. That requires handling angular change events; the component isn't recreated.

It's worth noting that the main reason why this comes up is because this is library code. Applications make specific guarantees about construction and destruction. Libraries do not.

addisonbeck

LGTM, but a review from @justindbaur would be smart

Migrated folder service from using active user state to single user s…

df1caad

…tate Added extra test cases for encrypted folder and decrypted folders Updated derived state to use decrypt with key

This comment has been minimized.

Sign in to view

gbubemismith added 7 commits November 6, 2024 16:27

Update callers in the web

9011af9

Update callers in the browser

d56dfd9

Update callers in libs

2584759

Update callers in cli

c2cc509

Fixed test

07593b6

Fixed folder state test

bb0d489

Fixed test

3865ee6

gbubemismith added 3 commits November 7, 2024 10:22

removed duplicate activeUserId

735d009

Merge branch 'main' into vault/PM-12049

c1a915c

Added takewhile operator to only make calls when userId is present

8f25b65

gbubemismith commented Nov 8, 2024

View reviewed changes

gbubemismith marked this pull request as ready for review November 8, 2024 17:52

gbubemismith requested review from a team as code owners November 8, 2024 17:52

gbubemismith requested review from addisonbeck and shane-melton November 8, 2024 17:52

gbubemismith commented Nov 8, 2024

View reviewed changes

audreyality reviewed Nov 11, 2024

View reviewed changes

libs/importer/src/components/import.component.ts Outdated Show resolved Hide resolved

Merge branch 'main' into vault/PM-12049

faba3d0

gbubemismith requested a review from audreyality November 12, 2024 19:00

audreyality previously approved these changes Nov 12, 2024

View reviewed changes

shane-melton reviewed Nov 15, 2024

View reviewed changes

Simplified to accept a single user id instead of an observable

47364bf

gbubemismith dismissed audreyality’s stale review via 47364bf November 19, 2024 15:36

gbubemismith requested a review from a team as a code owner November 19, 2024 15:36

Fixed merge conflict

12a8501

gbubemismith commented Nov 19, 2024

View reviewed changes

gbubemismith requested review from shane-melton and audreyality November 19, 2024 20:54

audreyality approved these changes Nov 19, 2024

View reviewed changes

gbubemismith requested a review from justindbaur November 19, 2024 21:52

addisonbeck approved these changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-12049] Remove usage of ActiveUserState from folder service #11880

[PM-12049] Remove usage of ActiveUserState from folder service #11880

gbubemismith commented Nov 6, 2024 •

edited

Loading

This comment has been minimized.

codecov bot commented Nov 7, 2024 •

edited

Loading

gbubemismith Nov 8, 2024

gbubemismith Nov 8, 2024 •

edited

Loading

gbubemismith Nov 8, 2024 •

edited

Loading

shane-melton left a comment

shane-melton Nov 14, 2024

shane-melton Nov 14, 2024

shane-melton Nov 15, 2024

shane-melton Nov 15, 2024

shane-melton Nov 15, 2024

shane-melton Nov 15, 2024

gbubemismith commented Nov 19, 2024

gbubemismith Nov 19, 2024 •

edited

Loading

audreyality Nov 19, 2024 •

edited

Loading

addisonbeck left a comment


		this.folderViews$ = this.decryptedFoldersState.state$;
		folderViews$(userId$: Observable<UserId>): Observable<FolderView[]> {

[PM-12049] Remove usage of ActiveUserState from folder service #11880

Are you sure you want to change the base?

[PM-12049] Remove usage of ActiveUserState from folder service #11880

Conversation

gbubemismith commented Nov 6, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

This comment has been minimized.

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

gbubemismith Nov 8, 2024

Choose a reason for hiding this comment

gbubemismith Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

gbubemismith Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

shane-melton left a comment

Choose a reason for hiding this comment

shane-melton Nov 14, 2024

Choose a reason for hiding this comment

shane-melton Nov 14, 2024

Choose a reason for hiding this comment

shane-melton Nov 15, 2024

Choose a reason for hiding this comment

shane-melton Nov 15, 2024

Choose a reason for hiding this comment

shane-melton Nov 15, 2024

Choose a reason for hiding this comment

shane-melton Nov 15, 2024

Choose a reason for hiding this comment

gbubemismith commented Nov 19, 2024

gbubemismith Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

audreyality Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

addisonbeck left a comment

Choose a reason for hiding this comment

gbubemismith commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

gbubemismith Nov 8, 2024 •

edited

Loading

gbubemismith Nov 8, 2024 •

edited

Loading

gbubemismith Nov 19, 2024 •

edited

Loading

audreyality Nov 19, 2024 •

edited

Loading