diff --git a/package.json b/package.json index 8424fb72..6672a7fd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "impro", - "version": "0.20.10", + "version": "0.20.11", "type": "module", "scripts": { "start": "rm -rf \"${BUILD_DIR:-build}\" && NODE_ENV=development eleventy --serve", diff --git a/src/css/style.css b/src/css/style.css index d456a9b2..ff4279ef 100644 --- a/src/css/style.css +++ b/src/css/style.css @@ -9252,6 +9252,10 @@ moderation-warning .toggle-content { height: 18px; } +.warning-area .button-group { + margin-top: 12px; +} + @media (min-width: 800px) { #login-form .form-title { display: none; diff --git a/src/js/notificationServiceManager.js b/src/js/notificationServiceManager.js index 66c21760..848a901c 100644 --- a/src/js/notificationServiceManager.js +++ b/src/js/notificationServiceManager.js @@ -4,6 +4,8 @@ import { DesktopNotificationService } from "/js/desktopNotificationService.js"; import { AppBadgeService } from "/js/appBadgeService.js"; import { startActiveTabMonitor } from "/js/activeTabMonitor.js"; import { PushNotificationService } from "/js/push/pushNotificationService.js"; +import { showToast } from "/js/toasts.js"; +import { html } from "/js/lib/lit-html.js"; export class NotificationServiceManager { constructor({ session, api, auth, router }) { @@ -43,9 +45,23 @@ export class NotificationServiceManager { this.appBadgeService?.start(), ].filter(Boolean); - this.pushNotificationService?.reassertIfEnabled().catch((error) => { - console.error("Failed to re-assert push registration", error); - }); + this.pushNotificationService + ?.reassertIfEnabled() + .then(({ newlyRevoked }) => { + if (!newlyRevoked) return; + showToast( + html``, + { style: "warning", timeout: 8000 }, + ); + }) + .catch((error) => { + console.error("Failed to re-assert push registration", error); + }); return () => { for (const cleanup of cleanups) { diff --git a/src/js/push/pushNotificationService.js b/src/js/push/pushNotificationService.js index 5024feef..23503505 100644 --- a/src/js/push/pushNotificationService.js +++ b/src/js/push/pushNotificationService.js @@ -1,10 +1,11 @@ import { resolveDid, getServiceEndpointFromDidDoc } from "/js/atproto.js"; import { Signal } from "/js/signals.js"; import { isTouchOnlyDevice, isStandalonePWA, isIOS } from "/js/utils.js"; -import { Api } from "/js/api.js"; +import { Api, ApiError } from "/js/api.js"; const STORAGE_KEY = "push-notifications-enabled"; const SERVICE_STORAGE_KEY = "push-notification-service"; +const NEEDS_REAUTH_STORAGE_KEY = "push-notifications-needs-reauth"; const APP_ID = "social.impro"; const PLATFORM = "web"; const SW_PATH = "/sw.js"; @@ -57,6 +58,9 @@ export class PushNotificationService { this.$deviceServiceDid = new Signal.State( localStorage.getItem(SERVICE_STORAGE_KEY), ); + this.$needsReauth = new Signal.State( + localStorage.getItem(NEEDS_REAUTH_STORAGE_KEY) === "true", + ); } get isSupported() { @@ -87,6 +91,10 @@ export class PushNotificationService { return this.serviceDid !== null; } + get needsReauth() { + return this.$needsReauth.get(); + } + _setDeviceServiceDid(did) { if (did === null) { localStorage.removeItem(SERVICE_STORAGE_KEY); @@ -105,6 +113,15 @@ export class PushNotificationService { this.$enabled.set(enabled); } + _setNeedsReauth(needsReauth) { + if (needsReauth) { + localStorage.setItem(NEEDS_REAUTH_STORAGE_KEY, "true"); + } else { + localStorage.removeItem(NEEDS_REAUTH_STORAGE_KEY); + } + this.$needsReauth.set(needsReauth); + } + async previewService(did) { const config = await this._loadServiceConfig(did); return { did, name: config.name ?? did, authUrl: config.authUrl ?? null }; @@ -116,6 +133,7 @@ export class PushNotificationService { await this.disable(); } this._forgetConfig(); + this._setNeedsReauth(false); this._setDeviceServiceDid(did); } @@ -125,6 +143,7 @@ export class PushNotificationService { await this.disable(); } this._forgetConfig(); + this._setNeedsReauth(false); this._setDeviceServiceDid(null); } @@ -208,21 +227,33 @@ export class PushNotificationService { appId: APP_ID, }); this._setEnabled(true); + this._setNeedsReauth(false); } async reassertIfEnabled() { - if (!this.$enabled.get() || !this.isSupported || !this.hasService) return; + if (!this.$enabled.get() || !this.isSupported || !this.hasService) { + return { enabled: this.isEnabled }; + } if (Notification.permission !== "granted") { // Permission was revoked out-of-band (browser site settings). this._setEnabled(false); - return; + return { enabled: false }; } try { const config = await this.fetchServiceConfig(); await this._subscribeAndRegister(config); } catch (error) { + if ( + error instanceof ApiError && + (error.status === 401 || error.status === 403) + ) { + const alreadyKnown = this.$needsReauth.get(); + this._setNeedsReauth(true); + return { enabled: true, newlyRevoked: !alreadyKnown }; + } console.error("Failed to re-assert push registration", error); } + return { enabled: true }; } async _apiForAccount(did) { @@ -286,6 +317,7 @@ export class PushNotificationService { async disable() { this._setEnabled(false); + this._setNeedsReauth(false); const subscription = await this._getSubscription(); if (!subscription) return; await this._unregisterDevice(subscription); diff --git a/src/js/router.js b/src/js/router.js index 0f7f0d21..c6089394 100644 --- a/src/js/router.js +++ b/src/js/router.js @@ -286,8 +286,11 @@ export class Router extends EventEmitter { const scrollY = this.scrollStates.get(path) ?? 0; this.currentPage.classList.remove("page-hidden"); this.currentPage.classList.add("page-visible"); - outgoingPage.classList.remove("page-visible"); - outgoingPage.classList.add("page-hidden"); + // A same-path load reuses the outgoing page, which must stay visible + if (outgoingPage !== page) { + outgoingPage.classList.remove("page-visible"); + outgoingPage.classList.add("page-hidden"); + } // Scroll before dispatching so a "manual" view's own scroll wins const scrollRestore = routeInfo.options.scrollRestore ?? "back"; switch (scrollRestore) { @@ -369,6 +372,10 @@ export class Router extends EventEmitter { window.open(path, "_blank", "noopener"); return; } + if (path === this.currentPath) { + window.scrollTo(0, 0); + return; + } if (replace) { window.history.replaceState(null, "", path); } else { diff --git a/src/js/views/settings/notifications.view.js b/src/js/views/settings/notifications.view.js index d83e78ed..b38f1021 100644 --- a/src/js/views/settings/notifications.view.js +++ b/src/js/views/settings/notifications.view.js @@ -5,6 +5,7 @@ import { classnames } from "/js/utils.js"; import { choiceModal } from "/js/modals/choice.modal.js"; import { showToast } from "/js/toasts.js"; import { Signal, ReactiveStore } from "/js/signals.js"; +import { alertIconTemplate } from "/js/templates/icons/alertIcon.template.js"; import "/js/components/toggle-switch.js"; function consumePushNotificationServiceCallbackParams() { @@ -72,6 +73,10 @@ export default async function settingsNotificationsView({ } return; } + await startPushEnableFlow(); + } + + async function startPushEnableFlow() { let permission = null; const choice = await choiceModal( "You'll be sent to the notification service to authorize push notifications. Message previews require additional read-only access to chat messages.", @@ -168,6 +173,9 @@ export default async function settingsNotificationsView({ pushNotificationService?.requiresInstall ?? false; const serviceDid = pushNotificationService?.serviceDid ?? null; const hasService = serviceDid !== null; + const pushNeedsReauth = pushNotificationService?.needsReauth ?? false; + const showReauthWarning = + pushSupported && hasService && pushEnabled && pushNeedsReauth; const systemRowDisabled = !isSupported || isDenied; const pushRowDisabled = !pushSupported || !hasService || pushBusy; @@ -233,6 +241,16 @@ export default async function settingsNotificationsView({ @change=${(event) => handlePushToggle(event.detail.checked)} > + ${showReauthWarning + ? html` +
+

${alertIconTemplate()} Authorization needed

+ The notification service no longer accepts this device's + registration, so push notifications aren't being delivered. + Disable and reenable the setting to re-authorize. +
+ ` + : null} `, diff --git a/tests/e2e/mockServer.js b/tests/e2e/mockServer.js index 1e2ac1b9..77469984 100644 --- a/tests/e2e/mockServer.js +++ b/tests/e2e/mockServer.js @@ -118,6 +118,7 @@ export class MockServer { this.notificationServiceDid = null; this.registerPushCalls = []; this.unregisterPushCalls = []; + this.registerPushStatus = 200; this.slingshotUnreachable = false; this.pdsEndpoint = "http://localhost:8081"; } @@ -181,6 +182,12 @@ export class MockServer { this.notificationServiceDid = did; } + // The notification service no longer accepts this user's registrations, as + // if the authorization was revoked server-side. Call before setup(). + failRegisterPushWithAuthError() { + this.registerPushStatus = 401; + } + setSearchHistory({ searches = [], profiles = [] } = {}) { this.searchHistory = { searches, profiles }; } @@ -1074,7 +1081,7 @@ export class MockServer { await page.route("**/xrpc/app.bsky.notification.registerPush*", (route) => { this.registerPushCalls.push(route.request().postDataJSON()); return route.fulfill({ - status: 200, + status: this.registerPushStatus, contentType: "text/plain", body: "", }); diff --git a/tests/e2e/specs/concerns/samePathNavigation.test.js b/tests/e2e/specs/concerns/samePathNavigation.test.js new file mode 100644 index 00000000..74ae35b4 --- /dev/null +++ b/tests/e2e/specs/concerns/samePathNavigation.test.js @@ -0,0 +1,32 @@ +import { test, expect } from "../../base.js"; +import { login } from "../../helpers.js"; +import { MockServer } from "../../mockServer.js"; + +test.describe("Same-path navigation", () => { + test("clicking a link to the current page keeps the view visible", async ({ + page, + }) => { + const mockServer = new MockServer(); + await mockServer.setup(page); + await login(page); + await page.goto("/settings/notifications"); + const section = page.locator( + '[data-testid="settings-section-push-notifications"]', + ); + await expect(section).toBeVisible({ timeout: 10000 }); + + // No view links to itself in static markup, but transient UI (e.g. the + // push re-auth toast) can — inject one to click. + await page.evaluate(() => { + const anchor = document.createElement("a"); + anchor.href = "/settings/notifications"; + anchor.textContent = "self"; + anchor.dataset.testid = "self-link"; + document.querySelector(".page-visible").appendChild(anchor); + }); + await page.locator('[data-testid="self-link"]').click(); + + await expect(section).toBeVisible(); + await expect(page).toHaveURL(/\/settings\/notifications$/); + }); +}); diff --git a/tests/e2e/specs/views/settingsNotifications.view.test.js b/tests/e2e/specs/views/settingsNotifications.view.test.js index a43e1f40..57de9a0a 100644 --- a/tests/e2e/specs/views/settingsNotifications.view.test.js +++ b/tests/e2e/specs/views/settingsNotifications.view.test.js @@ -404,6 +404,81 @@ test.describe("Settings > Notifications view", () => { .toBeNull(); }); + test("a stale authorization shows the re-auth warning with the toggle still on", async ({ + page, + }) => { + const mockServer = new MockServer(); + mockServer.setNotificationServiceDid(notificationService.did); + mockServer.failRegisterPushWithAuthError(); + await mockServer.setup(page); + await stubNotificationPermission(page, { initial: "granted" }); + await login(page); + await page.addInitScript(() => { + localStorage.setItem("push-notifications-enabled", "true"); + localStorage.setItem("push-notifications-needs-reauth", "true"); + }); + await page.goto("/settings/notifications"); + + const warning = page.locator('[data-testid="push-reauth-warning"]'); + await expect(warning).toBeVisible({ timeout: 10000 }); + // Only the authorization is stale — the user's choice stays on. + await expect( + page.locator('[data-testid="push-notifications-toggle"]'), + ).toHaveAttribute("checked", ""); + }); + + test("the re-authorize button restarts the enable flow", async ({ + page, + }) => { + const mockServer = new MockServer(); + mockServer.setNotificationServiceDid(notificationService.did); + mockServer.failRegisterPushWithAuthError(); + await mockServer.setup(page); + await stubNotificationPermission(page, { initial: "granted" }); + await login(page); + await page.addInitScript(() => { + localStorage.setItem("push-notifications-enabled", "true"); + localStorage.setItem("push-notifications-needs-reauth", "true"); + }); + await page.goto("/settings/notifications"); + + await page + .locator('[data-testid="push-reauth-button"]') + .click({ timeout: 10000 }); + await page + .locator( + '[data-testid="choice-modal"] [data-testid="modal-choice-without-previews"]', + ) + .click(); + + await expect(page).toHaveURL( + new RegExp( + notificationService.authUrl.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"), + ), + ); + }); + + test("no re-auth warning while the authorization is good", async ({ + page, + }) => { + const mockServer = new MockServer(); + mockServer.setNotificationServiceDid(notificationService.did); + await mockServer.setup(page); + await stubNotificationPermission(page, { initial: "granted" }); + await login(page); + await page.addInitScript(() => { + localStorage.setItem("push-notifications-enabled", "true"); + }); + await page.goto("/settings/notifications"); + + await expect( + page.locator('[data-testid="push-notifications-toggle"]'), + ).toHaveAttribute("checked", "", { timeout: 10000 }); + await expect( + page.locator('[data-testid="push-reauth-warning"]'), + ).toHaveCount(0); + }); + test("with a service named, the toggle is usable", async ({ page }) => { const mockServer = new MockServer(); mockServer.setNotificationServiceDid(notificationService.did); diff --git a/tests/unit/specs/pushNotificationService.test.js b/tests/unit/specs/pushNotificationService.test.js index 0239a138..7c6b89b4 100644 --- a/tests/unit/specs/pushNotificationService.test.js +++ b/tests/unit/specs/pushNotificationService.test.js @@ -1,6 +1,7 @@ import { describe, it, mock, beforeEach, afterEach } from "node:test"; import assert from "node:assert/strict"; import { PushNotificationService } from "/js/push/pushNotificationService.js"; +import { ApiError } from "/js/api.js"; import { Signal, effect } from "/js/signals.js"; const SUBSCRIPTION = { @@ -206,7 +207,7 @@ describe("PushNotificationService registration", () => { it("clears the stored flag when permission was revoked out-of-band", async () => { setupDom({ granted: false }); const { service, registerPush } = createService("did:web:notifs.example"); - await service.reassertIfEnabled(); + assert.deepEqual(await service.reassertIfEnabled(), { enabled: false }); assert.equal(registerPush.mock.calls.length, 0); assert.equal(service.$enabled.get(), false); }); @@ -656,3 +657,173 @@ describe("PushNotificationService service selection", () => { ); }); }); + +describe("PushNotificationService re-authorization", () => { + let originals; + + const CONFIG = { name: "Example Notifs", vapidPublicKey: "k", authUrl: "u" }; + + function authError(status) { + return new ApiError({ + status, + statusText: "Unauthorized", + data: null, + headers: null, + url: "https://pds.example/xrpc/app.bsky.notification.registerPush", + }); + } + + beforeEach(() => { + originals = { + localStorage: globalThis.localStorage, + notification: globalThis.Notification, + document: globalThis.document, + navigator: Object.getOwnPropertyDescriptor(globalThis, "navigator"), + pushManager: globalThis.window.PushManager, + matchMedia: globalThis.window.matchMedia, + fetch: globalThis.fetch, + consoleError: console.error, + }; + setupDom(); + globalThis.fetch = async (url) => ({ + ok: true, + status: 200, + json: async () => + String(url).includes("notif-service.json") + ? CONFIG + : { + service: [ + { + id: "#bsky_notif", + type: "BskyNotificationService", + serviceEndpoint: "https://notifs.example", + }, + ], + }, + }); + }); + + afterEach(() => { + globalThis.localStorage = originals.localStorage; + globalThis.Notification = originals.notification; + globalThis.document = originals.document; + globalThis.window.matchMedia = originals.matchMedia; + Object.defineProperty(globalThis, "navigator", originals.navigator); + globalThis.fetch = originals.fetch; + console.error = originals.consoleError; + if (originals.pushManager === undefined) { + delete globalThis.window.PushManager; + } else { + globalThis.window.PushManager = originals.pushManager; + } + }); + + it("flags a 401 registration rejection and reports the first detection", async () => { + const { service, registerPush } = createService("did:web:notifs.example"); + registerPush.mock.mockImplementation(async () => { + throw authError(401); + }); + + const result = await service.reassertIfEnabled(); + + assert.deepEqual(result, { enabled: true, newlyRevoked: true }); + assert.equal(service.needsReauth, true); + assert.equal( + globalThis.localStorage.getItem("push-notifications-needs-reauth"), + "true", + ); + // The user keeps push notifications on; only the authorization is stale. + assert.equal(service.isEnabled, true); + }); + + it("reports an already-known rejection as not new", async () => { + const { service, registerPush } = createService("did:web:notifs.example"); + registerPush.mock.mockImplementation(async () => { + throw authError(403); + }); + + assert.deepEqual(await service.reassertIfEnabled(), { + enabled: true, + newlyRevoked: true, + }); + assert.deepEqual(await service.reassertIfEnabled(), { + enabled: true, + newlyRevoked: false, + }); + assert.equal(service.needsReauth, true); + }); + + it("does not flag transient registration failures", async () => { + const { service, registerPush } = createService("did:web:notifs.example"); + console.error = () => {}; + for (const failure of [authError(500), new TypeError("Failed to fetch")]) { + registerPush.mock.mockImplementation(async () => { + throw failure; + }); + assert.deepEqual(await service.reassertIfEnabled(), { enabled: true }); + assert.equal(service.needsReauth, false); + } + assert.equal( + globalThis.localStorage.getItem("push-notifications-needs-reauth"), + null, + ); + }); + + it("clears the flag once a registration succeeds again", async () => { + globalThis.localStorage.setItem("push-notifications-needs-reauth", "true"); + const { service } = createService("did:web:notifs.example"); + assert.equal(service.needsReauth, true); + + assert.deepEqual(await service.reassertIfEnabled(), { enabled: true }); + + assert.equal(service.needsReauth, false); + assert.equal( + globalThis.localStorage.getItem("push-notifications-needs-reauth"), + null, + ); + }); + + it("disable() clears the flag", async () => { + globalThis.localStorage.setItem("push-notifications-needs-reauth", "true"); + const { service } = createService("did:web:notifs.example"); + service.api.unregisterPush = mock.fn(async () => {}); + + await service.disable(); + + assert.equal(service.needsReauth, false); + assert.equal( + globalThis.localStorage.getItem("push-notifications-needs-reauth"), + null, + ); + }); + + it("switching services clears the flag", async () => { + globalThis.localStorage.setItem("push-notifications-needs-reauth", "true"); + globalThis.localStorage.removeItem("push-notifications-enabled"); + const { service } = createService("did:web:notifs.example"); + + await service.selectService("did:web:elsewhere.example"); + + assert.equal(service.needsReauth, false); + }); + + it("notifies reactive readers when the flag changes", async () => { + const { service, registerPush } = createService("did:web:notifs.example"); + registerPush.mock.mockImplementation(async () => { + throw authError(401); + }); + const seen = []; + // Effects flush on an animation frame, so each change needs one to land. + const flush = () => + new Promise((resolve) => requestAnimationFrame(() => resolve())); + const dispose = effect(() => { + seen.push(service.needsReauth); + }); + + await service.reassertIfEnabled(); + await flush(); + + assert.deepEqual(seen, [false, true]); + dispose(); + }); +}); diff --git a/tests/unit/specs/router.test.js b/tests/unit/specs/router.test.js index 7bbcf2d8..6b3f1d74 100644 --- a/tests/unit/specs/router.test.js +++ b/tests/unit/specs/router.test.js @@ -346,6 +346,24 @@ describe("popstate", () => { }); describe("load", () => { + // Reached without go()'s same-path guard when popstate lands on the URL + // the app is already showing. + it("keeps the page visible when re-loading the current path", async () => { + const router = new Router(); + mountRouter(router); + router.addRoute("/load-same-test", () => Promise.resolve({})); + router.renderRoute(() => {}); + + await router.load("/load-same-test"); + const page = router.currentPage; + + await router.load("/load-same-test"); + + assert.deepEqual(router.currentPage, page); + assert(page.classList.contains("page-visible")); + assert(!page.classList.contains("page-hidden")); + }); + it("should load route and render view", async () => { const router = new Router(); const { defaultContainer } = mountRouter(router); @@ -472,6 +490,29 @@ describe("go", () => { window.history.replaceState(originalState, "", originalPath); } }); + + it("should treat navigation to the current path as a no-op", async () => { + const router = new Router(); + mountRouter(router); + router.addRoute("/go-same-test", () => Promise.resolve({})); + router.renderRoute(() => {}); + + try { + await router.go("/go-same-test"); + const page = router.currentPage; + const stateBefore = window.history.state; + + await router.go("/go-same-test"); + + assert.deepEqual(router.currentPage, page); + assert(page.classList.contains("page-visible")); + assert(!page.classList.contains("page-hidden")); + // No duplicate history entry pointing back at the same path. + assert.deepEqual(window.history.state, stateBefore); + } finally { + window.history.replaceState(originalState, "", originalPath); + } + }); }); describe("modifier-click navigation", () => {