fix(#348 review round-2 F5-F6): index page_access(workspace_id) + test the workspace-cache bust

Both are direct consequences of the round-1 F1 fix (uncaching hasRestrictedPagesInWorkspace): - F5: that EXISTS(SELECT 1 FROM page_access WHERE workspace_id=?) now runs per-request on every whole-workspace list endpoint (global search + suggest, favorites, notifications, recent, created-by), and page_access only had a space_id index → a seq scan in the common zero-restriction case. Added idx_page_access_workspace_id to the perf migration (up + down) so it's an index-only existence probe. - F6: the DomainMiddleware workspace cache invalidation was untested — the int-spec passed `{}` for cacheManager, so bustWorkspaceCache's `del` threw into its own try/catch and never ran. Added a Map-backed cache double with a working del and two tests: updateSetting busts WORKSPACE_SELF_HOSTED; updateSharingSettings busts WORKSPACE_SELF_HOSTED + WORKSPACE_BY_HOST(hostname). A missed/mismatched bust key now fails the suite instead of letting a stale security-relevant workspace row (enforceSso/status) outlive the mutation. Gate: server tsc 0; workspace-repo-update-setting + page-permission-workspace-filter int-specs pass on real Postgres (the new index applies via global-setup). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-05 02:52:02 +03:00
parent 321a0d3229
commit fd42e975b9
2 changed files with 89 additions and 0 deletions
@@ -83,6 +83,17 @@ export async function up(db: Kysely<any>): Promise<void> {
    CREATE INDEX IF NOT EXISTS idx_comments_page_id_id
      ON comments (page_id, id)
  `.execute(db);
+
+  // page_access(workspace_id): #348 made hasRestrictedPagesInWorkspace uncached
+  // (F1 fix), so `EXISTS(SELECT 1 FROM page_access WHERE workspace_id=?)` now runs
+  // per-request on every whole-workspace list endpoint (global search + suggest,
+  // favorites, notifications, recent, created-by). page_access only had a
+  // space_id index → that EXISTS was a seq scan in the common zero-restriction
+  // case. This index makes it an index-only existence probe.
+  await sql`
+    CREATE INDEX IF NOT EXISTS idx_page_access_workspace_id
+      ON page_access (workspace_id)
+  `.execute(db);
 }

 export async function down(db: Kysely<any>): Promise<void> {
@@ -92,6 +103,7 @@ export async function down(db: Kysely<any>): Promise<void> {
  await sql`DROP INDEX IF EXISTS idx_groups_name_trgm`.execute(db);
  await sql`DROP INDEX IF EXISTS idx_page_history_page_id`.execute(db);
  await sql`DROP INDEX IF EXISTS idx_comments_page_id_id`.execute(db);
+  await sql`DROP INDEX IF EXISTS idx_page_access_workspace_id`.execute(db);

  // Restore the original two-arg (dictionary-named) f_unaccent body.
  await sql`
@@ -1,7 +1,25 @@
 import { Kysely } from 'kysely';
 import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo';
+import { CacheKey } from 'src/common/helpers/cache-keys';
 import { getTestDb, destroyTestDb, createWorkspace } from './db';

+// A minimal Map-backed cache double with a working `del` (the previous `{}` stub
+// made bustWorkspaceCache's `del` throw into its own try/catch, so the #348
+// invalidation was never actually exercised — review F6).
+function makeCacheDouble() {
+  const store = new Map<string, unknown>();
+  return {
+    store,
+    get: async (k: string) => store.get(k),
+    set: async (k: string, v: unknown) => {
+      store.set(k, v);
+    },
+    del: async (k: string) => {
+      store.delete(k);
+    },
+  };
+}
+
 /**
 * A — WorkspaceRepo.updateSetting jsonb-MERGE (the html-embed kill-switch
 * write-half). Setting a single top-level key must NOT clobber sibling
@@ -60,3 +78,62 @@ describe('WorkspaceRepo.updateSetting (jsonb merge) [integration]', () => {
    expect(updated.settings).toEqual({ htmlEmbed: false });
  });
 });
+
+/**
+ * #348 F6 — the DomainMiddleware workspace cache (WORKSPACE_SELF_HOSTED /
+ * WORKSPACE_BY_HOST, 15s TTL) caches security-relevant fields (enforceSso/
+ * enforceMfa/status). Its correctness rests entirely on bustWorkspaceCache being
+ * called from every mutator. This exercises the real invalidation with a working
+ * cache double (not the {} stub, whose del throws-and-swallows): warm the cache
+ * like DomainMiddleware, mutate, and assert the busted key is gone so a stale
+ * workspace row can't outlive the mutation.
+ */
+describe('WorkspaceRepo bustWorkspaceCache invalidation [integration]', () => {
+  let db: Kysely<any>;
+
+  beforeAll(() => {
+    db = getTestDb();
+  });
+  afterAll(async () => {
+    await destroyTestDb();
+  });
+
+  it('updateSetting busts the self-hosted workspace cache key', async () => {
+    const cache = makeCacheDouble();
+    const repo = new WorkspaceRepo(db as any, cache as any);
+    const ws = await createWorkspace(db, { settings: {} });
+
+    // Warm the cache as DomainMiddleware would (self-hosted key).
+    cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws);
+    expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(true);
+
+    await repo.updateSetting(ws.id, 'htmlEmbed', true);
+
+    // The mutation must have invalidated the cached row.
+    expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false);
+  });
+
+  it('updateSharingSettings busts the by-host workspace cache key too', async () => {
+    const cache = makeCacheDouble();
+    const repo = new WorkspaceRepo(db as any, cache as any);
+    const ws = await createWorkspace(db, { settings: {} });
+    // createWorkspace assigns a unique hostname; read it back for the by-host key.
+    const { hostname } = await db
+      .selectFrom('workspaces')
+      .select(['hostname'])
+      .where('id', '=', ws.id)
+      .executeTakeFirstOrThrow();
+
+    // Warm BOTH keys (self-hosted + by-host); the by-host bust needs the row's
+    // hostname, which the mutator returns from the DB.
+    cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws);
+    cache.store.set(CacheKey.WORKSPACE_BY_HOST(hostname as string), ws);
+
+    await repo.updateSharingSettings(ws.id, 'allowInvite', true);
+
+    expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false);
+    expect(cache.store.has(CacheKey.WORKSPACE_BY_HOST(hostname as string))).toBe(
+      false,
+    );
+  });
+});