[pull] main from tldraw:main#581
Merged
Merged
Conversation
) In order to make group roles easy to read and safe to adjust, this PR refactors the dotcom groups prototype so authorization is a single predicate — `can(role, 'deleteGroup')` — instead of branching on role literals (`role === 'owner'` / `role === 'admin'`) or checking group membership existence directly. Each role is defined as a list of named capabilities in one `roles` table, so what a role can do reads off one place, and roles can be added or changed without hunting down scattered checks. This came out of renaming `admin` → `member`: it was hard to tell what that role actually grants. With capabilities, a role's powers are a list you can read, and the rename becomes a relabel rather than a behavioral question. The change is behavior-preserving, with one intentional exception: the group-name input is now `disabled` for members without the `editGroup` capability (previously they could type into it and the mutation silently failed server-side). ### Concepts | Term | Type | Meaning | | --- | --- | --- | | `capabilities` | `as const` array | The list of every capability, enumerable at runtime (`capabilities.ts`) | | `Capability` | `(typeof capabilities)[number]` | A single thing a member can do — derived from the list | | `roles` | table | The single source of truth: each role → its capability list (`roles.ts`) | | `Role` | `keyof typeof roles` | The role string stored in `group_user.role` (`'admin' \| 'owner'`) — derived from the table keys | | `can(role, capability)` | function | The one check. `role` is loose (`string \| null \| undefined`); unknown/null → `false` | | `isRole(role)` | guard | Whether a string is a known role (used for validating client input) | | `getRole(db, …)` | sync-worker helper | Resolves the role from the DB (mirrors the mutators' `getRole`); compose with `can` | The role stays a plain string in the DB; capabilities are never persisted — they're derived from that string in code. ### Usage ```ts // client — role comes straight from the synced membership const role = currentUser?.role can(role, 'editGroup') // boolean, no wrapper, no narrowing // mutators — resolve the role, then check it const role = await getRole(tx, userId, id) assert(can(role, 'deleteGroup'), ZErrorCode.forbidden) // sync-worker (Kysely) — getRole resolves from the DB const role = await getRole(db, userId, groupId) if (can(role, 'accessFiles')) { /* allow */ } ``` ### Where it's enforced Every group authorization check now goes through `can`, so they all consult the same table: - **Mutators** (`@tldraw/dotcom-shared`) — `getRole` + `can(role, …)` on each group mutation. - **Sync-worker** — `getRole` + `can(role, 'accessFiles')` for file access in `getAuth` (`requireWriteAccessToFile`), `worker.ts`, and the three `TLFileDurableObject` socket/snapshot/prune paths that previously checked membership existence directly. - **Client** — `TldrawApp.canUpdateFile` and `useHasFileAdminRights` use `can(role, 'accessFiles')` instead of bare membership checks. The "a group must always keep at least one owner" rule (last owner can't be demoted or leave) is a group **invariant**, not a capability, so it stays as explicit, labeled logic — it's now the only place that compares against a role literal. ### Capability → role matrix These map 1:1 onto current behavior — no role gains or loses anything. | Capability | Operation | admin | owner | | --- | --- | :---: | :---: | | `accessFiles` | open/edit the group's files, reorder own membership | ✓ | ✓ | | `addFiles` | link/move a file into the group, drag | ✓ | ✓ | | `removeFiles` | remove a file from the group, move-out | ✓ | ✓ | | `manageInvites` | regenerate the invite link | ✓ | ✓ | | `editGroup` | rename the group | | ✓ | | `editMembers` | change other members' roles (more member actions to come) | | ✓ | | `deleteGroup` | delete the group and its files | | ✓ | ### On the rename Stored role values stay `'admin' | 'owner'` here to keep this separate from the rename migration. Because no logic branches on the literal anymore, renaming `admin` → `member` later is just relabeling the key in the `roles` table (plus a data migration and flipping the hardcoded `role: 'admin'` in `acceptInvite.ts`). ### Change type - [x] `improvement` ### Test plan 1. As a group **owner**: rename the group, regenerate the invite, change a member's role, remove a file, delete the group — all succeed. 2. As a group **admin/member**: regenerate the invite and add/remove files succeed; the name input is disabled and the role dropdown / delete group are hidden. 3. Confirm the last owner still can't be demoted or leave. 4. Confirm a non-member is denied file access (socket connect, download) on a group-owned file. - [x] Unit tests - [ ] End to end tests `roles.test.ts` covers `can`/`isRole` and the role → capability matrix; existing `mutators.test.ts` group tests cover the mutator gates. ### Code changes | Section | LOC change | | -------------- | ----------- | | Core code | +139 / -67 | | Tests | +69 / -0 | | Apps | +83 / -92 |
The `update-snapshots` workflow regenerates Playwright snapshots and commits them via huppy-bot. When a run produces no snapshot changes, the `git commit` step failed with `nothing to commit, working tree clean` (exit 1), so the workflow reported a red ✗ even though nothing was wrong — there was simply nothing to update. This guards the commit step: if nothing is staged, it logs a message and exits 0 instead of attempting an empty commit. Runs that do produce snapshot changes still commit and push as before. ## Code changes | File | +/- | | --- | --- | | `.github/workflows/playwright-update-snapshots.yml` | +4 / −0 | ## Test plan - A label-triggered run with no snapshot diffs now completes green instead of failing on the empty commit. - A run that does produce snapshot changes still commits and pushes (the guard only short-circuits the empty case).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )