From 963c1fd3c4c3c64333770e2519de2acffdf93041 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 15 Feb 2024 18:30:31 -0500 Subject: [PATCH] #7618: increase managed storage delay and link extension immediately for CWS installs (#7628) --- src/background/background.ts | 10 + src/background/deploymentUpdater.test.ts | 2 +- src/background/deploymentUpdater.ts | 2 +- src/background/installer.ts | 61 +++++- src/store/enterprise/managedStorage.ts | 201 +++++++++++++----- .../enterprise/useManagedStorageState.ts | 7 +- src/utils/promiseUtils.ts | 6 + 7 files changed, 221 insertions(+), 68 deletions(-) diff --git a/src/background/background.ts b/src/background/background.ts index 384c4ba173..82e4da3456 100644 --- a/src/background/background.ts +++ b/src/background/background.ts @@ -48,6 +48,16 @@ import { initRuntimeLogging } from "@/development/runtimeLogging"; import initWalkthroughModalTrigger from "@/background/walkthroughModalTrigger"; import { initSidePanel } from "./sidePanel"; import initRestrictUnauthenticatedUrlAccess from "@/background/restrictUnauthenticatedUrlAccess"; +import { + initManagedStorage, + watchDelayedStorageInitialization, +} from "@/store/enterprise/managedStorage"; + +// Try to initialize managed storage as early as possible because it impacts background behavior +// 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/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); 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 fdfe1b58b6..b9069ac0e5 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"; @@ -40,6 +43,35 @@ 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 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({ + // Can't use SERVICE_URL directly because it contains a port number during development, resulting in an + // invalid URL match pattern + url: [ + // Setup page is page before sending user to the CWS + new URL("setup", DEFAULT_SERVICE_URL).href, + // 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/*", + ], + }); + + // The CWS install URL differs based on the extension listing slug. So instead, only match on the runtime id. + return likelyOnboardingTabs.some( + (tab) => + tab.url.includes(DEFAULT_SERVICE_URL) || + tab.url.includes(browser.runtime.id), + ); +} + /** * Install handler to complete authentication configuration for the extension. */ @@ -127,7 +159,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). @@ -199,18 +231,31 @@ 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 - const { ssoUrl, partnerId, controlRoomUrl, disableLoginTab } = - await readManagedStorage(); + // 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 isLikelyEndUserInstall())) { + console.debug("Skipping readManagedStorage for end-user install"); + + await openInstallPage(); + return; + } + + // Reminder: readManagedStorage waits up to 4.5 seconds for managed storage to be available + 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; } diff --git a/src/store/enterprise/managedStorage.ts b/src/store/enterprise/managedStorage.ts index a009737c5f..d3e0c920bc 100644 --- a/src/store/enterprise/managedStorage.ts +++ b/src/store/enterprise/managedStorage.ts @@ -15,29 +15,54 @@ * 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; +// 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; + +/** + * 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 +74,60 @@ async function readManagedStorageImmediately(): Promise { } } -// It's possible that managed storage is not available on the initial install event +/** + * 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. + * + * Required because other modules are using the values in managedStorageSnapshot vs. calling browser.storage.managed.get + * directly. + * + * @see waitForInitialManagedStorage + */ +export async function watchDelayedStorageInitialization(): Promise { + const values = await readPopulatedManagedStorage(); + + if (values != null) { + // Already initialized + 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(); + if (values != null) { + managedStorageSnapshot = values; + + if (initializationInterval) { + 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 installation 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 +135,46 @@ 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; - } - }, - { - maxWaitMillis: MAX_MANAGED_STORAGE_WAIT_MILLIS, - }, - ); + // Returns undefined if the promise times out + managedStorageSnapshot = await pollUntilTruthy< + Nullishable + >(readPopulatedManagedStorage, { + maxWaitMillis: MAX_MANAGED_STORAGE_WAIT_MILLIS, + }); - if (managedStorageState) { - notifyAll(managedStorageState); + console.info("Found managed storage settings", { + managedStorageState: managedStorageSnapshot, + }); - console.info("Read managed storage settings", { - managedStorageState, - }); - } else { - console.info("No manual storage settings found", { - managedStorageState, - }); - } + // After timeout, assume there's no policy set, so assign an empty value + managedStorageSnapshot ??= {}; + + notifyAllChangeListeners(managedStorageSnapshot); - return managedStorageState; + 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(() => { +export const initManagedStorage = once(async () => { 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) { @@ -108,14 +185,13 @@ export const initManagedStorage = once(() => { ); } - void waitForInitialManagedStorage(); + await waitForInitialManagedStorage(); }); /** * 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 +201,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()) ?? {}; + void initManagedStorage(); + const storage = await waitForInitialManagedStorage(); // eslint-disable-next-line security/detect-object-injection -- type-checked key return storage[key]; } @@ -139,20 +216,28 @@ 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()) ?? {}; + void initManagedStorage(); + return waitForInitialManagedStorage(); +} + +/** + * Returns true if managed storage has been initialized. + * @see waitForInitialManagedStorage + */ +export function isInitialized(): boolean { + return managedStorageSnapshot != null; } /** @@ -160,15 +245,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 +262,10 @@ export function subscribe( ): () => void { expectContext("extension"); - listeners.add(callback); + changeListeners.add(callback); return () => { - listeners.delete(callback); + changeListeners.delete(callback); }; } @@ -188,7 +273,13 @@ export function subscribe( * Helper method for resetting the module for testing. */ export function INTERNAL_reset(): void { - managedStorageState = undefined; - listeners.clear(); + managedStorageSnapshot = undefined; + changeListeners.clear(); + + 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..7e640cd093 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; }; @@ -34,7 +35,7 @@ type HookState = { */ function useManagedStorageState(): HookState { useEffect(() => { - initManagedStorage(); + void initManagedStorage(); }, []); const data = useSyncExternalStore(subscribe, getSnapshot); 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 },