fix(access): system collections are ordinary collections for restricted members (TASK-3376) (#1756)

A member restricted to specific collections saw every system collection
(Conventions, Playbooks) whether it was listed for them or not, and an
editor among them could write it, because requireEditPermission passes
any editor before it looks at collections. Eight sites each unioned the
system collections in. They now follow member_collection_access and
grants like any other collection, for reads and writes.

- Store: VisibleCollectionIDsQ, GetUserWorkspaces' freshness MAX and
  ResolveBacklinksVisibility drop the union; ListSystemCollectionIDs(Q)
  had no caller left and is removed.
- Server: the item-visibility rule, /me's full-access set, watch access,
  search and the SSE full set drop it.
- Migrations 108 / 082 list every live system collection for every
  member already restricted, so the upgrade changes nobody's access. A
  system collection created later is not added for them.
- Settings: Conventions and Playbooks are real checkboxes in the member
  access picker, checked by default when an owner moves a member from
  All to Specific, with a line saying what unchecking them does.

Also (PR #1756 codex round 2 P1): an editor or owner ROLE applies only
inside the collections a member's access reaches ('all', or listed in
member_collection_access). Before, requireEditPermission let any editor
through before it looked at the collection, so a restricted editor who
could see an item only through a VIEW grant could edit it, comment on it,
create beside it and bulk-mutate it. That was true on main for ordinary
collections too. Outside the member's reach the grant decides, and only
'edit' writes. The same rule now governs the collab socket's write
decision, the bulk per-item and move-target gates, and the cross-workspace
edit authorizer, and ResolveUserPermission's membership step checks the
member's listed collections instead of the navigation set (which lists a
collection because one item in it is granted).

