amadan

oficina / sheets Public

fix: revoking a teammate's grant stops claiming it removed their access

Issue oficina/sheets #7, the last of the five Fable findings left open
on core.

Removing somebody's grant said "Access removed." For a guest that is
true. For a teammate it is not: team standing reaches every sheet on the
instance as an editor by design (spec §8.2), and an explicit grant sits
on top of that rather than replacing it. So an owner could remove a
colleague's grant, read that access was removed, walk away, and believe
they had shut somebody out of a sealed sheet.

The notice now says what happened, in Paul's own words:

    The explicit permission is removed, but since <address> is on the
    team, they still have access.

**The person is named, and that is not decoration.** Revoking the grant
also takes them off the list below it, so an unnamed sentence would
refer to somebody the owner can no longer see anywhere on the screen.

**The id travels in the query and the address does not.** An email in a
request line is an email in the platform edge's access logs — the same
leak the share-link token was moved out of four commits ago — while a
member id is already in every revoke form's hidden field on this screen.
The address is read back from the row, and only after checking that the
sheet really does still admit that member, because the id came from a
URL.

The decision to report rather than to check is asked of `roleFor` after
the revoke, so the sentence reports what the gate actually says instead
of holding a second opinion that can drift from it.

**What this does NOT fix, ruled by Paul on 2026-09-01.** Because an
explicit grant takes precedence over team standing, it is also the only
way to hold a teammate BELOW editor — so removing a viewer grant from a
teammate widens their access rather than narrowing it. Measured, not
inferred: before revoke `role="viewer" mayWrite=false`, after revoke
`role="editor" mayWrite=true`. A button labelled Remove, styled as a
danger action, grants edit access. Paul's ruling is that the sentence is
what changes here and the behaviour stays; that is recorded as its own
open issue rather than being left to be rediscovered, because a notice
that describes an escalation politely is still an escalation.

The guest case keeps the plain sentence, and its test is the one that
matters: a fix that hedged every revoke would be its own kind of lie,
and without that test it would have passed.

make test green, including -race.

🤖 Authored by Claude Opus 5, directed by Paul. Notice wording is Paul's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0114wZuXnD6aGcDCzL39NpXD
Paul Campbell pushed by paul@keymail.dev 9b723a5b9e8488b46d66fc93eefa0cac934235c1 parent 1c7f43c
4 files changed, +216 −0
  • internal/sheets/grants_revoke_notice_test.go +104 −0
  • internal/sheets/handlers_share.go +21 −0
  • internal/sheets/members.go +18 −0
  • internal/sheets/sharenotice.go +73 −0

CI passed — run details

Intent

