Use global state for biometric prompt cancel storage (#8328)

* Fix error on close due to context differences in background

Desktop background does not have active user information. Also, we want to delete _all_ prompt cancelled data, not just that for the active user. Storing this on global and manipulating observables to active achieves this without needing any user information in the background.

* Remove potentially orphaned data

* Throw nice error if prompt cancelled used without active user

* Register migration
This commit is contained in:
Matt Gibson 2024-03-15 08:07:22 -05:00 committed by GitHub
parent 1e921eb4f6
commit 770f782a16
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 141 additions and 14 deletions

View File

@ -70,6 +70,9 @@ export class FakeAccountService implements AccountService {
}
async switchAccount(userId: UserId): Promise<void> {
const next =
userId == null ? null : { id: userId, ...this.accountsSubject["_buffer"]?.[0]?.[userId] };
this.activeAccountSubject.next(next);
await this.mock.switchAccount(userId);
}
}

View File

@ -1,7 +1,7 @@
import { firstValueFrom } from "rxjs";
import { makeEncString } from "../../../spec";
import { mockAccountServiceWith } from "../../../spec/fake-account-service";
import { makeEncString, trackEmissions } from "../../../spec";
import { FakeAccountService, mockAccountServiceWith } from "../../../spec/fake-account-service";
import { FakeSingleUserState } from "../../../spec/fake-state";
import { FakeStateProvider } from "../../../spec/fake-state-provider";
import { UserId } from "../../types/guid";
@ -23,10 +23,11 @@ describe("BiometricStateService", () => {
const userId = "userId" as UserId;
const encClientKeyHalf = makeEncString();
const encryptedClientKeyHalf = encClientKeyHalf.encryptedString;
const accountService = mockAccountServiceWith(userId);
let accountService: FakeAccountService;
let stateProvider: FakeStateProvider;
beforeEach(() => {
accountService = mockAccountServiceWith(userId);
stateProvider = new FakeStateProvider(accountService);
sut = new DefaultBiometricStateService(stateProvider);
@ -145,6 +146,13 @@ describe("BiometricStateService", () => {
});
describe("setPromptCancelled", () => {
let existingState: Record<UserId, boolean>;
beforeEach(() => {
existingState = { ["otherUser" as UserId]: false };
stateProvider.global.getFake(PROMPT_CANCELLED).stateSubject.next(existingState);
});
test("observable is updated", async () => {
await sut.setPromptCancelled();
@ -154,10 +162,39 @@ describe("BiometricStateService", () => {
it("updates state", async () => {
await sut.setPromptCancelled();
const nextMock = stateProvider.activeUser.getFake(PROMPT_CANCELLED).nextMock;
expect(nextMock).toHaveBeenCalledWith([userId, true]);
const nextMock = stateProvider.global.getFake(PROMPT_CANCELLED).nextMock;
expect(nextMock).toHaveBeenCalledWith({ ...existingState, [userId]: true });
expect(nextMock).toHaveBeenCalledTimes(1);
});
it("throws when called with no active user", async () => {
await accountService.switchAccount(null);
await expect(sut.setPromptCancelled()).rejects.toThrow(
"Cannot update biometric prompt cancelled state without an active user",
);
const nextMock = stateProvider.global.getFake(PROMPT_CANCELLED).nextMock;
expect(nextMock).not.toHaveBeenCalled();
});
});
describe("resetPromptCancelled", () => {
it("deletes all prompt cancelled state", async () => {
await sut.resetPromptCancelled();
const nextMock = stateProvider.global.getFake(PROMPT_CANCELLED).nextMock;
expect(nextMock).toHaveBeenCalledWith(null);
expect(nextMock).toHaveBeenCalledTimes(1);
});
it("updates observable to false", async () => {
const emissions = trackEmissions(sut.promptCancelled$);
await sut.setPromptCancelled();
await sut.resetPromptCancelled();
expect(emissions).toEqual([false, true, false]);
});
});
describe("setPromptAutomatically", () => {

View File

@ -1,4 +1,4 @@
import { Observable, firstValueFrom, map } from "rxjs";
import { Observable, firstValueFrom, map, combineLatest } from "rxjs";
import { UserId } from "../../types/guid";
import { EncryptedString, EncString } from "../models/domain/enc-string";
@ -107,7 +107,7 @@ export class DefaultBiometricStateService implements BiometricStateService {
private requirePasswordOnStartState: ActiveUserState<boolean>;
private encryptedClientKeyHalfState: ActiveUserState<EncryptedString | undefined>;
private dismissedRequirePasswordOnStartCalloutState: ActiveUserState<boolean>;
private promptCancelledState: ActiveUserState<boolean>;
private promptCancelledState: GlobalState<Record<UserId, boolean>>;
private promptAutomaticallyState: ActiveUserState<boolean>;
private fingerprintValidatedState: GlobalState<boolean>;
biometricUnlockEnabled$: Observable<boolean>;
@ -138,8 +138,15 @@ export class DefaultBiometricStateService implements BiometricStateService {
this.dismissedRequirePasswordOnStartCallout$ =
this.dismissedRequirePasswordOnStartCalloutState.state$.pipe(map(Boolean));
this.promptCancelledState = this.stateProvider.getActive(PROMPT_CANCELLED);
this.promptCancelled$ = this.promptCancelledState.state$.pipe(map(Boolean));
this.promptCancelledState = this.stateProvider.getGlobal(PROMPT_CANCELLED);
this.promptCancelled$ = combineLatest([
this.stateProvider.activeUserId$,
this.promptCancelledState.state$,
]).pipe(
map(([userId, record]) => {
return record?.[userId] ?? false;
}),
);
this.promptAutomaticallyState = this.stateProvider.getActive(PROMPT_AUTOMATICALLY);
this.promptAutomatically$ = this.promptAutomaticallyState.state$.pipe(map(Boolean));
@ -202,6 +209,15 @@ export class DefaultBiometricStateService implements BiometricStateService {
async logout(userId: UserId): Promise<void> {
await this.stateProvider.getUser(userId, ENCRYPTED_CLIENT_KEY_HALF).update(() => null);
await this.promptCancelledState.update(
(record) => {
delete record[userId];
return record;
},
{
shouldUpdate: (record) => record[userId] == true,
},
);
await this.stateProvider.getUser(userId, PROMPT_CANCELLED).update(() => null);
// Persist auto prompt setting through logout
// Persist dismissed require password on start callout through logout
@ -212,7 +228,24 @@ export class DefaultBiometricStateService implements BiometricStateService {
}
async setPromptCancelled(): Promise<void> {
await this.promptCancelledState.update(() => true);
await this.promptCancelledState.update(
(record, userId) => {
record ??= {};
record[userId] = true;
return record;
},
{
combineLatestWith: this.stateProvider.activeUserId$,
shouldUpdate: (_, userId) => {
if (userId == null) {
throw new Error(
"Cannot update biometric prompt cancelled state without an active user",
);
}
return true;
},
},
);
}
async resetPromptCancelled(): Promise<void> {

View File

@ -14,7 +14,7 @@ import {
describe.each([
[ENCRYPTED_CLIENT_KEY_HALF, "encryptedClientKeyHalf"],
[DISMISSED_REQUIRE_PASSWORD_ON_START_CALLOUT, true],
[PROMPT_CANCELLED, true],
[PROMPT_CANCELLED, { userId1: true, userId2: false }],
[PROMPT_AUTOMATICALLY, true],
[REQUIRE_PASSWORD_ON_START, true],
[BIOMETRIC_UNLOCK_ENABLED, true],

View File

@ -1,3 +1,4 @@
import { UserId } from "../../types/guid";
import { EncryptedString } from "../models/domain/enc-string";
import { KeyDefinition, BIOMETRIC_SETTINGS_DISK } from "../state";
@ -56,7 +57,7 @@ export const DISMISSED_REQUIRE_PASSWORD_ON_START_CALLOUT = new KeyDefinition<boo
* Stores whether the user has elected to cancel the biometric prompt. This is stored on disk due to process-reload
* wiping memory state. We don't want to prompt the user again if they've elected to cancel.
*/
export const PROMPT_CANCELLED = new KeyDefinition<boolean>(
export const PROMPT_CANCELLED = KeyDefinition.record<boolean, UserId>(
BIOMETRIC_SETTINGS_DISK,
"promptCancelled",
{

View File

@ -33,6 +33,7 @@ import { DomainSettingsMigrator } from "./migrations/34-move-domain-settings-to-
import { MoveThemeToStateProviderMigrator } from "./migrations/35-move-theme-to-state-providers";
import { VaultSettingsKeyMigrator } from "./migrations/36-move-show-card-and-identity-to-state-provider";
import { AvatarColorMigrator } from "./migrations/37-move-avatar-color-to-state-providers";
import { DeleteBiometricPromptCancelledData } from "./migrations/38-delete-orphaned-biometric-prompt-data";
import { RemoveEverBeenUnlockedMigrator } from "./migrations/4-remove-ever-been-unlocked";
import { AddKeyTypeToOrgKeysMigrator } from "./migrations/5-add-key-type-to-org-keys";
import { RemoveLegacyEtmKeyMigrator } from "./migrations/6-remove-legacy-etm-key";
@ -42,7 +43,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting
import { MinVersionMigrator } from "./migrations/min-version";
export const MIN_VERSION = 2;
export const CURRENT_VERSION = 37;
export const CURRENT_VERSION = 38;
export type MinVersion = typeof MIN_VERSION;
export function createMigrationBuilder() {
@ -82,7 +83,8 @@ export function createMigrationBuilder() {
.with(DomainSettingsMigrator, 33, 34)
.with(MoveThemeToStateProviderMigrator, 34, 35)
.with(VaultSettingsKeyMigrator, 35, 36)
.with(AvatarColorMigrator, 36, CURRENT_VERSION);
.with(AvatarColorMigrator, 36, 37)
.with(DeleteBiometricPromptCancelledData, 37, CURRENT_VERSION);
}
export async function currentVersion(

View File

@ -0,0 +1,28 @@
import { runMigrator } from "../migration-helper.spec";
import { IRREVERSIBLE } from "../migrator";
import { DeleteBiometricPromptCancelledData } from "./38-delete-orphaned-biometric-prompt-data";
describe("MoveThemeToStateProviders", () => {
const sut = new DeleteBiometricPromptCancelledData(37, 38);
describe("migrate", () => {
it("deletes promptCancelled from all users", async () => {
const output = await runMigrator(sut, {
authenticatedAccounts: ["user-1", "user-2"],
"user_user-1_biometricSettings_promptCancelled": true,
"user_user-2_biometricSettings_promptCancelled": false,
});
expect(output).toEqual({
authenticatedAccounts: ["user-1", "user-2"],
});
});
});
describe("rollback", () => {
it("is irreversible", async () => {
await expect(runMigrator(sut, {}, "rollback")).rejects.toThrow(IRREVERSIBLE);
});
});
});

View File

@ -0,0 +1,23 @@
import { KeyDefinitionLike, MigrationHelper } from "../migration-helper";
import { IRREVERSIBLE, Migrator } from "../migrator";
export const PROMPT_CANCELLED: KeyDefinitionLike = {
key: "promptCancelled",
stateDefinition: { name: "biometricSettings" },
};
export class DeleteBiometricPromptCancelledData extends Migrator<37, 38> {
async migrate(helper: MigrationHelper): Promise<void> {
await Promise.all(
(await helper.getAccounts()).map(async ({ userId }) => {
if (helper.getFromUser(userId, PROMPT_CANCELLED) != null) {
await helper.removeFromUser(userId, PROMPT_CANCELLED);
}
}),
);
}
async rollback(helper: MigrationHelper): Promise<void> {
throw IRREVERSIBLE;
}
}