bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-15 07:43:35 +00:00
Code Issues Releases Wiki Activity

[PM-7926] Handle complex user logout events (#9115)

Browse Source 
* Update activity when switching users

* Clear data of designated user

* Do not switchMap to null, always to Promise or Observable

* handle uninitialized popup services

* Switch to new account immediately and log out as inactive.

Split up done logging out and navigation so we can always display expire warning.

* Do not navigate in account switcher, main.background takes care of it

* Ignore storage updates from reseed events

* Remove loading on cancelled logout

* Catch missed account switch errors

* Avoid usage of active user state in sync service

Send service does not currently support specified user data
manipulation, so we ensure that the notification was sent to the
active user prior to processing the notification.

* Clear sequentialize caches on account switch

These caches are used to ensure that rapid calls to an async method are not repeated. However, the cached promises are valid only within a given userId context and must be cleared when that context changes.

* Revert `void` promise for notification reconnect

* Update libs/angular/src/services/jslib-services.module.ts

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>

* Handle switch account routing through messaging background -> app

* Use account switch status to handle unlocked navigation case.

* Revert "Handle switch account routing through messaging background -> app"

This reverts commit 8f35078ecb.

---------

Co-authored-by: Cesar Gonzalez <cesar.a.gonzalezcs@gmail.com>
Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>
This commit is contained in:
Matt Gibson
2024-05-14 16:27:43 -04:00
committed by GitHub
parent b4f4818635
commit 94b57687f5
15 changed files with 214 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
apps/browser/src/auth/popup/account-switching/account-switcher.component.ts
View File

@@ -1,7 +1,7 @@
import { Location } from "@angular/common";
import { Component, OnDestroy, OnInit } from "@angular/core";
import { Router } from "@angular/router";
import { Subject, firstValueFrom, map, switchMap, takeUntil } from "rxjs";
import { Subject, firstValueFrom, map, of, switchMap, takeUntil } from "rxjs";
import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service";
import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service";
@@ -49,7 +49,7 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy {
readonly currentAccount$ = this.accountService.activeAccount$.pipe(
switchMap((a) =>
a == null
? null
? of(null)
: this.authService.activeAccountStatus$.pipe(map((s) => ({ ...a, status: s }))),
),
);
@@ -106,12 +106,14 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy {
});
if (confirmed) {
this.messagingService.send("logout", { userId });
const result = await this.accountSwitcherService.logoutAccount(userId);
// unlocked logout responses need to be navigated out of the account switcher.
// other responses will be handled by background and app.component
if (result?.status === AuthenticationStatus.Unlocked) {
this.location.back();
}
}
// 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(["home"]);
this.loading = false;
}
ngOnDestroy() {

20
apps/browser/src/auth/popup/account-switching/account.component.ts
View File

@@ -1,10 +1,10 @@
import { CommonModule, Location } from "@angular/common";
import { Component, EventEmitter, Input, Output } from "@angular/core";
import { Router } from "@angular/router";
import { JslibModule } from "@bitwarden/angular/jslib.module";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { AvatarModule } from "@bitwarden/components";
import { AccountSwitcherService, AvailableAccount } from "./services/account-switcher.service";
@@ -21,9 +21,9 @@ export class AccountComponent {
constructor(
private accountSwitcherService: AccountSwitcherService,
private router: Router,
private location: Location,
private i18nService: I18nService,
private logService: LogService,
) {}
get specialAccountAddId() {
@@ -32,15 +32,19 @@ export class AccountComponent {
async selectAccount(id: string) {
this.loading.emit(true);
await this.accountSwitcherService.selectAccount(id);
let result;
try {
result = await this.accountSwitcherService.selectAccount(id);
} catch (e) {
this.logService.error("Error selecting account", e);
}
if (id === this.specialAccountAddId) {
// 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(["home"]);
} else {
// Navigate out of account switching for unlocked accounts
// locked or logged out account statuses are handled by background and app.component
if (result?.status === AuthenticationStatus.Unlocked) {
this.location.back();
}
this.loading.emit(false);
}
get status() {

31
apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts
View File

@@ -186,4 +186,35 @@ describe("AccountSwitcherService", () => {
expect(removeListenerSpy).toBeCalledTimes(1);
});
});
describe("logout", () => {
const userId1 = "1" as UserId;
const userId2 = "2" as UserId;
it("initiates logout", async () => {
let listener: (
message: { command: string; userId: UserId; status: AuthenticationStatus },
sender: unknown,
sendResponse: unknown,
) => void;
jest.spyOn(chrome.runtime.onMessage, "addListener").mockImplementation((addedListener) => {
listener = addedListener;
});
const removeListenerSpy = jest.spyOn(chrome.runtime.onMessage, "removeListener");
const logoutPromise = accountSwitcherService.logoutAccount(userId1);
listener(
{ command: "switchAccountFinish", userId: userId2, status: AuthenticationStatus.Unlocked },
undefined,
undefined,
);
const result = await logoutPromise;
expect(messagingService.send).toHaveBeenCalledWith("logout", { userId: userId1 });
expect(result).toEqual({ newUserId: userId2, status: AuthenticationStatus.Unlocked });
expect(removeListenerSpy).toBeCalledTimes(1);
});
});
});

59
apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts
View File

@@ -41,7 +41,7 @@ export class AccountSwitcherService {
SPECIAL_ADD_ACCOUNT_ID = "addAccount";
availableAccounts$: Observable<AvailableAccount[]>;
switchAccountFinished$: Observable<string>;
switchAccountFinished$: Observable<{ userId: UserId; status: AuthenticationStatus }>;
constructor(
private accountService: AccountService,
@@ -111,11 +111,11 @@ export class AccountSwitcherService {
);
// Create a reusable observable that listens to the switchAccountFinish message and returns the userId from the message
this.switchAccountFinished$ = fromChromeEvent<[message: { command: string; userId: string }]>(
chrome.runtime.onMessage,
).pipe(
this.switchAccountFinished$ = fromChromeEvent<
[message: { command: string; userId: UserId; status: AuthenticationStatus }]
>(chrome.runtime.onMessage).pipe(
filter(([message]) => message.command === "switchAccountFinish"),
map(([message]) => message.userId),
map(([message]) => ({ userId: message.userId, status: message.status })),
);
}
@@ -127,12 +127,46 @@ export class AccountSwitcherService {
if (id === this.SPECIAL_ADD_ACCOUNT_ID) {
id = null;
}
const userId = id as UserId;
// Creates a subscription to the switchAccountFinished observable but further
// filters it to only care about the current userId.
const switchAccountFinishedPromise = firstValueFrom(
const switchAccountFinishedPromise = this.listenForSwitchAccountFinish(userId);
// Initiate the actions required to make account switching happen
await this.accountService.switchAccount(userId);
this.messagingService.send("switchAccount", { userId }); // This message should cause switchAccountFinish to be sent
// Wait until we receive the switchAccountFinished message
return await switchAccountFinishedPromise;
}
/**
*
* @param userId the user id to logout
* @returns the userId and status of the that has been switch to due to the logout. null on errors.
*/
async logoutAccount(
userId: UserId,
): Promise<{ newUserId: UserId; status: AuthenticationStatus } | null> {
// logout creates an account switch to the next up user, which may be null
const switchPromise = this.listenForSwitchAccountFinish(null);
await this.messagingService.send("logout", { userId });
// wait for account switch to happen, the result will be the new user id and status
const result = await switchPromise;
return { newUserId: result.userId, status: result.status };
}
// Listens for the switchAccountFinish message and returns the userId from the message
// Optionally filters switchAccountFinish to an expected userId
private listenForSwitchAccountFinish(
expectedUserId: UserId | null,
): Promise<{ userId: UserId; status: AuthenticationStatus } | null> {
return firstValueFrom(
this.switchAccountFinished$.pipe(
filter((userId) => userId === id),
filter(({ userId }) => (expectedUserId ? userId === expectedUserId : true)),
timeout({
// Much longer than account switching is expected to take for normal accounts
// but the account switching process includes a possible full sync so we need to account
@@ -143,20 +177,13 @@ export class AccountSwitcherService {
throwError(() => new Error(AccountSwitcherService.incompleteAccountSwitchError)),
}),
),
);
// Initiate the actions required to make account switching happen
await this.accountService.switchAccount(id as UserId);
this.messagingService.send("switchAccount", { userId: id }); // This message should cause switchAccountFinish to be sent
// Wait until we recieve the switchAccountFinished message
await switchAccountFinishedPromise.catch((err) => {
).catch((err) => {
if (
err instanceof Error &&
err.message === AccountSwitcherService.incompleteAccountSwitchError
) {
this.logService.warning("message 'switchAccount' never responded.");
return;
return null;
}
throw err;
});

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

@@ -100,6 +100,7 @@ import { Message, MessageListener, MessageSender } from "@bitwarden/common/platf
// eslint-disable-next-line no-restricted-imports -- Used for dependency creation
import { SubjectMessageSender } from "@bitwarden/common/platform/messaging/internal";
import { Lazy } from "@bitwarden/common/platform/misc/lazy";
import { clearCaches } from "@bitwarden/common/platform/misc/sequentialize";
import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state";
import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
import { AppIdService } from "@bitwarden/common/platform/services/app-id.service";
@@ -815,6 +816,7 @@ export default class MainBackground {
logoutCallback,
this.billingAccountProfileStateService,
this.tokenService,
this.authService,
);
this.eventUploadService = new EventUploadService(
this.apiService,
@@ -1180,6 +1182,7 @@ export default class MainBackground {
* Switch accounts to indicated userId -- null is no active user
*/
async switchAccount(userId: UserId) {
let nextAccountStatus: AuthenticationStatus;
try {
const currentlyActiveAccount = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((account) => account?.id)),
@@ -1187,6 +1190,8 @@ export default class MainBackground {
// can be removed once password generation history is migrated to state providers
await this.stateService.clearDecryptedData(currentlyActiveAccount);
await this.accountService.switchAccount(userId);
// Clear sequentialized caches
clearCaches();
if (userId == null) {
this.loginEmailService.setRememberEmail(false);
@@ -1194,11 +1199,12 @@ export default class MainBackground {
await this.refreshBadge();
await this.refreshMenu();
await this.overlayBackground.updateOverlayCiphers();
await this.overlayBackground?.updateOverlayCiphers(); // null in popup only contexts
this.messagingService.send("goHome");
return;
}
const status = await this.authService.getAuthStatus(userId);
nextAccountStatus = await this.authService.getAuthStatus(userId);
const forcePasswordReset =
(await firstValueFrom(this.masterPasswordService.forceSetPasswordReason$(userId))) !=
ForceSetPasswordReason.None;
@@ -1206,7 +1212,9 @@ export default class MainBackground {
await this.systemService.clearPendingClipboard();
await this.notificationsService.updateConnection(false);
if (status === AuthenticationStatus.Locked) {
if (nextAccountStatus === AuthenticationStatus.LoggedOut) {
this.messagingService.send("goHome");
} else if (nextAccountStatus === AuthenticationStatus.Locked) {
this.messagingService.send("locked", { userId: userId });
} else if (forcePasswordReset) {
this.messagingService.send("update-temp-password", { userId: userId });
@@ -1214,11 +1222,14 @@ export default class MainBackground {
this.messagingService.send("unlocked", { userId: userId });
await this.refreshBadge();
await this.refreshMenu();
await this.overlayBackground.updateOverlayCiphers();
await this.overlayBackground?.updateOverlayCiphers(); // null in popup only contexts
await this.syncService.fullSync(false);
}
} finally {
this.messagingService.send("switchAccountFinish", { userId: userId });
this.messagingService.send("switchAccountFinish", {
userId: userId,
status: nextAccountStatus,
});
}
}
@@ -1239,6 +1250,13 @@ export default class MainBackground {
await this.eventUploadService.uploadEvents(userBeingLoggedOut);
const newActiveUser =
userBeingLoggedOut === activeUserId
? await firstValueFrom(this.accountService.nextUpAccount$.pipe(map((a) => a?.id)))
: null;
await this.switchAccount(newActiveUser);
// 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(
@@ -1273,11 +1291,6 @@ export default class MainBackground {
//Needs to be checked before state is cleaned
const needStorageReseed = await this.needsStorageReseed(userId);
const newActiveUser =
userBeingLoggedOut === activeUserId
? await firstValueFrom(this.accountService.nextUpAccount$.pipe(map((a) => a?.id)))
: null;
await this.stateService.clean({ userId: userBeingLoggedOut });
await this.accountService.clean(userBeingLoggedOut);
@@ -1286,16 +1299,10 @@ export default class MainBackground {
// 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
this.messagingService.send("switchAccountFinish");
} else {
this.messagingService.send("doneLoggingOut", {
expired: expired,
userId: userBeingLoggedOut,
});
}
this.messagingService.send("doneLoggingOut", {
expired: expired,
userId: userBeingLoggedOut,
});
if (needStorageReseed) {
await this.reseedStorage();
@@ -1308,9 +1315,7 @@ export default class MainBackground {
}
await this.refreshBadge();
await this.mainContextMenuHandler?.noAccess();
// 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.notificationsService.updateConnection(false);
await this.notificationsService.updateConnection(false);
await this.systemService.clearPendingClipboard();
await this.systemService.startProcessReload(this.authService);
}

7
apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts
View File

@@ -1,4 +1,4 @@
import { mergeMap } from "rxjs";
import { filter, mergeMap } from "rxjs";
import {
AbstractStorageService,
@@ -34,6 +34,11 @@ export default abstract class AbstractChromeStorageService
constructor(protected chromeStorageApi: chrome.storage.StorageArea) {
this.updates$ = fromChromeEvent(this.chromeStorageApi.onChanged).pipe(
filter(([changes]) => {
// Our storage services support changing only one key at a time. If more are changed, it's due to
// reseeding storage and we should ignore the changes.
return Object.keys(changes).length === 1;
}),
mergeMap(([changes]) => {
return Object.entries(changes).map(([key, change]) => {
// The `newValue` property isn't on the StorageChange object

9
apps/browser/src/popup/app.component.ts
View File

@@ -91,13 +91,9 @@ export class AppComponent implements OnInit, OnDestroy {
message: this.i18nService.t("loginExpired"),
});
}
// 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(["home"]);
});
this.changeDetectorRef.detectChanges();
} else if (msg.command === "authBlocked") {
} else if (msg.command === "authBlocked" || msg.command === "goHome") {
// 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(["home"]);
@@ -137,9 +133,6 @@ 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
this.router.navigate(["/remove-password"]);
} else if (msg.command === "switchAccountFinish") {
// TODO: unset loading?
// this.loading = false;
} else if (msg.command == "update-temp-password") {
// 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

1
apps/cli/src/bw.ts
View File

@@ -664,6 +664,7 @@ export class Main {
async (expired: boolean) => await this.logout(),
this.billingAccountProfileStateService,
this.tokenService,
this.authService,
);
this.totpService = new TotpService(this.cryptoFunctionService, this.logService);

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

@@ -38,6 +38,7 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
import { SystemService } from "@bitwarden/common/platform/abstractions/system.service";
import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service";
import { clearCaches } from "@bitwarden/common/platform/misc/sequentialize";
import { StateEventRunnerService } from "@bitwarden/common/platform/state";
import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/generator/password";
import { UserId } from "@bitwarden/common/types/guid";
@@ -396,6 +397,8 @@ export class AppComponent implements OnInit, OnDestroy {
this.router.navigate(["/remove-password"]);
break;
case "switchAccount": {
// Clear sequentialized caches
clearCaches();
if (message.userId != null) {
await this.stateService.clearDecryptedData(message.userId);
await this.accountService.switchAccount(message.userId);