Attributed to this commit: 121 g CO₂e (10264220 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on fix/revoke-notice-team-standing, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.

full ledger: fix/revoke-notice-team-standing

diff --git a/internal/sheets/grants_revoke_notice_test.go b/internal/sheets/grants_revoke_notice_test.go
new file mode 100644
index 0000000..c85cf06
--- /dev/null
+++ b/internal/sheets/grants_revoke_notice_test.go
@@ -0,0 +1,104 @@
+package sheets
+
+import (
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+)
+
+func httpGET(path string) *http.Request {
+ return httptest.NewRequest(http.MethodGet, path, nil)
+}
+
+// teammateOn is somebody with BOTH an explicit grant and team standing
+// — a colleague who was also shared with directly. Team standing is
+// what makes revoking the grant not the same as removing their access,
+// so it is the whole point of the fixture.
+func teammateOn(t *testing.T, app *App, sheetID uint64, address string, role SheetRole) Member {
+ t.Helper()
+ m := guestOn(t, app, sheetID, address, role)
+ if err := app.db.Model(&Member{}).Where("id = ?", m.ID).
+ Update("team_role", string(TeamMember)).Error; err != nil {
+ t.Fatalf("put %s on the team: %v", address, err)
+ }
+ m.TeamRole = string(TeamMember)
+ return m
+}
+
+// Removing a teammate's explicit grant removes the grant and does not
+// remove their access: team standing reaches every sheet on the
+// instance as an editor, by design (spec §8.2). The fallback is right
+// and stays. Saying "Access removed." on top of it is not.
+//
+// An owner who reads that and walks away believes they have shut
+// somebody out of a sealed sheet. They have not, and nothing on the
+// screen tells them.
+func TestRevokingATeammatesGrantDoesNotClaimTheyLostAccess(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ m := teammateOn(t, app, s.ID, "ada@example.com", SheetViewer)
+
+ form := url.Values{"member": {fmt.Sprint(m.ID)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/revoke", form), cookies))
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("revoke = %d: %s", rec.Code, rec.Body.String())
+ }
+
+ // The premise. If this ever stops holding, the notice below is not
+ // the thing to change — the fallback is.
+ if !app.mayWrite(s.ID, m) {
+ t.Fatal("a team member lost access when their explicit grant went; " +
+ "team standing is supposed to reach every sheet")
+ }
+
+ loc := rec.Header().Get("Location")
+ if strings.HasSuffix(loc, "notice=revoked") {
+ t.Fatalf("Location = %q — told the owner access was removed while "+
+ "team standing still admits the person", loc)
+ }
+ if !strings.Contains(loc, "notice=revoked-team") {
+ t.Fatalf("Location = %q, want the notice that says they still reach it", loc)
+ }
+
+ // An email address must not travel in the query — that is the same
+ // access-log leak the share token was just moved out of. The id
+ // does, and the address is read back from it server-side.
+ if strings.Contains(loc, "ada@example.com") ||
+ strings.Contains(loc, url.QueryEscape("ada@example.com")) {
+ t.Fatalf("Location = %q carries an email address", loc)
+ }
+
+ // The sentence has to name them, because revoking the grant also
+ // takes them off the list below: without a name the notice refers
+ // to somebody the owner can no longer see on the screen.
+ page := do(t, app, as(httpGET(loc), cookies)).Body.String()
+ if !strings.Contains(page, "ada@example.com") {
+ t.Fatal("the notice does not name the person it is about")
+ }
+ if !strings.Contains(page, "on the team") {
+ t.Fatalf("the notice does not say why they still reach it")
+ }
+}
+
+// The plain case has to keep the plain sentence: a guest with no team
+// standing really does lose the sheet, and a fix that hedged every
+// revoke would be its own kind of lie.
+func TestRevokingAGuestsGrantStillReportsAPlainRemoval(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Budget")
+ m := guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+
+ form := url.Values{"member": {fmt.Sprint(m.ID)}}
+ rec := do(t, app, as(postForm(sharePath(s)+"/revoke", form), cookies))
+
+ if app.mayRead(s.ID, m) {
+ t.Fatal("a guest kept access after their only grant was revoked")
+ }
+ if loc := rec.Header().Get("Location"); !strings.Contains(loc, "notice=revoked") ||
+ strings.Contains(loc, "revoked-team") {
+ t.Fatalf("Location = %q, want the plain removal notice", loc)
+ }
+}
diff --git a/internal/sheets/handlers_share.go b/internal/sheets/handlers_share.go
index a429bbc..77e51a6 100644
--- a/internal/sheets/handlers_share.go
+++ b/internal/sheets/handlers_share.go
@@ -83,6 +83,9 @@ func (a *App) renderShare(w http.ResponseWriter, r *http.Request, s SheetSummary
Sheet: s, People: people, Roles: shareableRoles,
LinkRoles: linkRoles, Links: rows, Notice: noticeText(notice),
}
+ if notice == noticeRevokedTeam {
+ view.Notice = a.revokedTeamNotice(r, s.ID)
+ }
if newLink != "" {
view.NewLink = a.linkURL(newLink)
}
@@ -101,6 +104,11 @@ func noticeText(code string) string {
return "Shared. They can sign in with that address to open it."
case "revoked":
return "Access removed."
+ case noticeRevokedTeam:
+ // renderShare replaces this with the named version when it can
+ // resolve who the revoke was about. This is the honest sentence
+ // when it cannot.
+ return revokedTeamText
case "bad-address":
return "That does not look like an email address."
case "bad-role":
@@ -173,6 +181,10 @@ func (a *App) handleShareRevoke(w http.ResponseWriter, r *http.Request) {
a.fail(w, r, err)
return
}
+ if a.stillReaches(s.ID, memberID) {
+ a.shareRedirectAbout(w, r, s, noticeRevokedTeam, memberID)
+ return
+ }
a.shareRedirect(w, r, s, "revoked")
}
@@ -229,6 +241,15 @@ func (a *App) sheetForSharing(w http.ResponseWriter, r *http.Request) (SheetSumm
// 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.
+// shareRedirectAbout is shareRedirect for a notice that has to name
+// somebody. It carries the member id and never the address: an address
+// in a request line is an address in the platform's access logs.
+func (a *App) shareRedirectAbout(w http.ResponseWriter, r *http.Request, s SheetSummary,
+ notice string, memberID uint64) {
+ http.Redirect(w, r, "/sheets/"+strconv.FormatUint(s.ID, 10)+"/share?notice="+notice+
+ "&who="+strconv.FormatUint(memberID, 10), http.StatusSeeOther)
+}
+
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/members.go b/internal/sheets/members.go
index 60c187c..13c2ac1 100644
--- a/internal/sheets/members.go
+++ b/internal/sheets/members.go
@@ -92,3 +92,21 @@ func (a *App) actor(r *http.Request) (Member, bool) {
}
return m, true
}
+
+// memberByID reads one member row.
+//
+// It exists for the share screen's revoke path, which has an id from a
+// form and needs the person's TEAM standing — the field that decides
+// whether removing their grant actually removed anything. Reading the
+// row is the only way to know: team standing is not in the grants
+// table, which is exactly why revoke could not see it.
+func (a *App) memberByID(id uint64) (Member, bool) {
+ if a.db == nil || id == 0 {
+ return Member{}, false
+ }
+ var m Member
+ if err := a.db.Where("id = ?", id).First(&m).Error; err != nil {
+ return Member{}, false
+ }
+ return m, true
+}
diff --git a/internal/sheets/sharenotice.go b/internal/sheets/sharenotice.go
new file mode 100644
index 0000000..a0839db
--- /dev/null
+++ b/internal/sheets/sharenotice.go
@@ -0,0 +1,73 @@
+package sheets
+
+import (
+ "net/http"
+ "strconv"
+)
+
+// noticeRevokedTeam is the code for a revoke that removed a grant and
+// did not remove anybody's access.
+//
+// Team standing reaches every sheet on the instance as an editor, by
+// design (spec §8.2), and an explicit grant sits on top of it. So
+// removing a teammate's grant takes the grant away and leaves the
+// person able to open the sheet. The screen used to report the first
+// half and stay silent about the second, which let an owner read
+// "Access removed", walk away, and believe they had shut somebody out
+// of a sealed sheet.
+//
+// RULED by Paul, 2026-09-01: the sentence is what changes here, not the
+// behaviour. Removing a restricting grant from a teammate still widens
+// their access from viewer to editor — see the issue filed against this
+// commit, which records that as a separate, open question rather than
+// letting it disappear behind a notice that now describes it politely.
+const noticeRevokedTeam = "revoked-team"
+
+// revokedTeamText is the sentence when we cannot name the person.
+//
+// Paul's wording, with the name generalised for the fallback below.
+const revokedTeamText = "The explicit permission is removed, but since they are " +
+ "on the team, they still have access."
+
+// stillReaches reports whether somebody can still open a sheet.
+//
+// Asked AFTER the revoke, and asked of roleFor rather than reasoned
+// about separately, so the notice reports what the gate actually says
+// instead of holding a second opinion that can drift from it.
+func (a *App) stillReaches(sheetID, memberID uint64) bool {
+ m, ok := a.memberByID(memberID)
+ if !ok {
+ // The grant is gone and we cannot tell what is left. The plain
+ // sentence is the one that claims less.
+ return false
+ }
+ _, still := a.roleFor(sheetID, m)
+ return still
+}
+
+// revokedTeamNotice names the person the notice is about.
+//
+// Naming them is not decoration: revoking the grant also takes them off
+// the list below, so an unnamed sentence would refer to somebody the
+// owner can no longer see anywhere on the screen.
+//
+// The member ID travels in the query and the address does not, which is
+// the whole reason this reads the row back. An email address in a
+// request line lands in the platform edge's access logs — the same leak
+// the share-link token was moved out of one commit earlier — while an
+// id is already on this screen in every revoke form's hidden field.
+//
+// Checked rather than trusted: the id came from a URL, so the sentence
+// is personalised only for a member this sheet genuinely still admits.
+func (a *App) revokedTeamNotice(r *http.Request, sheetID uint64) string {
+ id, err := strconv.ParseUint(r.URL.Query().Get("who"), 10, 64)
+ if err != nil || id == 0 {
+ return revokedTeamText
+ }
+ m, ok := a.memberByID(id)
+ if !ok || !a.stillReaches(sheetID, m.ID) {
+ return revokedTeamText
+ }
+ return "The explicit permission is removed, but since " + m.Address +
+ " is on the team, they still have access."
+}