From bda2ebcf5ccd3e15526e0d372eb465a6b855e93a Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Tue, 29 Sep 2026 20:02:54 -0400 Subject: [PATCH 1/2] ENG-2335 Add a shared check for whether an imported node is published to a group --- .../database/src/lib/__tests__/groups.test.ts | 312 ++++++++++++++++++ packages/database/src/lib/groups.ts | 143 ++++++++ 2 files changed, 455 insertions(+) create mode 100644 packages/database/src/lib/__tests__/groups.test.ts diff --git a/packages/database/src/lib/__tests__/groups.test.ts b/packages/database/src/lib/__tests__/groups.test.ts new file mode 100644 index 0000000000..3e4dbd6f06 --- /dev/null +++ b/packages/database/src/lib/__tests__/groups.test.ts @@ -0,0 +1,312 @@ +import { describe, expect, it, vi } from "vitest"; +import type { DGSupabaseClient } from "../client"; +import { getPublishedGroupIdsByRid } from "../groups"; + +type Row = Record; + +const readPath = (row: Row, path: string): unknown => + path + .split(".") + .reduce( + (value, key) => + value && typeof value === "object" ? (value as Row)[key] : undefined, + row, + ); + +// Applies `.in()` and `.not(…, "is", null)` like PostgREST; records `.in()` calls. +const makeClient = (tables: Record) => { + const inCalls: { table: string; column: string; values: unknown[] }[] = []; + const from = vi.fn((table: string) => { + const filters: ((row: Row) => boolean)[] = []; + const builder = { + select: vi.fn(() => builder), + in: vi.fn((column: string, values: unknown[]) => { + inCalls.push({ table, column, values }); + filters.push((row) => values.includes(readPath(row, column))); + return builder; + }), + not: vi.fn((column: string) => { + filters.push((row) => readPath(row, column) != null); + return builder; + }), + then: ( + resolve: (value: { data: Row[]; error: null }) => unknown, + reject?: (reason: unknown) => unknown, + ) => + Promise.resolve({ + data: (tables[table] ?? []).filter((row) => + filters.every((filter) => filter(row)), + ), + error: null, + }).then(resolve, reject), + }; + return builder; + }); + return { client: { from } as unknown as DGSupabaseClient, from, inCalls }; +}; + +const G1 = "group-1"; +const G2 = "group-2"; +const obsidianUrl = "obsidian:vault-a"; +const roamUrl = "https://roamresearch.com/#/app/graph-b"; +const obsidianRid = "orn:obsidian.note:vault-a/node-1"; +const roamRid = `${roamUrl}/uid-1`; + +const obsidianSpace = { spaceId: 20, url: obsidianUrl }; +const roamSpace = { spaceId: 30, url: roamUrl }; + +const sharing = ({ + space, + groupId, + permissions, +}: { + space: { spaceId: number; url: string }; + groupId: string; + permissions: string | null; +}): Row => ({ + space_id: space.spaceId, + group_id: groupId, + sharing_permissions: permissions, + Space: { url: space.url }, +}); + +const grant = ({ + groupId, + space, + sourceLocalId, +}: { + groupId: string; + space: { spaceId: number }; + sourceLocalId: string; +}): Row => ({ + account_uid: groupId, + space_id: space.spaceId, + source_local_id: sourceLocalId, +}); + +describe("getPublishedGroupIdsByRid", () => { + it("returns only the group the node is published to under partial space access", async () => { + const { client } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "partial" }), + sharing({ space: obsidianSpace, groupId: G2, permissions: "partial" }), + ], + ResourceAccess: [ + grant({ groupId: G1, space: obsidianSpace, sourceLocalId: "node-1" }), + grant({ groupId: G2, space: obsidianSpace, sourceLocalId: "node-2" }), + ], + }); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid] }), + ).resolves.toEqual({ [obsidianRid]: [G1] }); + }); + + it("returns every group the node is published to", async () => { + const { client } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: roamSpace, groupId: G1, permissions: "partial" }), + sharing({ space: roamSpace, groupId: G2, permissions: "partial" }), + ], + ResourceAccess: [ + grant({ groupId: G1, space: roamSpace, sourceLocalId: "uid-1" }), + grant({ groupId: G2, space: roamSpace, sourceLocalId: "uid-1" }), + ], + }); + + const result = await getPublishedGroupIdsByRid({ client, rids: [roamRid] }); + + expect(result[roamRid]?.sort()).toEqual([G1, G2]); + }); + + it("returns no groups for a node published to none of them", async () => { + const { client } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "partial" }), + ], + ResourceAccess: [ + grant({ groupId: G1, space: obsidianSpace, sourceLocalId: "node-2" }), + ], + }); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid] }), + ).resolves.toEqual({ [obsidianRid]: [] }); + }); + + it("counts a group with reader access to the space without a ResourceAccess grant", async () => { + const { client, from } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "reader" }), + ], + }); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid] }), + ).resolves.toEqual({ [obsidianRid]: [G1] }); + expect(from).not.toHaveBeenCalledWith("ResourceAccess"); + }); + + it("ignores a group the space belongs to but is not shared with", async () => { + const { client } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "partial" }), + sharing({ space: obsidianSpace, groupId: G2, permissions: null }), + ], + ResourceAccess: [ + grant({ groupId: G1, space: obsidianSpace, sourceLocalId: "node-1" }), + grant({ groupId: G2, space: obsidianSpace, sourceLocalId: "node-1" }), + ], + }); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid] }), + ).resolves.toEqual({ [obsidianRid]: [G1] }); + }); + + it("matches grants on both the space and the local id", async () => { + const { client } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "partial" }), + sharing({ space: roamSpace, groupId: G2, permissions: "partial" }), + ], + ResourceAccess: [ + grant({ groupId: G1, space: obsidianSpace, sourceLocalId: "node-1" }), + // Same local id as the Obsidian node, in the other space. + grant({ groupId: G2, space: roamSpace, sourceLocalId: "node-1" }), + grant({ groupId: G2, space: roamSpace, sourceLocalId: "uid-1" }), + ], + }); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid, roamRid] }), + ).resolves.toEqual({ [obsidianRid]: [G1], [roamRid]: [G2] }); + }); + + it("returns no groups for a node whose space is not shared with the user's groups", async () => { + const { client, from } = makeClient({ my_pseudo_accounts: [] }); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid] }), + ).resolves.toEqual({ [obsidianRid]: [] }); + expect(from).not.toHaveBeenCalledWith("ResourceAccess"); + }); + + it("queries local ids from all spaces in shared batches", async () => { + const localIds = Array.from({ length: 30 }, (_, i) => `node-${i}`); + const rids = [ + ...localIds.map((id) => `orn:obsidian.note:vault-a/${id}`), + ...localIds.map((id) => `${roamUrl}/uid-${id}`), + ]; + const { client, inCalls } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "partial" }), + sharing({ space: roamSpace, groupId: G1, permissions: "partial" }), + ], + }); + + await getPublishedGroupIdsByRid({ client, rids }); + + expect( + inCalls + .filter(({ column }) => column === "source_local_id") + .map(({ values }) => values.length), + ).toEqual([50, 10]); + }); + + it("shrinks batches so local ids × partial groups stay under max_rows", async () => { + const groupIds = Array.from({ length: 30 }, (_, i) => `group-${i}`); + const localIds = Array.from({ length: 100 }, (_, i) => `node-${i}`); + const { client, inCalls } = makeClient({ + my_pseudo_accounts: groupIds.map((groupId) => + sharing({ space: obsidianSpace, groupId, permissions: "partial" }), + ), + ResourceAccess: localIds.flatMap((sourceLocalId) => + groupIds.map((groupId) => + grant({ groupId, space: obsidianSpace, sourceLocalId }), + ), + ), + }); + + const result = await getPublishedGroupIdsByRid({ + client, + rids: localIds.map((id) => `orn:obsidian.note:vault-a/${id}`), + }); + + expect( + inCalls + .filter(({ column }) => column === "source_local_id") + .map(({ values }) => values.length), + ).toEqual([33, 33, 33, 1]); + expect(result["orn:obsidian.note:vault-a/node-99"]).toEqual(groupIds); + }); + + it("lists a group once for a rid repeated across batches", async () => { + const rids = Array.from( + { length: 60 }, + (_, i) => `orn:obsidian.note:vault-a/node-${i}`, + ); + rids[55] = obsidianRid; + const { client } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "partial" }), + ], + ResourceAccess: [ + grant({ groupId: G1, space: obsidianSpace, sourceLocalId: "node-1" }), + ], + }); + + const result = await getPublishedGroupIdsByRid({ client, rids }); + + expect(result[obsidianRid]).toEqual([G1]); + }); + + it("counts a grant only for a group with partial access to that space", async () => { + const G3 = "group-3"; + const { client } = makeClient({ + my_pseudo_accounts: [ + sharing({ space: obsidianSpace, groupId: G1, permissions: "reader" }), + sharing({ space: roamSpace, groupId: G1, permissions: "partial" }), + sharing({ space: roamSpace, groupId: G3, permissions: "partial" }), + ], + ResourceAccess: [ + // G1 already reads the whole Obsidian space. + grant({ groupId: G1, space: obsidianSpace, sourceLocalId: "node-1" }), + // G3 is not shared with the Obsidian space. + grant({ groupId: G3, space: obsidianSpace, sourceLocalId: "node-1" }), + grant({ groupId: G3, space: roamSpace, sourceLocalId: "uid-1" }), + ], + }); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid, roamRid] }), + ).resolves.toEqual({ [obsidianRid]: [G1], [roamRid]: [G3] }); + }); + + it("skips the database when there are no rids", async () => { + const { client, from } = makeClient({}); + + await expect( + getPublishedGroupIdsByRid({ client, rids: [] }), + ).resolves.toEqual({}); + expect(from).not.toHaveBeenCalled(); + }); + + it("throws the query error", async () => { + const error = { message: "boom" }; + const builder = { + select: vi.fn(() => builder), + in: vi.fn(() => builder), + not: vi.fn(() => builder), + then: (resolve: (value: unknown) => unknown) => + Promise.resolve({ data: null, error }).then(resolve), + }; + const client = { + from: vi.fn(() => builder), + } as unknown as DGSupabaseClient; + + await expect( + getPublishedGroupIdsByRid({ client, rids: [obsidianRid] }), + ).rejects.toEqual(error); + }); +}); diff --git a/packages/database/src/lib/groups.ts b/packages/database/src/lib/groups.ts index 66848d6171..14597bc2f3 100644 --- a/packages/database/src/lib/groups.ts +++ b/packages/database/src/lib/groups.ts @@ -1,6 +1,7 @@ import type { DGSupabaseClient } from "./client"; import type { Tables, Enums } from "../dbTypes"; import { isIgnorableUpsertError } from "./contextFunctions"; +import { ridToSpaceUriAndLocalId } from "./rid"; export type MyGroup = { id: string; @@ -103,3 +104,145 @@ export const ensurePartialSpaceAccess = async ({ }); return { existing: existingAccessByGroupId }; }; + +// `.in()` filters go in the GET URL, which caps the local ids per batch. +const LOCAL_ID_BATCH_SIZE = 50; +// `max_rows` in supabase/config.toml. Past it, rows are dropped silently, so a +// batch's rows (local ids × groups) must stay under it. +const MAX_ROWS = 1000; + +type SpaceSharing = { + spaceId: number; + fullAccessGroupIds: string[]; + partialAccessGroupIds: string[]; +}; + +// Spaces share with groups by membership. Unlike SpaceAccess, whose select +// policy needs `reader`, this view shows `partial` pairs. +const getSpaceSharingByUrl = async ({ + client, + spaceUris, +}: { + client: DGSupabaseClient; + spaceUris: string[]; +}): Promise> => { + const { data, error } = await client + .from("my_pseudo_accounts") + .select("space_id, group_id, sharing_permissions, Space!inner(url)") + .in("Space.url", spaceUris) + .not("sharing_permissions", "is", null); + if (error) throw error; + const sharingByUrl = new Map(); + for (const { + space_id: spaceId, + group_id: groupId, + sharing_permissions: permissions, + Space: { url }, + } of data) { + if (spaceId === null || groupId === null || permissions === null) continue; + let sharing = sharingByUrl.get(url); + if (!sharing) { + sharing = { spaceId, fullAccessGroupIds: [], partialAccessGroupIds: [] }; + sharingByUrl.set(url, sharing); + } + if (permissions === "partial") sharing.partialAccessGroupIds.push(groupId); + else sharing.fullAccessGroupIds.push(groupId); + } + return sharingByUrl; +}; + +const getGrantedGroupIdsByRid = async ({ + client, + rids, + sharingByUrl, +}: { + client: DGSupabaseClient; + rids: string[]; + sharingByUrl: Map; +}): Promise> => { + const partialRids = [...new Set(rids)].flatMap((rid) => { + const { spaceUri, sourceLocalId } = ridToSpaceUriAndLocalId(rid); + const sharing = sharingByUrl.get(spaceUri); + return sharing?.partialAccessGroupIds.length + ? [{ rid, sourceLocalId, sharing }] + : []; + }); + const groupCount = new Set( + partialRids.flatMap(({ sharing }) => sharing.partialAccessGroupIds), + ).size; + const batchSize = Math.max( + 1, + Math.min(LOCAL_ID_BATCH_SIZE, Math.floor(MAX_ROWS / groupCount)), + ); + const groupIdsByRid = new Map(); + for (let start = 0; start < partialRids.length; start += batchSize) { + const batch = partialRids.slice(start, start + batchSize); + const { data, error } = await client + .from("ResourceAccess") + .select("account_uid, space_id, source_local_id") + .in("space_id", [...new Set(batch.map(({ sharing }) => sharing.spaceId))]) + .in("account_uid", [ + ...new Set( + batch.flatMap(({ sharing }) => sharing.partialAccessGroupIds), + ), + ]) + .in("source_local_id", [ + ...new Set(batch.map(({ sourceLocalId }) => sourceLocalId)), + ]); + if (error) throw error; + // Independent filters: a row can pair a local id with another space of + // the batch, or with a group that is not `partial` there. + const nodeByResource = new Map( + batch.map((node) => [ + `${node.sharing.spaceId}/${node.sourceLocalId}`, + node, + ]), + ); + for (const { account_uid, space_id, source_local_id } of data) { + const node = nodeByResource.get(`${space_id}/${source_local_id}`); + if (!node?.sharing.partialAccessGroupIds.includes(account_uid)) continue; + groupIdsByRid.set(node.rid, [ + ...(groupIdsByRid.get(node.rid) ?? []), + account_uid, + ]); + } + } + return groupIdsByRid; +}; + +// A group counts for a node with `reader` access or higher to its space, or +// with `partial` access and a ResourceAccess grant. +export const getPublishedGroupIdsByRid = async ({ + client, + rids, +}: { + client: DGSupabaseClient; + rids: string[]; +}): Promise> => { + const result: Record = Object.fromEntries( + rids.map((rid) => [rid, []]), + ); + if (rids.length === 0) return result; + const spaceUriByRid = new Map( + rids.map((rid) => [rid, ridToSpaceUriAndLocalId(rid).spaceUri]), + ); + const sharingByUrl = await getSpaceSharingByUrl({ + client, + spaceUris: [...new Set(spaceUriByRid.values())], + }); + const grantedGroupIdsByRid = await getGrantedGroupIdsByRid({ + client, + rids, + sharingByUrl, + }); + + for (const [rid, spaceUri] of spaceUriByRid) { + const sharing = sharingByUrl.get(spaceUri); + if (!sharing) continue; + result[rid] = [ + ...sharing.fullAccessGroupIds, + ...(grantedGroupIdsByRid.get(rid) ?? []), + ]; + } + return result; +}; From 0199fda456b5b399372bf1e50bae9e51d9cc7d3a Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Sun, 4 Oct 2026 11:08:22 -0400 Subject: [PATCH 2/2] Correction found against a live check --- packages/database/src/lib/groups.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/database/src/lib/groups.ts b/packages/database/src/lib/groups.ts index 14597bc2f3..0df069399a 100644 --- a/packages/database/src/lib/groups.ts +++ b/packages/database/src/lib/groups.ts @@ -126,9 +126,12 @@ const getSpaceSharingByUrl = async ({ client: DGSupabaseClient; spaceUris: string[]; }): Promise> => { + // The view also reaches Space through LocalAccess; PostgREST needs the join named. const { data, error } = await client .from("my_pseudo_accounts") - .select("space_id, group_id, sharing_permissions, Space!inner(url)") + .select( + "space_id, group_id, sharing_permissions, Space!SpaceAccess_space_id_fkey!inner(url)", + ) .in("Space.url", spaceUris) .not("sharing_permissions", "is", null); if (error) throw error;