From 6a3f7bb554fbbd4906ed6540f69468c1a7f7b48f Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 14:26:25 -0500 Subject: [PATCH 01/16] #7618: [WIP] managedStorage delay and API improvements --- src/store/enterprise/managedStorage.test.ts | 15 ++ src/store/enterprise/managedStorage.ts | 160 ++++++++++++++------ src/utils/promiseUtils.ts | 6 + 3 files changed, 135 insertions(+), 46 deletions(-) diff --git a/src/store/enterprise/managedStorage.test.ts b/src/store/enterprise/managedStorage.test.ts index 33f410ae8b..2dc92450ce 100644 --- a/src/store/enterprise/managedStorage.test.ts +++ b/src/store/enterprise/managedStorage.test.ts @@ -20,6 +20,7 @@ import { readManagedStorage, readManagedStorageByKey, } from "@/store/enterprise/managedStorage"; +import { sleep } from "@/utils/timeUtils"; beforeEach(async () => { // eslint-disable-next-line new-cap -- test helper method @@ -52,3 +53,17 @@ describe("readManagedStorageByKey", () => { ); }); }); + +describe("watchStorageInitialization", () => { + it("watches initialization", async () => { + await expect(readManagedStorageByKey("partnerId")).resolves.toBeUndefined(); + + await sleep(4000); + + await browser.storage.managed.set({ partnerId: "taco-bell" }); + + await expect(readManagedStorageByKey("partnerId")).resolves.toBe( + "taco-bell", + ); + }, 10_000); +}); diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index a009737c5f..24d82372a4 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -15,29 +15,53 @@ * along with this program. If not, see . */ +/** + * @file A wrapper around the browser.storage.managed that tries to smooth over its initialization quirks and provide + * an interface for React's useExternalStore + */ + import { type ManagedStorageState } from "@/store/enterprise/managedStorageTypes"; import { isEmpty, once } from "lodash"; import { expectContext } from "@/utils/expectContext"; import pMemoize, { pMemoizeClear } from "p-memoize"; import { pollUntilTruthy } from "@/utils/promiseUtils"; +import type { Nullishable } from "@/utils/nullishUtils"; -const MAX_MANAGED_STORAGE_WAIT_MILLIS = 2000; +// 2024-02-15: bumped to 3.5s because 2s was too short: https://github.com/pixiebrix/pixiebrix-extension/issues/7618 +// Privacy Badger uses 4.5s timeout, but thinks policy should generally be available within 2.5s +const MAX_MANAGED_STORAGE_WAIT_MILLIS = 3500; + +/** + * Interval for checking managed storage initialization that takes longer than MAX_MANAGED_STORAGE_WAIT_MILLIS seconds. + */ +let initializationInterval: Nullishable>; /** * The managedStorageState, or undefined if it hasn't been initialized yet. */ -let managedStorageState: ManagedStorageState | undefined; +let managedStorageSnapshot: Nullishable; + +type ChangeListener = (state: ManagedStorageState) => void; -// TODO: Use `SimpleEventTarget` instead +// TODO: Use `SimpleEventTarget` instead -- need to add functionality to clear all listeners for INTERNAL_reset // eslint-disable-next-line local-rules/persistBackgroundData -- Functions -const listeners = new Set<(state: ManagedStorageState) => void>(); +const changeListeners = new Set(); -function notifyAll(managedStorageState: ManagedStorageState): void { - for (const listener of listeners) { - listener(managedStorageState); +function notifyAllChangeListeners( + managedStorageState: ManagedStorageState, +): void { + for (const listener of changeListeners) { + try { + listener(managedStorageState); + } catch { + // NOP - don't let a single listener error prevent others from being notified + } } } +/** + * Read managed storage immediately, returns {} if managed storage is unavailable/uninitialized. + */ async function readManagedStorageImmediately(): Promise { try { // Get all managed storage values @@ -49,7 +73,44 @@ async function readManagedStorageImmediately(): Promise { } } +/** + * Read managed storage immediately, returning undefined if not initialized or no policy is set. + * @see readManagedStorageImmediately + */ +async function readPopulatedManagedStorage(): Promise< + Nullishable +> { + const values = await readManagedStorageImmediately(); + if (typeof values === "object" && !isEmpty(values)) { + return values; + } +} + +/** + * Watch for managed storage initialization that occurs after waitForInitialManagedStorage + * + * We can't use `browser.storage.onChanged` because it doesn't fire on initialization. + * + * @see waitForInitialManagedStorage + */ +function watchStorageInitialization(): void { + initializationInterval = setInterval( + async () => { + const values = await readPopulatedManagedStorage(); + if (values != null) { + managedStorageSnapshot = values; + clearInterval(initializationInterval); + initializationInterval = undefined; + notifyAllChangeListeners(managedStorageSnapshot); + } + }, + // Most likely there's no policy. So only check once every 2 seconds to not consume resources + 2000, + ); +} + // It's possible that managed storage is not available on the initial install event + // Privacy Badger does a looping check for managed storage // - https://github.com/EFForg/privacybadger/blob/aeed0539603356a2825e7ce8472f6478abdc85fb/src/js/storage.js // - https://github.com/EFForg/privacybadger/issues/2770#issuecomment-853329201 @@ -57,47 +118,52 @@ async function readManagedStorageImmediately(): Promise { // uBlock (still) contains a workaround to automatically reload the extension on initial install // - https://github.com/gorhill/uBlock/commit/32bd47f05368557044dd3441dcaa414b7b009b39 const waitForInitialManagedStorage = pMemoize(async () => { - managedStorageState = await pollUntilTruthy( - async () => { - const values = await readManagedStorageImmediately(); - if (typeof values === "object" && !isEmpty(values)) { - return values; - } - }, + // Returns undefined if the promise times out + managedStorageSnapshot = await pollUntilTruthy( + readPopulatedManagedStorage, { maxWaitMillis: MAX_MANAGED_STORAGE_WAIT_MILLIS, }, ); - if (managedStorageState) { - notifyAll(managedStorageState); - - console.info("Read managed storage settings", { - managedStorageState, - }); - } else { - console.info("No manual storage settings found", { - managedStorageState, - }); + if (managedStorageSnapshot == null) { + // Watch for delayed initialization + watchStorageInitialization(); } - return managedStorageState; + console.info("Found managed storage settings", { + managedStorageState: managedStorageSnapshot, + }); + + // After timeout, assume there's no policy set, so assign an empty value + managedStorageSnapshot ??= {}; + + notifyAllChangeListeners(managedStorageSnapshot); + + return managedStorageSnapshot; }); /** - * Initialize the managed storage state and listen for changes. Safe to call multiple times. + * Initialize the managed storage state once and listen for changes. Safe to call multiple times. */ export const initManagedStorage = once(() => { expectContext("extension"); try { // https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/onChanged + // `onChanged` is only called when the policy changes, not on initialization // `browser.storage.managed.onChanged` might also exist, but it's not available in testing // See: https://github.com/clarkbw/jest-webextension-mock/issues/170 browser.storage.onChanged.addListener(async (changes, area) => { if (area === "managed") { - managedStorageState = await readManagedStorageImmediately(); - notifyAll(managedStorageState); + // If browser.storage.onChanged fires, it means storage must already be initialized + if (initializationInterval) { + clearInterval(initializationInterval); + initializationInterval = undefined; + } + + managedStorageSnapshot = await readManagedStorageImmediately(); + notifyAllChangeListeners(managedStorageSnapshot); } }); } catch (error) { @@ -114,8 +180,7 @@ export const initManagedStorage = once(() => { /** * Read a single-value from enterprise managed storage. * - * If managed storage has not been initialized yet, reads from the managed storage API. Waits up to - * MAX_MANAGED_STORAGE_WAIT_MILLIS for the data to be available. + * If managed storage has not been initialized yet, waits up to MAX_MANAGED_STORAGE_WAIT_MILLIS for the data to be available. * * @param key the key to read. * @see MAX_MANAGED_STORAGE_WAIT_MILLIS @@ -125,13 +190,14 @@ export async function readManagedStorageByKey< >(key: K): Promise { expectContext("extension"); - if (managedStorageState != null) { + if (managedStorageSnapshot != null) { + // Safe to read snapshot because snapshot is updated via change handler // eslint-disable-next-line security/detect-object-injection -- type-checked key - return managedStorageState[key]; + return managedStorageSnapshot[key]; } initManagedStorage(); - const storage = (await waitForInitialManagedStorage()) ?? {}; + const storage = await waitForInitialManagedStorage(); // eslint-disable-next-line security/detect-object-injection -- type-checked key return storage[key]; } @@ -139,20 +205,20 @@ export async function readManagedStorageByKey< /** * Read a managed storage state from enterprise managed storage. * - * If managed storage has not been initialized yet, reads from the managed storage API. Waits up to - * MAX_MANAGED_STORAGE_WAIT_MILLIS for the data to be available. + * If managed storage has not been initialized yet, waits up to MAX_MANAGED_STORAGE_WAIT_MILLIS for the data to + * be available. * * @see MAX_MANAGED_STORAGE_WAIT_MILLIS */ export async function readManagedStorage(): Promise { expectContext("extension"); - if (managedStorageState != null) { - return managedStorageState; + if (managedStorageSnapshot != null) { + return managedStorageSnapshot; } initManagedStorage(); - return (await waitForInitialManagedStorage()) ?? {}; + return waitForInitialManagedStorage(); } /** @@ -160,15 +226,15 @@ export async function readManagedStorage(): Promise { * @see useManagedStorageState * @see readManagedStorage */ -export function getSnapshot(): ManagedStorageState | undefined { +export function getSnapshot(): Nullishable { expectContext("extension"); - return managedStorageState; + return managedStorageSnapshot; } /** - * Subscribe to changes in the managed storage state. In practice, this should only fire once because managed - * storage is not mutable. + * Subscribe to changes in the managed storage state. + * * @param callback to receive the updated state. * @see useManagedStorageState */ @@ -177,10 +243,10 @@ export function subscribe( ): () => void { expectContext("extension"); - listeners.add(callback); + changeListeners.add(callback); return () => { - listeners.delete(callback); + changeListeners.delete(callback); }; } @@ -188,7 +254,9 @@ export function subscribe( * Helper method for resetting the module for testing. */ export function INTERNAL_reset(): void { - managedStorageState = undefined; - listeners.clear(); + managedStorageSnapshot = undefined; + changeListeners.clear(); + clearInterval(initializationInterval); + initializationInterval = undefined; pMemoizeClear(waitForInitialManagedStorage); } diff --git a/src/utils/promiseUtils.ts b/src/utils/promiseUtils.ts index 5efceee569..2a92cd6f6d 100644 --- a/src/utils/promiseUtils.ts +++ b/src/utils/promiseUtils.ts @@ -105,6 +105,12 @@ export async function awaitValue( throw new TimeoutError(`Value not found after ${waitMillis} milliseconds`); } +/** + * Poll until the looper returns a truthy value. If the timeout is reached, return undefined. + * @param looper the value generator + * @param maxWaitMillis maximium time to wait for the value + * @param intervalMillis time between each call to looper + */ export async function pollUntilTruthy( looper: (...args: unknown[]) => Promise | T, { maxWaitMillis = Number.MAX_SAFE_INTEGER, intervalMillis = 100 }, From ce021638373efd3b4452e723f0048a3e0386f0be Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 14:49:23 -0500 Subject: [PATCH 02/16] #7618: don't wait for managed storage for end-user onboarding --- src/background/installer.ts | 31 ++++++++++++++++++++++++-- src/store/enterprise/managedStorage.ts | 12 ++++++---- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/background/installer.ts b/src/background/installer.ts index fdfe1b58b6..28236e8dff 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -40,6 +40,27 @@ import { getExtensionConsoleUrl } from "@/utils/extensionUtils"; */ let _availableVersion: string | null = null; +/** + * Returns true if this appears to be a Chrome Web Store install and/or the user has an app URL where they're + * authenticated so the extension can be linked. + */ +async function isEndUserInstall(): Promise { + const onboardingTabs = await browser.tabs.query({ + // Can't use SERVICE_URL directly because it contains a port number during development, resulting in an + // invalid URL match pattern + url: [ + // App Setup / Authenticated URLs + new URL("setup", DEFAULT_SERVICE_URL).href, + DEFAULT_SERVICE_URL, + // Known CWS URLs: https://docs.pixiebrix.com/enterprise-it-setup/network-email-firewall-configuration + "https://chromewebstore.google.com/detail/pixiebrix/mpjjildhmpddojocokjkgmlkkkfjnepo", + "https://chrome.google.com/webstore/detail/pixiebrix-webapp-integrat/mpjjildhmpddojocokjkgmlkkkfjnepo", + ], + }); + + return onboardingTabs.length > 0; +} + /** * Install handler to complete authentication configuration for the extension. */ @@ -199,8 +220,14 @@ export async function handleInstall({ // XXX: under what conditions could onInstalled fire, but the extension is already linked? Is this the case during // development/loading an update of the extension from the file system? if (!(await isLinked())) { - // PERFORMANCE: readManagedStorageByKey waits up to 2 seconds for managed storage to be available. Shouldn't be - // notice-able for end-user relative to the extension download/install time + // If an end-user appears to be installing, jump to linking directly vs. waiting for readManagedStorage because + // readManagedStorage will wait until a timeout for managed storage to be available. + if (await isEndUserInstall()) { + await openInstallPage(); + return; + } + + // Reminder: readManagedStorageByKey waits up to 4.5 seconds for managed storage to be available const { ssoUrl, partnerId, controlRoomUrl, disableLoginTab } = await readManagedStorage(); diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index 24d82372a4..894c978c3e 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -27,9 +27,10 @@ import pMemoize, { pMemoizeClear } from "p-memoize"; import { pollUntilTruthy } from "@/utils/promiseUtils"; import type { Nullishable } from "@/utils/nullishUtils"; -// 2024-02-15: bumped to 3.5s because 2s was too short: https://github.com/pixiebrix/pixiebrix-extension/issues/7618 -// Privacy Badger uses 4.5s timeout, but thinks policy should generally be available within 2.5s -const MAX_MANAGED_STORAGE_WAIT_MILLIS = 3500; +// 2024-02-15: bumped to 4.5s because 2s was too short: https://github.com/pixiebrix/pixiebrix-extension/issues/7618 +// Privacy Badger uses 4.5s timeout, but thinks policy should generally be available within 2.5s. In installer.ts, +// we check for app tabs to skip the linking wait if the user appears to be installing from the web store. +const MAX_MANAGED_STORAGE_WAIT_MILLIS = 4500; /** * Interval for checking managed storage initialization that takes longer than MAX_MANAGED_STORAGE_WAIT_MILLIS seconds. @@ -87,10 +88,13 @@ async function readPopulatedManagedStorage(): Promise< } /** - * Watch for managed storage initialization that occurs after waitForInitialManagedStorage + * Watch for managed storage initialization that occurs after waitForInitialManagedStorage. * * We can't use `browser.storage.onChanged` because it doesn't fire on initialization. * + * Required because other modules are using the values in managedStorageSnapshot vs. calling browser.storage.managed.get + * directly. + * * @see waitForInitialManagedStorage */ function watchStorageInitialization(): void { From 929e2cc7f84bc17aff1cba2faf4477f473ea11b5 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 15:01:03 -0500 Subject: [PATCH 03/16] #7618: immediately initialize managed storage on module load --- src/background/background.ts | 2 ++ src/background/installer.ts | 7 +++++-- src/store/enterprise/managedStorage.ts | 8 ++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/background/background.ts b/src/background/background.ts index 384c4ba173..94ee09f797 100644 --- a/src/background/background.ts +++ b/src/background/background.ts @@ -48,6 +48,7 @@ import { initRuntimeLogging } from "@/development/runtimeLogging"; import initWalkthroughModalTrigger from "@/background/walkthroughModalTrigger"; import { initSidePanel } from "./sidePanel"; import initRestrictUnauthenticatedUrlAccess from "@/background/restrictUnauthenticatedUrlAccess"; +import { initManagedStorage } from "@/store/enterprise/managedStorage"; void initLocator(); void initMessengerLogging(); @@ -64,6 +65,7 @@ initContextMenus(); initContentScriptReadyListener(); initBrowserCommands(); initDeploymentUpdater(); +initManagedStorage(); void activateBrowserActionIcon(); initPartnerTheme(); initStarterMods(); diff --git a/src/background/installer.ts b/src/background/installer.ts index 28236e8dff..242020be94 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -27,7 +27,10 @@ import { getExtensionToken, getUserData, isLinked } from "@/auth/token"; import { isCommunityControlRoom } from "@/contrib/automationanywhere/aaUtils"; import { isEmpty } from "lodash"; import { expectContext } from "@/utils/expectContext"; -import { readManagedStorage } from "@/store/enterprise/managedStorage"; +import { + readManagedStorage, + isInitialized as isManagedStorageInitialized, +} from "@/store/enterprise/managedStorage"; import { Events } from "@/telemetry/events"; import { DEFAULT_SERVICE_URL, UNINSTALL_URL } from "@/urlConstants"; @@ -222,7 +225,7 @@ export async function handleInstall({ if (!(await isLinked())) { // If an end-user appears to be installing, jump to linking directly vs. waiting for readManagedStorage because // readManagedStorage will wait until a timeout for managed storage to be available. - if (await isEndUserInstall()) { + if (!isManagedStorageInitialized() && (await isEndUserInstall())) { await openInstallPage(); return; } diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index 894c978c3e..19c9371b2f 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -225,6 +225,14 @@ export async function readManagedStorage(): Promise { return waitForInitialManagedStorage(); } +/** + * Returns true if managed storage has been initialized. + * @see waitForInitialManagedStorage + */ +export function isInitialized(): boolean { + return managedStorageSnapshot != null; +} + /** * Get a _synchronous_ snapshot of the managed storage state. * @see useManagedStorageState From 809bd22659fd21586ab33a097764b32e86e0ea68 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 15:01:54 -0500 Subject: [PATCH 04/16] Clarify comment --- src/background/background.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/background/background.ts b/src/background/background.ts index 94ee09f797..041c7bac2f 100644 --- a/src/background/background.ts +++ b/src/background/background.ts @@ -50,6 +50,9 @@ import { initSidePanel } from "./sidePanel"; import initRestrictUnauthenticatedUrlAccess from "@/background/restrictUnauthenticatedUrlAccess"; import { initManagedStorage } from "@/store/enterprise/managedStorage"; +// Try to initialize managed storage as early as possible because it impacts background behavior +initManagedStorage(); + void initLocator(); void initMessengerLogging(); void initRuntimeLogging(); @@ -65,7 +68,6 @@ initContextMenus(); initContentScriptReadyListener(); initBrowserCommands(); initDeploymentUpdater(); -initManagedStorage(); void activateBrowserActionIcon(); initPartnerTheme(); initStarterMods(); From a0bf24fb1d685baa5f518541cc45ef861580577e Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 15:14:26 -0500 Subject: [PATCH 05/16] Fix CWS detection --- src/background/installer.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/background/installer.ts b/src/background/installer.ts index 242020be94..ab6430361d 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -48,6 +48,8 @@ let _availableVersion: string | null = null; * authenticated so the extension can be linked. */ async function isEndUserInstall(): Promise { + // Query existing app/CWS tabs: https://developer.chrome.com/docs/extensions/reference/api/tabs#method-query + // `browser.tabs.query` supports https://developer.chrome.com/docs/extensions/develop/concepts/match-patterns const onboardingTabs = await browser.tabs.query({ // Can't use SERVICE_URL directly because it contains a port number during development, resulting in an // invalid URL match pattern @@ -56,12 +58,17 @@ async function isEndUserInstall(): Promise { new URL("setup", DEFAULT_SERVICE_URL).href, DEFAULT_SERVICE_URL, // Known CWS URLs: https://docs.pixiebrix.com/enterprise-it-setup/network-email-firewall-configuration - "https://chromewebstore.google.com/detail/pixiebrix/mpjjildhmpddojocokjkgmlkkkfjnepo", - "https://chrome.google.com/webstore/detail/pixiebrix-webapp-integrat/mpjjildhmpddojocokjkgmlkkkfjnepo", + "https://chromewebstore.google.com/*", + "https://chrome.google.com/webstore/*", ], }); - return onboardingTabs.length > 0; + // The CWS install URL differs based on the extension listing slug. So instead, only match on the runtime id. + return onboardingTabs.some( + (tab) => + tab.url.includes(DEFAULT_SERVICE_URL) || + tab.url.includes(browser.runtime.id), + ); } /** From 9179f057af48f585b1c03e8934591f3d9f53c9db Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 15:15:32 -0500 Subject: [PATCH 06/16] Improve variable name --- src/background/installer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/background/installer.ts b/src/background/installer.ts index ab6430361d..9ea24d5afe 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -50,7 +50,7 @@ let _availableVersion: string | null = null; async function isEndUserInstall(): Promise { // Query existing app/CWS tabs: https://developer.chrome.com/docs/extensions/reference/api/tabs#method-query // `browser.tabs.query` supports https://developer.chrome.com/docs/extensions/develop/concepts/match-patterns - const onboardingTabs = await browser.tabs.query({ + const likelyOnboardingTabs = await browser.tabs.query({ // Can't use SERVICE_URL directly because it contains a port number during development, resulting in an // invalid URL match pattern url: [ @@ -64,7 +64,7 @@ async function isEndUserInstall(): Promise { }); // The CWS install URL differs based on the extension listing slug. So instead, only match on the runtime id. - return onboardingTabs.some( + return likelyOnboardingTabs.some( (tab) => tab.url.includes(DEFAULT_SERVICE_URL) || tab.url.includes(browser.runtime.id), From 7a20f9bb95e10696f0eecd6c74cfa05f2186480c Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 17:00:34 -0500 Subject: [PATCH 07/16] Fix strict null check errors --- src/background/installer.ts | 2 ++ src/store/enterprise/managedStorage.test.ts | 15 ----------- src/store/enterprise/managedStorage.ts | 27 ++++++++++++------- .../enterprise/useManagedStorageState.ts | 5 ++-- 4 files changed, 22 insertions(+), 27 deletions(-) diff --git a/src/background/installer.ts b/src/background/installer.ts index 9ea24d5afe..8f99ff82f6 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -233,6 +233,8 @@ export async function handleInstall({ // If an end-user appears to be installing, jump to linking directly vs. waiting for readManagedStorage because // readManagedStorage will wait until a timeout for managed storage to be available. if (!isManagedStorageInitialized() && (await isEndUserInstall())) { + console.debug("Skipping readManagedStorage for end-user install"); + await openInstallPage(); return; } diff --git a/src/store/enterprise/managedStorage.test.ts b/src/store/enterprise/managedStorage.test.ts index 2dc92450ce..33f410ae8b 100644 --- a/src/store/enterprise/managedStorage.test.ts +++ b/src/store/enterprise/managedStorage.test.ts @@ -20,7 +20,6 @@ import { readManagedStorage, readManagedStorageByKey, } from "@/store/enterprise/managedStorage"; -import { sleep } from "@/utils/timeUtils"; beforeEach(async () => { // eslint-disable-next-line new-cap -- test helper method @@ -53,17 +52,3 @@ describe("readManagedStorageByKey", () => { ); }); }); - -describe("watchStorageInitialization", () => { - it("watches initialization", async () => { - await expect(readManagedStorageByKey("partnerId")).resolves.toBeUndefined(); - - await sleep(4000); - - await browser.storage.managed.set({ partnerId: "taco-bell" }); - - await expect(readManagedStorageByKey("partnerId")).resolves.toBe( - "taco-bell", - ); - }, 10_000); -}); diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index 19c9371b2f..ef31969eba 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -103,8 +103,12 @@ function watchStorageInitialization(): void { const values = await readPopulatedManagedStorage(); if (values != null) { managedStorageSnapshot = values; - clearInterval(initializationInterval); - initializationInterval = undefined; + + if (initializationInterval) { + clearInterval(initializationInterval); + initializationInterval = undefined; + } + notifyAllChangeListeners(managedStorageSnapshot); } }, @@ -123,12 +127,11 @@ function watchStorageInitialization(): void { // - https://github.com/gorhill/uBlock/commit/32bd47f05368557044dd3441dcaa414b7b009b39 const waitForInitialManagedStorage = pMemoize(async () => { // Returns undefined if the promise times out - managedStorageSnapshot = await pollUntilTruthy( - readPopulatedManagedStorage, - { - maxWaitMillis: MAX_MANAGED_STORAGE_WAIT_MILLIS, - }, - ); + managedStorageSnapshot = await pollUntilTruthy< + Nullishable + >(readPopulatedManagedStorage, { + maxWaitMillis: MAX_MANAGED_STORAGE_WAIT_MILLIS, + }); if (managedStorageSnapshot == null) { // Watch for delayed initialization @@ -268,7 +271,11 @@ export function subscribe( export function INTERNAL_reset(): void { managedStorageSnapshot = undefined; changeListeners.clear(); - clearInterval(initializationInterval); - initializationInterval = undefined; + + if (initializationInterval != null) { + clearInterval(initializationInterval); + initializationInterval = undefined; + } + pMemoizeClear(waitForInitialManagedStorage); } diff --git a/src/store/enterprise/useManagedStorageState.ts b/src/store/enterprise/useManagedStorageState.ts index b8b8551e88..e79001c239 100644 --- a/src/store/enterprise/useManagedStorageState.ts +++ b/src/store/enterprise/useManagedStorageState.ts @@ -22,10 +22,11 @@ import { subscribe, } from "@/store/enterprise/managedStorage"; import { useEffect } from "react"; -import { type ManagedStorageState } from "@/store/enterprise/managedStorageTypes"; +import type { ManagedStorageState } from "@/store/enterprise/managedStorageTypes"; +import type { Nullishable } from "@/utils/nullishUtils"; type HookState = { - data: ManagedStorageState | undefined; + data: Nullishable; isLoading: boolean; }; From 05283b02d6515779c7742d56fc51c39c4dd125ac Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 17:13:05 -0500 Subject: [PATCH 08/16] Fix multiple tabs on managed organization id --- src/background/deploymentUpdater.ts | 2 +- src/background/installer.ts | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/background/deploymentUpdater.ts b/src/background/deploymentUpdater.ts index 445e3eab75..8b9afc6f5a 100644 --- a/src/background/deploymentUpdater.ts +++ b/src/background/deploymentUpdater.ts @@ -421,7 +421,7 @@ export async function updateDeployments(): Promise { sso: false, }); - void browser.runtime.openOptionsPage(); + await browser.runtime.openOptionsPage(); return; } diff --git a/src/background/installer.ts b/src/background/installer.ts index 8f99ff82f6..d79f579cab 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -158,7 +158,7 @@ export async function openInstallPage() { active: true, }); } else { - // Case 3: there's no Admin Console onboarding tab open + // Case 3: there's no Admin Console onboarding tab open. // // Open a new Admin Console tab which will automatically "links" the extension (by passing the native PixieBrix // token to the extension). @@ -240,16 +240,21 @@ export async function handleInstall({ } // Reminder: readManagedStorageByKey waits up to 4.5 seconds for managed storage to be available - const { ssoUrl, partnerId, controlRoomUrl, disableLoginTab } = - await readManagedStorage(); + const { + ssoUrl, + partnerId, + controlRoomUrl, + disableLoginTab, + managedOrganizationId, + } = await readManagedStorage(); if (disableLoginTab) { // IT manager has disabled the login tab return; } - if (ssoUrl) { - // Don't launch the SSO page automatically. The SSO flow will be launched by deploymentUpdater.ts:updateDeployments + if (ssoUrl || managedOrganizationId) { + // Don't launch the page automatically. The SSO flow will be launched by deploymentUpdater.ts:updateDeployments return; } From f65e55e98411a1306be6d2ffb3a8907ad540c7e5 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 17:22:56 -0500 Subject: [PATCH 09/16] #7618: fix match pattern --- src/background/installer.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/background/installer.ts b/src/background/installer.ts index d79f579cab..fc9f4e8b89 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -54,9 +54,10 @@ async function isEndUserInstall(): Promise { // Can't use SERVICE_URL directly because it contains a port number during development, resulting in an // invalid URL match pattern url: [ - // App Setup / Authenticated URLs + // Setup page is page before sending user to the CWS new URL("setup", DEFAULT_SERVICE_URL).href, - DEFAULT_SERVICE_URL, + // Base page is extension linking page. Needs path to be a valid URL match pattern + new URL("", DEFAULT_SERVICE_URL).href, // Known CWS URLs: https://docs.pixiebrix.com/enterprise-it-setup/network-email-firewall-configuration "https://chromewebstore.google.com/*", "https://chrome.google.com/webstore/*", From 27ee2a682469fdc33e93b0a3b3a6af3793559484 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 17:33:14 -0500 Subject: [PATCH 10/16] Bump timer to accomodate extra delay --- src/background/deploymentUpdater.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/background/deploymentUpdater.test.ts b/src/background/deploymentUpdater.test.ts index 5ce4cd992d..a278ca61c1 100644 --- a/src/background/deploymentUpdater.test.ts +++ b/src/background/deploymentUpdater.test.ts @@ -416,7 +416,7 @@ describe("updateDeployments", () => { expect(jest.mocked(uninstallAllDeployments).mock.calls).toHaveLength(0); expect(refreshRegistriesMock.mock.calls).toHaveLength(0); expect(saveSettingsStateMock).toHaveBeenCalledTimes(0); - }); + }, 10_000); test("do not open options page on update if restricted-version flag not set", async () => { isLinkedMock.mockResolvedValue(true); From 7d21b0b69721ce1e97380111eb3cc270c9b2efcd Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 18:10:43 -0500 Subject: [PATCH 11/16] #7618: remove side-effect from waitForInitialManagedStorage --- src/background/background.ts | 8 +++++-- src/store/enterprise/managedStorage.ts | 24 ++++++++++--------- .../enterprise/useManagedStorageState.ts | 2 +- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/background/background.ts b/src/background/background.ts index 041c7bac2f..990e051050 100644 --- a/src/background/background.ts +++ b/src/background/background.ts @@ -48,10 +48,14 @@ import { initRuntimeLogging } from "@/development/runtimeLogging"; import initWalkthroughModalTrigger from "@/background/walkthroughModalTrigger"; import { initSidePanel } from "./sidePanel"; import initRestrictUnauthenticatedUrlAccess from "@/background/restrictUnauthenticatedUrlAccess"; -import { initManagedStorage } from "@/store/enterprise/managedStorage"; +import { + initManagedStorage, + watchStorageInitialization, +} from "@/store/enterprise/managedStorage"; // Try to initialize managed storage as early as possible because it impacts background behavior -initManagedStorage(); +// Call watchStorageInitialization to handle case where storage is not immediately available within timeout +void initManagedStorage().then(async () => watchStorageInitialization()); void initLocator(); void initMessengerLogging(); diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index ef31969eba..b8048e0421 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -97,7 +97,14 @@ async function readPopulatedManagedStorage(): Promise< * * @see waitForInitialManagedStorage */ -function watchStorageInitialization(): void { +export async function watchStorageInitialization(): Promise { + const values = await readPopulatedManagedStorage(); + + if (values) { + // Already initialized + return; + } + initializationInterval = setInterval( async () => { const values = await readPopulatedManagedStorage(); @@ -117,7 +124,7 @@ function watchStorageInitialization(): void { ); } -// It's possible that managed storage is not available on the initial install event +// It's possible that managed storage is not available on the initial installation event // Privacy Badger does a looping check for managed storage // - https://github.com/EFForg/privacybadger/blob/aeed0539603356a2825e7ce8472f6478abdc85fb/src/js/storage.js @@ -133,11 +140,6 @@ const waitForInitialManagedStorage = pMemoize(async () => { maxWaitMillis: MAX_MANAGED_STORAGE_WAIT_MILLIS, }); - if (managedStorageSnapshot == null) { - // Watch for delayed initialization - watchStorageInitialization(); - } - console.info("Found managed storage settings", { managedStorageState: managedStorageSnapshot, }); @@ -153,7 +155,7 @@ const waitForInitialManagedStorage = pMemoize(async () => { /** * Initialize the managed storage state once and listen for changes. Safe to call multiple times. */ -export const initManagedStorage = once(() => { +export const initManagedStorage = once(async () => { expectContext("extension"); try { @@ -181,7 +183,7 @@ export const initManagedStorage = once(() => { ); } - void waitForInitialManagedStorage(); + await waitForInitialManagedStorage(); }); /** @@ -203,7 +205,7 @@ export async function readManagedStorageByKey< return managedStorageSnapshot[key]; } - initManagedStorage(); + void initManagedStorage(); const storage = await waitForInitialManagedStorage(); // eslint-disable-next-line security/detect-object-injection -- type-checked key return storage[key]; @@ -224,7 +226,7 @@ export async function readManagedStorage(): Promise { return managedStorageSnapshot; } - initManagedStorage(); + void initManagedStorage(); return waitForInitialManagedStorage(); } diff --git a/src/store/enterprise/useManagedStorageState.ts b/src/store/enterprise/useManagedStorageState.ts index e79001c239..7e640cd093 100644 --- a/src/store/enterprise/useManagedStorageState.ts +++ b/src/store/enterprise/useManagedStorageState.ts @@ -35,7 +35,7 @@ type HookState = { */ function useManagedStorageState(): HookState { useEffect(() => { - initManagedStorage(); + void initManagedStorage(); }, []); const data = useSyncExternalStore(subscribe, getSnapshot); From 972c7620a7faf5e664a236ed5e16d24877f234a9 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 18:11:29 -0500 Subject: [PATCH 12/16] Use consistent expressions --- src/store/enterprise/managedStorage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index b8048e0421..da9b6697bb 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -100,7 +100,7 @@ async function readPopulatedManagedStorage(): Promise< export async function watchStorageInitialization(): Promise { const values = await readPopulatedManagedStorage(); - if (values) { + if (values != null) { // Already initialized return; } From 8ce47de427a3c5aa8b833d6918d04ef533c1c156 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 18:14:58 -0500 Subject: [PATCH 13/16] Clarify names/comments --- src/background/installer.ts | 6 +++--- src/store/enterprise/managedStorage.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/background/installer.ts b/src/background/installer.ts index fc9f4e8b89..b9069ac0e5 100644 --- a/src/background/installer.ts +++ b/src/background/installer.ts @@ -47,7 +47,7 @@ let _availableVersion: string | null = null; * Returns true if this appears to be a Chrome Web Store install and/or the user has an app URL where they're * authenticated so the extension can be linked. */ -async function isEndUserInstall(): Promise { +async function isLikelyEndUserInstall(): Promise { // Query existing app/CWS tabs: https://developer.chrome.com/docs/extensions/reference/api/tabs#method-query // `browser.tabs.query` supports https://developer.chrome.com/docs/extensions/develop/concepts/match-patterns const likelyOnboardingTabs = await browser.tabs.query({ @@ -233,14 +233,14 @@ export async function handleInstall({ if (!(await isLinked())) { // If an end-user appears to be installing, jump to linking directly vs. waiting for readManagedStorage because // readManagedStorage will wait until a timeout for managed storage to be available. - if (!isManagedStorageInitialized() && (await isEndUserInstall())) { + if (!isManagedStorageInitialized() && (await isLikelyEndUserInstall())) { console.debug("Skipping readManagedStorage for end-user install"); await openInstallPage(); return; } - // Reminder: readManagedStorageByKey waits up to 4.5 seconds for managed storage to be available + // Reminder: readManagedStorage waits up to 4.5 seconds for managed storage to be available const { ssoUrl, partnerId, diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index da9b6697bb..003f81528a 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -29,7 +29,7 @@ import type { Nullishable } from "@/utils/nullishUtils"; // 2024-02-15: bumped to 4.5s because 2s was too short: https://github.com/pixiebrix/pixiebrix-extension/issues/7618 // Privacy Badger uses 4.5s timeout, but thinks policy should generally be available within 2.5s. In installer.ts, -// we check for app tabs to skip the linking wait if the user appears to be installing from the web store. +// skip waiting for managed storage before linking the Extension if the user appears to be installing from the CWS. const MAX_MANAGED_STORAGE_WAIT_MILLIS = 4500; /** From 8700c2270726ce36960a85692005a6dd7660aef5 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 18:19:30 -0500 Subject: [PATCH 14/16] Clarify watchDelayedStorageInitialization usage --- src/background/background.ts | 8 +++++--- src/store/enterprise/managedStorage.ts | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/background/background.ts b/src/background/background.ts index 990e051050..82e4da3456 100644 --- a/src/background/background.ts +++ b/src/background/background.ts @@ -50,12 +50,14 @@ import { initSidePanel } from "./sidePanel"; import initRestrictUnauthenticatedUrlAccess from "@/background/restrictUnauthenticatedUrlAccess"; import { initManagedStorage, - watchStorageInitialization, + watchDelayedStorageInitialization, } from "@/store/enterprise/managedStorage"; // Try to initialize managed storage as early as possible because it impacts background behavior -// Call watchStorageInitialization to handle case where storage is not immediately available within timeout -void initManagedStorage().then(async () => watchStorageInitialization()); +// Call watchDelayedStorageInitialization to handle case where storage is not immediately available within timeout. +// We might consider putting watchStorageInitialization in initManagedStorage, but having a non-terminating +// interval complicates testing. +void initManagedStorage().then(async () => watchDelayedStorageInitialization()); void initLocator(); void initMessengerLogging(); diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index 003f81528a..ee0cea3ed3 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -97,7 +97,7 @@ async function readPopulatedManagedStorage(): Promise< * * @see waitForInitialManagedStorage */ -export async function watchStorageInitialization(): Promise { +export async function watchDelayedStorageInitialization(): Promise { const values = await readPopulatedManagedStorage(); if (values != null) { From 3d700824f81299827821e9b5bd3d59eda6fdd160 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 18:22:39 -0500 Subject: [PATCH 15/16] Clarify interval usage --- src/store/enterprise/managedStorage.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index ee0cea3ed3..097bb32af7 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -105,6 +105,8 @@ export async function watchDelayedStorageInitialization(): Promise { return; } + // Use setInterval instead of pollUntilTruthy to clear on browser.storage.onChanged. pollUntilTruthy doesn't + // currently directly support an abort signal. initializationInterval = setInterval( async () => { const values = await readPopulatedManagedStorage(); From 61857c952024bb03792615c4579f70596185ad98 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 18:23:38 -0500 Subject: [PATCH 16/16] Use version instead of date --- src/store/enterprise/managedStorage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index 097bb32af7..d3e0c920bc 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -27,7 +27,7 @@ import pMemoize, { pMemoizeClear } from "p-memoize"; import { pollUntilTruthy } from "@/utils/promiseUtils"; import type { Nullishable } from "@/utils/nullishUtils"; -// 2024-02-15: bumped to 4.5s because 2s was too short: https://github.com/pixiebrix/pixiebrix-extension/issues/7618 +// 1.8.9: bumped to 4.5s because 2s was too short: https://github.com/pixiebrix/pixiebrix-extension/issues/7618 // Privacy Badger uses 4.5s timeout, but thinks policy should generally be available within 2.5s. In installer.ts, // skip waiting for managed storage before linking the Extension if the user appears to be installing from the CWS. const MAX_MANAGED_STORAGE_WAIT_MILLIS = 4500;