mirror of
https://github.com/bitwarden/browser
synced 2025-12-19 09:43:23 +00:00
[PM-18576] Fix missing user id on remove password (#13777)
* Passed in userId on RemovePasswordComponent. * Added userId on other references to KeyConnectorService methods * remove password component refactor, test coverage, enabled strict * explicit user id provided to key connector service * redirect to / instead when user not logged in or not managing organization * key connector service explicit user id * key connector service no longer requires account service * key connector service missing null type * cli convert to key connector unit tests * remove unnecessary SyncService * error toast not showing on ErrorResponse * bad import due to merge conflict * bad import due to merge conflict * missing loading in remove password component for browser extension * error handling in remove password component * organization observable race condition in key-connector * usesKeyConnector always returns boolean * unit test coverage * key connector reactive * reactive key connector service * introducing convertAccountRequired$ * cli build fix * moving message sending side effect to sync * key connector service unit tests * fix unit tests * unit tests in wrong place after KM code ownership move * infinite page reload * failing unit tests * failing unit tests --------- Co-authored-by: Todd Martin <tmartin@bitwarden.com>
This commit is contained in:
Maciej Zieniuk
GitHub
@@ -0,0 +1,252 @@
|
||||
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/key-management/key-connector/abstractions/key-connector.service";
|
||||
import { ErrorResponse } from "@bitwarden/common/models/response/error.response";
|
||||
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
|
||||
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
|
||||
import { SyncService } from "@bitwarden/common/platform/sync";
|
||||
import { UserId } from "@bitwarden/common/types/guid";
|
||||
import { DialogService, ToastService } from "@bitwarden/components";
|
||||
|
||||
import { mockAccountServiceWith } from "../../../common/spec";
|
||||
|
||||
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(
|
||||
mock<LogService>(),
|
||||
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);
|
||||
expect(mockRouter.navigate).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should redirect to login when no active account is found", async () => {
|
||||
await accountService.switchAccount(null as unknown as UserId);
|
||||
|
||||
await component.ngOnInit();
|
||||
|
||||
expect(mockRouter.navigate).toHaveBeenCalledWith(["/"]);
|
||||
});
|
||||
|
||||
it("should redirect to login when no organization is found", async () => {
|
||||
mockKeyConnectorService.getManagingOrganization.mockResolvedValue(
|
||||
null as unknown as Organization,
|
||||
);
|
||||
|
||||
await component.ngOnInit();
|
||||
|
||||
expect(mockKeyConnectorService.getManagingOrganization).toHaveBeenCalledWith(userId);
|
||||
expect(mockSyncService.fullSync).toHaveBeenCalledWith(false);
|
||||
expect(mockRouter.navigate).toHaveBeenCalledWith(["/"]);
|
||||
});
|
||||
});
|
||||
|
||||
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(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(mockRouter.navigate).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should handle error response and show error toast", async () => {
|
||||
const errorMessage = "Can't migrate user error";
|
||||
mockKeyConnectorService.migrateUser.mockRejectedValue(
|
||||
new ErrorResponse(
|
||||
{
|
||||
message: errorMessage,
|
||||
},
|
||||
404,
|
||||
),
|
||||
);
|
||||
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(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(mockRouter.navigate).toHaveBeenCalledWith(["/"]);
|
||||
});
|
||||
|
||||
it("should handle error response 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(mockRouter.navigate).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should handle error response and show error toast", async () => {
|
||||
const errorMessage = "Can't leave organization error";
|
||||
mockDialogService.openSimpleDialog.mockResolvedValue(true);
|
||||
mockOrganizationApiService.leave.mockRejectedValue(
|
||||
new ErrorResponse(
|
||||
{
|
||||
message: errorMessage,
|
||||
},
|
||||
404,
|
||||
),
|
||||
);
|
||||
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(mockRouter.navigate).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should not call leave when dialog is canceled", async () => {
|
||||
mockDialogService.openSimpleDialog.mockResolvedValue(false);
|
||||
|
||||
await component.leave();
|
||||
|
||||
expect(component.leaving).toBe(false);
|
||||
expect(mockOrganizationApiService.leave).not.toHaveBeenCalled();
|
||||
expect(mockRouter.navigate).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -1,33 +1,32 @@
|
||||
// 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, map } from "rxjs";
|
||||
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 { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
|
||||
import { KeyConnectorService } from "@bitwarden/common/key-management/key-connector/abstractions/key-connector.service";
|
||||
import { ErrorResponse } from "@bitwarden/common/models/response/error.response";
|
||||
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
|
||||
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
|
||||
import { LogService } from "@bitwarden/common/platform/abstractions/log.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;
|
||||
email: string;
|
||||
organization!: Organization;
|
||||
private activeUserId!: UserId;
|
||||
|
||||
constructor(
|
||||
private logService: LogService,
|
||||
private router: Router,
|
||||
private accountService: AccountService,
|
||||
private syncService: SyncService,
|
||||
private platformUtilsService: PlatformUtilsService,
|
||||
private i18nService: I18nService,
|
||||
private keyConnectorService: KeyConnectorService,
|
||||
private organizationApiService: OrganizationApiServiceAbstraction,
|
||||
@@ -36,35 +35,49 @@ export class RemovePasswordComponent implements OnInit {
|
||||
) {}
|
||||
|
||||
async ngOnInit() {
|
||||
this.organization = await this.keyConnectorService.getManagingOrganization();
|
||||
this.email = await firstValueFrom(
|
||||
this.accountService.activeAccount$.pipe(map((a) => a?.email)),
|
||||
);
|
||||
const activeAccount = await firstValueFrom(this.accountService.activeAccount$);
|
||||
if (activeAccount == null) {
|
||||
this.logService.info(
|
||||
"[Key Connector remove password] No active account found, redirecting to login.",
|
||||
);
|
||||
await this.router.navigate(["/"]);
|
||||
return;
|
||||
}
|
||||
this.activeUserId = activeAccount.id;
|
||||
|
||||
await this.syncService.fullSync(false);
|
||||
|
||||
this.organization = await this.keyConnectorService.getManagingOrganization(this.activeUserId);
|
||||
if (this.organization == null) {
|
||||
this.logService.info(
|
||||
"[Key Connector remove password] No organization found, redirecting to login.",
|
||||
);
|
||||
await this.router.navigate(["/"]);
|
||||
return;
|
||||
}
|
||||
this.loading = false;
|
||||
}
|
||||
|
||||
get action() {
|
||||
return this.continuing || this.leaving;
|
||||
}
|
||||
|
||||
convert = async () => {
|
||||
this.continuing = true;
|
||||
this.actionPromise = this.keyConnectorService.migrateUser();
|
||||
|
||||
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();
|
||||
// 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.router.navigate(["/"]);
|
||||
} catch (e) {
|
||||
this.toastService.showToast({
|
||||
variant: "error",
|
||||
title: this.i18nService.t("errorOccurred"),
|
||||
message: e.message,
|
||||
});
|
||||
this.continuing = false;
|
||||
|
||||
this.handleActionError(e);
|
||||
}
|
||||
};
|
||||
|
||||
@@ -79,25 +92,33 @@ 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();
|
||||
// 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.router.navigate(["/"]);
|
||||
} catch (e) {
|
||||
this.toastService.showToast({
|
||||
variant: "error",
|
||||
title: this.i18nService.t("errorOccurred"),
|
||||
message: e,
|
||||
});
|
||||
this.leaving = false;
|
||||
|
||||
this.handleActionError(e);
|
||||
}
|
||||
};
|
||||
|
||||
handleActionError(e: unknown) {
|
||||
let message = "";
|
||||
if (e instanceof ErrorResponse || e instanceof Error) {
|
||||
message = e.message;
|
||||
}
|
||||
|
||||
this.toastService.showToast({
|
||||
variant: "error",
|
||||
title: this.i18nService.t("errorOccurred"),
|
||||
message: message,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user