Upstream/bitwarden-browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser.git synced 2024-11-25 12:15:18 +01:00
Code Issues Projects Releases Wiki Activity

[PM-7838] [PM-7864] Ensure AuthStatus Changes Before Exiting (#9018)

Browse Source 
* Ensure AuthStatus Changes Before Exiting

* Do Not Display Account Without Name Or Email

* Fix Environment Selectors

* Add AccountService.clean to Web
This commit is contained in:
Justin Baur 2024-05-03 16:43:42 -04:00 committed by GitHub
parent b46766affd
commit e4ef7d362e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 182 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
apps/browser/src/background/main.background.ts
View File

@ -1,4 +1,4 @@
import { Subject, firstValueFrom, map, merge, timeout } from "rxjs";
import { Subject, filter, firstValueFrom, map, merge, timeout } from "rxjs";
import {
PinCryptoServiceAbstraction,
@ -1200,31 +1200,46 @@ export default class MainBackground {
}
async logout(expired: boolean, userId?: UserId) {
userId ??= (
await firstValueFrom(
this.accountService.activeAccount$.pipe(
timeout({
first: 2000,
with: () => {
throw new Error("No active account found to logout");
},
}),
),
)
)?.id;
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(
map((a) => a?.id),
timeout({
first: 2000,
with: () => {
throw new Error("No active account found to logout");
},
}),
),
);
await this.eventUploadService.uploadEvents(userId as UserId);
const userBeingLoggedOut = userId ?? activeUserId;
await this.eventUploadService.uploadEvents(userBeingLoggedOut);
// HACK: We shouldn't wait for the authentication status to change but instead subscribe to the
// authentication status to do various actions.
const logoutPromise = firstValueFrom(
this.authService.authStatusFor$(userBeingLoggedOut).pipe(
filter((authenticationStatus) => authenticationStatus === AuthenticationStatus.LoggedOut),
timeout({
first: 5_000,
with: () => {
throw new Error("The logout process did not complete in a reasonable amount of time.");
},
}),
),
);
await Promise.all([
this.syncService.setLastSync(new Date(0), userId),
this.cryptoService.clearKeys(userId),
this.cipherService.clear(userId),
this.folderService.clear(userId),
this.collectionService.clear(userId),
this.passwordGenerationService.clear(userId),
this.vaultTimeoutSettingsService.clear(userId),
this.syncService.setLastSync(new Date(0), userBeingLoggedOut),
this.cryptoService.clearKeys(userBeingLoggedOut),
this.cipherService.clear(userBeingLoggedOut),
this.folderService.clear(userBeingLoggedOut),
this.collectionService.clear(userBeingLoggedOut),
this.passwordGenerationService.clear(userBeingLoggedOut),
this.vaultTimeoutSettingsService.clear(userBeingLoggedOut),
this.vaultFilterService.clear(),
this.biometricStateService.logout(userId),
this.biometricStateService.logout(userBeingLoggedOut),
/* We intentionally do not clear:
* - autofillSettingsService
* - badgeSettingsService
@ -1235,20 +1250,28 @@ export default class MainBackground {
//Needs to be checked before state is cleaned
const needStorageReseed = await this.needsStorageReseed();
const newActiveUser = await firstValueFrom(
this.accountService.nextUpAccount$.pipe(map((a) => a?.id)),
);
await this.stateService.clean({ userId: userId });
await this.accountService.clean(userId);
const newActiveUser =
userBeingLoggedOut === activeUserId
? await firstValueFrom(this.accountService.nextUpAccount$.pipe(map((a) => a?.id)))
: null;
await this.stateEventRunnerService.handleEvent("logout", userId);
await this.stateService.clean({ userId: userBeingLoggedOut });
await this.accountService.clean(userBeingLoggedOut);
await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut);
// HACK: Wait for the user logging outs authentication status to transition to LoggedOut
await logoutPromise;
await this.switchAccount(newActiveUser);
if (newActiveUser != null) {
// we have a new active user, do not continue tearing down application
await this.switchAccount(newActiveUser as UserId);
this.messagingService.send("switchAccountFinish");
} else {
this.messagingService.send("doneLoggingOut", { expired: expired, userId: userId });
this.messagingService.send("doneLoggingOut", {
expired: expired,
userId: userBeingLoggedOut,
});
}
if (needStorageReseed) {

62
apps/desktop/src/app/app.component.ts
View File

@ -8,7 +8,7 @@ import {
ViewContainerRef,
} from "@angular/core";
import { Router } from "@angular/router";
import { firstValueFrom, map, Subject, takeUntil } from "rxjs";
import { filter, firstValueFrom, map, Subject, takeUntil, timeout } from "rxjs";
import { ModalRef } from "@bitwarden/angular/components/modal/modal.ref";
import { ModalService } from "@bitwarden/angular/services/modal.service";
@ -566,19 +566,42 @@ export class AppComponent implements OnInit, OnDestroy {
this.messagingService.send("updateAppMenu", { updateRequest: updateRequest });
}
private async logOut(expired: boolean, userId?: string) {
const userBeingLoggedOut =
(userId as UserId) ??
(await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))));
// Even though the userId parameter is no longer optional doesn't mean a message couldn't be
// passing null-ish values to us.
private async logOut(expired: boolean, userId: UserId) {
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
const userBeingLoggedOut = userId ?? activeUserId;
// Mark account as being cleaned up so that the updateAppMenu logic (executed on syncCompleted)
// doesn't attempt to update a user that is being logged out as we will manually
// call updateAppMenu when the logout is complete.
this.startAccountCleanUp(userBeingLoggedOut);
let preLogoutActiveUserId;
const nextUpAccount = await firstValueFrom(this.accountService.nextUpAccount$);
const nextUpAccount =
activeUserId === userBeingLoggedOut
? await firstValueFrom(this.accountService.nextUpAccount$) // We'll need to switch accounts
: null;
try {
// HACK: We shouldn't wait for authentication status to change here but instead subscribe to the
// authentication status to do various actions.
const logoutPromise = firstValueFrom(
this.authService.authStatusFor$(userBeingLoggedOut).pipe(
filter((authenticationStatus) => authenticationStatus === AuthenticationStatus.LoggedOut),
timeout({
first: 5_000,
with: () => {
throw new Error(
"The logout process did not complete in a reasonable amount of time.",
);
},
}),
),
);
// Provide the userId of the user to upload events for
await this.eventUploadService.uploadEvents(userBeingLoggedOut);
await this.syncService.setLastSync(new Date(0), userBeingLoggedOut);
@ -592,26 +615,33 @@ export class AppComponent implements OnInit, OnDestroy {
await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut);
preLogoutActiveUserId = this.activeUserId;
await this.stateService.clean({ userId: userBeingLoggedOut });
await this.accountService.clean(userBeingLoggedOut);
// HACK: Wait for the user logging outs authentication status to transition to LoggedOut
await logoutPromise;
} finally {
this.finishAccountCleanUp(userBeingLoggedOut);
}
if (nextUpAccount == null) {
// 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(["login"]);
} else if (preLogoutActiveUserId !== nextUpAccount.id) {
this.messagingService.send("switchAccount", { userId: nextUpAccount.id });
// We only need to change the display at all if the account being looked at is the one
// being logged out. If it was a background account, no need to do anything.
if (userBeingLoggedOut === activeUserId) {
if (nextUpAccount != null) {
this.messagingService.send("switchAccount", { userId: nextUpAccount.id });
} else {
// We don't have another user to switch to, bring them to the login page so they
// can sign into a user.
await this.accountService.switchAccount(null);
void this.router.navigate(["login"]);
}
}
await this.updateAppMenu();
// This must come last otherwise the logout will prematurely trigger
// a process reload before all the state service user data can be cleaned up
if (userBeingLoggedOut === preLogoutActiveUserId) {
if (userBeingLoggedOut === activeUserId) {
this.authService.logOut(async () => {
if (expired) {
this.platformUtilsService.showToast(
@ -702,7 +732,7 @@ export class AppComponent implements OnInit, OnDestroy {
// 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
options[1] === "logOut"
? this.logOut(false, userId)
? this.logOut(false, userId as UserId)
: await this.vaultTimeoutService.lock(userId);
}
}

5
apps/desktop/src/app/layout/account-switcher.component.ts
View File

@ -92,6 +92,11 @@ export class AccountSwitcherComponent {
return null;
}
if (!active.name && !active.email) {
// We need to have this information at minimum to display them.
return null;
}
return {
id: active.id,
name: active.name,

30
apps/web/src/app/app.component.ts
View File

@ -2,7 +2,7 @@ import { DOCUMENT } from "@angular/common";
import { Component, Inject, NgZone, OnDestroy, OnInit } from "@angular/core";
import { NavigationEnd, Router } from "@angular/router";
import * as jq from "jquery";
import { Subject, firstValueFrom, map, switchMap, takeUntil, timer } from "rxjs";
import { Subject, filter, firstValueFrom, map, switchMap, takeUntil, timeout, timer } from "rxjs";
import { EventUploadService } from "@bitwarden/common/abstractions/event/event-upload.service";
import { NotificationsService } from "@bitwarden/common/abstractions/notifications.service";
@ -13,6 +13,7 @@ import { InternalPolicyService } from "@bitwarden/common/admin-console/abstracti
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { PaymentMethodWarningsServiceAbstraction as PaymentMethodWarningService } from "@bitwarden/common/billing/abstractions/payment-method-warnings-service.abstraction";
import { BroadcasterService } from "@bitwarden/common/platform/abstractions/broadcaster.service";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
@ -136,9 +137,7 @@ export class AppComponent implements OnDestroy, OnInit {
this.router.navigate(["/"]);
break;
case "logout":
// 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.logOut(!!message.expired, message.redirect);
await this.logOut(!!message.expired, message.redirect);
break;
case "lockVault":
await this.vaultTimeoutService.lock();
@ -266,7 +265,20 @@ export class AppComponent implements OnDestroy, OnInit {
private async logOut(expired: boolean, redirect = true) {
await this.eventUploadService.uploadEvents();
const userId = await this.stateService.getUserId();
const userId = (await this.stateService.getUserId()) as UserId;
const logoutPromise = firstValueFrom(
this.authService.authStatusFor$(userId).pipe(
filter((authenticationStatus) => authenticationStatus === AuthenticationStatus.LoggedOut),
timeout({
first: 5_000,
with: () => {
throw new Error("The logout process did not complete in a reasonable amount of time.");
},
}),
),
);
await Promise.all([
this.syncService.setLastSync(new Date(0)),
this.cryptoService.clearKeys(),
@ -274,11 +286,11 @@ export class AppComponent implements OnDestroy, OnInit {
this.folderService.clear(userId),
this.collectionService.clear(userId),
this.passwordGenerationService.clear(),
this.biometricStateService.logout(userId as UserId),
this.biometricStateService.logout(userId),
this.paymentMethodWarningService.clear(),
]);
await this.stateEventRunnerService.handleEvent("logout", userId as UserId);
await this.stateEventRunnerService.handleEvent("logout", userId);
await this.searchService.clearIndex();
this.authService.logOut(async () => {
@ -291,6 +303,10 @@ export class AppComponent implements OnDestroy, OnInit {
}
await this.stateService.clean({ userId: userId });
await this.accountService.clean(userId);
await logoutPromise;
if (redirect) {
// 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

39
libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts
View File

@ -1,5 +1,5 @@
import { MockProxy, any, mock } from "jest-mock-extended";
import { BehaviorSubject, of } from "rxjs";
import { BehaviorSubject, from, of } from "rxjs";
import { FakeAccountService, mockAccountServiceWith } from "../../../spec/fake-account-service";
import { SearchService } from "../../abstractions/search.service";
@ -106,6 +106,13 @@ describe("VaultTimeoutService", () => {
// Both are available by default and the specific test can change this per test
availableVaultTimeoutActionsSubject.next([VaultTimeoutAction.Lock, VaultTimeoutAction.LogOut]);
authService.authStatusFor$.mockImplementation((userId) => {
return from([
accounts[userId]?.authStatus ?? AuthenticationStatus.LoggedOut,
AuthenticationStatus.Locked,
]);
});
authService.getAuthStatus.mockImplementation((userId) => {
return Promise.resolve(accounts[userId]?.authStatus);
});
@ -387,18 +394,6 @@ describe("VaultTimeoutService", () => {
expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", "user1");
});
it("should call messaging service locked message if no user passed into lock", async () => {
setupLock();
await vaultTimeoutService.lock();
// Currently these pass `undefined` (or what they were given) as the userId back
// but we could change this to give the user that was locked (active) to these methods
// so they don't have to get it their own way, but that is a behavioral change that needs
// to be tested.
expect(messagingService.send).toHaveBeenCalledWith("locked", { userId: undefined });
});
it("should call locked callback if no user passed into lock", async () => {
setupLock();
@ -414,25 +409,31 @@ describe("VaultTimeoutService", () => {
it("should call state event runner with user passed into lock", async () => {
setupLock();
await vaultTimeoutService.lock("user2");
const user2 = "user2" as UserId;
expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", "user2");
await vaultTimeoutService.lock(user2);
expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", user2);
});
it("should call messaging service locked message with user passed into lock", async () => {
setupLock();
await vaultTimeoutService.lock("user2");
const user2 = "user2" as UserId;
expect(messagingService.send).toHaveBeenCalledWith("locked", { userId: "user2" });
await vaultTimeoutService.lock(user2);
expect(messagingService.send).toHaveBeenCalledWith("locked", { userId: user2 });
});
it("should call locked callback with user passed into lock", async () => {
setupLock();
await vaultTimeoutService.lock("user2");
const user2 = "user2" as UserId;
expect(lockedCallback).toHaveBeenCalledWith("user2");
await vaultTimeoutService.lock(user2);
expect(lockedCallback).toHaveBeenCalledWith(user2);
});
});
});

48
libs/common/src/services/vault-timeout/vault-timeout.service.ts
View File

@ -1,4 +1,4 @@
import { combineLatest, firstValueFrom, switchMap } from "rxjs";
import { combineLatest, filter, firstValueFrom, map, switchMap, timeout } from "rxjs";
import { SearchService } from "../../abstractions/search.service";
import { VaultTimeoutSettingsService } from "../../abstractions/vault-timeout/vault-timeout-settings.service";
@ -80,7 +80,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction {
);
}
async lock(userId?: string): Promise<void> {
async lock(userId?: UserId): Promise<void> {
const authed = await this.stateService.getIsAuthenticated({ userId: userId });
if (!authed) {
return;
@ -94,7 +94,27 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction {
await this.logOut(userId);
}
const currentUserId = (await firstValueFrom(this.accountService.activeAccount$)).id;
const currentUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
const lockingUserId = userId ?? currentUserId;
// HACK: Start listening for the transition of the locking user from something to the locked state.
// This is very much a hack to ensure that the authentication status to retrievable right after
// it does its work. Particularly the `lockedCallback` and `"locked"` message. Instead
// lockedCallback should be deprecated and people should subscribe and react to `authStatusFor$` themselves.
const lockPromise = firstValueFrom(
this.authService.authStatusFor$(lockingUserId).pipe(
filter((authStatus) => authStatus === AuthenticationStatus.Locked),
timeout({
first: 5_000,
with: () => {
throw new Error("The lock process did not complete in a reasonable amount of time.");
},
}),
),
);
if (userId == null || userId === currentUserId) {
await this.searchService.clearIndex();
@ -102,19 +122,21 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction {
await this.collectionService.clearActiveUserCache();
}
await this.masterPasswordService.clearMasterKey((userId ?? currentUserId) as UserId);
await this.masterPasswordService.clearMasterKey(lockingUserId);
await this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
await this.stateService.setCryptoMasterKeyAuto(null, { userId: userId });
await this.stateService.setUserKeyAutoUnlock(null, { userId: lockingUserId });
await this.stateService.setCryptoMasterKeyAuto(null, { userId: lockingUserId });
await this.cipherService.clearCache(userId);
await this.cipherService.clearCache(lockingUserId);
await this.stateEventRunnerService.handleEvent("lock", (userId ?? currentUserId) as UserId);
await this.stateEventRunnerService.handleEvent("lock", lockingUserId);
// FIXME: We should send the userId of the user that was locked, in the case of this method being passed
// undefined then it should give back the currentUserId. Better yet, this method shouldn't take
// an undefined userId at all. All receivers need to be checked for how they handle getting undefined.
this.messagingService.send("locked", { userId: userId });
// HACK: Sit here and wait for the the auth status to transition to `Locked`
// to ensure the message and lockedCallback will get the correct status
// if/when they call it.
await lockPromise;
this.messagingService.send("locked", { userId: lockingUserId });
if (this.lockedCallback != null) {
await this.lockedCallback(userId);
@ -162,7 +184,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction {
return diffSeconds >= vaultTimeoutSeconds;
}
private async executeTimeoutAction(userId: string): Promise<void> {
private async executeTimeoutAction(userId: UserId): Promise<void> {
const timeoutAction = await firstValueFrom(
this.vaultTimeoutSettingsService.vaultTimeoutAction$(userId),
);