1
0
mirror of https://github.com/bitwarden/browser.git synced 2024-06-25 10:25:36 +02:00

[PM-4561] Migrate Browser Account Settings (#6886)

* Move Account Settings

* Add Another Test

* Refactor Tests

* Update Notification Bar to Get Value From Global

* Also Migrate Disable Context Menu

* Add Explanation
This commit is contained in:
Justin Baur 2023-11-17 09:20:42 -05:00 committed by GitHub
parent e84e02c441
commit 5b1717fd41
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 528 additions and 42 deletions

View File

@ -3,7 +3,7 @@ import ChangePasswordRuntimeMessage from "../../background/models/changePassword
import AutofillField from "../models/autofill-field";
import { WatchedForm } from "../models/watched-form";
import { FormData } from "../services/abstractions/autofill.service";
import { UserSettings } from "../types";
import { GlobalSettings, UserSettings } from "../types";
interface HTMLElementWithFormOpId extends HTMLElement {
formOpId: string;
@ -97,6 +97,7 @@ async function loadNotificationBar() {
const userSettingsStorageValue = await getFromLocalStorage(activeUserId);
if (userSettingsStorageValue[activeUserId]) {
const userSettings: UserSettings = userSettingsStorageValue[activeUserId].settings;
const globalSettings: GlobalSettings = await getFromLocalStorage("global");
// Do not show the notification bar on the Bitwarden vault
// because they can add logins and change passwords there
@ -107,11 +108,11 @@ async function loadNotificationBar() {
// show the notification bar on (for login detail collection or password change).
// It is managed in the Settings > Excluded Domains page in the browser extension.
// Example: '{"bitwarden.com":null}'
const excludedDomainsDict = userSettings.neverDomains;
const excludedDomainsDict = globalSettings.neverDomains;
if (!excludedDomainsDict || !(window.location.hostname in excludedDomainsDict)) {
// Set local disabled preferences
disabledAddLoginNotification = userSettings.disableAddLoginNotification;
disabledChangedPasswordNotification = userSettings.disableChangedPasswordNotification;
disabledAddLoginNotification = globalSettings.disableAddLoginNotification;
disabledChangedPasswordNotification = globalSettings.disableChangedPasswordNotification;
if (!disabledAddLoginNotification || !disabledChangedPasswordNotification) {
// If the user has not disabled both notifications, then handle the initial page change (null -> actual page)

View File

@ -1,4 +1,5 @@
import { Region } from "@bitwarden/common/platform/abstractions/environment.service";
import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state";
import { VaultTimeoutAction } from "@bitwarden/common/src/enums/vault-timeout-action.enum";
import { CipherType } from "@bitwarden/common/vault/enums/cipher-type";
@ -34,13 +35,15 @@ export type UserSettings = {
settings: {
equivalentDomains: string[][];
};
neverDomains?: { [key: string]: any };
disableAddLoginNotification?: boolean;
disableChangedPasswordNotification?: boolean;
vaultTimeout: number;
vaultTimeoutAction: VaultTimeoutAction;
};
export type GlobalSettings = Pick<
GlobalState,
"disableAddLoginNotification" | "disableChangedPasswordNotification" | "neverDomains"
>;
/**
* A HTMLElement (usually a form element) with additional custom properties added by this script
*/

View File

@ -33,6 +33,10 @@ export class FakeStorageService implements AbstractStorageService {
this.store = store;
}
get internalStore() {
return this.store;
}
internalUpdateValuesRequireDeserialization(value: boolean) {
this._valuesRequireDeserialization = value;
}

View File

@ -420,8 +420,8 @@ export abstract class StateService<T extends Account = Account> {
setMainWindowSize: (value: number, options?: StorageOptions) => Promise<void>;
getMinimizeOnCopyToClipboard: (options?: StorageOptions) => Promise<boolean>;
setMinimizeOnCopyToClipboard: (value: boolean, options?: StorageOptions) => Promise<void>;
getNeverDomains: (options?: StorageOptions) => Promise<{ [id: string]: any }>;
setNeverDomains: (value: { [id: string]: any }, options?: StorageOptions) => Promise<void>;
getNeverDomains: (options?: StorageOptions) => Promise<{ [id: string]: unknown }>;
setNeverDomains: (value: { [id: string]: unknown }, options?: StorageOptions) => Promise<void>;
getNoAutoPromptBiometricsText: (options?: StorageOptions) => Promise<string>;
setNoAutoPromptBiometricsText: (value: string, options?: StorageOptions) => Promise<void>;
getOpenAtLogin: (options?: StorageOptions) => Promise<boolean>;

View File

@ -222,12 +222,9 @@ export class AccountSettings {
clearClipboard?: number;
collapsedGroupings?: string[];
defaultUriMatch?: UriMatchType;
disableAddLoginNotification?: boolean;
disableAutoBiometricsPrompt?: boolean;
disableAutoTotpCopy?: boolean;
disableBadgeCounter?: boolean;
disableChangedPasswordNotification?: boolean;
disableContextMenuItem?: boolean;
disableGa?: boolean;
dismissedAutoFillOnPageLoadCallout?: boolean;
dontShowCardsCurrentTab?: boolean;
@ -239,7 +236,6 @@ export class AccountSettings {
environmentUrls: EnvironmentUrls = new EnvironmentUrls();
equivalentDomains?: any;
minimizeOnCopyToClipboard?: boolean;
neverDomains?: { [id: string]: any };
passwordGenerationOptions?: PasswordGeneratorOptions;
usernameGenerationOptions?: UsernameGeneratorOptions;
generatorOptions?: GeneratorOptions;

View File

@ -36,4 +36,8 @@ export class GlobalState {
enableBrowserIntegrationFingerprint?: boolean;
enableDuckDuckGoBrowserIntegration?: boolean;
region?: string;
neverDomains?: { [id: string]: unknown };
disableAddLoginNotification?: boolean;
disableChangedPasswordNotification?: boolean;
disableContextMenuItem?: boolean;
}

View File

@ -1121,18 +1121,18 @@ export class StateService<
async getDisableAddLoginNotification(options?: StorageOptions): Promise<boolean> {
return (
(await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))
?.settings?.disableAddLoginNotification ?? false
(await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions())))
?.disableAddLoginNotification ?? false
);
}
async setDisableAddLoginNotification(value: boolean, options?: StorageOptions): Promise<void> {
const account = await this.getAccount(
const globals = await this.getGlobals(
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
account.settings.disableAddLoginNotification = value;
await this.saveAccount(
account,
globals.disableAddLoginNotification = value;
await this.saveGlobals(
globals,
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
}
@ -1193,8 +1193,8 @@ export class StateService<
async getDisableChangedPasswordNotification(options?: StorageOptions): Promise<boolean> {
return (
(await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))
?.settings?.disableChangedPasswordNotification ?? false
(await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions())))
?.disableChangedPasswordNotification ?? false
);
}
@ -1202,30 +1202,30 @@ export class StateService<
value: boolean,
options?: StorageOptions
): Promise<void> {
const account = await this.getAccount(
const globals = await this.getGlobals(
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
account.settings.disableChangedPasswordNotification = value;
await this.saveAccount(
account,
globals.disableChangedPasswordNotification = value;
await this.saveGlobals(
globals,
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
}
async getDisableContextMenuItem(options?: StorageOptions): Promise<boolean> {
return (
(await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))
?.settings?.disableContextMenuItem ?? false
(await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions())))
?.disableContextMenuItem ?? false
);
}
async setDisableContextMenuItem(value: boolean, options?: StorageOptions): Promise<void> {
const account = await this.getAccount(
const globals = await this.getGlobals(
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
account.settings.disableContextMenuItem = value;
await this.saveAccount(
account,
globals.disableContextMenuItem = value;
await this.saveGlobals(
globals,
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
}
@ -2295,19 +2295,19 @@ export class StateService<
);
}
async getNeverDomains(options?: StorageOptions): Promise<{ [id: string]: any }> {
async getNeverDomains(options?: StorageOptions): Promise<{ [id: string]: unknown }> {
return (
await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions()))
)?.settings?.neverDomains;
await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))
)?.neverDomains;
}
async setNeverDomains(value: { [id: string]: any }, options?: StorageOptions): Promise<void> {
const account = await this.getAccount(
async setNeverDomains(value: { [id: string]: unknown }, options?: StorageOptions): Promise<void> {
const globals = await this.getGlobals(
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
account.settings.neverDomains = value;
await this.saveAccount(
account,
globals.neverDomains = value;
await this.saveGlobals(
globals,
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
}

View File

@ -11,10 +11,11 @@ import { AddKeyTypeToOrgKeysMigrator } from "./migrations/5-add-key-type-to-org-
import { RemoveLegacyEtmKeyMigrator } from "./migrations/6-remove-legacy-etm-key";
import { MoveBiometricAutoPromptToAccount } from "./migrations/7-move-biometric-auto-prompt-to-account";
import { MoveStateVersionMigrator } from "./migrations/8-move-state-version";
import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-settings-to-global";
import { MinVersionMigrator } from "./migrations/min-version";
export const MIN_VERSION = 2;
export const CURRENT_VERSION = 8;
export const CURRENT_VERSION = 9;
export type MinVersion = typeof MIN_VERSION;
export async function migrate(
@ -38,7 +39,8 @@ export async function migrate(
.with(AddKeyTypeToOrgKeysMigrator, 4, 5)
.with(RemoveLegacyEtmKeyMigrator, 5, 6)
.with(MoveBiometricAutoPromptToAccount, 6, 7)
.with(MoveStateVersionMigrator, 7, CURRENT_VERSION)
.with(MoveStateVersionMigrator, 7, 8)
.with(MoveBrowserSettingsToGlobal, 8, CURRENT_VERSION)
.migrate(migrationHelper);
}

View File

@ -17,6 +17,20 @@ describe("MigrationBuilder", () => {
}
}
class TestMigratorWithInstanceMethod extends Migrator<0, 1> {
private async instanceMethod(helper: MigrationHelper, value: string) {
await helper.set("test", value);
}
async migrate(helper: MigrationHelper): Promise<void> {
await this.instanceMethod(helper, "migrate");
}
async rollback(helper: MigrationHelper): Promise<void> {
await this.instanceMethod(helper, "rollback");
}
}
let sut: MigrationBuilder<number>;
beforeEach(() => {
@ -114,4 +128,9 @@ describe("MigrationBuilder", () => {
expect(rollback).not.toBeCalled();
});
});
it("should be able to call instance methods", async () => {
const helper = new MigrationHelper(0, mock(), mock());
await sut.with(TestMigratorWithInstanceMethod, 0, 1).migrate(helper);
});
});

View File

@ -93,7 +93,7 @@ export class MigrationBuilder<TCurrent extends number = 0> {
);
if (shouldMigrate) {
const method = direction === "up" ? migrator.migrate : migrator.rollback;
await method(helper);
await method.bind(migrator)(helper);
helper.info(
`Migrator ${migrator.constructor.name} (to version ${migrator.toVersion}) migrated - ${direction}`
);

View File

@ -0,0 +1,355 @@
import { mock } from "jest-mock-extended";
import { FakeStorageService } from "../../../spec/fake-storage.service";
import { MigrationHelper } from "../migration-helper";
import { Migrator } from "../migrator";
import { MoveBrowserSettingsToGlobal } from "./9-move-browser-settings-to-global";
type TestState = { authenticatedAccounts: string[] } & { [key: string]: unknown };
// This could become a helper available to anyone
const runMigrator = async <TMigrator extends Migrator<number, number>>(
migrator: TMigrator,
initalData?: Record<string, unknown>
): Promise<Record<string, unknown>> => {
const fakeStorageService = new FakeStorageService(initalData);
const helper = new MigrationHelper(migrator.fromVersion, fakeStorageService, mock());
await migrator.migrate(helper);
return fakeStorageService.internalStore;
};
describe("MoveBrowserSettingsToGlobal", () => {
const myMigrator = new MoveBrowserSettingsToGlobal(8, 9);
// This could be the state for a browser client who has never touched the settings or this could
// be a different client who doesn't make it possible to toggle these settings
it("doesn't set any value to global if there is no equivalent settings on the account", async () => {
const testInput: TestState = {
authenticatedAccounts: ["user1"],
global: {
theme: "system", // A real global setting that should persist after migration
},
user1: {
settings: {
region: "Self-hosted",
},
},
};
const output = await runMigrator(myMigrator, testInput);
// No additions to the global state
expect(output["global"]).toEqual({
theme: "system",
});
// No additions to user state
expect(output["user1"]).toEqual({
settings: {
region: "Self-hosted",
},
});
});
// This could be a user who opened up the settings page and toggled the checkbox, since this setting infers undefined
// as false this is essentially the default value.
it("sets the setting from the users settings if they have toggled the setting but placed it back to it's inferred", async () => {
const testInput: TestState = {
authenticatedAccounts: ["user1"],
global: {
theme: "system", // A real global setting that should persist after migration
},
user1: {
settings: {
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example.com": null,
},
region: "Self-hosted",
},
},
};
const output = await runMigrator(myMigrator, testInput);
// User settings should have moved to global
expect(output["global"]).toEqual({
theme: "system",
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example.com": null,
},
});
// Migrated settings should be deleted
expect(output["user1"]).toEqual({
settings: { region: "Self-hosted" },
});
});
// The user has set a value and it's not the default, we should respect that choice globally
it("should take the only users settings", async () => {
const testInput: TestState = {
authenticatedAccounts: ["user1"],
global: {
theme: "system", // A real global setting that should persist after migration
},
user1: {
settings: {
disableAddLoginNotification: true,
disableChangedPasswordNotification: true,
disableContextMenuItem: true,
neverDomains: {
"example.com": null,
},
region: "Self-hosted",
},
},
};
const output = await runMigrator(myMigrator, testInput);
// The value for the single user value should be set to global
expect(output["global"]).toEqual({
theme: "system",
disableAddLoginNotification: true,
disableChangedPasswordNotification: true,
disableContextMenuItem: true,
neverDomains: {
"example.com": null,
},
});
expect(output["user1"]).toEqual({
settings: { region: "Self-hosted" },
});
});
// No browser client at the time of this writing should ever have multiple authenticatedAccounts
// but in the bizzare case, we should interpret any user having the feature turned on as the value for
// all the accounts.
it("should take the false value if there are conflicting choices", async () => {
const testInput: TestState = {
authenticatedAccounts: ["user1", "user2"],
global: {
theme: "system", // A real global setting that should persist after migration
},
user1: {
settings: {
disableAddLoginNotification: true,
disableChangedPasswordNotification: true,
disableContextMenuItem: true,
neverDomains: {
"example.com": null,
},
region: "Self-hosted",
},
},
user2: {
settings: {
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example2.com": null,
},
region: "Self-hosted",
},
},
};
const output = await runMigrator(myMigrator, testInput);
// The false settings should be respected over the true values
// neverDomains should be combined into a single object
expect(output["global"]).toEqual({
theme: "system",
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example.com": null,
"example2.com": null,
},
});
expect(output["user1"]).toEqual({
settings: { region: "Self-hosted" },
});
expect(output["user2"]).toEqual({
settings: { region: "Self-hosted" },
});
});
// Once again, no normal browser should have conflicting values at the time of this comment but:
// if one user has toggled the setting back to on and one user has never touched the setting,
// persist the false value into the global state.
it("should persist the false value if one user has that in their settings", async () => {
const testInput: TestState = {
authenticatedAccounts: ["user1", "user2"],
global: {
theme: "system", // A real global setting that should persist after migration
},
user1: {
settings: {
region: "Self-hosted",
},
},
user2: {
settings: {
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example.com": null,
},
region: "Self-hosted",
},
},
};
const output = await runMigrator(myMigrator, testInput);
// The false settings should be respected over the true values
// neverDomains should be combined into a single object
expect(output["global"]).toEqual({
theme: "system",
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example.com": null,
},
});
expect(output["user1"]).toEqual({
settings: { region: "Self-hosted" },
});
expect(output["user2"]).toEqual({
settings: { region: "Self-hosted" },
});
});
// Once again, no normal browser should have conflicting values at the time of this comment but:
// if one user has toggled the setting off and one user has never touched the setting,
// persist the false value into the global state.
it("should persist the false value from a user with no settings since undefined is inferred as false", async () => {
const testInput: TestState = {
authenticatedAccounts: ["user1", "user2"],
global: {
theme: "system", // A real global setting that should persist after migration
},
user1: {
settings: {
region: "Self-hosted",
},
},
user2: {
settings: {
disableAddLoginNotification: true,
disableChangedPasswordNotification: true,
disableContextMenuItem: true,
neverDomains: {
"example.com": null,
},
region: "Self-hosted",
},
},
};
const output = await runMigrator(myMigrator, testInput);
// The false settings should be respected over the true values
// neverDomains should be combined into a single object
expect(output["global"]).toEqual({
theme: "system",
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example.com": null,
},
});
expect(output["user1"]).toEqual({
settings: { region: "Self-hosted" },
});
expect(output["user2"]).toEqual({
settings: { region: "Self-hosted" },
});
});
// This is more realistic, a browser user could have signed into the application and logged out, then signed
// into a different account. Pre browser account switching, the state for the user _is_ kept on disk but the account
// id of the non-current account isn't saved to the authenticatedAccounts array so we don't have a great way to
// get the state and include it in our calculations for what the global state should be.
it("only cares about users defined in authenticatedAccounts", async () => {
const testInput: TestState = {
authenticatedAccounts: ["user1"],
global: {
theme: "system", // A real global setting that should persist after migration
},
user1: {
settings: {
disableAddLoginNotification: true,
disableChangedPasswordNotification: true,
disableContextMenuItem: true,
neverDomains: {
"example.com": null,
},
region: "Self-hosted",
},
},
user2: {
settings: {
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example2.com": null,
},
region: "Self-hosted",
},
},
};
const output = await runMigrator(myMigrator, testInput);
// The true settings should be respected over the false values because that whole users values
// shouldn't be respected.
// neverDomains should be combined into a single object
expect(output["global"]).toEqual({
theme: "system",
disableAddLoginNotification: true,
disableChangedPasswordNotification: true,
disableContextMenuItem: true,
neverDomains: {
"example.com": null,
},
});
expect(output["user1"]).toEqual({
settings: { region: "Self-hosted" },
});
expect(output["user2"]).toEqual({
settings: {
disableAddLoginNotification: false,
disableChangedPasswordNotification: false,
disableContextMenuItem: false,
neverDomains: {
"example2.com": null,
},
region: "Self-hosted",
},
});
});
});

View File

@ -0,0 +1,102 @@
import { MigrationHelper } from "../migration-helper";
import { Migrator } from "../migrator";
type NeverDomains = { [id: string]: unknown };
type ExpectedAccountType = {
settings?: {
neverDomains?: NeverDomains;
disableAddLoginNotification?: boolean;
disableChangedPasswordNotification?: boolean;
disableContextMenuItem?: boolean;
};
};
type TargetGlobalState = {
neverDomains?: NeverDomains;
disableAddLoginNotification?: boolean;
disableChangedPasswordNotification?: boolean;
disableContextMenuItem?: boolean;
};
export class MoveBrowserSettingsToGlobal extends Migrator<8, 9> {
// Will first check if any of the accounts have a value from the given accountSelector
// if they do have a value it will set that value into global state but if multiple
// users have differing values it will prefer the false setting,
// if all users have true then it will take true.
tryAddSetting(
accounts: { userId: string; account: ExpectedAccountType }[],
accountSelector: (account: ExpectedAccountType) => boolean | undefined,
globalSetter: (value: boolean | undefined) => void
): void {
const hasValue = accounts.some(({ account }) => {
return accountSelector(account) !== undefined;
});
if (hasValue) {
const value = !accounts.some(({ account }) => {
return (accountSelector(account) ?? false) === false;
});
globalSetter(value);
}
}
async migrate(helper: MigrationHelper): Promise<void> {
const global = await helper.get<object>("global");
const accounts = await helper.getAccounts<ExpectedAccountType>();
const globalNeverDomainsValue = accounts.reduce((accumulator, { account }) => {
const normalizedNeverDomains = account.settings?.neverDomains ?? {};
for (const [id, value] of Object.entries(normalizedNeverDomains)) {
accumulator ??= {};
accumulator[id] = value;
}
return accumulator;
}, undefined as NeverDomains);
const targetGlobalState: TargetGlobalState = {};
if (globalNeverDomainsValue != null) {
targetGlobalState.neverDomains = globalNeverDomainsValue;
}
this.tryAddSetting(
accounts,
(a) => a.settings?.disableAddLoginNotification,
(v) => (targetGlobalState.disableAddLoginNotification = v)
);
this.tryAddSetting(
accounts,
(a) => a.settings?.disableChangedPasswordNotification,
(v) => (targetGlobalState.disableChangedPasswordNotification = v)
);
this.tryAddSetting(
accounts,
(a) => a.settings?.disableContextMenuItem,
(v) => (targetGlobalState.disableContextMenuItem = v)
);
await helper.set<TargetGlobalState>("global", {
...global,
...targetGlobalState,
});
await Promise.all(
accounts.map(async ({ userId, account }) => {
delete account.settings?.disableAddLoginNotification;
delete account.settings?.disableChangedPasswordNotification;
delete account.settings?.disableContextMenuItem;
delete account.settings?.neverDomains;
await helper.set(userId, account);
})
);
}
rollback(helper: MigrationHelper): Promise<void> {
throw new Error("Method not implemented.");
}
}