Skip to content

feat(sharing): per-folder / per-project collaborators API - #111

Merged
nbkdoesntknowcoding merged 1 commit into
nbkdoesntknowcoding:mainfrom
Jason-jo17:feat/resource-collaborators-api
Jul 24, 2026
Merged

feat(sharing): per-folder / per-project collaborators API#111
nbkdoesntknowcoding merged 1 commit into
nbkdoesntknowcoding:mainfrom
Jason-jo17:feat/resource-collaborators-api

Conversation

@Jason-jo17

@Jason-jo17 Jason-jo17 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What & why

A folder or project can't be shared with a specific person today. The permission engine already supports it — only the API surface was missing: doc_acl rows are only ever written from routes/docs.ts and routes/org.ts, and routes/folders.ts has no sharing endpoints at all (nor does routes/projects.ts).

This adds the missing CRUD — and nothing else:

GET    /api/{folders|projects}/:id/collaborators
POST   /api/{folders|projects}/:id/collaborators
PATCH  /api/{folders|projects}/:id/collaborators/:userId
DELETE /api/{folders|projects}/:id/collaborators/:userId

Deliberately additive — no schema change, and no change to how permissions resolve. It writes the same doc_acl rows the org access matrix already writes, for resource types doc_acl has always modelled ('folder' | 'project'). app_effective_permission() precedence (explicit deny wins, then best positive) and the canAccess() folder/project → doc inheritance already do the rest — so a grant on a folder automatically covers every doc inside it, with no new resolution logic.

Authorization. Managing shares requires admin on the resource via canAccess(), which falls through to the workspace role — owners/admins manage sharing without an explicit grant on every resource. Unknown ids and ids belonging to another tenant both 404 (existence is checked through RLS, so this can't be used to probe for foreign ids). Every grant/update/revoke is written to iam_audit_log.

Scope (intentional). The grantee must already be a workspace member; POST returns not_a_workspace_member otherwise. Inviting an outside email needs a pending grant that materialises on signup plus a restricted-visibility guest role — kept out of this PR because it changes a security default and deserves its own decision.

Validation lives in lib/collaborators/grants.ts rather than in the route so it's importable without the db/config graph (same split as lib/community/handle.ts) — that's what makes it unit-testable. The permission vocabulary is pinned to doc_acl's, and a test asserts a 'comment' tier does not validate yet, so a future tier can't get half-landed by accident.

Linked issue

Refs #112 — which also asks the core vs enterprise question: CONTRIBUTING lists org-IAM-SSO as enterprise, but doc_acl, lib/iam.ts, folders.ts and projects.ts are all in this repo, so resource-level ACL reads as core while org-wide IAM is the enterprise part. Built in core on that basis — happy to relocate, it's one route file, one lib file and one registration line. Please treat #112 as the decision point before merging.

Test evidence

  • pnpm --filter @boppl/api run typecheck — clean.
  • eslint --max-warnings 0 on routes/collaborators.ts, lib/collaborators/grants.ts, grants.test.ts, server.ts — clean.
  • verify-core-onlyCLEAN (core compiles with the enterprise modules absent).
  • 12 new unit tests pass: vocabulary pinned to doc_acl; explicit deny stays grantable (it's not the same as revoking); 'comment' rejected; non-uuid principal and non-ISO expiry rejected; empty patch rejected; null expiry = permanent.
  • Full API test suite run against a clean-main baseline: identical pre-existing failures, +12 passing on top — no regressions.

Not run: a live end-to-end against a real Postgres with RLS enabled. Worth exercising grant → inherited doc access → revoke on a real instance before merge, since the inheritance path is the whole payoff of this change.

Checklist

  • One logical change; matches the surrounding code style (mirrors the org.ts grant-writing convention).
  • Ran the relevant checks (typecheck, lint, pnpm test, core-boundary) for what I touched.
  • Commits are signed off for the DCO (git commit -s).
  • This is a core change — not enterprise-adjacent and not in the licensing/release machinery. (Flagged in Folders and projects cannot be shared with a specific person (no collaborators API) #112 for confirmation, since resource ACL sits near the org-IAM line.)
  • I understand this PR is merged here on GitHub once approved (authorship + DCO preserved) and ships in the next tagged release.

No new dependencies (so the license-policy gate is unaffected) and no migration.

Folders and projects had no way to add a collaborator: `doc_acl` rows were only
ever written from routes/docs.ts and routes/org.ts, and routes/folders.ts had no
sharing endpoints at all. This adds the missing CRUD, and nothing else.

  GET    /api/{folders|projects}/:id/collaborators
  POST   /api/{folders|projects}/:id/collaborators
  PATCH  /api/{folders|projects}/:id/collaborators/:userId
  DELETE /api/{folders|projects}/:id/collaborators/:userId

Purely additive — no schema change and no change to how permissions resolve. It
writes the SAME `doc_acl` rows the org access matrix already writes, for resource
types `doc_acl` has always modelled ('folder' | 'project'), so
app_effective_permission() precedence and the canAccess() folder/project -> doc
inheritance already do the rest: a grant here automatically covers everything
inside the folder/project.

Authorization: managing shares requires `admin` ON THE RESOURCE via canAccess(),
which falls through to the workspace role. Unknown ids and ids in another tenant
both 404 (existence is checked through RLS, so this can't probe foreign ids).
Every grant/update/revoke is written to iam_audit_log.

Scope: the grantee must already be a workspace member — POST returns
`not_a_workspace_member` otherwise. Inviting an outside email needs a pending
grant that materialises on signup plus guest-visibility work, which land separately.

Validation lives in lib/collaborators/grants.ts rather than the route so it is
importable without the db/config graph (same split as lib/community/handle.ts),
which is what makes it unit-testable. A test asserts 'comment' does NOT validate
yet, so the future comment tier can't be half-landed by accident.

Signed-off-by: Jason-jo17 <jason@theboringpeople.in>
@Jason-jo17

Copy link
Copy Markdown
Contributor Author

Heads-up on the project half of this PR — projects already have a sharing path.

While building the phase-4 UI I found that projects already ship an "Access · {project}" modal in ProjectsPage.tsx: add people by email, roles Viewer/Editor/Admin, backed by GET/POST/PATCH/DELETE /api/projects/:id/members on project_members.

So after this PR a project has two valid ways to grant access:

  1. project_members — the existing endpoints + UI
  2. doc_acl project grants — what this PR adds

I checked whether the doc_acl path actually works at the RLS layer, because app_can_see_project in migration 0028 only consults project_members. It does work — 0028 is superseded by 0049 and then 0054, and 0054 explicitly drops the editor gate so "a positive project ACL grant now grants visibility to ANY non-admin principal". So both paths are real and RLS-honoured; this isn't a bug, it's duplicated surface.

Folders are the unambiguous gapdoc_acl is their only path and there was no API or UI at all.

Options, your call:

  • Merge as-is — both paths coexist (they already can; app_acl_permits is deny-first and expiry-aware).
  • Narrow this PR to folders only — I'll drop the project registration; it's a one-line change.
  • Consolidate on one mechanism for projects — bigger, and worth its own issue.

I've deliberately kept the phase-4 UI (#197) to folders only so it doesn't put a rival dialog next to the working project Access modal.

@nbkdoesntknowcoding
nbkdoesntknowcoding merged commit 2798e33 into nbkdoesntknowcoding:main Jul 24, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants