amadan

oficina / sheets Public

feat: a share screen — invite, set a role, revoke

Scriptless, because an invite is exactly the thing somebody does from a
locked-down machine.

Spec 9.5's ruling is honoured in words on the screen itself: sharing
with somebody outside the team puts a row in their directory carrying
the team's name, the behaviour is kept, and the obligation that follows
is to say so before somebody uses it rather than leave it an emergent
property nobody decided. A test asserts the disclosure is there.

Sheet owners only, and everybody else gets a 404 — somebody who can
read but not share learns nothing new, and somebody who can do neither
learns nothing at all.

The last owner cannot be removed. A sheet with no owner can never be
shared again and there is no admin path to repair it, so refusing is
the difference between a mistake and an unrecoverable one. Ownership
can still be handed on, because the alternative to offering the owner
role is that transferring a sheet needs a database edit.

The notice is a code selecting a fixed sentence, never text from the
query — the same shape signinMessage uses, and for the same reason.
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 b31a40ed41d84b1f79efdd52a1541c66b37b5abc parent a8d07b5
6 files changed, +452 −0
  • internal/sheets/app.go +4 −0
  • internal/sheets/handlers_share.go +167 −0
  • internal/sheets/handlers_share_test.go +207 −0
  • internal/sheets/handlers_sheets.go +7 −0
  • internal/sheets/templates/share.html +66 −0
  • internal/sheets/templates/sheet.html +1 −0

CI passed — run details

