amadan

oficina / sheets Public

feat: grants, roles, and the two-user isolation suite

Spec 8.2. Team roles (owner, member, guest) and per-sheet roles (owner,
editor, commenter, viewer), with access decided by a hand-written
grants join — the one AGENTS.md said listSheets would grow, and not a
scope.Owned, which is single-column equality on user_id and would
return only the sheets somebody created.

A guest sees exactly what they were granted; a colleague reaches every
sheet, because one instance per team is what that means. The join is an
inner join for guests specifically so no grants means no sheets rather
than everything.

Every refusal is a 404. A viewer asking to write and a stranger asking
to read both get what a nonexistent sheet gives, because a 403 confirms
the row exists and the set of ids that 403 is a map of the team's
sheets.

Two bugs the suite found while being written, both of which would have
made sharing quietly useless:

- **A shared-with guest could not sign in.** auth's Authorize was the
  static SHEETS_OWNERS list, so `grant` created a Grant row and a guest
  Member for somebody auth would then refuse. App.Admits now answers
  "on the allowlist, or holds a grant", and stays fail-closed: a
  database error is a refusal, because "we could not check" is not
  "yes".
- **The two admission gates disagreed.** handleBegin used the static
  list while redemption used Admits, so a guest would have been
  admitted at redemption and never sent a link to redeem. Both ask
  App.Admits now.

Roles are compared with Allows, never with ==. The second spelling
silently excludes owners, and would do it on whichever route somebody
forgot. An unrecognised role ranks below every real one, so a grant row
written by a newer build grants nothing rather than everything.

The suite split into two files because the line cap caught it at 381
lines — service-level questions in one, the same questions through real
routes with a real session in the other. A service check that passes
and a route that forgot to call it look identical from inside Go.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RN4HhHR4rHH3Fxh4LMds5
Paul Campbell pushed by paul@keymail.dev a8d07b5fd0945d646763d8b94ecc974746d42eac parent 841e1ce
23 files changed, +982 −68
  • cmd/sheets/main.go +11 −7
  • internal/sheets/app.go +9 −0
  • internal/sheets/auth.go +16 −2
  • internal/sheets/begin.go +8 −2
  • internal/sheets/begin_test.go +15 −12
  • internal/sheets/grants.go +242 −0
  • internal/sheets/grants_http_test.go +142 −0
  • internal/sheets/grants_test.go +247 −0
  • internal/sheets/handlers_grid.go +22 −7
  • internal/sheets/handlers_grid_test.go +14 −5
  • internal/sheets/handlers_sheets.go +17 −4
  • internal/sheets/handlers_sheets_test.go +1 −1
  • internal/sheets/handlers_stream.go +13 −0
  • internal/sheets/handlers_table.go +7 −0
  • internal/sheets/helpers_test.go +47 −0
  • internal/sheets/members.go +19 −1
  • internal/sheets/migrations.go +1 −1
  • internal/sheets/migrations/0004_alter_members.sql +8 −0
  • internal/sheets/migrations/schema.sql +7 −1
  • internal/sheets/models.go +88 −1
  • internal/sheets/sheetsvc.go +35 −17
  • internal/sheets/sheetsvc_test.go +12 −6
  • internal/sheets/signin_test.go +1 −1
