From ee728ccfa7e984709123922e5a29604cf166c48e Mon Sep 17 00:00:00 2001 From: Grace Kind Date: Sun, 16 Aug 2026 13:40:23 -0500 Subject: [PATCH] Move service selection back to device --- package.json | 2 +- src/js/app.js | 4 +- src/js/dataLayer/derived.js | 3 - src/js/dataLayer/mutations.js | 6 - src/js/preferences.js | 33 --- src/js/push/courierPushService.js | 97 +++++-- src/js/views/settings/advanced.view.js | 1 - tests/e2e/mockServer.js | 29 +-- .../views/settings/advanced.view.test.js | 13 +- tests/unit/specs/courierPushService.test.js | 244 +++++++++++++----- tests/unit/specs/preferences.test.js | 48 ---- 11 files changed, 270 insertions(+), 210 deletions(-) diff --git a/package.json b/package.json index d08545a7..55abb73d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "impro", - "version": "0.18.206", + "version": "0.18.207", "type": "module", "scripts": { "start": "rm -rf \"${BUILD_DIR:-build}\" && NODE_ENV=development eleventy --serve", diff --git a/src/js/app.js b/src/js/app.js index 7cccdaf7..82db30f0 100644 --- a/src/js/app.js +++ b/src/js/app.js @@ -133,9 +133,7 @@ export async function main() { router, ) : null; - const courierPushService = session - ? new CourierPushService(api, dataLayer) - : null; + const courierPushService = session ? new CourierPushService(api) : null; const postComposerService = session ? new PostComposerService(dataLayer, identityResolver, pluginService, { draftsEnabled: await checkDraftsEnabled(), diff --git a/src/js/dataLayer/derived.js b/src/js/dataLayer/derived.js index 95f6c58c..f23f5d95 100644 --- a/src/js/dataLayer/derived.js +++ b/src/js/dataLayer/derived.js @@ -224,9 +224,6 @@ export class Derived extends ReactiveStore { const patches = this.patchStore.$preferencePatches.get(); return this.patchStore.applyPreferencePatches(preferences, patches); }); - this.$notificationServiceDid = new Signal.Computed(() => { - return this.$preferences.get()?.getNotificationServiceDid() ?? null; - }); this.$notifications = new Signal.Computed(() => { const data = this.dataStore.$notifications.get(); if (!data) return null; diff --git a/src/js/dataLayer/mutations.js b/src/js/dataLayer/mutations.js index 1590a49c..0348fe71 100644 --- a/src/js/dataLayer/mutations.js +++ b/src/js/dataLayer/mutations.js @@ -593,12 +593,6 @@ export class Mutations { await this.preferencesProvider.updatePreferences(newPreferences); } - async setNotificationServiceDid(did) { - const preferences = this.preferencesProvider.requirePreferences(); - const newPreferences = preferences.setNotificationServiceDid(did); - await this.preferencesProvider.updatePreferences(newPreferences); - } - async subscribeLabeler(profile, labelerInfo) { const patchId = this.patchStore.addPreferencePatch({ type: "subscribeLabeler", diff --git a/src/js/preferences.js b/src/js/preferences.js index 16a23809..da358bb4 100644 --- a/src/js/preferences.js +++ b/src/js/preferences.js @@ -22,8 +22,6 @@ export const INSTALLED_PLUGINS_PREF_TYPE = "app.bsky.actor.defs#improInstalledPluginsPref"; export const SEARCH_HISTORY_PREF_TYPE = "app.bsky.actor.defs#improSearchHistoryPref"; -export const PUSH_NOTIFICATION_SERVICE_PREF_TYPE = - "app.bsky.actor.defs#improPushNotificationServicePref"; function getContentTextFromEmbed(embed) { const texts = []; @@ -674,37 +672,6 @@ export class Preferences { return clone; } - getNotificationServiceDid() { - const pref = Preferences.getPreferenceByType( - this.obj, - PUSH_NOTIFICATION_SERVICE_PREF_TYPE, - ); - return pref?.serviceDid ?? null; - } - - setNotificationServiceDid(did) { - const clone = this.clone(); - if (did === null) { - clone.obj = clone.obj.filter( - (pref) => pref.$type !== PUSH_NOTIFICATION_SERVICE_PREF_TYPE, - ); - return clone; - } - const existing = Preferences.getPreferenceByType( - clone.obj, - PUSH_NOTIFICATION_SERVICE_PREF_TYPE, - ); - if (existing) { - existing.serviceDid = did; - } else { - clone.obj.push({ - $type: PUSH_NOTIFICATION_SERVICE_PREF_TYPE, - serviceDid: did, - }); - } - return clone; - } - getFollowingFeedPreference() { const followingFeedPreference = this.obj.find( (preference) => diff --git a/src/js/push/courierPushService.js b/src/js/push/courierPushService.js index 03ff6b1a..75ba7616 100644 --- a/src/js/push/courierPushService.js +++ b/src/js/push/courierPushService.js @@ -3,9 +3,9 @@ import { Signal } from "/js/signals.js"; import { isTouchOnlyDevice, isStandalonePWA, isIOS } from "/js/utils.js"; import { auth } from "/js/auth.js"; import { Api } from "/js/api.js"; -import { Preferences } from "/js/preferences.js"; const STORAGE_KEY = "courier-push-enabled"; +const SERVICE_STORAGE_KEY = "courier-push-service"; const APP_ID = "social.impro"; const PLATFORM = "web"; const SW_PATH = "/sw.js"; @@ -33,15 +33,28 @@ function urlBase64ToUint8Array(base64String) { return bytes; } +function matchesVapidKey(subscription, vapidPublicKey) { + const existing = subscription.options?.applicationServerKey; + if (!existing) return true; + const bytes = new Uint8Array(existing); + const expected = urlBase64ToUint8Array(vapidPublicKey); + return ( + bytes.length === expected.length && + bytes.every((byte, index) => byte === expected[index]) + ); +} + // Client-side half of the spec's "Enable flow". export class CourierPushService { - constructor(api, dataLayer) { + constructor(api) { this.api = api; - this.dataLayer = dataLayer; this._configPromise = null; this.$enabled = new Signal.State( localStorage.getItem(STORAGE_KEY) === "true", ); + this.$deviceServiceDid = new Signal.State( + localStorage.getItem(SERVICE_STORAGE_KEY), + ); } get isSupported() { @@ -62,14 +75,25 @@ export class CourierPushService { return this.isSupported && this.hasService && this.$enabled.get(); } + // A browser holds one push subscription, bound to one service's VAPID key, + // shared by every account signed in here — so the choice is per-device. get serviceDid() { - return this.dataLayer?.derived.$notificationServiceDid.get() ?? null; + return this.$deviceServiceDid.get(); } get hasService() { return this.serviceDid !== null; } + _setDeviceServiceDid(did) { + if (did === null) { + localStorage.removeItem(SERVICE_STORAGE_KEY); + } else { + localStorage.setItem(SERVICE_STORAGE_KEY, did); + } + this.$deviceServiceDid.set(did); + } + _setEnabled(enabled) { if (enabled) { localStorage.setItem(STORAGE_KEY, "true"); @@ -90,7 +114,7 @@ export class CourierPushService { await this.disable(); } this._forgetConfig(); - await this.dataLayer.mutations.setNotificationServiceDid(did); + this._setDeviceServiceDid(did); } async clearService() { @@ -99,7 +123,7 @@ export class CourierPushService { await this.disable(); } this._forgetConfig(); - await this.dataLayer.mutations.setNotificationServiceDid(null); + this._setDeviceServiceDid(null); } async fetchServiceConfig() { @@ -165,6 +189,10 @@ export class CourierPushService { const registration = await navigator.serviceWorker.register(SW_PATH); await navigator.serviceWorker.ready; let subscription = await registration.pushManager.getSubscription(); + if (subscription && !matchesVapidKey(subscription, config.vapidPublicKey)) { + await subscription.unsubscribe(); + subscription = null; + } if (!subscription) { subscription = await registration.pushManager.subscribe({ userVisibleOnly: true, @@ -202,20 +230,48 @@ export class CourierPushService { async unregisterAccount(did) { const subscription = await this._getSubscription(); - if (!subscription) return; + if (!subscription || !this.serviceDid) return; const api = await this._apiForAccount(did); if (!api) return; - const preferences = new Preferences(await api.getPreferences(), null, { - persist: false, - }); - const serviceDid = preferences.getNotificationServiceDid(); - if (!serviceDid) return; - await api.unregisterPush({ - serviceDid, + await api.unregisterPush(this._unregisterPayload(subscription)); + } + + _unregisterPayload(subscription) { + return { + serviceDid: this.serviceDid, token: JSON.stringify(subscription), platform: PLATFORM, appId: APP_ID, - }); + }; + } + + async _listAccountDids() { + try { + return (await auth.listAccounts()).map((account) => account.did); + } catch (error) { + console.warn("Failed to list accounts for push teardown", error); + return []; + } + } + + async _unregisterDevice(subscription) { + if (!this.serviceDid) return; + const payload = this._unregisterPayload(subscription); + const currentDid = this.api.session?.did ?? null; + try { + await this.api.unregisterPush(payload); + } catch (error) { + console.error("Failed to unregister push subscription", error); + } + for (const did of await this._listAccountDids()) { + if (did === currentDid) continue; + try { + const api = await this._apiForAccount(did); + await api?.unregisterPush(payload); + } catch (error) { + console.error("Failed to unregister push for another account", error); + } + } } async _getSubscription() { @@ -228,16 +284,7 @@ export class CourierPushService { this._setEnabled(false); const subscription = await this._getSubscription(); if (!subscription) return; - try { - await this.api.unregisterPush({ - serviceDid: this.serviceDid, - token: JSON.stringify(subscription), - platform: PLATFORM, - appId: APP_ID, - }); - } catch (error) { - console.error("Failed to unregister push subscription", error); - } + await this._unregisterDevice(subscription); await subscription.unsubscribe(); } } diff --git a/src/js/views/settings/advanced.view.js b/src/js/views/settings/advanced.view.js index 8f7ce530..a333bb49 100644 --- a/src/js/views/settings/advanced.view.js +++ b/src/js/views/settings/advanced.view.js @@ -161,7 +161,6 @@ export default async function settingsAdvancedView({ state.$notificationServiceLoading.set(true); try { const { name } = await courierPushService.previewService(did); - const wasEnabled = courierPushService.isEnabled; await courierPushService.selectService(did); showToast(`Selected notification service: ${name}`, { style: "success" }); } catch (error) { diff --git a/tests/e2e/mockServer.js b/tests/e2e/mockServer.js index 47688d98..5fdc8082 100644 --- a/tests/e2e/mockServer.js +++ b/tests/e2e/mockServer.js @@ -102,8 +102,7 @@ export class MockServer { this.pluginReadme = "# Remote Themes\n\nA test readme for the plugin."; this.tokenRefreshShouldFail = false; this.notificationServiceUnreachable = false; - // Which notification service the account's preferences name; null means - // none has been chosen. + // Which notification service this device has chosen; null means none. this.notificationServiceDid = null; this.registerPushCalls = []; this.unregisterPushCalls = []; @@ -121,8 +120,8 @@ export class MockServer { this.notificationServiceUnreachable = true; } - // Start with a notification service already chosen, as if the account had - // picked one on another device. + // Start with a notification service already chosen on this device, as if a + // previous launch had left it there. Call before setup(). setNotificationServiceDid(did) { this.notificationServiceDid = did; } @@ -542,6 +541,12 @@ export class MockServer { }); } + if (this.notificationServiceDid) { + await page.addInitScript((did) => { + localStorage.setItem("courier-push-service", did); + }, this.notificationServiceDid); + } + // Stub the external destinations "open in bsky.app" / "translate" links // point at. These open via window.open, and popups are separate pages that // page-level routes don't apply to — so the route has to be on the context @@ -858,15 +863,6 @@ export class MockServer { }, ] : []), - ...(this.notificationServiceDid - ? [ - { - $type: - "app.bsky.actor.defs#improPushNotificationServicePref", - serviceDid: this.notificationServiceDid, - }, - ] - : []), ...(this.labelerSubscriptions.length > 0 ? [ { @@ -2533,13 +2529,6 @@ export class MockServer { if (hiddenPostsPref) { this.hiddenPostUris = hiddenPostsPref.items || []; } - // Absent means the user chose None, so this tracks removal too. - this.notificationServiceDid = - body?.preferences?.find( - (p) => - p.$type === - "app.bsky.actor.defs#improPushNotificationServicePref", - )?.serviceDid ?? null; const searchHistoryPref = body?.preferences?.find( (p) => p.$type === "app.bsky.actor.defs#improSearchHistoryPref", ); diff --git a/tests/e2e/specs/views/settings/advanced.view.test.js b/tests/e2e/specs/views/settings/advanced.view.test.js index 557c1738..972a3b6a 100644 --- a/tests/e2e/specs/views/settings/advanced.view.test.js +++ b/tests/e2e/specs/views/settings/advanced.view.test.js @@ -4,6 +4,11 @@ import { login } from "../../../helpers.js"; import { MockServer } from "../../../mockServer.js"; import { notificationService } from "../../../testData.js"; +// The chosen service is device state, so it lands in localStorage rather than +// in the account's preferences. +const storedService = (page) => + page.evaluate(() => localStorage.getItem("courier-push-service")); + test.describe("Settings Advanced view", () => { test("should display header and App View section", async ({ page }) => { const mockServer = new MockServer(); @@ -382,7 +387,7 @@ test.describe("Settings Advanced view", () => { await expect(page.locator('[data-testid="toast"]')).toContainText( notificationService.name, ); - expect(mockServer.notificationServiceDid).toBe(notificationService.did); + expect(await storedService(page)).toBe(notificationService.did); }); test("a stored service that matches no preset selects Custom and prefills it", async ({ @@ -432,7 +437,7 @@ test.describe("Settings Advanced view", () => { await select.selectOption("none"); await page.locator('[data-testid="notification-service-save"]').click(); - await expect.poll(() => mockServer.notificationServiceDid).toBeNull(); + await expect.poll(() => storedService(page)).toBeNull(); }); test("rejects input that isn't a DID without hitting the network", async ({ @@ -454,7 +459,7 @@ test.describe("Settings Advanced view", () => { await expect( page.locator('[data-testid="notification-service-error"]'), ).toBeVisible(); - expect(mockServer.notificationServiceDid).toBeNull(); + expect(await storedService(page)).toBeNull(); }); test("a service that won't resolve is rejected and not stored", async ({ @@ -478,7 +483,7 @@ test.describe("Settings Advanced view", () => { page.locator('[data-testid="notification-service-error"]'), ).toBeVisible({ timeout: 10000 }); // The previous state must survive a failed switch, so nothing is stored. - expect(mockServer.notificationServiceDid).toBeNull(); + expect(await storedService(page)).toBeNull(); }); }); diff --git a/tests/unit/specs/courierPushService.test.js b/tests/unit/specs/courierPushService.test.js index 936fea88..8af8aa54 100644 --- a/tests/unit/specs/courierPushService.test.js +++ b/tests/unit/specs/courierPushService.test.js @@ -9,6 +9,17 @@ const SUBSCRIPTION = { unsubscribe: async () => {}, }; +const FRESH_SUBSCRIPTION = { + endpoint: "https://push.example/ep/fresh", + keys: { p256dh: "p2", auth: "a2" }, + unsubscribe: async () => {}, +}; + +// "BKxQ" is the base64url the service config carries; decoded it is the key a +// subscription reports back through options.applicationServerKey. +const VAPID_KEY = "BKxQ"; +const VAPID_KEY_BYTES = Uint8Array.from([4, 172, 80]); + function setupDom({ enabled = true, granted = true } = {}) { globalThis.localStorage = { _data: enabled ? { "courier-push-enabled": "true" } : {}, @@ -50,6 +61,7 @@ function setupDom({ enabled = true, granted = true } = {}) { const registration = { pushManager: { getSubscription: async () => SUBSCRIPTION, + subscribe: async () => FRESH_SUBSCRIPTION, }, }; Object.defineProperty(globalThis, "navigator", { @@ -65,29 +77,19 @@ function setupDom({ enabled = true, granted = true } = {}) { }); } -// The chosen service lives in account preferences, so the service reads it -// through the dataLayer. Share one of these between two CourierPushService -// instances to model the same account on a later launch. -function makeDataLayer(serviceDid = null) { - const $notificationServiceDid = new Signal.State(serviceDid); - return { - derived: { $notificationServiceDid }, - mutations: { - setNotificationServiceDid: mock.fn(async (did) => { - $notificationServiceDid.set(did); - }), - }, - }; -} - -function createService(dataLayer = makeDataLayer()) { +// The chosen service is device state, so a service DID is seeded the same way +// a previous launch would have left it: in localStorage. +function createService(serviceDid = null) { + if (serviceDid !== null) { + globalThis.localStorage.setItem("courier-push-service", serviceDid); + } const registerPush = mock.fn(async () => {}); - const api = { registerPush }; - return { - service: new CourierPushService(api, dataLayer), - registerPush, - dataLayer, - }; + const api = { registerPush, session: { did: "did:plc:current" } }; + const service = new CourierPushService(api); + // Enumerating accounts reaches for real OAuth storage; tests that care about + // the other-account fan-out override this. + service._listAccountDids = async () => []; + return { service, registerPush }; } describe("CourierPushService registration", () => { @@ -135,6 +137,53 @@ describe("CourierPushService registration", () => { }); }); + // A PushSubscription is bound to the VAPID key it was created with, so one + // left behind by a different service can't receive this service's pushes — + // the gateway rejects them, silently, forever. + it("replaces a subscription bound to another service's key", async () => { + setupDom(); + const { service, registerPush } = createService("did:web:notifs.example"); + const unsubscribe = mock.fn(async () => {}); + SUBSCRIPTION.options = { + applicationServerKey: Uint8Array.from([9, 9, 9]).buffer, + }; + SUBSCRIPTION.unsubscribe = unsubscribe; + + try { + await service._subscribeAndRegister({ vapidPublicKey: VAPID_KEY }); + } finally { + delete SUBSCRIPTION.options; + SUBSCRIPTION.unsubscribe = async () => {}; + } + + assert.equal(unsubscribe.mock.calls.length, 1); + assert.equal( + JSON.parse(registerPush.mock.calls[0].arguments[0].token).endpoint, + FRESH_SUBSCRIPTION.endpoint, + ); + }); + + it("keeps a subscription bound to this service's key", async () => { + setupDom(); + const { service, registerPush } = createService("did:web:notifs.example"); + const unsubscribe = mock.fn(async () => {}); + SUBSCRIPTION.options = { applicationServerKey: VAPID_KEY_BYTES.buffer }; + SUBSCRIPTION.unsubscribe = unsubscribe; + + try { + await service._subscribeAndRegister({ vapidPublicKey: VAPID_KEY }); + } finally { + delete SUBSCRIPTION.options; + SUBSCRIPTION.unsubscribe = async () => {}; + } + + assert.equal(unsubscribe.mock.calls.length, 0); + assert.equal( + JSON.parse(registerPush.mock.calls[0].arguments[0].token).endpoint, + SUBSCRIPTION.endpoint, + ); + }); + it("skips the launch re-assert when no service is selected", async () => { setupDom(); const { service, registerPush } = createService(); @@ -144,9 +193,7 @@ describe("CourierPushService registration", () => { it("skips the launch re-assert when push is disabled", async () => { setupDom({ enabled: false }); - const { service, registerPush } = createService( - makeDataLayer("did:web:notifs.example"), - ); + const { service, registerPush } = createService("did:web:notifs.example"); await service.reassertIfEnabled(); assert.equal(registerPush.mock.calls.length, 0); }); @@ -155,9 +202,7 @@ describe("CourierPushService registration", () => { // stored flag is only reconciled the next time we go looking. it("clears the stored flag when permission was revoked out-of-band", async () => { setupDom({ granted: false }); - const { service, registerPush } = createService( - makeDataLayer("did:web:notifs.example"), - ); + const { service, registerPush } = createService("did:web:notifs.example"); await service.reassertIfEnabled(); assert.equal(registerPush.mock.calls.length, 0); assert.equal(service.$enabled.get(), false); @@ -165,7 +210,7 @@ describe("CourierPushService registration", () => { it("disable() unregisters this device and clears the flag", async () => { setupDom(); - const { service } = createService(); + const { service } = createService("did:web:notifs.example"); const unregisterPush = mock.fn(async () => {}); service.api.unregisterPush = unregisterPush; await service.disable(); @@ -205,7 +250,7 @@ describe("CourierPushService registration", () => { it("is supported in an uninstalled non-iOS mobile browser", () => { setupDom(); simulateUninstalled({ ios: false }); - const { service } = createService(makeDataLayer("did:web:notifs.example")); + const { service } = createService("did:web:notifs.example"); assert.equal(service.isSupported, true); assert.equal(service.requiresInstall, false); }); @@ -228,7 +273,7 @@ describe("CourierPushService registration", () => { it("registers against the selected service, not the default", async () => { setupDom(); const { service, registerPush } = createService( - makeDataLayer("did:web:elsewhere.example"), + "did:web:elsewhere.example", ); await service._subscribeAndRegister({ vapidPublicKey: "k" }); assert.equal( @@ -238,30 +283,17 @@ describe("CourierPushService registration", () => { }); // Removing an account happens from a different account's session, so the - // teardown borrows the removed account's own session and preferences. - function stubRemovedAccount(service, { preferences }) { + // teardown borrows the removed account's own session. + function stubRemovedAccount(service) { const unregisterPush = mock.fn(async () => {}); - const getPreferences = mock.fn(async () => preferences); - service._apiForAccount = mock.fn(async () => ({ - unregisterPush, - getPreferences, - })); - return { unregisterPush, getPreferences }; + service._apiForAccount = mock.fn(async () => ({ unregisterPush })); + return { unregisterPush }; } - const servicePref = (serviceDid) => [ - { - $type: "app.bsky.actor.defs#improPushNotificationServicePref", - serviceDid, - }, - ]; - - it("unregisters a removed account at the service it chose", async () => { + it("unregisters a removed account at this device's service", async () => { setupDom(); - const { service } = createService(makeDataLayer("did:web:mine.example")); - const { unregisterPush } = stubRemovedAccount(service, { - preferences: servicePref("did:web:theirs.example"), - }); + const { service } = createService("did:web:notifs.example"); + const { unregisterPush } = stubRemovedAccount(service); await service.unregisterAccount("did:plc:removed"); @@ -271,8 +303,7 @@ describe("CourierPushService registration", () => { ); assert.equal(unregisterPush.mock.calls.length, 1); const args = unregisterPush.mock.calls[0].arguments[0]; - // The removed account's service, not the current account's. - assert.equal(args.serviceDid, "did:web:theirs.example"); + assert.equal(args.serviceDid, "did:web:notifs.example"); assert.equal(args.appId, "social.impro"); assert.equal(args.platform, "web"); assert.deepEqual(JSON.parse(args.token), { @@ -281,10 +312,10 @@ describe("CourierPushService registration", () => { }); }); - it("skips a removed account that never chose a service", async () => { + it("skips the teardown on a device with no service", async () => { setupDom(); const { service } = createService(); - const { unregisterPush } = stubRemovedAccount(service, { preferences: [] }); + const { unregisterPush } = stubRemovedAccount(service); await service.unregisterAccount("did:plc:removed"); @@ -295,13 +326,11 @@ describe("CourierPushService registration", () => { // would silently kill push for the accounts that remain. it("leaves the shared browser subscription in place", async () => { setupDom(); - const { service } = createService(); + const { service } = createService("did:web:notifs.example"); const unsubscribe = mock.fn(async () => {}); const originalUnsubscribe = SUBSCRIPTION.unsubscribe; SUBSCRIPTION.unsubscribe = unsubscribe; - stubRemovedAccount(service, { - preferences: servicePref("did:web:theirs.example"), - }); + stubRemovedAccount(service); try { await service.unregisterAccount("did:plc:removed"); @@ -317,7 +346,7 @@ describe("CourierPushService registration", () => { // best-effort, so the error has to surface rather than be swallowed. it("surfaces a failed teardown to the caller", async () => { setupDom(); - const { service } = createService(); + const { service } = createService("did:web:notifs.example"); service._apiForAccount = async () => { throw new Error("session already gone"); }; @@ -326,6 +355,73 @@ describe("CourierPushService registration", () => { /session already gone/, ); }); + + // Tearing down the subscription strands every account still registered + // against it, so disable() has to cover all of them. + it("unregisters every signed-in account before unsubscribing", async () => { + setupDom(); + const { service } = createService("did:web:notifs.example"); + const order = []; + const unregisterPush = mock.fn(async () => {}); + service.api.unregisterPush = mock.fn(async () => order.push("current")); + service._listAccountDids = async () => [ + "did:plc:current", + "did:plc:other", + "did:plc:third", + ]; + service._apiForAccount = mock.fn(async (did) => ({ + unregisterPush: async (payload) => { + order.push(did); + return unregisterPush(payload); + }, + })); + const originalUnsubscribe = SUBSCRIPTION.unsubscribe; + SUBSCRIPTION.unsubscribe = mock.fn(async () => order.push("unsubscribe")); + + try { + await service.disable(); + } finally { + SUBSCRIPTION.unsubscribe = originalUnsubscribe; + } + + // The current account goes through the app's own api, and is not + // unregistered twice. + assert.deepEqual(order, [ + "current", + "did:plc:other", + "did:plc:third", + "unsubscribe", + ]); + assert.equal( + unregisterPush.mock.calls[0].arguments[0].serviceDid, + "did:web:notifs.example", + ); + }); + + it("still unsubscribes when another account's teardown fails", async () => { + setupDom(); + const { service } = createService("did:web:notifs.example"); + service.api.unregisterPush = mock.fn(async () => {}); + service._listAccountDids = async () => ["did:plc:other"]; + service._apiForAccount = async () => { + throw new Error("session already gone"); + }; + const unsubscribe = mock.fn(async () => {}); + const originalUnsubscribe = SUBSCRIPTION.unsubscribe; + SUBSCRIPTION.unsubscribe = unsubscribe; + const originalError = console.error; + console.error = () => {}; + + try { + await service.disable(); + } finally { + SUBSCRIPTION.unsubscribe = originalUnsubscribe; + console.error = originalError; + } + + assert.equal(unsubscribe.mock.calls.length, 1); + assert.equal(service.$enabled.get(), false); + }); }); describe("CourierPushService service selection", () => { @@ -399,17 +495,33 @@ describe("CourierPushService service selection", () => { assert.equal(fetchCalls.length, 0, "it must not hit the network"); }); - it("naming a service persists it across launches", async () => { - const { service, dataLayer } = createService(); + // The choice is device state: it belongs to the browser, not the account, + // because one subscription serves every account signed in here. + it("naming a service persists it on the device across launches", async () => { + const { service } = createService(); await service.selectService("did:web:notifs.example"); assert.equal(service.serviceDid, "did:web:notifs.example"); assert.equal(service.hasService, true); + assert.equal( + globalThis.localStorage.getItem("courier-push-service"), + "did:web:notifs.example", + ); - // Same account, later launch: the choice comes back from preferences. - const { service: relaunched } = createService(dataLayer); + // Same device, later launch. + const { service: relaunched } = createService(); assert.equal(relaunched.serviceDid, "did:web:notifs.example"); }); + it("clearing the service clears the device's choice", async () => { + const { service } = createService("did:web:notifs.example"); + + await service.clearService(); + + assert.equal(service.serviceDid, null); + assert.equal(service.hasService, false); + assert.equal(globalThis.localStorage.getItem("courier-push-service"), null); + }); + it("resolves a service through its DID document", async () => { const { service } = createService(); const preview = await service.previewService("did:web:notifs.example"); @@ -418,7 +530,7 @@ describe("CourierPushService service selection", () => { }); it("resolves a service once per session", async () => { - const { service, dataLayer } = createService(); + const { service } = createService(); await service.selectService("did:web:notifs.example"); await service.fetchServiceConfig(); const afterFirst = fetchCalls.length; @@ -432,7 +544,7 @@ describe("CourierPushService service selection", () => { // A fresh instance is the app-launch case. It re-resolves rather than // reading a persisted copy: how long the config stays good is the // service's call, made in its own cache headers. - const { service: relaunched } = createService(dataLayer); + const { service: relaunched } = createService(); await relaunched.fetchServiceConfig(); assert.ok(fetchCalls.length > afterFirst); }); diff --git a/tests/unit/specs/preferences.test.js b/tests/unit/specs/preferences.test.js index e11e44f6..ddfb2517 100644 --- a/tests/unit/specs/preferences.test.js +++ b/tests/unit/specs/preferences.test.js @@ -5,7 +5,6 @@ import { PLUGIN_SETTINGS_PREF_TYPE, INSTALLED_PLUGINS_PREF_TYPE, SEARCH_HISTORY_PREF_TYPE, - PUSH_NOTIFICATION_SERVICE_PREF_TYPE, } from "/js/preferences.js"; describe("Preferences.createLoggedOutPreferences", () => { @@ -3315,50 +3314,3 @@ describe("Preferences recent search profiles", () => { assert.deepEqual(preferences.getRecentSearchProfiles(), []); }); }); - -describe("Preferences push notification service", () => { - it("is null when the account has never chosen one", () => { - assert.equal(new Preferences([], []).getNotificationServiceDid(), null); - }); - - it("round-trips the chosen service", () => { - const preferences = new Preferences([], []).setNotificationServiceDid( - "did:web:notifs.example", - ); - assert.equal( - preferences.getNotificationServiceDid(), - "did:web:notifs.example", - ); - }); - - it("replaces rather than appends when the service changes", () => { - const preferences = new Preferences([], []) - .setNotificationServiceDid("did:web:notifs.example") - .setNotificationServiceDid("did:web:elsewhere.example"); - const records = preferences.obj.filter( - (pref) => pref.$type === PUSH_NOTIFICATION_SERVICE_PREF_TYPE, - ); - assert.equal(records.length, 1); - assert.equal(records[0].serviceDid, "did:web:elsewhere.example"); - }); - - it("drops the record entirely when set back to none", () => { - const preferences = new Preferences([], []) - .setNotificationServiceDid("did:web:notifs.example") - .setNotificationServiceDid(null); - assert.deepEqual( - preferences.obj.filter( - (pref) => pref.$type === PUSH_NOTIFICATION_SERVICE_PREF_TYPE, - ), - [], - ); - assert.equal(preferences.getNotificationServiceDid(), null); - }); - - it("leaves other preferences untouched", () => { - const preferences = new Preferences([], []) - .addRecentSearch("cats") - .setNotificationServiceDid("did:web:notifs.example"); - assert.equal(preferences.getRecentSearches().length, 1); - }); -}); -- 2.51.2