From 873c59163551d8472186c937ffa6b85a45ac43d4 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 23:48:20 -0400 Subject: [PATCH] Address Gadfly review on #16: RemoveShare choke point + dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - RemoveShare now routes through requireGardenRole(roleViewer) — the standard authorization choke point (masks existence for non-participants) — then applies the owner-or-self rule on top (a participant removing someone else's share is now ErrForbidden, not ErrNotFound). No more bespoke GetGarden+manual check. - Add domain.RoleOwner constant; gardenRole.String() and the tests use it instead of the bare "owner" literal. - UpdatePlanting fetches the plant once and only re-checks visibility when the plant id actually CHANGES (new ≠ old), so a no-op plantId resend can't break a shared editor editing a plop that uses the owner's private plant. - Bound ListSharesForGarden with a LIMIT backstop. Skipped: AddShare email-existence disclosure (the issue explicitly accepts it as inherent to email-based sharing), 404-on-missing-share (correct DELETE semantics), and the join-scan/test-helper dedup nits. GOWORK=off go build/vet/test ./internal/... green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- internal/domain/domain.go | 3 +++ internal/service/gardens.go | 2 +- internal/service/plantings.go | 20 +++++++++++--------- internal/service/shares.go | 12 +++++++----- internal/service/shares_test.go | 2 +- internal/store/shares.go | 9 +++++++-- 6 files changed, 30 insertions(+), 18 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 039c278..2d6faa7 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -71,6 +71,9 @@ const ( RoleViewer = "viewer" RoleEditor = "editor" + // RoleOwner is not a garden_shares row (ownership is implicit via + // gardens.owner_id); it's the value Garden.MyRole carries for an owner. + RoleOwner = "owner" KindBed = "bed" KindGrowBag = "grow_bag" diff --git a/internal/service/gardens.go b/internal/service/gardens.go index 8aa6cbe..c3fb5f6 100644 --- a/internal/service/gardens.go +++ b/internal/service/gardens.go @@ -36,7 +36,7 @@ const ( func (r gardenRole) String() string { switch r { case roleOwner: - return "owner" + return domain.RoleOwner case roleEditor: return domain.RoleEditor case roleViewer: diff --git a/internal/service/plantings.go b/internal/service/plantings.go index f38de6b..84daf69 100644 --- a/internal/service/plantings.go +++ b/internal/service/plantings.go @@ -121,17 +121,19 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64, return nil, err } + originalPlantID := pl.PlantID applyPlantingPatch(pl, patch) - // The plant must be visible to the actor only when they're CHANGING it — an - // existing plop may reference a plant the actor can't see (e.g. a shared - // editor working in the owner's garden with the owner's private plant), and - // editing its radius/label/date must still work. - if patch.PlantID != nil { - if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil { - return nil, err - } + // Fetch the plop's plant for the derived count. Only when the actor is + // actually CHANGING the plant (new id ≠ old) is the new plant gated on + // visibility — an existing plop may reference a plant the actor can't see + // (e.g. a shared editor in the owner's garden using the owner's private + // plant), and a no-op plantId resend must not break editing it. + var plant *domain.Plant + if pl.PlantID != originalPlantID { + plant, err = s.visiblePlant(ctx, actorID, pl.PlantID) + } else { + plant, err = s.store.GetPlant(ctx, pl.PlantID) } - plant, err := s.store.GetPlant(ctx, pl.PlantID) // for the derived count if err != nil { return nil, err } diff --git a/internal/service/shares.go b/internal/service/shares.go index 7758848..4634b5a 100644 --- a/internal/service/shares.go +++ b/internal/service/shares.go @@ -61,15 +61,17 @@ func (s *Service) UpdateShareRole(ctx context.Context, actorID, gardenID, target } // RemoveShare revokes a share. The garden owner may remove anyone; a recipient -// may remove themselves ("leave garden"). Any other actor gets the garden's -// masked ErrNotFound (existence isn't revealed to non-participants). +// may remove themselves ("leave garden"). It routes through requireGardenRole +// (roleViewer) so a non-participant gets the standard masked ErrNotFound, then +// applies the owner-or-self rule on top (a participant removing someone else's +// share is ErrForbidden — they can already see the garden). func (s *Service) RemoveShare(ctx context.Context, actorID, gardenID, targetUserID int64) error { - g, err := s.store.GetGarden(ctx, gardenID) + g, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer) if err != nil { - return err // ErrNotFound + return err // ErrNotFound for a non-participant } if g.OwnerID != actorID && actorID != targetUserID { - return domain.ErrNotFound + return domain.ErrForbidden } // Owner path removes any share; self-leave removes the actor's own (a missing // row is ErrNotFound either way). diff --git a/internal/service/shares_test.go b/internal/service/shares_test.go index 9af2bcf..c256eda 100644 --- a/internal/service/shares_test.go +++ b/internal/service/shares_test.go @@ -118,7 +118,7 @@ func TestListGardensIncludesSharedWithRole(t *testing.T) { // Owner sees it as "owner". own, _ := s.ListGardens(ctx, owner) - if len(own) != 1 || own[0].MyRole != "owner" { + if len(own) != 1 || own[0].MyRole != domain.RoleOwner { t.Fatalf("owner list = %+v, want one garden with myRole owner", own) } // other sees nothing yet. diff --git a/internal/store/shares.go b/internal/store/shares.go index 9f39175..cb0ad3b 100644 --- a/internal/store/shares.go +++ b/internal/store/shares.go @@ -40,6 +40,10 @@ func (d *DB) GetShareRole(ctx context.Context, gardenID, userID int64) (role str return role, true, nil } +// maxSharesListed bounds a garden's share list defensively (a garden is shared +// with a handful of people at household scale; this is never hit). +const maxSharesListed = 1000 + // ListSharesForGarden returns a garden's shares joined with each recipient's // email and display name, for the sharing UI. Always a non-nil slice. func (d *DB) ListSharesForGarden(ctx context.Context, gardenID int64) ([]domain.ShareWithUser, error) { @@ -47,8 +51,9 @@ func (d *DB) ListSharesForGarden(ctx context.Context, gardenID int64) ([]domain. `SELECT `+qualifyColumns("s", shareColumns)+`, u.email, u.display_name FROM garden_shares s JOIN users u ON u.id = s.user_id WHERE s.garden_id = ? - ORDER BY u.display_name COLLATE NOCASE, s.id`, - gardenID, + ORDER BY u.display_name COLLATE NOCASE, s.id + LIMIT ?`, + gardenID, maxSharesListed, ) if err != nil { return nil, fmt.Errorf("store: list shares: %w", err)