From 560033cb880e617d649b07734db563d28da0e7ae Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Wed, 10 Apr 2024 08:01:34 -0500 Subject: [PATCH] Remove usages of `chrome.storage` (#8664) * Enable clearing and retrieving all values from local storage I didn't add these methods to the base abstract class because we don't currently have a use case for them. If we develop one, we can just lift it up. * Use new browser local storage methods for reseed task * Remove the now dangerous methods enabling usage of `chrome.storage` Any direct reference to chrome storage needs to handle serialization tags, which is best dealt with through the classes implementing `AbstractChromeStorageService` --- apps/browser/src/autofill/utils/index.spec.ts | 28 ------ apps/browser/src/autofill/utils/index.ts | 13 --- .../browser/src/background/main.background.ts | 16 +--- .../abstract-chrome-storage-api.service.ts | 26 ++++-- .../chrome-storage-api.service.spec.ts | 10 ++- .../browser-local-storage.service.spec.ts | 89 +++++++++++++++++++ .../services/browser-local-storage.service.ts | 28 ++++++ 7 files changed, 149 insertions(+), 61 deletions(-) create mode 100644 apps/browser/src/platform/services/browser-local-storage.service.spec.ts diff --git a/apps/browser/src/autofill/utils/index.spec.ts b/apps/browser/src/autofill/utils/index.spec.ts index 2fe8496b8d..af67d41601 100644 --- a/apps/browser/src/autofill/utils/index.spec.ts +++ b/apps/browser/src/autofill/utils/index.spec.ts @@ -8,7 +8,6 @@ import { generateRandomCustomElementName, sendExtensionMessage, setElementStyles, - getFromLocalStorage, setupExtensionDisconnectAction, setupAutofillInitDisconnectAction, } from "./index"; @@ -124,33 +123,6 @@ describe("setElementStyles", () => { }); }); -describe("getFromLocalStorage", () => { - it("returns a promise with the storage object pulled from the extension storage api", async () => { - const localStorage: Record = { - testValue: "test", - another: "another", - }; - jest.spyOn(chrome.storage.local, "get").mockImplementation((keys, callback) => { - const localStorageObject: Record = {}; - - if (typeof keys === "string") { - localStorageObject[keys] = localStorage[keys]; - } else if (Array.isArray(keys)) { - for (const key of keys) { - localStorageObject[key] = localStorage[key]; - } - } - - callback(localStorageObject); - }); - - const returnValue = await getFromLocalStorage("testValue"); - - expect(chrome.storage.local.get).toHaveBeenCalled(); - expect(returnValue).toEqual({ testValue: "test" }); - }); -}); - describe("setupExtensionDisconnectAction", () => { afterEach(() => { jest.clearAllMocks(); diff --git a/apps/browser/src/autofill/utils/index.ts b/apps/browser/src/autofill/utils/index.ts index 2644425d70..72e7f9ab62 100644 --- a/apps/browser/src/autofill/utils/index.ts +++ b/apps/browser/src/autofill/utils/index.ts @@ -106,18 +106,6 @@ function setElementStyles( } } -/** - * Get data from local storage based on the keys provided. - * - * @param keys - String or array of strings of keys to get from local storage - * @deprecated Do not call this, use state-relevant services instead - */ -async function getFromLocalStorage(keys: string | string[]): Promise> { - return new Promise((resolve) => { - chrome.storage.local.get(keys, (storage: Record) => resolve(storage)); - }); -} - /** * Sets up a long-lived connection with the extension background * and triggers an onDisconnect event if the extension context @@ -278,7 +266,6 @@ export { buildSvgDomElement, sendExtensionMessage, setElementStyles, - getFromLocalStorage, setupExtensionDisconnectAction, setupAutofillInitDisconnectAction, elementIsFillableFormField, diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index f649c5a598..a7bbca9cf0 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -234,7 +234,7 @@ import RuntimeBackground from "./runtime.background"; export default class MainBackground { messagingService: MessagingServiceAbstraction; - storageService: AbstractStorageService & ObservableStorageService; + storageService: BrowserLocalStorageService; secureStorageService: AbstractStorageService; memoryStorageService: AbstractMemoryStorageService; memoryStorageForStateProviders: AbstractMemoryStorageService & ObservableStorageService; @@ -1255,18 +1255,8 @@ export default class MainBackground { return; } - const getStorage = (): Promise => - new Promise((resolve) => { - chrome.storage.local.get(null, (o: any) => resolve(o)); - }); - - const clearStorage = (): Promise => - new Promise((resolve) => { - chrome.storage.local.clear(() => resolve()); - }); - - const storage = await getStorage(); - await clearStorage(); + const storage = await this.storageService.getAll(); + await this.storageService.clear(); for (const key in storage) { // eslint-disable-next-line diff --git a/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts b/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts index a5681d65c0..64935ab591 100644 --- a/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts +++ b/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts @@ -10,6 +10,8 @@ import { fromChromeEvent } from "../../browser/from-chrome-event"; export const serializationIndicator = "__json__"; +type serializedObject = { [serializationIndicator]: true; value: string }; + export const objToStore = (obj: any) => { if (obj == null) { return null; @@ -61,11 +63,7 @@ export default abstract class AbstractChromeStorageService return new Promise((resolve) => { this.chromeStorageApi.get(key, (obj: any) => { if (obj != null && obj[key] != null) { - let value = obj[key]; - if (value[serializationIndicator] && typeof value.value === "string") { - value = JSON.parse(value.value); - } - resolve(value as T); + resolve(this.processGetObject(obj[key])); return; } resolve(null); @@ -95,4 +93,22 @@ export default abstract class AbstractChromeStorageService }); }); } + + /** Backwards compatible resolution of retrieved object with new serialized storage */ + protected processGetObject(obj: T | serializedObject): T | null { + if (this.isSerialized(obj)) { + obj = JSON.parse(obj.value); + } + return obj as T; + } + + /** Type guard for whether an object is tagged as serialized */ + protected isSerialized(value: T | serializedObject): value is serializedObject { + const asSerialized = value as serializedObject; + return ( + asSerialized != null && + asSerialized[serializationIndicator] && + typeof asSerialized.value === "string" + ); + } } diff --git a/apps/browser/src/platform/services/abstractions/chrome-storage-api.service.spec.ts b/apps/browser/src/platform/services/abstractions/chrome-storage-api.service.spec.ts index bb89dc8a6a..812901879d 100644 --- a/apps/browser/src/platform/services/abstractions/chrome-storage-api.service.spec.ts +++ b/apps/browser/src/platform/services/abstractions/chrome-storage-api.service.spec.ts @@ -66,6 +66,7 @@ describe("ChromeStorageApiService", () => { describe("get", () => { let getMock: jest.Mock; + const key = "key"; beforeEach(() => { // setup get @@ -76,7 +77,6 @@ describe("ChromeStorageApiService", () => { }); it("returns a stored value when it is serialized", async () => { - const key = "key"; const value = { key: "value" }; store[key] = objToStore(value); const result = await service.get(key); @@ -84,7 +84,6 @@ describe("ChromeStorageApiService", () => { }); it("returns a stored value when it is not serialized", async () => { - const key = "key"; const value = "value"; store[key] = value; const result = await service.get(key); @@ -95,5 +94,12 @@ describe("ChromeStorageApiService", () => { const result = await service.get("key"); expect(result).toBeNull(); }); + + it("returns null when the stored object is null", async () => { + store[key] = null; + + const result = await service.get(key); + expect(result).toBeNull(); + }); }); }); diff --git a/apps/browser/src/platform/services/browser-local-storage.service.spec.ts b/apps/browser/src/platform/services/browser-local-storage.service.spec.ts new file mode 100644 index 0000000000..37ea37dbf6 --- /dev/null +++ b/apps/browser/src/platform/services/browser-local-storage.service.spec.ts @@ -0,0 +1,89 @@ +import { objToStore } from "./abstractions/abstract-chrome-storage-api.service"; +import BrowserLocalStorageService from "./browser-local-storage.service"; + +describe("BrowserLocalStorageService", () => { + let service: BrowserLocalStorageService; + let store: Record; + + beforeEach(() => { + store = {}; + + service = new BrowserLocalStorageService(); + }); + + describe("clear", () => { + let clearMock: jest.Mock; + + beforeEach(() => { + clearMock = chrome.storage.local.clear as jest.Mock; + }); + + it("uses the api to clear", async () => { + await service.clear(); + + expect(clearMock).toHaveBeenCalledTimes(1); + }); + }); + + describe("getAll", () => { + let getMock: jest.Mock; + + beforeEach(() => { + // setup get + getMock = chrome.storage.local.get as jest.Mock; + getMock.mockImplementation((key, callback) => { + if (key == null) { + callback(store); + } else { + callback({ [key]: store[key] }); + } + }); + }); + + it("returns all values", async () => { + store["key1"] = "string"; + store["key2"] = 0; + const result = await service.getAll(); + + expect(result).toEqual(store); + }); + + it("handles empty stores", async () => { + const result = await service.getAll(); + + expect(result).toEqual({}); + }); + + it("handles stores with null values", async () => { + store["key"] = null; + + const result = await service.getAll(); + expect(result).toEqual(store); + }); + + it("handles values processed for storage", async () => { + const obj = { test: 2 }; + const key = "key"; + store[key] = objToStore(obj); + + const result = await service.getAll(); + + expect(result).toEqual({ + [key]: obj, + }); + }); + + // This is a test of backwards compatibility before local storage was serialized. + it("handles values that were stored without processing for storage", async () => { + const obj = { test: 2 }; + const key = "key"; + store[key] = obj; + + const result = await service.getAll(); + + expect(result).toEqual({ + [key]: obj, + }); + }); + }); +}); diff --git a/apps/browser/src/platform/services/browser-local-storage.service.ts b/apps/browser/src/platform/services/browser-local-storage.service.ts index 2efd03a046..e1f9f63676 100644 --- a/apps/browser/src/platform/services/browser-local-storage.service.ts +++ b/apps/browser/src/platform/services/browser-local-storage.service.ts @@ -4,4 +4,32 @@ export default class BrowserLocalStorageService extends AbstractChromeStorageSer constructor() { super(chrome.storage.local); } + + /** + * Clears local storage + */ + async clear() { + await chrome.storage.local.clear(); + } + + /** + * Retrieves all objects stored in local storage. + * + * @remarks This method processes values prior to resolving, do not use `chrome.storage.local` directly + * @returns Promise resolving to keyed object of all stored data + */ + async getAll(): Promise> { + return new Promise((resolve) => { + this.chromeStorageApi.get(null, (allStorage) => { + const resolved = Object.entries(allStorage).reduce( + (agg, [key, value]) => { + agg[key] = this.processGetObject(value); + return agg; + }, + {} as Record, + ); + resolve(resolved); + }); + }); + } }