From 7b8507cf9b46d11bad472ee1d1a34454dce3076c Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Tue, 1 Nov 2022 11:25:46 +0100 Subject: [PATCH] [EC-522] Improve handling of rxjs subjects (#3772) * [EC-522] feat: no public rxjs subjects * [EC-522] feat: improve null handling * [EC-552] fix: init subject with empty set instead of null * [EC-552] fix: don't push null into account subject * [EC-522] feat: remove null filter --- .eslintrc.json | 1 + .../browser-session.decorator.spec.ts | 2 +- .../session-sync.decorator.spec.ts | 8 ++++++-- apps/browser/src/popup/vault/vault-select.component.ts | 4 ++-- apps/desktop/src/app/app.component.ts | 9 +++++---- apps/desktop/src/app/guards/login.guard.ts | 3 ++- .../src/app/layout/account-switcher.component.ts | 2 +- .../src/app/layout/search/search-bar.service.ts | 2 +- .../src/services/encryptedMessageHandlerService.ts | 4 +++- apps/desktop/src/services/nativeMessaging.service.ts | 4 +++- .../vault/vault-filter/shared/vault-filter.service.ts | 2 +- libs/common/src/abstractions/state.service.ts | 4 ++-- libs/common/src/services/state.service.ts | 10 ++++++---- libs/common/src/services/system.service.ts | 6 ++++-- .../src/services/vaultTimeout/vaultTimeout.service.ts | 5 ++++- 15 files changed, 42 insertions(+), 24 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 14b9a88844..6a5fc5b83f 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -68,6 +68,7 @@ } ], "rxjs-angular/prefer-takeuntil": "error", + "rxjs/no-exposed-subjects": ["error", { "allowProtected": true }], "no-restricted-syntax": [ "error", { diff --git a/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts b/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts index b48658efa3..cc8a561876 100644 --- a/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts +++ b/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts @@ -38,7 +38,7 @@ describe("browserSession decorator", () => { @browserSession class TestClass { @sessionSync({ initializer: (s: string) => s }) - behaviorSubject = new BehaviorSubject(""); + private behaviorSubject = new BehaviorSubject(""); constructor(private stateService: StateService) {} diff --git a/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.spec.ts b/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.spec.ts index b177d118f8..38ec7aac9a 100644 --- a/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.spec.ts +++ b/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.spec.ts @@ -7,7 +7,11 @@ describe("sessionSync decorator", () => { const ctor = String; class TestClass { @sessionSync({ ctor: ctor, initializer: initializer }) - testProperty = new BehaviorSubject(""); + private testProperty = new BehaviorSubject(""); + + complete() { + this.testProperty.complete(); + } } it("should add __syncedItemKeys to prototype", () => { @@ -19,7 +23,7 @@ describe("sessionSync decorator", () => { ctor: ctor, initializer: initializer, }), - testClass.testProperty.complete(), + testClass.complete(), ]); }); }); diff --git a/apps/browser/src/popup/vault/vault-select.component.ts b/apps/browser/src/popup/vault/vault-select.component.ts index 04ae04d0fa..57daa89496 100644 --- a/apps/browser/src/popup/vault/vault-select.component.ts +++ b/apps/browser/src/popup/vault/vault-select.component.ts @@ -54,12 +54,12 @@ export class VaultSelectComponent implements OnInit, OnDestroy { buttonRef: ElementRef; @ViewChild("vaultSelectorTemplate", { read: TemplateRef }) templateRef: TemplateRef; - private _selectedVault = new BehaviorSubject(null); + private _selectedVault = new BehaviorSubject(null); isOpen = false; loaded = false; organizations$: Observable; - selectedVault$: Observable = this._selectedVault.asObservable(); + selectedVault$: Observable = this._selectedVault.asObservable(); vaultFilter: VaultFilter = new VaultFilter(); enforcePersonalOwnership = false; diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index 0541e5493c..82753a1776 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -11,7 +11,7 @@ import { import { DomSanitizer } from "@angular/platform-browser"; import { Router } from "@angular/router"; import { IndividualConfig, ToastrService } from "ngx-toastr"; -import { Subject, takeUntil } from "rxjs"; +import { firstValueFrom, Subject, takeUntil } from "rxjs"; import { ModalRef } from "@bitwarden/angular/components/modal/modal.ref"; import { ModalService } from "@bitwarden/angular/services/modal.service"; @@ -175,7 +175,7 @@ export class AppComponent implements OnInit, OnDestroy { await this.vaultTimeoutService.lock(message.userId); break; case "lockAllVaults": - for (const userId in this.stateService.accounts.getValue()) { + for (const userId in await firstValueFrom(this.stateService.accounts$)) { if (userId != null) { await this.vaultTimeoutService.lock(userId); } @@ -429,7 +429,7 @@ export class AppComponent implements OnInit, OnDestroy { private async updateAppMenu() { let updateRequest: MenuUpdateRequest; - const stateAccounts = this.stateService.accounts?.getValue(); + const stateAccounts = await firstValueFrom(this.stateService.accounts$); if (stateAccounts == null || Object.keys(stateAccounts).length < 1) { updateRequest = { accounts: null, @@ -593,7 +593,8 @@ export class AppComponent implements OnInit, OnDestroy { } private async checkForSystemTimeout(timeout: number): Promise { - for (const userId in this.stateService.accounts.getValue()) { + const accounts = await firstValueFrom(this.stateService.accounts$); + for (const userId in accounts) { if (userId == null) { continue; } diff --git a/apps/desktop/src/app/guards/login.guard.ts b/apps/desktop/src/app/guards/login.guard.ts index 4957f80c8c..b4dd5e485a 100644 --- a/apps/desktop/src/app/guards/login.guard.ts +++ b/apps/desktop/src/app/guards/login.guard.ts @@ -1,5 +1,6 @@ import { Injectable } from "@angular/core"; import { CanActivate } from "@angular/router"; +import { firstValueFrom } from "rxjs"; import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; @@ -17,7 +18,7 @@ export class LoginGuard implements CanActivate { ) {} async canActivate() { - const accounts = this.stateService.accounts.getValue(); + const accounts = await firstValueFrom(this.stateService.accounts$); if (accounts != null && Object.keys(accounts).length >= maxAllowedAccounts) { this.platformUtilsService.showToast("error", null, this.i18nService.t("accountLimitReached")); return false; diff --git a/apps/desktop/src/app/layout/account-switcher.component.ts b/apps/desktop/src/app/layout/account-switcher.component.ts index 0c1ca74bd5..495c461a42 100644 --- a/apps/desktop/src/app/layout/account-switcher.component.ts +++ b/apps/desktop/src/app/layout/account-switcher.component.ts @@ -97,7 +97,7 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { ) {} async ngOnInit(): Promise { - this.stateService.accounts + this.stateService.accounts$ .pipe( concatMap(async (accounts: { [userId: string]: Account }) => { for (const userId in accounts) { diff --git a/apps/desktop/src/app/layout/search/search-bar.service.ts b/apps/desktop/src/app/layout/search/search-bar.service.ts index 24f3d0513f..5e12af5e10 100644 --- a/apps/desktop/src/app/layout/search/search-bar.service.ts +++ b/apps/desktop/src/app/layout/search/search-bar.service.ts @@ -8,7 +8,7 @@ export type SearchBarState = { @Injectable() export class SearchBarService { - private searchTextSubject = new BehaviorSubject(null); + private searchTextSubject = new BehaviorSubject(null); searchText$ = this.searchTextSubject.asObservable(); private _state = { diff --git a/apps/desktop/src/services/encryptedMessageHandlerService.ts b/apps/desktop/src/services/encryptedMessageHandlerService.ts index 719b6bed08..f5ac1e5fce 100644 --- a/apps/desktop/src/services/encryptedMessageHandlerService.ts +++ b/apps/desktop/src/services/encryptedMessageHandlerService.ts @@ -1,3 +1,5 @@ +import { firstValueFrom } from "rxjs"; + import { AuthService } from "@bitwarden/common/abstractions/auth.service"; import { CipherService } from "@bitwarden/common/abstractions/cipher.service"; import { MessagingService } from "@bitwarden/common/abstractions/messaging.service"; @@ -78,7 +80,7 @@ export class EncryptedMessageHandlerService { } private async statusCommandHandler(): Promise { - const accounts = this.stateService.accounts.getValue(); + const accounts = await firstValueFrom(this.stateService.accounts$); const activeUserId = await this.stateService.getUserId(); if (!accounts || !Object.keys(accounts)) { diff --git a/apps/desktop/src/services/nativeMessaging.service.ts b/apps/desktop/src/services/nativeMessaging.service.ts index 484522d7a5..83e3705dc7 100644 --- a/apps/desktop/src/services/nativeMessaging.service.ts +++ b/apps/desktop/src/services/nativeMessaging.service.ts @@ -1,5 +1,6 @@ import { Injectable } from "@angular/core"; import { ipcRenderer } from "electron"; +import { firstValueFrom } from "rxjs"; import Swal from "sweetalert2"; import { CryptoService } from "@bitwarden/common/abstractions/crypto.service"; @@ -58,7 +59,8 @@ export class NativeMessagingService { const remotePublicKey = Utils.fromB64ToArray(rawMessage.publicKey).buffer; // Validate the UserId to ensure we are logged into the same account. - const userIds = Object.keys(this.stateService.accounts.getValue()); + const accounts = await firstValueFrom(this.stateService.accounts$); + const userIds = Object.keys(accounts); if (!userIds.includes(rawMessage.userId)) { ipcRenderer.send("nativeMessagingReply", { command: "wrongUserId", appId: appId }); return; diff --git a/apps/web/src/app/vault/vault-filter/shared/vault-filter.service.ts b/apps/web/src/app/vault/vault-filter/shared/vault-filter.service.ts index 9ae665b898..2ebc64e5d7 100644 --- a/apps/web/src/app/vault/vault-filter/shared/vault-filter.service.ts +++ b/apps/web/src/app/vault/vault-filter/shared/vault-filter.service.ts @@ -18,7 +18,7 @@ import { CollectionView } from "@bitwarden/common/models/view/collection.view"; @Injectable() export class VaultFilterService extends BaseVaultFilterService { - private _collapsedFilterNodes = new BehaviorSubject>(null); + private _collapsedFilterNodes = new BehaviorSubject>(new Set()); collapsedFilterNodes$: Observable> = this._collapsedFilterNodes.asObservable(); constructor( diff --git a/libs/common/src/abstractions/state.service.ts b/libs/common/src/abstractions/state.service.ts index ccd9be514f..2c0712d029 100644 --- a/libs/common/src/abstractions/state.service.ts +++ b/libs/common/src/abstractions/state.service.ts @@ -1,4 +1,4 @@ -import { BehaviorSubject, Observable } from "rxjs"; +import { Observable } from "rxjs"; import { KdfType } from "../enums/kdfType"; import { ThemeType } from "../enums/themeType"; @@ -27,7 +27,7 @@ import { CollectionView } from "../models/view/collection.view"; import { SendView } from "../models/view/send.view"; export abstract class StateService { - accounts: BehaviorSubject<{ [userId: string]: T }>; + accounts$: Observable<{ [userId: string]: T }>; activeAccount$: Observable; activeAccountUnlocked$: Observable; diff --git a/libs/common/src/services/state.service.ts b/libs/common/src/services/state.service.ts index 11751b9ec8..0c1c3b725a 100644 --- a/libs/common/src/services/state.service.ts +++ b/libs/common/src/services/state.service.ts @@ -65,8 +65,10 @@ export class StateService< TAccount extends Account = Account > implements StateServiceAbstraction { - accounts = new BehaviorSubject<{ [userId: string]: TAccount }>({}); - private activeAccountSubject = new BehaviorSubject(null); + private accountsSubject = new BehaviorSubject<{ [userId: string]: TAccount }>({}); + accounts$ = this.accountsSubject.asObservable(); + + private activeAccountSubject = new BehaviorSubject(null); activeAccount$ = this.activeAccountSubject.asObservable(); private activeAccountUnlockedSubject = new BehaviorSubject(false); @@ -2530,11 +2532,11 @@ export class StateService< await this.pruneInMemoryAccounts(); await this.state().then((state) => { if (state.accounts == null || Object.keys(state.accounts).length < 1) { - this.accounts.next(null); + this.accountsSubject.next({}); return; } - this.accounts.next(state.accounts); + this.accountsSubject.next(state.accounts); }); } diff --git a/libs/common/src/services/system.service.ts b/libs/common/src/services/system.service.ts index 643f00831c..ee2dbf33b5 100644 --- a/libs/common/src/services/system.service.ts +++ b/libs/common/src/services/system.service.ts @@ -1,3 +1,5 @@ +import { firstValueFrom } from "rxjs"; + import { AuthService } from "../abstractions/auth.service"; import { MessagingService } from "../abstractions/messaging.service"; import { PlatformUtilsService } from "../abstractions/platformUtils.service"; @@ -19,7 +21,7 @@ export class SystemService implements SystemServiceAbstraction { ) {} async startProcessReload(authService: AuthService): Promise { - const accounts = this.stateService.accounts.getValue(); + const accounts = await firstValueFrom(this.stateService.accounts$); if (accounts != null) { const keys = Object.keys(accounts); if (keys.length > 0) { @@ -56,7 +58,7 @@ export class SystemService implements SystemServiceAbstraction { } private async executeProcessReload() { - const accounts = this.stateService.accounts.getValue(); + const accounts = await firstValueFrom(this.stateService.accounts$); const doRefresh = accounts == null || Object.keys(accounts).length == 0 || diff --git a/libs/common/src/services/vaultTimeout/vaultTimeout.service.ts b/libs/common/src/services/vaultTimeout/vaultTimeout.service.ts index f4a9e0330c..79521d686a 100644 --- a/libs/common/src/services/vaultTimeout/vaultTimeout.service.ts +++ b/libs/common/src/services/vaultTimeout/vaultTimeout.service.ts @@ -1,3 +1,5 @@ +import { firstValueFrom } from "rxjs"; + import { AuthService } from "../../abstractions/auth.service"; import { CipherService } from "../../abstractions/cipher.service"; import { CollectionService } from "../../abstractions/collection.service"; @@ -52,7 +54,8 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { return; } - for (const userId in this.stateService.accounts.getValue()) { + const accounts = await firstValueFrom(this.stateService.accounts$); + for (const userId in accounts) { if (userId != null && (await this.shouldLock(userId))) { await this.executeTimeoutAction(userId); }