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)