diff --git a/internal/sheets/app.go b/internal/sheets/app.go
index af9e972..78bf53c 100644
--- a/internal/sheets/app.go
+++ b/internal/sheets/app.go
@@ -144,6 +144,10 @@ func (a *App) Router() http.Handler {
// else, including editing any cell, works here.
r.Get("/sheets/{sheetID}/table", a.handleTable)
r.Post("/sheets/{sheetID}/table", a.handleTableWrite)
+ // Sharing. Sheet owners only — see sheetForSharing.
+ r.Get("/sheets/{sheetID}/share", a.handleShare)
+ r.Post("/sheets/{sheetID}/share", a.handleShareGrant)
+ r.Post("/sheets/{sheetID}/share/revoke", a.handleShareRevoke)
// The grid's own API. Under /api because it answers JSON and
// carries no chrome; behind the same session as everything
diff --git a/internal/sheets/handlers_share.go b/internal/sheets/handlers_share.go
new file mode 100644
index 0000000..21c9c42
--- /dev/null
+++ b/internal/sheets/handlers_share.go
@@ -0,0 +1,167 @@
+package sheets
+
+import (
+ "net/http"
+ "strconv"
+)
+
+// shareView is the share screen's payload.
+type shareView struct {
+ Sheet SheetSummary
+ People []sharedWith
+ Roles []SheetRole
+ Notice string
+}
+
+// shareableRoles is what the screen offers, most able first.
+//
+// Owner is offered: handing a sheet on is a real thing to want, and
+// leaving it out means the only way to transfer one is a database
+// edit.
+var shareableRoles = []SheetRole{SheetOwner, SheetEditor, SheetCommenter, SheetViewer}
+
+// handleShare renders who can reach a sheet.
+func (a *App) handleShare(w http.ResponseWriter, r *http.Request) {
+ s, viewer, ok := a.sheetForSharing(w, r)
+ if !ok {
+ return
+ }
+ a.renderShare(w, r, s, viewer, r.URL.Query().Get("notice"))
+}
+
+func (a *App) renderShare(w http.ResponseWriter, r *http.Request, s SheetSummary, _ Member, notice string) {
+ people, err := a.grantsOn(s.ID)
+ if err != nil {
+ a.fail(w, r, err)
+ return
+ }
+ a.render(w, r, "share", "Sharing "+s.Title, http.StatusOK, shareView{
+ Sheet: s, People: people, Roles: shareableRoles, Notice: noticeText(notice),
+ })
+}
+
+// noticeText maps a code to a fixed sentence.
+//
+// A code, never the text itself: the value comes from a query string,
+// and a page that echoed it would take a redirect parameter straight
+// into the document. This is the same shape signinMessage uses, for the
+// same reason.
+func noticeText(code string) string {
+ switch code {
+ case "shared":
+ return "Shared. They can sign in with that address to open it."
+ case "revoked":
+ return "Access removed."
+ case "bad-address":
+ return "That does not look like an email address."
+ case "bad-role":
+ return "Unknown role."
+ case "last-owner":
+ return "A sheet needs an owner. Make somebody else the owner first."
+ default:
+ return ""
+ }
+}
+
+// handleShareGrant adds or changes somebody's access.
+func (a *App) handleShareGrant(w http.ResponseWriter, r *http.Request) {
+ s, viewer, ok := a.sheetForSharing(w, r)
+ if !ok {
+ return
+ }
+ role := SheetRole(r.FormValue("role"))
+ if !role.Valid() {
+ a.shareRedirect(w, r, s, "bad-role")
+ return
+ }
+ if _, err := a.grant(s.ID, r.FormValue("address"), role, viewer); err != nil {
+ // The address is not echoed back. It is somebody's email, and
+ // an error string is a log line waiting to happen.
+ a.shareRedirect(w, r, s, "bad-address")
+ return
+ }
+ a.shareRedirect(w, r, s, "shared")
+}
+
+// handleShareRevoke removes somebody's access.
+func (a *App) handleShareRevoke(w http.ResponseWriter, r *http.Request) {
+ s, _, ok := a.sheetForSharing(w, r)
+ if !ok {
+ return
+ }
+ memberID, err := strconv.ParseUint(r.FormValue("member"), 10, 64)
+ if err != nil || memberID == 0 {
+ a.notFound(w, r)
+ return
+ }
+ // A sheet with no owner is a sheet nobody can ever share again, and
+ // there is no admin path to repair it. Refusing the last one is the
+ // difference between a mistake and an unrecoverable mistake.
+ if a.wouldOrphan(s.ID, memberID) {
+ a.shareRedirect(w, r, s, "last-owner")
+ return
+ }
+ if err := a.revoke(s.ID, memberID); err != nil {
+ a.fail(w, r, err)
+ return
+ }
+ a.shareRedirect(w, r, s, "revoked")
+}
+
+// wouldOrphan reports whether removing this grant would leave the sheet
+// with no owner at all.
+func (a *App) wouldOrphan(sheetID, memberID uint64) bool {
+ people, err := a.grantsOn(sheetID)
+ if err != nil {
+ return true // cannot tell means do not risk it
+ }
+ var owners, losing int
+ for _, p := range people {
+ if p.Role != SheetOwner {
+ continue
+ }
+ owners++
+ if p.Member.ID == memberID {
+ losing++
+ }
+ }
+ return owners-losing < 1
+}
+
+// sheetForSharing loads the sheet and proves the viewer may change who
+// can reach it.
+//
+// Sheet owners only. Changing who else can read a sheet is the one
+// power the people it affects cannot undo, so it does not come with
+// team standing — see roleFor, which hands colleagues editor and not
+// owner for exactly this reason.
+func (a *App) sheetForSharing(w http.ResponseWriter, r *http.Request) (SheetSummary, Member, bool) {
+ s, ok := a.sheetFromURL(w, r)
+ if !ok {
+ return SheetSummary{}, Member{}, false
+ }
+ viewer, ok := a.actor(r)
+ if !ok {
+ a.notFound(w, r)
+ return SheetSummary{}, Member{}, false
+ }
+ if !a.mayShare(s.ID, viewer) {
+ // 404, not 403: somebody who can read a sheet but not share it
+ // learns nothing new, and somebody who can do neither learns
+ // nothing at all.
+ a.notFound(w, r)
+ return SheetSummary{}, Member{}, false
+ }
+ return s, viewer, true
+}
+
+// shareRedirect answers a POST with a 303 back to the screen.
+//
+// The notice is a fixed code, and the sheet id is re-formatted from a
+// uint64 rather than passed through — both end up in a Location header,
+// and a number that came back out as a number cannot carry anything
+// else.
+func (a *App) shareRedirect(w http.ResponseWriter, r *http.Request, s SheetSummary, notice string) {
+ http.Redirect(w, r, "/sheets/"+strconv.FormatUint(s.ID, 10)+"/share?notice="+notice,
+ http.StatusSeeOther)
+}
diff --git a/internal/sheets/handlers_share_test.go b/internal/sheets/handlers_share_test.go
new file mode 100644
index 0000000..7a9c3cb
--- /dev/null
+++ b/internal/sheets/handlers_share_test.go
@@ -0,0 +1,207 @@
+package sheets
+
+import (
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+)
+
+func sharePath(s SheetSummary) string { return fmt.Sprintf("/sheets/%d/share", s.ID) }
+
+func TestSharingAddsAGrantAndRedirects(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ form := url.Values{"address": {"ada@elsewhere.example"}, "role": {string(SheetEditor)}}
+ rec := do(t, app, as(postForm(sharePath(s), form), cookies))
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("POST share = %d, want 303: %s", rec.Code, rec.Body.String())
+ }
+
+ ada, err := app.memberFor("ada@elsewhere.example")
+ if err != nil {
+ t.Fatalf("memberFor: %v", err)
+ }
+ role, ok := app.roleFor(s.ID, ada)
+ if !ok || role != SheetEditor {
+ t.Fatalf("after sharing, role = %q (%v), want editor", role, ok)
+ }
+
+ body := do(t, app, as(httptest.NewRequest(http.MethodGet, sharePath(s), nil), cookies)).Body.String()
+ if !strings.Contains(body, "ada@elsewhere.example") {
+ t.Error("the share screen does not list the person it just shared with")
+ }
+}
+
+// Spec §9.5, ruled: the behaviour is kept and the obligation is to
+// disclose it in words. A guest grant discloses the granting team's
+// name, and the screen has to say so before somebody uses it.
+func TestTheShareScreenDisclosesTheTeamNameLeak(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ body := do(t, app, as(httptest.NewRequest(http.MethodGet, sharePath(s), nil), cookies)).Body.String()
+ // Matched without the apostrophe: html/template escapes it to
+ // &#39;, so asserting on the prose as typed fails on copy that is
+ // actually there — a test failing for a reason unrelated to what it
+ // is checking.
+ for _, want := range []string{"see your team", "name"} {
+ if !strings.Contains(body, want) {
+ t.Fatalf("the share screen does not disclose that sharing reveals the "+
+ "team's name (spec §9.5): missing %q", want)
+ }
+ }
+}
+
+// Only a sheet owner may change who else can reach it, and everybody
+// else gets a 404 rather than a 403.
+func TestOnlyAnOwnerCanReachTheShareScreen(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetEditor)
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ if rec := do(t, app, as(httptest.NewRequest(http.MethodGet, sharePath(s), nil), guestCookies)); rec.Code != http.StatusNotFound {
+ t.Errorf("an editor reached the share screen: %d", rec.Code)
+ }
+ form := url.Values{"address": {"eve@elsewhere.example"}, "role": {string(SheetEditor)}}
+ if rec := do(t, app, as(postForm(sharePath(s), form), guestCookies)); rec.Code != http.StatusNotFound {
+ t.Errorf("an editor shared a sheet they do not own: %d", rec.Code)
+ }
+ // And the grant must not exist.
+ eve, err := app.memberFor("eve@elsewhere.example")
+ if err != nil {
+ t.Fatalf("memberFor: %v", err)
+ }
+ if _, ok := app.roleFor(s.ID, eve); ok {
+ t.Fatal("an editor's share attempt created a grant anyway")
+ }
+}
+
+func TestTheSheetPageOffersShareOnlyToAnOwner(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetEditor)
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ page := fmt.Sprintf("/sheets/%d", s.ID)
+ owner := do(t, app, as(httptest.NewRequest(http.MethodGet, page, nil), cookies)).Body.String()
+ if !strings.Contains(owner, sharePath(s)) {
+ t.Error("the owner is not offered a way to share")
+ }
+ editor := do(t, app, as(httptest.NewRequest(http.MethodGet, page, nil), guestCookies)).Body.String()
+ if strings.Contains(editor, sharePath(s)) {
+ t.Error("an editor is offered a Share link that would 404")
+ }
+}
+
+func TestRevokingThroughTheScreen(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ ada := guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+
+ form := url.Values{"member": {fmt.Sprint(ada.ID)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/revoke", form), cookies))
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("revoke = %d, want 303", rec.Code)
+ }
+ if app.mayRead(s.ID, ada) {
+ t.Fatal("the revoked guest can still read")
+ }
+}
+
+// A sheet with no owner can never be shared again and there is no admin
+// path to repair it. Refusing the last one is the difference between a
+// mistake and an unrecoverable mistake.
+func TestTheLastOwnerCannotBeRemoved(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ me := signedInMember(t, app)
+
+ form := url.Values{"member": {fmt.Sprint(me.ID)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/revoke", form), cookies))
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("revoke = %d", rec.Code)
+ }
+ if loc := rec.Header().Get("Location"); !strings.Contains(loc, "last-owner") {
+ t.Errorf("Location = %q, want the last-owner refusal", loc)
+ }
+ if !app.mayShare(s.ID, me) {
+ t.Fatal("the last owner was removed; the sheet can never be shared again")
+ }
+}
+
+// Handing a sheet on has to be possible, or the only way to transfer
+// one is a database edit.
+func TestOwnershipCanBeHandedOn(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ me := signedInMember(t, app)
+
+ form := url.Values{"address": {"ada@elsewhere.example"}, "role": {string(SheetOwner)}}
+ if rec := do(t, app, as(postForm(sharePath(s), form), cookies)); rec.Code != http.StatusSeeOther {
+ t.Fatalf("granting ownership = %d", rec.Code)
+ }
+ // Now the original owner can step back, because they are no longer
+ // the last one.
+ revoke := url.Values{"member": {fmt.Sprint(me.ID)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/revoke", revoke), cookies))
+ if loc := rec.Header().Get("Location"); strings.Contains(loc, "last-owner") {
+ t.Fatal("stepping back was refused even though a second owner exists")
+ }
+}
+
+func TestShareRefusesAnUnknownRole(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ form := url.Values{"address": {"ada@elsewhere.example"}, "role": {"admin"}}
+ rec := do(t, app, as(postForm(sharePath(s), form), cookies))
+ if loc := rec.Header().Get("Location"); !strings.Contains(loc, "bad-role") {
+ t.Fatalf("Location = %q, want the bad-role refusal", loc)
+ }
+ ada, _ := app.memberFor("ada@elsewhere.example")
+ if _, ok := app.roleFor(s.ID, ada); ok {
+ t.Fatal("an unknown role still created a grant")
+ }
+}
+
+// The notice is a code that selects a fixed sentence, never text from
+// the query. A page that echoed it would take a redirect parameter
+// straight into the document.
+func TestTheNoticeIsNeverEchoed(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ const probe = "<script>alert(1)</script>"
+ body := do(t, app, as(httptest.NewRequest(http.MethodGet,
+ sharePath(s)+"?notice="+url.QueryEscape(probe), nil), cookies)).Body.String()
+ if strings.Contains(body, "alert(1)") {
+ t.Fatal("the share screen echoed its notice parameter")
+ }
+ if noticeText(probe) != "" {
+ t.Fatal("noticeText returned something for an unknown code")
+ }
+}
+
+// Sharing is chrome that must work with scripting off: an invite is
+// exactly the thing somebody does from a locked-down machine.
+func TestTheShareScreenIsScriptless(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+
+ body := withoutScript(do(t, app,
+ as(httptest.NewRequest(http.MethodGet, sharePath(s), nil), cookies)).Body.String())
+ for _, want := range []string{
+ fmt.Sprintf(`action="/sheets/%d/share"`, s.ID),
+ `name="address"`,
+ `name="role"`,
+ } {
+ if !strings.Contains(body, want) {
+ t.Errorf("with scripts stripped, %q is gone", want)
+ }
+ }
+}
diff --git a/internal/sheets/handlers_sheets.go b/internal/sheets/handlers_sheets.go
index 6a04dd0..dbf2108 100644
--- a/internal/sheets/handlers_sheets.go
+++ b/internal/sheets/handlers_sheets.go
@@ -23,6 +23,10 @@ type sheetPageView struct {
Sheet SheetSummary
Rows int
Cols int
+ // MayShare drives whether the page offers a Share link. The route
+ // checks again — this only decides what to render, and a button
+ // that 404s is worse than no button.
+ MayShare bool
// Warned lists cells whose IMPORTED fill and ink fail WCAG AA.
// Spec §11.1 ruled that such a pair is rendered as authored and
// that the obligation is surfacing, not overriding — so this is a
@@ -81,6 +85,9 @@ func (a *App) handleSheetPage(w http.ResponseWriter, r *http.Request) {
return
}
view := sheetPageView{Sheet: s, Rows: defaultWindowRows, Cols: defaultWindowCols}
+ if viewer, ok := a.actor(r); ok {
+ view.MayShare = a.mayShare(s.ID, viewer)
+ }
// Best effort: a sheet whose handle will not open still renders its
// page, and the grid's own fetch reports the failure properly.
if h, err := a.openTabFor(s); err == nil {
diff --git a/internal/sheets/templates/share.html b/internal/sheets/templates/share.html
new file mode 100644
index 0000000..1074217
--- /dev/null
+++ b/internal/sheets/templates/share.html
@@ -0,0 +1,66 @@
+{{define "content"}}
+<header rst-page-header>
+<div rst-page-header-titles>
+<h1>Sharing “{{.Data.Sheet.Title}}”</h1>
+<p rst-page-header-sub>Who can open this sheet, and what they can do with it.</p>
+</div>
+<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}">Back to the sheet</a>
+</header>
+
+{{with .Data.Notice}}
+<div rst-callout><div rst-callout-body><p>{{.}}</p></div></div>
+{{end}}
+
+<form rst-form method="post" action="/sheets/{{.Data.Sheet.ID}}/share">
+<div rst-field-row>
+<div rst-field>
+<label rst-field-label for="address">Share with</label>
+<input rst-input id="address" name="address" type="email" autocomplete="off"
+ placeholder="name@example.com" required>
+</div>
+<div rst-field>
+<label rst-field-label for="role">Can</label>
+<select rst-input id="role" name="role">
+{{range .Data.Roles}}<option value="{{.}}">{{.}}</option>{{end}}
+</select>
+</div>
+<button rst-btn="primary" type="submit">Share</button>
+</div>
+</form>
+
+{{/* Spec §9.5, ruled: a guest grant puts a row in that person's
+ directory carrying the granting team's name. The behaviour is kept
+ and the obligation is to disclose it in words, here, rather than
+ leave it an emergent property nobody decided. */}}
+<div rst-callout>
+<div rst-callout-body>
+<p>Sharing with somebody outside your team lets them see your team's
+name, because this sheet then appears in their list alongside their own.
+They see nothing else about the team, and only the sheets you share.</p>
+</div>
+</div>
+
+<h2>Who has access</h2>
+{{if .Data.People}}
+<ul rst-list>
+{{$sheet := .Data.Sheet}}
+{{range .Data.People}}
+<li rst-lrow>
+<div rst-row-main>
+<span>{{.Member.Address}}</span>
+<span rst-row-sub>{{.Role}}{{if eq .Member.TeamRole "guest"}} · guest{{end}}</span>
+</div>
+<form method="post" action="/sheets/{{$sheet.ID}}/share/revoke">
+<input type="hidden" name="member" value="{{.Member.ID}}">
+<button rst-btn="ghost" class="rst-danger" type="submit">Remove</button>
+</form>
+</li>
+{{end}}
+</ul>
+{{else}}
+<div rst-empty>
+<p rst-empty-title>Nobody yet</p>
+<p rst-empty-body>Only you can open this sheet.</p>
+</div>
+{{end}}
+{{end}}
diff --git a/internal/sheets/templates/sheet.html b/internal/sheets/templates/sheet.html
index 668dd36..56aa0d7 100644
--- a/internal/sheets/templates/sheet.html
+++ b/internal/sheets/templates/sheet.html
@@ -40,6 +40,7 @@
<button rst-btn="ghost" type="button" data-action="fill-down" hidden>Fill down</button>
<button rst-btn="ghost" type="button" data-action="fill-right" hidden>Fill right</button>
<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/table">Table view</a>
+{{if .Data.MayShare}}<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/share">Share</a>{{end}}
<span data-sheet-live data-state="off" role="status">Connecting</span>
</div>