Keep push alive across a deploy, not just across a week
#143 added a device-local flag recording that background notifications were switched on in this browser, and made the renewal on app start key off it. It is not in KEEP_ON_SIGN_OUT, and that is the whole bug: clearSignedInData() runs on two different endings and only one of them is a sign-out. The other is a session expiring, which is what a deploy does to every signed-in browser at once. That path deliberately does not remove the push subscription -- there is no session left to remove it with -- so the subscription stays registered at Stalwart and the browser keeps its own. Losing the flag there left nothing to renew them: push would have gone quiet a week after every deploy, with the switch in Settings still reading as on because both ends of the subscription still existed. That is the exact failure #143 was written to prevent, reintroduced through a different door, and the first deploy carrying #143 would have been the thing that triggered it. Signing out for real still forgets it. That happens directly in unsubscribeThisDevice, next to destroying the subscription, and it happens even when the server cannot be reached -- a browser that goes on believing it has push would have renewal resurrect it on the next sign-in. Both halves are tested now, because they are one invariant seen from two sides: storage.test.ts covers the flag surviving an expiry, webpush.test.ts covers a sign-out clearing it with the server unreachable.
This commit is contained in:
@@ -75,6 +75,7 @@ describe("device-trusted storage", () => {
|
||||
saveJson(accountKey("acct1", "recent"), [{ email: "[email protected]" }]);
|
||||
store.set("ihasmail:lastUser", "[email protected]");
|
||||
store.set("ihasmail:pushDeviceId", "ihasmail-abc");
|
||||
store.set("ihasmail:pushEnabled", "1");
|
||||
|
||||
clearSignedInData();
|
||||
|
||||
@@ -84,6 +85,14 @@ describe("device-trusted storage", () => {
|
||||
// Kept on purpose: prefills sign-in, and only a trusted device wrote it.
|
||||
expect(store.get("ihasmail:lastUser")).toBe("[email protected]");
|
||||
expect(store.get("ihasmail:pushDeviceId")).toBe("ihasmail-abc");
|
||||
/*
|
||||
* Kept for the ending that is not a sign-out. A deploy expires every
|
||||
* session, and that path clears local data without unsubscribing -- there
|
||||
* is no session left to unsubscribe with. Losing the flag there would
|
||||
* strand a live subscription with nothing renewing it, and the switch in
|
||||
* Settings would still say background notifications were on.
|
||||
*/
|
||||
expect(store.get("ihasmail:pushEnabled")).toBe("1");
|
||||
});
|
||||
|
||||
it("clears everything, lastUser included, for an untrusted sign-in", () => {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { client } from "@/jmap/client";
|
||||
import {
|
||||
applicationServerKey,
|
||||
@@ -8,10 +8,14 @@ import {
|
||||
needsRenewal,
|
||||
RENEW_WITHIN_MS,
|
||||
subscriptionPayload,
|
||||
pushEnabledHere,
|
||||
setPushEnabledHere,
|
||||
supportsEmailPush,
|
||||
unsubscribeThisDevice,
|
||||
webPushAvailable,
|
||||
type JmapPushSubscription,
|
||||
} from "@/lib/webpush";
|
||||
import { setDeviceTrusted } from "@/lib/storage";
|
||||
import type { JmapSession } from "@/jmap/types";
|
||||
|
||||
/**
|
||||
@@ -240,3 +244,62 @@ describe("needsRenewal", () => {
|
||||
expect(needsRenewal([sub(MINE, "whenever")], MINE, NOW)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Whether push is on *in this browser* is the flag the renewal on app start
|
||||
* keys off, so the two endings that can clear it have to be told apart.
|
||||
*
|
||||
* Signing out clears it, alongside destroying the subscription itself: a
|
||||
* browser left notifying for a mailbox nobody is signed into is somebody
|
||||
* else's mail on a shared machine. A session merely expiring must not, because
|
||||
* that path -- which is what a deploy does to everyone at once -- leaves the
|
||||
* subscription registered and has no session left to remove it with. That half
|
||||
* is enforced by `KEEP_ON_SIGN_OUT` and tested in storage.test.ts.
|
||||
*/
|
||||
describe("remembering that push is on here", () => {
|
||||
let store: Map<string, string>;
|
||||
|
||||
beforeEach(() => {
|
||||
store = new Map();
|
||||
Object.defineProperty(globalThis, "localStorage", {
|
||||
configurable: true,
|
||||
value: {
|
||||
getItem: (k: string) => store.get(k) ?? null,
|
||||
setItem: (k: string, v: string) => void store.set(k, v),
|
||||
removeItem: (k: string) => void store.delete(k),
|
||||
},
|
||||
});
|
||||
setDeviceTrusted(true);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
setDeviceTrusted(false);
|
||||
Reflect.deleteProperty(globalThis, "localStorage");
|
||||
});
|
||||
|
||||
it("round-trips, and is off until something turns it on", () => {
|
||||
expect(pushEnabledHere()).toBe(false);
|
||||
setPushEnabledHere(true);
|
||||
expect(pushEnabledHere()).toBe(true);
|
||||
setPushEnabledHere(false);
|
||||
expect(pushEnabledHere()).toBe(false);
|
||||
});
|
||||
|
||||
it("stays off on a device nobody said was theirs", () => {
|
||||
// Push is refused there anyway; reading the flag as set would start the
|
||||
// renewal trying on every load for a subscription that cannot exist.
|
||||
setPushEnabledHere(true);
|
||||
setDeviceTrusted(false);
|
||||
expect(pushEnabledHere()).toBe(false);
|
||||
});
|
||||
|
||||
it("is cleared by signing out, even when the server end cannot be reached", () => {
|
||||
setPushEnabledHere(true);
|
||||
vi.spyOn(client, "call").mockRejectedValue(new Error("offline"));
|
||||
return unsubscribeThisDevice().then(() => {
|
||||
// The subscription may well survive at the server; this browser must
|
||||
// still stop believing it has push, or renewal would resurrect it.
|
||||
expect(pushEnabledHere()).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
+11
-1
@@ -22,8 +22,18 @@ const PREFIX = "ihasmail:";
|
||||
* - `deviceTrusted` is how the next boot knows to read at all.
|
||||
* - `pushDeviceId` is a random id for this browser, so re-subscribing replaces
|
||||
* rather than accumulates. The subscription itself is removed on sign-out.
|
||||
* - `pushEnabled` records that background notifications were switched on here,
|
||||
* and is what the renewal on app start keys off. It is kept because this
|
||||
* function runs on two different endings and only one of them is a sign-out:
|
||||
* a *deploy* expires every session, and the handler for that clears local
|
||||
* data without removing the push subscription, because there is no longer a
|
||||
* session to remove it with. Dropping the flag there would leave the
|
||||
* subscription registered, the switch still reading as on, and nothing
|
||||
* renewing it -- so push would go quiet a week after every deploy, which is
|
||||
* the exact failure the renewal exists to prevent. Signing out for real
|
||||
* clears it directly, in `unsubscribeThisDevice`, alongside the subscription.
|
||||
*/
|
||||
const KEEP_ON_SIGN_OUT = ["lastUser", "deviceTrusted", "pushDeviceId"];
|
||||
const KEEP_ON_SIGN_OUT = ["lastUser", "deviceTrusted", "pushDeviceId", "pushEnabled"];
|
||||
|
||||
const TRUST_KEY = `${PREFIX}deviceTrusted`;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user