remove password component refactor, test coverage, enabled strict

apps/browser/src/auth/popup/remove-password.component.html

@@ -13,12 +13,7 @@
         <p>{{ "convertOrganizationEncryptionDesc" | i18n: organization.name }}</p>
       </div>
       <div class="box-content-row">
-        <button
-          type="button"
-          class="btn block primary"
-          (click)="convert()"
-          [disabled]="actionPromise"
-        >
+        <button type="button" class="btn block primary" (click)="convert()" [disabled]="action">
           <i
             class="bwi bwi-spinner bwi-spin"
             title="{{ 'loading' | i18n }}"
@@ -33,7 +28,7 @@
           type="button"
           class="btn btn-outline-secondary block"
           (click)="leave()"
-          [disabled]="actionPromise"
+          [disabled]="action"
         >
           <i
             class="bwi bwi-spinner bwi-spin"

apps/desktop/src/auth/remove-password.component.html

@@ -3,21 +3,11 @@
     <h1>{{ "removeMasterPassword" | i18n }}</h1>
     <p>{{ "convertOrganizationEncryptionDesc" | i18n: organization.name }}</p>
     <div class="buttons">
-      <button
-        type="submit"
-        class="btn primary block"
-        [disabled]="actionPromise"
-        (click)="convert()"
-      >
+      <button type="submit" class="btn primary block" [disabled]="action" (click)="convert()">
         <b [hidden]="continuing">{{ "removeMasterPassword" | i18n }}</b>
         <i class="bwi bwi-spinner bwi-spin" [hidden]="!continuing" aria-hidden="true"></i>
       </button>
-      <button
-        type="button"
-        class="btn secondary block"
-        [disabled]="actionPromise"
-        (click)="leave()"
-      >
+      <button type="button" class="btn secondary block" [disabled]="action" (click)="leave()">
         <b [hidden]="leaving">{{ "leaveOrganization" | i18n }}</b>
         <i class="bwi bwi-spinner bwi-spin" [hidden]="!leaving" aria-hidden="true"></i>
       </button>

libs/angular/src/auth/components/remove-password.component.spec.ts

