Fix active account and searchBar observables/subscriptions (#3268)

* Change subscription to rely on observables and not on BehaviourSubject

* Ensure OnDestroy is added to AppComponent

* Fix check for no active accounts to redirect to the login page instead of lock

* Change subscription handling on SearchBarService

* Fix naming convention: Observables should have a $ suffix

* Remove obsolete linter hint

* Fix activeAccountUnlocked getting exposed as Observable but is instantiated as BehaviourSubject

apps/browser/src/popup/app.component.ts

@@ -56,7 +56,7 @@ export class AppComponent implements OnInit, OnDestroy {
     // Clear them aggressively to make sure this doesn't occur
     await this.clearComponentStates();
 
-    this.stateService.activeAccount.pipe(takeUntil(this.destroy$)).subscribe((userId) => {
+    this.stateService.activeAccount$.pipe(takeUntil(this.destroy$)).subscribe((userId) => {
       this.activeUserId = userId;
     });
 
@@ -84,7 +84,7 @@ export class AppComponent implements OnInit, OnDestroy {
               });
             }
 
-            if (this.stateService.activeAccount.getValue() == null) {
+            if (this.activeUserId === null) {
               this.router.navigate(["home"]);
             }
           });

apps/desktop/src/app/accounts/lock.component.ts

@@ -1,4 +1,4 @@
-import { Component, NgZone, OnDestroy } from "@angular/core";
+import { Component, NgZone } from "@angular/core";
 import { ActivatedRoute, Router } from "@angular/router";
 import { ipcRenderer } from "electron";
 
@@ -22,7 +22,7 @@ const BroadcasterSubscriptionId = "LockComponent";
   selector: "app-lock",
   templateUrl: "lock.component.html",
 })
