Sharing backend: shares CRUD + ACL enforcement everywhere (#16) #35

Merged
steve merged 2 commits from phase-6-sharing-api into main 2026-07-19 03:49:55 +00:00
6 changed files with 30 additions and 18 deletions
Showing only changes of commit 873c591635 - Show all commits
+3
View File
@@ -71,6 +71,9 @@ const (
RoleViewer = "viewer" RoleViewer = "viewer"
RoleEditor = "editor" 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" KindBed = "bed"
KindGrowBag = "grow_bag" KindGrowBag = "grow_bag"
+1 -1
View File
@@ -36,7 +36,7 @@ const (
func (r gardenRole) String() string { func (r gardenRole) String() string {
switch r { switch r {
case roleOwner: case roleOwner:
return "owner" return domain.RoleOwner
Outdated
Review

🟡 "owner" role string duplicated between service String() and store SQL literal; no domain.RoleOwner constant

maintainability · flagged by 2 models

  • internal/service/gardens.go:39"owner" role string is hardcoded in gardenRole.String() while the store SQL at internal/store/gardens.go:78 emits 'owner' AS my_role as a literal. There is no domain.RoleOwner constant (only RoleViewer/RoleEditor). If either side drifts, owned-garden MyRole silently mismatches. Fix: add domain.RoleOwner = "owner" and reference it in both places. - internal/store/shares.go:61ListSharesForGarden inlines the share-column scan ins…

🪰 Gadfly · advisory

🟡 **"owner" role string duplicated between service String() and store SQL literal; no domain.RoleOwner constant** _maintainability · flagged by 2 models_ - **`internal/service/gardens.go:39`** — `"owner"` role string is hardcoded in `gardenRole.String()` while the store SQL at `internal/store/gardens.go:78` emits `'owner' AS my_role` as a literal. There is no `domain.RoleOwner` constant (only `RoleViewer`/`RoleEditor`). If either side drifts, owned-garden `MyRole` silently mismatches. Fix: add `domain.RoleOwner = "owner"` and reference it in both places. - **`internal/store/shares.go:61`** — `ListSharesForGarden` inlines the share-column scan ins… <sub>🪰 Gadfly · advisory</sub>
case roleEditor: case roleEditor:
return domain.RoleEditor return domain.RoleEditor
case roleViewer: case roleViewer:
1
+11 -9
View File
@@ -121,17 +121,19 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64,
return nil, err return nil, err
} }
originalPlantID := pl.PlantID
applyPlantingPatch(pl, patch) applyPlantingPatch(pl, patch)
// The plant must be visible to the actor only when they're CHANGING it — an // Fetch the plop's plant for the derived count. Only when the actor is
// existing plop may reference a plant the actor can't see (e.g. a shared // actually CHANGING the plant (new id ≠ old) is the new plant gated on
// editor working in the owner's garden with the owner's private plant), and // visibility — an existing plop may reference a plant the actor can't see
// editing its radius/label/date must still work. // (e.g. a shared editor in the owner's garden using the owner's private
Outdated
Review

🟠 Plant-visibility re-check triggers on patch field presence, not actual value change, defeating the shared-editor fix for a no-op plantId resend

error-handling, maintainability · flagged by 1 model

  • internal/service/plantings.go:129-133 — The "only re-check plant visibility when the plant is changed" guard checks whether patch.PlantID is present, not whether the value actually differs from the planting's current PlantID: go if patch.PlantID != nil { if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil { return nil, err } } If a caller sends plantId in the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects…

🪰 Gadfly · advisory

🟠 **Plant-visibility re-check triggers on patch field presence, not actual value change, defeating the shared-editor fix for a no-op plantId resend** _error-handling, maintainability · flagged by 1 model_ - **`internal/service/plantings.go:129-133`** — The "only re-check plant visibility when the plant is *changed*" guard checks whether `patch.PlantID` is *present*, not whether the value actually *differs* from the planting's current `PlantID`: ```go if patch.PlantID != nil { if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil { return nil, err } } ``` If a caller sends `plantId` in the PATCH body equal to the plop's existing plant (e.g. a user re-opens a plant picker and re-selects… <sub>🪰 Gadfly · advisory</sub>
if patch.PlantID != nil { // plant), and a no-op plantId resend must not break editing it.
if _, err := s.visiblePlant(ctx, actorID, pl.PlantID); err != nil { var plant *domain.Plant
return nil, err if pl.PlantID != originalPlantID {
} plant, err = s.visiblePlant(ctx, actorID, pl.PlantID)
} else {
Outdated
Review

🟡 Redundant duplicate GetPlant query in UpdatePlanting when the plant is changed

error-handling, performance · flagged by 2 models

  • internal/service/plantings.go:134GetPlant after a plant-id change can return ErrNotFound and surface as a raw 404 where ErrInvalidInput (400) was intended. When patch.PlantID != nil, the code first calls s.visiblePlant(...) which maps a missing/invisible plant to ErrInvalidInput (line 130, confirmed at plantings.go:176-187). But visiblePlant already fetches the plant and returns it — that value is discarded (if _, err := ...), and then line 134 calls `s.store.GetPlan…

🪰 Gadfly · advisory

🟡 **Redundant duplicate GetPlant query in UpdatePlanting when the plant is changed** _error-handling, performance · flagged by 2 models_ - `internal/service/plantings.go:134` — **`GetPlant` after a plant-id change can return `ErrNotFound` and surface as a raw 404 where `ErrInvalidInput` (400) was intended.** When `patch.PlantID != nil`, the code first calls `s.visiblePlant(...)` which maps a missing/invisible plant to `ErrInvalidInput` (line 130, confirmed at `plantings.go:176-187`). But `visiblePlant` already fetches the plant and returns it — that value is discarded (`if _, err := ...`), and then line 134 calls `s.store.GetPlan… <sub>🪰 Gadfly · advisory</sub>
plant, err = s.store.GetPlant(ctx, pl.PlantID)
} }
plant, err := s.store.GetPlant(ctx, pl.PlantID) // for the derived count
if err != nil { if err != nil {
return nil, err return nil, err
} }
+7 -5
View File
2
@@ -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 // RemoveShare revokes a share. The garden owner may remove anyone; a recipient
// may remove themselves ("leave garden"). Any other actor gets the garden's // may remove themselves ("leave garden"). It routes through requireGardenRole
// masked ErrNotFound (existence isn't revealed to non-participants). // (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
Outdated
Review

🔴 RemoveShare bypasses requireGardenRole authorization choke point

maintainability, security · flagged by 5 models

  • RemoveShare bypasses the requireGardenRole authorization choke point, allowing an actor with no role on a garden to reach store.DeleteShare. internal/service/shares.go:66 opens the garden with a raw store.GetGarden instead of funneling through requireGardenRole — the comment on requireGardenRole calls it “THE place authorization for a garden is decided.” A stranger can call RemoveShare with targetUserID == actorID; the ad-hoc check `g.OwnerID != actorID && actorID != targ…

🪰 Gadfly · advisory

🔴 **RemoveShare bypasses requireGardenRole authorization choke point** _maintainability, security · flagged by 5 models_ - **`RemoveShare` bypasses the `requireGardenRole` authorization choke point**, allowing an actor with no role on a garden to reach `store.DeleteShare`. `internal/service/shares.go:66` opens the garden with a raw `store.GetGarden` instead of funneling through `requireGardenRole` — the comment on `requireGardenRole` calls it “THE place authorization for a garden is decided.” A stranger can call `RemoveShare` with `targetUserID == actorID`; the ad-hoc check `g.OwnerID != actorID && actorID != targ… <sub>🪰 Gadfly · advisory</sub>
// share is ErrForbidden — they can already see the garden).
func (s *Service) RemoveShare(ctx context.Context, actorID, gardenID, targetUserID int64) error { 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 { if err != nil {
return err // ErrNotFound return err // ErrNotFound for a non-participant
Outdated
Review

🟡 Owner removing a non-existent share returns ErrNotFound (404) instead of 204; behavior is defensible but undocumented beyond a comment

error-handling, maintainability, security · flagged by 2 models

  • internal/service/shares.go:71Owner removing a non-existent share / self-leave of a non-share returns ErrNotFound instead of 204. In RemoveShare, the guard is g.OwnerID != actorID && actorID != targetUserID (confirmed). The GetGarden lookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through to s.store.DeleteShare (line 76), which returns domain.ErrNotFound when 0 rows are affected (confirmed at `internal/store/sha…

🪰 Gadfly · advisory

🟡 **Owner removing a non-existent share returns ErrNotFound (404) instead of 204; behavior is defensible but undocumented beyond a comment** _error-handling, maintainability, security · flagged by 2 models_ - `internal/service/shares.go:71` — **Owner removing a non-existent share / self-leave of a non-share returns `ErrNotFound` instead of 204.** In `RemoveShare`, the guard is `g.OwnerID != actorID && actorID != targetUserID` (confirmed). The `GetGarden` lookup at line 67 masks the garden from strangers (good). But the owner path and the self-leave path both fall through to `s.store.DeleteShare` (line 76), which returns `domain.ErrNotFound` when 0 rows are affected (confirmed at `internal/store/sha… <sub>🪰 Gadfly · advisory</sub>
} }
if g.OwnerID != actorID && actorID != targetUserID { 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 // Owner path removes any share; self-leave removes the actor's own (a missing
// row is ErrNotFound either way). // row is ErrNotFound either way).
+1 -1
View File
1
@@ -118,7 +118,7 @@ func TestListGardensIncludesSharedWithRole(t *testing.T) {
// Owner sees it as "owner". // Owner sees it as "owner".
own, _ := s.ListGardens(ctx, 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) t.Fatalf("owner list = %+v, want one garden with myRole owner", own)
} }
// other sees nothing yet. // other sees nothing yet.
+7 -2
View File
@@ -40,6 +40,10 @@ func (d *DB) GetShareRole(ctx context.Context, gardenID, userID int64) (role str
return role, true, nil 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
Outdated
Review

🟠 ListSharesForGarden is unbounded — missing LIMIT clause

performance · flagged by 1 model

  • internal/store/shares.go:46ListSharesForGarden queries garden_shares joined with users but has no LIMIT clause, making the result set unbounded. ListGardensForActor (in the same PR area) explicitly caps at maxGardensListed = 1000 as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface. *…

🪰 Gadfly · advisory

🟠 **ListSharesForGarden is unbounded — missing LIMIT clause** _performance · flagged by 1 model_ - **`internal/store/shares.go:46`** — `ListSharesForGarden` queries `garden_shares` joined with `users` but has no `LIMIT` clause, making the result set unbounded. `ListGardensForActor` (in the same PR area) explicitly caps at `maxGardensListed = 1000` as a "defensive backstop against a pathologically large result"; the shares list should have the same defense. Even at normal sharing scales this is unlikely to be hit, but the endpoint should not be the only unbounded list query in the surface. *… <sub>🪰 Gadfly · advisory</sub>
// ListSharesForGarden returns a garden's shares joined with each recipient's // ListSharesForGarden returns a garden's shares joined with each recipient's
// email and display name, for the sharing UI. Always a non-nil slice. // 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) { 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 `SELECT `+qualifyColumns("s", shareColumns)+`, u.email, u.display_name
FROM garden_shares s JOIN users u ON u.id = s.user_id FROM garden_shares s JOIN users u ON u.id = s.user_id
WHERE s.garden_id = ? WHERE s.garden_id = ?
ORDER BY u.display_name COLLATE NOCASE, s.id`, ORDER BY u.display_name COLLATE NOCASE, s.id
gardenID, LIMIT ?`,
gardenID, maxSharesListed,
) )
if err != nil { if err != nil {
return nil, fmt.Errorf("store: list shares: %w", err) return nil, fmt.Errorf("store: list shares: %w", err)
1