diff --git a/internal/server/authz_cross_workspace.go b/internal/server/authz_cross_workspace.go index 06fdb8180..0d7f180ac 100644 --- a/internal/server/authz_cross_workspace.go +++ b/internal/server/authz_cross_workspace.go @@ -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 { diff --git a/internal/server/authz_cross_workspace_test.go b/internal/server/authz_cross_workspace_test.go index 2d20713bf..d7c26a0f8 100644 --- a/internal/server/authz_cross_workspace_test.go +++ b/internal/server/authz_cross_workspace_test.go @@ -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") +} diff --git a/internal/server/handlers_collab.go b/internal/server/handlers_collab.go index 9624904be..fe95770bc 100644 --- a/internal/server/handlers_collab.go +++ b/internal/server/handlers_collab.go @@ -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) diff --git a/internal/server/handlers_events.go b/internal/server/handlers_events.go index f35af4352..94e343622 100644 --- a/internal/server/handlers_events.go +++ b/internal/server/handlers_events.go @@ -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 { diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index e9c5fbec2..7dbc66d3f 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -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"} } diff --git a/internal/server/handlers_me.go b/internal/server/handlers_me.go index 26d39a4fd..88a848f80 100644 --- a/internal/server/handlers_me.go +++ b/internal/server/handlers_me.go @@ -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{}{} } diff --git a/internal/server/handlers_push.go b/internal/server/handlers_push.go index dd0cc9256..0fdc7efaa 100644 --- a/internal/server/handlers_push.go +++ b/internal/server/handlers_push.go @@ -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 diff --git a/internal/server/handlers_push_store_error_test.go b/internal/server/handlers_push_store_error_test.go index 7d266116a..21293703a 100644 --- a/internal/server/handlers_push_store_error_test.go +++ b/internal/server/handlers_push_store_error_test.go @@ -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 diff --git a/internal/server/handlers_ref_resolver_test.go b/internal/server/handlers_ref_resolver_test.go index fd0d99814..3154f3823 100644 --- a/internal/server/handlers_ref_resolver_test.go +++ b/internal/server/handlers_ref_resolver_test.go @@ -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+"/") { diff --git a/internal/server/handlers_search.go b/internal/server/handlers_search.go index 13b82b5d3..bbccba8cd 100644 --- a/internal/server/handlers_search.go +++ b/internal/server/handlers_search.go @@ -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 diff --git a/internal/server/handlers_watch_access.go b/internal/server/handlers_watch_access.go index 8d91a1de2..689108218 100644 --- a/internal/server/handlers_watch_access.go +++ b/internal/server/handlers_watch_access.go @@ -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)) diff --git a/internal/server/server.go b/internal/server/server.go index bcc6629cf..449469820 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -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. diff --git a/internal/server/task3376_editor_grant_escalation_test.go b/internal/server/task3376_editor_grant_escalation_test.go new file mode 100644 index 000000000..6b4652508 --- /dev/null +++ b/internal/server/task3376_editor_grant_escalation_test.go @@ -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) + } +} diff --git a/internal/server/task3376_system_collection_surfaces_test.go b/internal/server/task3376_system_collection_surfaces_test.go new file mode 100644 index 000000000..9a74f861a --- /dev/null +++ b/internal/server/task3376_system_collection_surfaces_test.go @@ -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) + } +} diff --git a/internal/store/backlinks_visibility.go b/internal/store/backlinks_visibility.go index 2a2ca9257..8767d9b8d 100644 --- a/internal/store/backlinks_visibility.go +++ b/internal/store/backlinks_visibility.go @@ -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) diff --git a/internal/store/grants.go b/internal/store/grants.go index a11ec40b7..c40c3d7ca 100644 --- a/internal/store/grants.go +++ b/internal/store/grants.go @@ -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" diff --git a/internal/store/migrations/108_system_collection_access_backfill.sql b/internal/store/migrations/108_system_collection_access_backfill.sql new file mode 100644 index 000000000..9ceaafe48 --- /dev/null +++ b/internal/store/migrations/108_system_collection_access_backfill.sql @@ -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; diff --git a/internal/store/permissions_test.go b/internal/store/permissions_test.go index bff7ad14c..c1f9dbd92 100644 --- a/internal/store/permissions_test.go +++ b/internal/store/permissions_test.go @@ -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") } } diff --git a/internal/store/pgmigrations/082_system_collection_access_backfill.sql b/internal/store/pgmigrations/082_system_collection_access_backfill.sql new file mode 100644 index 000000000..f07abe7b7 --- /dev/null +++ b/internal/store/pgmigrations/082_system_collection_access_backfill.sql @@ -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; diff --git a/internal/store/system_collection_access_backfill_test.go b/internal/store/system_collection_access_backfill_test.go new file mode 100644 index 000000000..b8b781f4d --- /dev/null +++ b/internal/store/system_collection_access_backfill_test.go @@ -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) + } +} diff --git a/internal/store/workspace_members.go b/internal/store/workspace_members.go index 5d43e8554..98787cd4d 100644 --- a/internal/store/workspace_members.go +++ b/internal/store/workspace_members.go @@ -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) } diff --git a/web/e2e/permission-walk-system-status.spec.ts b/web/e2e/permission-walk-system-status.spec.ts index 507687060..725a990cc 100644 --- a/web/e2e/permission-walk-system-status.spec.ts +++ b/web/e2e/permission-walk-system-status.spec.ts @@ -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; // What the server answered to a status PATCH on each seeded item, per account. const serverEdits: Record> = {}; +// Accounts the server hides the seeded items from (404). TASK-3376: exactly +// editorSpecific, whose collection access lists Tasks only. +const HIDDEN: ReadonlySet = 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 diff --git a/web/src/routes/[username]/[workspace]/settings/+page.svelte b/web/src/routes/[username]/[workspace]/settings/+page.svelte index 9fb6fd75f..d7a016ed8 100644 --- a/web/src/routes/[username]/[workspace]/settings/+page.svelte +++ b/web/src/routes/[username]/[workspace]/settings/+page.svelte @@ -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'); }} > @@ -1062,13 +1073,22 @@ {/each} {#each systemCollections as coll (coll.id)} -