Skip to content

Commit 6309030

Browse files
jsscclrjsscclr
authored
fix(dotcom): clean up guest file_state rows when unsharing group-owned files (tldraw#9049)
In order to stop a file lingering in an ex-guest's recents (and continuing to sync to them) after its owner revokes access, this PR fixes the `delete_file_states` trigger so it cleans up both guest records when a **group-owned** file is unshared. Fixes tldraw#9037. Two records make a shared file visible to a guest, and neither was cleaned up on unshare: - **`file_state`** (sync/visit state). The trigger (from `000_seed.sql`) decides which rows belong to "guests" with `OLD."ownerId" != "userId"`. The groups backend (`023_groups.sql`) introduced group-owned files where `ownerId` is `NULL` (per the `ownerId` XOR `owningGroupId` constraint). For those files `NULL != "userId"` evaluates to `NULL`, so the `DELETE` matched nothing and guest `file_state` rows survived — leaving the now-private file syncing to ex-guests. Legacy (user-owned) files were unaffected, which is why this only bites group files. - **`group_file`** (the file link created when a visitor opens a shared file, in their home group — home group id == user id). For groups-migrated users the sidebar file list is built from these links (`TldrawApp.getMyFiles`), so even with `file_state` cleaned up the revoked file's name kept showing in an ex-guest's recent files. Nothing deleted these links on unshare for any ownership model. Migration `034` replaces the function so it cleans up both, for both ownership models: - `file_state`: delete states for anyone who isn't the legacy owner (`IS DISTINCT FROM`, so a `NULL` `ownerId` no longer accidentally protects a guest) or a current member of the owning group (`NOT EXISTS` against `group_user`, a no-op for legacy files); - `group_file`: delete the file's links except the owning group's own row, the legacy owner's home-group link, and links of current owning-group members — the same access rule, with the same NULL-safety. The trigger binding (`file_shared_update`) is unchanged. Note: viewing access itself is enforced at read time (and duplication needs source read access since tldraw#9042) — this PR is about cleaning up the stale per-user records that kept revoked files syncing and listed. > [!NOTE] > This migration started life as `033` and was amended in place, then renamed to `034` after tldraw#9029's `033_rename_group_admin_role_to_member` merged. The persistent preview DB has the stale `033_fix_unshare_group_file_cleanup` in its ledger, so the `reset-preview-db` label flow needs to run before re-testing on the preview deploy. ### Change type - [x] `bugfix` ### Test plan This bug is only visible on **group-owned** files, so you need the groups flag and two accounts (an owner and a guest). The cleanest way to see the fix is to run the identical flow on current tldraw.com (the bug) and on this PR's preview deploy (the fix), side by side. **Setup (both accounts)** 1. Open the preview deploy at https://pr-9049-preview-deploy.tldraw.com/ and sign in. 2. Turn on the groups flag for each account: go to https://pr-9049-preview-deploy.tldraw.com/admin, search for the email, and enrol it in groups. **Current tldraw.com behaviour (the bug)** Run this flow on production https://tldraw.com/ (where the fix isn't deployed): 1. As the owner, create a group, create a file **in that group**, and enable sharing. 2. As the guest, open the share link — the file appears in their recent files (creates a guest `file_state` and a `group_file` link). 3. As the owner, set sharing to "no access" (`shared = false`). 4. ❌ The file **stays** in the guest's recent files and keeps syncing to them (its name is still visible). This is the regression. **With this PR (the fix)** Repeat the exact same flow on https://pr-9049-preview-deploy.tldraw.com/: 1. Owner creates a group, creates a file in it, enables sharing. 2. Guest opens the share link — file appears in their recent files. 3. Owner sets sharing to "no access". 4. ✅ Both guest records are removed: the file stops syncing to the guest **and** disappears from their recent files. Group members and the owner keep their access and their recents entries. (Sanity check, optional: a **legacy** personal file that's unshared already cleaned up its `file_state` before this PR, but its `group_file` links from migrated guests also linger — so unsharing a personal shared file now also clears the guest's recents entry.) Verified end to end on the local dev stack (two accounts, share → visit → unshare): the guest's `file_state` and `group_file` rows are deleted by the trigger, the guest gets "Invite only" on the file URL, and the file disappears from their recent files; the owner keeps access and their recents entry. - [x] Unit tests `apps/dotcom/zero-cache/delete_file_states.test.ts` is a focused postgres integration test that loads the real shipped function body from migration `034` (rather than a hand-copied duplicate) and covers group-owned unshare, legacy unshare, and a still-shared control — for both `file_state` and `group_file` cleanup. It also runs the original buggy function body as a regression guard, proving the old function left the group guest's state and link behind and the new one removes them. The trigger is `plpgsql`, so it needs a real postgres — the suite is opt-in via `ZERO_CACHE_TEST_POSTGRES_URL` (a **direct** connection string; the suite is also hardened against transaction-pooled connections by schema-qualifying everything it owns and running verbatim SQL under `SET LOCAL search_path` in transactions) and skips cleanly (exit 0) when unset, so CI stays green. `zero-cache` had no vitest project, so this PR also adds `apps/dotcom/zero-cache/vitest.config.ts`. Verified locally: 7/7 pass against the dev postgres; skips with exit 0 without a DB; `yarn typecheck` passes. ### Release notes - Fix tldraw.com files lingering in a guest's recent files (and continuing to sync to them) after the owner revokes access to a group-owned file. ### Code changes | Section | LOC change | | -------------- | ---------- | | Tests | +250 / -0 | | Apps | +56 / -0 | | Config/tooling | +15 / -0 |
1 parent 98e46ae commit 6309030

2 files changed

Lines changed: 338 additions & 0 deletions

File tree

‎apps/dotcom/zero-cache/delete_file_states.test.ts‎

Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,280 @@
1+
import { readFileSync } from 'fs'
2+
import { dirname, join } from 'path'
3+
import { fileURLToPath } from 'url'
4+
import pg from 'pg'
5+
import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'
6+
7+
// Focused integration test for the `delete_file_states` trigger that cleans up
8+
// guest file_state rows and guest home-group file links (group_file rows) when a
9+
// file is unshared (shared: true -> false).
10+
//
11+
// Regression coverage for: unsharing a group-owned file (ownerId NULL,
12+
// owningGroupId set) used to leave guest file_states behind, because the original
13+
// trigger keyed on `OLD."ownerId" != "userId"` and `NULL != x` is NULL in SQL.
14+
// Visiting a shared file also links it into the visitor's home group via a
15+
// group_file row (home group id == user id) — that link is what puts the file in
16+
// their recent files, and nothing cleaned it up either, so the file name lingered
17+
// in an ex-guest's recent files after access was revoked.
18+
// See migration 034_fix_unshare_group_file_cleanup.sql.
19+
//
20+
// This talks to a real postgres (the trigger is plpgsql, so fakes can't exercise
21+
// it). It is opt-in: set ZERO_CACHE_TEST_POSTGRES_URL (local dev stack:
22+
// postgres://user:password@localhost:6543/postgres) to run it. Without a
23+
// connection string the suite is skipped so CI stays green.
24+
//
25+
// Safety: the suite isolates itself in a throwaway schema, and it must stay safe
26+
// even if the URL points at a shared database through a transaction-mode pooler
27+
// (pgbouncer on 6432, Neon pooler). Transaction pooling can hand each statement
28+
// to a different backend session, so session-level `SET search_path` cannot be
29+
// trusted to stick — and unqualified DDL like `DROP TABLE ... CASCADE` would then
30+
// run against `public`. So no statement here relies on session state: everything
31+
// the test owns is schema-qualified explicitly, and the statements that cannot be
32+
// qualified (the shipped migration SQL, executed verbatim, and the UPDATEs whose
33+
// trigger body resolves table names at execution time) run inside a transaction
34+
// with `SET LOCAL search_path` — a transaction is exactly the unit a
35+
// transaction-mode pooler pins to a single backend, and SET LOCAL expires with it.
36+
37+
const DIRNAME = dirname(fileURLToPath(import.meta.url))
38+
const CONNECTION_STRING = process.env.ZERO_CACHE_TEST_POSTGRES_URL
39+
40+
// The real, shipped function body. We load it from the migration file so the test
41+
// exercises exactly what runs in production rather than a hand-copied duplicate.
42+
const FIXED_FUNCTION_SQL = readFileSync(
43+
join(DIRNAME, 'migrations', '034_fix_unshare_group_file_cleanup.sql'),
44+
'utf8'
45+
)
46+
47+
// The original (buggy) function body, kept here so we can prove the bug exists and
48+
// that the fix is what closes it.
49+
const BUGGY_FUNCTION_SQL = `
50+
CREATE OR REPLACE FUNCTION delete_file_states() RETURNS TRIGGER AS $$
51+
BEGIN
52+
IF OLD.shared = TRUE AND NEW.shared = FALSE THEN
53+
DELETE FROM file_state WHERE "fileId" = OLD.id AND OLD."ownerId" != "userId";
54+
END IF;
55+
RETURN NEW;
56+
END;
57+
$$ LANGUAGE plpgsql;
58+
`
59+
60+
const schemaName = `tldraw_test_${process.pid}`
61+
62+
// Minimal schema covering only the tables the trigger reads or writes, plus the
63+
// trigger binding. Recreated before each test (the trigger references the function,
64+
// which we (re)create first). Dropping `file` CASCADE also drops the trigger.
65+
// Fully qualified so it cannot touch `public` no matter what session it runs on.
66+
const SCHEMA_SQL = `
67+
DROP TABLE IF EXISTS "${schemaName}"."file_state", "${schemaName}"."group_file", "${schemaName}"."file", "${schemaName}"."group_user", "${schemaName}"."group" CASCADE;
68+
CREATE TABLE "${schemaName}"."group" (
69+
"id" TEXT PRIMARY KEY
70+
);
71+
CREATE TABLE "${schemaName}"."group_user" (
72+
"userId" TEXT NOT NULL,
73+
"groupId" TEXT NOT NULL,
74+
PRIMARY KEY ("userId", "groupId")
75+
);
76+
CREATE TABLE "${schemaName}"."file" (
77+
"id" TEXT PRIMARY KEY,
78+
"ownerId" TEXT,
79+
"owningGroupId" TEXT,
80+
"shared" BOOLEAN NOT NULL,
81+
-- mirror the production XOR invariant so the test seed stays realistic
82+
CHECK (("ownerId" IS NULL) != ("owningGroupId" IS NULL))
83+
);
84+
CREATE TABLE "${schemaName}"."file_state" (
85+
"userId" TEXT NOT NULL,
86+
"fileId" TEXT NOT NULL,
87+
PRIMARY KEY ("userId", "fileId")
88+
);
89+
CREATE TABLE "${schemaName}"."group_file" (
90+
"fileId" TEXT NOT NULL,
91+
"groupId" TEXT NOT NULL,
92+
PRIMARY KEY ("fileId", "groupId")
93+
);
94+
CREATE TRIGGER file_shared_update
95+
AFTER UPDATE OF shared ON "${schemaName}"."file"
96+
FOR EACH ROW
97+
EXECUTE FUNCTION "${schemaName}".delete_file_states();
98+
`
99+
100+
const describeMaybe = CONNECTION_STRING ? describe : describe.skip
101+
102+
describeMaybe('delete_file_states trigger (unshare cleanup)', () => {
103+
// A single Client, not a Pool. Two reasons: a Pool reaps idle clients (10s
104+
// default) and transparently replaces them, silently resetting session state;
105+
// and inTestSchema's BEGIN/COMMIT must run on one client, while pool.query may
106+
// use a different client per call.
107+
let client: pg.Client
108+
109+
// Runs statements inside one transaction with search_path pinned to the test
110+
// schema. This is for SQL we execute verbatim (the shipped migration and the
111+
// original buggy function) and for statements whose trigger body resolves
112+
// unqualified table names at execution time. SET LOCAL scopes the setting to
113+
// the transaction, so it works through transaction-mode poolers and cannot
114+
// leak to or from other sessions.
115+
async function inTestSchema(...statements: string[]) {
116+
await client.query('BEGIN')
117+
try {
118+
await client.query(`SET LOCAL search_path TO "${schemaName}"`)
119+
for (const sql of statements) {
120+
await client.query(sql)
121+
}
122+
await client.query('COMMIT')
123+
} catch (err) {
124+
await client.query('ROLLBACK')
125+
throw err
126+
}
127+
}
128+
129+
beforeAll(async () => {
130+
client = new pg.Client({ connectionString: CONNECTION_STRING })
131+
await client.connect()
132+
await client.query(`DROP SCHEMA IF EXISTS "${schemaName}" CASCADE`)
133+
await client.query(`CREATE SCHEMA "${schemaName}"`)
134+
// sanity-check that SET LOCAL pins the schema for a whole transaction on
135+
// this connection before any unqualified SQL runs
136+
await client.query('BEGIN')
137+
await client.query(`SET LOCAL search_path TO "${schemaName}"`)
138+
const res = await client.query<{ schema: string }>('SELECT current_schema() AS schema')
139+
await client.query('COMMIT')
140+
if (res.rows[0]?.schema !== schemaName) {
141+
throw new Error(
142+
`SET LOCAL search_path did not hold within a transaction (current_schema() is ` +
143+
`"${res.rows[0]?.schema}", expected "${schemaName}"). Refusing to run unqualified ` +
144+
`DDL against this connection.`
145+
)
146+
}
147+
})
148+
149+
afterAll(async () => {
150+
if (!client) return
151+
await client.query(`DROP SCHEMA IF EXISTS "${schemaName}" CASCADE`)
152+
await client.end()
153+
})
154+
155+
async function seed() {
156+
// group g1 with an owner and a member; guest is NOT a member
157+
await client.query(`INSERT INTO "${schemaName}"."group" ("id") VALUES ('g1')`)
158+
await client.query(
159+
`INSERT INTO "${schemaName}"."group_user" ("userId", "groupId") VALUES ('uOwner', 'g1'), ('uMember', 'g1')`
160+
)
161+
162+
// group-owned shared file: ownerId NULL, owningGroupId set
163+
await client.query(
164+
`INSERT INTO "${schemaName}"."file" ("id", "ownerId", "owningGroupId", "shared") VALUES ('fGroup', NULL, 'g1', true)`
165+
)
166+
// legacy user-owned shared file: ownerId set, owningGroupId NULL
167+
await client.query(
168+
`INSERT INTO "${schemaName}"."file" ("id", "ownerId", "owningGroupId", "shared") VALUES ('fLegacy', 'uOwner', NULL, true)`
169+
)
170+
// a shared file we will NOT unshare, as a control
171+
await client.query(
172+
`INSERT INTO "${schemaName}"."file" ("id", "ownerId", "owningGroupId", "shared") VALUES ('fControl', 'uOwner', NULL, true)`
173+
)
174+
175+
// everyone has a file_state on every file
176+
for (const fileId of ['fGroup', 'fLegacy', 'fControl']) {
177+
await client.query(
178+
`INSERT INTO "${schemaName}"."file_state" ("userId", "fileId") VALUES ('uOwner', $1), ('uMember', $1), ('uGuest', $1)`,
179+
[fileId]
180+
)
181+
}
182+
183+
// group_file rows: the owning group's own row for fGroup, plus home-group
184+
// file links (home group id == user id) created by visiting a shared file
185+
await client.query(
186+
`INSERT INTO "${schemaName}"."group_file" ("fileId", "groupId") VALUES
187+
('fGroup', 'g1'),
188+
('fGroup', 'uMember'),
189+
('fGroup', 'uGuest'),
190+
('fLegacy', 'uOwner'),
191+
('fLegacy', 'uGuest'),
192+
('fControl', 'uGuest')`
193+
)
194+
}
195+
196+
// Unsharing fires the trigger, whose body reads `file_state` and `group_user`
197+
// unqualified — resolved via search_path at execution time — so the UPDATE
198+
// must run with the test schema pinned.
199+
async function unshare(fileId: string) {
200+
await inTestSchema(
201+
`UPDATE "${schemaName}"."file" SET "shared" = false WHERE "id" = '${fileId}'`
202+
)
203+
}
204+
205+
async function statesFor(fileId: string): Promise<string[]> {
206+
const res = await client.query<{ userId: string }>(
207+
`SELECT "userId" FROM "${schemaName}"."file_state" WHERE "fileId" = $1 ORDER BY "userId"`,
208+
[fileId]
209+
)
210+
return res.rows.map((r) => r.userId)
211+
}
212+
213+
async function linksFor(fileId: string): Promise<string[]> {
214+
const res = await client.query<{ groupId: string }>(
215+
`SELECT "groupId" FROM "${schemaName}"."group_file" WHERE "fileId" = $1 ORDER BY "groupId"`,
216+
[fileId]
217+
)
218+
return res.rows.map((r) => r.groupId)
219+
}
220+
221+
describe('with the fixed function (migration 034)', () => {
222+
beforeEach(async () => {
223+
await inTestSchema(FIXED_FUNCTION_SQL)
224+
await client.query(SCHEMA_SQL)
225+
await seed()
226+
})
227+
228+
it('removes the guest state but keeps group members when a group-owned file is unshared', async () => {
229+
await unshare('fGroup')
230+
// owner + member of the owning group keep access; guest is cleaned up
231+
expect(await statesFor('fGroup')).toEqual(['uMember', 'uOwner'])
232+
})
233+
234+
it('removes the guest file link but keeps the owning group row and member links', async () => {
235+
await unshare('fGroup')
236+
// the file still lives in its owning group, and the member keeps their
237+
// home-group link (they retain access); the guest's link is cleaned up
238+
// so the file stops showing in their recent files
239+
expect(await linksFor('fGroup')).toEqual(['g1', 'uMember'])
240+
})
241+
242+
it('removes the guest state but keeps the owner when a legacy file is unshared', async () => {
243+
await unshare('fLegacy')
244+
expect(await statesFor('fLegacy')).toEqual(['uOwner'])
245+
})
246+
247+
it('removes the guest file link but keeps the owner link when a legacy file is unshared', async () => {
248+
await unshare('fLegacy')
249+
expect(await linksFor('fLegacy')).toEqual(['uOwner'])
250+
})
251+
252+
it('leaves still-shared files untouched', async () => {
253+
await unshare('fGroup')
254+
expect(await statesFor('fControl')).toEqual(['uGuest', 'uMember', 'uOwner'])
255+
expect(await linksFor('fControl')).toEqual(['uGuest'])
256+
})
257+
})
258+
259+
describe('with the original buggy function (regression guard)', () => {
260+
beforeEach(async () => {
261+
await inTestSchema(BUGGY_FUNCTION_SQL)
262+
await client.query(SCHEMA_SQL)
263+
await seed()
264+
})
265+
266+
it('demonstrates the bug: group-owned guest state survives because ownerId is NULL', async () => {
267+
await unshare('fGroup')
268+
// NULL != "userId" is NULL, so the old DELETE matched nothing: the guest lingers
269+
expect(await statesFor('fGroup')).toEqual(['uGuest', 'uMember', 'uOwner'])
270+
// and the old function never touched group_file at all, so the guest's
271+
// home-group link (their recent-files entry) survived too
272+
expect(await linksFor('fGroup')).toEqual(['g1', 'uGuest', 'uMember'])
273+
})
274+
275+
it('still works for legacy files (so the regression is group-specific)', async () => {
276+
await unshare('fLegacy')
277+
expect(await statesFor('fLegacy')).toEqual(['uOwner'])
278+
})
279+
})
280+
})

‎apps/dotcom/zero-cache/migrations/034_fix_unshare_group_file_cleanup.sql‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
-- Fix guest cleanup when a group-owned file is unshared.
2+
--
3+
-- The original delete_file_states trigger (000_seed.sql) keys on
4+
-- `OLD."ownerId" != "userId"` to decide which file_states are "guests". The groups
5+
-- backend (023_groups.sql) introduced group-owned files where `ownerId` is NULL
6+
-- (see the ownerId XOR owningGroupId constraint). For those files
7+
-- `NULL != "userId"` evaluates to NULL, so the DELETE matched no rows and guest
8+
-- file_states lingered after unsharing — leaving the now-private file syncing to
9+
-- ex-guests and showing in their recent files.
10+
--
11+
-- Visiting a shared file also links it into the visitor's home group via a
12+
-- group_file row (home group id == user id by convention), which is what puts
13+
-- the file in their recent files. Nothing cleaned those links up on unshare
14+
-- either — so the file's name kept showing in an ex-guest's recent files even
15+
-- once their file_state and access were gone.
16+
--
17+
-- This replaces the function so it cleans up both records for both ownership
18+
-- models:
19+
-- * file_state: delete states for anyone who isn't the legacy owner
20+
-- (NULL-safe) or a current member of the owning group;
21+
-- * group_file: delete the file's links except the owning group's own row
22+
-- and home-group links of users who keep access (the legacy owner /
23+
-- owning-group members).
24+
-- The trigger binding (file_shared_update) is unchanged.
25+
CREATE OR REPLACE FUNCTION delete_file_states() RETURNS TRIGGER AS $$
26+
BEGIN
27+
IF OLD.shared = TRUE AND NEW.shared = FALSE THEN
28+
DELETE FROM file_state fs
29+
WHERE fs."fileId" = OLD.id
30+
-- not the legacy owner (IS DISTINCT FROM is NULL-safe, so a NULL ownerId
31+
-- never accidentally protects a guest the way `!=` did)
32+
AND OLD."ownerId" IS DISTINCT FROM fs."userId"
33+
-- not a current member of the owning group (no-op for legacy files, where
34+
-- owningGroupId is NULL and the subquery returns nothing)
35+
AND NOT EXISTS (
36+
SELECT 1 FROM group_user gu
37+
WHERE gu."groupId" = OLD."owningGroupId"
38+
AND gu."userId" = fs."userId"
39+
);
40+
41+
DELETE FROM group_file gf
42+
WHERE gf."fileId" = OLD.id
43+
-- never the owning group's own row: that's where the file lives
44+
AND gf."groupId" IS DISTINCT FROM OLD."owningGroupId"
45+
-- not the legacy owner's home-group link (home group id == user id)
46+
AND gf."groupId" IS DISTINCT FROM OLD."ownerId"
47+
-- not a home-group link of a current owning-group member (no-op for
48+
-- legacy files, where owningGroupId is NULL and the subquery returns
49+
-- nothing)
50+
AND NOT EXISTS (
51+
SELECT 1 FROM group_user gu
52+
WHERE gu."groupId" = OLD."owningGroupId"
53+
AND gu."userId" = gf."groupId"
54+
);
55+
END IF;
56+
RETURN NEW;
57+
END;
58+
$$ LANGUAGE plpgsql;

0 commit comments

Comments
 (0)