@@ -0,0 +1,197 @@
+import { Router } from "@angular/router";
+import { mock } from "jest-mock-extended";
+
+import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction";
+import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
+import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service";
+import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { SyncService } from "@bitwarden/common/platform/sync";
+import { mockAccountServiceWith } from "@bitwarden/common/spec";
+import { UserId } from "@bitwarden/common/types/guid";
+import { DialogService, ToastService } from "@bitwarden/components";
+
+import { RemovePasswordComponent } from "./remove-password.component";
+
+describe("RemovePasswordComponent", () => {
+  let component: RemovePasswordComponent;
+
+  const userId = "test-user-id" as UserId;
+  const organization = {
+    id: "test-organization-id",
+    name: "test-organization-name",
+  } as Organization;
+
+  const accountService = mockAccountServiceWith(userId);
+
+  const mockRouter = mock<Router>();
+  const mockSyncService = mock<SyncService>();
+  const mockI18nService = mock<I18nService>();
+  const mockKeyConnectorService = mock<KeyConnectorService>();
+  const mockOrganizationApiService = mock<OrganizationApiServiceAbstraction>();
+  const mockDialogService = mock<DialogService>();
+  const mockToastService = mock<ToastService>();
+
+  beforeEach(async () => {
+    jest.clearAllMocks();
+
+    await accountService.switchAccount(userId);
+
+    component = new RemovePasswordComponent(
+      mockRouter,
+      accountService,
+      mockSyncService,
+      mockI18nService,
+      mockKeyConnectorService,
+      mockOrganizationApiService,
+      mockDialogService,
+      mockToastService,
+    );
+  });
+
+  describe("ngOnInit", () => {
+    it("should set activeUserId and organization", async () => {
+      mockKeyConnectorService.getManagingOrganization.mockResolvedValue(organization);
+
+      await component.ngOnInit();
+
+      expect(component["activeUserId"]).toBe("test-user-id");
+      expect(component.organization).toEqual(organization);
+      expect(component.loading).toEqual(false);
+
+      expect(mockKeyConnectorService.getManagingOrganization).toHaveBeenCalledWith(userId);
+      expect(mockSyncService.fullSync).toHaveBeenCalledWith(false);
+    });
+
+    it("should throw an error if no active account is found", async () => {
+      await accountService.switchAccount(null as unknown as UserId);
+
+      await expect(component.ngOnInit()).rejects.toThrow(new Error("No active account found"));
+    });
+
+    it("should throw an error if no organization is found", async () => {
+      mockKeyConnectorService.getManagingOrganization.mockResolvedValue(
+        null as unknown as Organization,
+      );
+
+      await expect(component.ngOnInit()).rejects.toThrow(new Error("No organization found"));
+    });
+  });
+
+  describe("get action", () => {
+    it.each([
+      [true, false],
+      [false, true],
+      [true, true],
+    ])(
+      "should return true when continuing is $continuing and leaving is $leaving",
+      (continuing, leaving) => {
+        component.continuing = continuing;
+        component.leaving = leaving;
+
+        expect(component.action).toBe(true);
+      },
+    );
+
+    it("should return false when continuing and leaving are both false", () => {
+      component.continuing = false;
+      component.leaving = false;
+
+      expect(component.action).toBe(false);
+    });
+  });
+
+  describe("convert", () => {
+    beforeEach(async () => {
+      mockKeyConnectorService.getManagingOrganization.mockResolvedValue(organization);
+
+      await component.ngOnInit();
+    });
+
+    it("should call migrateUser and show success toast", async () => {
+      mockI18nService.t.mockReturnValue("removed master password");
+
+      await component.convert();
+
+      expect(component.continuing).toBe(true);
+      expect(mockKeyConnectorService.migrateUser).toHaveBeenCalledWith(userId);
+      expect(mockKeyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId);
+      expect(mockToastService.showToast).toHaveBeenCalledWith({
+        variant: "success",
+        message: "removed master password",
+      });
+      expect(mockRouter.navigate).toHaveBeenCalledWith([""]);
+    });
+
+    it("should handle errors and show error toast", async () => {
+      const errorMessage = "Can't migrate user error";
+      mockKeyConnectorService.migrateUser.mockRejectedValue(new Error(errorMessage));
+      mockI18nService.t.mockReturnValue("error occurred");
+
+      await component.convert();
+
+      expect(component.continuing).toBe(false);
+      expect(mockKeyConnectorService.migrateUser).toHaveBeenCalledWith(userId);
+      expect(mockToastService.showToast).toHaveBeenCalledWith({
+        variant: "error",
+        title: "error occurred",
+        message: errorMessage,
+      });
+      expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled();
+      expect(mockRouter.navigate).not.toHaveBeenCalled();
+    });
+  });
+
+  describe("leave", () => {
+    beforeEach(async () => {
+      mockKeyConnectorService.getManagingOrganization.mockResolvedValue(organization);
+
+      await component.ngOnInit();
+    });
+
+    it("should call leave and show success toast", async () => {
+      mockDialogService.openSimpleDialog.mockResolvedValue(true);
+      mockI18nService.t.mockReturnValue("left organization");
+
+      await component.leave();
+
+      expect(component.leaving).toBe(true);
+      expect(mockOrganizationApiService.leave).toHaveBeenCalledWith(organization.id);
+      expect(mockToastService.showToast).toHaveBeenCalledWith({
+        variant: "success",
+        message: "left organization",
+      });
+      expect(mockKeyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId);
+      expect(mockRouter.navigate).toHaveBeenCalledWith([""]);
+    });
+
+    it("should handle errors and show error toast", async () => {
+      const errorMessage = "Can't leave organization error";
+      mockDialogService.openSimpleDialog.mockResolvedValue(true);
+      mockOrganizationApiService.leave.mockRejectedValue(new Error(errorMessage));
+      mockI18nService.t.mockReturnValue("error occurred");
+
+      await component.leave();
+
+      expect(component.leaving).toBe(false);
+      expect(mockOrganizationApiService.leave).toHaveBeenCalledWith(organization.id);
+      expect(mockToastService.showToast).toHaveBeenCalledWith({
+        variant: "error",
+        title: "error occurred",
+        message: errorMessage,
+      });
+      expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled();
+      expect(mockRouter.navigate).not.toHaveBeenCalled();
+    });
+
+    it("should not call leave if dialog is canceled", async () => {
+      mockDialogService.openSimpleDialog.mockResolvedValue(false);
+
+      await component.leave();
+
+      expect(component.leaving).toBe(false);
+      expect(mockOrganizationApiService.leave).not.toHaveBeenCalled();
+      expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled();
+      expect(mockRouter.navigate).not.toHaveBeenCalled();
+    });
+  });
+});

