diff --git a/apps/web/src/app/server-storage/server-sign-in.component.spec.ts b/apps/web/src/app/server-storage/server-sign-in.component.spec.ts index f92c57529..469754075 100644 --- a/apps/web/src/app/server-storage/server-sign-in.component.spec.ts +++ b/apps/web/src/app/server-storage/server-sign-in.component.spec.ts @@ -1,5 +1,6 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { TranslateModule } from '@ngx-translate/core'; +import { readServerAccessToken } from '@iptvnator/services'; import { SERVER_STORAGE_CONFIG, ServerApiError, @@ -60,6 +61,9 @@ describe('ServerSignInComponent', () => { await component.submit(new Event('submit')); expect(localStorage.getItem(TOKEN_KEY)).toBe('right'); + // Only for the instance it was given for. + expect(readServerAccessToken('i-1')).toBe('right'); + expect(readServerAccessToken('i-2')).toBeNull(); expect(open).toHaveBeenCalled(); }); diff --git a/apps/web/src/app/server-storage/server-sign-in.component.ts b/apps/web/src/app/server-storage/server-sign-in.component.ts index b7a93d618..26b05f7cc 100644 --- a/apps/web/src/app/server-storage/server-sign-in.component.ts +++ b/apps/web/src/app/server-storage/server-sign-in.component.ts @@ -70,7 +70,7 @@ export class ServerSignInComponent { // Every request and the reload read the token from storage, and the // next start keeps it only for the server it remembers. if ( - !writeServerAccessToken(token) || + !writeServerAccessToken(token, this.config.instance.instanceId) || !rememberServer(this.config.baseUrl, this.config.instance) ) { clearServerAccessToken(); diff --git a/apps/web/src/app/services/pwa.service.spec.ts b/apps/web/src/app/services/pwa.service.spec.ts index b381bde46..151dec81d 100644 --- a/apps/web/src/app/services/pwa.service.spec.ts +++ b/apps/web/src/app/services/pwa.service.spec.ts @@ -21,8 +21,9 @@ import { isStalkerProbeTimeout, } from '@iptvnator/portal/stalker/data-access'; import { - SERVER_ACCESS_TOKEN_STORAGE_KEY, + clearServerAccessToken, ServerStorageStatusService, + writeServerAccessToken, } from '@iptvnator/services'; import { PwaService } from './pwa.service'; @@ -284,7 +285,7 @@ describe('PwaService', () => { }); it('sends a stored server token only while server mode runs', async () => { - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 'kept-token'); + writeServerAccessToken('kept-token', 'i'); const originalFetch = globalThis.fetch; const fetchMock = jest.fn().mockResolvedValue({ ok: true, @@ -335,7 +336,7 @@ describe('PwaService', () => { }); } finally { globalThis.fetch = originalFetch; - localStorage.removeItem(SERVER_ACCESS_TOKEN_STORAGE_KEY); + clearServerAccessToken(); } }); diff --git a/docs/architecture/pwa-self-hosted.md b/docs/architecture/pwa-self-hosted.md index 016482a68..cc0e8be61 100644 --- a/docs/architecture/pwa-self-hosted.md +++ b/docs/architecture/pwa-self-hosted.md @@ -240,7 +240,10 @@ settings and guards read them in every build. replay; the writes of a tab closed before it could send them are queued, ahead of its own, by the next re-read in another tab once that tab's record has gone stale; the result replays the - closed tab's edits first and this tab's newer ones last. Tabs never clear each other's. + closed tab's edits first and this tab's newer ones last. An adopted edit + changes only what is still as its page saw it (`EditStep.yields`): an + item or key changed since, by this tab or another device, keeps its + newer value. Tabs never clear each other's. Cache and saved writes belong to one server database (`iptvnator-server:cache-owner`); another one behind the same address starts clean. Also, @@ -297,7 +300,10 @@ settings and guards read them in every build. `/server-sign-in` (the start-up screen lets it through although the playlists could not load), which checks the token against `/v1/instance/status` before it stores it under `iptvnator-server:access-token` and reloads (a - browser that refuses to store it is told so instead). + browser that refuses to store it is told so instead). The token is stored + with the instance id it was given for, and a page sends it only to that + instance: a tab still open on the old backend after `BACKEND_URL` changed + does not send the new server's token there, and goes to sign-in instead. `ServerAccessTokenInterceptor` adds the header to `HttpClient` calls to the backend, and `PwaService` adds it to its two raw `fetch` calls only while server mode runs on a verified instance. The browser forgets the token @@ -308,8 +314,10 @@ settings and guards read them in every build. address is the remembered one (`ensureVerified`), and reloads if not, before sign-in sends the token. Both turn the guard's 401 (`isServerAccessChallenge`) into `reportAccessDenied()`, - so a token changed on the server sends an open page back to sign-in; a - late 401 for a token replaced since (a new sign-in) changes nothing. + as the storage API client does with any 401, so a token changed on the + server sends an open page back to sign-in; a late 401 for a token + replaced since (a new sign-in) changes nothing, nor does one for a page + that holds its token back until the instance is checked. - **Moving browser data.** `ServerMigrationService` offers, once per server instance (marker `iptvnator-server:migration:`), to move the playlists this browser kept in IndexedDB (`planServerMigration`), Stalker diff --git a/libs/services/src/lib/server-access-token.spec.ts b/libs/services/src/lib/server-access-token.spec.ts index 0887bf5c3..2cd738527 100644 --- a/libs/services/src/lib/server-access-token.spec.ts +++ b/libs/services/src/lib/server-access-token.spec.ts @@ -1,31 +1,40 @@ import { clearServerAccessToken, readServerAccessToken, + SERVER_ACCESS_TOKEN_OWNER_KEY, SERVER_ACCESS_TOKEN_STORAGE_KEY, - serverAuthorizationHeaders, writeServerAccessToken, } from './server-access-token'; describe('server access token', () => { afterEach(() => localStorage.clear()); - it('has no header without a token', () => { + it('has no token until one is stored', () => { expect(readServerAccessToken()).toBeNull(); - expect(serverAuthorizationHeaders()).toEqual({}); + expect(readServerAccessToken('i-1')).toBeNull(); }); - it('stores a trimmed token and builds the bearer header', () => { - writeServerAccessToken(' s3cret '); + it('stores a trimmed token for the instance it was given for', () => { + writeServerAccessToken(' s3cret ', 'i-1'); expect(localStorage.getItem(SERVER_ACCESS_TOKEN_STORAGE_KEY)).toBe( 's3cret' ); - expect(serverAuthorizationHeaders()).toEqual({ - Authorization: 'Bearer s3cret', - }); + expect(readServerAccessToken('i-1')).toBe('s3cret'); + expect(readServerAccessToken()).toBe('s3cret'); clearServerAccessToken(); expect(readServerAccessToken()).toBeNull(); + expect(localStorage.getItem(SERVER_ACCESS_TOKEN_OWNER_KEY)).toBeNull(); + }); + + it("never hands another instance's token out", () => { + writeServerAccessToken('new-server', 'i-2'); + expect(readServerAccessToken('i-1')).toBeNull(); + + // A token stored without its instance belongs to none. + localStorage.removeItem(SERVER_ACCESS_TOKEN_OWNER_KEY); + expect(readServerAccessToken('i-2')).toBeNull(); }); it('reports a token browser storage refuses', () => { @@ -35,11 +44,11 @@ describe('server access token', () => { throw new DOMException('full', 'QuotaExceededError'); }); try { - expect(writeServerAccessToken('s3cret')).toBe(false); + expect(writeServerAccessToken('s3cret', 'i-1')).toBe(false); expect(readServerAccessToken()).toBeNull(); } finally { refuse.mockRestore(); } - expect(writeServerAccessToken('s3cret')).toBe(true); + expect(writeServerAccessToken('s3cret', 'i-1')).toBe(true); }); }); diff --git a/libs/services/src/lib/server-access-token.ts b/libs/services/src/lib/server-access-token.ts index 1bde6e6dd..aac90e92e 100644 --- a/libs/services/src/lib/server-access-token.ts +++ b/libs/services/src/lib/server-access-token.ts @@ -10,21 +10,43 @@ export function isServerAccessChallenge( return status === 401 && challenge === SERVER_ACCESS_CHALLENGE; } -export function readServerAccessToken(): string | null { +/** The instance the stored token was given for. */ +export const SERVER_ACCESS_TOKEN_OWNER_KEY = + 'iptvnator-server:access-token-instance'; + +/** + * The stored token; with `instanceId`, only when it was given for that + * instance: a tab still open on another server (a changed backend URL) + * never sends it there. + */ +export function readServerAccessToken( + instanceId?: string | null +): string | null { try { const token = localStorage .getItem(SERVER_ACCESS_TOKEN_STORAGE_KEY) ?.trim(); - return token ? token : null; + return token && + (instanceId === undefined || + localStorage.getItem(SERVER_ACCESS_TOKEN_OWNER_KEY) === + (instanceId ?? '')) + ? token + : null; } catch { return null; } } /** False when browser storage refused it (private mode, full). */ -export function writeServerAccessToken(token: string): boolean { +export function writeServerAccessToken( + token: string, + instanceId: string | null +): boolean { try { + // The owner goes last: a token stored without one is sent nowhere. + localStorage.removeItem(SERVER_ACCESS_TOKEN_OWNER_KEY); localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, token.trim()); + localStorage.setItem(SERVER_ACCESS_TOKEN_OWNER_KEY, instanceId ?? ''); return true; } catch { return false; @@ -34,13 +56,8 @@ export function writeServerAccessToken(token: string): boolean { export function clearServerAccessToken(): void { try { localStorage.removeItem(SERVER_ACCESS_TOKEN_STORAGE_KEY); + localStorage.removeItem(SERVER_ACCESS_TOKEN_OWNER_KEY); } catch { // Nothing stored. } } - -/** `Authorization` header for calls to the self-hosted backend. */ -export function serverAuthorizationHeaders(): Record { - const token = readServerAccessToken(); - return token ? { Authorization: `Bearer ${token}` } : {}; -} diff --git a/libs/services/src/lib/server-storage-status.service.spec.ts b/libs/services/src/lib/server-storage-status.service.spec.ts index 6755593ef..319ec7823 100644 --- a/libs/services/src/lib/server-storage-status.service.spec.ts +++ b/libs/services/src/lib/server-storage-status.service.spec.ts @@ -1,6 +1,10 @@ import { TestBed } from '@angular/core/testing'; import type { ServerInstanceInfo } from '@iptvnator/shared/interfaces'; -import { SERVER_ACCESS_TOKEN_STORAGE_KEY } from './server-access-token'; +import { + readServerAccessToken, + SERVER_ACCESS_TOKEN_STORAGE_KEY, + writeServerAccessToken, +} from './server-access-token'; import { ServerStorageStatusService } from './server-storage-status.service'; const instance: ServerInstanceInfo = { @@ -52,7 +56,7 @@ describe('ServerStorageStatusService', () => { it('ignores a refusal of a token replaced since', () => { status.activate(instance, jest.fn()); - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 'new'); + writeServerAccessToken('new', 'i-1'); status.reportAccessDenied('old'); expect(status.state()).toBe('online'); @@ -66,4 +70,27 @@ describe('ServerStorageStatusService', () => { localStorage.getItem(SERVER_ACCESS_TOKEN_STORAGE_KEY) ).toBeNull(); }); + + it("sends no other instance's token and asks for this one's", () => { + status.activate(instance, jest.fn()); + // Another tab signed in to the server a new backend URL leads to. + writeServerAccessToken('new-server', 'i-2'); + + expect(status.accessToken()).toBeNull(); + status.reportAccessDenied(null); + + expect(status.state()).toBe('unauthorized'); + expect(readServerAccessToken('i-2')).toBe('new-server'); + }); + + it('keeps the token of a page that holds it back on a refusal', () => { + status.activate(instance, jest.fn(), false); + writeServerAccessToken('kept', 'i-1'); + + expect(status.accessToken()).toBeNull(); + status.reportAccessDenied(null); + + expect(status.state()).toBe('online'); + expect(readServerAccessToken('i-1')).toBe('kept'); + }); }); diff --git a/libs/services/src/lib/server-storage-status.service.ts b/libs/services/src/lib/server-storage-status.service.ts index 06bb7a982..e3d7e3784 100644 --- a/libs/services/src/lib/server-storage-status.service.ts +++ b/libs/services/src/lib/server-storage-status.service.ts @@ -54,8 +54,9 @@ export class ServerStorageStatusService { * for. */ accessToken(): string | null { - return this.isActive() && this.verified - ? readServerAccessToken() + const instance = this.instanceSignal(); + return instance && this.isActive() && this.verified + ? readServerAccessToken(instance.instanceId) : null; } @@ -83,15 +84,25 @@ export class ServerStorageStatusService { } /** - * The access guard refused a request outside the storage API (a - * provider proxy call): the stored token is stale, ask for a new one. + * The access guard refused a request sent with `sentToken`: the token + * stored for this instance is stale or missing, ask for a new one. */ reportAccessDenied(sentToken: string | null): void { - // A late answer to a token replaced since (a new sign-in) is moot. - if (this.isActive() && readServerAccessToken() === sentToken) { - clearServerAccessToken(); - this.stateSignal.set('unauthorized'); + const instance = this.instanceSignal(); + // A late answer to a token replaced since (a new sign-in) is moot, + // and so is one to a page that held its token back until the + // instance is checked. + if ( + !instance || + !this.isActive() || + readServerAccessToken(instance.instanceId) !== sentToken + ) { + return; } + if (sentToken) { + clearServerAccessToken(); + } + this.stateSignal.set('unauthorized'); } retry(): void { diff --git a/libs/services/src/lib/server-storage/server-access-token.interceptor.spec.ts b/libs/services/src/lib/server-storage/server-access-token.interceptor.spec.ts index 152990e43..58ad04a05 100644 --- a/libs/services/src/lib/server-storage/server-access-token.interceptor.spec.ts +++ b/libs/services/src/lib/server-storage/server-access-token.interceptor.spec.ts @@ -9,7 +9,10 @@ import { provideHttpClientTesting, } from '@angular/common/http/testing'; import { TestBed } from '@angular/core/testing'; -import { SERVER_ACCESS_TOKEN_STORAGE_KEY } from '../server-access-token'; +import { + SERVER_ACCESS_TOKEN_STORAGE_KEY, + writeServerAccessToken, +} from '../server-access-token'; import { ServerStorageStatusService } from '../server-storage-status.service'; import { isBackendUrl, @@ -19,7 +22,7 @@ import { SERVER_STORAGE_CONFIG } from './server-storage.config'; describe('ServerAccessTokenInterceptor', () => { beforeEach(() => { - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 't'); + writeServerAccessToken('t', 'i'); TestBed.configureTestingModule({ providers: [ provideHttpClient(withInterceptorsFromDi()), diff --git a/libs/services/src/lib/server-storage/server-api-client.spec.ts b/libs/services/src/lib/server-storage/server-api-client.spec.ts index 96a6fea9a..024128790 100644 --- a/libs/services/src/lib/server-storage/server-api-client.spec.ts +++ b/libs/services/src/lib/server-storage/server-api-client.spec.ts @@ -1,4 +1,3 @@ -import { SERVER_ACCESS_TOKEN_STORAGE_KEY } from '../server-access-token'; import { ServerApiClient, ServerApiError } from './server-api-client'; // jsdom has Headers but no Response or fetch. @@ -16,13 +15,14 @@ describe('ServerApiClient', () => { const status = { reportSuccess: jest.fn(), reportOffline: jest.fn(), - reportUnauthorized: jest.fn(), + reportAccessDenied: jest.fn(), }; + let token: string | null = null; const onWriteFailure = jest.fn(); afterEach(() => { jest.clearAllMocks(); - localStorage.clear(); + token = null; }); function client(fetchFn: jest.Mock) { @@ -31,11 +31,12 @@ describe('ServerApiClient', () => { status, onWriteFailure, fetchFn: fetchFn as unknown as typeof fetch, + accessToken: () => token, }); } it('calls /v1 with the token and reports success', async () => { - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 't'); + token = 't'; const fetchFn = jest .fn() .mockResolvedValue(response(200, { ok: true })); @@ -130,6 +131,7 @@ describe('ServerApiClient', () => { baseUrl: '/api', status, fetchFn: fetchFn as unknown as typeof fetch, + accessToken: () => null, holdWrites: true, }); @@ -172,8 +174,8 @@ describe('ServerApiClient', () => { expect(status.reportOffline).not.toHaveBeenCalled(); }); - it('clears the token and reports unauthorized on 401', async () => { - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 'old'); + it('reports the refused token on 401', async () => { + token = 'old'; const fetchFn = jest .fn() .mockResolvedValue(response(401, { code: 'unauthorized' })); @@ -181,52 +183,9 @@ describe('ServerApiClient', () => { await expect( client(fetchFn).getJson('/playlists') ).rejects.toBeInstanceOf(ServerApiError); - expect( - localStorage.getItem(SERVER_ACCESS_TOKEN_STORAGE_KEY) - ).toBeNull(); - expect(status.reportUnauthorized).toHaveBeenCalled(); - }); - - it('keeps a token stored while a request with the old one ran', async () => { - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 'old'); - const fetchFn = jest.fn(async () => { - // The user signs in again before the old request is answered. - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 'new'); - return response(401, { code: 'unauthorized' }); - }); - - await expect( - client(fetchFn).getJson('/playlists') - ).rejects.toBeInstanceOf(ServerApiError); - expect(localStorage.getItem(SERVER_ACCESS_TOKEN_STORAGE_KEY)).toBe( - 'new' - ); - expect(status.reportUnauthorized).not.toHaveBeenCalled(); - }); - - it('sends no token to an instance not checked yet and keeps it on a 401', async () => { - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 'kept'); - const fetchFn = jest - .fn() - .mockResolvedValue(response(401, { code: 'unauthorized' })); - const unchecked = new ServerApiClient({ - baseUrl: '/api', - status, - fetchFn: fetchFn as unknown as typeof fetch, - accessToken: () => null, - }); - - await expect(unchecked.getJson('/instance')).rejects.toBeInstanceOf( - ServerApiError - ); - - expect(fetchFn.mock.calls[0][1].headers).not.toHaveProperty( - 'Authorization' - ); - expect(localStorage.getItem(SERVER_ACCESS_TOKEN_STORAGE_KEY)).toBe( - 'kept' - ); - expect(status.reportUnauthorized).not.toHaveBeenCalled(); + // The status decides: it ignores a token replaced since. + expect(status.reportAccessDenied).toHaveBeenCalledWith('old'); + expect(status.reportOffline).not.toHaveBeenCalled(); }); it('passes 304 through and maps error bodies', async () => { diff --git a/libs/services/src/lib/server-storage/server-api-client.ts b/libs/services/src/lib/server-storage/server-api-client.ts index f7e64d44b..9f6a27cba 100644 --- a/libs/services/src/lib/server-storage/server-api-client.ts +++ b/libs/services/src/lib/server-storage/server-api-client.ts @@ -3,10 +3,6 @@ import type { ServerUserStateCurrent, ServerUserStateWriteResponse, } from '@iptvnator/shared/interfaces'; -import { - clearServerAccessToken, - readServerAccessToken, -} from '../server-access-token'; import type { ServerStorageStatusService } from '../server-storage-status.service'; const REQUEST_TIMEOUT_MS = 20_000; @@ -41,7 +37,7 @@ export interface ServerApiClientOptions { readonly baseUrl: string; readonly status: Pick< ServerStorageStatusService, - 'reportSuccess' | 'reportOffline' | 'reportUnauthorized' + 'reportSuccess' | 'reportOffline' | 'reportAccessDenied' >; /** Failed writes the user should hear about: network, 413, 5xx. */ readonly onWriteFailure?: (error: ServerApiError) => void; @@ -53,8 +49,8 @@ export interface ServerApiClientOptions { * the time it does. */ readonly holdWrites?: boolean; - /** The token to send; the stored one by default. */ - readonly accessToken?: () => string | null; + /** The token to send (`ServerStorageStatusService.accessToken`). */ + readonly accessToken: () => string | null; } /** `fetch` for `/v1`: token, timeout, error mapping, connection state. */ @@ -66,7 +62,7 @@ export class ServerApiClient { } private token(): string | null { - return (this.options.accessToken ?? readServerAccessToken)(); + return this.options.accessToken(); } url(path: string): string { @@ -104,11 +100,7 @@ export class ServerApiClient { } if (response.status === 401) { - // A late answer to a token replaced since (a new sign-in) is moot. - if (readServerAccessToken() === token) { - clearServerAccessToken(); - this.options.status.reportUnauthorized(); - } + this.options.status.reportAccessDenied(token); throw new ServerApiError( 401, 'unauthorized', diff --git a/libs/services/src/lib/server-storage/server-storage-session.spec.ts b/libs/services/src/lib/server-storage/server-storage-session.spec.ts index e796501e3..792898de2 100644 --- a/libs/services/src/lib/server-storage/server-storage-session.spec.ts +++ b/libs/services/src/lib/server-storage/server-storage-session.spec.ts @@ -4,7 +4,7 @@ import { provideRouter } from '@angular/router'; import { TranslateService } from '@ngx-translate/core'; import { PlaylistStorageBackend } from '../playlist-storage-backend'; import { RuntimeCapabilitiesService } from '../runtime-capabilities.service'; -import { SERVER_ACCESS_TOKEN_STORAGE_KEY } from '../server-access-token'; +import { writeServerAccessToken } from '../server-access-token'; import { ServerStorageStatusService } from '../server-storage-status.service'; import { SERVER_STORAGE_CONFIG } from './server-storage.config'; import { ServerStorageSession } from './server-storage-session'; @@ -102,7 +102,7 @@ describe('ServerStorageSession', () => { }); it('starts with a stored token', async () => { - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 't'); + writeServerAccessToken('t', 'i-1'); const { session, status } = setup('token'); await session.start(); @@ -110,6 +110,15 @@ describe('ServerStorageSession', () => { expect(status.state()).toBe('online'); }); + it("asks for a token when the stored one is another instance's", async () => { + writeServerAccessToken('t', 'i-2'); + const { session, status } = setup('token'); + + await session.start(); + + expect(status.state()).toBe('unauthorized'); + }); + it('retries a write the server refused while it stays online', async () => { const { session, status } = setup('none'); await session.start(); @@ -213,7 +222,7 @@ describe('ServerStorageSession', () => { ) .mockImplementation(() => undefined); await session.start(); - localStorage.setItem(SERVER_ACCESS_TOKEN_STORAGE_KEY, 't'); + writeServerAccessToken('t', 'i-1'); expect(status.accessToken()).toBeNull(); fetchMock.mockResolvedValue({ diff --git a/libs/services/src/lib/server-storage/server-storage-session.ts b/libs/services/src/lib/server-storage/server-storage-session.ts index 34583ae51..15ed0d7f5 100644 --- a/libs/services/src/lib/server-storage/server-storage-session.ts +++ b/libs/services/src/lib/server-storage/server-storage-session.ts @@ -85,7 +85,10 @@ export class ServerStorageSession { // Also while sign-in is pending: this page may already hold another // page load's writes, so it must mark itself closed on page hide. this.listen(); - if (this.config.instance.auth === 'token' && !readServerAccessToken()) { + if ( + this.config.instance.auth === 'token' && + !readServerAccessToken(this.config.instance.instanceId) + ) { this.status.reportUnauthorized(); return; } diff --git a/libs/services/src/lib/server-storage/server-unsent-writes.ts b/libs/services/src/lib/server-storage/server-unsent-writes.ts index 9e5654eeb..4b8a3a8f3 100644 --- a/libs/services/src/lib/server-storage/server-unsent-writes.ts +++ b/libs/services/src/lib/server-storage/server-unsent-writes.ts @@ -65,7 +65,11 @@ export function claimCache(local: UserDataStorage, instanceId: string): void { } /** Saved edits: JSON drops an undefined side, which reads back as such. */ -type Steps = readonly { readonly before: unknown; readonly after: unknown }[]; +type Steps = readonly { + readonly before: unknown; + readonly after: unknown; + readonly yields?: true; +}[]; export interface UnsentWrite { readonly namespace: ServerUserStateNamespace; @@ -91,15 +95,24 @@ function readSteps(stored: unknown): Steps { return Array.isArray(stored) ? stored .filter((step) => step && typeof step === 'object') - .map((step: { before?: unknown; after?: unknown }) => ({ - before: step.before, - after: step.after, - })) + .map( + (step: { + before?: unknown; + after?: unknown; + yields?: unknown; + }) => ({ + before: step.before, + after: step.after, + ...(step.yields === true + ? { yields: true as const } + : {}), + }) + ) : []; } const saveSteps = (steps: Steps) => - steps.map(({ before, after }) => ({ before, after })); + steps.map(({ before, after, yields }) => ({ before, after, yields })); /** * Records `page` as the tab's current page load and returns the one diff --git a/libs/services/src/lib/server-storage/server-user-data-adopt.ts b/libs/services/src/lib/server-storage/server-user-data-adopt.ts index 617576586..4baf7a20f 100644 --- a/libs/services/src/lib/server-storage/server-user-data-adopt.ts +++ b/libs/services/src/lib/server-storage/server-user-data-adopt.ts @@ -2,16 +2,17 @@ import type { ServerUserStateNamespace } from '@iptvnator/shared/interfaces'; import { behind, type EditStep, - uncertain, UNKNOWN_TAG, + yielding, } from './server-user-data-entries'; import { replaySteps } from './server-user-data-edit'; import type { UserDataWriteQueue } from './server-user-data-queue'; /** * A closed tab's saved edits, ahead of this tab's own (which may have been - * made on them): sent with them, replayed first on a conflict. Returns the - * value to show. + * made on them): sent with them, replayed first on a conflict, and only + * where the entry is still as that tab saw it (`yields`): a change made + * since, here or elsewhere, is newer. Returns the value to show. */ export function adoptEdits( writes: UserDataWriteQueue, @@ -22,7 +23,7 @@ export function adoptEdits( foreign: boolean ): unknown { const key = `${namespace}|${playlistId}`; - const adopted = uncertain(steps); + const adopted = yielding(steps); const queued = writes.isQueued(key) ? writes.unsent(key) : undefined; // Without a server version this page read, the write must conflict and // be replayed on the server's value, never overwrite it blindly. @@ -42,7 +43,7 @@ export function adoptEdits( }); return value; } - const value = replaySteps(current, steps); + const value = replaySteps(current, adopted); writes.queue(namespace, playlistId, value, value, { foreign: unknown }); const entry = writes.unsent(key); if (entry) { diff --git a/libs/services/src/lib/server-storage/server-user-data-edit.spec.ts b/libs/services/src/lib/server-storage/server-user-data-edit.spec.ts index a120193f0..9e2cc4a93 100644 --- a/libs/services/src/lib/server-storage/server-user-data-edit.spec.ts +++ b/libs/services/src/lib/server-storage/server-user-data-edit.spec.ts @@ -60,6 +60,42 @@ describe('applyEdit', () => { }); }); +describe('applyEdit of a gone page (yields)', () => { + it('changes only what is still as that page saw it', () => { + // Map keys: `a` changed since, `b` did not. + expect( + applyEdit({ a: 3, b: 1 }, { a: 1, b: 1 }, { a: 2, b: 2 }, true) + ).toEqual({ a: 3, b: 2 }); + // List items by key: a changed item keeps its newer state. + const item = (seconds: number) => ({ + contentType: 'vod', + contentXtreamId: 1, + positionSeconds: seconds, + }); + expect(applyEdit([item(30)], [item(10)], [item(20)], true)).toEqual([ + item(30), + ]); + expect(applyEdit([item(10)], [item(10)], [item(20)], true)).toEqual([ + item(20), + ]); + // Adds and removes still apply where nothing changed. + expect(applyEdit(['a', 'b'], ['a', 'b'], ['b', 'c'], true)).toEqual([ + 'b', + 'c', + ]); + }); + + it('keeps a value or entry replaced since', () => { + expect(applyEdit('new', 'old', 'mine', true)).toBe('new'); + expect(applyEdit('old', 'old', 'mine', true)).toBe('mine'); + expect(applyEdit(['x', 'y'], ['x'], undefined, true)).toEqual([ + 'x', + 'y', + ]); + expect(applyEdit(['x'], ['x'], undefined, true)).toBeUndefined(); + }); +}); + describe('withoutDeleted', () => { it('drops deleted playlists and keeps every other id', () => { const read = withoutDeleted( diff --git a/libs/services/src/lib/server-storage/server-user-data-edit.ts b/libs/services/src/lib/server-storage/server-user-data-edit.ts index 96c0a16b2..14da90886 100644 --- a/libs/services/src/lib/server-storage/server-user-data-edit.ts +++ b/libs/services/src/lib/server-storage/server-user-data-edit.ts @@ -49,15 +49,18 @@ export function refreshedNamespace( * Applies one local edit (`before` → `after`, as the user made it against * an older or empty cache) to the server's current value: list items the * edit added or changed are set, items it removed are dropped, everything - * else the server has stays. Maps work the same by key. + * else the server has stays. Maps work the same by key. A `yields` edit + * (a gone page's) changes only what is still as it was `before`. */ export function applyEdit( server: unknown, before: unknown, - after: unknown + after: unknown, + yields = false ): unknown { + const free = (now: unknown, was: unknown) => !yields || same(now, was); if (after === undefined) { - return undefined; + return free(server, before) ? undefined : server; } if ( Array.isArray(after) && @@ -70,14 +73,17 @@ export function applyEdit( const now = new Map(after.map((item) => [itemKey(item), item])); const result = (server ?? []).filter((item) => { const key = itemKey(item); - return !(was.has(key) && !now.has(key)); + return !(was.has(key) && !now.has(key) && free(item, was.get(key))); }); for (const [key, item] of now) { - if (JSON.stringify(was.get(key)) === JSON.stringify(item)) { - // Untouched by the edit: the server's state of it wins. + // Untouched by the edit: the server's state of it wins. + if (same(was.get(key), item)) { continue; } const index = result.findIndex((entry) => itemKey(entry) === key); + if (!free(index < 0 ? undefined : result[index], was.get(key))) { + continue; + } if (index < 0) { result.push(item); } else { @@ -87,20 +93,25 @@ export function applyEdit( return result; } if (isMap(after) && isMapOrMissing(server) && isMapOrMissing(before)) { + const was = before ?? {}; const result: Record = { ...(server ?? {}) }; - for (const key of Object.keys(before ?? {})) { - if (!(key in after)) { + for (const key of Object.keys(was)) { + if (!(key in after) && free(result[key], was[key])) { delete result[key]; } } for (const [key, value] of Object.entries(after)) { - if (JSON.stringify((before ?? {})[key]) !== JSON.stringify(value)) { + if (!same(was[key], value) && free(result[key], was[key])) { result[key] = value; } } return result; } - return after; + return free(server, before) ? after : server; +} + +function same(a: unknown, b: unknown): boolean { + return JSON.stringify(a) === JSON.stringify(b); } function isMap(value: unknown): value is Record { @@ -120,11 +131,11 @@ function isListOrMissing(value: unknown): value is unknown[] | undefined { /** Edits replayed on `server` in order. */ export function replaySteps( server: unknown, - steps: readonly { readonly before: unknown; readonly after: unknown }[] + steps: readonly EditStep[] ): unknown { let value = server; for (const step of steps) { - value = applyEdit(value, step.before, step.after); + value = applyEdit(value, step.before, step.after, step.yields); } return value; } diff --git a/libs/services/src/lib/server-storage/server-user-data-entries.ts b/libs/services/src/lib/server-storage/server-user-data-entries.ts index dfad7526a..794c63d02 100644 --- a/libs/services/src/lib/server-storage/server-user-data-entries.ts +++ b/libs/services/src/lib/server-storage/server-user-data-entries.ts @@ -33,6 +33,11 @@ export interface EditStep { * it may have landed, so a later edit never joins it. */ readonly sent?: number; + /** + * Made on a page that is gone (a closed tab's saved edit): it changes + * only what is still as that page saw it, never a change made since. + */ + readonly yields?: true; } /** A travelling write's steps, marked as carried by it. */ @@ -45,6 +50,11 @@ export function uncertain(steps: readonly EditStep[]): EditStep[] { return steps.map((step) => ({ ...step, sent: 0 })); } +/** A gone page's steps: uncertain, and giving way to later changes. */ +export function yielding(steps: readonly EditStep[]): EditStep[] { + return steps.map((step) => ({ ...step, sent: 0, yields: true })); +} + /** * `steps` plus the edit `before` → `after`: joined with the last step when * it continues it, kept apart otherwise. @@ -58,6 +68,7 @@ export function addStep( if ( last && last.sent === undefined && + !last.yields && JSON.stringify(last.after) === JSON.stringify(before) ) { return [...steps.slice(0, -1), { before: last.before, after }]; diff --git a/libs/services/src/lib/server-storage/server-user-data-sync.spec.ts b/libs/services/src/lib/server-storage/server-user-data-sync.spec.ts index abab5aea2..313592641 100644 --- a/libs/services/src/lib/server-storage/server-user-data-sync.spec.ts +++ b/libs/services/src/lib/server-storage/server-user-data-sync.spec.ts @@ -1,118 +1,7 @@ import { ServerApiError } from './server-api-client'; import { UNCONFIRMED_TTL_MS } from './server-unsent-writes'; import { ServerUserDataStorage } from './server-user-data-storage'; - -function memoryStorage() { - const values = new Map(); - return { - values, - getItem: (key: string) => values.get(key) ?? null, - setItem: (key: string, value: string) => void values.set(key, value), - removeItem: (key: string) => void values.delete(key), - keys: () => [...values.keys()], - }; -} - -/** A server that keeps entries, tags them by content and refuses stale tags. */ -function fakeServer(initial: Record> = {}) { - const state: Record> = { - 'xtream-favorites': {}, - 'xtream-recent-items': {}, - 'xtream-collection-items': {}, - 'xtream-playback-positions': {}, - ...structuredClone(initial), - }; - /** Playlists deleted on the server: their user state is refused. */ - const deleted = new Set(); - const tagOf = (value: unknown) => - value === undefined - ? null - : `t${JSON.stringify(value).replace(/"/g, "'")}`; - const answer = (body: unknown) => - ({ json: async () => body }) as unknown as Response; - const write = ( - path: string, - headers: Record, - value?: unknown - ) => { - const [, , namespace, playlistId] = path.split('/'); - if (value !== undefined && deleted.has(playlistId)) { - throw new ServerApiError(404, 'playlist_missing', 'gone'); - } - const current = state[namespace][playlistId]; - const expected = - headers['If-None-Match'] === '*' - ? null - : headers['If-Match']?.replace(/^"|"$/g, ''); - if (expected !== undefined && expected !== tagOf(current)) { - throw new ServerApiError(412, 'conflict', 'changed', { - ...(current === undefined ? {} : { value: current }), - tag: tagOf(current), - }); - } - if (value === undefined) { - delete state[namespace][playlistId]; - } else { - state[namespace][playlistId] = value; - } - return answer({ tag: tagOf(value) }); - }; - const api = { - getJson: jest.fn(async () => { - const tags: Record> = {}; - for (const [namespace, entries] of Object.entries(state)) { - tags[namespace] = Object.fromEntries( - Object.entries(entries).map(([id, value]) => [ - id, - tagOf(value) as string, - ]) - ); - } - return { entries: structuredClone(state), tags }; - }), - sendJson: jest.fn( - async ( - _method: string, - path: string, - body: { value: unknown }, - headers: Record = {} - ) => write(path, headers, body.value) - ), - request: jest.fn( - async (path: string, init: { headers?: Record }) => - write(path, init.headers ?? {}) - ), - readJson: jest.fn((response: Response) => response.json()), - sendKeepalive: jest.fn( - async ( - _method: string, - path: string, - body?: { value: unknown }, - headers: Record = {} - ) => { - try { - const response = write(path, headers, body?.value); - const { tag } = await response.json(); - return { status: 'saved' as const, tag }; - } catch (error) { - if ( - error instanceof ServerApiError && - error.code === 'playlist_missing' - ) { - return { status: 'gone' as const }; - } - return error instanceof ServerApiError && error.current - ? { - status: 'conflict' as const, - current: error.current, - } - : { status: 'failed' as const }; - } - } - ), - }; - return { state, api, deleted }; -} +import { fakeServer, memoryStorage } from './server-user-data.test-helpers'; const favorites = (storage: ServerUserDataStorage) => JSON.parse(storage.getItem('xtream-favorites') ?? '{}'); @@ -792,6 +681,69 @@ describe('ServerUserDataStorage with other devices', () => { expect(server.state['xtream-playback-positions']['p1']).toEqual(at(30)); }); + it("keeps progress this tab saved over a closed tab's older edit", async () => { + const at = (seconds: number) => [ + { + contentType: 'vod', + contentXtreamId: 1, + positionSeconds: seconds, + }, + ]; + const server = fakeServer({ + 'xtream-playback-positions': { p1: at(10) }, + }); + const shared = memoryStorage(); + const clock = jest.spyOn(Date, 'now').mockReturnValue(1_000); + const closed = new ServerUserDataStorage( + { + ...server.api, + sendJson: jest.fn(() => + Promise.reject(new ServerApiError(0, 'network', 'offline')) + ), + } as never, + shared, + jest.fn(), + 'default', + memoryStorage() + ); + await closed.hydrate(); + closed.setItem( + 'xtream-playback-positions', + JSON.stringify({ p1: at(20) }) + ); + await closed.flush(); + closed.close(); + + // This tab saves newer progress before it adopts the closed one. + const tab = new ServerUserDataStorage( + server.api as never, + shared, + jest.fn(), + 'default', + memoryStorage() + ); + await tab.hydrate(); + tab.setItem( + 'xtream-playback-positions', + JSON.stringify({ p1: at(30) }) + ); + // Sent on the server's value: the closed tab's write is unconfirmed. + await tab.flush(); + await tab.flush(); + expect(server.state['xtream-playback-positions']['p1']).toEqual(at(30)); + + clock.mockReturnValue(1_000 + UNCONFIRMED_TTL_MS + 1); + await tab.hydrate(); + await tab.flush(); + await tab.flush(); + clock.mockRestore(); + + expect(server.state['xtream-playback-positions']['p1']).toEqual(at(30)); + expect( + JSON.parse(tab.getItem('xtream-playback-positions') ?? '{}')['p1'] + ).toEqual(at(30)); + }); + it("keeps the last page load's saved writes when storage refuses the copy", async () => { const server = fakeServer({ 'xtream-favorites': { p1: ['S'] } }); const local = memoryStorage(); diff --git a/libs/services/src/lib/server-storage/server-user-data.test-helpers.ts b/libs/services/src/lib/server-storage/server-user-data.test-helpers.ts new file mode 100644 index 000000000..f946b543e --- /dev/null +++ b/libs/services/src/lib/server-storage/server-user-data.test-helpers.ts @@ -0,0 +1,116 @@ +import { ServerApiError } from './server-api-client'; + +/** Browser storage in memory, with the `keys` listing tabs use. */ +export function memoryStorage() { + const values = new Map(); + return { + values, + getItem: (key: string) => values.get(key) ?? null, + setItem: (key: string, value: string) => void values.set(key, value), + removeItem: (key: string) => void values.delete(key), + keys: () => [...values.keys()], + }; +} + +/** A server that keeps entries, tags them by content and refuses stale tags. */ +export function fakeServer( + initial: Record> = {} +) { + const state: Record> = { + 'xtream-favorites': {}, + 'xtream-recent-items': {}, + 'xtream-collection-items': {}, + 'xtream-playback-positions': {}, + ...structuredClone(initial), + }; + /** Playlists deleted on the server: their user state is refused. */ + const deleted = new Set(); + const tagOf = (value: unknown) => + value === undefined + ? null + : `t${JSON.stringify(value).replace(/"/g, "'")}`; + const answer = (body: unknown) => + ({ json: async () => body }) as unknown as Response; + const write = ( + path: string, + headers: Record, + value?: unknown + ) => { + const [, , namespace, playlistId] = path.split('/'); + if (value !== undefined && deleted.has(playlistId)) { + throw new ServerApiError(404, 'playlist_missing', 'gone'); + } + const current = state[namespace][playlistId]; + const expected = + headers['If-None-Match'] === '*' + ? null + : headers['If-Match']?.replace(/^"|"$/g, ''); + if (expected !== undefined && expected !== tagOf(current)) { + throw new ServerApiError(412, 'conflict', 'changed', { + ...(current === undefined ? {} : { value: current }), + tag: tagOf(current), + }); + } + if (value === undefined) { + delete state[namespace][playlistId]; + } else { + state[namespace][playlistId] = value; + } + return answer({ tag: tagOf(value) }); + }; + const api = { + getJson: jest.fn(async () => { + const tags: Record> = {}; + for (const [namespace, entries] of Object.entries(state)) { + tags[namespace] = Object.fromEntries( + Object.entries(entries).map(([id, value]) => [ + id, + tagOf(value) as string, + ]) + ); + } + return { entries: structuredClone(state), tags }; + }), + sendJson: jest.fn( + async ( + _method: string, + path: string, + body: { value: unknown }, + headers: Record = {} + ) => write(path, headers, body.value) + ), + request: jest.fn( + async (path: string, init: { headers?: Record }) => + write(path, init.headers ?? {}) + ), + readJson: jest.fn((response: Response) => response.json()), + sendKeepalive: jest.fn( + async ( + _method: string, + path: string, + body?: { value: unknown }, + headers: Record = {} + ) => { + try { + const response = write(path, headers, body?.value); + const { tag } = await response.json(); + return { status: 'saved' as const, tag }; + } catch (error) { + if ( + error instanceof ServerApiError && + error.code === 'playlist_missing' + ) { + return { status: 'gone' as const }; + } + return error instanceof ServerApiError && error.current + ? { + status: 'conflict' as const, + current: error.current, + } + : { status: 'failed' as const }; + } + } + ), + }; + return { state, api, deleted }; +}