-export class LockComponent extends BaseLockComponent implements OnDestroy {
+export class LockComponent extends BaseLockComponent {
   private deferFocus: boolean = null;
   authenicatedUrl = "vault";
   unAuthenicatedUrl = "update-temp-password";
@@ -103,6 +103,7 @@ export class LockComponent extends BaseLockComponent implements OnDestroy {
   }
 
   ngOnDestroy() {
+    super.ngOnDestroy();
     this.broadcasterService.unsubscribe(BroadcasterSubscriptionId);
   }
 

apps/desktop/src/app/app.component.ts

@@ -1,6 +1,7 @@
 import {
   Component,
   NgZone,
+  OnDestroy,
   OnInit,
   SecurityContext,
   Type,
@@ -77,7 +78,7 @@ const systemTimeoutOptions = {
     </div>
   `,
 })
-export class AppComponent implements OnInit {
+export class AppComponent implements OnInit, OnDestroy {
   @ViewChild("settings", { read: ViewContainerRef, static: true }) settingsRef: ViewContainerRef;
   @ViewChild("premium", { read: ViewContainerRef, static: true }) premiumRef: ViewContainerRef;
   @ViewChild("passwordHistory", { read: ViewContainerRef, static: true })
@@ -129,7 +130,7 @@ export class AppComponent implements OnInit {
   ) {}
 
   ngOnInit() {
-    this.stateService.activeAccount.pipe(takeUntil(this.destroy$)).subscribe((userId) => {
+    this.stateService.activeAccount$.pipe(takeUntil(this.destroy$)).subscribe((userId) => {
       this.activeUserId = userId;
     });
 

apps/desktop/src/app/layout/search/search-bar.service.ts

@@ -8,15 +8,16 @@ export type SearchBarState = {
 
 @Injectable()
 export class SearchBarService {
-  searchText = new BehaviorSubject<string>(null);
+  private searchTextSubject = new BehaviorSubject<string>(null);
+  searchText$ = this.searchTextSubject.asObservable();
 
   private _state = {
     enabled: false,
     placeholderText: "",
   };
 
-  // tslint:disable-next-line:member-ordering
+  private stateSubject = new BehaviorSubject<SearchBarState>(this._state);
-  state = new BehaviorSubject<SearchBarState>(this._state);
+  state$ = this.stateSubject.asObservable();
 
   setEnabled(enabled: boolean) {
     this._state.enabled = enabled;
@@ -29,10 +30,10 @@ export class SearchBarService {
   }
 
   setSearchText(value: string) {
-    this.searchText.next(value);
+    this.searchTextSubject.next(value);
   }
 
   private updateState() {
-    this.state.next(this._state);
+    this.stateSubject.next(this._state);
   }
 }

apps/desktop/src/app/layout/search/search.component.ts

@@ -1,5 +1,6 @@
 import { Component, OnDestroy, OnInit } from "@angular/core";
 import { UntypedFormControl } from "@angular/forms";
+import { Subscription } from "rxjs";
 
 import { StateService } from "@bitwarden/common/abstractions/state.service";
 
@@ -13,8 +14,10 @@ export class SearchComponent implements OnInit, OnDestroy {
   state: SearchBarState;
   searchText: UntypedFormControl = new UntypedFormControl(null);
 
+  private activeAccountSubscription: Subscription;
+
   constructor(private searchBarService: SearchBarService, private stateService: StateService) {
-    this.searchBarService.state.subscribe((state) => {
+    this.searchBarService.state$.subscribe((state) => {
       this.state = state;
     });
 
@@ -24,13 +27,13 @@ export class SearchComponent implements OnInit, OnDestroy {
   }
 
   ngOnInit() {
-    this.stateService.activeAccount.subscribe((value) => {
+    this.activeAccountSubscription = this.stateService.activeAccount$.subscribe((value) => {
       this.searchBarService.setSearchText("");
       this.searchText.patchValue("");
     });
   }
 
   ngOnDestroy() {
-    this.stateService.activeAccount.unsubscribe();
+    this.activeAccountSubscription.unsubscribe();
   }
 }

apps/desktop/src/app/send/send.component.ts

@@ -56,7 +56,7 @@ export class SendComponent extends BaseSendComponent implements OnInit, OnDestro
       policyService,
       logService
     );
-    this.searchBarService.searchText.subscribe((searchText) => {
+    this.searchBarService.searchText$.subscribe((searchText) => {
       this.searchText = searchText;
       this.searchTextChanged();
     });

apps/desktop/src/app/vault/ciphers.component.ts

@@ -14,7 +14,7 @@ export class CiphersComponent extends BaseCiphersComponent {
   constructor(searchService: SearchService, searchBarService: SearchBarService) {
     super(searchService);
 
-    searchBarService.searchText.subscribe((searchText) => {
+    searchBarService.searchText$.subscribe((searchText) => {
       this.searchText = searchText;
       this.search(200);
     });

libs/angular/src/components/lock.component.ts

@@ -1,5 +1,6 @@
-import { Directive, NgZone, OnInit } from "@angular/core";
+import { Directive, NgZone, OnDestroy, OnInit } from "@angular/core";
 import { Router } from "@angular/router";
+import { Subscription } from "rxjs";
 import { take } from "rxjs/operators";
 
 import { ApiService } from "@bitwarden/common/abstractions/api.service";
@@ -20,7 +21,7 @@ import { SymmetricCryptoKey } from "@bitwarden/common/models/domain/symmetricCry
 import { SecretVerificationRequest } from "@bitwarden/common/models/request/secretVerificationRequest";
 
 @Directive()
-export class LockComponent implements OnInit {
+export class LockComponent implements OnInit, OnDestroy {
   masterPassword = "";
   pin = "";
   showPassword = false;
@@ -39,6 +40,8 @@ export class LockComponent implements OnInit {
   private invalidPinAttempts = 0;
   private pinSet: [boolean, boolean];
 
+  private activeAccountSubscription: Subscription;
+
   constructor(
     protected router: Router,
     protected i18nService: I18nService,
@@ -57,11 +60,15 @@ export class LockComponent implements OnInit {
   async ngOnInit() {
     // Load the first and observe updates
     await this.load();
-    this.stateService.activeAccount.subscribe(async () => {
+    this.activeAccountSubscription = this.stateService.activeAccount$.subscribe(async () => {
       await this.load();
     });
   }
 
+  ngOnDestroy() {
+    this.activeAccountSubscription.unsubscribe();
+  }
+
   async submit() {
     if (this.pinLock && (this.pin == null || this.pin === "")) {
       this.platformUtilsService.showToast(

libs/common/spec/services/folder.service.spec.ts

@@ -32,7 +32,7 @@ describe("Folder Service", () => {
     stateService.getEncryptedFolders().resolves({
       "1": folderData("1", "test"),
     });
-    stateService.activeAccount.returns(activeAccount);
+    stateService.activeAccount$.returns(activeAccount);
     stateService.activeAccountUnlocked.returns(activeAccountUnlocked);
     (window as any).bitwardenContainerService = new ContainerService(cryptoService);
 

libs/common/src/abstractions/state.service.ts

@@ -26,9 +26,8 @@ import { SendView } from "../models/view/sendView";
 
 export abstract class StateService<T extends Account = Account> {
   accounts: BehaviorSubject<{ [userId: string]: T }>;
-  activeAccount: BehaviorSubject<string>;
+  activeAccount$: Observable<string>;
-
+  activeAccountUnlocked$: Observable<boolean>;
-  activeAccountUnlocked: Observable<boolean>;
 
   addAccount: (account: T) => Promise<void>;
   setActiveUser: (userId: string) => Promise<void>;

libs/common/src/services/environment.service.ts

@@ -22,7 +22,7 @@ export class EnvironmentService implements EnvironmentServiceAbstraction {
   private scimUrl: string = null;
 
   constructor(private stateService: StateService) {
-    this.stateService.activeAccount.subscribe(async () => {
+    this.stateService.activeAccount$.subscribe(async () => {
       await this.setUrlsFromStorage();
     });
   }

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

@@ -25,7 +25,7 @@ export class FolderService implements InternalFolderServiceAbstraction {
     private cipherService: CipherService,
     private stateService: StateService
   ) {
-    this.stateService.activeAccountUnlocked.subscribe(async (unlocked) => {
+    this.stateService.activeAccountUnlocked$.subscribe(async (unlocked) => {
       if ((Utils.global as any).bitwardenContainerService == null) {
         return;
       }

libs/common/src/services/state.service.ts

@@ -55,8 +55,11 @@ export class StateService<
 > implements StateServiceAbstraction<TAccount>
 {
   accounts = new BehaviorSubject<{ [userId: string]: TAccount }>({});
-  activeAccount = new BehaviorSubject<string>(null);
+  private activeAccountSubject = new BehaviorSubject<string>(null);
-  activeAccountUnlocked = new BehaviorSubject<boolean>(false);
+  activeAccount$ = this.activeAccountSubject.asObservable();
+
+  private activeAccountUnlockedSubject = new BehaviorSubject<boolean>(false);
+  activeAccountUnlocked$ = this.activeAccountUnlockedSubject.asObservable();
 
   private hasBeenInited = false;
   private isRecoveredSession = false;
@@ -73,17 +76,17 @@ export class StateService<
     protected useAccountCache: boolean = true
   ) {
     // If the account gets changed, verify the new account is unlocked
-    this.activeAccount.subscribe(async (userId) => {
+    this.activeAccountSubject.subscribe(async (userId) => {
-      if (userId == null && this.activeAccountUnlocked.getValue() == false) {
+      if (userId == null && this.activeAccountUnlockedSubject.getValue() == false) {
         return;
       } else if (userId == null) {
-        this.activeAccountUnlocked.next(false);
+        this.activeAccountUnlockedSubject.next(false);
       }
 
       // FIXME: This should be refactored into AuthService or a similar service,
       //  as checking for the existance of the crypto key is a low level
       //  implementation detail.
-      this.activeAccountUnlocked.next((await this.getCryptoMasterKey()) != null);
+      this.activeAccountUnlockedSubject.next((await this.getCryptoMasterKey()) != null);
     });
   }
 
@@ -125,7 +128,7 @@ export class StateService<
         state.activeUserId = storedActiveUser;
       }
       await this.pushAccounts();
-      this.activeAccount.next(state.activeUserId);
+      this.activeAccountSubject.next(state.activeUserId);
 
       return state;
     });
@@ -154,7 +157,7 @@ export class StateService<
     await this.scaffoldNewAccountStorage(account);
     await this.setLastActive(new Date().getTime(), { userId: account.profile.userId });
     await this.setActiveUser(account.profile.userId);
-    this.activeAccount.next(account.profile.userId);
+    this.activeAccountSubject.next(account.profile.userId);
   }
 
   async setActiveUser(userId: string): Promise<void> {
@@ -162,7 +165,7 @@ export class StateService<
     await this.updateState(async (state) => {
       state.activeUserId = userId;
       await this.storageService.save(keys.activeUserId, userId);
-      this.activeAccount.next(state.activeUserId);
+      this.activeAccountSubject.next(state.activeUserId);
       return state;
     });
 
@@ -497,12 +500,12 @@ export class StateService<
       this.reconcileOptions(options, await this.defaultInMemoryOptions())
     );
 
-    if (options.userId == this.activeAccount.getValue()) {
+    if (options.userId == this.activeAccountSubject.getValue()) {
       const nextValue = value != null;
 
       // Avoid emitting if we are already unlocked
-      if (this.activeAccountUnlocked.getValue() != nextValue) {
+      if (this.activeAccountUnlockedSubject.getValue() != nextValue) {
-        this.activeAccountUnlocked.next(nextValue);
+        this.activeAccountUnlockedSubject.next(nextValue);
       }
     }
   }