libs/angular/src/auth/components/remove-password.component.ts

@@ -1,33 +1,29 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import { Directive, OnInit } from "@angular/core";
 import { Router } from "@angular/router";
 import { firstValueFrom } from "rxjs";
 
 import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction";
 import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
-import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
-import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
+import { UserId } from "@bitwarden/common/types/guid";
 import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction";
 import { DialogService, ToastService } from "@bitwarden/components";
 
 @Directive()
 export class RemovePasswordComponent implements OnInit {
-  actionPromise: Promise<void | boolean>;
   continuing = false;
   leaving = false;
 
   loading = true;
-  organization: Organization;
-  migratingUser: Account;
+  organization!: Organization;
+  private activeUserId!: UserId;
 
   constructor(
     private router: Router,
     private accountService: AccountService,
     private syncService: SyncService,
-    private platformUtilsService: PlatformUtilsService,
     private i18nService: I18nService,
     private keyConnectorService: KeyConnectorService,
     private organizationApiService: OrganizationApiServiceAbstraction,
@@ -36,35 +32,47 @@ export class RemovePasswordComponent implements OnInit {
   ) {}
 
   async ngOnInit() {
-    this.migratingUser = await firstValueFrom(this.accountService.activeAccount$);
-    this.organization = await this.keyConnectorService.getManagingOrganization(
-      this.migratingUser.id,
-    );
+    const activeAccount = await firstValueFrom(this.accountService.activeAccount$);
+    if (activeAccount == null) {
+      throw new Error("No active account found");
+    }
+    this.activeUserId = activeAccount.id;
+
+    this.organization = await this.keyConnectorService.getManagingOrganization(this.activeUserId);
+    if (this.organization == null) {
+      throw new Error("No organization found");
+    }
     await this.syncService.fullSync(false);
     this.loading = false;
   }
 
+  get action() {
+    return this.continuing || this.leaving;
+  }
+
   convert = async () => {
     this.continuing = true;
-    this.actionPromise = this.keyConnectorService.migrateUser(this.migratingUser.id);
 
     try {
-      await this.actionPromise;
+      await this.keyConnectorService.migrateUser(this.activeUserId);
+
       this.toastService.showToast({
         variant: "success",
-        title: null,
         message: this.i18nService.t("removedMasterPassword"),
       });
-      await this.keyConnectorService.removeConvertAccountRequired(this.migratingUser.id);
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      this.router.navigate([""]);
+      await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId);
+
+      await this.router.navigate([""]);
     } catch (e) {
-      this.toastService.showToast({
-        variant: "error",
-        title: this.i18nService.t("errorOccurred"),
-        message: e.message,
-      });
+      this.continuing = false;
+
+      if (e instanceof Error) {
+        this.toastService.showToast({
+          variant: "error",
+          title: this.i18nService.t("errorOccurred"),
+          message: e.message,
+        });
+      }
     }
   };
 
@@ -79,25 +87,27 @@ export class RemovePasswordComponent implements OnInit {
       return false;
     }
 
+    this.leaving = true;
     try {
-      this.leaving = true;
-      this.actionPromise = this.organizationApiService.leave(this.organization.id);
-      await this.actionPromise;
+      await this.organizationApiService.leave(this.organization.id);
+
       this.toastService.showToast({
         variant: "success",
-        title: null,
         message: this.i18nService.t("leftOrganization"),
       });
-      await this.keyConnectorService.removeConvertAccountRequired(this.migratingUser.id);
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      this.router.navigate([""]);
+      await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId);
+
+      await this.router.navigate([""]);
     } catch (e) {
-      this.toastService.showToast({
-        variant: "error",
-        title: this.i18nService.t("errorOccurred"),
-        message: e,
-      });
+      this.leaving = false;
+
+      if (e instanceof Error) {
+        this.toastService.showToast({
+          variant: "error",
+          title: this.i18nService.t("errorOccurred"),
+          message: e.message,
+        });
+      }
     }
   };
 }