diff --git a/cmd/sheets/main.go b/cmd/sheets/main.go
index 29819d6..ba72685 100644
--- a/cmd/sheets/main.go
+++ b/cmd/sheets/main.go
@@ -74,12 +74,6 @@ func main() {
logger.Error("sign-in", "err", err)
os.Exit(1)
}
- signin, err := sheets.NewAuth(d.Writer(), authEnv, logger)
- if err != nil {
- logger.Error("sign-in", "err", err)
- os.Exit(1)
- }
-
// Same shape as the sign-in refusals above, and for the same
// reason: an instance with no DEK answers /healthz perfectly well
// and then fails on the first cell anybody types. The key arrives
@@ -91,14 +85,24 @@ func main() {
os.Exit(1)
}
+ // The app is built first and auth attached second, because the two
+ // are mutually dependent: auth's admission gate has to read this
+ // app's grants table so a guest somebody shared a sheet with can
+ // sign in at all. See App.Admits.
app := sheets.New(sheets.Config{
DB: d.G,
- Auth: signin,
Sealer: sealer,
Origin: authEnv.Origin,
Admit: sheets.Admits(authEnv.Owners),
})
+ signin, err := sheets.NewAuth(d.Writer(), authEnv, logger, app.Admits)
+ if err != nil {
+ logger.Error("sign-in", "err", err)
+ os.Exit(1)
+ }
+ app.UseAuth(signin)
+
// rastrillo.Options.Mux wants a *http.ServeMux; the app's own
// router is a chi.Router (an http.Handler), so it mounts under one.
mux := http.NewServeMux()
diff --git a/internal/sheets/app.go b/internal/sheets/app.go
index 429e3bc..af9e972 100644
--- a/internal/sheets/app.go
+++ b/internal/sheets/app.go
@@ -72,6 +72,15 @@ func New(cfg Config) *App {
}
}
+// UseAuth attaches the sign-in flow after construction.
+//
+// It exists because the two are mutually dependent: auth needs an
+// admission gate that reads this app's grants table, and the app needs
+// auth to read sessions. Building the App first and attaching auth
+// second is the smaller of the two knots — the alternative is a gate
+// that closes over a database handle nobody else uses.
+func (a *App) UseAuth(x *auth.Auth) { a.auth = x }
+
// Router returns the mounted handler.
func (a *App) Router() http.Handler {
r := chi.NewRouter()
diff --git a/internal/sheets/auth.go b/internal/sheets/auth.go
index 4861efc..63a0477 100644
--- a/internal/sheets/auth.go
+++ b/internal/sheets/auth.go
@@ -129,13 +129,27 @@ func splitOwners(raw string) []string {
// underlying flow carries rate-limiter state on the value, so one per
// request would get a fresh empty budget every time and rate-limit
// nothing, silently.
-func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger) (*auth.Auth, error) {
+//
+// authorize is the admission gate consulted at redemption. It is a
+// parameter rather than Admits(env.Owners) because admission is no
+// longer a static list: a guest who has been granted a sheet must be
+// able to sign in, and only the database knows who that is. Passing
+// App.Admits closes the loop.
+func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger, authorize func(string) bool) (*auth.Auth, error) {
+ if authorize == nil {
+ // A nil Authorize admits every verified address, which on a
+ // public-hostname instance means anyone who can receive mail.
+ // Refusing here beats defaulting to the allowlist, because a
+ // caller that forgot to pass one has a bug either way and this
+ // is the version that says so.
+ return nil, errors.New("sign-in: refusing to wire auth with no admission gate")
+ }
return auth.New(auth.Config{
DB: db,
Origin: env.Origin,
InstanceKey: env.InstanceKey,
Mailer: env.Mailer,
- Authorize: Admits(env.Owners),
+ Authorize: authorize,
SigninPath: "/signin",
Subject: "Your Sheets sign-in link",
Logger: logger,
diff --git a/internal/sheets/begin.go b/internal/sheets/begin.go
index 50efd5d..f9f200e 100644
--- a/internal/sheets/begin.go
+++ b/internal/sheets/begin.go
@@ -2,7 +2,7 @@ package sheets
import "net/http"
-// handleBegin gates POST /signin on the allowlist BEFORE auth mints and
+// handleBegin gates POST /signin on admission BEFORE auth mints and
// emails a link.
//
// rastrillo/auth checks Config.Authorize at admission — the moment a
@@ -33,7 +33,13 @@ func (a *App) handleBegin(w http.ResponseWriter, r *http.Request) {
http.Error(w, "bad form", http.StatusBadRequest)
return
}
- if a.admit != nil && !a.admit(r.FormValue("address")) {
+ // App.Admits, NOT a.admit. The two must ask the same question as
+ // auth's own Authorize, or the answers disagree: a.admit is the
+ // static SHEETS_OWNERS list, so a guest somebody shared a sheet
+ // with would be admitted at redemption and never sent a link to
+ // redeem — a share that silently does nothing. Caught by the
+ // two-user isolation suite the moment a guest tried to sign in.
+ if !a.Admits(r.FormValue("address")) {
// Identical to auth's own success redirect, so the two cases are
// indistinguishable from outside.
http.Redirect(w, r, "/signin?sent=1", http.StatusSeeOther)
diff --git a/internal/sheets/begin_test.go b/internal/sheets/begin_test.go
index 0efb74f..71aacab 100644
--- a/internal/sheets/begin_test.go
+++ b/internal/sheets/begin_test.go
@@ -77,26 +77,29 @@ func appWithOrigin(t *testing.T, origin string) (*App, *recorder) {
t.Fatalf("migrate: %v", err)
}
rec := &recorder{}
- owners := []string{"paul@example.com"}
+ owners := []string{signedInAddress}
+ sealer, err := seal.New(make([]byte, dekBytes))
+ if err != nil {
+ t.Fatalf("seal.New: %v", err)
+ }
+ // The app is built first and auth attached second, the same order
+ // main uses and for the same reason: auth's admission gate reads
+ // this app's grants table so a shared-with guest can sign in.
+ app := New(Config{
+ DB: d.G, Sealer: sealer, Origin: origin, Admit: Admits(owners),
+ })
a, err := NewAuth(d.Writer(), AuthEnv{
Origin: testOrigin,
InstanceKey: "test-instance-key",
Owners: owners,
Mailer: rec,
- }, quietLogger())
+ }, quietLogger(), app.Admits)
if err != nil {
t.Fatalf("NewAuth: %v", err)
}
- // A real sealer, not nil: every test that signs in and then touches
- // a sheet needs one, and an app that can sign you in but cannot
- // seal a title is a shape production never has.
- sealer, err := seal.New(make([]byte, dekBytes))
- if err != nil {
- t.Fatalf("seal.New: %v", err)
- }
- return New(Config{
- DB: d.G, Auth: a, Sealer: sealer, Origin: origin, Admit: Admits(owners),
- }), rec
+ app.UseAuth(a)
+ rememberMailer(app, rec)
+ return app, rec
}
func postSignin(t *testing.T, app *App, address string) *httptest.ResponseRecorder {
diff --git a/internal/sheets/grants.go b/internal/sheets/grants.go
new file mode 100644
index 0000000..4c1e0cb
--- /dev/null
+++ b/internal/sheets/grants.go
@@ -0,0 +1,242 @@
+package sheets
+
+import (
+ "errors"
+ "strings"
+
+ "gorm.io/gorm"
+ "gorm.io/gorm/clause"
+
+ "amadan.net/oficina/sheets/internal/sheets/store"
+)
+
+// errNoAccess is what every read answers when the viewer may not see a
+// sheet.
+//
+// Callers turn it into a 404, never a 403 (AGENTS.md). A 403 confirms
+// the row exists, which is a fact about somebody else's data — and on
+// an instance where guests hold single-sheet grants, the set of ids
+// that 403 is a map of the team's sheets.
+var errNoAccess = errors.New("sheets: no grant")
+
+// roleFor returns a member's effective role on a sheet, and whether
+// they have any access at all.
+//
+// Two sources, in order:
+//
+// 1. An explicit Grant row. This is the authority for everyone, and
+// the only source for a guest.
+// 2. Team standing. An owner or member of the team can reach every
+// sheet on the instance — that is what "one instance per team"
+// means, and it is why the previous instance-wide list was right
+// for a single-team instance and wrong the moment guests existed.
+//
+// A guest gets NOTHING from step 2. That is the whole distinction in
+// §8.2: a guest sees exactly the sheets granted to them.
+func (a *App) roleFor(sheetID uint64, m Member) (SheetRole, bool) {
+ if a.db == nil {
+ return "", false
+ }
+ var g Grant
+ err := a.db.Where("sheet_id = ? AND member_id = ?", sheetID, m.ID).First(&g).Error
+ if err == nil {
+ if role := SheetRole(g.Role); role.Valid() {
+ return role, true
+ }
+ // A row whose role this build does not recognise grants
+ // nothing. Falling through to team standing would be worse: it
+ // would silently upgrade an unreadable grant to full access.
+ return "", false
+ }
+ if !errors.Is(err, gorm.ErrRecordNotFound) {
+ return "", false
+ }
+
+ switch TeamRole(m.TeamRole) {
+ case TeamOwner, TeamMember:
+ // Team standing reaches every sheet, as an editor. Not as
+ // owner: sheet ownership is about who may change sharing, and
+ // handing that to every colleague by default makes the grant
+ // table decorative.
+ if a.sheetExists(sheetID) {
+ return SheetEditor, true
+ }
+ }
+ return "", false
+}
+
+// mayRead is the gate every read route uses.
+func (a *App) mayRead(sheetID uint64, m Member) bool {
+ role, ok := a.roleFor(sheetID, m)
+ return ok && role.Allows(SheetViewer)
+}
+
+// mayWrite is the gate every cell-writing route uses.
+func (a *App) mayWrite(sheetID uint64, m Member) bool {
+ role, ok := a.roleFor(sheetID, m)
+ return ok && role.Allows(SheetEditor)
+}
+
+// mayShare is the gate the share routes use. Sheet owners only: a
+// person who can change who else can read a sheet is exercising the
+// one power that cannot be undone by the people it affects.
+func (a *App) mayShare(sheetID uint64, m Member) bool {
+ role, ok := a.roleFor(sheetID, m)
+ return ok && role.Allows(SheetOwner)
+}
+
+func (a *App) sheetExists(id uint64) bool {
+ var n int64
+ if err := a.db.Model(&store.Sheet{}).
+ Where("id = ? AND deleted_at IS NULL", id).Count(&n).Error; err != nil {
+ return false
+ }
+ return n == 1
+}
+
+// visibleSheets returns the sheets a member may see.
+//
+// This is the hand-written grants join AGENTS.md requires, and the
+// reason it is hand-written rather than scope.Owned: scope.Owned is
+// single-column equality on user_id, our sheets are team-shared, and
+// applied here it would return only the sheets a person created — which
+// fails as a plausible short list rather than as an error.
+//
+// The query is deliberately one statement rather than "list all, then
+// filter in Go". A filter in Go reads every sheet on the instance into
+// memory before discarding most of them, which is both wasteful and the
+// shape where somebody later forgets the discard.
+func (a *App) visibleSheets(m Member) ([]store.Sheet, error) {
+ if a.db == nil {
+ return nil, errNoDB
+ }
+ q := a.db.Model(&store.Sheet{}).Where("sheets.deleted_at IS NULL")
+
+ switch TeamRole(m.TeamRole) {
+ case TeamOwner, TeamMember:
+ // The whole instance. One team, one set of sheets.
+ default:
+ // A guest sees exactly what they were granted. The join is an
+ // inner join precisely so a guest with no grants gets an empty
+ // list rather than everything.
+ q = q.Joins("JOIN grants ON grants.sheet_id = sheets.id AND grants.member_id = ?", m.ID)
+ }
+
+ var rows []store.Sheet
+ if err := q.Order("sheets.updated_at DESC, sheets.id DESC").Find(&rows).Error; err != nil {
+ return nil, err
+ }
+ return rows, nil
+}
+
+// grantsOn lists who can reach a sheet, for the share screen.
+type sharedWith struct {
+ Member Member
+ Role SheetRole
+}
+
+func (a *App) grantsOn(sheetID uint64) ([]sharedWith, error) {
+ if a.db == nil {
+ return nil, errNoDB
+ }
+ var rows []Grant
+ if err := a.db.Where("sheet_id = ?", sheetID).Order("id ASC").Find(&rows).Error; err != nil {
+ return nil, err
+ }
+ out := make([]sharedWith, 0, len(rows))
+ for _, g := range rows {
+ var m Member
+ if err := a.db.Where("id = ?", g.MemberID).First(&m).Error; err != nil {
+ continue // a grant whose member vanished names nobody
+ }
+ out = append(out, sharedWith{Member: m, Role: SheetRole(g.Role)})
+ }
+ return out, nil
+}
+
+// grant gives an address a role on a sheet, creating the member as a
+// guest if this is the first time the instance has seen them.
+//
+// Guest is the default for a new person, and the direction matters:
+// a guest wrongly made a member sees every sheet on the instance, while
+// a member wrongly left a guest sees one sheet too few and says so.
+// Only SHEETS_OWNERS promotes beyond guest, at sign-in.
+func (a *App) grant(sheetID uint64, address string, role SheetRole, by Member) (Member, error) {
+ if a.db == nil {
+ return Member{}, errNoDB
+ }
+ if !role.Valid() {
+ return Member{}, errors.New("sheets: unknown role")
+ }
+ addr := strings.ToLower(strings.TrimSpace(address))
+ if addr == "" {
+ return Member{}, errors.New("sheets: a share needs an address")
+ }
+ if !strings.Contains(addr, "@") {
+ return Member{}, errors.New("sheets: that does not look like an email address")
+ }
+ m, err := a.memberFor(addr)
+ if err != nil {
+ return Member{}, err
+ }
+ // Upsert: re-sharing with somebody who already has access changes
+ // their role rather than failing on the unique index, which is what
+ // a person doing it expects.
+ g := Grant{SheetID: sheetID, MemberID: m.ID, Role: string(role), GrantedBy: by.ID}
+ if err := a.db.Clauses(clause.OnConflict{
+ Columns: []clause.Column{{Name: "sheet_id"}, {Name: "member_id"}},
+ DoUpdates: clause.AssignmentColumns([]string{"role", "granted_by", "updated_at"}),
+ }).Create(&g).Error; err != nil {
+ return Member{}, err
+ }
+ return m, nil
+}
+
+// revoke removes a grant. Removing one that is not there is not an
+// error: the caller wanted that person not to have access, and they do
+// not.
+func (a *App) revoke(sheetID, memberID uint64) error {
+ if a.db == nil {
+ return errNoDB
+ }
+ return a.db.Where("sheet_id = ? AND member_id = ?", sheetID, memberID).
+ Delete(&Grant{}).Error
+}
+
+// Admits is the instance's admission gate: who may hold a session here.
+//
+// Two ways in, and the second is what makes sharing mean anything:
+//
+// 1. The SHEETS_OWNERS allowlist — the team.
+// 2. Holding at least one grant — a guest somebody shared a sheet
+// with.
+//
+// Without the second, `grant` would happily create a Grant row and a
+// guest Member for somebody who could then never sign in to use it:
+// auth refuses an address its Authorize does not admit, so the share
+// would be a row nobody could ever exercise. That is not a
+// hypothetical — it is what this app did between the grants table
+// landing and this function existing.
+//
+// It stays fail-closed. An address with no allowlist entry and no grant
+// is refused, and a database error is refused rather than admitted:
+// "we could not check" is not "yes".
+func (a *App) Admits(address string) bool {
+ addr := strings.ToLower(strings.TrimSpace(address))
+ if addr == "" {
+ return false
+ }
+ if a.admit != nil && a.admit(addr) {
+ return true
+ }
+ if a.db == nil {
+ return false
+ }
+ var n int64
+ if err := a.db.Model(&Grant{}).
+ Joins("JOIN members ON members.id = grants.member_id").
+ Where("members.address = ?", addr).Count(&n).Error; err != nil {
+ return false
+ }
+ return n > 0
+}
diff --git a/internal/sheets/grants_http_test.go b/internal/sheets/grants_http_test.go
new file mode 100644
index 0000000..1d862e2
--- /dev/null
+++ b/internal/sheets/grants_http_test.go
@@ -0,0 +1,142 @@
+package sheets
+
+import (
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+)
+
+// The HTTP half of the two-user isolation suite: the same questions as
+// grants_test.go, asked through the real routes with a real session,
+// because a service-level check that passes and a route that forgot to
+// call it look identical from inside Go.
+
+// The HTTP surface, end to end: a guest signs in for real and is told
+// nothing about the sheets they were not given.
+func TestAGuestGetsA404NotA403(t *testing.T) {
+ app, cookies := signIn(t)
+ shared, _ := newSheet(t, app, cookies, "Shared")
+ private, _ := newSheet(t, app, cookies, "Private")
+ guestOn(t, app, shared.ID, "ada@elsewhere.example", SheetViewer)
+
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ for _, tc := range []struct {
+ path string
+ want int
+ }{
+ {fmt.Sprintf("/sheets/%d", shared.ID), http.StatusOK},
+ {fmt.Sprintf("/sheets/%d", private.ID), http.StatusNotFound},
+ {fmt.Sprintf("/sheets/%d/table", private.ID), http.StatusNotFound},
+ {fmt.Sprintf("/api/sheets/%d/tabs/%d/cells", private.ID, private.TabID), http.StatusNotFound},
+ {fmt.Sprintf("/api/sheets/%d/tabs/%d/stream", private.ID, private.TabID), http.StatusNotFound},
+ } {
+ rec := do(t, app, as(httptest.NewRequest(http.MethodGet, tc.path, nil), guestCookies))
+ if rec.Code != tc.want {
+ t.Errorf("GET %s as a guest = %d, want %d", tc.path, rec.Code, tc.want)
+ }
+ }
+}
+
+// A viewer may look and must not write. This is the one that would fail
+// silently if a route checked "has any grant" instead of "has enough".
+func TestAViewerCannotWrite(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Read only")
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ if rec := do(t, app, as(httptest.NewRequest(http.MethodGet, cells, nil), guestCookies)); rec.Code != http.StatusOK {
+ t.Fatalf("a viewer cannot read the grid: %d", rec.Code)
+ }
+ if rec := postCell(t, app, guestCookies, cells, `{"r":0,"c":0,"raw":"sneaky"}`); rec.Code != http.StatusNotFound {
+ t.Errorf("a viewer wrote a cell: %d", rec.Code)
+ }
+ form := url.Values{"r": {"0"}, "c": {"0"}, "raw": {"sneaky"}, "page": {"1"}}
+ if rec := do(t, app, as(postForm(fmt.Sprintf("/sheets/%d/table", s.ID), form), guestCookies)); rec.Code == http.StatusSeeOther {
+ t.Error("a viewer wrote a cell through the scriptless table")
+ }
+}
+
+// A commenter may read and must not write cells. Commenting itself is
+// a later slice; what this pins is that the role does not leak edit
+// rights on the way there.
+func TestACommenterCannotWriteCells(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Comment only")
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetCommenter)
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ if rec := do(t, app, as(httptest.NewRequest(http.MethodGet, cells, nil), guestCookies)); rec.Code != http.StatusOK {
+ t.Fatalf("a commenter cannot read the grid: %d", rec.Code)
+ }
+ if rec := postCell(t, app, guestCookies, cells, `{"r":0,"c":0,"raw":"sneaky"}`); rec.Code != http.StatusNotFound {
+ t.Errorf("a commenter wrote a cell: %d", rec.Code)
+ }
+ _ = s
+}
+
+func TestAnEditorCanWrite(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Editable")
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetEditor)
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ rec := postCell(t, app, guestCookies, cells, `{"r":0,"c":0,"raw":"=2*3"}`)
+ if rec.Code != http.StatusOK {
+ t.Fatalf("an editor could not write: %d %s", rec.Code, rec.Body.String())
+ }
+ if g := decodeGrid(t, rec); len(g.Cells) != 1 || g.Cells[0].Display != "6" {
+ t.Fatalf("the editor's write produced %+v", g.Cells)
+ }
+ _ = s
+}
+
+// The stream carries every cell of every edit, so it needs the same
+// grant a read of the grid does. Without the check it is the quietest
+// possible leak, because nothing renders it.
+func TestTheStreamIsGated(t *testing.T) {
+ app, cookies := signIn(t)
+ private, _ := newSheet(t, app, cookies, "Private")
+ other, _ := newSheet(t, app, cookies, "Shared")
+ guestOn(t, app, other.ID, "ada@elsewhere.example", SheetViewer)
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ path := fmt.Sprintf("/api/sheets/%d/tabs/%d/stream", private.ID, private.TabID)
+ if rec := do(t, app, as(httptest.NewRequest(http.MethodGet, path, nil), guestCookies)); rec.Code != http.StatusNotFound {
+ t.Fatalf("a guest opened the stream of a sheet they cannot read: %d", rec.Code)
+ }
+}
+
+// signInAs drives the real magic-link flow for an arbitrary admitted
+// address, so a guest's session is minted the way a person's is rather
+// than fabricated.
+func signInAs(t *testing.T, app *App, address string) []*http.Cookie {
+ t.Helper()
+ mail := app.testMailer(t)
+ before := len(mail.addresses())
+
+ req := httptest.NewRequest(http.MethodPost, "/signin", strings.NewReader("address="+url.QueryEscape(address)))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ req.Header.Set("Origin", testOrigin)
+ if rec := do(t, app, req); rec.Code != http.StatusSeeOther {
+ t.Fatalf("POST /signin for %s = %d", address, rec.Code)
+ }
+ if len(mail.addresses()) == before {
+ t.Fatalf("no link was emailed to %s; is that address admitted?", address)
+ }
+ u, err := url.Parse(mail.lastLink(t))
+ if err != nil {
+ t.Fatalf("parse link: %v", err)
+ }
+ rec := do(t, app, httptest.NewRequest(http.MethodGet, u.RequestURI(), nil))
+ cookies := rec.Result().Cookies()
+ if len(cookies) == 0 {
+ t.Fatalf("verify minted no session for %s (status %d, location %q)",
+ address, rec.Code, rec.Header().Get("Location"))
+ }
+ return cookies
+}
diff --git a/internal/sheets/grants_test.go b/internal/sheets/grants_test.go
new file mode 100644
index 0000000..693dc73
--- /dev/null
+++ b/internal/sheets/grants_test.go
@@ -0,0 +1,247 @@
+package sheets
+
+import "testing"
+
+// The two-user isolation suite AGENTS.md requires.
+//
+// Every test here asks the same question from a different angle: can
+// somebody reach a sheet nobody gave them? The answer must always be
+// "they get exactly what a nonexistent sheet gives" — a 404, never a
+// 403, because a 403 confirms the row exists and the set of ids that
+// 403 is a map of the team's sheets.
+
+// guestOn shares a sheet with a fresh address at a role, and returns
+// that member. The address is deliberately outside SHEETS_OWNERS, so
+// the only way they reach anything is the grant itself.
+func guestOn(t *testing.T, app *App, sheetID uint64, address string, role SheetRole) Member {
+ t.Helper()
+ m, err := app.grant(sheetID, address, role, signedInMember(t, app))
+ if err != nil {
+ t.Fatalf("grant %s as %s: %v", address, role, err)
+ }
+ if TeamRole(m.TeamRole) != TeamGuest {
+ t.Fatalf("%s was minted as %q, want guest — a guest wrongly made a member "+
+ "sees every sheet on the instance", address, m.TeamRole)
+ }
+ return m
+}
+
+func TestAGuestSeesOnlyWhatTheyWereGranted(t *testing.T) {
+ app, cookies := signIn(t)
+ shared, _ := newSheet(t, app, cookies, "Shared with Ada")
+ private, _ := newSheet(t, app, cookies, "Not shared")
+
+ ada := guestOn(t, app, shared.ID, "ada@elsewhere.example", SheetViewer)
+
+ got, err := app.listSheets(ada)
+ if err != nil {
+ t.Fatalf("listSheets: %v", err)
+ }
+ if len(got) != 1 {
+ t.Fatalf("a guest granted one sheet sees %d: %+v", len(got), got)
+ }
+ if got[0].ID != shared.ID {
+ t.Fatalf("the guest sees sheet %d, want the one they were granted (%d)", got[0].ID, shared.ID)
+ }
+ if app.mayRead(private.ID, ada) {
+ t.Fatal("a guest can read a sheet nobody granted them")
+ }
+}
+
+func TestAGuestWithNoGrantsSeesNothing(t *testing.T) {
+ app, cookies := signIn(t)
+ newSheet(t, app, cookies, "Team sheet")
+
+ stranger, err := app.memberFor("nobody@elsewhere.example")
+ if err != nil {
+ t.Fatalf("memberFor: %v", err)
+ }
+ got, err := app.listSheets(stranger)
+ if err != nil {
+ t.Fatalf("listSheets: %v", err)
+ }
+ // Empty, not everything. The inner join is what makes this true;
+ // a left join here would hand a stranger the whole instance.
+ if len(got) != 0 {
+ t.Fatalf("a member with no grants sees %d sheets, want 0", len(got))
+ }
+}
+
+// A colleague reaches every sheet — that is what one instance per team
+// means, and it is the half the guest rule must not break.
+func TestATeamMemberSeesEverySheet(t *testing.T) {
+ app, cookies := signIn(t)
+ one, _ := newSheet(t, app, cookies, "One")
+ two, _ := newSheet(t, app, cookies, "Two")
+
+ colleague, err := app.memberFor("colleague@example.com")
+ if err != nil {
+ t.Fatalf("memberFor: %v", err)
+ }
+ if err := app.db.Model(&colleague).Update("team_role", string(TeamMember)).Error; err != nil {
+ t.Fatalf("promote: %v", err)
+ }
+ colleague.TeamRole = string(TeamMember)
+
+ got, err := app.listSheets(colleague)
+ if err != nil {
+ t.Fatalf("listSheets: %v", err)
+ }
+ if len(got) != 2 {
+ t.Fatalf("a team member sees %d of 2 sheets", len(got))
+ }
+ if !app.mayWrite(one.ID, colleague) || !app.mayWrite(two.ID, colleague) {
+ t.Fatal("a team member cannot write to a team sheet")
+ }
+ // But not owner: sharing is not something every colleague inherits.
+ if app.mayShare(one.ID, colleague) {
+ t.Fatal("a team member can change who else can read a sheet they were never given")
+ }
+}
+
+func TestRolesAreOrdered(t *testing.T) {
+ cases := []struct {
+ have, want SheetRole
+ allowed bool
+ }{
+ {SheetOwner, SheetEditor, true},
+ {SheetOwner, SheetViewer, true},
+ {SheetEditor, SheetEditor, true},
+ {SheetEditor, SheetOwner, false},
+ {SheetCommenter, SheetViewer, true},
+ {SheetCommenter, SheetEditor, false},
+ {SheetViewer, SheetViewer, true},
+ {SheetViewer, SheetCommenter, false},
+ // An unknown role ranks below every real one, so a row written
+ // by a newer build grants nothing rather than everything.
+ {SheetRole("admin"), SheetViewer, false},
+ {SheetRole(""), SheetViewer, false},
+ }
+ for _, c := range cases {
+ if got := c.have.Allows(c.want); got != c.allowed {
+ t.Errorf("%q.Allows(%q) = %v, want %v", c.have, c.want, got, c.allowed)
+ }
+ }
+}
+
+func TestRevokeRemovesAccess(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Shared then not")
+ ada := guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+
+ if !app.mayRead(s.ID, ada) {
+ t.Fatal("the grant did not take")
+ }
+ if err := app.revoke(s.ID, ada.ID); err != nil {
+ t.Fatalf("revoke: %v", err)
+ }
+ if app.mayRead(s.ID, ada) {
+ t.Fatal("a revoked guest can still read")
+ }
+ // Revoking again is not an error: the caller wanted them not to
+ // have access, and they do not.
+ if err := app.revoke(s.ID, ada.ID); err != nil {
+ t.Fatalf("revoking twice: %v", err)
+ }
+}
+
+// Re-sharing changes the role rather than failing on the unique index,
+// which is what somebody doing it expects.
+func TestRegrantingChangesTheRole(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Promote me")
+ ada := guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+
+ if _, err := app.grant(s.ID, "ada@elsewhere.example", SheetEditor, signedInMember(t, app)); err != nil {
+ t.Fatalf("re-grant: %v", err)
+ }
+ role, ok := app.roleFor(s.ID, ada)
+ if !ok || role != SheetEditor {
+ t.Fatalf("after re-granting, role = %q (%v), want editor", role, ok)
+ }
+}
+
+func TestGrantRefusesRubbish(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Sheet")
+ by := signedInMember(t, app)
+
+ for _, tc := range []struct {
+ address string
+ role SheetRole
+ }{
+ {"", SheetViewer},
+ {" ", SheetViewer},
+ {"not-an-address", SheetViewer},
+ {"ada@elsewhere.example", SheetRole("admin")},
+ {"ada@elsewhere.example", SheetRole("")},
+ } {
+ if _, err := app.grant(s.ID, tc.address, tc.role, by); err == nil {
+ t.Errorf("grant(%q, %q) was accepted", tc.address, tc.role)
+ }
+ }
+}
+
+// The creator owns what they made, written as a Grant rather than
+// inferred from Sheet.CreatedBy — two sources of truth for one question
+// is how the share screen ends up unable to hand ownership on.
+func TestTheCreatorOwnsTheSheet(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Mine")
+ me := signedInMember(t, app)
+
+ role, ok := app.roleFor(s.ID, me)
+ if !ok || role != SheetOwner {
+ t.Fatalf("the creator's role is %q (%v), want owner", role, ok)
+ }
+ if !app.mayShare(s.ID, me) {
+ t.Fatal("the creator cannot share their own sheet")
+ }
+}
+
+// Sharing has to make somebody able to sign in, or the grant is a row
+// nobody can ever exercise. This is what the app did between the grants
+// table landing and App.Admits existing.
+func TestASharedWithGuestMaySignIn(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Shared")
+
+ if app.Admits("ada@elsewhere.example") {
+ t.Fatal("a stranger is admitted before any share")
+ }
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+ if !app.Admits("ada@elsewhere.example") {
+ t.Fatal("a guest who holds a grant is refused sign-in; the share would be unusable")
+ }
+ // Case and padding must not decide who gets in.
+ if !app.Admits(" Ada@Elsewhere.Example ") {
+ t.Fatal("admission is case-sensitive; the same person would be refused")
+ }
+}
+
+func TestAdmissionIsFailClosed(t *testing.T) {
+ app, _ := signIn(t)
+ for _, addr := range []string{"", " ", "nobody@elsewhere.example"} {
+ if app.Admits(addr) {
+ t.Errorf("Admits(%q) = true", addr)
+ }
+ }
+}
+
+// A grant row whose role this build cannot read must grant nothing, not
+// fall through to team standing — falling through would silently
+// upgrade an unreadable grant to full access.
+func TestAnUnreadableGrantGrantsNothing(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Sheet")
+ me := signedInMember(t, app)
+
+ if err := app.db.Model(&Grant{}).
+ Where("sheet_id = ? AND member_id = ?", s.ID, me.ID).
+ Update("role", "from-the-future").Error; err != nil {
+ t.Fatalf("corrupt the grant: %v", err)
+ }
+ if _, ok := app.roleFor(s.ID, me); ok {
+ t.Fatal("a grant with an unrecognised role still granted access")
+ }
+}
diff --git a/internal/sheets/handlers_grid.go b/internal/sheets/handlers_grid.go
index 6c95a96..712d5f4 100644
--- a/internal/sheets/handlers_grid.go
+++ b/internal/sheets/handlers_grid.go
@@ -32,7 +32,7 @@ type cellWrite struct {
// handleCellsGet serves a window of a tab.
func (a *App) handleCellsGet(w http.ResponseWriter, r *http.Request) {
- h, ok := a.openTab(w, r)
+ h, ok := a.openTab(w, r, SheetViewer)
if !ok {
return
}
@@ -58,7 +58,7 @@ func (a *App) handleCellPost(w http.ResponseWriter, r *http.Request) {
http.Error(w, "Sign in first.", http.StatusForbidden)
return
}
- h, ok := a.openTab(w, r)
+ h, ok := a.openTab(w, r, SheetEditor)
if !ok {
return
}
@@ -130,23 +130,38 @@ func (a *App) publish(tabID uint64, resp gridResponse) {
a.hub.Publish(tabID, live.Message{Version: resp.Version, Data: data})
}
-// openTab resolves {sheetID}/{tabID} and opens the tab's handle,
-// answering for itself if it cannot. The bool is whether the caller
-// should carry on.
+// openTab resolves {sheetID}/{tabID}, checks the viewer holds at least
+// `need` on the sheet, and opens the tab's handle. It answers for
+// itself if it cannot; the bool is whether the caller should carry on.
//
// The tab is checked against the sheet, not just looked up: both ids
// come from the URL, so without the pairing check any tab could be read
-// through any sheet, and the sheet id in the path would mean nothing.
-func (a *App) openTab(w http.ResponseWriter, r *http.Request) (*store.Handle, bool) {
+// through any sheet, and the sheet id in the path — the thing the grant
+// is keyed on — would mean nothing.
+//
+// Every refusal here is a 404. A viewer asking to write, and a stranger
+// asking to read, both get the same answer as a sheet that does not
+// exist. Anything else lets somebody enumerate the team's sheets by
+// status code.
+func (a *App) openTab(w http.ResponseWriter, r *http.Request, need SheetRole) (*store.Handle, bool) {
if a.db == nil || a.sealer == nil {
a.fail(w, r, errNoSealer)
return nil, false
}
+ viewer, ok := a.actor(r)
+ if !ok {
+ a.notFound(w, r)
+ return nil, false
+ }
sheetID, tabID := sheetIDFrom(r), tabIDFrom(r)
if sheetID == 0 || tabID == 0 || !a.tabBelongsTo(sheetID, tabID) {
a.notFound(w, r)
return nil, false
}
+ if role, has := a.roleFor(sheetID, viewer); !has || !role.Allows(need) {
+ a.notFound(w, r)
+ return nil, false
+ }
h, err := store.Open(a.db, a.sealer, tabID)
if err != nil {
a.fail(w, r, err)
diff --git a/internal/sheets/handlers_grid_test.go b/internal/sheets/handlers_grid_test.go
index 8eeaa34..b2c5ea0 100644
--- a/internal/sheets/handlers_grid_test.go
+++ b/internal/sheets/handlers_grid_test.go
@@ -5,26 +5,35 @@ import (
"fmt"
"net/http"
"net/http/httptest"
+ "strconv"
"strings"
"testing"
)
// newSheet creates a sheet through the real HTTP route and returns the
// cells endpoint for its first tab.
+//
+// It reads the new sheet out of the redirect rather than by listing.
+// Listing needs a viewer, and a helper that picked its own would be
+// asserting visibility as a side effect of setup — which is how the
+// grants change first broke this: it listed as a member the caller had
+// never signed in as, who was correctly a guest and correctly saw
+// nothing.
func newSheet(t *testing.T, app *App, cookies []*http.Cookie, title string) (SheetSummary, string) {
t.Helper()
rec := do(t, app, as(postForm("/sheets", map[string][]string{"title": {title}}), cookies))
if rec.Code != http.StatusSeeOther {
t.Fatalf("creating a sheet = %d: %s", rec.Code, rec.Body.String())
}
- list, err := app.listSheets()
+ loc := rec.Header().Get("Location")
+ id, err := strconv.ParseUint(strings.TrimPrefix(loc, "/sheets/"), 10, 64)
if err != nil {
- t.Fatalf("listSheets: %v", err)
+ t.Fatalf("Location %q is not a sheet URL: %v", loc, err)
}
- if len(list) == 0 {
- t.Fatal("no sheet was created")
+ s, err := app.sheetSummary(id, signedInMember(t, app))
+ if err != nil {
+ t.Fatalf("sheetSummary(%d): %v", id, err)
}
- s := list[0]
return s, fmt.Sprintf("/api/sheets/%d/tabs/%d/cells", s.ID, s.TabID)
}
diff --git a/internal/sheets/handlers_sheets.go b/internal/sheets/handlers_sheets.go
index 53bfa4c..6a04dd0 100644
--- a/internal/sheets/handlers_sheets.go
+++ b/internal/sheets/handlers_sheets.go
@@ -30,9 +30,14 @@ type sheetPageView struct {
Warned []string
}
-// handleSheetList renders every sheet on the instance.
+// handleSheetList renders the sheets the viewer may see.
func (a *App) handleSheetList(w http.ResponseWriter, r *http.Request) {
- list, err := a.listSheets()
+ viewer, ok := a.actor(r)
+ if !ok {
+ http.Error(w, "Sign in first.", http.StatusForbidden)
+ return
+ }
+ list, err := a.listSheets(viewer)
if err != nil {
a.fail(w, r, err)
return
@@ -92,14 +97,22 @@ func (a *App) handleSheetPage(w http.ResponseWriter, r *http.Request) {
// 404, never a 403 (AGENTS.md). A 403 tells whoever asked that the row
// exists, which is a fact about somebody else's data.
func (a *App) sheetFromURL(w http.ResponseWriter, r *http.Request) (SheetSummary, bool) {
+ viewer, ok := a.actor(r)
+ if !ok {
+ a.notFound(w, r)
+ return SheetSummary{}, false
+ }
id, err := strconv.ParseUint(chi.URLParam(r, "sheetID"), 10, 64)
if err != nil || id == 0 {
a.notFound(w, r)
return SheetSummary{}, false
}
- s, err := a.sheetSummary(id)
+ s, err := a.sheetSummary(id, viewer)
if err != nil {
- if errors.Is(err, gorm.ErrRecordNotFound) {
+ // No grant and no such sheet are the same answer. See
+ // sheetSummary for why, and the two-user isolation suite for
+ // the proof that they stay the same.
+ if errors.Is(err, gorm.ErrRecordNotFound) || errors.Is(err, errNoAccess) {
a.notFound(w, r)
return SheetSummary{}, false
}
diff --git a/internal/sheets/handlers_sheets_test.go b/internal/sheets/handlers_sheets_test.go
index 3f56d5c..2f68c04 100644
--- a/internal/sheets/handlers_sheets_test.go
+++ b/internal/sheets/handlers_sheets_test.go
@@ -112,7 +112,7 @@ func TestSheetCreateIgnoresFieldsItWasNotAsking(t *testing.T) {
if loc := rec.Header().Get("Location"); loc == "/sheets/4242" {
t.Fatal("a form field set the sheet's id")
}
- list, err := app.listSheets()
+ list, err := app.listSheets(signedInMember(t, app))
if err != nil {
t.Fatalf("listSheets: %v", err)
}
diff --git a/internal/sheets/handlers_stream.go b/internal/sheets/handlers_stream.go
index 9c0ee0c..937e41c 100644
--- a/internal/sheets/handlers_stream.go
+++ b/internal/sheets/handlers_stream.go
@@ -48,11 +48,24 @@ const (
// written against 101 would never pass here. The conflation is worth
// naming because the source document this design came from made it.
func (a *App) handleStream(w http.ResponseWriter, r *http.Request) {
+ viewer, ok := a.actor(r)
+ if !ok {
+ a.notFound(w, r)
+ return
+ }
sheetID, tabID := sheetIDFrom(r), tabIDFrom(r)
if sheetID == 0 || tabID == 0 || !a.tabBelongsTo(sheetID, tabID) {
a.notFound(w, r)
return
}
+ // The stream carries every cell of every edit on this tab, so it
+ // needs the same grant a read of the grid does. Without this check
+ // the fan-out would be the one route that ignored sharing entirely
+ // — and the quietest possible leak, since nothing renders it.
+ if !a.mayRead(sheetID, viewer) {
+ a.notFound(w, r)
+ return
+ }
rc := http.NewResponseController(w)
diff --git a/internal/sheets/handlers_table.go b/internal/sheets/handlers_table.go
index a9c1595..cc49d50 100644
--- a/internal/sheets/handlers_table.go
+++ b/internal/sheets/handlers_table.go
@@ -132,6 +132,13 @@ func (a *App) handleTableWrite(w http.ResponseWriter, r *http.Request) {
http.Error(w, "Sign in first.", http.StatusForbidden)
return
}
+ // sheetFromURL proved the viewer may READ. Writing needs more, and
+ // a commenter or viewer reaching this route gets the same 404 as a
+ // stranger.
+ if !a.mayWrite(s.ID, actor) {
+ a.notFound(w, r)
+ return
+ }
rowIdx, err := strconv.Atoi(r.FormValue("r"))
if err != nil || rowIdx < 0 || rowIdx >= maxWindowRows {
a.notFound(w, r)
diff --git a/internal/sheets/helpers_test.go b/internal/sheets/helpers_test.go
index ccaab16..4f4e797 100644
--- a/internal/sheets/helpers_test.go
+++ b/internal/sheets/helpers_test.go
@@ -6,6 +6,7 @@ import (
"net/http/httptest"
"net/url"
"path/filepath"
+ "sync"
"testing"
rdb "github.com/carlosframework/rastrillo/db"
@@ -39,6 +40,52 @@ func newTestApp(t *testing.T) *App {
return New(Config{DB: d.G, Sealer: s, Admit: func(string) bool { return true }})
}
+// testMailers remembers which recorder belongs to which test App, so a
+// test holding only an *App can still read the mail it sent.
+//
+// A map rather than a field on App: the recorder is a test fixture and
+// App is production code, and threading a mailer through Config purely
+// so tests can reach it would put test scaffolding in the shipped
+// struct.
+var (
+ testMailersMu sync.Mutex
+ testMailers = map[*App]*recorder{}
+)
+
+func rememberMailer(app *App, rec *recorder) {
+ testMailersMu.Lock()
+ defer testMailersMu.Unlock()
+ testMailers[app] = rec
+}
+
+// testMailer returns the recorder behind this App's sign-in mail.
+func (a *App) testMailer(t *testing.T) *recorder {
+ t.Helper()
+ testMailersMu.Lock()
+ defer testMailersMu.Unlock()
+ rec, ok := testMailers[a]
+ if !ok {
+ t.Fatal("this App was not built by appWithOrigin, so no mail was recorded")
+ }
+ return rec
+}
+
+// signedInAddress is who signIn signs in as, and who appWithRecorder's
+// allowlist admits. Named once so a test helper and the fixture cannot
+// drift apart — which they did the moment team roles existed and a
+// helper listing as somebody else started seeing nothing.
+const signedInAddress = "paul@example.com"
+
+// signedInMember is the Member behind signIn's session.
+func signedInMember(t *testing.T, app *App) Member {
+ t.Helper()
+ m, err := app.memberFor(signedInAddress)
+ if err != nil {
+ t.Fatalf("memberFor %s: %v", signedInAddress, err)
+ }
+ return m
+}
+
// testMember is the actor most service tests write as.
func testMember(t *testing.T, app *App) Member {
t.Helper()
diff --git a/internal/sheets/members.go b/internal/sheets/members.go
index 117dc5e..60c187c 100644
--- a/internal/sheets/members.go
+++ b/internal/sheets/members.go
@@ -39,7 +39,16 @@ func (a *App) memberFor(address string) (Member, error) {
if a.db == nil {
return Member{}, errNoDB
}
- m := Member{Address: addr}
+ // SHEETS_OWNERS is the team; anybody else who reaches here arrived
+ // through a share and is a guest. Guest is the safe default: the
+ // wrong direction is unrecoverable in the way that matters, since a
+ // guest wrongly made a member sees every sheet on the instance.
+ role := TeamGuest
+ if a.admit != nil && a.admit(addr) {
+ role = TeamOwner
+ }
+
+ m := Member{Address: addr, TeamRole: string(role)}
if err := a.db.Clauses(clause.OnConflict{DoNothing: true}).Create(&m).Error; err != nil {
return Member{}, err
}
@@ -54,6 +63,15 @@ func (a *App) memberFor(address string) (Member, error) {
}
return Member{}, err
}
+ // Promote a row that predates the allowlist, or predates team roles
+ // existing at all. Never demote: a role somebody was given by hand
+ // is not this function's to take away.
+ if role == TeamOwner && TeamRole(existing.TeamRole) != TeamOwner {
+ if err := a.db.Model(&existing).Update("team_role", string(TeamOwner)).Error; err != nil {
+ return Member{}, err
+ }
+ existing.TeamRole = string(TeamOwner)
+ }
return existing, nil
}
diff --git a/internal/sheets/migrations.go b/internal/sheets/migrations.go
index 043442f..f18e6eb 100644
--- a/internal/sheets/migrations.go
+++ b/internal/sheets/migrations.go
@@ -50,5 +50,5 @@ var BootSchema = migrate.Merge(sessions.Schema, auth.Schema, Schema)
// moved.
var Models = []any{
store.Sheet{}, store.Tab{}, store.Row{}, store.Col{},
- store.Cell{}, store.Format{}, store.Event{}, Member{},
+ store.Cell{}, store.Format{}, store.Event{}, Member{}, Grant{},
}
diff --git a/internal/sheets/migrations/0004_alter_members.sql b/internal/sheets/migrations/0004_alter_members.sql
new file mode 100644
index 0000000..ba20d1d
--- /dev/null
+++ b/internal/sheets/migrations/0004_alter_members.sql
@@ -0,0 +1,8 @@
+ALTER TABLE `members` ADD `team_role` text;
+
+CREATE TABLE `grants` (`id` integer PRIMARY KEY AUTOINCREMENT,`sheet_id` integer,`member_id` integer,`role` text,`granted_by` integer,`created_at` datetime,`updated_at` datetime);
+
+CREATE INDEX `idx_grants_sheet_id` ON `grants`(`sheet_id`);
+
+CREATE UNIQUE INDEX `idx_grant_sheet_member` ON `grants`(`sheet_id`,`member_id`);
+
diff --git a/internal/sheets/migrations/schema.sql b/internal/sheets/migrations/schema.sql
index 0a63a05..5feda3e 100644
--- a/internal/sheets/migrations/schema.sql
+++ b/internal/sheets/migrations/schema.sql
@@ -9,7 +9,9 @@ CREATE TABLE `events` (`id` integer PRIMARY KEY AUTOINCREMENT,`kind` text,`tab_i
CREATE TABLE `formats` (`id` integer PRIMARY KEY AUTOINCREMENT,`digest` text,`number_fmt` text,`bold` numeric,`italic` numeric,`fill_intent_h` real,`fill_intent_c` real,`fill_hex` text,`align` text, `ink_hex` text);
-CREATE TABLE `members` (`id` integer PRIMARY KEY AUTOINCREMENT,`address` text,`created_at` datetime);
+CREATE TABLE `grants` (`id` integer PRIMARY KEY AUTOINCREMENT,`sheet_id` integer,`member_id` integer,`role` text,`granted_by` integer,`created_at` datetime,`updated_at` datetime);
+
+CREATE TABLE `members` (`id` integer PRIMARY KEY AUTOINCREMENT,`address` text,`created_at` datetime, `team_role` text);
CREATE TABLE `rows` (`id` integer PRIMARY KEY AUTOINCREMENT,`tab_id` integer,`position` integer,`height` integer);
@@ -25,6 +27,10 @@ CREATE INDEX `idx_events_tab_id` ON `events`(`tab_id`);
CREATE UNIQUE INDEX `idx_formats_digest` ON `formats`(`digest`);
+CREATE UNIQUE INDEX `idx_grant_sheet_member` ON `grants`(`sheet_id`,`member_id`);
+
+CREATE INDEX `idx_grants_sheet_id` ON `grants`(`sheet_id`);
+
CREATE UNIQUE INDEX `idx_members_address` ON `members`(`address`);
CREATE UNIQUE INDEX `idx_row_tab_pos` ON `rows`(`tab_id`,`position`);
diff --git a/internal/sheets/models.go b/internal/sheets/models.go
index 2d7c0f1..a75b7de 100644
--- a/internal/sheets/models.go
+++ b/internal/sheets/models.go
@@ -19,7 +19,94 @@ import "time"
// (0, false) for an email Subject, and dropping that ok scopes every
// query to uid 0 (AGENTS.md / Mechanical rules).
type Member struct {
+ ID uint64 `gorm:"primaryKey"`
+ Address string `gorm:"uniqueIndex;size:320"`
+ // TeamRole is this person's standing on the instance. It defaults
+ // to guest for anyone who arrives by being granted a single sheet;
+ // SHEETS_OWNERS promotes to owner at first sight. A guest is the
+ // safe default because the wrong direction is unrecoverable in the
+ // way that matters: a guest wrongly made a member sees every sheet.
+ TeamRole string `gorm:"size:16"`
+ CreatedAt time.Time
+}
+
+// TeamRole is a person's standing on this instance as a whole.
+//
+// Spec §8.2: owner, member, guest. A guest holds no team-wide rights —
+// they see exactly the sheets granted to them and learn nothing about
+// the team beyond §9.5's disclosure. That is the whole point of the
+// distinction, and it is why a guest cannot be given a team role by
+// accident: guests arrive by being granted one sheet.
+type TeamRole string
+
+const (
+ TeamOwner TeamRole = "owner"
+ TeamMember TeamRole = "member"
+ TeamGuest TeamRole = "guest"
+)
+
+// SheetRole is a person's standing on one sheet.
+//
+// Spec §8.2: owner, editor, commenter, viewer. Ordered, and the order
+// is load-bearing — see Allows, which is how every route asks its
+// question.
+type SheetRole string
+
+const (
+ SheetOwner SheetRole = "owner"
+ SheetEditor SheetRole = "editor"
+ SheetCommenter SheetRole = "commenter"
+ SheetViewer SheetRole = "viewer"
+)
+
+// rank orders sheet roles from most to least able. Unknown roles rank
+// below every real one, so a role this build does not recognise — a row
+// written by a newer version — grants nothing rather than everything.
+func (r SheetRole) rank() int {
+ switch r {
+ case SheetOwner:
+ return 4
+ case SheetEditor:
+ return 3
+ case SheetCommenter:
+ return 2
+ case SheetViewer:
+ return 1
+ }
+ return 0
+}
+
+// Allows reports whether this role is at least as able as want.
+//
+// Every authorization question in the app is this one function, so
+// there is one place to read and one place to get it wrong. Callers ask
+// `role.Allows(SheetEditor)`, never `role == SheetEditor` — the second
+// spelling silently excludes owners, and would do it on whichever route
+// somebody forgot.
+func (r SheetRole) Allows(want SheetRole) bool { return r.rank() >= want.rank() }
+
+// Valid reports whether a role is one this build knows.
+func (r SheetRole) Valid() bool { return r.rank() > 0 }
+
+// Grant is one person's access to one sheet.
+//
+// This table is the authority. Spec §9.1.1: every instance re-checks
+// authorization on every request, whatever the directory says — the
+// directory answers "where might I have things", never "may I have
+// this".
+//
+// Plaintext, per spec §2.2, which lists membership, grants and roles
+// among what this design deliberately does not seal.
+//
+// (SheetID, MemberID) is a uniqueIndex: a person has one role on a
+// sheet. Two rows would make "what may they do" depend on which the
+// query happened to return first.
+type Grant struct {
ID uint64 `gorm:"primaryKey"`
- Address string `gorm:"uniqueIndex;size:320"`
+ SheetID uint64 `gorm:"uniqueIndex:idx_grant_sheet_member;index"`
+ MemberID uint64 `gorm:"uniqueIndex:idx_grant_sheet_member"`
+ Role string `gorm:"size:16"`
+ GrantedBy uint64
CreatedAt time.Time
+ UpdatedAt time.Time
}
diff --git a/internal/sheets/sheetsvc.go b/internal/sheets/sheetsvc.go
index 2a5f0a4..fa2511b 100644
--- a/internal/sheets/sheetsvc.go
+++ b/internal/sheets/sheetsvc.go
@@ -95,7 +95,17 @@ func (a *App) createSheet(title string, by Member) (SheetSummary, error) {
return err
}
tab.SheetID = sheet.ID
- return tx.Create(&tab).Error
+ if err := tx.Create(&tab).Error; err != nil {
+ return err
+ }
+ // The creator owns it, written here rather than inferred from
+ // Sheet.CreatedBy. Inferring would mean two sources of truth
+ // for one question, and the share screen could not then take
+ // ownership away or hand it on.
+ return tx.Create(&Grant{
+ SheetID: sheet.ID, MemberID: by.ID,
+ Role: string(SheetOwner), GrantedBy: by.ID,
+ }).Error
}); err != nil {
return SheetSummary{}, err
}
@@ -104,26 +114,27 @@ func (a *App) createSheet(title string, by Member) (SheetSummary, error) {
}, nil
}
-// listSheets returns every live sheet on the instance, most recently
+// listSheets returns the sheets a member may see, most recently
// touched first.
//
-// Deliberately unscoped by creator. Sheets are team-shared, and
-// scope.Owned here would return only the caller's own rows — failing as
-// a plausible short list rather than as an error, which is the worst
-// available failure shape and also the default thing to reach for
-// (AGENTS.md / Mechanical rules; TestListSheetsIsInstanceWideNotPerCreator
-// is the guard). When spec §8's grants land, this grows a hand-written
-// grants join. It does NOT grow a scope.Owned.
-func (a *App) listSheets() ([]SheetSummary, error) {
+// It grew the hand-written grants join the previous comment here
+// promised — visibleSheets, grants.go — and it did NOT grow a
+// scope.Owned. scope.Owned is single-column equality on user_id, our
+// sheets are team-shared, and applied here it would return only the
+// sheets somebody created: a plausible short list rather than an error
+// (AGENTS.md / Mechanical rules).
+//
+// A team owner or member sees every sheet, which is what one instance
+// per team means. A guest sees exactly what they were granted.
+func (a *App) listSheets(viewer Member) ([]SheetSummary, error) {
if a.db == nil {
return nil, errNoDB
}
if a.sealer == nil {
return nil, errNoSealer
}
- var rows []store.Sheet
- if err := a.db.Where("deleted_at IS NULL").
- Order("updated_at DESC, id DESC").Find(&rows).Error; err != nil {
+ rows, err := a.visibleSheets(viewer)
+ if err != nil {
return nil, err
}
out := make([]SheetSummary, 0, len(rows))
@@ -137,16 +148,23 @@ func (a *App) listSheets() ([]SheetSummary, error) {
return out, nil
}
-// sheetSummary reads one sheet. A sheet that is not there is
-// gorm.ErrRecordNotFound, which handlers turn into a 404 — never a 403
-// (AGENTS.md: a row that isn't yours 404s).
-func (a *App) sheetSummary(id uint64) (SheetSummary, error) {
+// sheetSummary reads one sheet a viewer may see.
+//
+// A sheet that is not there and a sheet they may not reach are the same
+// answer, on purpose: handlers turn both into a 404, never a 403
+// (AGENTS.md). A 403 confirms the row exists, and on an instance where
+// guests hold single-sheet grants the set of ids that 403 is a map of
+// the team's sheets.
+func (a *App) sheetSummary(id uint64, viewer Member) (SheetSummary, error) {
if a.db == nil {
return SheetSummary{}, errNoDB
}
if a.sealer == nil {
return SheetSummary{}, errNoSealer
}
+ if !a.mayRead(id, viewer) {
+ return SheetSummary{}, errNoAccess
+ }
var s store.Sheet
if err := a.db.Where("id = ? AND deleted_at IS NULL", id).First(&s).Error; err != nil {
return SheetSummary{}, err
diff --git a/internal/sheets/sheetsvc_test.go b/internal/sheets/sheetsvc_test.go
index f8b720f..90229f0 100644
--- a/internal/sheets/sheetsvc_test.go
+++ b/internal/sheets/sheetsvc_test.go
@@ -67,7 +67,11 @@ func TestTabNameIsSealedAtRest(t *testing.T) {
// This is the scope.Owned trap AGENTS.md names. The wrong
// implementation returns one row and reads as a slightly short list
// rather than as an error.
-func TestListSheetsIsInstanceWideNotPerCreator(t *testing.T) {
+//
+// Team standing, not creation, is what makes both rows visible: one
+// instance per team means a colleague reaches every sheet. A guest is
+// the other half of §8.2 and has its own suite in grants_test.go.
+func TestListSheetsIsTeamWideNotPerCreator(t *testing.T) {
app := newTestApp(t)
ada, err := app.memberFor("ada@example.com")
if err != nil {
@@ -83,7 +87,7 @@ func TestListSheetsIsInstanceWideNotPerCreator(t *testing.T) {
if _, err := app.createSheet("Bob's", bob); err != nil {
t.Fatalf("createSheet bob: %v", err)
}
- got, err := app.listSheets()
+ got, err := app.listSheets(ada)
if err != nil {
t.Fatalf("listSheets: %v", err)
}
@@ -126,10 +130,12 @@ func TestCreateSheetTruncatesAnOverlongTitle(t *testing.T) {
func TestSheetSummaryForAMissingSheetIsNotFound(t *testing.T) {
app := newTestApp(t)
- _, err := app.sheetSummary(999999)
- if !errors.Is(err, gorm.ErrRecordNotFound) {
- t.Fatalf("sheetSummary of a missing sheet = %v, want gorm.ErrRecordNotFound "+
- "(handlers turn that into a 404, never a 403)", err)
+ _, err := app.sheetSummary(999999, testMember(t, app))
+ // errNoAccess, not ErrRecordNotFound: the grant check runs first,
+ // and a sheet nobody can reach is indistinguishable from one that
+ // is not there. Handlers turn both into the same 404.
+ if !errors.Is(err, errNoAccess) && !errors.Is(err, gorm.ErrRecordNotFound) {
+ t.Fatalf("sheetSummary of a missing sheet = %v, want a not-found-shaped error", err)
}
}
diff --git a/internal/sheets/signin_test.go b/internal/sheets/signin_test.go
index c42b260..75a7626 100644
--- a/internal/sheets/signin_test.go
+++ b/internal/sheets/signin_test.go
@@ -76,7 +76,7 @@ func testAuth(t *testing.T) *App {
InstanceKey: "test-instance-key",
Owners: []string{"paul@example.com"},
Mailer: nil, // auth falls back to a logging sender; no mail is sent in this test
- }, quietLogger())
+ }, quietLogger(), Admits([]string{"paul@example.com"}))
if err != nil {
t.Fatalf("NewAuth: %v", err)
}