The collection check binds the EDITOR role only (codex round 3, lead
ruling): an owner can set any member's collection access, their own
included, so the owner role keeps its full edit bypass, and the server
agrees with the web client's canEditItem / canEditCollection.
This commit is contained in:
xarmian
2026-10-03 09:19:10 -04:00
committed by GitHub
parent 0929f65705
commit 3bcef0e04d
24 changed files with 955 additions and 205 deletions
+20 -2
View File
@@ -771,6 +771,9 @@ func (s *Server) crossWorkspaceRole(r *http.Request, ws *models.Workspace, user
// never read from workspaceRole(r);
// - it runs only AFTER the scoped visibility check.
//
// (TASK-3376: the ordering alone did NOT make it safe. An item-scoped
// visibility check passes on a VIEW grant, so the base role must also reach
// the collection; see the base-role branch below.)
// That ordering is what makes the base-role branch safe. DR-10a's
// escalation is the fast path used ALONE: an editor whose membership is
// collection_access="specific" passing the role check for a collection
@@ -809,9 +812,24 @@ func (s *Server) crossWorkspaceEditAllowed(ws *models.Workspace, user *models.Us
}
// Base-role branch, mirroring requireEditPermission. "guest" is
// excluded: a guest has no role-based permission at all, only
// grants.
// grants. Since TASK-3376 (PR #1756 codex round 2 P1) the role, like
// requireEditPermission's, applies only inside the member's collection
// access: visibility can come from a VIEW grant, so "the caller can see
// it" no longer stands in for "the role reaches it".
if role != "guest" && roleLevel(role) >= roleLevel("editor") {
return true, nil
member, err := s.store.GetWorkspaceMember(ws.ID, user.ID)
if err != nil {
return false, err
}
if member != nil {
reaches, err := s.memberRoleReachesCollection(ws.ID, member, scope.permCollectionID())
if err != nil {
return false, err
}
if reaches {
return true, nil
}
}
}
perm, err := s.store.ResolveUserPermission(ws.ID, user.ID, scope.itemID(), scope.permCollectionID())
if err != nil {
+64 -15
View File
@@ -691,9 +691,13 @@ func TestCrossWorkspace_GrantsWidenNeverNarrow(t *testing.T) {
// --- System collections ------------------------------------------------
// Restricted members keep access to system collections (conventions,
// playbooks). That allowance survives the item-grant filtering branch,
// which is the shape that used to 404 them.
// A system collection (conventions, playbooks) is an ordinary collection
// for a restricted member: reachable only when it is in their
// member_collection_access, for reads AND writes (TASK-3376, Dave's day-84
// ruling). Before it, every restricted member reached every system
// collection implicitly, and an editor among them could write it. The
// listed leg is the control: the same scopes pass once the collection is
// granted, so the unlisted denials are about the listing, not the fixture.
func TestCrossWorkspace_RestrictedMemberSystemCollection(t *testing.T) {
f := newCrossWSFixture(t)
sysColl, err := f.srv.store.CreateCollection(f.wsB.ID, models.CollectionCreate{Name: "Conventions B", IsSystem: true})
@@ -712,24 +716,35 @@ func TestCrossWorkspace_RestrictedMemberSystemCollection(t *testing.T) {
}
r := f.request(u, reqOpts{wsRoleCtx: "editor", wsIDCtx: f.wsA.ID})
assertAllowed(t, f.srv.AuthorizeCrossWorkspaceRead(r, f.wsB.Slug, CrossWorkspaceItemScope(sysItem)),
"restricted member reads a system-collection item")
assertAllowed(t, f.srv.AuthorizeCrossWorkspaceEdit(r, f.wsB.Slug, CrossWorkspaceCollectionScope(sysColl.ID)),
"restricted member creates into a system collection")
// The non-system collection they were NOT granted stays hidden — the
// system allowance must not blanket the workspace.
assertDenied(t, f.srv.AuthorizeCrossWorkspaceRead(r, f.wsB.Slug, CrossWorkspaceItemScope(f.hiddenIB)),
CrossWorkspaceItemNotVisible, "system allowance is not a blanket pass")
assertDenied(t, f.srv.AuthorizeCrossWorkspaceRead(r, f.wsB.Slug, CrossWorkspaceItemScope(sysItem)),
CrossWorkspaceItemNotVisible, "unlisted system collection: item read")
assertDenied(t, f.srv.AuthorizeCrossWorkspaceEdit(r, f.wsB.Slug, CrossWorkspaceCollectionScope(sysColl.ID)),
CrossWorkspaceCollectionNotVisible, "unlisted system collection: create into it")
// Activate the item-grant filtering branch with an unrelated grant.
// The system allowance must survive it.
// The fixture's ordinary hidden collection is the baseline these
// denials must match.
assertDenied(t, f.srv.AuthorizeCrossWorkspaceRead(r, f.wsB.Slug, CrossWorkspaceItemScope(f.hiddenIB)),
CrossWorkspaceItemNotVisible, "unlisted ordinary collection, same refusal")
// The item-grant filtering branch used to carry its own system union
// (server.go item rule c). An unrelated grant activates it; the
// unlisted system collection must stay closed through it.
if _, err := f.srv.store.CreateItemGrant(f.wsB.ID, f.hiddenIB.ID, u.ID, "view", f.ownerBoth.ID); err != nil {
t.Fatalf("CreateItemGrant: %v", err)
}
assertDenied(t, f.srv.AuthorizeCrossWorkspaceRead(r, f.wsB.Slug, CrossWorkspaceItemScope(sysItem)),
CrossWorkspaceItemNotVisible, "unlisted system collection with item-grant filtering active")
assertDenied(t, f.srv.AuthorizeCrossWorkspaceEdit(r, f.wsB.Slug, CrossWorkspaceCollectionScope(sysColl.ID)),
CrossWorkspaceCollectionNotVisible, "unlisted system collection scope with item-grant filtering active")
// Control: listed, it behaves like any granted collection.
if err := f.srv.store.SetMemberCollectionAccess(f.wsB.ID, u.ID, "specific", []string{f.collB.ID, sysColl.ID}); err != nil {
t.Fatalf("SetMemberCollectionAccess (listed): %v", err)
}
assertAllowed(t, f.srv.AuthorizeCrossWorkspaceRead(r, f.wsB.Slug, CrossWorkspaceItemScope(sysItem)),
"system-collection item with item-grant filtering active")
"listed system collection: item read")
assertAllowed(t, f.srv.AuthorizeCrossWorkspaceEdit(r, f.wsB.Slug, CrossWorkspaceCollectionScope(sysColl.ID)),
"system collection scope with item-grant filtering active")
"listed system collection: create into it")
}
// --- Grants on soft-deleted resources ----------------------------------
@@ -1137,3 +1152,37 @@ func TestCrossWorkspace_WriteCollectionNotFoundRefusesForeignVerdicts(t *testing
t.Fatalf("workspace-only denial: got %d, want 403: %s", rec.Code, rec.Body.String())
}
}
// A restricted editor who can SEE an item in another workspace only through a
// VIEW grant may not edit it through the cross-workspace authorizer
// (TASK-3376, PR #1756 codex round 2 P1): the base role reaches only the
// member's listed collections. The edit grant is the control.
func TestCrossWorkspace_RestrictedEditorViewGrantIsNotEdit(t *testing.T) {
f := newCrossWSFixture(t)
u := f.member("vg@example.com", "vguser", "editor", f.wsA)
if err := f.srv.store.AddWorkspaceMember(f.wsB.ID, u.ID, "editor"); err != nil {
t.Fatalf("AddWorkspaceMember: %v", err)
}
if err := f.srv.store.SetMemberCollectionAccess(f.wsB.ID, u.ID, "specific", []string{f.collB.ID}); err != nil {
t.Fatalf("SetMemberCollectionAccess: %v", err)
}
g, err := f.srv.store.CreateItemGrant(f.wsB.ID, f.hiddenIB.ID, u.ID, "view", f.ownerBoth.ID)
if err != nil {
t.Fatalf("CreateItemGrant: %v", err)
}
r := f.request(u, reqOpts{wsRoleCtx: "editor", wsIDCtx: f.wsA.ID})
assertAllowed(t, f.srv.AuthorizeCrossWorkspaceRead(r, f.wsB.Slug, CrossWorkspaceItemScope(f.hiddenIB)),
"precondition: the view grant makes the item readable")
assertDenied(t, f.srv.AuthorizeCrossWorkspaceEdit(r, f.wsB.Slug, CrossWorkspaceItemScope(f.hiddenIB)),
CrossWorkspaceInsufficientPermission, "view grant is not edit for a restricted editor")
if err := f.srv.store.DeleteItemGrant(g.ID, f.wsB.ID); err != nil {
t.Fatalf("DeleteItemGrant: %v", err)
}
if _, err := f.srv.store.CreateItemGrant(f.wsB.ID, f.hiddenIB.ID, u.ID, "edit", f.ownerBoth.ID); err != nil {
t.Fatalf("CreateItemGrant edit: %v", err)
}
assertAllowed(t, f.srv.AuthorizeCrossWorkspaceEdit(r, f.wsB.Slug, CrossWorkspaceItemScope(f.hiddenIB)),
"control: an edit grant edits")
}
+14
View File
@@ -678,8 +678,22 @@ func (s *Server) authorizeCollabAccess(r *http.Request, item *models.Item) (coll
// `role != "guest" && requireRole(r, "editor")`. The visibility
// checks below decide READ admission to the room; canWrite decides
// whether the admitted conn may persist inbound frames. Per TASK-265.
//
// TASK-3376 (PR #1756 codex round 2 P1): the role applies only inside
// the member's collection access, exactly as in requireEditPermission
// (memberRoleReachesCollection). Outside it, a grant decides, so a view
// grant that admits a restricted editor to the room does not let them
// write.
var canWrite bool
roleReaches := false
if member != nil && roleLevel(member.Role) >= roleLevel("editor") {
reaches, err := s.memberRoleReachesCollection(wsID, member, item.CollectionID)
if err != nil {
return collabAccess{}, err
}
roleReaches = reaches
}
if roleReaches {
canWrite = true
} else {
perm, err := s.store.ResolveUserPermission(wsID, fresh.ID, item.ID, item.CollectionID)
-4
View File
@@ -913,13 +913,9 @@ func (s *Server) computeSSEVisibility(r *http.Request, workspaceID string) sseVi
}
if member != nil {
memberColls, _ := s.store.GetMemberCollectionAccess(workspaceID, user.ID)
sysColls, _ := s.store.ListSystemCollectionIDs(workspaceID)
for _, id := range memberColls {
fullCollIDSet[id] = true
}
for _, id := range sysColls {
fullCollIDSet[id] = true
}
}
v.fullCollSet = make(map[string]bool, len(fullCollIDSet))
for id := range fullCollIDSet {
+20
View File
@@ -283,6 +283,19 @@ func (s *Server) handleBulkItems(w http.ResponseWriter, r *http.Request) {
resp.Failed = append(resp.Failed, bulkItemFailure{Ref: ref, Error: "item not found"})
continue
}
// Per-item EDIT gate (TASK-3376, PR #1756 codex round 2 P1). Seeing
// an item is not editing it: a restricted editor who sees this one
// through a view grant, outside their collection access, may not
// bulk-mutate it any more than PATCH it.
canEdit, eerr := s.canEditInCollection(r, workspaceID, item.ID, item.CollectionID)
if eerr != nil {
resp.Failed = append(resp.Failed, bulkItemFailure{Ref: ref, Error: eerr.Error()})
continue
}
if !canEdit {
resp.Failed = append(resp.Failed, bulkItemFailure{Ref: itemRefOrSlug(*item), Error: "insufficient permissions", Code: "forbidden"})
continue
}
var droppedFields []string
updated, opErr := s.applyBulkOp(r, workspaceID, item, &req, actor, source, visibleIDs, resolvedTarget, batchID, &droppedFields)
@@ -908,6 +921,13 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m
if !isCollectionVisible(targetColl.ID, visibleIDs) {
return nil, &bulkOpError{message: "target collection not found", code: "invalid_collection"}
}
// Writing into the target needs edit there too, as handleMoveItem's
// requireEditPermission on the target does (TASK-3376).
if ok, err := s.canEditInCollection(r, workspaceID, "", targetColl.ID); err != nil {
return nil, &bulkOpError{message: err.Error(), code: "internal_error"}
} else if !ok {
return nil, &bulkOpError{message: "insufficient permissions on the target collection", code: "forbidden"}
}
if targetColl.ID == item.CollectionID {
return nil, &bulkOpError{message: "item is already in this collection", code: "same_collection"}
}
+1 -9
View File
@@ -145,7 +145,7 @@ func (s *Server) handleGetMe(w http.ResponseWriter, r *http.Request) {
// Strict full-access set — used for per-item visibility
// decisions. Mirrors guestResourceFilter's fullCollIDs for
// restricted members: explicit member_collection_access +
// system collections + direct collection grants. Item-grant
// direct collection grants. Item-grant
// collections are intentionally excluded — having an item grant
// in a collection does NOT confer access to siblings.
fullSet := make(map[string]struct{})
@@ -157,14 +157,6 @@ func (s *Server) handleGetMe(w http.ResponseWriter, r *http.Request) {
for _, id := range memberColls {
fullSet[id] = struct{}{}
}
sysColls, err := s.store.ListSystemCollectionIDs(workspaceID)
if err != nil {
writeInternalError(w, err)
return
}
for _, id := range sysColls {
fullSet[id] = struct{}{}
}
for _, g := range collGrants {
fullSet[g.CollectionID] = struct{}{}
}
+2 -1
View File
@@ -432,7 +432,8 @@ func (s *Server) handlePushToItem(w http.ResponseWriter, r *http.Request) {
// (receipt, so a successor can re-derive it instead of trusting it):
// computeWatchAccessVisibility's inputs are platform role, workspace
// membership, CollectionAccess, GuestVisibleResources,
// GetMemberCollectionAccess, ListSystemCollectionIDs — all per-USER,
// GetMemberCollectionAccess — all per-USER (ListSystemCollectionIDs was
// one too until TASK-3376 removed it),
// identical for every session this function counts, since it only ever
// counts one user's own sessions — plus bearerAuth, which is
// per-CONNECTION and the sole varying input. One varying boolean means
@@ -44,12 +44,13 @@ import (
// opposite of the fail-closed posture they document.
//
// SECOND GAP, MEASURED NOT GUESSED. This reaches only the FIRST of the
// function's four store calls. Closing the DB makes GetWorkspaceMember
// fail immediately, so the function returns before GuestVisibleResources,
// GetMemberCollectionAccess or ListSystemCollectionIDs ever run — and a
// mutation re-swallowing GetMemberCollectionAccess's error survives this
// test (verified, matrix entry M11). The later three are propagated by
// inspection, not by measurement.
// function's store calls (four when written; three since TASK-3376 removed
// ListSystemCollectionIDs). Closing the DB makes GetWorkspaceMember fail
// immediately, so the function returns before GuestVisibleResources or
// GetMemberCollectionAccess ever run — and a mutation re-swallowing
// GetMemberCollectionAccess's error survives this test (verified, matrix
// entry M11). The later calls are propagated by inspection, not by
// measurement.
//
// Reaching them needs the earlier calls to SUCCEED while a later one
// fails, which this package cannot express today: srv.store is a
+22 -12
View File
@@ -517,15 +517,15 @@ func TestCheckItemVisible_AdminBearerRestrictedRoleDeniedOnHiddenCollection(t *t
}
}
// TestRefResolver_RestrictedMemberWithSystemCollection pins round-2 P1.2:
// a restricted member with conventions/playbooks (system collections)
// access plus an item grant in an UNRELATED collection must be able to
// resolve refs that live in the system collection. Pre-round-2,
// checkItemVisible's item-grants branch only consulted direct
// collection_grants + member_collection_access — not system collections —
// so a restricted member could LIST conventions via the existing API
// (VisibleCollectionIDs includes system collections) but 404'd on
// detail-fetch / ref-resolve.
// TestRefResolver_RestrictedMemberWithSystemCollection pins round-2 P1.2
// as TASK-3376 restated it: a restricted member who is entitled to a
// system collection, plus an item grant in an UNRELATED collection (which
// turns on checkItemVisible's item-grants branch), must be able to resolve
// refs in that system collection. Since TASK-3376 the entitlement is the
// member_collection_access listing, as for any collection; an unlisted
// system collection resolves to 404 like any hidden one. Both legs run
// through the item-grants branch, so the listed 302 is the control for the
// unlisted 404.
func TestRefResolver_RestrictedMemberWithSystemCollection(t *testing.T) {
f := newRefResolverFixture(t)
@@ -599,11 +599,21 @@ func TestRefResolver_RestrictedMemberWithSystemCollection(t *testing.T) {
t.Fatalf("CreateAPIToken: %v", err)
}
// Resolver should redirect — system collections must remain visible
// to restricted members even when the item-grants branch fires.
// Unlisted: hidden like any collection the member is not given.
rr = f.doAuth(tok.Token, "GET", "/-/r/"+f.ws.Slug+"/"+sysItem.Ref, nil)
if rr.Code != http.StatusNotFound {
t.Fatalf("restricted member on an UNLISTED system collection: expected 404, got %d body=%s",
rr.Code, rr.Body.String())
}
// Listed: the item-grants branch must still find it through the
// member's collection list.
if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, member.ID, "specific", []string{collA.ID, sysColl.ID}); err != nil {
t.Fatalf("SetMemberCollectionAccess (listed): %v", err)
}
rr = f.doAuth(tok.Token, "GET", "/-/r/"+f.ws.Slug+"/"+sysItem.Ref, nil)
if rr.Code != http.StatusFound {
t.Fatalf("restricted member on system collection: expected 302, got %d body=%s",
t.Fatalf("restricted member on a LISTED system collection: expected 302, got %d body=%s",
rr.Code, rr.Body.String())
}
if !strings.Contains(rr.Header().Get("Location"), "/"+sysColl.Slug+"/") {
+3 -7
View File
@@ -258,8 +258,8 @@ func (a searchAccess) none() bool {
// admin is an ordinary user and only membership counts (BUG-1616), not
// even a guest grant.
// - A member with "all" collection access reads everything; a restricted
// member reads their collections, the system collections, and anything
// granted to them.
// member reads their listed collections and anything granted to them.
// System collections are listed like any other (TASK-3376).
// - A non-member reads exactly what is granted to them, which with no
// grants is nothing.
func (s *Server) searchAccessFor(r *http.Request, ws *models.Workspace) (searchAccess, error) {
@@ -311,13 +311,9 @@ func (s *Server) searchAccessFor(r *http.Request, ws *models.Workspace) (searchA
if err != nil {
return searchAccess{}, err
}
sysColls, err := s.store.ListSystemCollectionIDs(ws.ID)
if err != nil {
return searchAccess{}, err
}
seen := map[string]bool{}
var colls []string
for _, group := range [][]string{memberColls, sysColls, grantColls} {
for _, group := range [][]string{memberColls, grantColls} {
for _, id := range group {
if !seen[id] {
seen[id] = true
+7 -14
View File
@@ -94,8 +94,8 @@ type watchAccessVisibility struct {
// including a collection ID merely because the caller has an item
// grant somewhere inside it, which is exactly the leak this field
// must not reproduce. Built from GuestVisibleResources'
// fullCollectionIDs (+ GetMemberCollectionAccess/
// ListSystemCollectionIDs for a member) instead — see
// fullCollectionIDs (+ GetMemberCollectionAccess for a member)
// instead — see
// computeWatchAccessVisibility.
visibleCollIDs map[string]bool
// grantedItemIDs are individually-granted items (guest item grants,
@@ -133,8 +133,9 @@ func (v watchAccessVisibility) allows(collectionID, itemID string) bool {
// GuestVisibleResources' fullCollectionIDs return (populated ONLY from
// direct collection_grants — never widened by an item grant) as the
// "genuinely full access" set, exactly like computeSSEVisibility's own
// fullCollSet construction, with GetMemberCollectionAccess +
// ListSystemCollectionIDs layered on for an actual workspace member.
// fullCollSet construction, with GetMemberCollectionAccess layered on for
// an actual workspace member. (A ListSystemCollectionIDs layer was removed
// by TASK-3376: system collections are listed like any other.)
//
// Admin bypass (TASK-2533 codex round 2 finding 2 — the argument below
// REPLACES a now-falsified one, not just a code fix): this used to treat
@@ -192,7 +193,8 @@ func (v watchAccessVisibility) allows(collectionID, itemID string) bool {
//
// Population (CONVE-18): four store calls in this function could fail —
// GetWorkspaceMember, GuestVisibleResources, GetMemberCollectionAccess,
// ListSystemCollectionIDs. All four now report. The last two were not
// ListSystemCollectionIDs (the last removed since by TASK-3376, leaving
// three). All of them report. The last two were not
// merely swallowed but discarded into `_`, which is how they escaped
// round 1's sweep. Search boundary: this function only; the sibling
// resolver computeSSEVisibility (handlers_events.go) has the same shape
@@ -246,18 +248,9 @@ func (s *Server) computeWatchAccessVisibility(bearerAuth bool, user *models.User
"workspace_id", workspaceID, "user_id", user.ID, "error", mcErr)
return watchAccessVisibility{}, mcErr
}
sysColls, scErr := s.store.ListSystemCollectionIDs(workspaceID)
if scErr != nil {
slog.Warn("watch-access: failed to list system collections, denying all in workspace",
"workspace_id", workspaceID, "error", scErr)
return watchAccessVisibility{}, scErr
}
for _, id := range memberColls {
fullCollSet[id] = true
}
for _, id := range sysColls {
fullCollSet[id] = true
}
}
grantedSet := make(map[string]bool, len(grantedItemIDs))
+104 -41
View File
@@ -3181,13 +3181,11 @@ func (s *Server) checkItemVisibleQ(q store.Queryer, workspaceID string, item *mo
// a) the collection itself has a full grant (any item passes), OR
// b) for restricted members: the collection is in member_collection_access
// (the member's explicit collection-access list), OR
// c) the item's collection is a system collection — restricted
// members always retain access to system collections (conventions,
// playbooks, …); pre-round-2 this branch missed the system-
// collections union that guestResourceFilterCore performed, so a
// restricted member with an item grant in a non-system collection
// was 404'd on a system-collection item they were entitled to see.
// d) the specific item is in the granted-items list.
// c) the specific item is in the granted-items list.
// System collections get no branch of their own: a restricted member
// reaches one only through (a) or (b), like any collection (TASK-3376).
// A per-record reach for conventions and playbooks, if it is built
// (TASK-3376 phase 2), belongs beside (c), as item-level access.
for _, id := range grantCollIDs {
if id == item.CollectionID {
return true, nil
@@ -3206,19 +3204,6 @@ func (s *Server) checkItemVisibleQ(q store.Queryer, workspaceID string, item *mo
return true, nil
}
}
// System-collections union — mirror guestResourceFilterCore's
// pre-round-2 behavior. ListSystemCollectionIDs is a workspace-
// scoped lookup (no per-user filter), so the same call is correct
// for every restricted member in the workspace.
sysColls, err := s.store.ListSystemCollectionIDsQ(q, workspaceID)
if err != nil {
return false, err
}
for _, id := range sysColls {
if id == item.CollectionID {
return true, nil
}
}
}
for _, id := range grantedItemIDs {
if id == item.ID {
@@ -3256,8 +3241,8 @@ func (s *Server) isItemVisibleToGuest(r *http.Request, workspaceID string, item
// - admin users
// - members with "all" collection access (grants should merge, not replace)
// For guests: returns direct collection grants as fullCollIDs + item grants.
// For restricted members: returns member_collection_access + system collections
// + direct collection grants as fullCollIDs, plus item grants as grantedItemIDs.
// For restricted members: returns member_collection_access + direct
// collection grants as fullCollIDs, plus item grants as grantedItemIDs.
// This ensures item grants are additive to the member's existing access.
func (s *Server) guestResourceFilter(r *http.Request, workspaceID string) (fullCollIDs, grantedItemIDs []string, err error) {
return s.guestResourceFilterCore(r, workspaceID, false)
@@ -3352,8 +3337,7 @@ func isCollectionVisible(collectionID string, visibleIDs []string) bool {
// Reuses the existing guestResourceFilter/isCollectionVisible/
// isItemVisibleToGuest helpers rather than a bespoke visibility pass:
// guestResourceFilter's fullCollIDs is already the STRICT full-access set
// (member_collection_access ∪ system collections ∪ direct collection
// grants, excluding item-grant-only collections) — the same strict set
// (member_collection_access ∪ direct collection grants, excluding item-grant-only collections) — the same strict set
// requireCollectionFullyVisible narrows to — so collection grants are
// filtered directly against it with no extra narrowing step.
//
@@ -3434,33 +3418,112 @@ func (s *Server) filterUserGrantsForCaller(r *http.Request, workspaceID string,
// OAuth/MCP consent allow-list, which this helper does not (DR-10 of
// PLAN-2357).
func (s *Server) requireEditPermission(w http.ResponseWriter, r *http.Request, workspaceID string, itemID, collectionID string) bool {
role := workspaceRole(r)
// Editors and owners always have edit access
if role != "guest" && requireRole(r, "editor") {
return true
}
// For guests and members with insufficient role (e.g., viewers),
// check grant-based permissions as an override.
user := currentUser(r)
if user == nil {
writeError(w, http.StatusForbidden, "forbidden", "Insufficient permissions")
return false
}
perm, err := s.store.ResolveUserPermission(workspaceID, user.ID, itemID, collectionID)
ok, err := s.canEditInCollection(r, workspaceID, itemID, collectionID)
if err != nil {
writeInternalError(w, err)
return false
}
if permissionLevel(perm) < permissionLevel("edit") {
if !ok {
writeError(w, http.StatusForbidden, "forbidden", "Insufficient permissions")
return false
}
return true
}
// canEditInCollection is requireEditPermission's decision without the
// response, for callers that report per row (bulk) rather than per request.
//
// An editor or owner ROLE applies only inside the collections the member's
// access reaches (TASK-3376, PR #1756 codex round 2 P1). Before it, the role
// short-circuited before the collection was looked at, so a member with
// collection_access='specific' who could SEE an item through a VIEW grant
// could edit it, comment on it and create beside it. Outside their reach the
// grant decides, through ResolveUserPermission, and only 'edit' writes.
func (s *Server) canEditInCollection(r *http.Request, workspaceID, itemID, collectionID string) (bool, error) {
role := workspaceRole(r)
if role != "guest" && requireRole(r, "editor") {
reaches, err := s.editorRoleReaches(r, workspaceID, collectionID)
if err != nil {
return false, err
}
if reaches {
return true, nil
}
}
// Guests, viewers, and editors outside their collection access: the
// grant cascade decides.
user := currentUser(r)
if user == nil {
return false, nil
}
perm, err := s.store.ResolveUserPermission(workspaceID, user.ID, itemID, collectionID)
if err != nil {
return false, err
}
return permissionLevel(perm) >= permissionLevel("edit"), nil
}
// editorRoleReaches reports whether the caller's editor/owner role applies to
// collectionID: always for a member with 'all' collection access, and for a
// restricted member only when the collection is in their
// member_collection_access. A collection or item GRANT is not "reach": it
// carries its own level, which ResolveUserPermission applies.
//
// Two principals keep the old unconditional behavior because they have no
// per-member access to consult: a user-less legacy workspace token (fixed
// editor role in its own workspace) and a platform admin on a cookie session
// (which visibleCollectionIDs also leaves unfiltered).
func (s *Server) editorRoleReaches(r *http.Request, workspaceID, collectionID string) (bool, error) {
user := currentUser(r)
if user == nil {
return true, nil
}
if user.Role == "admin" && !isBearerAuth(r) {
return true, nil
}
member, err := s.store.GetWorkspaceMember(workspaceID, user.ID)
if err != nil {
return false, err
}
if member == nil {
return false, nil
}
return s.memberRoleReachesCollection(workspaceID, member, collectionID)
}
// memberRoleReachesCollection is the member half of editorRoleReaches, shared
// with the collab socket's write decision, which has no request context.
//
// The OWNER role always reaches (PR #1756 codex round 3, lead ruling). An
// owner can set any member's collection access, their own included
// (handleSetMemberCollectionAccess is owner-gated with no self exception), so
// a restriction on an owner's writes is one click from undone. It would also
// split the server from the web client, whose canEditItem/canEditCollection
// treat owners as editors everywhere. The collection check binds the EDITOR
// role only.
func (s *Server) memberRoleReachesCollection(workspaceID string, member *models.WorkspaceMember, collectionID string) (bool, error) {
if member.Role == "owner" {
return true, nil
}
if member.CollectionAccess == "all" || member.CollectionAccess == "" {
return true, nil
}
if collectionID == "" {
return false, nil
}
listed, err := s.store.GetMemberCollectionAccess(workspaceID, member.UserID)
if err != nil {
return false, err
}
for _, id := range listed {
if id == collectionID {
return true, nil
}
}
return false, nil
}
// resolveWorkspace resolves a workspace by slug or UUID, scoped to the
// authenticated user's accessible workspaces when a user context is present.
// Returns nil (not an error) if no workspace is found.
@@ -0,0 +1,172 @@
package server
import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/models"
)
// TestTask3376_RestrictedEditorViewGrantCannotWrite pins PR #1756 codex
// round 2's P1. requireEditPermission let any editor through before it
// looked at the collection, so a member with collection_access='specific'
// who could SEE an item only through a VIEW grant could PATCH it and create
// beside it. The editor role now reaches a collection only when the member's
// access includes it ('all', or listed); anywhere else the grant decides,
// and a view grant is not an edit grant. Run for an ordinary collection and
// a system one (the case TASK-3376 newly opened). The edit-grant leg is the
// control: same member, same item, same requests, only the grant level
// differs.
func TestTask3376_RestrictedEditorViewGrantCannotWrite(t *testing.T) {
for _, tc := range []struct {
name string
system bool
}{{"ordinary", false}, {"system", true}} {
t.Run(tc.name, func(t *testing.T) {
f := newRefResolverFixture(t)
st := f.srv.store
listed, err := st.CreateCollection(f.ws.ID, models.CollectionCreate{Name: "Listed", Slug: "listed", Prefix: "LST"})
if err != nil {
t.Fatalf("CreateCollection listed: %v", err)
}
hidden, err := st.CreateCollection(f.ws.ID, models.CollectionCreate{
Name: "Hidden", Slug: "hidden", Prefix: "HID", IsSystem: tc.system,
})
if err != nil {
t.Fatalf("CreateCollection hidden: %v", err)
}
target, err := st.CreateItem(f.ws.ID, hidden.ID, models.ItemCreate{Title: "Granted item"})
if err != nil {
t.Fatalf("CreateItem: %v", err)
}
editor, err := st.CreateUser(models.UserCreate{
Email: "ed-" + tc.name + "@example.com", Name: "Ed", Username: "ed" + tc.name, Password: "pw-test-12345",
})
if err != nil {
t.Fatalf("CreateUser: %v", err)
}
if err := st.AddWorkspaceMember(f.ws.ID, editor.ID, "editor"); err != nil {
t.Fatalf("AddWorkspaceMember: %v", err)
}
if err := st.SetMemberCollectionAccess(f.ws.ID, editor.ID, "specific", []string{listed.ID}); err != nil {
t.Fatalf("SetMemberCollectionAccess: %v", err)
}
grant, err := st.CreateItemGrant(f.ws.ID, target.ID, editor.ID, "view", f.owner.ID)
if err != nil {
t.Fatalf("CreateItemGrant: %v", err)
}
tok, err := st.CreateAPIToken(editor.ID, models.APITokenCreate{Name: "ed-tok", WorkspaceID: f.ws.ID}, 0, 0)
if err != nil {
t.Fatalf("CreateAPIToken: %v", err)
}
do := func(method, path string, body interface{}) int {
t.Helper()
return f.doAuth(tok.Token, method, path, body).Code
}
itemPath := "/api/v1/workspaces/" + f.ws.Slug + "/items/" + target.Ref
createPath := "/api/v1/workspaces/" + f.ws.Slug + "/collections/" + hidden.Slug + "/items"
commentPath := itemPath + "/comments"
// Precondition: the view grant really makes the item readable.
if code := do("GET", itemPath, nil); code != http.StatusOK {
t.Fatalf("precondition: view-granted item GET = %d, want 200", code)
}
// View grant: no write of any kind.
if code := do("PATCH", itemPath, map[string]interface{}{"title": "Escalated"}); code != http.StatusForbidden && code != http.StatusNotFound {
t.Errorf("view grant: PATCH = %d, want 403/404", code)
}
if code := do("POST", createPath, map[string]interface{}{"title": "Planted"}); code != http.StatusForbidden && code != http.StatusNotFound {
t.Errorf("view grant: create in the collection = %d, want 403/404", code)
}
if code := do("POST", commentPath, map[string]interface{}{"body": "hi"}); code != http.StatusForbidden && code != http.StatusNotFound {
t.Errorf("view grant: comment = %d, want 403/404", code)
}
// Bulk: the per-item gate refuses it as forbidden, not applied.
rr := f.doAuth(tok.Token, "POST", "/api/v1/workspaces/"+f.ws.Slug+"/items/bulk",
map[string]interface{}{"ids": []string{target.ID}, "op": "archive"})
if rr.Code != http.StatusOK || !strings.Contains(rr.Body.String(), `"code":"forbidden"`) {
t.Errorf("view grant: bulk archive = %d %s, want 200 with a forbidden failure", rr.Code, rr.Body.String())
}
// Collab: admitted to read through the grant, but not to write.
collabReq := httptest.NewRequest("GET", "/api/v1/collab/"+target.ID, nil)
collabReq = collabReq.WithContext(context.WithValue(collabReq.Context(), ctxCurrentUser, editor))
if access, err := f.srv.authorizeCollabAccess(collabReq, target); err != nil || access.canWrite {
t.Errorf("view grant: collab access = %+v, %v; want admitted read-only", access, err)
}
if got, _ := st.GetItem(target.ID); got == nil || got.Title != "Granted item" || got.DeletedAt != nil {
t.Errorf("view grant: item changed: %+v", got)
}
// Control: an EDIT grant on the same item permits the edit.
if err := st.DeleteItemGrant(grant.ID, f.ws.ID); err != nil {
t.Fatalf("DeleteItemGrant: %v", err)
}
if _, err := st.CreateItemGrant(f.ws.ID, target.ID, editor.ID, "edit", f.owner.ID); err != nil {
t.Fatalf("CreateItemGrant edit: %v", err)
}
if code := do("PATCH", itemPath, map[string]interface{}{"title": "Edited"}); code != http.StatusOK {
t.Errorf("edit grant: PATCH = %d, want 200", code)
}
collabReq2 := httptest.NewRequest("GET", "/api/v1/collab/"+target.ID, nil)
collabReq2 = collabReq2.WithContext(context.WithValue(collabReq2.Context(), ctxCurrentUser, editor))
if access, err := f.srv.authorizeCollabAccess(collabReq2, target); err != nil || !access.canWrite {
t.Errorf("edit grant: collab access = %+v, %v; want writable", access, err)
}
})
}
}
// A restricted member with the OWNER role keeps the full edit bypass (PR #1756
// codex round 3, lead ruling): an owner can rewrite their own collection
// access, and the web client's canEditItem treats owners as editors
// everywhere, so the server must agree. Same shape as the editor test above,
// where the same request is refused: only the role differs.
func TestTask3376_RestrictedOwnerRoleKeepsEdit(t *testing.T) {
f := newRefResolverFixture(t)
st := f.srv.store
listed, err := st.CreateCollection(f.ws.ID, models.CollectionCreate{Name: "Listed", Slug: "listed", Prefix: "LST"})
if err != nil {
t.Fatalf("CreateCollection listed: %v", err)
}
hidden, err := st.CreateCollection(f.ws.ID, models.CollectionCreate{Name: "Hidden", Slug: "hidden", Prefix: "HID"})
if err != nil {
t.Fatalf("CreateCollection hidden: %v", err)
}
target, err := st.CreateItem(f.ws.ID, hidden.ID, models.ItemCreate{Title: "Granted item"})
if err != nil {
t.Fatalf("CreateItem: %v", err)
}
co, err := st.CreateUser(models.UserCreate{Email: "co@example.com", Name: "Co", Username: "coowner", Password: "pw-test-12345"})
if err != nil {
t.Fatalf("CreateUser: %v", err)
}
// Role owner, but NOT workspaces.owner_id, so ResolveUserPermission's
// owner_id step does not apply and the role path is what is measured.
if err := st.AddWorkspaceMember(f.ws.ID, co.ID, "owner"); err != nil {
t.Fatalf("AddWorkspaceMember: %v", err)
}
if err := st.SetMemberCollectionAccess(f.ws.ID, co.ID, "specific", []string{listed.ID}); err != nil {
t.Fatalf("SetMemberCollectionAccess: %v", err)
}
if _, err := st.CreateItemGrant(f.ws.ID, target.ID, co.ID, "view", f.owner.ID); err != nil {
t.Fatalf("CreateItemGrant: %v", err)
}
tok, err := st.CreateAPIToken(co.ID, models.APITokenCreate{Name: "co-tok", WorkspaceID: f.ws.ID}, 0, 0)
if err != nil {
t.Fatalf("CreateAPIToken: %v", err)
}
if rr := f.doAuth(tok.Token, "PATCH", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+target.Ref, map[string]interface{}{"title": "Owner edit"}); rr.Code != http.StatusOK {
t.Errorf("restricted owner role: PATCH = %d, want 200; body=%s", rr.Code, rr.Body.String())
}
collabReq := httptest.NewRequest("GET", "/api/v1/collab/"+target.ID, nil)
collabReq = collabReq.WithContext(context.WithValue(collabReq.Context(), ctxCurrentUser, co))
if access, err := f.srv.authorizeCollabAccess(collabReq, target); err != nil || !access.canWrite {
t.Errorf("restricted owner role: collab access = %+v, %v; want writable", access, err)
}
}
@@ -0,0 +1,109 @@
package server
import (
"net/http"
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/models"
)
// TestTask3376_SystemCollectionSurfaces drives the surfaces that each carried
// their own "system collections are always visible to restricted members"
// union until TASK-3376: item read, search, and the /me permission payload
// (visible and full-access sets). A restricted member who is not given the
// system collection must get nothing from it on any of them; once it is
// listed, every one of them answers. The listed leg is the control: the same
// requests, the same item, only the listing differs.
func TestTask3376_SystemCollectionSurfaces(t *testing.T) {
f := newRefResolverFixture(t)
st := f.srv.store
alpha, err := st.CreateCollection(f.ws.ID, models.CollectionCreate{Name: "Alpha", Slug: "alpha", Prefix: "ALPHA"})
if err != nil {
t.Fatalf("CreateCollection alpha: %v", err)
}
sysColl, err := st.CreateCollection(f.ws.ID, models.CollectionCreate{
Name: "Conventions", Slug: "conventions", Prefix: "CONV", IsSystem: true,
})
if err != nil {
t.Fatalf("CreateCollection (system): %v", err)
}
sysItem, err := st.CreateItem(f.ws.ID, sysColl.ID, models.ItemCreate{Title: "Zebracorn rule"})
if err != nil {
t.Fatalf("CreateItem: %v", err)
}
member, err := st.CreateUser(models.UserCreate{
Email: "m3376@example.com", Name: "M", Username: "m3376", Password: "pw-test-12345",
})
if err != nil {
t.Fatalf("CreateUser: %v", err)
}
if err := st.AddWorkspaceMember(f.ws.ID, member.ID, "editor"); err != nil {
t.Fatalf("AddWorkspaceMember: %v", err)
}
tok, err := st.CreateAPIToken(member.ID, models.APITokenCreate{Name: "m-tok", WorkspaceID: f.ws.ID}, 0, 0)
if err != nil {
t.Fatalf("CreateAPIToken: %v", err)
}
get := func(path string) (int, string) {
t.Helper()
rr := f.doAuth(tok.Token, "GET", path, nil)
return rr.Code, rr.Body.String()
}
itemPath := "/api/v1/workspaces/" + f.ws.Slug + "/items/" + sysItem.Ref
searchPath := "/api/v1/search?q=Zebracorn&workspace=" + f.ws.Slug
mePath := "/api/v1/workspaces/" + f.ws.Slug + "/me"
type meBody struct {
Visible []string `json:"visible_collection_ids"`
Full []string `json:"full_access_collection_ids"`
}
has := func(ids []string, id string) bool {
for _, x := range ids {
if x == id {
return true
}
}
return false
}
readMe := func() meBody {
t.Helper()
rr := f.doAuth(tok.Token, "GET", mePath, nil)
if rr.Code != http.StatusOK {
t.Fatalf("/me: %d %s", rr.Code, rr.Body.String())
}
var m meBody
parseJSON(t, rr, &m)
return m
}
// --- Unlisted ---
if err := st.SetMemberCollectionAccess(f.ws.ID, member.ID, "specific", []string{alpha.ID}); err != nil {
t.Fatalf("SetMemberCollectionAccess: %v", err)
}
if code, body := get(itemPath); code != http.StatusNotFound {
t.Errorf("unlisted: item GET = %d, want 404; body=%s", code, body)
}
if code, body := get(searchPath); code != http.StatusOK || strings.Contains(body, sysItem.ID) {
t.Errorf("unlisted: search returned the system item (code %d): %s", code, body)
}
if m := readMe(); has(m.Visible, sysColl.ID) || has(m.Full, sysColl.ID) {
t.Errorf("unlisted: /me still lists the system collection: visible=%v full=%v", m.Visible, m.Full)
}
// --- Listed (control) ---
if err := st.SetMemberCollectionAccess(f.ws.ID, member.ID, "specific", []string{alpha.ID, sysColl.ID}); err != nil {
t.Fatalf("SetMemberCollectionAccess (listed): %v", err)
}
if code, body := get(itemPath); code != http.StatusOK {
t.Errorf("listed: item GET = %d, want 200; body=%s", code, body)
}
if code, body := get(searchPath); code != http.StatusOK || !strings.Contains(body, sysItem.ID) {
t.Errorf("listed: search did not return the system item (code %d): %s", code, body)
}
if m := readMe(); !has(m.Visible, sysColl.ID) || !has(m.Full, sysColl.ID) {
t.Errorf("listed: /me does not list the system collection: visible=%v full=%v", m.Visible, m.Full)
}
}
+3 -9
View File
@@ -137,8 +137,9 @@ func (s *Store) ResolveBacklinksVisibilityQ(q Queryer, userID, workspaceID strin
}
// Restricted member branch — merge grant collections with
// member_collection_access + system collections. Same as lines
// 1798-1825 of server.guestResourceFilterCore.
// member_collection_access. System collections are not added: a
// restricted member reaches one only when it is listed or granted
// (TASK-3376).
fullCollSet := make(map[string]bool)
for _, id := range grantCollIDs {
fullCollSet[id] = true
@@ -150,13 +151,6 @@ func (s *Store) ResolveBacklinksVisibilityQ(q Queryer, userID, workspaceID strin
for _, id := range memberColls {
fullCollSet[id] = true
}
sysColls, err := s.ListSystemCollectionIDsQ(q, workspaceID)
if err != nil {
return nil, nil, err
}
for _, id := range sysColls {
fullCollSet[id] = true
}
fullCollIDs = make([]string, 0, len(fullCollSet))
for id := range fullCollSet {
fullCollIDs = append(fullCollIDs, id)
+13 -7
View File
@@ -391,21 +391,27 @@ func (s *Store) ResolveUserPermission(workspaceID, userID, itemID, collectionID
return "", fmt.Errorf("check membership: %w", err)
}
if member != nil {
// Check collection visibility for members with "specific" access
// A restricted member's ROLE applies only to a collection in their
// member_collection_access (TASK-3376, PR #1756 codex round 2 P1).
// This used to consult VisibleCollectionIDs, the NAVIGATION set, which
// also lists a collection because one item in it is granted, so a
// view grant on one item handed the member their editor role over
// the whole collection. Grants are resolved above with their own
// levels and are not reach for the role.
if collectionID != "" && member.CollectionAccess == "specific" {
visIDs, err := s.VisibleCollectionIDs(workspaceID, userID)
listed, err := s.GetMemberCollectionAccess(workspaceID, userID)
if err != nil {
return "", err
}
visible := false
for _, id := range visIDs {
reach := false
for _, id := range listed {
if id == collectionID {
visible = true
reach = true
break
}
}
if !visible {
return "", nil // Collection not visible → deny
if !reach {
return "", nil // role does not reach this collection → deny
}
}
return member.Role, nil // "owner", "editor", or "viewer"
@@ -0,0 +1,19 @@
-- Migration 108: list system collections for restricted members (TASK-3376).
--
-- Until TASK-3376 every member with collection_access = 'specific' saw every
-- system collection (conventions, playbooks) implicitly, and an editor among
-- them could write it. They are now ordinary collections: a restricted member
-- reaches one only when it is in member_collection_access or granted.
--
-- So that the upgrade changes nobody's access, this lists every live system
-- collection for every member already restricted, which is exactly what each
-- of them could see before. Owners can then remove them per member. A system
-- collection created after this migration is NOT listed for existing
-- restricted members; that is the new rule, stated in the release notes.
INSERT OR IGNORE INTO member_collection_access (workspace_id, user_id, collection_id, created_at)
SELECT wm.workspace_id, wm.user_id, c.id, strftime('%Y-%m-%dT%H:%M:%SZ', 'now')
FROM workspace_members wm
JOIN collections c ON c.workspace_id = wm.workspace_id
WHERE wm.collection_access = 'specific'
AND c.is_system = 1
AND c.deleted_at IS NULL;
+29 -14
View File
@@ -110,7 +110,10 @@ func TestVisibleCollectionIDs_SpecificAccess(t *testing.T) {
}
}
func TestVisibleCollectionIDs_SystemCollectionsAlwaysVisible(t *testing.T) {
// System collections are ordinary collections for a restricted member: seen
// only when listed (TASK-3376). The listed leg is the control, so the
// unlisted absence is about the listing and not the fixture.
func TestVisibleCollectionIDs_SystemCollectionsOnlyWhenListed(t *testing.T) {
t.Parallel()
s, ws, _, member := setupPermissionTest(t)
@@ -124,23 +127,35 @@ func TestVisibleCollectionIDs_SystemCollectionsAlwaysVisible(t *testing.T) {
conventionsID = c.ID
}
}
// Set member to specific access with only "tasks"
_ = s.SetMemberCollectionAccess(ws.ID, member.ID, "specific", []string{tasksID})
ids, err := s.VisibleCollectionIDs(ws.ID, member.ID)
if err != nil {
t.Fatalf("VisibleCollectionIDs: %v", err)
if tasksID == "" || conventionsID == "" {
t.Fatalf("fixture lacks tasks or conventions: tasks=%q conventions=%q", tasksID, conventionsID)
}
idSet := make(map[string]bool)
for _, id := range ids {
idSet[id] = true
visible := func() map[string]bool {
t.Helper()
ids, err := s.VisibleCollectionIDs(ws.ID, member.ID)
if err != nil {
t.Fatalf("VisibleCollectionIDs: %v", err)
}
set := make(map[string]bool)
for _, id := range ids {
set[id] = true
}
return set
}
// Conventions is system → always visible even though not explicitly granted
if !idSet[conventionsID] {
t.Error("expected conventions (system) collection to always be visible")
if err := s.SetMemberCollectionAccess(ws.ID, member.ID, "specific", []string{tasksID}); err != nil {
t.Fatalf("SetMemberCollectionAccess: %v", err)
}
if visible()[conventionsID] {
t.Error("an unlisted system collection is visible to a restricted member")
}
if err := s.SetMemberCollectionAccess(ws.ID, member.ID, "specific", []string{tasksID, conventionsID}); err != nil {
t.Fatalf("SetMemberCollectionAccess (listed): %v", err)
}
if !visible()[conventionsID] {
t.Error("a listed system collection is not visible to a restricted member")
}
}
@@ -0,0 +1,10 @@
-- Migration 082: list system collections for restricted members (TASK-3376).
-- Mirrors SQLite migration 108; see it for the rationale.
INSERT INTO member_collection_access (workspace_id, user_id, collection_id, created_at)
SELECT wm.workspace_id, wm.user_id, c.id, to_char(now() AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"')
FROM workspace_members wm
JOIN collections c ON c.workspace_id = wm.workspace_id
WHERE wm.collection_access = 'specific'
AND c.is_system = TRUE
AND c.deleted_at IS NULL
ON CONFLICT DO NOTHING;
@@ -0,0 +1,133 @@
package store
import (
"sort"
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/models"
)
// TestSystemCollectionAccessBackfill runs migration 108 (SQLite) / 082
// (Postgres) against a manufactured pre-upgrade state (TASK-3376).
//
// Before TASK-3376 a restricted member saw every system collection without a
// member_collection_access row. The migration lists them, so the upgrade
// changes nobody's access. Ordinary tests cannot reach it: migrations run
// before any member exists. Runs on whichever dialect testStore provides.
func TestSystemCollectionAccessBackfill(t *testing.T) {
t.Parallel()
s := testStore(t)
ws := createTestWorkspace(t, s, "Backfill 3376")
if err := s.SeedCollectionsFromTemplate(ws.ID, "startup"); err != nil {
t.Fatalf("seed: %v", err)
}
coll := func(slug string) *models.Collection {
t.Helper()
c, err := s.GetCollectionBySlug(ws.ID, slug)
if err != nil || c == nil {
t.Fatalf("no %s collection: %v", slug, err)
}
return c
}
conv, pb, tasks := coll("conventions"), coll("playbooks"), coll("tasks")
// A system collection that was soft-deleted must not be listed.
gone, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Old System", IsSystem: true})
if err != nil {
t.Fatalf("create system collection: %v", err)
}
if err := s.DeleteCollection(gone.ID, ""); err != nil {
t.Fatalf("soft-delete: %v", err)
}
member := func(email, mode string, ids []string) string {
t.Helper()
u := createTestUser(t, s, email, email, "password123")
if err := s.AddWorkspaceMember(ws.ID, u.ID, "editor"); err != nil {
t.Fatalf("add member: %v", err)
}
if err := s.SetMemberCollectionAccess(ws.ID, u.ID, mode, ids); err != nil {
t.Fatalf("set access: %v", err)
}
return u.ID
}
restricted := member("restricted@example.com", "specific", []string{tasks.ID})
// Already lists conventions: the backfill must not fail on the conflict.
partly := member("partly@example.com", "specific", []string{tasks.ID, conv.ID})
unrestricted := member("all@example.com", "all", nil)
// Control: the visibility change is in effect, so the restricted member
// does NOT see the system collections before the backfill. Without this
// the assertions below would pass against the old implicit union.
vis, err := s.VisibleCollectionIDs(ws.ID, restricted)
if err != nil {
t.Fatalf("visible: %v", err)
}
for _, id := range vis {
if id == conv.ID || id == pb.ID {
t.Fatalf("control failed: restricted member sees system collection %s before the backfill", id)
}
}
runSystemAccessBackfill(t, s)
runSystemAccessBackfill(t, s) // idempotent
want := func(userID string, ids ...string) {
t.Helper()
got, err := s.GetMemberCollectionAccess(ws.ID, userID)
if err != nil {
t.Fatalf("get access: %v", err)
}
sort.Strings(got)
sort.Strings(ids)
if strings.Join(got, ",") != strings.Join(ids, ",") {
t.Errorf("member %s access = %v, want %v", userID, got, ids)
}
}
want(restricted, tasks.ID, conv.ID, pb.ID)
want(partly, tasks.ID, conv.ID, pb.ID)
want(unrestricted) // 'all' members get no rows
vis, err = s.VisibleCollectionIDs(ws.ID, restricted)
if err != nil {
t.Fatalf("visible after: %v", err)
}
seen := map[string]bool{}
for _, id := range vis {
seen[id] = true
}
if !seen[conv.ID] || !seen[pb.ID] {
t.Errorf("after backfill the restricted member should see conventions and playbooks; visible = %v", vis)
}
if seen[gone.ID] {
t.Errorf("a soft-deleted system collection was listed")
}
}
func runSystemAccessBackfill(t *testing.T, s *Store) {
t.Helper()
name, dir := "migrations/108_system_collection_access_backfill.sql", migrationsFS
if s.dialect.Driver() == DriverPostgres {
name = "pgmigrations/082_system_collection_access_backfill.sql"
dir = pgMigrationsFS
}
raw, err := dir.ReadFile(name)
if err != nil {
t.Fatalf("read migration %s: %v", name, err)
}
var sb strings.Builder
for _, line := range strings.Split(string(raw), "\n") {
if strings.HasPrefix(strings.TrimSpace(line), "--") {
continue
}
sb.WriteString(line + "\n")
}
stmt := strings.TrimSpace(sb.String())
if !strings.HasPrefix(strings.ToUpper(stmt), "INSERT") {
t.Fatalf("migration %s no longer starts with its INSERT; update this test", name)
}
if _, err := s.db.Exec(stmt); err != nil {
t.Fatalf("backfill failed: %v\n%s", err, stmt)
}
}
+7 -53
View File
@@ -317,7 +317,10 @@ func (s *Store) ListWorkspaceMembers(workspaceID string) ([]models.WorkspaceMemb
// VisibleCollectionIDs returns the set of collection IDs a member can see.
// Returns nil if the member has "all" access (meaning no filtering needed).
// System collections (conventions, playbooks) are always included for members.
// System collections (conventions, playbooks) are ordinary collections here:
// a restricted member sees one only when it is in their
// member_collection_access or granted (TASK-3376). Migration 108 listed them
// for every member restricted before that change.
func (s *Store) VisibleCollectionIDs(workspaceID, userID string) ([]string, error) {
return s.VisibleCollectionIDsQ(s.db, workspaceID, userID)
}
@@ -339,7 +342,7 @@ func (s *Store) VisibleCollectionIDsQ(q Queryer, workspaceID, userID string) ([]
return nil, nil
}
// "specific" access — get the granted collection IDs + system collections
// "specific" access — get the granted collection IDs
rows, err := q.Query(s.q(`
SELECT collection_id FROM member_collection_access
WHERE workspace_id = ? AND user_id = ?
@@ -361,24 +364,6 @@ func (s *Store) VisibleCollectionIDsQ(q Queryer, workspaceID, userID string) ([]
return nil, err
}
// Always include system collections for members
sysRows, err := q.Query(s.q(`
SELECT id FROM collections
WHERE workspace_id = ? AND is_system = ? AND deleted_at IS NULL
`), workspaceID, s.dialect.BoolToInt(true))
if err != nil {
return nil, fmt.Errorf("get system collections: %w", err)
}
defer sysRows.Close()
for sysRows.Next() {
var id string
if err := sysRows.Scan(&id); err != nil {
return nil, err
}
ids[id] = true
}
// Also include collections from direct collection grants. This ensures
// that members with "specific" access who are granted additional
// collections can see them even if they aren't in member_collection_access.
@@ -531,35 +516,6 @@ func (s *Store) GetMemberCollectionAccessQ(q Queryer, workspaceID, userID string
return ids, rows.Err()
}
// ListSystemCollectionIDs returns the IDs of system collections in a workspace.
// System collections are always visible to members regardless of collection_access mode.
func (s *Store) ListSystemCollectionIDs(workspaceID string) ([]string, error) {
return s.ListSystemCollectionIDsQ(s.db, workspaceID)
}
// ListSystemCollectionIDsQ is ListSystemCollectionIDs parameterized over its
// executor (see Queryer).
func (s *Store) ListSystemCollectionIDsQ(q Queryer, workspaceID string) ([]string, error) {
rows, err := q.Query(s.q(`
SELECT id FROM collections
WHERE workspace_id = ? AND is_system = ? AND deleted_at IS NULL
`), workspaceID, s.dialect.BoolToInt(true))
if err != nil {
return nil, fmt.Errorf("list system collections: %w", err)
}
defer rows.Close()
var ids []string
for rows.Next() {
var id string
if err := rows.Scan(&id); err != nil {
return nil, err
}
ids = append(ids, id)
}
return ids, rows.Err()
}
// GetUserMemberWorkspaces returns only workspaces where the user has
// a workspace_members row — NO guest-grant fallback. Mirrors the
// first half of GetUserWorkspaces but without the UNION with
@@ -621,8 +577,7 @@ func (s *Store) GetUserWorkspaces(userID string) ([]models.Workspace, error) {
// effective UpdatedAt reflects item activity, not just row mtime.
// For members with collection_access='specific', restrict MAX to
// items in collections the member can actually see (via
// member_collection_access, system collections, collection_grants,
// or item_grants) — otherwise the freshness signal leaks activity
// member_collection_access, collection_grants, or item_grants) — otherwise the freshness signal leaks activity
// for collections the member doesn't have access to. Members with
// 'all' (or empty) access see every collection, so the predicate
// short-circuits and behaves like an unrestricted MAX. The
@@ -639,7 +594,6 @@ func (s *Store) GetUserWorkspaces(userID string) ([]models.Workspace, error) {
AND c.deleted_at IS NULL
AND (
COALESCE(wm.collection_access, '') IN ('', 'all')
OR c.is_system = ?
OR EXISTS (
SELECT 1 FROM member_collection_access mca
WHERE mca.workspace_id = w.id
@@ -665,7 +619,7 @@ func (s *Store) GetUserWorkspaces(userID string) ([]models.Workspace, error) {
LEFT JOIN users ou ON ou.id = w.owner_id
WHERE wm.user_id = ? AND w.deleted_at IS NULL
ORDER BY wm.sort_order ASC, w.name ASC
`), s.dialect.BoolToInt(true), userID)
`), userID)
if err != nil {
return nil, fmt.Errorf("get user workspaces: %w", err)
}
+30 -6
View File
@@ -23,6 +23,12 @@ import {
* PATCH answers for Delete too. The PATCH writes the value the item already
* holds, so the probe does not move the world the legs then read.
*
* editorSpecific is restricted to Tasks, so since TASK-3376 it does not see
* the system collections at all (they are ordinary collections for a
* restricted member, reached only when listed). Its probe answers 404, the
* item legs below do not apply to it, and a leg of their own pins that
* contract instead.
*
* Its own spec, with its own world, because granting the guests a convention
* and a playbook would take the playbooks empty state away from the guests in
* permission-walk-system.spec.ts, which walks that state's create door.
@@ -45,6 +51,9 @@ let walk: PermissionWalk;
const slugs = {} as Record<Sys, string>;
// What the server answered to a status PATCH on each seeded item, per account.
const serverEdits: Record<string, Record<Sys, boolean>> = {};
// Accounts the server hides the seeded items from (404). TASK-3376: exactly
// editorSpecific, whose collection access lists Tasks only.
const HIDDEN: ReadonlySet<AccountKey> = new Set(['editorSpecific']);
test.beforeAll(async () => {
walk = await seedPermissionWalk();
@@ -94,10 +103,12 @@ test.beforeAll(async () => {
const res = await api.patch(`/api/v1/workspaces/${walk.workspaceSlug}/items/${slugs[coll]}`, {
data: { fields_patch: { status: 'draft' } }
});
// Every account can see both items (members by role, guests by
// grant), so 200 or 403 are the only answers this walk reasons
// about; anything else means the probe itself is broken.
expect([200, 403], `${key} PATCH ${coll}: ${res.status()} ${await res.text()}`).toContain(res.status());
// Every account but the HIDDEN ones can see both items (members
// by role and collection access, guests by grant), so 200 or 403
// are the answers this walk reasons about; a HIDDEN account must
// get 404 (TASK-3376). Anything else means the probe is broken.
const allowed = HIDDEN.has(key) ? [404] : [200, 403];
expect(allowed, `${key} PATCH ${coll}: ${res.status()} ${await res.text()}`).toContain(res.status());
serverEdits[key][coll] = res.status() === 200;
}
await api.dispose();
@@ -106,7 +117,7 @@ test.beforeAll(async () => {
// tell a per-item gate from a role constant.
const guests: AccountKey[] = ['guestItemEdit', 'guestPrecedence'];
for (const coll of ['conventions', 'playbooks'] as const) {
for (const group of [ACCOUNT_KEYS.filter((k) => !guests.includes(k)), guests]) {
for (const group of [ACCOUNT_KEYS.filter((k) => !guests.includes(k) && !HIDDEN.has(k)), guests]) {
const answers = new Set(group.map((k) => serverEdits[k][coll]));
expect(answers.size, `server answers for ${coll} over ${group}: ${JSON.stringify(serverEdits)}`).toBe(2);
}
@@ -120,7 +131,20 @@ async function open(page: Page, key: AccountKey, path: string) {
await waitForAccessSettled(page);
}
for (const key of ACCOUNT_KEYS) {
// TASK-3376: a member restricted to Tasks does not see the system collections
// unless they are listed for it. The beforeAll probe already required 404 on
// both seeded items; this leg pins that the pages agree and show it nothing.
test('editorSpecific: the system collections are not in its reach', async ({ page }) => {
for (const coll of ['conventions', 'playbooks'] as const) {
expect(serverEdits.editorSpecific[coll], `editorSpecific can edit ${coll}`).toBe(false);
}
await open(page, 'editorSpecific', '/conventions');
await expect(page.getByText(TITLE.conventions, { exact: true })).toHaveCount(0);
await open(page, 'editorSpecific', '/playbooks');
await expect(page.getByText(TITLE.playbooks, { exact: true })).toHaveCount(0);
});
for (const key of ACCOUNT_KEYS.filter((k) => !HIDDEN.has(k))) {
test.describe(key, () => {
test('Conventions: toggle, Enable all, Edit and Delete render iff the server lets the account edit the item', async ({
page
@@ -703,6 +703,17 @@
}
}
// TASK-3376: system collections are ordinary choices for a restricted
// member. Moving a member from All to Specific starts with them checked,
// so restricting someone does not silently stop their agents loading the
// workspace's rules. A member already restricted keeps their saved list.
function setAccessMode(next: 'all' | 'specific') {
if (next === 'specific' && accessMode !== 'specific' && accessCollectionIds.length === 0) {
accessCollectionIds = systemCollections.map((c) => c.id);
}
accessMode = next;
}
function toggleAccessCollection(collId: string) {
if (accessCollectionIds.includes(collId)) {
accessCollectionIds = accessCollectionIds.filter(id => id !== collId);
@@ -1042,7 +1053,7 @@
id="access-mode-{member.user_id}"
class="role-select"
value={accessMode}
onchange={(e) => { accessMode = (e.target as HTMLSelectElement).value as 'all' | 'specific'; }}
onchange={(e) => { setAccessMode((e.target as HTMLSelectElement).value as 'all' | 'specific'); }}
>
<option value="all">All collections</option>
<option value="specific">Specific collections</option>
@@ -1062,13 +1073,22 @@
</label>
{/each}
{#each systemCollections as coll (coll.id)}
<label class="access-coll-item access-coll-system" title="System collection — always visible">
<input type="checkbox" checked disabled />
<label class="access-coll-item access-coll-system">
<input
type="checkbox"
checked={accessCollectionIds.includes(coll.id)}
onchange={() => toggleAccessCollection(coll.id)}
/>
<span class="access-coll-icon">{coll.icon || '#'}</span>
<span class="access-coll-name">{coll.name}</span>
<Chip size="sm" color="var(--accent-gray)">system</Chip>
</label>
{/each}
{#if systemCollections.length > 0}
<p class="access-coll-hint">
{systemCollections.map((c) => c.name).join(' and ')} hold the rules and procedures agents load. Unchecked, this member's agents won't receive them.
</p>
{/if}
</div>
{/if}
<div class="access-actions">
@@ -1387,8 +1407,7 @@
.access-coll-item input[type="checkbox"] { margin: 0; cursor: pointer; }
.access-coll-icon { font-size: 0.95em; }
.access-coll-name { flex: 1; }
.access-coll-system { opacity: 0.6; cursor: default; }
.access-coll-system:hover { background: none; }
.access-coll-hint { margin: var(--space-1) var(--space-2) 0; font-size: 0.8em; color: var(--text-muted); }
.access-actions { display: flex; align-items: center; gap: var(--space-2); }
.invitations-section { margin-top: var(--space-4); }
.invitations-section h3 { font-size: 0.9em; color: var(--text-muted); margin-bottom: var(--space-2); }
@@ -0,0 +1,142 @@
import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest';
import { cleanup, render, screen, waitFor } from '@testing-library/svelte';
import { page } from '$app/state';
import SettingsPage from './+page.svelte';
/**
* TASK-3376: system collections (Conventions, Playbooks) are ordinary
* collections for a restricted member. In the member access picker they are
* real checkboxes, not a disabled always-checked row, and they start CHECKED
* when an owner switches a member from All to Specific, so that restricting
* someone does not silently stop their agents loading the workspace's rules.
* A member who is ALREADY restricted keeps their saved list as is.
*/
const meCalls: Array<(value: unknown) => void> = [];
const saves: Array<{ mode: string; ids: string[] }> = [];
let memberAccess: { collection_access: string; collection_ids: string[] } = { collection_access: 'all', collection_ids: [] };
const COLLECTIONS = [
{ id: 'c-tasks', slug: 'tasks', name: 'Tasks', icon: 'T', is_system: false, is_default: true, sort_order: 0 },
{ id: 'c-conv', slug: 'conventions', name: 'Conventions', icon: 'C', is_system: true, is_default: true, sort_order: 1 },
{ id: 'c-pb', slug: 'playbooks', name: 'Playbooks', icon: 'P', is_system: true, is_default: true, sort_order: 2 },
];
vi.mock('$lib/stores/toast.svelte', () => ({
toastStore: { show: () => 'toast-id', dismiss: () => {}, get toasts() { return []; } },
quietExternalToasts: () => false,
}));
vi.mock('$lib/api/client', () => ({
api: {
workspaces: {
get: vi.fn(async () => ({ id: 'ws1', slug: 'ws', name: 'WS', context: {} })),
me: vi.fn(() => new Promise((resolve) => { meCalls.push(resolve); })),
list: vi.fn(async () => []),
update: vi.fn(async () => ({})),
delete: vi.fn(async () => ({})),
restore: vi.fn(async () => ({ slug: 'ws', name: 'WS' })),
},
collections: { list: vi.fn(async () => COLLECTIONS) },
members: {
list: vi.fn(async () => ({
members: [{ user_id: 'u2', user_name: 'Bob', user_email: 'bob@example.com', role: 'editor' }],
invitations: [],
})),
getMemberCollectionAccess: vi.fn(async () => memberAccess),
setMemberCollectionAccess: vi.fn(async (_ws: string, _u: string, mode: string, ids: string[]) => {
saves.push({ mode, ids: [...ids] });
return { collection_access: mode, collection_ids: ids };
}),
remove: vi.fn(async () => ({})),
},
},
isPlanLimitError: () => false,
planLimitMessage: () => '',
PadApiError: class extends Error {},
}));
vi.mock('$lib/services/sse.svelte', () => ({
sseService: { onItemEvent: () => () => {} },
}));
vi.mock('$app/navigation', () => ({ goto: () => Promise.resolve() }));
vi.mock('$lib/stores/auth.svelte', () => ({
authStore: {
get identityEpoch() { return 0; },
get userId() { return 'u1'; },
identityFence() { return () => true; },
onIdentityChange() { return () => {}; },
clear() {},
},
}));
const OWNER = { role: 'owner', collection_grants: [], item_grants: [] };
function checkbox(name: string): HTMLInputElement {
const label = screen.getByText(name, { selector: '.access-coll-name' }).closest('label');
expect(label, `no access row for ${name}`).not.toBeNull();
return label!.querySelector('input[type="checkbox"]') as HTMLInputElement;
}
function setAccessMode(next: 'all' | 'specific') {
const select = document.querySelector('select[id^="access-mode-"]') as HTMLSelectElement;
expect(select, 'the access panel is not open').not.toBeNull();
select.value = next;
select.dispatchEvent(new Event('change', { bubbles: true }));
}
async function openAccessPanel() {
render(SettingsPage);
await waitFor(() => expect(meCalls.length).toBeGreaterThan(0));
meCalls[0]!(OWNER);
await waitFor(() => expect(screen.getByRole('tab', { name: /Members/ })).toBeTruthy());
screen.getByRole('tab', { name: /Members/ }).click();
await waitFor(() => expect(screen.getByText('Bob')).toBeTruthy());
screen.getByRole('button', { name: /Manage access/ }).click();
await waitFor(() => expect(document.querySelector('select[id^="access-mode-"]')).not.toBeNull());
}
describe('TASK-3376: system collections in the member access picker', () => {
beforeEach(() => {
meCalls.length = 0;
saves.length = 0;
memberAccess = { collection_access: 'all', collection_ids: [] };
page.params = { username: 'dave', workspace: 'ws' };
window.location.hash = '';
});
afterEach(() => {
cleanup();
window.location.hash = '';
});
it('All to Specific starts with the system collections checked, and they can be unchecked', async () => {
await openAccessPanel();
setAccessMode('specific');
await waitFor(() => expect(checkbox('Conventions').checked).toBe(true));
expect(checkbox('Playbooks').checked).toBe(true);
expect(checkbox('Tasks').checked).toBe(false);
// Real choices, not the old disabled always-checked row.
expect(checkbox('Conventions').disabled).toBe(false);
expect(checkbox('Playbooks').disabled).toBe(false);
checkbox('Conventions').click();
await waitFor(() => expect(checkbox('Conventions').checked).toBe(false));
screen.getByRole('button', { name: /^Save$/ }).click();
await waitFor(() => expect(saves.length).toBe(1));
expect(saves[0]).toEqual({ mode: 'specific', ids: ['c-pb'] });
});
it('an already-restricted member keeps their saved list; nothing is added for them', async () => {
memberAccess = { collection_access: 'specific', collection_ids: ['c-tasks'] };
await openAccessPanel();
await waitFor(() => expect(checkbox('Tasks').checked).toBe(true));
expect(checkbox('Conventions').checked).toBe(false);
expect(checkbox('Playbooks').checked).toBe(false);
screen.getByRole('button', { name: /^Save$/ }).click();
await waitFor(() => expect(saves.length).toBe(1));
expect(saves[0]).toEqual({ mode: 'specific', ids: ['c-tasks'] });
});
});